[codex] Stabilize local workspace comments#344
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] Missing changeset for user-visible i18n changes. The PR renames button labels from "Send to Claude" to "Add to chat" and "Comment" to "Save comment", and adds new i18n keys (
sendToChat,addedToChat). These are user-facing changes that require a changeset per project convention. Runpnpm changeset, selectpatchfor@open-codesign/i18nand@open-codesign/desktop, and describe the changes. This ensures the changelog and release notes are accurate.
Suggested fix:pnpm changeset
Then commit the generated
.mdfile. -
[Nit] The
findReusablePendingCommentForSelectorandfindReusableWorkspaceFileCommentForSelectorfunctions iterate over thecommentsarray from the end for each element selection. If a design accumulates hundreds of comments, this could become a minor performance bottleneck. Consider storing a map fromsnapshotId+selectorto comment index for O(1) lookups if this becomes noticeable. Not a blocker.
Summary
No blockers, security issues, or data-loss risks found. The PR correctly separates comment saving from sending, adds a queued-comment model, reuses existing pending comments per selector, handles cancellation robustly, and treats missing workspace watcher paths as recoverable without spurious error toasts. Test coverage is excellent across main-process, renderer, and runtime overlay layers. The only actionable issue is the missing changeset.
Testing
- Not run (automation). The PR adds extensive unit tests for store, helpers, and overlay behavior. CI should pass.
Open-CoDesign Bot
Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues introduced in this follow-up. The prior [Minor] finding about the missing changeset has been resolved — .changeset/comment-mode-copy.md is now present with appropriate patch bumps for @open-codesign/desktop and @open-codesign/i18n. All user-facing i18n changes are now covered.
Summary
All material concerns from the initial review are addressed. The PR is directionally sound, well-tested, and ready to merge.
Residual observations
- The
findReusablePendingCommentForSelector/findReusableWorkspaceFileCommentForSelectoriteration over the comments array from the end is still O(n) per selection. As noted in the initial review, this is a Nit-level concern and not a blocker. If comment counts grow significantly in the future, aMap<snapshotId+selector, index>could be added for O(1) lookup, but that is a follow-up optimization. - No new dependencies, no silent fallbacks, no security concerns. All license checks pass per lockfile.
Testing
- Not run (automation). Existing and new unit tests cover the added functionality well. CI should pass.
Open-CoDesign Bot
Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues introduced in this follow-up. The incremental diff is a clean refinement:
workspace-watcher.ts: RenamedisPermissionWatchErrortoshouldFallbackToPollingand addedEISDIRto the fallback condition. This correctly handles cases where a workspace directory cannot be natively watched (e.g., certain Windows mounts) by falling back to polling instead of surfacing an error.workspace-watcher.test.ts: Refactored the polling fallback test into a parameterizedit.eachthat now covers bothEPERMandEISDIRscenarios. Good test hygiene improvement.
All prior concerns remain resolved: the missing changeset was added, the cancellation race condition was fixed, and the comment-mode UX is well-tested.
Residual observations
- The
EISDIRaddition is a minor but sensible edge-case catch. No impact on behavior for normal workflows.
Summary
The PR continues to be directionally sound and well-tested. The incremental changes are safe and improve error resilience. Ready to merge.
Testing
- Not run (automation). Unit tests cover the new polling-fallback paths. CI should pass.
Open-CoDesign Bot
Summary
This PR stabilizes comment mode for local workspace previews and fixes a few related local-workspace edge cases found during manual testing.
What changed
Files / App.jsxpreview.Why
Local workspace comment mode had a few state mismatches between the preview iframe, saved comment rows, and the renderer store. These could leave the UI in stale states: saved comments not appearing in the bubble, duplicated comments on the same selection, deleted comments leaving an orphaned bubble, and saved comments opening without the target element highlighted.
Validation