Repository navigation
fix(desktop): allow unchanged edit-and-resend - #5815
Conversation
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I found one P2 at exact head a5e62e3a2170197e2b77488e2c24a31ea8e8d02a.
P2: add a regression through the real renderer submit path for unchanged edit-and-resend. The production fix is the removal of the early return that used to sit before prepareRevisionSend() in apps/desktop/src/renderer/app-shell.tsx; however, the current revision tests only invoke prepareRevisionSend() directly (apps/desktop/src/main/__tests__/app-shell-revision-actions.test.ts:160-225,301-315). They therefore never execute the guard that caused the bug. I built and ran the same six tests on both this head and its exact base 2f32205528a4436783c800715261d4b6e286023d, where the unchanged-text rejection is still present, and both runs pass 6/6. Hosted test is green for the same reason.
Please add a mounted renderer/integration regression that enters edit mode for a sent user turn, leaves the composer text unchanged, submits through the actual composer callback, and asserts that reviseBeforeTurn plus the normal send reach the prepared child Session. A helper-only assertion that bypasses sendWithAttachments() would not distinguish the old behavior.
The implementation itself is narrowly scoped and the removed originalText state has no remaining consumers. On Node 24.18.1, npm ci, npm run build:test, Desktop typecheck, renderer architecture, lint, format, git diff --check, and the six revision-action tests pass. Current main is 03237142ddbb8145c80ec541f54f6292cbe714a9; the PR is 2 ahead / 2 behind and the merge-tree is clean (35945bcc1cb0bde668ee20eab44247e405809bf1). I did not run a packaged Electron/manual UI journey or native Windows/macOS validation.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 6448ddeae2227630c3f6eedbfc76a31df216c3fd.
The previous test-discrimination P2 is fixed. The new regression mounts ChatComposerRegion, submits through the real form, and uses the same createRevisionAwareOnSend factory that AppShell calls. I temporarily restored the old unchanged-text guard in that production factory; the new test failed because reviseBeforeTurn was not called (0 versus 1), then passed after the diagnostic mutation was removed.
This head is still not merge-ready because the current hosted test and the exact-base local architecture gate fail with one P1 (inline): extracting the submit path created a new unclassified renderer file and new legacy AppShell closure debt, which the strict-base ratchet explicitly forbids. Updating the generated ledger does not authorize that new debt.
Validation: Desktop build/typecheck; focused revision suites (7/7); lint; format; ASF headers; git diff --check; current-main merge-tree. Hosted test fails only at the renderer architecture gate with the same three diagnostics reproduced locally. Current main is 0f98c2a4; the PR is 3 commits ahead / 5 behind and merge-tree is clean.
Not verified: packaged Electron/manual UI or native Windows/macOS.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| "src/renderer/custom-pet-companion-model.ts", | ||
| "src/renderer/custom-pet-companion.tsx", | ||
| "src/renderer/daily-review-actions.ts", | ||
| "src/renderer/desktop-composer-submit.ts", |
There was a problem hiding this comment.
[P1] Do not admit this new AppShell helper by adding it to the generated debt ledger. The strict-base architecture check compares this head with the PR base and currently fails with three linked violations: AppShell gains ./desktop-composer-submit.js dependency debt, this file adds forbidden legacyAppShellClosure debt, and it is a new unclassified renderer source outside an approved legacy directory. The hosted test is red on exactly these diagnostics, and I reproduced them locally with check:architecture --base 2f322055... --strict-base. Please place the extracted submit owner behind an approved application/feature boundary (or make an independently reviewed architecture-policy change); a ledger entry alone cannot waive the ratchet.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 1b71607d8406cc2ae8e686555f80649333f3ab8e. I found no remaining P0-P3 issues in this revision.
The previous architecture P1 is fixed. The submit implementation now lives under the Conversation feature (features/conversation/controller/composer-submit.ts) and AppShell reaches it through the feature public API. The exact-base strict renderer architecture gate passes, including all 112 checker tests.
The unchanged edit-and-resend regression remains connected to the mounted Composer form and the same createRevisionAwareOnSend factory used by AppShell. As a discrimination check, I temporarily blocked revision sends before prepareRevisionSend() in the relocated production function; the test failed with reviseBeforeTurn called 0 instead of 1. After restoring the exact head, the focused revision suites passed 7/7 and the complete Desktop test suite passed 2888/2888.
Validation on Node 24.18.1: build:test, full Desktop build and typecheck, strict-base renderer architecture, lint, format, ASF headers, Windows test inventory, git diff --check, focused revision tests, full Desktop tests, and the WorkHub layout Electron E2E file (3/3). Current main is ec324da4; the PR is 4 commits ahead / 7 behind and the current-main merge-tree is clean (f342632b).
The hosted test check is still red because the WorkHub layout E2E lost its Electron session while closing a native menu. That path is outside this PR's changed files, and the same file passed locally 3/3, but the required hosted check should be rerun and green before merge. I did not validate packaged Electron or native Windows/macOS behavior.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
No description provided. |
|
Skimmed through the diffs. The unchanged resend now follows the existing revision-and-send flow, with a regression test covering the Composer submit path. The earlier architecture issue is resolved, and CI is green. LGTM. |
Brings in the composer-submit extraction (apache#5815) and the allow-unchanged edit-and-resend decision, merged into the revision staged-context feature: - app-shell.tsx keeps the upstream extraction; the staged-context wiring and the apache#5274 re-keyed quotesForSend/clearQuotes semantics move into features/conversation/controller/composer-submit.ts (owner-keyed ports). - app-shell-revision-actions.ts keeps the @maka/ui delegation; TurnRevisionDraftBase drops originalText and the unchanged gate (revisionStagedContextUnchanged) so unchanged resends pass through per upstream apache#5815, while the mixed-context conflict gate stays. - conversation-copy.ts keeps both new locale keys (revisionDraftQuoteConflict, revisionMixedContextUnsupported) on top of upstream's revisionUnchanged removal and apache#5815 copy updates. - renderer-architecture.json regenerated via check-renderer-architecture --write; app-shell.tsx budget drops 12091 -> 11154. Tests: upstream app-shell-revision-resend gains a stagedContext fake; the ui revision suite expectations updated to the allow-unchanged behavior. Generated-by: GLM-5.3-Flash (ZCode)
Review follow-ups (#5274 review): - prepareRevisionSend re-keyed the edit-start snapshot, so quotes the user removed or re-annotated during the edit reappeared on the branch and could be sent. The re-key now lands the plate's current quotes, and the send reads them live (the before-send snapshot retires — the re-key covers its case). - Drops the stale-base footer copy block and a dead regenerate key: no production code on current main references the old footer shape that survived in this branch's base. The no-op gate keeps counting the model-facing comment. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main into fix/desktop-revision-structured-context fix(desktop,ui): preserve the quote plate across the in-flight revision send The revision send awaited prepareRevisionSend before reading the plate, and the lifecycle re-keys it mid-send: the plate's current quotes are copied onto the branch child and the source bucket is emptied in place. The still-running sendWithAttachments invocation holds quotesForSend from the render that started the send, so it read the emptied source bucket, got undefined, and submitted the edited replacement without its quotes or their edited annotations (#5274 review). Read the plate through the re-keyed owner explicitly: quotesForSend now resolves the bucket by owner key (defaulting to the live draft key for the non-revision send paths), and the revision send passes the prepared draft's session id. The resumed send therefore reads the child bucket the lifecycle just wrote — removals and re-annotations included. The regression test drives beginEditUserMessage → an in-render quotesForSend capture → prepareRevisionSend and asserts the re-keyed plate with its edited annotation still reaches the send; it fails on the previous head with `actual: undefined`. Generated-by: GLM-5.3-Flash (ZCode) fix(desktop,ui): keep quotes added during a cancelled edit Cancelling a revision draft cleared both draft keys wholesale, so quotes the user staged during the edit were lost together with the edit's own restaged set (#5274 review: "Cancelling an edit deletes quotes added during it" — on main they stayed in the composer, and adding quotes during an edit is supported). clearQuotes now returns the entries it removed, and clearRevisionStagedContext re-stages whatever lies beyond the edit's beginEdit snapshot (matched per QuoteRef field set, counted as a multiset) onto the source Session the cancel returns to — whether the user added them before or after the branch child was prepared. The edit's own items are still dropped wherever the commit left them. The regression tests drive a cancelled edit with a user-added quote on each side of the prepare step and assert the addition survives on the source key while the composer text rolls back; both fail on the previous head, the first with the edit's restaged quote returned instead of the user's own, the second with no plate restore at all. Generated-by: GLM-5.3-Flash (ZCode) fix(ui): treat negative owned counts as fully user-owned clearRevisionStagedContext drops the edit's own quotes by per-key count: the first N entries matching the beginEdit snapshot are the edit's, and everything past that count is the user's own staging. The kept check compared the remaining count with `=== 0`, so once the counter ran negative the surplus stopped matching: when the user added a second copy of a quote the edit had also staged, the third identical plate entry was dropped despite being the user's. Treat any non-positive count as fully user-owned (`<= 0`), so every entry past the edit's own count survives the cancel. Fixes a P3 raised inline on #5274. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main (8648258) Brings in the composer-submit extraction (#5815) and the allow-unchanged edit-and-resend decision, merged into the revision staged-context feature: - app-shell.tsx keeps the upstream extraction; the staged-context wiring and the #5274 re-keyed quotesForSend/clearQuotes semantics move into features/conversation/controller/composer-submit.ts (owner-keyed ports). - app-shell-revision-actions.ts keeps the @maka/ui delegation; TurnRevisionDraftBase drops originalText and the unchanged gate (revisionStagedContextUnchanged) so unchanged resends pass through per upstream #5815, while the mixed-context conflict gate stays. - conversation-copy.ts keeps both new locale keys (revisionDraftQuoteConflict, revisionMixedContextUnsupported) on top of upstream's revisionUnchanged removal and #5815 copy updates. - renderer-architecture.json regenerated via check-renderer-architecture --write; app-shell.tsx budget drops 12091 -> 11154. Tests: upstream app-shell-revision-resend gains a stagedContext fake; the ui revision suite expectations updated to the allow-unchanged behavior. Generated-by: GLM-5.3-Flash (ZCode) fix(desktop): assemble the revision staged context outside the frozen shell Root cause: upstream #5546 extracted app-shell helpers and drove the frozen shell's nonTriviaTokens baseline down to 11129 with zero headroom, so the merge that re-attached this PR's stagedContext wiring (a 7-line factory in the createAppShellRevisionActions ports object, +25 tokens) broke the renderer architecture ratchet: 11129 -> 11154. Fix: assemble the RevisionStagedContext closure inside the desktop useComposerAttachments controller (a feature file outside the debt ledger), right beside the hooks that own the quote/attachment buckets. The frozen shell now forwards one member (stagedContext,) instead of building the factory inline. The restoreAttachments entry the shell only carried for that factory is dropped from its destructure: its sole consumer was the deleted block (RevisionStagedContext declares no such member and nothing in @maka/ui reads it from the staged context). Verification: - check:renderer-architecture --base 6e21e61 --strict-base: red (11129 -> 11154) before, green (11129 == base) after; ledger diff is the single app-shell.tsx nonTriviaTokens line, no other entry moved. - desktop focused suites (app-shell-revision-actions, use-composer-quotes-revision-send, app-shell-revision-resend, new-task-staged-content): 18/18 pass. - packages/ui full suite: 710/710 pass. - typecheck green: ui tsconfig + desktop preload/main/renderer/storybook. - biome lint clean on both touched source files; check:asf-headers green. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main (28cc4e6) Resolve the renderer-architecture.json conflict by re-pricing the ledger from the merged tree via check-renderer-architecture.mjs --write (all entries at real merged-tree values, within baseline); drop the trailing blank line at EOF in conversation-copy.ts flagged by diff-check. Merge upstream/main (0aa2707) Resolve renderer-architecture.json by re-pricing from the merged tree via check-renderer-architecture.mjs --write. Merge upstream/main (d6876d7) Resolve the renderer-architecture.json conflict by taking main's ledger (new #5851 file entries) as the base and re-pricing every entry from the merged tree via check-renderer-architecture.mjs --write; the check passes against the ratchet baseline. Merge upstream/main (d7dffca) Merge upstream/main (c838e1f) Fourth structural catch-up for #5274: carries #5868 (Composer staging moved into a persistent ComposerStagingProvider owner) and #5884 (task archive timestamps). Conflict resolution ports the revision staged-context semantics onto the new owner structure: ComposerStagingSubmission gains optional owner-key reads/clears (snapshot for captured submissions, live plate for the revision re-key) and ComposerStagingCommands gains stagedContext(); the frozen shell forwards the one composerStaging handle and the revision assembler derives the gate probe and plate reads from it. Merge upstream/main (1e80e3b) into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json Merge upstream/main (c7fa6bb) into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json Merge upstream/main (255ae23) into fix/desktop-revision-structured-context Merge upstream/main (229e1b4) into fix/desktop-revision-structured-context Merge upstream/main (f7633c3) into fix/desktop-revision-structured-context Merge upstream/main (7c90bac) into fix/desktop-revision-structured-context Merge remote-tracking branch 'upstream/main' into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json fix(desktop): drop the stale regenerate footer copy #5468 removed the Regenerate feature (87ff279), deleting the footer's `regenerate` label and the `regenerateRunning` / `regenerateAgain` / `regenerate` / `requestRegenerate` keys from the conversation copy. This branch's stale base still carried the block, so the merge-base diff re-adds those keys across the contract type and all three locales (including the zh-TW 本輪迴答 wording) while nothing on current main references them — merging would resurrect dead strings. Drop the block from the type and the zh-CN / zh-TW / en entries, matching the upstream shape exactly; the `lineage.regenerated*` keys are a different, live feature and stay. Generated-by: GLM-5.3-Flash (ZCode)
3 participants
Summary
Issue #5132 called for unchanged text in an edit draft to be resendable. PR #5468 removed Regenerate in favor of edit-and-resend, but the unchanged-text guard remained. After that removal, users could no longer retry the same prompt from its original turn.
Remove the guard so an unchanged draft follows the existing revision-and-send path. Remove its unused original-text state and rejection copy, and update the composer guidance to say that users may resend as is.
Background: #5132, #5468.
Verification
tsc -p apps/desktop/tsconfig.renderer.json --noEmit.npm --workspace @maka/desktop run build:main.node --test apps/desktop/dist/main/__tests__/app-shell-revision-actions.test.js.git diff --checkpassed.DesktopSessionSummary.idis missing. No manual Desktop UI run was performed.AI use
Tool(s) and scope: OpenAI Codex inspected the send path, made the code and copy changes, and ran the checks above.
Checklist
AppShell.sendWithAttachments. A helper-only test would pass with the old guard. A red/green regression test needs a testable renderer submission seam; the Electron E2E policy reserves that tier for Electron-owned behavior. This remains unchecked rather than adding a test that cannot detect the defect.Does this PR entail a change in behavior?