You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(web): a mounted chat view no longer keeps every thread copy it replaced - #36390
No change needed this round. PR #36390 at f88d1a1 passed all 37 checks. Macroscope approved it with no line comments, and CodeRabbit generated no actionable comments. CodeRabbit marked its Linked Issues pre-merge check inconclusive because the issue also names optimistic-message, settings-persistence, lifecycle and send-hook callbacks as possible retention edges. That is not a high-rated finding. The diagnosis this fix answers to pins the leak on the compiler-memoized focusComposer/scheduleComposerFocus closures, and the full-app WeakRef retention test from the repro branch fails without this change and passes with it, so the reproduced chain is settled by the focus callbacks alone. CodeRabbit's note that the PR description's Evidence section is a placeholder is for the PR-writing step. The fix has no visible UI change, and the browser test is its proof. The fix moves focusComposer and scheduleComposerFocus into a useComposerFocus hook so they no longer close over ChatView's render scope, and ChatView passes only the stable versions on.
On origin/main f8dc37d I added the WeakRef retention test from open PR #31464 to apps/web/src/components/ChatView.browser.test.tsx. It renders the whole AppRoot in real Chromium with only the socket faked. For six rounds it opens a thread, streams a turn-item.updated event into it, takes a WeakRef to the thread's projection, and switches to another thread; then it forces a GC through CDP. vp test run src/components/ChatView.browser.test.tsx -t "lets go of the thread structures" failed in both runs on main: the projections from rounds 0, 1, 2 and 3 were still alive (expected []). With #31464's fix applied temporarily (stable focusComposer/scheduleComposerFocus in a useComposerFocus hook), the same test passed. Then I reverted the fix and committed only the test. I did not reproduce the reporter's own setup (desktop dev build, Replay threads, heap snapshots); the retention is shown through this full-app browser test.
Evidence
Real development web app in Chromium, 1280×800, light theme, fresh synthetic data on each commit. Stream six Replay turns while switching between two threads and typing drafts, keep chat mounted, then force CDP garbage collection. The displayed diagnostic reads WeakRefs to the actual thread projections; the latest two are excluded.
Recordings mask chat history to hide local workspace paths and append the inspected, unmasked result screenshot. Thread navigation, composer input, and generation controls remain visible.
The scenario comparison exited 0: failsOnBase=true, passesOnHead=true.
Not shown:
The reporter's desktop Electron setup and heap-snapshot retaining paths were not inspected; this proof directly demonstrates historical projection retention and collection in the real development web client.
Tests
Previous round: vp test run src/components/ChatView.browser.test.tsx passed 8/8, including the retention test, which failed before the fix; vp run check:changed passed. This round: CI on f88d1a1 passed all 37 checks.
Other closures in ChatView that the compiler memoizes may pin render scopes in the same way; I did not audit them beyond what this test reaches.
Auditing the other callbacks the issue names (optimistic-message, settings-persistence, lifecycle, send-hook) for similar render-scope retention; the reproduced chain is resolved by the focus callbacks alone.
Fails on main: ChatView's compiler-memoized focus callbacks chain old
render scopes, so replaced thread structures survive a GC (#36383).
Test taken from #31464.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…placed
ChatView defined focusComposer and scheduleComposerFocus in its own render,
so the React Compiler memoized them there and each one pinned that render's
scope, which held the previous render's handlers. Old renders and their
thread projections formed a chain freed only on unmount. Make both
callbacks in a small useComposerFocus hook instead, and pass only the
stable versions on, including to useComposerSelection.
Fixes#36383
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Macroscope's review found this PR approvable — The change moves composer-focus callbacks into a dedicated hook to reduce retained chat-render memory while preserving existing focus behavior and model-picker protection. Its effects are limited to client-side focus handling and memory lifetime.
You can add or adjust custom eligibility rules. Learn more.
No actionable comments were generated in the recent review.
Walkthrough
ChatView moves composer focus callbacks into useComposerFocus and uses its immediate and scheduled callbacks. A browser test checks thread-structure projection reachability after repeated updates and thread switches.
Changes: Chat focus and retention
Layer / File(s)
Summary
Composer focus callbacks and retention test apps/web/src/components/chat/useComposerFocus.ts, apps/web/src/components/ChatView.tsx, apps/web/src/components/ChatView.browser.test.tsx
useComposerFocus provides immediate and scheduled focus callbacks. ChatView uses these callbacks for artifact-template insertion and composer selection. The browser test checks which thread-structure projections remain reachable after repeated updates, thread switches, and garbage collection.
This change moves composer focus handling into a dedicated hook so that it no longer retains obsolete thread data. Focus behavior is intended to stay the same, and no merge-blocking risk was identified.
Pre-merge checks: 3 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Linked Issues check
[#36383] The issue asks that long-lived callbacks keep their semantics without retaining a growing chain of obsolete render data. It names several retention edges: focus, optimistic-message and settin…
Provide evidence that the named optimistic-message, settings-persistence, lifecycle, and send-hook callbacks no longer retain obsolete ChatView render scopes at head, or that they are not retention paths in the reproduced workload. Alternat…
Passed checks: 3 passed
Check name
Status
Explanation
Title check
The title "fix(web): a mounted chat view no longer keeps every thread copy it replaced" matches the main change. The PR moves focusComposer and scheduleComposerFocus into a useComposerFocus hook so th…
Out of Scope Changes check
The whole-PR diff from merge base f8dc37d to head f88d1a1 touches three areas. ChatView.tsx replaces the local focusComposer and scheduleComposerFocus closures with the values returned by useCompose…
Description check
The description states the problem as a retention leak in ChatView focus callbacks, with a repro on main, a fix that moves focusComposer and scheduleComposerFocus into useComposerFocus, and a Fixes li…
Full details: Title check
Explanation
The title "fix(web): a mounted chat view no longer keeps every thread copy it replaced" matches the main change. The PR moves focusComposer and scheduleComposerFocus into a useComposerFocus hook so that ChatView no longer retains replaced thread projections. The title is a conventional commit, is specific, and is one sentence. It is somewhat long at 75 characters.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff from merge base f8dc37d to head f88d1a1 touches three areas. ChatView.tsx replaces the local focusComposer and scheduleComposerFocus closures with the values returned by useComposerFocus, and removes the now-unused isModelPickerOpen import. The new useComposerFocus.ts hook contains the same focus logic moved out of the component. ChatView.browser.test.tsx adds the retention regression test. Each change directly supports the focusComposer retention fix or its test. No unrelated behavior change appears in the diff summary.
Full details: Description check
Explanation
The description states the problem as a retention leak in ChatView focus callbacks, with a repro on main, a fix that moves focusComposer and scheduleComposerFocus into useComposerFocus, and a Fixes link to issue #36383. It also lists tests run and the model and harness used. It is mostly complete. The Problem section is not explicit; the text starts with status notes and an argument about CodeRabbit and Macroscope comments, which is off-template. The Verification section has long evidence tables, but the Evidence section is a placeholder in the author's own words for a later step. The Scope section does not explain why the bug is a fix and not a feature, and it does not state a maintainer request. The description includes non-template material such as review-bot status and an 'Not in this change' list that concedes unaudited paths.
Full details: Linked Issues check
Explanation
[#36383] The issue asks that long-lived callbacks keep their semantics without retaining a growing chain of obsolete render data. It names several retention edges: focus, optimistic-message and settings-persistence callbacks, and lifecycle and send-hook callbacks. The PR changes only the focus path. At head, apps/web/src/components/chat/useComposerFocus.ts defines stable focusComposer and scheduleComposerFocus callbacks, and ChatView.tsx passes the stable versions to useComposerSelection, insertArtifactTemplate, and the subscription. Focus behavior matches the removed code: focusAtEnd on the current handle, and a requestAnimationFrame deferral that skips focus when isModelPickerOpen() is true. The author reports a full-app browser test (ChatView.browser.test.tsx, 'lets go of the thread structures') that failed before the change and passed after it. That test was not run in this review. The PR does not address or audit the optimistic-message, settings-persistence, lifecycle, or send-hook callback edges named in the issue. The issue's desktop Replay workload and production ownership are also unverified by the PR.
Resolution
Provide evidence that the named optimistic-message, settings-persistence, lifecycle, and send-hook callbacks no longer retain obsolete ChatView render scopes at head, or that they are not retention paths in the reproduced workload. Alternatively, confirm the issue scope is limited to the focusComposer edge. Include the result of running the full-app browser retention test at head, since it was not run here.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size:M30-99 changed lines (additions + deletions).vouch:trustedAuthor is trusted by repo permissions or the VOUCHED list.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
No change needed this round. PR #36390 at f88d1a1 passed all 37 checks. Macroscope approved it with no line comments, and CodeRabbit generated no actionable comments. CodeRabbit marked its Linked Issues pre-merge check inconclusive because the issue also names optimistic-message, settings-persistence, lifecycle and send-hook callbacks as possible retention edges. That is not a high-rated finding. The diagnosis this fix answers to pins the leak on the compiler-memoized focusComposer/scheduleComposerFocus closures, and the full-app WeakRef retention test from the repro branch fails without this change and passes with it, so the reproduced chain is settled by the focus callbacks alone. CodeRabbit's note that the PR description's Evidence section is a placeholder is for the PR-writing step. The fix has no visible UI change, and the browser test is its proof. The fix moves focusComposer and scheduleComposerFocus into a useComposerFocus hook so they no longer close over ChatView's render scope, and ChatView passes only the stable versions on.
Fixes #36383
Reproduction on main
On origin/main f8dc37d I added the WeakRef retention test from open PR #31464 to apps/web/src/components/ChatView.browser.test.tsx. It renders the whole AppRoot in real Chromium with only the socket faked. For six rounds it opens a thread, streams a turn-item.updated event into it, takes a WeakRef to the thread's projection, and switches to another thread; then it forces a GC through CDP.
vp test run src/components/ChatView.browser.test.tsx -t "lets go of the thread structures"failed in both runs on main: the projections from rounds 0, 1, 2 and 3 were still alive (expected []). With #31464's fix applied temporarily (stable focusComposer/scheduleComposerFocus in a useComposerFocus hook), the same test passed. Then I reverted the fix and committed only the test. I did not reproduce the reporter's own setup (desktop dev build, Replay threads, heap snapshots); the retention is shown through this full-app browser test.Evidence
Real development web app in Chromium, 1280×800, light theme, fresh synthetic data on each commit. Stream six Replay turns while switching between two threads and typing drafts, keep chat mounted, then force CDP garbage collection. The displayed diagnostic reads WeakRefs to the actual thread projections; the latest two are excluded.
Recordings mask chat history to hide local workspace paths and append the inspected, unmasked result screenshot. Thread navigation, composer input, and generation controls remain visible.
Scenario source · Chromium launcher · Runner configuration
The scenario comparison exited 0: failsOnBase=true, passesOnHead=true.
Not shown:
Tests
Previous round: vp test run src/components/ChatView.browser.test.tsx passed 8/8, including the retention test, which failed before the fix; vp run check:changed passed. This round: CI on f88d1a1 passed all 37 checks.
Not in this change
Who did the work
Run by the t3code factory in T3 Code.