refactor(desktop): own Conversation publication and observation - #5869
Conversation
Move transcript publication, observation resources and actual readers below the Conversation owner. Keep AppShell on explicit target, chrome and semantic command capabilities while preserving the persistent Composer. Generated-by: OpenAI Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 7524313ab3ff7b47844a5a4e4dd1750d7341fd0a. I found no substantiated P0–P3 issue in the publication and observation paths I inspected.
This refactor moves transcript publication and its reader identity into the Conversation workspace (apps/desktop/src/renderer/features/conversation/model/conversation-workspace.ts:44-84,142-149). ConversationLifecycle owns observation, reading position, and live-to-durable reconciliation (apps/desktop/src/renderer/features/conversation/ui/conversation-lifecycle.tsx:108-152); the observation effect rejects late handoffs, reuses the reader across subscription retries, and closes it on retirement (apps/desktop/src/renderer/features/conversation/controller/use-conversation-observation.ts:113-192). Transcript, Composer, and cross-feature message readers subscribe beneath the owner rather than receiving message arrays through AppShell (apps/desktop/src/renderer/features/conversation/ui/conversation-readers.tsx:35-92). No storage schema, Host protocol, or IPC contract changed in this PR.
I checked A→B→C selection/publication, seed and error fencing, observation disposal/retry, transient-message projection, invocation-time Copy/Save reads, and the persistent Composer boundary. Local Node 24 build:test, Desktop typecheck (including stories), 87 focused Desktop tests, 121 architecture tests, and the AppShell hook gate passed. Current-head hosted test/label checks passed; merge-tree against fresh main 5ac266b1 and git diff --check are clean. I did not independently run the packaged Electron lifecycle, real-account acceptance, or the full Desktop/UI suites, and I did not prove the claimed render isolation outside the focused production-component harness. This 62-file refactor still needs independent human integration review; GitHub reports REVIEW_REQUIRED.
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.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 7524313a, comparing moved logic against main: event subscription/retry/backoff and teardown order on switch/close, shell-run sync, event-stream health polling (incl. visibility gating), pending interaction load/subscribe, live-content seed generations and reveal, the 1000 ms streaming-settle fallback, refresh/retry commands, transcript publication, provider placement, and the always-mounted lifecycle. Behavior is preserved; the new drop of buffered display events on observation end is documented and safe given Host replay on resubscribe. Hook gate, renderer architecture guard (121/121), renderer + stories tsc, and 239 focused desktop tests pass locally.
No P0–P2. P3s inline, plus:
publishedSession(model/conversation-workspace.ts:125) is a frozen getter-only object but is passed asactiveIdRef(use-conversation-target.ts:56) into factories typed with a writable.current; a future write would compile and throw at runtime. Consider aReadonlyref type.renderer/bootstrap-selection-lease.tsandrenderer/shell-run-update-state.tsre-exports are now imported only by their own tests, while the README says no compatibility layer is kept.
Not verified: packaged Electron, Host reconnect and profile handoff; render-count impact of the first inline note.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| const view = useSyncExternalStore(workspace.publication.subscribe, workspace.publication.getSnapshot); | ||
| const catalog = useSessionCatalogController(); | ||
| const requestedId = useExternalStoreSelector(catalog, selectActiveSessionId); | ||
| const requested = useExternalStoreSelector(catalog, selectSessionById, requestedId); |
There was a problem hiding this comment.
P3: These two catalog row reads no longer pass shellSessionRowEqual, so the lifecycle re-renders on every row update (e.g. activityAt/preview text while streaming), rebuilding handlers and a new frame scheduler each time. Subscriptions are id-keyed so it's perf-only, but passing the equality (or selecting just the needed fields) would restore the old behavior.
| searchTarget={props.searchTarget} clearSearchTarget={props.clearSearchTarget} sessionUi={ui} | ||
| landmarkSessionId={displayed?.shared || displayed?.localState === 'pending' ? null : displayed?.id ?? null} | ||
| listTurnLandmarks={props.listTurnLandmarks} setTurnIndex={workspace.setTurnIndex} | ||
| onRestoreError={(error, sessionId) => ui.setMessageLoadErrorBySession((current) => ({ ...current, [sessionId]: transcriptErrorMessage(error, uiLocale, 'read') }))} |
There was a problem hiding this comment.
P3: For an unrecognised error while restoring the reading position, this used to surface the generic operation-failed copy via localizedShellErrorMessage under the desktop scope; it now shows "Task content is temporarily unavailable" under message-read. If intended, worth a note and a test; otherwise keep the previous mapping.
|
|
||
| import type { UiCatalog, UiLocale } from '@maka/core/ui-locale'; | ||
| import { classifiedErrorFallback } from './operation-diagnostics.js'; | ||
| const COPY = { |
There was a problem hiding this comment.
P3: This duplicates the read/refresh strings from shell-copy. The old messageReadErrorMessage / messageRefreshErrorMessage are now only called from expected-error-presentation.test.ts, so the test pins the unused helpers while the production path here is untested and the two copies can drift. Suggest removing the old helpers and pointing the test at this module.
Filter Catalog bookkeeping at the Conversation lifecycle, preserve restore-error copy and diagnostic scope, and test production transcript copy directly. Mark Session identity consumers readonly and remove unused legacy forwarding modules. Generated-by: OpenAI Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current Conversation ownership follow-up. The lifecycle now compares requested and displayed catalog rows by the fields it actually consumes (apps/desktop/src/renderer/features/conversation/ui/conversation-lifecycle.tsx:54-63), avoiding event-rate rail bookkeeping renders while still reacting to status and profile changes. Restore errors use the Desktop action fallback rather than the transcript-read fallback (apps/desktop/src/renderer/application/contracts/transcript-copy.ts:26-31); read and refresh retain their prior locale-specific copy and diagnostic scopes. The removed shell files are re-export shims. I found no substantiated P0–P3 issue in the inspected increment.
Node 24 Desktop main build, renderer typecheck, 19 focused ownership/error tests, 121 architecture checker fixtures, and the architecture guard pass. The current-head hosted test check, fresh-main merge-tree, and diff check are clean. I did not run packaged Electron, real account/session switching, or the full UI suite.
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.
|
Thanks @chihumyum — moving publication and observation out of AppShell into a sealed Conversation owner is a big clarity win, and the follow-up on the review notes was quick and precise. Merging now. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: two independent automated reviews of this head found no blocking issues, and CI is green.
3 participants
Summary
Refs #4582 — R2 M2 / C, based on
d6876d708fdb8dc0bf35fd8e194ada4d69c92599after #5850 and #5851.AppShell currently owns transcript publication, observation resources and the messages threaded into its children. This moves those responsibilities into a persistent
ConversationProviderandConversationLifecycle. Actual transcript, pending-message, usage and approval readers subscribe below that owner. An ordinary durable-message update with unchanged chrome/pending/usage projections no longer renders Shell or Composer in the production-component harness.Catalog readers filter rail-only bookkeeping while still observing status/profile changes. Reading-position failures retain their original localized action-failed copy and
desktopdiagnostic scope. Transcript read/refresh copy has one production source and direct tests; consumer Session refs are readonly, and unused bootstrap/ShellRun forwarding modules are removed.No intended product, visual, shortcut, IPC or storage change. The render evidence establishes notification isolation for the tested path, not elapsed-time or full-app performance gains.
Verification
Local checks passed with Node 24.19.0 / npm 11.19.0:
upstream/mainand AppShell hook gate passed.7fb2baaa3a236626451fc84772ae82dd36206916, after rebuilding: all 34 Electron tests passed (20 files, 3.7 minutes, one worker, no retries). This includes streaming/remount recovery, local-message persistence across renderer/application restart, unavailable live endpoint with cached history, history paging, references, draft focus,/compact, Workbar/Terminal/Side Chat lifetimes and WorkHub native window reconstruction.git diff --checkpassed.New tests mount the production owner/readers and exercise notification isolation, persistent Composer and observer lifetimes, A→B→C handoff, stale results, seed-before-transcript ordering, retry and cleanup. The fake transcript rejects snapshots before its first batch, matching the Desktop adapter.
Windows native interaction and real-account acceptance were not run locally.
Integration and review focus
The current assumptions remain Catalog-owned selection, the existing Main/preload Host observer, distinct requested/published/owner identities, and one Conversation workspace per renderer. See the feature README for public capabilities, retained transitional consumers and redesign triggers. Shared observation across surfaces or a relocated Host observer requires revisiting this lifecycle seam.
Review gates
7fb2baaa3a236626451fc84772ae82dd36206916. The previous implementation run passed on7524313ab; it does not validate this follow-up commit.AI use
Tool(s) and scope: OpenAI Codex implemented the refactor, tests and documentation and ran the local verification. The commit includes
Generated-by: OpenAI Codex; retain it in the final squash commit. Final review and merge remain human decisions.Checklist
Does this PR entail a change in behavior?