Conversation
|
🤖
|
📦 npm preview published —
|
|
🤖 |
pat-lewczuk
left a comment
There was a problem hiding this comment.
🤖 om-auto-review-pr — Code review
Verdict: 🔴 CHANGES REQUESTED — 1 major, 1 minor, 3 nits. No blockers.
The change itself is clean, small and well-commented, and the token-level approach (one --measure lever, default = absence of the attribute) is exactly right. What it does not account for is that width: 'narrow' was persisted for users who never chose it — so the "full width by default" fix will not reach a large share of existing installs.
🔴 Major
1. Existing installs keep the 820px column, because narrow was persisted without ever being chosen — packages/web/src/lib/appearance.ts:35, packages/web/src/components/appearance-provider.tsx:73-80
AppearanceProvider.save() PUTs the full appearance object on every change (deliberately — the server merges shallowly, so a partial { accent } would drop density). Before this PR the default width was narrow, so:
- Any user who ever changed Accent or Density has
appearance.width: "narrow"written into~/.cezar/ui-state.json— a value they never picked. The diff's own test shows it:settings.test.tsx:300asserts a PUT of{ accent: 'violet', density: 'compact', width: 'narrow' }from an accent+density interaction. appearanceSchema(packages/cezar/src/server/server.ts:678) storeswidthverbatim, andnormalizeWidthhonors a storednarrow.
Net effect after this ships: every user who has ever opened Settings → Appearance and touched anything stays on the old 820px measure and never sees the fix. The PR body's compatibility note — "Existing narrow preferences remain honored" — reads as if those were deliberate choices; for this population they were not.
The PR is a bug fix whose whole point is the default. Please make this an explicit decision rather than an implicit one, either:
- (a) add a workspace migration (
packages/cezar/src/workspace/migrations.ts) that deletesappearance.widthwhen it equals"narrow", accepting that the handful of users who genuinely chose Narrow get reset once and re-pick it — the migration framework is already the sanctioned reshaping path (BACKWARD_COMPATIBILITY.md§9) and is ordered/idempotent/non-blocking; or - (b) keep the current behavior and say so honestly in the PR body and the
Widthdoc comment: "the new default only reaches installs that never wrote an appearance preference; everyone else opts in via Settings → Appearance."
Either is defensible — (a) delivers the fix, (b) is a smaller change. What is not fine is shipping it as if narrow on disk always meant intent.
🟡 Minor
2. :root[data-width='wide'] is unreachable, and its comment names a path that does not exist — packages/web/src/styles/index.css:256-258
Nothing in the new code can stamp data-width="wide": applyAppearance() removes the attribute for the default (now wide), and the pre-paint script in index.html:39-40 only stamps narrow. The comment says the rule is kept "as a broad-mode alias for older mirrors that stamped it" — but older mirrors are read by the new pre-paint script, which never writes wide. The only path that could still stamp it is a stale cached index.html shell from before this change (the shell is served without an explicit Cache-Control — server.ts:1407).
That is a legitimate reason to keep the rule, but the comment should name it (stale cached shell, not old mirrors) — or the rule should go. As written the next reader will delete it for the wrong reason or keep it for the wrong reason.
⚪ Nits
3. One-time narrow→full flash on the first load after upgrade — packages/web/index.html:39
writeStoredAppearance() runs on every server response, so every existing browser currently holds cez-width: "narrow" in its localStorage mirror (written from the old default, even for users with no server-side preference). After upgrade, the new pre-paint script reads that mirror, stamps data-width="narrow", and the cockpit paints at 820px until the GET /api/v1/workspace/ui-state lands and the provider clears it. Self-healing — the mirror is rewritten to wide on that same load, so only the first load flashes. Worth a sentence in the PR body so it is not mistaken for a bug report later.
4. Stale spec — .ai/specs/2026-07-27-publishable-react-components.md:708
The built-in
--cezar-measurevalue is820pxfornarrowand1180pxforwide.
wide is no longer 1180px. The --cezar-measure public token is not implemented yet, so nothing breaks — but the spec is now wrong about the value a future implementation would ship.
5. The wide enum value no longer means "wide" — packages/web/src/lib/appearance.ts:31
Keeping the stored enum as narrow | wide while the UI says "Full" is the right compatibility call, and the type comment does explain it. Consider one more clause at the type site noting that wide is a legacy spelling for the uncapped mode, so nobody later "fixes" the label mismatch by renaming the persisted value.
✅ Validation gate
Run on the PR head (d452393) in an isolated worktree, TMPDIR outside the repo as the PR body advises:
| Command | Result |
|---|---|
npm run typecheck |
✅ pass |
npm test |
✅ pass — 327 files / 6196 tests |
npm run test:unit |
✅ pass — 36/36 |
npm run build |
✅ pass — check:pack ok, 475 files |
npm run test:package |
✅ pass — 16/16 |
A first npm test run failed 3 files on waitFor timeouts (task-changes.test.tsx:187 and two others) while the machine was still loaded from npm ci; a clean re-run was fully green, and CI is green on the same SHA. Environmental flake, not a PR defect.
CI: all 3 checks passing (Unit, build, E2E, and package, Publish npm snapshot, license/cla). No merge conflicts; the branch is behind main but merges cleanly.
Not covered here: real-browser E2E — this is a visible layout change on a needs-qa PR, so the uncapped measure on a wide display still wants a manual look (especially the transcript line length on ultra-wide monitors, which is the readability cost the 820px measure was originally buying).
Checked and fine
max-width: noneis valid for all 5max-w-[var(--measure)]consumers (task-thread ×2, run-header, task-commits ×2); nocalc()consumer needs a length.- Enum, schema, and
/api/v1/workspace/ui-stateround-trip are unchanged — additive-only contract honored, noBACKWARD_COMPATIBILITY.mdsurface broken. - Tests were updated alongside the behavior change and both
applyAppearancebranches stay covered. - No new dependencies, no
any, no diff-level auto-detections triggered.
|
🤖
|
|
@matgren, the full-width default will not actually reach existing installs: |
|
🤖 Full validation gate green on autofix: skipped (not my PR — re-run with --autofix to fix it here). |
|
🤖 |
QA ReportResultPASS PR #976 ( Environment
Scenario
NotesThis was an evidence-only run. The browser used a dry-run task fixture and the DOM measurements above; no source files were modified by QA and no QA: PASS Inline screenshots |
🧪 Follow-up: add a UI/integration testAutomate this verified scenario with |
|
🤖 |
# Conflicts: # BACKWARD_COMPATIBILITY.md
|
🤖 ✅ Fixed the persisted legacy |





🎯 What changes
Task detail pages no longer default to the old 820px reading column. The shared
--measuretoken now defaults to full available width, so the run header, session transcript, and composer use the screen instead of leaving a large unused gutter on wide displays. Users who prefer the old reading measure can still chooseNarrowin Settings → Appearance.📋 Scope
This is limited to the cockpit appearance/layout path. The stored
widthenum remainsnarrow | widefor compatibility, but the default flips towide;data-width="narrow"now opts into the 820px cap. The pre-paint localStorage mirror and Appearance settings copy were updated to match, and the existing preference round-trip tests now assert the new default.💥 Compatibility
Existing
widepreferences keep working as the broad mode. Migration 002 removesappearance.width: "narrow"from workspace state once, because older full-object writes persisted the default without proving that Narrow was chosen; users who want Narrow can select it again after upgrading. The rollback surface is the default only: switching the default back would return new/cleared cockpit state to the narrow column. The first load after upgrade can briefly paint the old narrow mirror before the migration response rewrites it.🧪 Validation
npm run typecheckTMPDIR=/tmp TMP=/tmp npm testTMPDIR=/tmp TMP=/tmp npm run test:unitTMPDIR=/tmp TMP=/tmp npm run buildTMPDIR=/tmp TMP=/tmp npm run test:packageThe first plain
npm testrun failed because this Cezar task environment setsTMPDIRinside the repository, so tests that intentionally create non-git temp directories were still under a git checkout. Rerunning with temp fixtures outside the repo passed all 327 test files / 6196 tests.