Conversation
c0386bc to
5f683a5
Compare
There was a problem hiding this comment.
Findings
-
[Major] Inline comments are silently disabled when zoom is not 100% — this is a behavior regression without user-visible explanation, so annotation appears to stop working in zoomed views (
apps/desktop/src/renderer/src/components/PreviewPane.tsx:140,apps/desktop/src/renderer/src/components/PreviewPane.tsx:157,apps/desktop/src/renderer/src/components/PreviewPane.tsx:175).
Suggested fix:const inlineCommentDisabled = previewZoom !== 100; {inlineCommentDisabled && ( <output className="...">{t('preview.inlineCommentsDisabledAtZoom')}</output> )} <InlineCommentComposer />
-
[Major]
previewZoomis not validated before being used as a divisor —10000 / previewZoomcan produce invalid dimensions (Infinity%/NaN%) and break preview layout if any caller sets0or a non-finite value (apps/desktop/src/renderer/src/components/PreviewPane.tsx:117,apps/desktop/src/renderer/src/store.ts:685).
Suggested fix:const ZOOM_OPTIONS = [50, 75, 90, 100, 110, 125, 150, 175, 200] as const; setPreviewZoom(zoom) { const safe = ZOOM_OPTIONS.includes(zoom as (typeof ZOOM_OPTIONS)[number]) ? zoom : 100; set({ previewZoom: safe }); }
-
[Minor] Hardcoded UI size token bypass (
min-w-[6rem]) violates the UI-token-only constraint and creates an untracked spacing primitive (apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:182).
Suggested fix:className="... min-w-[var(--size-dropdown-min)] ..."
Summary
- Review mode: initial
- 3 issues found in added/modified lines: one behavioral regression (silent inline-comment disable at non-100 zoom), one zoom-state correctness guard missing, and one UI token constraint violation.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.mdin this checkout.
Testing
- Not run (automation)
- Suggested additions: component tests for zoom dropdown interaction, preview scaling safety on invalid zoom input, and inline comment behavior at non-100 zoom.
open-codesign Bot
| <div className={COMMENT_HINT_CLASS}>{t('preview.clickToComment')}</div> | ||
| <PhoneFrame>{iframe}</PhoneFrame> | ||
| <InlineCommentComposer /> | ||
| {previewZoom === 100 && <InlineCommentComposer />} |
There was a problem hiding this comment.
[Major] Inline comments are hidden whenever zoom is not 100%, with no in-UI explanation. This is a silent behavior regression for annotation in zoomed preview states.
Suggested fix:
const inlineCommentDisabled = previewZoom !== 100;
{inlineCommentDisabled && <output className="...">{t(preview.inlineCommentsDisabledAtZoom)}</output>}
<InlineCommentComposer />| set({ selectedElement: null }); | ||
| }, | ||
|
|
||
| setPreviewZoom(zoom) { |
There was a problem hiding this comment.
[Major] setPreviewZoom accepts any number; downstream code divides by previewZoom, so 0/non-finite input can yield invalid dimensions and broken layout.
Suggested fix:
const ZOOM_OPTIONS = [50, 75, 90, 100, 110, 125, 150, 175, 200] as const;
setPreviewZoom(zoom) {
const safe = ZOOM_OPTIONS.includes(zoom as (typeof ZOOM_OPTIONS)[number]) ? zoom : 100;
set({ previewZoom: safe });
}| {zoomOpen && ( | ||
| <div | ||
| role="menu" | ||
| className="absolute right-0 top-full mt-2 min-w-[6rem] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10" |
There was a problem hiding this comment.
[Minor] min-w-[6rem] introduces a hardcoded UI value instead of using design tokens.
Suggested fix:
className="... min-w-[var(--size-dropdown-min)] ..."There was a problem hiding this comment.
Findings
- [Major] Hardcoded width value bypasses the design-token requirement —
min-w-[6rem]introduces a raw UI size outsidepackages/uitokens, violating the project’s hard constraint for tokenized UI values, evidenceapps/desktop/src/renderer/src/components/PreviewToolbar.tsx:182.
Suggested fix:className="absolute right-0 top-full mt-2 min-w-[var(--size-stage-min)] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10"
Summary
- Review mode: follow-up after new commits
- 1 issue found in added/modified lines.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested tests: add a component test that opens the zoom menu and asserts the token-backed width class is present.
open-codesign Bot
| {zoomOpen && ( | ||
| <div | ||
| role="menu" | ||
| className="absolute right-0 top-full mt-2 min-w-[6rem] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10" |
There was a problem hiding this comment.
[Major] min-w-[6rem] is a hardcoded UI size and violates the token-only UI constraint. Please switch this to an existing token-backed value (for example min-w-[var(--size-stage-min)]) so sizing stays centralized and consistent.
Adds a zoom selector to the preview toolbar with steps 50/75/90/100/110/125/150/175/200%. Applied via CSS transform on the iframe wrapper so the artifact's own layout is untouched. Default is 100%. Inline-comment composer is hidden when zoom != 100% to avoid stale rect math.
Previously the InlineCommentComposer was silently gated behind `previewZoom === 100`, so users at 75% / 50% etc. would click an element and get no feedback or composer — a hidden regression. Render the composer at every zoom level and scale the rect reported from inside the iframe into parent viewport coords (multiply by zoom/100) so any consumer of selectedElement.rect sees positions consistent with what the user actually sees on screen.
9dae8ec to
bcf447c
Compare
There was a problem hiding this comment.
Findings
- [Major] Hardcoded width value violates the token-only UI constraint —
min-w-[6rem]introduces a raw UI size outside shared tokens, which conflicts with project constraints, evidenceapps/desktop/src/renderer/src/components/PreviewToolbar.tsx:182.
Suggested fix:className="absolute right-0 top-full mt-2 min-w-[var(--size-stage-min)] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10"
Summary
- Review mode: follow-up after new commits
- 1 issue found in added/modified lines.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested tests: add a component test asserting the zoom menu uses a token-backed min-width class.
open-codesign Bot
| {zoomOpen && ( | ||
| <div | ||
| role="menu" | ||
| className="absolute right-0 top-full mt-2 min-w-[6rem] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10" |
There was a problem hiding this comment.
[Major] min-w-[6rem] is a hardcoded UI size and violates the token-only UI constraint. Use a token-backed value instead, e.g. min-w-[var(--size-stage-min)].
Suggested fix:
className="absolute right-0 top-full mt-2 min-w-[var(--size-stage-min)] rounded-[var(--radius-md)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)] py-1 z-10"Codex flagged min-w-[6rem] in PreviewToolbar zoom dropdown as a raw UI value. Add --size-menu-compact (6rem) to packages/ui tokens and reference it from the dropdown so the constraint of token-only UI values holds.
There was a problem hiding this comment.
Findings
- No high-confidence issues found in added/modified lines.
Summary
- Review mode: follow-up after new commits
- No issues found in the latest added/modified lines.
- Residual risk: zoom behavior is covered by unit tests for math/state, but there is still no UI-level test validating dropdown interaction and selection-to-comment flow across non-100% zoom and each viewport mode.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested tests: add a renderer component test for zoom dropdown open/select/close behavior and a Playwright path that clicks an element at 75%/200% zoom then applies an inline comment.
open-codesign Bot
Summary
previewZoom+setPreviewZoominto the Zustand store;PreviewPaneapplies the zoom viatransform: scale()on the iframe wrapper (with inverse width/height so the layout box still fills the viewport).i18n: added
preview.zoom(en + zh-CN). Option labels are simple\${n}%strings.Test plan
pnpm typecheckpnpm test(added unit tests: default 100, set to 150 updates store)pnpm lint(no new errors; pre-existing warnings unchanged)PRINCIPLES §5b