[amrmelsayed]

PIR Review: In-preview typography controls (zoom) for the Codev Markdown Preview

Fixes #1070

Summary

Adds an in-surface zoom control to the Codev Markdown Preview so a reviewer can resize the prose without leaving for the Settings editor. Three commands (increase / decrease / reset font size) are surfaced as native editor/title buttons plus command-palette entries, gated to the preview; they step codev.markdownPreview.fontSize (wired by #1053) and write the value back to the setting, so the control and the Settings editor stay one source of truth, and the existing onDidChangeConfiguration re-render reflows live. Separately, the shared canvas's horizontal-mode column tokens were made em-relative so the reading measure scales with the prose under zoom rather than collapsing to too-narrow.

Files Changed

• apps/vscode/package.json (+41 / -0) — 3 commands, editor/title buttons (+/−) + Reset in the overflow menu, commandPalette entries, all gated activeCustomEditorId == codev.markdownPreview
• apps/vscode/src/markdown-preview/font-size-control.ts (+79 / -0) — new; pure stepping/clamp + write-scope resolution (no vscode import)
• apps/vscode/src/extension.ts (+44 / -0) — command handlers + configTargetFor / stepMarkdownPreviewFontSize
• apps/vscode/src/__tests__/preview-font-size-control.test.ts (+71 / -0) — new; unit tests for the pure logic
• packages/artifact-canvas/src/styles/default-theme.css (+8 / -3) — column-width 400px→25em, column-gap 48px→3em
• packages/artifact-canvas/src/__tests__/default-theme.test.ts (+9 / -0) — locks the em column tokens

Commits

• 029bbeee6 [PIR vscode: in-preview typography controls (zoom) for the Codev Markdown Preview #1070] Surface Reset Font Size in the preview title overflow menu
• 98ad33c54 [PIR vscode: in-preview typography controls (zoom) for the Codev Markdown Preview #1070] Make horizontal-mode column tokens em-relative so measure tracks font size
• 6ddca99bb [PIR vscode: in-preview typography controls (zoom) for the Codev Markdown Preview #1070] Add in-preview font-size zoom: commands + editor-title buttons + write-back

(Plan-revision and thread commits omitted; they touch no shipped code.)

Test Results

• pnpm compile (vscode: check-types + lint + esbuild): ✓ pass (only pre-existing lint warning in tunnel.ts, untouched)
• vscode pnpm test:unit: ✓ 845 pass (72 files) — needed pnpm --filter 'codev-vscode^...' build first to build codev-types/codev-sdk dist in a fresh worktree
• artifact-canvas pnpm test: ✓ 177 pass; pnpm build: ✓ (dist CSS confirmed 25em/3em)
• porch verify: ✓ build + tests
• Manual (human, dev-approval gate): approved after live verification. Builder additionally captured horizontal-mode evidence via Playwright + Chromium against the artifact-canvas example (same --codev-canvas-font-size token the preview injects): cap fallback holds (tall fence/table stay bounded with inner scroll at 24/28px), measured chars/line matched the plan grid exactly at 20/24/28px, and the flagged 1200px·24px over-wide single-column case reproduced as predicted.

Architecture Updates

No arch changes. This adds a UI affordance (commands + title-bar menus) and changes two token values in the shared canvas theme (px→em); it introduces no new module boundary, invariant, port, or state, and the server/client isolation and resolver contracts are untouched. The durable knowledge from this work is design wisdom, routed to lessons (below), not system-shape facts.

Lessons Learned Updates

Two COLD lessons added to codev/resources/lessons-learned.md (UI/UX), both [From #1070]:

1. activeCustomEditorId tracks the active editor, not focus — correct for gating a custom editor's menu visibility, but a trap for scoping a keybinding that shadows a workbench default (it stays true when focus leaves the editor, silently stealing the global key). This is why v1 ships buttons + palette but no keybindings.
2. em-relative CSS multicol does not give a constant measure — it trades which failure mode you get (px→too-narrow under zoom, em→too-wide at the 1-col boundary). Constant measure needs a whole-column container cap (measure = 50 + 6/n by construction), which must be JS-computed and recompute on font-size change, not just resize.

Neither rises to the always-injected HOT tier (both are surface-specific recipes, not repo-wide invariants).

Things to Look At During PR Review

• Write-scope resolution (resolveWriteScope + configTargetFor): the control writes fontSize back to the scope the value already lives in (workspace-folder → workspace → global) so a workspace override can't silently swallow a click. Global is the default for a fresh personal preference. Worth a sanity check that this matches how you'd expect the setting to behave under a .vscode/settings.json override.
• Reset semantics: Reset writes 0 (the documented "use built-in default" sentinel) to both fontSize and lineHeight, so a user who tuned either in Settings gets back to baseline from the surface. steppedFontSize never returns 0 — the sentinel is reset-only.
• The em token change is a deliberate failure-mode preference, not a fix (see the plan and the second lesson). Rhythm scaling (paragraph-spacing/gutter, which carry asserted test baselines) and the whole-column container cap were deliberately deferred, not overlooked — the container cap is the recorded response if the wide 1-column case reads badly in practice.
• Fixtures unaffected by the token change: the three horizontal-mode geometry fixtures mock resolved computed values (getClientRects width, columnGap: '48px'), so they're independent of the stylesheet and needed no edits — verified at implement time.

How to Test Locally

• View diff: VSCode sidebar → right-click builder pir-1070 → Review Diff
• Run dev: VSCode sidebar → Run Dev, or afx dev pir-1070
• What to verify (maps to the plan's Test Plan):
  • Preview title bar shows ⊖ ⊕ buttons; Reset lives in the ⋯ overflow menu and in the palette (all only when the preview is the active editor).
  • +/− reflow live; the value persists across closing/reopening the preview (proves write-back).
  • In horizontal mode at several zoom levels with fences + a wide table + an image in view: tall blocks fall back to inner vertical scroll (checked first — the silent failure mode); column count drops as you zoom; judge the wide-pane 1-column (~1200px·24px) case.
  • cmd+=/cmd+-/cmd+0 still perform workbench zoom everywhere (no keybindings shipped).

[amrmelsayed]

Architect integration review (vscode lane)

Risk tier: medium. The headline +907/-5 across 12 files overstates it. Splitting docs from code:

• Docs/state: ~632 lines — plan (363), builder thread (166), review (72), status.yaml (29), lessons (2).
• Code: ~280 lines across 7 files — font-size-control.ts (86), its test (79), extension.ts (48), package.json (41), default-theme.css (11), canvas token test (9), canvas README (6).

280 lines across 7 files, touching one shared package, is medium by our triage table, not high. Consultation coverage already exceeds what medium requires (3-way: gemini APPROVE, codex APPROVE, claude COMMENT).

Post-consultation fixes — verified independently

Single-pass protocols never re-review the commit that answers the consultation, so I checked all three of c1a075981 against source rather than accepting the summary.

1. Clamp-before-step (font-size-control.ts) — correct, and the builder's read of the reviewer was right on both halves.

The old code clamped only the result. From a stored fontSize: 48 (above MAX = 40):

• increase → next = 49 → clamped to 40. The value shrank on "increase" — the real defect.
• decrease → next = 47 → 47 > MAX → clamped to 40.

So the reviewer's specific claim that decrease "returns 47" was factually wrong; result-clamping already caught it. The builder said so and fixed the genuine issue anyway. Under the new code, clampFontSize(effectiveFontSize(48)) = 40, so increase holds at 40 and decrease steps to 39. Correct, and covered by a new regression test.

Verifying a reviewer's claim against the file instead of implementing it as stated is exactly right.

2. packages/artifact-canvas/README.md — a real doc miss, correctly fixed. It still documented 400px/48px. Now reads 25em/3em and explains the baseline equivalence. Worth noting this is the same defect class as #1454: a stale doc left behind a changed mechanism. Caught by the reviewer, not by us.

3. Write-scope defensive branch — documented, no behaviour change. The comment at the call site is accurate: getConfiguration is called without a resource, so inspect() cannot surface a workspace-folder value. Keeping the branch as unit-tested defensive cover is the right call over deleting a correct pure function.

Architecture fit

• Shared-surface change was routed and approved. Workstream B (packages/artifact-canvas) went to the main architect as stakeholder before the plan gate; approved on a corrected rationale.
• The token change reaches the shipped artifact, not just source — verified 25em/3em in packages/artifact-canvas/dist/*.css.
• No keybindings ship, so no workbench default is shadowed — verified from the manifest (zero contributed bindings use =, - or 0). The activeCustomEditorId gate is correct where it is used, on menus.
• Rhythm scaling deliberately excluded, avoiding the two asserted baselines (paragraph-spacing 16px, gutter 1.9rem). Deferred pending measurement rather than changed speculatively.
• Lessons correctly tiered. Both entries land COLD in lessons-learned.md with an explicit justification for not promoting them to the always-injected HOT tier ("surface-specific recipes, not repo-wide invariants"). That is the right call.

The one judgement that is not mine

At 1200px / 24px the change produces a single ~100-character column. That is the designed trade — em prevents the measure collapsing to too-narrow and pays for it with an over-wide single-column case at that boundary. Measured, predicted, and flagged, with the whole-column container cap (measure = 50 + 6/n, ~52–56ch by construction) recorded as the response if it reads badly. That call belongs to the human at the gate, and the lane is honest about it.

No blocking findings from me. Merge decision rests with the repo owner.