test(desktop): cover the review store - #66034
Merged
Merged
Conversation
Follow-up to #65890 (router transitions off) and #65898 (structural compare + first-paint budget): profiling the switch path on real 1000+- message sessions with a new CDP harness showed the remaining freeze is NOT markdown rendering — it's a forced-reflow cascade from mount-time layout reads interleaved with style writes across the transcript's layout effects, plus the first-paint budget cut landing too late to stop the full-budget commit. Measured on the two largest local sessions (996 and 1363 messages), main-thread longtask totals per switch: warm 2450ms -> 557ms and 1158ms -> 194ms; first paint 1690ms -> 444ms. Harness: scripts/profile-session-switch.mjs (same CDP family as profile-real-stream.mjs). - use-resize-observer: drop the synchronous initial callback and ride the observer's spec-guaranteed first delivery instead (same frame, after layout, before paint). The sync call ran while the commit's layout was dirty, so every size read in a callback forced a full reflow — with one instance per user bubble (measureClamp read scrollHeight, then WROTE --human-msg-full, re-dirtying layout for the next bubble), the switch commit thrashed for over a second. Inside RO timing the same reads are free. Composer metrics (2x getBoundingClientRect + documentElement style writes) rides the same fix. - Same class, same fix at the remaining call sites profiling surfaced: ExpandableBlock and TerminalOutput (dozens per tool-heavy transcript) now measure/pin via RO initial delivery; the tool-window and thinking-preview pins drop their sync pin() call; the thread timeline's initial active-tick compute joins its existing scroll-time rAF batching so back-to-back transcript updates coalesce. - thread/list: cut the render budget in the RENDER phase (state-from- props adjustment) instead of the post-commit layout effect. The effect-time cut was too late — on a warm switch React first built and committed the full 300-part tree, then re-rendered at 60, then bumped back to 300, so the expensive commit still happened (and on a cold switch the bump rAF usually fired while the transcript was still empty, so the prefetched messages rendered at full budget anyway). The render-phase cut restarts the component before any child renders; a second trigger handles the cold path where messages land later under the same sessionKey. - thread/list: backfill 60 -> 300 inside startTransition so the older turns' markdown+shiki render is interruptible background work instead of a synchronous freeze one frame after the switch paints. Functional Math.max so an urgent "Show earlier" click can't be rebased back down. - composer focus: skip the rAF/timeout focus retries when the element is already focused — focus() runs the full focusing steps (forcing layout) even on the active element, ~585ms per switch on a large dirty DOM. - Replace the tautological render-budget test (it re-declared the constants locally and asserted 60 < 300) with behavior tests of the now-exported buildGroups + firstVisibleGroupIndex. Verification: apps/desktop `npx tsc --noEmit` clean; full `npx vitest run` 210 files / 1763 passed; manual CDP check confirms the deferred backfill commits the full transcript, stays pinned to bottom, and "Show earlier" still pages.
OutThisLife
force-pushed
the
bb/review-store-tests
branch
from
July 17, 2026 01:51
26d4d6a to
f6edcb3
Compare
alt-glitch
added
type/test
Collaborator
tonydwb
reviewed
Jul 17, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Desktop test coverage (817 lines). Covers the review store.
Note: diff contains console.log statements in a profiling script (apps/desktop/scripts/profile-session-switch.mjs). These are development diagnostics in the test scaffolding, not production debug artifacts. Not blocking.
Checked diff — no hardcoded secrets, no security concerns.
Looks good. No blocking issues.
Reviewed by Hermes Agent
tonydwb
reviewed
Jul 17, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary (Re-review)
Verdict: LGTM / Comment
Test coverage PR for the review store in the desktop app. Additions only, no production code changes. Prior COMMENT review confirmed. No issues found.
Gravezzz
pushed a commit
to Gravezzz/hermes-agent
that referenced
this pull request
Jul 21, 2026
…re-tests test(desktop): cover the review store
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…re-tests test(desktop): cover the review store
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What
Adds unit test coverage for
apps/desktop/src/store/review.ts(the review pane store), which previously had none.Coverage
35 tests across the store's public surface:
refreshReview— closed-pane no-op, not-a-repo/no-bridge path (clears the loading skeleton), populate from bridge, excluded-path filtering (node_moduleset al.), dropping a selection whose file vanished, and error handling (clears list, keepsisRepo).$reviewMaxChurn— largest added+removed across files; 0 for empty.selectReviewFile/clearReviewSelection— sets path + fetches diff, coerces falsy diff to''(notnull), no-bridge path, full clear.toggleReviewTreeMode,openReview,closeReview.stage/unstage/revertforward the path (incl. thenull= "all" case) and re-sync.requestRevert/cancelRevert/confirmRevert, including the{ path: null }("revert all") vsundefined("closed") distinction.commitChanges(trims message, toggles busy, bails on blank),pushChanges,createOrOpenPr(open existing vs create-then-open).refreshShipInfo— populate, reset on no-bridge, reset on throw.generateCommitMessage— one-shot from diff, empty on blank diff, empty whencommitContextis absent.$reviewCommitDefault— remembers the split-button default.Follows the sibling
coding-status.test.tsconventions: stubwindow.hermesDesktop.git.reviewper-test;requestOneShotandrefreshRepoStatusmocked viavi.mock.Verification
npx vitest run --environment jsdom src/store/review.test.ts→ 35 passednpx tsc -b→ clean