🤖 perf: let React Compiler compile RightSidebar, ReviewPanel, and the startup gate - #4429
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6f0c7befb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4adb57e to
581b09c
Compare
0082353 to
999a465
Compare
999a465 to
80c9520
Compare
## Summary `ChatInputInner` now compiles under React Compiler, and the guard's `KNOWN_SKIPPED` baseline drops it. The audit shows no `CompileError` for it. ## Background After coder#4425, three kinds of blockers were left: a `react-hooks/exhaustive-deps` suppression, ref writes during render, and manual `useCallback`/`useMemo` calls whose dependencies the compiler could not preserve (the compiler skips the whole component when it can't keep an existing manual memo). ## Implementation - **Edit-mode effect (lint suppression removed).** It used to depend only on `editingMessage?.id` and hid the draft callbacks from its deps. It now lists all deps and uses an applied-edit-id ref, so it still applies the edit draft once per edit target and never clobbers in-progress edit text. The ref resets when editing ends, so re-editing the same message re-applies it, as before. - **Render-time ref writes → layout effects.** `editingMessageIdRef` and `handleSendRef` are now synced in `useLayoutEffect`. Layout effects run in the same task as the commit, so no async callback can observe the old value after the commit, which is the guarantee the existing comment asked for. - **Manual memos removed** where the compiler rejected them: `handleToastDismiss`, `idleCompactionProps`, `setPreferredModel`, `cycleToNextModel`. The compiler memoizes these now. This follows AGENTS.md ("don't hand-memoize") and adds no new manual memoization. - The global keydown listener reads `cycleToNextModel` through a latest-value ref instead of re-subscribing when it changes (same pattern as `composerApiRef` in this file). ## Risks Medium-low. Edit-mode entry and the global model-cycle shortcut are the behavior-sensitive paths; both keep their semantics. The memoization is now compiler-managed. ## Validation - Guard: `10/18 hot components compile (8 known skipped)`. - `tests/ui/chat`, `tests/ui/compaction`, ChatInput unit tests, `useAIViewKeybinds` tests pass. - Perf (Electron perf e2e, local, serial, 3 runs each): see Measurements. ## Measurements Electron perf e2e (`tests/e2e/scenarios/perf.*.spec.ts`, `XUM_PROFILE_REACT=1`), local, serial, 3 runs each, medians. Baseline is `main` at 349657d; "after" is the top of this stack (coder#4429), because the layers were measured together. React time is the summed `actualDuration` of the profiled subtrees. | Scenario | Profiled subtree | main | stack top | | --- | --- | --- | --- | | Open chat with a 1000-file review | `chat-pane.transcript` | 388 ms | 292 ms | | Open chat with a 1000-file review | `chat-pane.input` | 76 ms | 61 ms | | Open chat with a 1000-file review | all profiled | 530 ms | 424 ms | | Open workspace, large history | `chat-pane.transcript` | 331 ms | 295 ms | | Open workspace, large history | `chat-pane.input` | 52 ms | 46 ms | | Open workspace, medium history | `chat-pane.transcript` | 206 ms | 191 ms | Commit counts are unchanged; the gain is less work per commit. Typing in a large chat and the review hunk specs show no change beyond noise. ChatInput alone, measured before coder#4418 landed: New Workspace typing went from 165 `CreationControls` renders per run to 2. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$4.05`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=4.05 -->
## Summary `ChatPaneContent` now compiles under React Compiler and leaves the guard baseline. ## Implementation - `handleSendQueuedImmediately` threw inside `try/catch` (unsupported). The interrupt call now goes through `runWithCatch`, which releases the in-flight guard and rethrows on error. The failure-result path clears the guard and throws outside any `try`, so the guard is cleared once instead of twice (clearing is idempotent). - `handleEditQueuedMessage` and `handleSendQueuedImmediately` drop their `useCallback` wrappers. Their optional-chaining deps (`workspaceState?.queuedMessage`) did not match what the compiler inferred, so it refused to compile the component. The compiler memoizes both now. ## Validation - Guard: `11/18 hot components compile (7 known skipped)`. - ChatPane unit tests and `tests/ui/chat` pass. ## Measurements Electron perf e2e (`tests/e2e/scenarios/perf.*.spec.ts`, `XUM_PROFILE_REACT=1`), local, serial, 3 runs each, medians. Baseline is `main` at 349657d; "after" is the top of this stack (coder#4429), because the layers were measured together. React time is the summed `actualDuration` of the profiled subtrees. | Scenario | Profiled subtree | main | stack top | | --- | --- | --- | --- | | Open chat with a 1000-file review | `chat-pane.transcript` | 388 ms | 292 ms | | Open chat with a 1000-file review | `chat-pane.input` | 76 ms | 61 ms | | Open chat with a 1000-file review | all profiled | 530 ms | 424 ms | | Open workspace, large history | `chat-pane.transcript` | 331 ms | 295 ms | | Open workspace, large history | `chat-pane.input` | 52 ms | 46 ms | | Open workspace, medium history | `chat-pane.transcript` | 206 ms | 191 ms | Commit counts are unchanged; the gain is less work per commit. Typing in a large chat and the review hunk specs show no change beyond noise. ChatInput alone, measured before coder#4418 landed: New Workspace typing went from 165 `CreationControls` renders per run to 2. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$4.05`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=4.05 -->
…ge (coder#4428) ## Summary `MessageRenderer` and `AssistantMessage` now compile under React Compiler and leave the guard baseline. Both render once per transcript row. ## Implementation - `MessageRenderer` reassigned its destructured `message` prop (`message = useStreamingMessageDelta(...)`), which the compiler can't lower. The prop is now destructured as `messageProp`, and the streamed value is a new `const message`. - `AssistantMessage`'s fork handler had `??` inside `try/catch`; it now uses `runWithCatch` with the same body and error handler. ## Validation - Guard: `13/18 hot components compile (5 known skipped)`. - `src/browser/features/Messages` unit tests (incl. `MessageRenderer.test.tsx`) and `tests/ui/chat/forkFromResponse.test.ts` pass. ## Measurements Electron perf e2e (`tests/e2e/scenarios/perf.*.spec.ts`, `XUM_PROFILE_REACT=1`), local, serial, 3 runs each, medians. Baseline is `main` at 349657d; "after" is the top of this stack (coder#4429), because the layers were measured together. React time is the summed `actualDuration` of the profiled subtrees. | Scenario | Profiled subtree | main | stack top | | --- | --- | --- | --- | | Open chat with a 1000-file review | `chat-pane.transcript` | 388 ms | 292 ms | | Open chat with a 1000-file review | `chat-pane.input` | 76 ms | 61 ms | | Open chat with a 1000-file review | all profiled | 530 ms | 424 ms | | Open workspace, large history | `chat-pane.transcript` | 331 ms | 295 ms | | Open workspace, large history | `chat-pane.input` | 52 ms | 46 ms | | Open workspace, medium history | `chat-pane.transcript` | 206 ms | 191 ms | Commit counts are unchanged; the gain is less work per commit. Typing in a large chat and the review hunk specs show no change beyond noise. ChatInput alone, measured before coder#4418 landed: New Workspace typing went from 165 `CreationControls` renders per run to 2. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$4.05`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=4.05 -->
## Summary `ImmersiveReviewView` now compiles under React Compiler and leaves the guard's `KNOWN_SKIPPED` baseline (17/18 hot components compile). Iterating hunks in immersive review spends about 18% less script time. ## Background Follow-up to the React Compiler coverage stack (coder#4422, coder#4425–coder#4429) and coder#4439. The compiler skipped the whole component because of `??=`, a `try/catch/finally` in the copy-file handler, three render-time ref writes, and one ref read during render. ## Implementation - `oldLineMap ??= …` becomes `oldLineMap = oldLineMap ?? …`. - The copy-file handler's `try/catch/finally` uses `runWithCatchFinally` with the same body, error handler, and cleanup. Early `return`s keep their meaning because the `try` was the handler's last statement. - `activeFilePathRef`, `activeFileContentVersionRef`, and `handleCopyFileRef` are synced in `useLayoutEffect` instead of during render. Layout effects run in the commit's task, before passive effects, which is the ordering the existing comment requires for `isStale()`. - `selectedLineSummary` (render path) read `hunkJumpLineRangeRef` through `getCurrentLineSelection`. A state mirror, `hunkJumpLineRange`, now feeds render. Every ref write also sets the state, next to the cursor/selection updates it batches with, and the hot-path callbacks keep reading the ref, so their identities stay stable. One visible difference: after a comment submit or composer cancel that clears the hunk-jump range while no line selection is active, the "Lines …" label now updates right away. Before, it kept showing the cleared range until the next render. ## Validation - Guard: `17/18 hot components compile (1 known skipped)`; `make static-check` green. - `src/browser/features/RightSidebar/CodeReview` unit tests (79) and `tests/ui/review` (14) pass. ## Measurements Electron perf e2e, local, serial, 5 runs each, `main` at 334a5a6 vs this branch. ScriptDuration per run: | Scenario | main (sorted runs) | this PR (sorted runs) | Median | | --- | --- | --- | --- | | Immersive hunk iteration, 1500 lines / 150 hunks | 343, 351, 355, 356, 440 ms | 270, 273, 290, 294, 299 ms | 355 → 290 ms | | Immersive mark-read, 1000 files | 14, 16, 18, 19, 19 ms | 14, 14, 20, 25, 26 ms | 18 → 20 ms (noise) | Hunk-iteration wall time went from 448 to 376 ms (median). The regular review reopen scenario, which doesn't render this component, moved about 10% between runs, which is this host's noise level. The hunk-iteration ranges don't overlap. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$4.05`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=4.05 -->
Summary
RightSidebarComponent,ReviewPanel, andUserPreferencesStartupGate(AppLoader) now compile under React Compiler and leave the guard baseline. Two hot components remain skipped and are deferred (see below).Implementation
iin asetLayoutclosure while incrementing it (i++on a captured variable is unsupported); the closure now captures a per-iterationtabIndex.layoutRawRefis synced in a layout effect instead of during render. The terminal session sync effect keeps running only on workspace change. Instead of hidinglayoutbehind aneslint-disable, it snapshots the layout through a ref when the request starts, which is the same layout the old closure captured, so behavior is unchanged.runWithCatchFinally/runWithCatch. Four latest-value refs (isReadRef,selectedHunkIdRef,showReadHunksRef,filteredHunksRef) are synced in layout effects instead of during render.isRefreshBlockedstate andcontrollerRefare declared before the callback that uses them (the compiler rejects use-before-declaration).bootstrappedRef.current || ready; the ref is set together withready, soreadyalone decides.Deferred (tracked in the stack's final report)
ProjectSidebarInner: three render-time mutable session caches (sticky workflow-group expansion, retained workflow groups, run names) must become immutable React state. That is a behavioral refactor with its own tests, not a syntax fix.ImmersiveReviewView:selectedLineSummaryreadshunkJumpLineRangeRefduring render; fixing it means moving the hunk-jump range into state, which changes render timing on the hunk-iteration hot path.Validation
16/18 hot components compile (2 known skipped).src/browser/features/RightSidebarandsrc/browser/components/AppLoaderunit tests;tests/ui/review,tests/ui/rightSidebar,tests/ui/layout(11 suites, 79 tests) pass.Measurements
Electron perf e2e (
tests/e2e/scenarios/perf.*.spec.ts,XUM_PROFILE_REACT=1), local, serial, 3 runs each, medians. Baseline ismainat 349657d; "after" is the top of this stack (#4429), because the layers were measured together. React time is the summedactualDurationof the profiled subtrees.chat-pane.transcriptchat-pane.inputchat-pane.transcriptchat-pane.inputchat-pane.transcriptCommit counts are unchanged; the gain is less work per commit. Typing in a large chat and the review hunk specs show no change beyond noise. ChatInput alone, measured before #4418 landed: New Workspace typing went from 165
CreationControlsrenders per run to 2.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$4.05