Skip to content

refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) - #5954

Merged
chihumyum merged 10 commits into
apache:mainfrom
chihumyum:refactor/composer-read-side
Oct 3, 2026
Merged

chihumyum merged 10 commits into
apache:mainfrom
chihumyum:refactor/composer-read-side

Conversation

@chihumyum

@chihumyum chihumyum commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This is the second of the two PRs that finish R2 M3 and the last code PR of R2. #5935 moved the submission side. This PR moves the read side. AppShellContent no longer reads the displayed Session's Turn, interaction, queue or load state. It no longer holds the Composer's editor handle, the Turn-footer pending marks or the resume offer, and it does not read published messages for Copy/Save. After this PR, no retained-root row is scheduled for M3, and Conversation's transitional table has no rows. The two remaining M5 rows belong to slice B.

Refs #4582

The eight commits can be reviewed one at a time:

  1. Explicit exports. The six export * lines in features/conversation/index.ts become named exports. Symbols only tests read move to testing.ts.
  2. Copy and Save. conversation-markdown.ts moves into Conversation. The target adapter offers renderPublishedConversation(sessionName, locale) in place of readMessages, so the palette writes Conversation's export and never sees the messages.
  3. Composer edits. The shell held the editor handle in 16 places and passed it to Workbar and Session Collaboration. It now gets ComposerEditingCommands: appendText, replaceText, focus, openModelPicker, seedDraft, discardDraft and claimVisibleDraft. None of them reads a draft back. ConversationComposerRegion attaches the handle itself.
  4. Turn marks and resume. useTurnActionRegistry and useShellResume move into the submission owner, and the turnActions shell port goes away.
    • ConversationTranscriptRegion derives the Turn presentation from the owner's marks. Branch is still withheld from a shared Session.
    • The transcript region injects the banner's resume action, and the Composer region injects the send-slot offer. One resume instance still sits behind both.
    • The command handle gains clearPendingTurnActions(sessionId) for Session teardown.
    • The two resume actions are now memoized, so the owner's reader keeps its identity while nothing they show changes.
  5. Turn readers. useAppShellSessionUiReads and useShellLiveTurn are retired, and each region reads what it renders:
    • ConversationComposerRegion reads the Turn summary, the queue and the owner Session's interaction. It derives Stop, the mode, permission and goal reasons, the model-switch gate, the executor picker's hold and the slash commands (/compact is withheld while a Turn runs). The shell passes only the catalog row's arrival and status and the executor selection.
    • ConversationTranscriptRegion injects the running Turn's activity. It also holds the health notice's model picker for that Turn, and useShellChatModel keeps the status half of the gate.
    • ConversationActivityConsumer feeds the custom pet. petActivityForSession maps its reading onto the pack state.
    • ConversationHomeSurface renders the main column and sets data-home-surface.
    • The gate logic moves unchanged into composerTurnGates and liveTurnFlags.
  6. Delivery recovery (behavior fix). A withdrawn send returns its attachments, directories and quotes to the draft of the Session it left again; see "Root cause".
  7. Retained rows. useActiveExecutionBoundary, useSessionSettingIntent and useShellChatModel stay at the root with the cross-region command reason; see "Review focus".
  8. Review follow-ups. These answer the four P3s from the first automated review:
    • The Turn-action registry clears its own timers when the owner unmounts. The shell's unmount call, which reached an unbound handle, is gone, and so is the no-argument "clear every Session" form.
    • The transcript and the activity reader read a narrow Turn reader, so a send or an edit draft no longer repaints the transcript.
    • The health notice's picker gate is a typed region input, localInteractionAvailable.

Which caller loses which capability

Caller Loses Keeps / gets instead
AppShellContent useAppShellSessionUiReads, useShellLiveTurn, useTurnActionRegistry, useAppShellTurnPresentation, useShellResume; the turnActive, activeStreamingLive, hasLiveTurnContent, activeInteraction, activeMessageQueue and activeExecution reads; the mode, permission and goal reasons, the model-switch gate, the slash-command list and the executor props; composerRef (16 uses); readMessages sessionState and executorComposer region inputs; ComposerEditingCommands; clearPendingTurnActions; renderPublishedConversation
useAppShellSessionUiState (transitional adapter) readMessages, the editor handle in its queue surface renderPublishedConversation; a queue surface of the named edits and the plate's entry actions
Workbar, Session Collaboration the main Composer's editor handle seedDraft + focus (Work Board), discardDraft (Guest Turn requests), taken structurally
Conversation public entry 6 export * lines; useAppShellSessionUiReads, useShellResume, chatTurnActivity, executorComposerProps, desktopSlashCommandPresentation, LiveTurnSnapshot; test-only symbols move to testing.ts ConversationActivityConsumer, ConversationHomeSurface, ConversationActivity
Legacy renderer files use-app-shell-session-ui-reads.ts, use-shell-live-turn.ts; use-turn-action-registry.ts and conversation-markdown.ts move into the feature —

Root cause (commit 6)

Editing a queued steering entry, or choosing Edit on a saved local message that never reached the Host (which cancels it first), restores its text to the editor's keyed draft. It is also meant to restore the entry's attachments, directory references and quotes through the queue's draftContextRestorer slot. #5747 rewrote the shell's staging calls and removed the only assignment to that slot (d74f25311, AppShell draftContextRestorer.current = …). Since then, the text comes back and the staged context is silently dropped.

Staging now offers restoreContext(draftKey, context) on its command handle, and the submission owner binds the slot to it. The restore stays keyed by the Session the send left, so it still lands there if the user navigates away before the Host answers. With no staging owner mounted, restoreContext does nothing. A message whose outcome is unknown keeps its identity and offers only the Host check, bound to its Session and Message; the owner test now pins that.

Review focus

  • Three hooks are retained, not moved. Each is read by several regions that the root composes:

    • the execution boundary: the Composer's permission control and unreadable notice, the palette's permission-mode command, the Session Collaboration dialog gate and the health notice's picker gate;
    • the setting overlay: the Composer's controls, the transcript's model picker, the palette, new-task creation and Session teardown;
    • the model selection: the Composer's pickers, the transcript's labels and health notice, the staging vision gate, readiness, new-task submission and Workbar.

    Moving any of them into one feature would make the palette, Workbar or Session Collaboration read it through a second root projection. The retained-root rows list every consumer.

  • The exports table. The remaining M3 rows in "Transitional feature exports outside Conversation" (TaskEntryWorkspacePickerConsumer, GuestTurnRequests, ModuleHubSkillCatalogRevisionBoundary) are other features' projections into the Composer. The checker rejects any feature-to-feature import, so the root composes them. They are marked "stays" with that reason, not scheduled.

  • ConversationHomeSurface renders the shell's .mainColumn div. data-home-surface is keyed by CSS on that element, and a new legacy root file to host a surface component would fail the legacy-growth check. The shell still owns the class, inert and aria-busy. It passes eligible (its empty-transcript condition), and Conversation adds only the live-content and load-error half.

  • Region inputs. sessionState and executorComposer are region-only props, not forwarded to the surface. They carry what the gates combine with the Turn: the catalog row's arrival and status, and the executor selection.

Coordination with #5952

#5952 (slice B's M5 closure) conflicts with this PR only in the hook gate, the retained-root table and the generated ledger. app-shell.tsx merges cleanly (checked with git merge-tree against d084b62d0). Whichever PR lands second merges main, keeps both sides' rows and regenerates the ledger. #5952 removes the transcript's model labels and model picker, so the merge also drops those consumers from the useShellChatModel and useSessionSettingIntent rows.

Inventory (229e1b466 → this branch)

Measure Before After
Transitional Conversation capabilities (#4582 measure) 2 0
Hook gate: hooks / call sites 25 / 30 20 / 25
Retained-root rows scheduled for M3 8 0
Retained-root rows scheduled for M5 (slice B) 2 2
AppShell-family bridge references / action factories (#4582 measures) 1 / 1 1 / 1 (the E2E fixture)
Conversation export * lines 6 0
Root symbol uses, appShell zone (Conversation) 61 (22) 58 (19)
app-shell.tsx lines / import declarations / nonTriviaTokens 1,760 / 42 / 8,017 1,623 / 37 / 7,509
composerRef uses in app-shell.tsx 16 0
Legacy renderer files 175 171

scripts/check-app-shell-hooks.mjs and the retained-root table are edited by hand. In renderer-architecture.json, the retired legacy paths and the emptied conversation-runtime-and-presentation ownership entry are removed by hand. The rest is regenerated with --write, and the only rootSymbolUses additions are the two new exports.

Verification

At 2e6dacdc7 on 229e1b466, Node 24.19.0, unless noted:

  • npm --workspace @maka/desktop run clean:main && build:test && test:dist: 3,223 / 3,223 pass, 0 cancelled.
  • New fake-DOM owner tests run the real Conversation, staging and submission owners:
    • composer-submission-owner.test.ts: 15 new tests and 1 extended. They cover named edits, Turn marks and the registry's unmount, shared-Session Branch, resume, the narrow transcript read, the picker gate, the Turn readers, mode gates, interaction and queue, activity and home surface, delivery recovery across navigation, the steering bubble, unknown outcome, and a source check that AppShell reads none of it.
    • conversation-owner.test.ts: Copy/Save export.
    • custom-pet-activity.test.ts: the pet mapping.
    • Rewired: session-ui-reads.test.ts (a regional reader in place of the retired hook) and command-palette-desktop-actions.test.ts (Save writes the Conversation export).
  • Tests fail without the change. Each mutation was applied to the source, rebuilt, and the named test failed (35 mutations; the two cleanup mutations were re-run after commit 8):
    • Copy/Save: the export ignores the published range; Save drops the export.
    • Named edits: replaceText appends; a claim outlives its editor; seedDraft writes the visible draft instead of its key.
    • Turn marks and resume: the transcript gets no marks; cleanup is a no-op; cleanup ignores the Session; the registry keeps its marks after unmount; Branch is offered in a shared Session; the banner gets no action; the offer is rebuilt every render; resume is read without the owner Session.
    • Turn readers: Stop is not offered; live content establishes liveness; the health picker is not narrowed; an unobservable Turn counts as running; the home surface ignores live content; it ignores a failed load; the interaction is read for the displayed Session; the mode gates ignore a loading row; the executor ignores the Turn; /compact is offered while running; the transcript gets no Turn activity; the queue is not injected; the pet's two fields are swapped; the narrow reader follows the edit draft; the picker ignores the local-interaction gate.
    • Delivery recovery: the slot is left unfilled, as on main; the context is restored into the displayed Session; directories are dropped; quotes are dropped; the staging handle is a no-op; the unknown-outcome check loses the Message id; an unknown outcome is not checked.
    • Two mutations survived the first version of the tests (the offer's identity, and restoring into the displayed Session). The tests now re-render the owner and publish the second Session before asserting, and both mutations fail.
  • npm run check:app-shell-hooks: ok, 20 hooks / 25 call sites.
  • npm run check:renderer-architecture -- --base 229e1b466 --strict-base: passed. It admits the two new exports.
  • --report: transitional Conversation capabilities 0; every gate entry has a row; M3 scheduled 0, M5 2.
  • These all pass:
    • npm --workspace @maka/desktop run typecheck (includes Storybook);
    • npm run lint, npm run format:check;
    • npx knip --workspace apps/desktop, npx knip --workspace packages/ui;
    • npm run check:asf-headers;
    • npm run windows:inventory (current, 119 declarations);
    • npm run check:e2e-budget (34 tests in 20 files, unchanged);
    • npm run astryx:surface-inventory.
  • Desktop E2E, at bc859b385 (before commit 8): npm run build:with-deps then npx playwright test --config e2e/playwright.config.ts, all 34 Electron tests: 34 passed (3.7 min), 0 failed, no retries. No Electron test was added; per e2e/AGENTS.md, the new coverage is in fake-DOM owner tests. The existing specs drive first send, queueing on a running Session, local recovery across restart, /compact, directory references, side chat, streaming remount and the Workbar through the real app.
  • Not run: a packaged build, Storybook smoke.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Opus 5.5 in Claude Code wrote the implementation, the tests, the ledger regeneration and this description. The author reviewed them. Each commit carries Generated-by: Claude Opus 5.5.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

…icit exports

The public entry re-exported six modules wholesale. Production code
outside the feature uses three of their names: `useShellChatModel` and
`SessionHealthNoticeView` (AppShell and the chat surfaces) and
`executorComposerProps` (AppShell). Only those stay public. The names
that only tests read move to `testing.ts`.

Refs apache#4582

Generated-by: Claude Opus 5.5
Copy and Save read the published messages through the transitional
adapter's `readMessages` and rendered them in the command palette.

The Conversation controller now owns `renderPublishedConversation`. It
renders the published range as Markdown when called, and the palette
receives that export, not the messages. `conversation-markdown` moves
into the feature. The adapter drops `readMessages`, and the
corresponding row leaves the Conversation README's transitional table.

Refs apache#4582

Generated-by: Claude Opus 5.5
AppShell held the main Composer's editor handle in 16 places and passed
it to Workbar and Session Collaboration. Those callers could read
drafts back and write any draft key.

Conversation now builds `ComposerEditingCommands` from its own handle:
- appendText, replaceText, focus and openModelPicker for the visible
  draft;
- seedDraft for Work Board and discardDraft for a Guest's settled turn
  request;
- claimVisibleDraft for Module Hub's later append, which stays current
  only while the same editor is mounted.

None of these reads a draft back. The shell's queue surface carries
these intents and the plate's entry actions, and nothing else.
`ConversationComposerRegion` attaches the handle to the Composer
itself. Workbar and Session Collaboration take the two keyed intents
structurally.

Refs apache#4582

Generated-by: Claude Opus 5.5
…ission owner

AppShell called three hooks for the Turn footer and the resume offer:
`useTurnActionRegistry` held the pending marks and lent them to the
submission owner through its shell port, `useAppShellTurnPresentation`
derived the footer from them, and `useShellResume` built the banner and
send-slot resume actions.

The submission owner now holds the marks and the resume instance.
`ConversationTranscriptRegion` derives the Turn presentation from the
owner's marks (Branch is still withheld from a shared Session) and
injects the banner's resume action; `ConversationComposerRegion`
injects the send-slot offer. One resume instance stays behind both. The
shell's command handle gains `clearPendingTurnActions` for Session
teardown and Host changes, a no-op while the owner is unmounted.

The two resume actions are memoized, so the owner's reader keeps its
identity while nothing the actions show changes.

The hook gate and the retained-root table drop the three rows; the
legacy registry file moves under Conversation.

Refs apache#4582

Generated-by: Claude Opus 5.5
…hat render it

AppShell subscribed to the displayed Session's load, Turn summary,
interaction and queue through `useAppShellSessionUiReads`, derived
live-turn flags with `useShellLiveTurn`, and computed every control a
running Turn holds in its own render body. Each Turn boundary therefore
re-rendered the whole shell.

Each region now reads what it renders:
- `ConversationComposerRegion` reads the Turn summary, the queue and
  the owner Session's interaction. It derives Stop, the mode,
  permission and goal reasons, the model-switch gate, the executor
  picker's hold and the slash commands (`/compact` is withheld while a
  Turn runs). The shell passes only the catalog row's arrival and
  status and the executor selection.
- `ConversationTranscriptRegion` injects the running Turn's activity
  and holds the health notice's model picker for the same Turn. The
  shell's `useShellChatModel` keeps the status half of that gate.
- `ConversationActivityConsumer` feeds the custom pet whether an
  observable Turn runs and whether the owner Session waits on an
  answer; `petActivityForSession` maps that onto the pack state.
- `ConversationHomeSurface` renders the main column and marks it as
  the home surface when the shell's empty-transcript condition holds
  and there is no live Turn content and no failed load.

The gate logic moves unchanged into `composerTurnGates` and
`liveTurnFlags`. The two legacy hooks, their gate entries and
retained-root rows, and the Conversation README's last transitional
row are gone, together with the `chatTurnActivity`,
`executorComposerProps` and `desktopSlashCommandPresentation` exports.

Refs apache#4582

Generated-by: Claude Opus 5.5
…ion it left

Editing a queued steering entry or a cancelled local message restores
its text to the editor's keyed draft, and was meant to restore its
attachments, directory references and quotes through the queue's
`draftContextRestorer` slot. apache#5747 removed the only line that filled
that slot when it moved the shell's staging calls, so since then the
text came back and the staged context was dropped.

Staging now offers `restoreContext(draftKey, context)` on its command
handle, a no-op while no owner is mounted, and the Composer submission
owner binds the queue's slot to it. The restore stays keyed by the
Session the send left, so it lands there even after navigation. A
message whose outcome is unknown keeps its identity: it offers only the
Host check, bound to its Session and Message.

Refs apache#4582

Generated-by: Claude Opus 5.5
…egion reasons

`useActiveExecutionBoundary`, `useSessionSettingIntent` and
`useShellChatModel` were the last retained-root rows scheduled for M3.
Each serves several regions the root composes, so each stays at the
root with the `cross-region command` reason and its full consumer list:
- the execution boundary feeds the Composer's permission control and
  unreadable notice, the palette's permission-mode command, the Session
  Collaboration dialog gate and the health notice's picker gate, and is
  reloaded by the submission owner and Conversation lifecycle;
- the setting overlay feeds the Composer's model, thinking and mode
  controls, the transcript's model picker, the palette's permission-mode
  command, new-task creation and Session teardown;
- the model selection feeds the Composer's model and executor pickers,
  the transcript's labels and health notice, the staging vision gate,
  the readiness and new-task submission model and Workbar.

The exports table's remaining M3 rows (the workspace picker, guest Turn
requests and the skill catalog boundary) are other features' projections
into the Composer; a feature cannot import another, so the root composes
them and they stay. The guest row no longer mentions the Composer ref,
which it lost to the named edits.

Refs apache#4582

Generated-by: Claude Opus 5.5

@Astro-Han Astro-Han 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.

Reviewed bc859b385480a8d33a5b5a00452f69f3a9b5b3d2 — shape, gate and the cross-PR question, since a deeper pass is being done elsewhere.

No P0–P3 findings at this depth.

Shape. 52 files, of which 36 are production (+676/−497) and 16 tests. The production surface is on-subject for moving the Composer's reads, edits and delivery recovery downward: app-shell.tsx shrinks (+45/−182), use-composer-submission.ts gains (+45/−5), the conversation controller files move in the same direction, two new custom-pet-companion modules appear, and the README set is updated alongside. No unrelated area is touched.

Both ledgers only tighten, which is the criterion this series is held to. apps/desktop/renderer-architecture.json moves +10/−92 (net −82) and scripts/check-app-shell-hooks.mjs moves 0/−10 — its allowances are reduced, not increased. Nothing is loosened.

Overlap with #5952, which I was asked to check. They do overlap, on four files: app-shell.tsx, scripts/check-app-shell-hooks.mjs, renderer/README.md and the generated architecture ledger. Both are moving ownership at the AppShell root, so the second to land needs a real merge on app-shell.tsx and on the hooks allowance — the latter especially, because two independent reductions of the same quota can conflict in a way that silently changes how much the shell is permitted to own.

One caveat on that answer: #5952's live head has since drifted from its frozen head (d084b62d → 03c30db7), and I have reported that rather than reviewing it. The overlap above is measured on the frozen revision, so it should be re-measured once #5952 is re-frozen, and the ordering question settled then.

Gate on this head: test and label are green; mergeable is true, and the base is current main.

What I did not judge

  • The internal correctness of the moved Composer logic across 36 production files — that is the deep pass this card deliberately leaves to another lineage.
  • No Electron run.

I did not approve, request changes, or merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han 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.

Automated review (Claude lineage). This was not written by a human, and it does not approve the PR. Please verify before acting.

Review of bc859b38 against merge-base 229e1b466, which is the current main tip. It covers all seven commits.

Verdict: no P0–P2. The move preserves behavior as far as I could trace it. There are four P3s, posted inline.

What I checked:

  • Turn readers. liveTurnFlags keeps the old useShellLiveTurn matching rule. composerTurnGates keeps the mode, permission and goal ladder unchanged.
    • The waiting check now reads activeSession?.status instead of activeSessionForView?.status. The two are equivalent, because the pending placeholder's status is 'active'.
    • The pet still requires execution.available && turnActive.
    • Slash commands and executor props take the same inputs.
  • Model-switch gate split. The shell keeps only the status half, which reaches notice.actionDisabled through useShellChatModel. The transcript region adds the running Turn to sessionHealthModelPickerAvailable. chat-recovery-notice.tsx:62-64 ORs the two, and the transcript is the notice's only consumer, so the behavior matches main. The Composer's own gate still includes the Turn (composer-turn-gates.ts:68).
  • Session identity. The shell's activeId is the same workspace.target store (use-conversation-target.ts:45). Moving resume, Turn presentation and the Turn reads onto workspace.target / submission.activeId does not change which Session they read.
  • Named Composer edits. Each one maps 1:1 onto the old composerRef call. queueSurface is memoized on the stable queue commands, so composerEditing is stable and the Guest Turn request polling effect does not restart.
  • Copy/Save. renderConversationMarkdown moved verbatim. The export reads the published range when it is invoked.
  • Delivery recovery (commit 6): the root cause is confirmed. On main, draftContextRestorer is declared and called (use-session-message-queue.ts:91) but never assigned. restoreContext is line-for-line the restorer that d74f25311 removed. It is bound in a layout effect with stable deps and a guarded cleanup, and it stays keyed by the Session the send left.
  • Ledger and gates. check:app-shell-hooks passes locally (20 hooks / 25 call sites). CI is green, and its test job runs check:renderer-architecture --strict-base and knip.

Coordination with #5952. git merge-tree shows textual conflicts only in renderer-architecture.json, renderer/README.md and scripts/check-app-shell-hooks.mjs.

  • app-shell.tsx auto-merges. The merged file has no dangling references to the identifiers #5952 removes.
  • Whichever PR lands second should also drop "the transcript's model labels" and "the transcript's model picker" from the useShellChatModel and useSessionSettingIntent rows that this PR rewrites, as the description notes.
  • No strict order is needed. Landing #5952 first would let this PR fix its two rows during its rebase.

#5951 merges cleanly with this PR.

Comment thread apps/desktop/src/renderer/app-shell.tsx Outdated
Comment thread apps/desktop/src/renderer/features/conversation/ui/conversation-readers.tsx Outdated
…ts picker gate

Review follow-ups on apache#5954:
- The Turn-action registry clears its timers and marks when its owner
  unmounts. The shell's unmount call reached an already-unbound handle,
  so it is removed, and with it the no-argument "clear every Session"
  form: `clearPendingTurnActions(sessionId)` now needs a Session.
- The transcript and the activity reader read a narrow Turn reader
  (displayed and owner Session, pending marks, the banner's resume
  action). A send-pending or edit-draft change no longer repaints the
  transcript.
- The health notice's picker gate is a typed region input,
  `localInteractionAvailable`, instead of a prop read through a cast.

Refs apache#4582

Generated-by: Claude Opus 5.5
…-side

apache#5826 locked the executor picker and send while a send's Host admission
is pending by narrowing the shell's executor props in the Composer
region. On this branch the region computes those props itself, so it
passes `sendPending: submission.newTaskSendPending` to
`executorComposerProps` (which apache#5826 taught to honor it) and the shell
cannot pass it. apache#5826's owner tests now select an executor through the
region's gate inputs.

`createExecutorSessionActivator` joins the explicit Conversation exports
(AppShell uses it); `executorSubmissionError` and
`executorComposerProps` are test-only and come from `testing.ts`. The
`useShellChatModel` row lists the first-send activation among its
consumers. The ledger is main's copy with this branch's retired legacy
paths and ownership entry removed, regenerated.

Generated-by: Claude Opus 5.5

@Astro-Han Astro-Han 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.

Automated review (Claude lineage). This was not written by a human, and it does not approve the PR. Please verify before acting.

Incremental review of 1e8cc72b against bc859b38. The new work is 2e6dacdc (review follow-ups) plus a merge of main at f7633c3d (#5826). There was no rebase, so I diffed the follow-up commit directly and compared the merge against git merge-tree's automatic result.

Verdict: no P0–P3 in the delta. All four earlier P3s are resolved.

The earlier P3s:

  • Unmount cleanup. useTurnActionRegistry now clears its own timers and marks on unmount (use-turn-action-registry.ts:101).
    • The dead unmount call is gone from the shell and app-shell-effects.ts, and clearAll is gone with it.
    • On the old base, clearAll was only wired to that unmount cleanup, so no Host-change path is lost.
    • Covered by the new owner-unmount test.
  • Clear-all foot-gun. clearPendingTurnActions(sessionId: string) now requires a Session and only calls clearForSession (use-composer-submission.ts:255). Both remaining callers pass a Session id: teardown and the session-change effect.
  • Picker gate typing. ConversationTranscriptRegion takes a typed, required localInteractionAvailable input (conversation-readers.tsx:59). It derives sessionHealthModelPickerAvailable = localInteractionAvailable && !turn.turnActive itself (:104). The cast is gone, and app-shell.tsx:1527 passes the input.
  • Transcript re-renders. The owner now publishes a narrow turnReader through ComposerTurnContext (use-composer-submission.ts:270, composer-submission-provider.tsx:68).
    • It carries the displayed and owner Session, the shared flag, pending marks and safeResumeAction.
    • The transcript and activity readers read only this context. The Composer region reads both readers.
    • No consumer still reads a moved field from the wide reader.
    • The new no-repaint test covers it.

Merge with #5826:

  • Main's region-level send-pending lock was folded into executorComposerProps(..., { sendPending: submission.newTaskSendPending }) (conversation-readers.tsx:192). Main's lock was sendBlocked || newTaskSendPending plus executorPicker.disabled || newTaskSendPending.
  • executorComposerProps already applies sendPending to both sendBlocked and the picker's disabled, so the behavior is unchanged.
  • executorSessionPending and createExecutorSessionActivator merged in cleanly.
  • The test imports moved to testing.ts, and the ledger counts were regenerated.

CI and status: test passes on 1e8cc72b. The PR is mergeable and is blocked only on review. Locally, check-app-shell-hooks passes (20 hooks, 25 call sites).

#5952 at 3c9bf72e: it still conflicts only in renderer-architecture.json, renderer/README.md and scripts/check-app-shell-hooks.mjs. app-shell.tsx merges automatically with no stale references. No order is required. Whichever PR lands second should merge main again, regenerate the ledger, and drop the transcript consumers from the useShellChatModel and useSessionSettingIntent README rows.

@Astro-Han Astro-Han 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.

Reviewed the increment bc859b38..1e8cc72b029d94b8dd6a77fc475dc47ef90b1da5 (13 files, +118/−70), against the four P3s already on this PR and the gate.

All four P3s are addressed, and three of them structurally rather than by adjustment.

  1. Lifecycle (the unmount half doing nothing). clearAll no longer exists anywhere in the renderer, and the per-session handle is what remains: the commands layer exposes clearPendingTurnActions(sessionId) delegating to the registry's clearForSession(sessionId), and the bootstrap interface (app-shell-effects.ts) now declares only clearPendingTurnActionsForSession: (sessionId: string) => void. So the dead unmount path and the option behind it are removed rather than left in place.
  2. The falsy-id nit is closed by the type, not by care. The contract is clearPendingTurnActions(sessionId: string): void — the no-argument form is gone, so the "a falsy id clears every Session" hazard cannot be written any more. The author also states that an empty id clears nothing; with the no-arg form removed, that leaves only the empty-string edge, which I read as intended but did not exercise.
  3. The typing cast is gone. TranscriptHealthGate no longer appears in the renderer, and sessionHealthModelPickerAvailable now lives in chat-message-surface.tsx — the region's own file — with the shell passing a required localInteractionAvailable input instead. So the narrowing no longer depends on a prop read through a cast that a rename could silently break, and the author reports a test pinning it.
  4. The narrow Turn reader is real. use-composer-submission.ts builds turnReader and publishes it through a dedicated context (ComposerTurnContext.Provider), which conversation-readers.tsx consumes via useComposerTurnReader(). So the transcript and activity readers take the narrow reader rather than the whole composer reader, which is what the performance P3 asked for; the author reports a test asserting that a send-pending or edit-draft change does not move it.

Gate on this head: test is completed/success; mergeable is true.

Scope, stated plainly: this pass verified the four P3s' resolution and the gate, as the dispatch scoped it. It is not a re-audit of the 36-file move, and Claude's own review of the previous head already covers the behaviour-preservation tracing.

What I could not judge

  • The empty-string edge in (2), as noted.
  • No Electron run.

I did not approve, request changes, or merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han 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.

Approved at @Astro-Han's explicit request: no blocking findings in our automated reviews of this head and CI is green. It now conflicts with main, so please merge main or rebase; we will re-check the conflict resolution before merging.

…composer-read-side

apache#5952 (R2 M5 closure) and this branch both cut the AppShell hook gate
and rewrote retained-root rows:
- Hook gate: both removals apply. `useEffect` (apache#5952) and
  `useAppShellTurnPresentation` (this branch) are gone: 19 hooks / 24
  call sites, matching the merged tree.
- Retained-root table: apache#5952's rewritten rows are the base. This
  branch's removed hooks drop out, and its three cross-region rows
  replace apache#5952's M3 rows. Those rows no longer list the transcript's
  model labels and model picker, which apache#5952 removed from ChatView. The
  Session-workspace row names the Composer edits instead of the ref,
  and the bootstrap row drops the pending-ref cleanup this branch
  retired. No row is scheduled for removal.
- Exports table: apache#5952's two "stays" rows (OverlaysConsumer,
  ManualDiagnosticReportConsumer) and this branch's three are kept.
- Ledger: main's copy minus this branch's retired legacy paths and
  emptied ownership entry, regenerated.

Generated-by: Claude Opus 5.5
@chihumyum
chihumyum merged commit 7c90bac into apache:main Oct 3, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants