Skip to content

fix(desktop): unify deterministic preview source editing - #440

Merged
Sun-sunshine06 merged 5 commits into
OpenCoworkAI:mainfrom
L4b0R:fix/deterministic-text-coverage
Sep 28, 2026
Merged

Sun-sunshine06 merged 5 commits into
OpenCoworkAI:mainfrom
L4b0R:fix/deterministic-text-coverage

Conversation

@L4b0R

@L4b0R L4b0R commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

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.

  • Resolve same-file lexical const strings/aliases and static object/array paths to exact original string literals, with mutation/escape/reflection checks.
  • Edit independent mixed-child segments and shared component/map literals. Preserve icons, nested elements and unrelated equal-valued definitions; clearly communicate shared-definition scope.
  • Use one inspector and preview field panel. Remove the source picker and explicitly reject retired selectionMode: source requests.
  • Bind individual fields to actual DOM structure, retaining Text-node boundaries and bounded mutation history. Reject ambiguous or replaced text, including same-value dynamic survivors; revalidate the current pinned instance before save.
  • Allow selecting disabled controls, explicitly choosing containing elements, and exiting via Escape without triggering artifact actions.
  • Bind nested HTML-to-JSX/TSX previews to the requested file/design/workspace, refresh unexpanded file-tree sources, and reject stale selections/acknowledgements.
  • Localize editability/source/inspect/save/refresh explanations in Chinese and English; technical diagnostics remain collapsed.

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

  • Bug fix
  • New feature / UX improvement
  • Documentation
  • Breaking change: the explicitly selected legacy source mode is retired; its V1 request remains parseable but returns source-mode-removed. Omitted mode / preview use 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

  • Full-repository pnpm lint: passed (661 files).
  • Full-workspace pnpm -r typecheck: passed.
  • Full-repository 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 nested pnpm to 11.7.0).
  • Targeted desktop engine/IPC/path/preload/renderer suites passed during implementation (521 cases before the subsequent localization and Escape additions).
  • Final localization/FilesTab/Hook/panel subset: 64 passed; i18n package: 15 passed.
  • Runtime package: 198 passed; shared package: 295 passed.
  • Real system Chromium browser suite: 22/22 passed, including Chinese explanations, exact file patches, refresh/new-page reopen, shared references, dynamic refusal, disabled controls, ancestor selection, mutation history, commit-time validation and external source conflicts.
  • Desktop production main/preload/renderer build and 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

  • Component props, map callback parameter instances, computed/state-derived text, dynamic keys and cross-file/imported applications remain unsupported.
  • Shared definitions affect every use; there is no single-instance override.
  • Complex container uses and ambiguous/imperatively changed DOM can be refused conservatively. Unrendered branches cannot be selected.
  • Preview metadata is an untrusted UI hint, never authority for caller-supplied source spans. This is not a proof against arbitrary same-frame JavaScript tampering or future imperative overwrites.
  • Existing external-editor TOCTOU boundary remains documented.

Checklist

  • Reviewed relevant context and reproduced with local generated designs
  • pnpm lint, full-workspace typecheck and pnpm test pass locally
  • Added/updated tests
  • Added a changeset
  • Updated public source-editing documentation
  • Compatibility: same persisted designs and V1 channel, explicit retired-mode refusal
  • Upgradeability: no database or on-disk schema changes
  • Lean: no new shipped dependencies, existing Babel parser reused
  • Elegance: one user-facing editing flow, localized field-level reasons

Screenshots / 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.

@github-actions github-actions Bot added docs Documentation area:desktop apps/desktop (Electron shell, renderer) labels Sep 26, 2026
Comment thread packages/runtime/src/source-edit-instrumentation.test.ts Fixed

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review mode: initial

Findings

  • [Nit] Dead preview direct-text fallback path — streamSourceEdit always emits target.textLayout as an array (apps/desktop/src/main/source-edit-engine.ts, the targets.push({... textLayout: details.textLayout ...}) block), so packages/runtime/src/source-edit-instrumentation.ts always sets fieldPlans for every target (if (target.textLayout && target.editableFields) is always true). Consequently packages/runtime/src/overlay.ts markedFieldPlan()/sourceEditSelection() never take the !fieldPlan directTexts/directText branch, even though directTexts is still populated and SOURCE_EDITING.md describes direct-text validation. This is now an unreachable protection path that can mislead future changes.
    Suggested fix: either drop directText/directTexts and the overlay branch, or keep it explicitly as a legacy-only fallback with a comment plus a test that exercises a context having directTexts but no fieldPlans.

Questions

  • SOURCE_EDITING.md states observer overflow "disables text mapping until reload" (bounded to 4,096 records / 10,000 visited nodes per batch). In the packages/runtime/src/overlay.ts diff, processTextMutations only sets mutationOverflow = true and returns; I could not see mutationOverflow being read anywhere before sourceEditSelection assembles fieldStates (the file is truncated in the provided diff). Can you confirm that a selection made while mutationOverflow === true forces text fields to unmapped/refuses the target? If the flag is never consulted, an overflowed mutation batch would leave stale ready bindings 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 in analyzeSourceEdit, 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-source rejection so files containing effects/timers/globals still expose static literal candidates. Per SOURCE_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 the mutationOverflow guard 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.ts scans 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 the mutationOverflow behavior called out above and a real packaged-Electron/preload E2E run, which the PR body already acknowledges is not covered.

Open-CoDesign Bot

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in packages/runtime/src/source-edit-instrumentation.test.ts: it replaces <script>-block extraction with document.indexOf('textObserver.observe(document.documentElement') plus compiled.offset (the last var 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 in packages/runtime/src/index.ts (the source-edit overlay script is inserted before JSX_TEMPLATE_BEGIN). It does not change product behavior.

Residual observations

  • Prior [Nit] unchanged — the !fieldPlan directTexts fallback in packages/runtime/src/overlay.ts is still unreachable because analyze always emits textLayout, so fieldPlans is 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 of packages/runtime/src/overlay.ts is truncated where fieldStates is assembled, so I cannot confirm that mutationOverflow === true forces text fields to unmapped/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 mutationOverflow fail-closed question above and a packaged-Electron/preload E2E run, both acknowledged in the PR body.

Open-CoDesign Bot

Sun-sunshine06 and others added 2 commits September 28, 2026 19:22
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>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as source-mode-removed in apps/desktop/src/main/source-edit-engine.ts and both IPC handlers in apps/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 the createWebWebResearchRun refactor in apps/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. the if (entry === null) continue; skip in toProviderRows and 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 / directTexts fallback branch in packages/runtime/src/overlay.ts is still unreachable, because analyze always emits textLayout and therefore always populates fieldPlans (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.ts still does not let me confirm that mutationOverflow === true forces text fields to unmapped/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 mutationOverflow fail-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>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: a 15_000 ms timeout added to one case in apps/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:176 that 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 / directTexts fallback in packages/runtime/src/overlay.ts remains unreachable for current analyze output, since fieldPlans is always populated for any target that carries textLayout + 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.ts still does not let me confirm that mutationOverflow === true forces text fields to unmapped/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 as source-mode-removed in apps/desktop/src/main/source-edit-engine.ts and both handlers in apps/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 mutationOverflow fail-closed path.

Open-CoDesign Bot

@Sun-sunshine06
Sun-sunshine06 merged commit 787a8e4 into OpenCoworkAI:main Sep 28, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:desktop apps/desktop (Electron shell, renderer) docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants