refactor: trim redundant optimizations from the perf PR#3928
Conversation
Drops the pieces whose cost isn't justified by a measurable win, keeping every hot-path fix: - Timeline k-way merge: revert to concat+toSorted. TimSort is near-linear on this nearly-sorted input, and the derivation already runs behind a useMemo; the PR kept the toSorted fallback anyway, so the merge was a second implementation of the same ordering. - useDesktopLocalBootstraps: keep the equality bail (the actual fix for per-tick re-renders) inside the existing hook instead of a module-global store + refcounted shared interval for two consumers. - Hover prewarm controller: remove the unused setTimeoutFn/clearTimeoutFn injection params (neither the component nor the tests pass them). - Composer partialize: drop the per-draft WeakMap cache. Debounced JSON serialization already removed the keystroke-path cost; the remaining shallow clones are too small to warrant a second caching layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes update four web behaviors: sidebar hover timer handling, composer draft persistence, desktop bootstrap polling, and session timeline ordering. ChangesSidebar hover prewarm
Composer draft persistence
Desktop bootstrap polling
Timeline ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c12e50d. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/connection/useDesktopLocalBootstraps.ts (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment is stale — no longer describes actual behavior.
The JSDoc still claims "so every renderer consumer reads the same topology", but the rewrite moved topology state and polling from a shared store into per-hook
useState/useEffect. Each hook instance now independently readsreadDesktopSecondaryBootstraps()on its own interval and keeps its own reference viabootstrapsEqual, so multiple consumers no longer share a single topology or reference — they poll and settle independently. Update the comment to reflect the new per-hook polling model.📝 Suggested doc fix
- * each poll returns a fresh array, so the previous reference is kept when the - * topology is unchanged to avoid re-rendering consumers every tick. Use this - * instead of polling the bridge ad hoc so every renderer consumer reads the - * same topology. + * each poll returns a fresh array, so the previous reference is kept when the + * topology is unchanged to avoid re-rendering this consumer every tick. Each + * call to this hook polls independently (no shared subscription), so use it + * per consumer rather than relying on a shared/synchronized topology. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/connection/useDesktopLocalBootstraps.ts` around lines 32 - 36, Update the JSDoc above the desktop bootstrap hook to describe its current per-hook useState/useEffect polling behavior, including that each hook independently calls readDesktopSecondaryBootstraps() and retains its reference via bootstrapsEqual. Remove the claim that renderer consumers share one topology or reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/connection/useDesktopLocalBootstraps.ts`:
- Around line 37-53: Replace the per-instance useState and polling in
useDesktopLocalBootstraps with a shared external snapshot/store so all consumers
read the same bootstrap list. Centralize polling and deduplicate updates using
bootstrapsEqual, then have the hook subscribe to that store and return its
current snapshot while preserving cleanup behavior.
---
Nitpick comments:
In `@apps/web/src/connection/useDesktopLocalBootstraps.ts`:
- Around line 32-36: Update the JSDoc above the desktop bootstrap hook to
describe its current per-hook useState/useEffect polling behavior, including
that each hook independently calls readDesktopSecondaryBootstraps() and retains
its reference via bootstrapsEqual. Remove the claim that renderer consumers
share one topology or reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49b1ca47-505e-49f9-a60e-dcf69f5fdb37
📒 Files selected for processing (5)
apps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/composerDraftStore.tsapps/web/src/connection/useDesktopLocalBootstraps.tsapps/web/src/session-logic.ts
💤 Files with no reviewable changes (1)
- apps/web/src/composerDraftStore.ts
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved This refactor removes optimization complexity (caching, shared polling, k-way merge) in favor of simpler implementations. The changes are purely deletions and simplifications with no functional behavior changes - only performance trade-offs where simplicity was preferred over micro-optimizations. You can customize Macroscope's approvability policy. Learn more. |
Addresses review: readDesktopSecondaryBootstraps (bridge IPC) now runs before the state update instead of inside the updater, which React may invoke more than once. Doc comment no longer claims a shared topology snapshot across consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate bec10f9
1 participant
Follow-up to #3825, based on that branch so the diff shows only the delta. Drops the pieces of the perf PR whose complexity isn't justified by a measurable win, while keeping every hot-path fix (aggregate-indexed replay, bounded shell catch-up, incremental projections, O(append) reducer, skip-persist-while-running, debounced draft serialization, hover prewarm, shared activity ordering cache).
What's trimmed
Timeline k-way merge → back to
concat + toSorted(~45 lines). V8's sort is TimSort, which is near-linear on nearly-sorted input — and this input is three already-sorted arrays concatenated. The derivation also runs behind auseMemoin ChatView, so it fires per event, not per render. The PR retained thetoSortedfallback path regardless, so the merge was a second implementation of the same ordering. The real win insession-logic.ts— the sharedorderActivitiescache that stops four derivations re-sorting the full activity history — is kept.useDesktopLocalBootstraps: equality bail instead of a module-global store (~40 lines). There are exactly two consumers (Sidebar, CommandPalette), so collapsing two 2s intervals into one shared refcounted poller saves nothing real. The actual fix — not re-rendering when the topology is unchanged — now lives inside the original hook assetBootstraps(prev => bootstrapsEqual(prev, next) ? prev : next).Hover prewarm: dead injection params removed.
setTimeoutFn/clearTimeoutFnwere copied from the jump-hint controller's pattern but nothing passes them — the component uses defaults and the tests use fake timers. The controller and its tests are otherwise kept as-is.Composer
partialize: per-draft WeakMap cache dropped (~17 lines). Two mechanisms addressed the same keystroke path;createDebouncedJsonStorage(kept) removes the per-keystrokeJSON.stringify, which was the dominant cost. What remains without the cache is shallow-cloning small context arrays for retained drafts — not worth a second caching layer unless measured otherwise.Verification
pnpm run typecheckcleanSidebar.logic,composerDraftStore,session-logictest files: 193 passedpnpm run lint: same 30 pre-existing warnings as the base branch, none introduced🤖 Generated with Claude Code
Note
Remove redundant optimizations from perf-related code in sidebar, draft store, and timeline
createSidebarHoverPrewarmControllerin Sidebar.logic.ts, using globalsetTimeout/clearTimeoutdirectly.WeakMapdraft cache (partializedDraftCache) from composerDraftStore.ts; partialized drafts are now recomputed on every call.useSyncExternalStore-based polling in useDesktopLocalBootstraps.ts with per-instanceuseState/useEffectintervals; reference equality is preserved when topology is unchanged.deriveTimelineEntriesin session-logic.ts by removing the k-way merge path and always usingtoSortedon the concatenated rows.Macroscope summarized bec10f9.
Note
Low Risk
Simplifications with no auth or data-model changes; the only behavioral nuance is two independent bootstrap poll intervals instead of one shared poller, which the authors judged negligible for two consumers.
Overview
Follow-up cleanup that removes complexity from the recent perf work without undoing the meaningful hot-path wins (debounced draft persistence, hover prewarm, shared activity ordering cache, etc.).
deriveTimelineEntriesinsession-logic.tsno longer uses a custom k-way merge over three sorted arrays; it concatenates andtoSortedbycreatedAtinstead (~45 lines removed).useDesktopLocalBootstrapsdrops the module-globaluseSyncExternalStorerefcounted poller. Each mount now runs its own 2s interval and only updates state whenbootstrapsEqualsays the topology changed (Sidebar + Command Palette = two pollers).Composer draft
partializeno longer keeps a per-draftWeakMapcache; unchanged drafts are recomputed on each partialization, relying oncreateDebouncedJsonStoragefor the heavy cost.createSidebarHoverPrewarmControllerno longer accepts injectable timer functions; it uses globalsetTimeout/clearTimeoutandReturnType<typeof setTimeout>for the handle type. Sidebar hover prewarm behavior is unchanged; a comment inSidebar.tsxis clarified.Reviewed by Cursor Bugbot for commit bec10f9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Performance
Documentation