fix(desktop): unify deterministic preview source editing - #440
Merged
Sun-sunshine06 merged 5 commits intoSep 28, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Review mode: initial
Findings
- [Nit] Dead preview direct-text fallback path —
streamSourceEditalways emitstarget.textLayoutas an array (apps/desktop/src/main/source-edit-engine.ts, thetargets.push({... textLayout: details.textLayout ...})block), sopackages/runtime/src/source-edit-instrumentation.tsalways setsfieldPlansfor every target (if (target.textLayout && target.editableFields)is always true). Consequentlypackages/runtime/src/overlay.tsmarkedFieldPlan()/sourceEditSelection()never take the!fieldPlandirectTexts/directTextbranch, even thoughdirectTextsis still populated andSOURCE_EDITING.mddescribes direct-text validation. This is now an unreachable protection path that can mislead future changes.
Suggested fix: either dropdirectText/directTextsand the overlay branch, or keep it explicitly as a legacy-only fallback with a comment plus a test that exercises a context havingdirectTextsbut nofieldPlans.
Questions
SOURCE_EDITING.mdstates observer overflow "disables text mapping until reload" (bounded to 4,096 records / 10,000 visited nodes per batch). In thepackages/runtime/src/overlay.tsdiff,processTextMutationsonly setsmutationOverflow = trueand returns; I could not seemutationOverflowbeing read anywhere beforesourceEditSelectionassemblesfieldStates(the file is truncated in the provided diff). Can you confirm that a selection made whilemutationOverflow === trueforces text fields tounmapped/refuses the target? If the flag is never consulted, an overflowed mutation batch would leave stalereadybindings and could allow a save for a field whose rendered text no longer matches its source — the opposite of the documented fail-closed behavior.
Summary
- Review mode: initial
- No Blockers or Majors found. The diff is large but internally consistent: the retired
selectionMode: 'source'path is explicitly rejected inanalyzeSourceEdit,planSourceEdit, and both IPC handlers (source-mode-removed), the write path stays a deterministic single-span AST patch, and no new shipped dependency or direct provider SDK import is introduced (reuses@babel/parser). - Linked issue: the PR body states there is no linked public issue, so there is no closure claim to validate. The stated scope (unified preview-only editing, lexical provenance, per-field preview binding) matches the changed code paths and tests.
- Residual risk (documented, not a defect): the PR intentionally relaxes the prior blanket
unsafe-sourcerejection so files containing effects/timers/globals still expose static literal candidates. PerSOURCE_EDITING.md, preview metadata is an advisory UI hint and main re-resolves the target independently, so the only new exposure is against the user's own workspace source — but the same-frame preview validation (and themutationOverflowguard in the question above) is the sole barrier for the "displayed text diverged from source" case, so it is worth confirming it fails closed. - i18n coverage is well guarded:
apps/desktop/src/renderer/src/preview/source-edit-messages.test.tsscans the backend sources for emitted reason codes and asserts a localized key exists for each, which mitigates the risk of untranslated refusals.
Testing
- Not run (automation). The PR adds substantial Vitest coverage (
source-edit-provenance.test.ts,source-edit-engine.text-coverage.test.ts,source-edit-binding.test.ts,overlay.test.ts, IPC and renderer suites) plus a Chromium browser suite, and a changeset. Remaining verification gap is themutationOverflowbehavior called out above and a real packaged-Electron/preload E2E run, which the PR body already acknowledges is not covered.
Open-CoDesign Bot
Contributor
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
- No new findings. The delta since the previous review (
6aea2fa→552fdb1) is a single test-only change inpackages/runtime/src/source-edit-instrumentation.test.ts: it replaces<script>-block extraction withdocument.indexOf('textObserver.observe(document.documentElement')pluscompiled.offset(the lastvar source = …match index) to assert the source-edit observer runs before compiled user code. This is a more direct, less fragile assertion and it matches the generator ordering inpackages/runtime/src/index.ts(the source-edit overlay script is inserted beforeJSX_TEMPLATE_BEGIN). It does not change product behavior.
Residual observations
- Prior [Nit] unchanged — the
!fieldPlandirectTextsfallback inpackages/runtime/src/overlay.tsis still unreachable becauseanalyzealways emitstextLayout, sofieldPlansis always populated (packages/runtime/src/source-edit-instrumentation.ts). Not made worse by this delta; optional cleanup only (drop the branch, or document it as legacy-only with a context-level test). - Prior question unchanged — the new commits do not touch
processTextMutations/sourceEditSelection, and the public diff view ofpackages/runtime/src/overlay.tsis truncated wherefieldStatesis assembled, so I cannot confirm thatmutationOverflow === trueforces text fields tounmapped/refuses the target. Already raised; not re-filed as a finding.
Summary
- Review mode: follow-up after new commits
- No Blockers, Majors, or Minors found. The retired
selectionMode: 'source'path remains explicitly rejected (source-mode-removed) in the engine and both IPC handlers, the deterministic single-span write path is unchanged, and this delta is test-only. - No linked public issue claim to validate (PR body states none). No new shipped dependency or direct provider SDK import.
- Merge readiness: directionally sound; the remaining items are non-blocking polish/questions already raised, not merge blockers.
Testing
- Not run (automation). Remaining verification gap: the
mutationOverflowfail-closed question above and a packaged-Electron/preload E2E run, both acknowledged in the PR body.
Open-CoDesign Bot
Integrate main 143cf96 into PR OpenCoworkAI#440 without rewriting the original branch history. Preserve both source-edit and web-search translations through the normal three-way merge. Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Co-Authored-By: Claude Code <noreply@anthropic.com>
Inject the runtime mutation in the form submit capture phase so live preview invalidation cannot disable Save before the validation test submits. Keep a held-pointer regression stimulus across the overlay refresh interval and retain the existing real validation and no-overwrite assertions. Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Co-Authored-By: Claude Code <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
- No new Blockers, Majors, or Minors found. In the source-editing path this run is directionally unchanged from the previously reviewed state: the retired
selectionMode: 'source'value is still explicitly rejected assource-mode-removedinapps/desktop/src/main/source-edit-engine.tsand both IPC handlers inapps/desktop/src/main/source-edits-ipc.ts, and the deterministic single-span write path (hash check, path safety, writer leases, reparse-before-commit) is untouched. - No new shipped dependency and no direct provider SDK import in the source-editing diff; the executor still routes LLM calls through the existing layer.
Questions
- The compare diff for this run (
552fdb1→6c762ed) is dominated by files that are not in this PR's changed-file set (changedFiles: 34, all source-editing):.changeset/tavily-settings-entry.md,WEB_SEARCH.md,apps/desktop/src/main/web-search-settings.*,apps/desktop/src/main/onboarding/register.ts,apps/desktop/src/main/provider-settings.ts, and thecreateWebWebResearchRunrefactor inapps/desktop/src/main/ipc/generate.ts. These look like main-line Tavily web-search work pulled in by a rebase/merge rather than this PR's contribution. Can you confirm they are not intended to ship in this branch? If they are, they belong in a separate PR/changeset with their own review (e.g. theif (entry === null) continue;skip intoProviderRowsand the new search-settings IPC surface would need independent review), and keeping them here conflicts with the PR's stated scope.
Residual observations
- Prior [Nit] unchanged — the
!fieldPlan/directTextsfallback branch inpackages/runtime/src/overlay.tsis still unreachable, becauseanalyzealways emitstextLayoutand therefore always populatesfieldPlans(packages/runtime/src/source-edit-instrumentation.ts). Not made worse by this delta; optional cleanup only (drop the branch, or document it as legacy-only with a context-level test). - Prior question unchanged — the truncated public view of
packages/runtime/src/overlay.tsstill does not let me confirm thatmutationOverflow === trueforces text fields tounmapped/refuses the target. Not re-filed as a finding.
Summary
- Review mode: follow-up after new commits
- The source-editing delta carries no new material issues; the previous review's findings remain non-blocking polish. There is no linked public issue closure claim to validate (PR body states none).
- Merge readiness: the source-editing change is directionally sound. The one thing to resolve before merge is scope hygiene: confirm the Tavily/web-search files in the compare diff are main-line artifacts and not part of this branch.
Testing
- Not run (automation). Remaining verification gaps: the
mutationOverflowfail-closed behavior noted above and a packaged-Electron/preload E2E run, both already acknowledged in the PR body.
Open-CoDesign Bot
Use the existing 15-second registration-test budget for the first search settings case, which cold-imports the onboarding IPC graph. CI run 36422390415 exceeded its previous default five-second budget. Keep all assertions, production code, browser tests, and global timeouts unchanged. Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Co-Authored-By: Claude Code <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
- No Blockers, Majors, or Minors found. The delta since the prior review (
6c762ed→bf20818) is a single test-only change: a15_000ms timeout added to one case inapps/desktop/src/main/onboarding-ipc.test.ts:176. This is a benign flake mitigation and does not touch the source-editing runtime path, IPC contracts, or the deterministic writer.
Residual observations
- [Nit] Scope hygiene (carried over from the previous review, and now the entire delta): the PR still carries a test-timeout tweak in
apps/desktop/src/main/onboarding-ipc.test.ts:176that is unrelated to source editing. Harmless, but it could ship in its own change if you want this branch scoped strictly to source editing. Not a merge blocker. - [Nit] Unchanged from the prior review — the
!fieldPlan/directTextsfallback inpackages/runtime/src/overlay.tsremains unreachable for currentanalyzeoutput, sincefieldPlansis always populated for any target that carriestextLayout+editableFields(packages/runtime/src/source-edit-instrumentation.ts). Optional cleanup only (drop the branch, or document it as legacy-only with a context-level test). - Prior open question — the truncated public view of
packages/runtime/src/overlay.tsstill does not let me confirm thatmutationOverflow === trueforces text fields tounmapped/refuses the target. Not re-filed as a finding.
Summary
- Review mode: follow-up after new commits
- No new material issues. The retired
selectionMode: 'source'path is still explicitly rejected assource-mode-removedinapps/desktop/src/main/source-edit-engine.tsand both handlers inapps/desktop/src/main/source-edits-ipc.ts; main-process re-resolution, hash/path checks, writer leases, and the atomic single-span write remain intact. - A changeset is present (
.changeset/trace-generated-text-definitions.md) and covers the user-visible packages (desktop,runtime,shared,i18n), so the missing-changeset concern does not apply. - No linked public issue closure claim to validate (PR body states none).
- The source-editing change is ready to merge; the only open item is the optional scope nit above, which is not a blocker.
Testing
- Not run (automation). Remaining gaps already acknowledged in the PR body: packaged Electron/preload E2E and the
mutationOverflowfail-closed path.
Open-CoDesign Bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix deterministic editing coverage for generated JSX/TSX and replace the confusing preview/source selection split with one preview-selection workflow. Previously, six locally generated designs were rejected by the preview inspector despite containing editable static definitions. The old inspector required narrow App ownership, sole literal children and globally restricted execution; nested resolved-source identity also caused stale/missing previews.
conststrings/aliases and static object/array paths to exact original string literals, with mutation/escape/reflection checks.selectionMode: sourcerequests.Writes remain deterministic single-span AST patches to real workspace files, without an LLM, generated-code evaluation for provenance, broad string replacement or DOM-only persistence. Existing path checks, source hashes, writer leases and atomic commit protections are retained.
Type of change
sourcemode is retired; its V1 request remains parseable but returnssource-mode-removed. Omitted mode /previewuse the unified flow. No on-disk schema migration.Linked issue
No linked public issue; based on actual generated-design reproduction and user feedback. Private design originals were read-only and are not included. Portable fixtures cover their representative structures.
Validation
pnpm lint: passed (661 files).pnpm -r typecheck: passed.pnpm test: passed, including script tests and all 10 Turbo test tasks. Desktop: 170 suites, 2,498 passed / 1 skipped. Ran with Corepack pnpm 10.33.4 and its shims first on PATH (the host otherwise resolves nestedpnpmto 11.7.0).git diff --check: passed.The browser harness uses installed Chromium and an HTTP bridge invoking production IPC handlers and the atomic writer. This is not packaged Electron/preload E2E. No browser or runtime dependency is added. Screenshots were captured locally but not manually visually reviewed; real-pointer, DOM/layout/accessibility and filesystem assertions were exercised.
Remaining boundaries
Checklist
pnpm lint, full-workspace typecheck andpnpm testpass locallyScreenshots / recordings
No public screenshots attached: local evidence includes generated-design content. Reviewable automated examples live in
SourceEditPanel.browser.test.ts; screenshots and DOM logs are produced in the ignored local validation directory.