feat(desktop): comment mode gating by hqhq1025 · Pull Request #75 · OpenCoworkAI/open-codesign

hqhq1025 · 2026-04-19T04:12:39Z

Summary

Fixes the always-on iframe click listener that violated Claude Design's mode-based interaction. Previously every click on a preview element popped the inline comment composer (and preventDefault killed any real link/button inside the design). Now the iframe is click-pass-through by default and only intercepts when the user enters Comment mode from the preview toolbar.

Behaviour

New interactionMode ('default' | 'comment') in the renderer store; defaults to 'default'.
Toolbar gets a MessageSquare Comment button. Clicking toggles the mode and postMessages {__codesign:true, type:'SET_MODE', mode} into the iframe.
Runtime overlay holds a module-local currentMode, gates mouseover/mouseout/click on it, and clears any outline when leaving comment mode. Listener is registered in the same idempotent reattach loop as the existing handlers.
InlineCommentComposer and the comment hint badge only render in comment mode.
ESC priority now: command palette > comment mode > settings view > workspace view.
Switching projects resets interactionMode along with the rest of the workspace state.

Test plan

pnpm typecheck
pnpm test (added 3 store unit tests for default state, set, and selection cleanup on exit)
pnpm lint
Manual smoke: launch app, generate a design, confirm links/buttons inside the iframe behave normally; toggle Comment mode, verify hover outline + click composer; press ESC to exit.

Compatibility / upgradeability / no bloat / elegance

Compatibility: green — no schema or IPC changes; default mode preserves passive viewing for existing flows.
Upgradeability: green — SET_MODE payload tagged with __codesign discriminator, easy to extend with future modes.
No bloat: green — zero new deps, ~130 LOC across overlay/store/UI/i18n.
Elegance: green — single source of truth in the store; overlay reads one variable; UI reflects mode through one toggle.

github-actions

Findings

[Major] Untrusted iframe content can toggle interaction mode via forged postMessage — onParentMessage accepts any message with __codesign/type and does not verify sender, so generated page code can self-post SET_MODE and force comment interception unexpectedly, evidence packages/runtime/src/overlay.ts:79
Suggested fix:

function onParentMessage(ev) {
  if (!ev || ev.source !== window.parent) return;
  var data = ev.data;
  if (!data || data.__codesign !== true || data.type !== 'SET_MODE') return;
  var next = data.mode === 'comment' ? 'comment' : 'default';
  if (next === currentMode) return;
  currentMode = next;
  if (currentMode === 'default') clearHover();
}

[Minor] Hardcoded color in toolbar button breaks token-only UI rule — added class uses literal text-white instead of a design token, evidence apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:166
Suggested fix:

commentActive
  ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]'
  : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

Summary

Review mode: initial
2 issues found in the latest diff (1 Major, 1 Minor).
docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.
Residual risk: no automated coverage added for preview-runtime message trust boundary in this PR.

Testing

Not run (automation)

open-codesign Bot

github-actions · 2026-04-19T05:47:55Z

  }
+  function onParentMessage(ev) {
+    var data = ev && ev.data;
+    if (!data || data.__codesign !== true || data.type !== 'SET_MODE') return;


onParentMessage currently trusts any sender. Please gate on ev.source === window.parent before parsing data so untrusted iframe scripts cannot self-toggle comment mode via forged postMessage.

github-actions · 2026-04-19T05:47:55Z

+          onClick={() => setInteractionMode(commentActive ? 'default' : 'comment')}
+          className={`inline-flex items-center gap-1.5 h-[var(--size-control-xs)] px-3 rounded-[var(--radius-md)] text-[var(--text-sm)] font-medium border transition-[background-color,border-color,color] duration-[var(--duration-fast)] ease-[var(--ease-out)] disabled:opacity-40 disabled:pointer-events-none ${
+            commentActive
+              ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)]'


This introduces a literal color class (text-white). Please switch to a UI token class (for example text-[var(--color-text-on-accent)]) to satisfy the token-only styling constraint.

github-actions

Findings

[Major] Missing sender validation on SET_MODE control message — onParentMessage accepts any message event with the right envelope and does not verify ev.source === window.parent, so untrusted in-iframe scripts can forge control messages and toggle interaction mode. Evidence: packages/runtime/src/overlay.ts:77
Suggested fix:

function onParentMessage(ev) {
  if (!ev || ev.source !== window.parent) return;
  var data = ev.data;
  if (!data || data.__codesign !== true || data.type !== 'SET_MODE') return;
  var next = data.mode === 'comment' ? 'comment' : 'default';
  if (next === currentMode) return;
  currentMode = next;
  if (currentMode === 'default') clearHover();
}

[Minor] Hardcoded text color violates token-only UI constraint — active comment-mode button uses text-white instead of a UI token-backed color. Evidence: apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:166
Suggested fix:

commentActive
  ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]'
  : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

Summary

Review mode: follow-up after new commits
2 issues found in the latest diff (1 Major, 1 Minor).
docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.
Residual risk: no integration test asserts that forged iframe SET_MODE messages are ignored at runtime boundary.

Testing

Not run (automation)

open-codesign Bot

github-actions · 2026-04-19T06:22:25Z

  }
+  function onParentMessage(ev) {
+    var data = ev && ev.data;
+    if (!data || data.__codesign !== true || data.type !== 'SET_MODE') return;


onParentMessage still trusts any message sender. Please gate on ev.source === window.parent before accepting SET_MODE to prevent forged control messages from untrusted iframe code.

Suggested patch:

if (!ev || ev.source !== window.parent) return; const data = ev.data; if (!data || data.__codesign !== true || data.type !== "SET_MODE") return;

github-actions · 2026-04-19T06:22:25Z

+          onClick={() => setInteractionMode(commentActive ? 'default' : 'comment')}
+          className={`inline-flex items-center gap-1.5 h-[var(--size-control-xs)] px-3 rounded-[var(--radius-md)] text-[var(--text-sm)] font-medium border transition-[background-color,border-color,color] duration-[var(--duration-fast)] ease-[var(--ease-out)] disabled:opacity-40 disabled:pointer-events-none ${
+            commentActive
+              ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)]'


This adds a literal color class (text-white), which conflicts with the token-only UI rule. Please switch to a token, e.g. text-[var(--color-text-on-accent)].

Suggested patch:

commentActive ? "border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]" : "border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]"

Codex review on PR #75 flagged: the iframe-side `onParentMessage` handler checks the message envelope (__codesign + type=SET_MODE) but never confirms the event actually originated from window.parent. Untrusted in-iframe scripts can synthesise MessageEvent-shaped objects or bounce events off the iframe itself via window.postMessage(self,...), both of which would slip past the type check. Today only SET_MODE is processed and the worst case is a no-op mode flip, but the structural fix protects every future control type added to the switch. - overlay.ts: reject onParentMessage events whose ev.source !== window.parent - overlay.test.ts: harness captures the registered message + click listeners and asserts a forged-source SET_MODE leaves currentMode unchanged (click still passes through), while the same envelope from window.parent flips into comment mode (click is intercepted)

hqhq1025 · 2026-04-19T07:12:22Z

Addressed Codex review (Major) on the missing sender validation for the SET_MODE control message.

Latest force-push (0865b6b) hardens the iframe-side onParentMessage handler in packages/runtime/src/overlay.ts:

New if (!ev || ev.source !== window.parent) return; guard runs before the __codesign envelope check, so any MessageEvent whose source isn't the embedding parent window is dropped — even if it carries a perfectly shaped SET_MODE payload. This neutralises both synthetic listener invocations from in-iframe scripts and window.postMessage(self, ...) self-bounce attempts.
Origin checking is intentionally not added: the parent renderer runs from file:// (Electron) and the iframe's own origin is opaque (null) under sandbox="allow-scripts", so any string we'd compare to event.origin would either be wrong or a footgun. ev.source === window.parent is the load-bearing check.
Unit coverage in packages/runtime/src/overlay.test.ts now spins up the OVERLAY_SCRIPT in a sandbox, captures the registered message + click listeners, and asserts: forged-source SET_MODE leaves currentMode at default (click is not intercepted), null-source SET_MODE is also rejected, and a SET_MODE whose ev.source === window.parent correctly flips into comment mode.

Branch was rebased onto latest origin/main (which moved with #72/#73/#87/#91/#89/#92) before the new commit was added.

Verified locally:

pnpm --filter @open-codesign/runtime test → 8/8 pass (5 existing + 3 new SET_MODE source-validation tests)
pnpm vitest run src/renderer/src/store.test.ts src/renderer/src/components/PreviewPane.test.ts (desktop scope) → 36/36 pass
The unrelated snapshots-ipc.test.ts failures on this checkout are environmental (better-sqlite3 native module compiled for NODE_MODULE_VERSION 130, runtime is 127 — needs pnpm rebuild better-sqlite3); not touched by this change.

github-actions

Findings

[Major] Silent fallback hides preview control-channel failures — both new postMessage calls catch and ignore errors, which breaks the "no silent fallbacks" constraint and makes mode-sync failures invisible to users. Evidence apps/desktop/src/renderer/src/components/PreviewPane.tsx:126, apps/desktop/src/renderer/src/components/PreviewPane.tsx:186
Suggested fix:
```
} catch (err) {
  pushIframeError(formatIframeError('bridge', 'Failed to sync preview interaction mode'));
  console.warn('[PreviewPane] failed to post SET_MODE', err);
}
```

[Minor] Hardcoded color class violates token-only UI rule — active comment mode uses text-white instead of a UI token-backed value. Evidence apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:181
Suggested fix:

commentActive
  ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]'
  : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

Summary

Review mode: follow-up after new commits
2 issues found in the latest diff (1 Major, 1 Minor).
docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

Not run (automation)

open-codesign Bot

github-actions · 2026-04-19T07:14:12Z

+    if (!win) return;
+    try {
+      win.postMessage({ __codesign: true, type: 'SET_MODE', mode: interactionMode }, '*');
+    } catch {


This catch swallows a newly introduced control-channel failure. Given the project constraint of no silent fallbacks, please surface this (UI error and/or throw with context) instead of ignoring it.

Suggested patch:

} catch (err) { pushIframeError(formatIframeError('bridge', 'Failed to sync preview interaction mode')); console.warn('[PreviewPane] failed to post SET_MODE', err); }

github-actions · 2026-04-19T07:14:12Z

+          onClick={() => setInteractionMode(commentActive ? 'default' : 'comment')}
+          className={`inline-flex items-center gap-1.5 h-[var(--size-control-xs)] px-3 rounded-[var(--radius-md)] text-[var(--text-sm)] font-medium border transition-[background-color,border-color,color] duration-[var(--duration-fast)] ease-[var(--ease-out)] disabled:opacity-40 disabled:pointer-events-none ${
+            commentActive
+              ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)]'


This adds text-white, which violates the token-only UI value rule. Please use a token-backed text color for the active state.

Suggested patch:

commentActive ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]' : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

hqhq1025 · 2026-04-19T07:47:50Z

Addressed the Codex [Major] finding on apps/desktop/src/renderer/src/components/PreviewPane.tsx:126 (and the adjacent onLoad site).

Both SET_MODE postMessage calls used to swallow errors silently, hiding mode-sync failures from the user. Extracted a postModeToPreviewWindow helper that funnels any thrown reason through the existing pushIframeError store sink, so failures appear in the CanvasErrorBar toast surface alongside other iframe errors. Added unit coverage for the throw path, the success path, and the missing-window guard.

Commit: bb442ec

github-actions

Findings

[Minor] Hardcoded color class violates token-only UI rule — active comment state uses text-white, which bypasses packages/ui token values required by project constraints. Evidence apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:181
Suggested fix:

commentActive
  ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]'
  : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

Summary

Review mode: follow-up after new commits
1 issue found in the latest diff (Minor).
docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

Not run (automation)

open-codesign Bot

github-actions · 2026-04-19T07:50:31Z

+          onClick={() => setInteractionMode(commentActive ? 'default' : 'comment')}
+          className={`inline-flex items-center gap-1.5 h-[var(--size-control-xs)] px-3 rounded-[var(--radius-md)] text-[var(--text-sm)] font-medium border transition-[background-color,border-color,color] duration-[var(--duration-fast)] ease-[var(--ease-out)] disabled:opacity-40 disabled:pointer-events-none ${
+            commentActive
+              ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)]'


This introduces text-white, which violates the token-only UI value rule in project constraints. Please switch to a token-backed text color for the active state.

Suggested patch:

commentActive ? 'border-[var(--color-accent)] bg-[var(--color-accent)] text-[var(--color-text-on-accent)] hover:bg-[var(--color-accent-hover)]' : 'border-[var(--color-border)] bg-[var(--color-surface)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] hover:border-[var(--color-border-strong)]'

The runtime overlay used to always intercept hover and click on the iframe. That broke the underlying design (links unclickable, every click popped the comment composer) and contradicted Claude Design's mode-based interaction. Add an explicit Comment mode the user enters from the preview toolbar; until then the iframe behaves normally. - store: interactionMode (default | comment) + setter that clears selectedElement when leaving comment mode - runtime overlay: gate mouseover/mouseout/click on currentMode and accept SET_MODE postMessage from the parent; clear outline on exit - PreviewPane: post SET_MODE on mode change and on iframe load; only render InlineCommentComposer + comment hint in comment mode - PreviewToolbar: Comment mode button (lucide MessageSquare) with active token styling - App: ESC exits comment mode before navigating views - i18n: preview.commentMode + preview.commentModeHint (en + zh-CN) - tests: store interactionMode default, set, and selection cleanup

…SET_MODE) Untrusted iframe content could call window.parent.postMessage with a SET_MODE envelope. Today the listener happens to ignore it, but that is incidental — any future handler additions would silently expose host state to model-generated code. Replace the implicit ignore with an explicit type-allowlist switch (ELEMENT_SELECTED, IFRAME_ERROR) that warns on unknown types. SET_MODE remains parent → iframe only. Vitest covers the trust boundary: a forged SET_MODE is rejected and interactionMode stays unchanged.

Codex review on PR #75 flagged: the iframe-side `onParentMessage` handler checks the message envelope (__codesign + type=SET_MODE) but never confirms the event actually originated from window.parent. Untrusted in-iframe scripts can synthesise MessageEvent-shaped objects or bounce events off the iframe itself via window.postMessage(self,...), both of which would slip past the type check. Today only SET_MODE is processed and the worst case is a no-op mode flip, but the structural fix protects every future control type added to the switch. - overlay.ts: reject onParentMessage events whose ev.source !== window.parent - overlay.test.ts: harness captures the registered message + click listeners and asserts a forged-source SET_MODE leaves currentMode unchanged (click still passes through), while the same envelope from window.parent flips into comment mode (click is intercepted)

Codex flagged the two SET_MODE postMessage sites in PreviewPane for catching and ignoring errors, leaving mode-sync failures invisible (violates the no-silent-fallback rule). Extract a shared helper that forwards the failure reason through the existing pushIframeError sink so the CanvasErrorBar surfaces it.

…mode active state

hqhq1025 · 2026-04-19T08:13:23Z

Addressed Codex minor: replaced text-white with text-[var(--color-on-accent)] on the active comment-mode toggle in PreviewToolbar.tsx. Now uses the canonical "text on accent surface" token (defined in both light and dark themes in packages/ui/src/tokens.css). Also rebased onto origin/main to clear earlier dirty/divergent state. Desktop tests green (305/305).

github-actions

Findings

No high-confidence issues found on added/modified lines in the latest diff.

Summary

Review mode: follow-up after new commits
No new actionable issues identified in this revision.
and : Not found in repo/docs.
Residual risk: no integration test in this PR that validates end-to-end mode toggle + iframe click pass-through behavior in the renderer.

Testing

Not run (automation)

open-codesign Bot

hqhq1025 force-pushed the wt/loop-fix-comment-mode branch from dfb88b0 to e4fa884 Compare April 19, 2026 05:45

github-actions Bot reviewed Apr 19, 2026

View reviewed changes

hqhq1025 force-pushed the wt/loop-fix-comment-mode branch from e07c92c to 0865b6b Compare April 19, 2026 07:12

github-actions Bot reviewed Apr 19, 2026

View reviewed changes

hqhq1025 added 5 commits April 19, 2026 16:06

fix(desktop): use token-based color instead of text-white in comment …

1cbacf4

…mode active state

hqhq1025 force-pushed the wt/loop-fix-comment-mode branch from bb442ec to 1cbacf4 Compare April 19, 2026 08:13

github-actions Bot reviewed Apr 19, 2026

View reviewed changes

hqhq1025 merged commit 62ef6aa into main Apr 19, 2026
5 of 6 checks passed

hqhq1025 deleted the wt/loop-fix-comment-mode branch April 19, 2026 08:19

Conversation

hqhq1025 commented Apr 19, 2026

Summary

Behaviour

Test plan

Compatibility / upgradeability / no bloat / elegance

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

hqhq1025 commented Apr 19, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

hqhq1025 commented Apr 19, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

hqhq1025 commented Apr 19, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant