Conversation
…ce replay Read a small recent transcript range, load earlier Turns on upward input, and avoid projecting root-session history for ShellRun resource queries. Generated-by: Codex
Session-switch verification updateThis PR reduces work on the path to a useful transcript: a small complete-Turn history window, automatic earlier paging on upward input, and lineage checks before ShellRun inheritance discovery. It adds no history projection or persistent cache. The controlled comparisons (50 packaged returns per side, before rebasing) measured 79.3% lower C content-frame median for bounded reads and 93.9% lower B median for avoiding unrelated root-session replay. C's first-batch→ready median fell 1,031→56 ms; history requests 20→2, batches 41→3. B's full-cycle Host CPU median fell 14.135→0.210 CPU seconds, with no continued full replay observed after display. The old B tail includes reconnects; the full report retains those samples and explains the limits. Fresh-fixture verification of 26086c9 additionally observes current main's placement and fade-in. These are separate endpoints from the older DOM-frame comparisons:
A/B remain local workloads; C is the active remote workload. At n=10, nearest-rank p95 is the maximum, not a strong tail guarantee. All active returns pass same-turn and output-continuity checks. C still opens zero new target subscriptions and delivers two pages / three batches. The earlier 1,000-turn C regression also passed ten returns after the resource fix. Correctness on current main: 9,148 tests passed, 26 conditional skips, seven Chromium layout/scroll scenarios passed, plus build/typecheck/lint/format/Knip and repository guards. Mutation tests detect removal of the bounded initial read, pending guard and each lineage-before-history check; 77 focused tests pass after restoration.
Screenshots are synthetic-fixture captures from the implementation campaign; final browser assertions verify the behavior after rebasing. The exact Windows/WSL workload and original five-second settled-page endpoint remain unverified. Startup-to-ready timings from #5556 are not claimed as effects of this session-switch PR. Prepared with Codex assistance at the contributor's direction. |
Remove the unused HStack import after dropping the earlier-history button and regenerate the required Astryx surface inventory. Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed current head 2d8f9d6abb76fd25a2666e1599594ab9b19990dd and found no substantiated P0–P3 issue in the changed paths.
Desktop now reads up to 128 KiB of recent transcript history initially and 512 KiB per earlier-history read (apps/desktop/src/preload/transcript-contract.ts:26-29). The UI replaces the manual earlier-history button with an upward-scroll-near-top trigger (packages/ui/src/transcript-scroll-authority.tsx:256-267,338-355); recovery and root ShellRun projection are adjusted accordingly. I checked pagination budgets, session switching/recovery, root update behavior, scroll triggering, and the revised Desktop E2E that uses an upward wheel gesture. There is no database schema migration.
The current-head test check passes (including the updated Desktop E2E), and the branch merges cleanly with current main. I could not rerun tests locally (Node 18 and no dependencies), exercise real Electron touchpads, or measure the claimed performance benefit. This is not a merge approval.
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.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Bounds the transcript history read when (re)opening a conversation (128 KiB open / 512 KiB per earlier read, replacing a single 64 MiB budget), replaces the "load earlier" button with upward-scroll loading, and checks session lineage before inherited ShellRun discovery so root sessions skip transcript projection. The issue is real — base transcript-contract.ts had one 64 MiB budget and base session-manager.ts read history before lineage. Direction is right and well tested; one wiring slip.
Findings
- [P2]
apps/desktop/src/main/runtime-host-session-observer.ts:269—#transcriptInitialHistoryBytesis initialized fromdeps.transcriptHistoryBytes, not from a dedicated dep; the deps interface (line 117) declares onlytranscriptHistoryBytes?. Production never passes it (only the e2e fixture,runtime-host-boot.ts:1149), so defaults behave, but any embedder that tunes the earlier-read budget silently overrides the opening budget too — the 128/512 KiB split this PR exists to create cannot be configured independently, and a compat override (e.g. back to 64 MiB) would quietly defeat the fix. One-line fix plus an interface entry, or a comment stating the coupling is deliberate. - [P3]
packages/ui/src/chat-view.tsx:556-569— auto-load failures are fully swallowed (Promise.resolve(...).then(settled, settled)) and the button that previously communicated pending state is gone, so a failed history read is invisible to the reader (they must just scroll up again and hope). Consider a transient inline notice on rejection; the retry-only-on-next-gesture behavior itself is pinned by the new unit test.
Verdict
needs-changes — one-line dep-wiring fix for the initial-history budget; otherwise sound, CI green (run 36097628895).
|
Review follow-up on current head
Validation: full build/typecheck; 104 focused observer, WorkHub, scroll-history, and export tests; strict renderer architecture against |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 36edb69ed25f1311022d67353554c308e0436fc5. I found no substantiated P0–P3 issue in the changed behavior I checked.
The change bounds initial transcript history to 128 KiB and earlier reads to 512 KiB, while continuing past a budget when needed to finish a Turn or restore a previously delivered floor (apps/desktop/src/preload/transcript-contract.ts:26-30, apps/desktop/src/main/runtime-host-session-observer.ts:1590-1596,1660-1670). Upward reader input requests earlier history without the retired button, with a per-session pending guard (packages/ui/src/transcript-scroll-authority.tsx:256-267, packages/ui/src/chat-view.tsx:563-579). Root-session ShellRun lookups now check lineage before projecting transcript history, preserving the inherited-resource path for child/revision Sessions (packages/runtime/src/session-manager.ts:1395-1408,1459-1474). I also checked the refreshed startup test and upstream merge; no migration or protocol-state change was introduced.
The exact-head hosted test job passed, including Desktop E2E and Storybook smoke. git diff --check and a merge-tree against current main 0fd75408 are clean. I did not reproduce the reported macOS performance measurements or run a local packaged/long-session test; large individual Turns may still exceed the requested byte budget. This is a COMMENTED review, not a merge approval.
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
There was a problem hiding this comment.
Independent second review of head 36edb69e (a different model lineage from the parallel review).
The paging itself looks correct:
- reads stop only on whole-Turn boundaries, and an over-budget single Turn arrives whole;
- no rows are lost or duplicated across pages;
- a recovery reset re-reads down to the oldest delivered message;
- stale pages are dropped when you switch sessions, and a load that finishes in one session can't unlock another's pending load;
- jumping to or restoring an old Turn loads down to it;
- the ShellRun fast path is equivalent, and IPC frames are unchanged.
However, shrinking the opening load from 64 MiB to 128 KiB means most sessions now hold only part of their history in the renderer, and several renderer features still assume the loaded messages are the whole conversation.
P1: Copy/Save conversation silently exports only the loaded range.
app-shell-command-actions.ts:219-247passes the loadedmessagesstraight torenderConversationMarkdown(conversation-markdown.ts:46-57), with nohasOldercheck.- This was latent before and hidden by the 64 MiB budget. At 128 KiB, a user who opens a long session and hasn't scrolled up gets only the last few Turns. The success toast still reports a line count as if the export were complete.
- Please export via a full Host read, or at least refuse or warn when
hasOlderis true.
P2: WorkHub linked work and the filter-by-work view use only loaded messages.
workhub-root.tsx:257-258→linked-work.ts:35-95→workhub-conversation.tsx:93-95.- Delegations in older history disappear from the rails, links and hues. The work filter can show only some matching Turns, or the empty state, with no hint that older history exists.
- The PR removed the comment that described this case. The only remaining way to load more is scrolling up, with no visual cue.
P2 (process): the merge commit f105ea616 reformats whole files. It touches runtime-host-boot.ts, conversation-copy.ts, runtime-host-session-observer.ts and its test, though the real change is about ten lines.
biome.jsoncdeliberately excludesapps/desktop/**andpackages/ui/**from formatting, because source-shape tests depend on the layout.- Commit
8b7cc3da8then loosened the regexes inmain-startup-lifetime.test.tsto tolerate the new layout. - This hides the real diff and will conflict with other PRs touching
runtime-host-boot.ts. Please revert the reformatting.
P3:
- The "load earlier" button is gone (
chat-view.tsx:563-579,transcript-scroll-authority.tsx:256). Screen-reader users, and keyboard users whose focus is in the composer, can no longer load older history, and the empty state gives no hint that more exists. - Huge Turns need about four times as many reads. The 128 KiB opening budget is also used as the Host page size (
desktop-transcript-replica.ts:273), so a 20 MiB Turn takes about 160 reads instead of 40. Keeping 512 KiB pages and using the budget only as the stop condition would avoid this. - The context-usage meter can go blank.
latest-request-usage.ts:48-67finds nothing when the loaded range has no anchoredtoken_usage, for example when the recent Turns aborted or one Turn fills the range. - The regenerate-approval banner can lose its text preview when the source Turn is older than the loaded range (
turn-request-inbox.ts:70-85). - ShellRun listing now rejects when the session header can't be read (
session-manager.ts:1402), where it used to return the session's own runs. This is intentional and tested, but worth stating in the PR.
Test gaps:
- nothing covers export or WorkHub links with partial history;
- the E2E fixture sets the opening budget to 1 MiB (
runtime-host-boot.ts:1328), so the real 128 KiB path is only unit-tested; - nothing tests the page count for an oversized Turn.
Verified: the touched unit tests pass locally (runtime 7/7, ui 7/7, desktop 77/77), and the new tests fail on the old code.
Not verified: E2E, Storybook, the full suite, and switch latency or memory on huge sessions.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; the P1 was checked against the code, but please verify before acting.
…ssion-switch # Conflicts: # apps/desktop/src/main/__tests__/runtime-host-session-observer.test.ts
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 997e0fdc after the upstream merge and the new full-history fix. The previous P1 (truncated Copy/Save) is fixed: both commands now call readCompleteTranscript (app-shell-command-actions.ts:229-255), which reads to sequence 0 and refuses cached, incomplete, or switched-session ranges (transcript-reading-position.ts:130-149). The previous WorkHub partial-link/filter P2 is also addressed: WorkHub requests history to sequence 0 and only derives linked work when historyComplete is true (use-workhub-controller.ts:494-545, workhub-root.tsx:257-262). New tests exercise both paths. I found no new P0-P3 in those changes.
The earlier P2 scope/test-quality concern remains: merge commit f105ea616 reformatted formatter-excluded Desktop/UI files, and main-startup-lifetime.test.ts loosens source-shape regexes to accommodate that churn; biome.jsonc:48-58 explicitly excludes those trees. Please revert the unrelated reformatting rather than treating this as resolved. The prior accessibility and oversized-Turn P3 observations are not changed by the latest fix and still need product judgment.
The exact-head hosted test is green, git diff --check and a merge-tree against current main are clean. I did not run a local full suite, measure switch latency/memory on long sessions, or exercise real Electron/assistive technology. This COMMENTED review is not an approval or merge recommendation.
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
There was a problem hiding this comment.
A second, independent review of head 997e0fdc (Claude lineage). It adds to the hqhq1025 review 5334256529 and does not repeat its findings.
Confirmed fixed:
- Export (the earlier P1). It now goes through
readCompleteTranscript(transcript-reading-position.ts:130-150). That call reads back to the start and fails unless the history is complete and live, and the host side keeps reading until it reaches sequence 0 (runtime-host-session-observer.ts:418-445). - WorkHub links and the filter by work item. They now wait for
historyComplete(workhub-root.tsx:257-262), after the eager full load inuse-workhub-controller.ts:494-509.
Still open: the P2 for reformatting whole files, and the P3s from the previous head.
New P3s:
- WorkHub links can vanish for the rest of the session after a failed full load (
use-workhub-controller.ts:494-509). The "full history requested" flag is set once and never reset. IfloadEarlier(0)fails partway, for example on a host reconnect, the next ready snapshot clears the error (:524) and nothing retries. Work colours, labels and the filter stay missing, with no error, until the user switches sessions. The error text at:506is also hard-coded English. - WorkHub history is no longer bounded. Opening WorkHub on a long session reads the entire history in one go, with no cap. For sessions over 64 MiB, that is more than main loads today. Please either note in the description that the "bounded history" claim doesn't cover WorkHub, or derive links from a host-side index.
- Export on a large session gives no feedback while it reads. It pulls the full history into the chat view and shows no progress, and the Save dialog opens only after the read finishes. A clipboard failure now shows the generic export error instead of "clipboard unavailable".
Tests: build:test passes, and the touched tests pass: desktop 122/122, runtime 7/7 and ui 7/7. check:architecture and the renderer typecheck are green. Not run: E2E, Storybook, and timing or memory measurements on large sessions.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
|
@hqhq1025 @Astro-Han Thank you for the reviews. Follow-up on current head
The 64 MiB retained transcript cache is unchanged. The history-read contract is now 128 KiB for initial session open and 512 KiB for later pages. Hosted CI run 36385836102 passes on this exact head, including Runtime Host, Desktop E2E, Storybook smoke, transcript geometry, and installed CLI validation. There are no unresolved review threads. |
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e5f27b7c1dbac0a6f23d21d6d9e4b6b7c92bbce9, focusing on the new history/export/WorkHub/UI commits and the prior findings. I found no new substantiated P0–P2 issue.
The earlier truncated Copy/Save P1 remains fixed: both commands await readCompleteTranscript (app-shell-command-actions.ts:118-126,218-256), which reads to sequence 0 and rejects cached, incomplete, or switched-session ranges (transcript-reading-position.ts:130-149). WorkHub still waits for complete history before deriving linked work (workhub-root.tsx:257-262); this head additionally makes a failed full read retryable with visible feedback (use-workhub-controller.ts:496-519,791-825) and tests that retry. The earlier unrelated whole-file reformat/test-regex P2 is closed: the final PR diff no longer changes main-startup-lifetime.test.ts or conversation-copy.ts, and the affected production files now have focused diffs. The load-earlier button is restored in empty and populated chat states (chat-view.tsx:915-989), and clipboard denial now gives explicit feedback (app-shell-command-actions.ts:221-229).
Previously noted P3 trade-offs remain: reading to sequence 0 can load an entire long WorkHub/export history (use-workhub-controller.ts:506, transcript-reading-position.ts:144); the Host also reads through an oversized Turn even beyond the byte budget (runtime-host-session-observer.ts:1507-1512). I did not independently measure memory/latency on long sessions, run the local full suite, or exercise real Electron/assistive technology. The hosted current-head test, diff-check, and a merge-tree against current main pass; the PR base still lags main, so merged-tree tests have not run.
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
There was a problem hiding this comment.
A second, independent review of head e5f27b7c (Claude lineage), alongside the hqhq1025 review 5335154568. We found no P0–P2.
Every blocking item from earlier rounds is resolved:
- Reformatting and test regexes. The reformatting is gone from the final diff:
runtime-host-boot.tsis now +4/−1 andruntime-host-session-observer.ts+16/−2, andconversation-copy.tsis no longer in the diff.main-startup-lifetime.test.tsalso left the diff, so its regexes match main again. - The load-earlier button is restored (
chat-view.tsx:926-938), with labels in all three locales. - A failed WorkHub full-history read is now shown and retried (
use-workhub-controller.ts:504-519,:791-826), with localized copy. - Clipboard failures show
clipboardUnavailableagain (app-shell-command-actions.ts:224-229). - Oversized Turns are read in 512 KiB pages after the first cut (
runtime-host-session-observer.ts:1478-1482), and a new test covers it.
P3 (non-blocking):
- The button's "Retry" label never appears in practice. It shows only when
loadEarlierrejects, but both production callers catch the error and report it elsewhere. The button still works. - Keyboard focus drops to the page body. Clicking the button disables it natively (
:933), with no busy or screen-reader announcement. Main does the same. - The WorkHub history error can linger. If a full read fails and the user then loads the rest manually, the error stays, because only the full-read path clears it (
use-workhub-controller.ts:497-503). Clearing it wheneverhistoryCompletebecomes true would fix this. - The history error takes priority in the display and in
retry(:791,:823), so it can hide a send error whose admission is unknown. - Carried over and acceptable as trade-offs:
- WorkHub's full read has no overall cap.
- The usage meter can go blank.
- Export shows no progress.
Tests: build:test passes. Desktop 124/124, ui chat-view-load-earlier 7/7, runtime 7/7; the renderer architecture check and typecheck also pass. Not run: E2E, Storybook play, the full suite.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
|
Reviewed the new exact-head feedback and fixed the two WorkHub state issues in I left the other P3 observations unchanged: the button focus/busy behavior is inherited from main, the Retry-label reachability does not prevent the button from working, and full WorkHub/export reads plus the usage-meter behavior are already documented trade-offs rather than new blockers. Validation on Node 24: Desktop main build passed, focused WorkHub controller tests 30/30, Biome and |
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed head 5017c7534780f21fef65ab13a93c92c95ce16a3e and found no new substantiated P0–P3 issue in this revision.
This commit reconciles the WorkHub history error once a subsequent snapshot reports complete history (use-workhub-controller.ts:530-535) and gives an uncertain or rejected send priority over a simultaneous history-read error in the displayed error and Retry action (use-workhub-controller.ts:749-759,797-801,829-841). The two new tests exercise completion after a manual history read and recovery of an uncertain send while history remains incomplete (workhub-send-visibility.test.ts:887-959). I also rechecked the earlier complete-history export/linked-work fixes at this head; they remain present.
Locally, the focused WorkHub suite passes (30 tests), the diff check and merge-tree against current main are clean, and the exact-head hosted test check passes. I did not independently run the full suite, a real long-session memory/latency test, or native Electron and accessibility smoke. Full-history reads remain unbounded in total; a single oversized Turn can exceed a page's byte budget. The PR description's verification heading still names the previous e5f27b7c1 head, so those local results should not be read as tests of this commit.
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
There was a problem hiding this comment.
Incremental re-review at 5017c7534 (delta from e5f27b7c1, which our previous full-PR review found clean: 2 files, +89/−7).
Verdict: no P0–P3 found in the delta.
use-workhub-controller.ts:533clears a stalehistoryErroronce a snapshot reportshistoryComplete. This is the right place, because the full-history load that follows will set a fresh error if it fails again.retryableSendError(:752-759) is scoped to the currentsessionId. It now takes precedence in both the surfacederrorand theretry()routing. This means a send that is unknown or rejected is no longer hidden behind a concurrent history or read error, and Retry re-sends or recovers instead of only reloading history.canRetryis slightly narrower than before, because it now requiresattempt.sessionId === sessionId. That is correct: the old branch could offer Retry for another session's pending attempt and then fall through toretryResolution.- Note: after a successful send retry, a coexisting
historyErrorstays visible until the next complete snapshot or the next Retry. This is acceptable and does not rise to a finding. - The two new regressions in
workhub-send-visibility.test.ts:887-959cover both precedence cases.
The earlier conclusions on the rest of the PR carry over, because the delta touches no other files.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
Extreme-scenario stress tests on
|
| Target workload | Visible frame median / maximum | History pages | Renderer payload |
|---|---|---|---|
| Ordinary root, 100 Turns × 10 tools | 386.6 / 473.4 ms | 2 | 0.202 MiB |
| Ordinary root, 1,000 Turns × 10 tools | 386.0 / 718.8 ms | 2 | 0.204 MiB |
| One Turn with 1,000 tools | 2,654.1 / 3,295.3 ms | 16 | 2.427 MiB |
| One Turn with a 4 MiB assistant message | 1,635.7 / 1,995.0 ms | 2 | 4.001 MiB |
| Existing 300-Turn branch, fresh app/Host | 385.8 / 393.7 ms | 2 | 0.203 MiB |
| Restore the first Turn of 100-Turn history | 942.1 / 1,729.0 ms | 21 | 2.529 MiB |
| Restore the first Turn of 1,000-Turn history | 5,667.1 / 6,104.2 ms | 201 | 25.475 MiB |
The visible-frame endpoint starts at actual Renderer pointerdown and includes target DOM, placement/fade and double RAF, followed by a viewport assertion. It is a browser observation. Five returns support a descriptive median/maximum, not a tail guarantee. These are same-version workload controls; the earlier historical before/after measurements remain separate.
Findings under these stress conditions:
- Whole-Turn work remains material. The 1,000-tool Turn costs 2.17 Host CPU seconds at the median. This build already uses 512 KiB continuation pages after a mid-Turn cut. A separate 4 MiB tool-output case is much faster (302.7 ms), but sends only 3,708 bytes of display data; the full assistant-message case above avoids confusing bounded tool presentation with full-message transport.
- Branch first-frame latency hides resource work. A 300-Turn root costs 0.14 Host CPU seconds over the complete away/return cycle plus 1.5 seconds of observation. Its branch costs 1.44; reopening that committed branch in a new app/Host still costs 1.50, with 1.09 occurring after display. The restarted branch's sampled Host RSS median is 686.6 MiB. This measures process residency, not per-return allocation or proof of a leak. The retained inherited-resource projection path warrants separate attribution before choosing a mechanism.
- Distant restoration fills the intervening range. For 1,000 Turns, first protocol ready is 103.7 ms, but final target-range ready is 5,193.5 ms and the old position becomes visible at 5,667.1 ms. All ten restoration samples reached the first Turn with zero top-offset error. Using the first ready timestamp would miss most of this cost.
Branch creation was measured separately from returns: one 100-Turn creation was observed committed after 11.893 s; one 300-Turn creation after 91.360 s. Both commands first reported an unknown outcome, and the harness only queried the original result. A 1,000-Turn / 10,000-tool branch was still unpublished after 600 seconds of reconciliation, so it has zero return samples. That isolated Host was stopped after preserving diagnostics. The report also retains a separate aborted 4,000-tool fixture import; it is not counted as read latency.
All 55 successful returns passed target-visibility and history-page error checks. Native fixture generation validated terminal invocations, settled tools, projection diagnostics and SQLite integrity. The product commit is unchanged by this measurement follow-up. The observed costs remain useful as boundary-regression evidence. Prioritizing optimization requires real usage distributions and operation frequency; these stress numbers alone do not justify a new projection, cache, index or other architectural complexity.
Full report and tradeoffs · Per-return numerical evidence · Reproduction
This campaign covers local idle macOS conditions. The original Windows/WSL active-remote workload remains outside these new measurements. Prepared with Codex assistance at the contributor's direction.
…ssion-switch # Conflicts: # docs/windows-test-inventory.md
|
Conflict follow-up on current head
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed 8d73d4e237609dd784b816181a8e4cc2061a7579. This head merges current main into the previously reviewed branch. Its only manual merge resolution is the Windows test inventory: the resulting counts and added entry match main. The PR-specific behavior remains: initial transcript reads use the smaller budget while preserving whole Turns and recovery floors (apps/desktop/src/main/runtime-host-session-observer.ts:1441-1504); root Sessions avoid historical ShellRun projection (packages/runtime/src/session-manager.ts:1397-1405,1462-1468); Copy/Save and WorkHub explicitly request complete history (apps/desktop/src/renderer/app-shell-command-actions.ts:115-127, features/workhub/controller/use-workhub-controller.ts:497-555). I found no new substantiated P0–P3 in the inspected merge and these paths.
The exact-head hosted test check passes. A merge-tree and git diff --check against current main 28cc4e647 are clean. I did not rerun the full suite locally, profile a long real Session, or validate Electron rendering/scroll behavior. Whole-session export and WorkHub derivation may still read unbounded history, as described in the PR. This is not a merge approval.
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
There was a problem hiding this comment.
Reviewed commit 79fd458. This increment adds indexed transcript-window seeking and forward paging, detached complete reads so exports do not replace or acknowledge a parked view, and deferred rendering of settled collapsed process bodies. I inspected the Main/IPC/renderer window and recovery paths (notably apps/desktop/src/main/runtime-host-session-observer.ts:1573, apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:275, and packages/ui/src/chat-view.tsx:607). I found no substantiated P0–P3 issue in those paths.
Node 24 npm ci, npm run build:test, Desktop typecheck, 80 focused Desktop tests, and 75 focused UI tests passed. git diff --check is clean. This head has no hosted checks; against fresh main 0aa2707 there are conflicts in apps/desktop/renderer-architecture.json and apps/desktop/src/renderer/app-shell.tsx. It is not ready for merge. I did not verify real Electron long-session scrolling, packaged builds, or the full test suite. The prior green run and reviews belong to older heads.
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
There was a problem hiding this comment.
Reviewed the current head. Since the previously reviewed implementation, the branch merges upstream Plan state changes and updates one transcript Storybook file. The merge resolution updates the renderer architecture inventory for the upstream Conversation ownership (apps/desktop/renderer-architecture.json:778); its architecture guard passes. The story now waits for the real setup scrollend before preparing the nested-scroller boundary, and checks that preparation does not itself load history (apps/desktop/stories/app-shell.stories.tsx:3090). It also verifies that a never-opened completed process has no mounted body (apps/desktop/stories/app-shell.stories.tsx:4680). No substantiated P0-P3 issue in these changes or the inspected transcript path.
The current-head hosted test check is green; fresh-main merge-tree and diff check are clean. Local Storybook typecheck did not complete because of missing/stale built runtime-host storage-usage exports in this checkout, so I did not independently run the browser stories, a long Electron session, or packaged Desktop. This comment is limited to the inspected paths and is not merge approval.
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
There was a problem hiding this comment.
Reviewed at 2a104db4. Bounding transcript work on session switches is a solid direction, and the touched test suites pass locally (desktop 262/262, ui 82/82, runtime 7/7). One P2 and four P3s, inline below:
- P2 — a bounded window opened while the session is still writing can end with
newerset and then stop following live output until the user scrolls or jumps to latest (reproduced with a throwaway test). - P3 — a turn-completion persistence check can now reject when the read is cancelled, surfacing a spurious refresh-failed toast (reproduced).
- P3 —
transcriptInitialHistoryBytesis never forwarded, so the partial-history e2e runs with the default budget. - P3 — Resume eligibility uses the last visible turn when viewing older history.
- P3 — first send after opening at a saved position / search hit reopens the reader and drops loaded history even when the view already reached the tail.
Minor, not blocking: WorkHub now loads full history on open with no cap (previously 64 MiB for the first read); worth a conscious decision.
Not verified: e2e and Storybook specs were not run; no real Electron long-session testing.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
|
@Astro-Han Thanks for the review. All five inline findings are fixed in
For the non-blocking WorkHub full-history read observation, I am intentionally retaining the existing correctness tradeoff in this PR. Whole-session links and filtering need complete history; restoring a hard cap would make those results incomplete. A separate Host index/projection would require a storage and consistency design, so this follow-up does not introduce one. That cost remains documented rather than claimed as optimized. Validation on this head: Desktop 3,099/3,099, UI 698/698, 97 focused tests, workspace build/typecheck (including Storybook types), strict renderer architecture, and the existing partial-history Electron journey all pass. Eight deletion experiments detect the fixed defects or the necessary budget, reconnect-bound, and pending-navigation protections. Detailed results and ablation evidence. Hosted CI for this head is still running at the time of this reply. The preceding head's full CI passed; its Storybook and geometry results are not counted as new runs of this fix. The earlier performance distributions retain their original build identities. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head d86fe4f1e0dcba72e9bb8027f641b77e3cf9c714. I found no substantiated P0–P3 issue in this eight-file increment.
The prior live-tail P2 is addressed in apps/desktop/src/main/runtime-host-session-observer.ts:1646-1660: after a historical page reaches its target, the reader checks the current durable high-water mark and closes concurrent tail growth within the remaining answer budget; a full-budget or explicitly restored window stays parked. The new transcript-window tests cover both budgets, subsequent live appends, and reconnect bounds.
The four prior P3 paths are also addressed: apps/desktop/src/renderer/platform/desktop/desktop-transcript-range-store.ts:310-323 treats navigation cancellation of a background durability check as a non-error while retaining real failures; apps/desktop/src/main/runtime-host-desktop-candidate.ts:170,692 forwards the configurable initial read budget; packages/ui/src/chat-view.tsx:1051 hides Resume when later history exists; and desktop-transcript-range-store.ts:343-347 preserves an already-tail-covering positioned window when returning to latest. Focused tests exercise these paths.
Local Node 24 core/storage/runtime/runtime-host/UI and Desktop main builds passed, as did 97 focused Desktop/UI tests. The current-head hosted test check passed; merge-tree against fresh main 5ac266b1 and git diff --check are clean. I did not run real Electron long-session/Windows-WSL scenarios, the E2E or Storybook suites, or independently reproduce the PR's performance measurements. This is an incremental review, not a fresh full-depth review of all 55 changed files. GitHub still 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.
Adapt bounded transcript reads, complete export, and parked-window streaming handoff to Conversation ownership. Preserve one observation owner and verify bookmark/search restoration and paging.
There was a problem hiding this comment.
Reviewed the current head 8bf70dace65bb099319a4cc3779a804f0abbe0df, which merges main and adapts the existing bounded transcript-window implementation to Conversation ownership. The merge routes initial bookmarked/search Turns into the owned transcript opening (apps/desktop/src/renderer/features/conversation/controller/use-conversation-observation.ts:126-131), keeps parked-window durable tail settlement without publishing unseen messages (apps/desktop/src/renderer/features/conversation/model/transcript-commands.ts:40-49), and wires older/newer/latest paging through the transcript reader (apps/desktop/src/renderer/features/conversation/ui/conversation-readers.tsx:57-65). I also checked complete-export and restore/cancellation paths in the merge resolution. I found no substantiated new P0–P3 issue in this increment.
Node 24 build:test, 195 focused Desktop tests, and Desktop preload/main/renderer/stories typecheck passed, including indexed-window navigation, concurrent tail growth, parked-tail durability, export, and Conversation ownership. The typecheck initially failed because I installed with npm ci --ignore-scripts; applying the repository's required scripts/apply-dependency-patches.mjs resolved those errors. The current-head hosted test passed; merge with fresh main 9b089f58 and git diff --check are clean. I did not run packaged Electron, native platform navigation, or an independent full-suite/performance run.
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.


Summary
Fixes #5627. Refs #5680.
This PR reduces median return latency by 79.3% for an actively generating remote conversation and 93.9% for a very large idle local conversation. Restoring the start of a 1,000-Turn history improves by 89.7%, from 3.30 s to 339 ms. These are independent controlled comparisons collected during this PR's implementation, with the scope and build identities described below.
The changes avoid unnecessary full-history work and make transcript reads, transfers, and rendering follow the reader's current position. No new persistent table, index, projection, cross-page cache, or worker is introduced.
Measured improvements
Production Desktop on macOS / Apple M2 Pro / 16 GiB, using synthetic histories. Times below are medians (p50); lower is better.
Each comparison has 50 returns per side, except the existing-branch check (5 per side). The first two rows measure the earlier bounded-reader and resource-query changes; the remaining rows measure the later window/lazy-mount changes against an already optimized baseline. Baselines and display endpoints differ between those campaigns, so the reductions are within-row comparisons and must not be added together. These measure returns after startup, not first-Host-access or launch-to-ready time. The large histories and 1,000-tool Turn are stress scenarios.
Less CPU, transfer, and memory
CPU and memory figures cover the complete switch-away/return cycle and post-display observation. RSS peaks are sampled every 200 ms, not kernel high-water measurements. The evidence reports retain raw samples, exclusions, and per-process results: earlier switch comparisons, window and lazy-mount comparisons.
Main approach
The retained replica budget remains 64 MiB. Historical windows keep their displayed endpoint separate from the subscription high-water mark, never acknowledge unseen tail history, and never append across a gap. Forward reads close concurrent tail growth within the remaining answer budget before handing off to live updates. Cancelled navigation cannot overwrite a newer position or surface a spurious background-refresh error, and Resume is offered only when the window covers the session tail.
Copy/Save use a complete live view or a temporary full-history consumer that closes after use without replacing or acknowledging the displayed window. WorkHub still requires complete history for whole-session links and filtering; that exceptional path is not bounded to the initial visible range. Sending does not wait for history navigation before command admission.
Tradeoffs and measurement limits
The giant-Turn improvement defers work: it avoids 37,000 hidden descendant nodes and reduces switch-time Renderer CPU by 44.4%, but first expansion p50 increases 752.1 → 877.6 ms. Whole-cycle CPU including two expansions improves much less. Repeated Host projection of a huge Turn and oversized Markdown parsing remain outside this change.
Not every tail improves. In the first later-campaign run, recent-history p95 changes 501.4 → 554.6 ms and giant-Turn p95 2,544.5 → 3,102.2 ms; restoring the history start improves 3,470.6 → 455.9 ms. All completed slow samples are retained. A reversed-order recent-history comparison, another 50 returns per side, gives p50/p95 451.4/469.1 → 419.8/435.5 ms; it does not establish universal tail improvement. The earlier idle-local baseline includes four connection recoveries, so its 41.7 s p95 is not characterized as a normal interaction.
Benchmark scope, build identities, and additional checks
The earlier comparisons used
fb9df6c3dplus successive implementations before rebasing. They measure a useful-content DOM condition followed by two animation frames; the active case additionally requires continuing output. The Linux Host runs in Docker/LinuxKit with a macOS bind mount. +20 ms is injected RTT, not a measurement of WSL latency.The later comparisons use baseline
8d73d4eand require the target in the viewport, opacity at least 0.99, and two animation frames. Neither endpoint is a physical compositor timestamp or a guarantee of completely settled layout. OS caches were not flushed. These macOS controlled tests do not independently reproduce the original Windows/WSL workload or its approximate five-second settled-page milestone.Four-way ablation, 10/50/100/300-tool gradients, 100/300/1,000-Turn restore distances, an existing 300-Turn branch, combined giant-Turn restoration, and active/idle remote checks at 0/+20 ms added RTT are in the linked reports. A 1,000-Turn branch-creation preparation stalled with unknown outcome and produced no return samples; creation itself was not optimized.
Formal timing used identified candidates, followed by separately identified correctness fixes and package checks. A rebuilt delivery package on
929a30090rechecked each later case five times: recent history 453.0 ms, giant Turn 2,188.1 ms, and restoring the start 345.6 ms (p50). It also passed real-window restoration, paging in both directions, return-to-latest, and input integrity checks. Performance distributions remain attributed to their original builds; the latest review fixes did not rerun the 50-return campaigns.Verification
Current head:
8bf70dace65bb099319a4cc3779a804f0abbe0df(conflicts resolved againstmainatd7dffca98c5b9f878ef9b71b4dcda4122e03f1c6).Conflict-resolution validation on this head passes: workspace build and typecheck, 3,113 Desktop tests, 700 UI tests, 165 focused Runtime/Host tests, 121 architecture checker tests with the strict current-main comparison, Biome and repository guards, and the existing partial-history Electron journey. The bounded windows, complete export, and parked-reader streaming handoff now follow the Conversation owner introduced on main. The redundant paging wrapper was removed and the full affected suites still pass. Hosted CI for this head is pending; the performance campaigns above remain attributed to their original builds.
Hosted CI passed on the preceding head, including affected workspace tests, Runtime Host tests, Desktop E2E, Storybook smoke, transcript geometry, and installed CLI validation.
Earlier verification records remain available: Storybook fixes, geometry checks, and deletion ablation and the preceding head's successful CI. The linked reports retain the design, reproduction instructions, raw samples, build identities, and exclusions for their respective optimization stages.
AI use
OpenAI Codex assisted implementation, regression tests, benchmark investigation, validation, and this description at the contributor's direction.
Checklist
Does this PR entail a change in behavior?