fix(mobile): sidebar no longer re-registers shortcuts every render#22135

open2
main
GitHub fork: perf/mobile-sidebar-memo
opened7d agobyOoliver
View on GitHubTwo-way sync enabledPR last imported 6d ago

Note

🤖 Claude Opus 5.5 on behalf of Oliver

On mobile, every ThreadNavigationSidebar render rebuilt the thread list, handed LegendList a new data array, and re-registered the 9 thread jump shortcuts. Each re-registration re-rendered the app-root keyboard provider and sent native a new command list. After the React Compiler migration #21145, the list derivations stayed mutable across later hook calls, so the compiler silently dropped the cache holding listItems.

The fix restructures the sidebar so the compiler can cache it, and removes the four useMemos it still had:

  • Every hook in the pane now runs before the list derivations. A small useSidebarListFilters hook owns the filter state and project scopes, so the pane gets them as frozen hook results.
  • useThreadJumpShortcuts registers once on mount and reads the latest list through useEffectEvent, so it no longer depends on the list at all. HomeScreen uses the same hook and benefits too. The shortcuts now keep the priority they got at mount instead of jumping to the top on each render; only HomeScreen and the sidebar register them, and they're never on screen together.

Proof:

  • New test caches the sidebar thread list in apps/mobile/src/reactCompilerCoverage.test.ts checks that buildThreadListV2ListItems runs only on the cache-miss branch. It fails with main's sidebar and passes here.
  • New test threadKeyboardShortcuts.test.tsx checks that the hook registers once across re-renders and still jumps within the latest list. It fails on main's hook.
  • In a jsdom harness running the compiled sidebar, 20 unrelated renders caused 20 shortcut re-registrations, keyboard provider renders and LegendList data changes on main, and none here.
  • Ran vp test run on reactCompilerCoverage.test.ts (896 passed) and both keyboard tests, plus mobile typecheck.
  • Not checked: a real device or simulator. There is no visible UI change, so no screenshots.

Claude Opus 5.5 via Claude Code in T3 Code

Checks

20 successful · 9 skipped
All checks passed
d9866ac
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8e1881b9-f155-47ab-aafb-7bff39f58926
📥 Commits

Reviewing files that changed from the base of the PR and between 3487c8d190f95c5c2fc7d65443bdc8d398791d87 and d9866ac381357d267674c3297e702dfb1dfee296.

📒 Files selected for processing: 1
  • apps/mobile/src/reactCompilerCoverage.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The mobile keyboard code now exposes a shared hardware-command registration helper, and thread-jump shortcuts use effect-based registration. The thread sidebar consolidates filter derivation and reorganizes list setup and rendering. Tests cover shortcut registration and selection, plus sidebar compiler caching.

Changes: Mobile thread behavior

Layer / File(s)Summary
Command registration and thread-jump shortcuts
apps/mobile/src/features/keyboard/hardwareKeyboardCommands.ts, apps/mobile/src/features/keyboard/threadKeyboardShortcuts.ts, apps/mobile/src/features/keyboard/threadKeyboardShortcuts.test.tsx, apps/mobile/vite.config.ts
A shared helper registers and unregisters hardware keyboard commands. Thread-jump shortcuts use effect-based registration, and tests cover stable registration and selection from the latest thread list. The test file is included in the React test project.
Sidebar filters and list rendering
apps/mobile/src/features/threads/ThreadNavigationSidebar.tsx, apps/mobile/src/reactCompilerCoverage.test.ts
The sidebar moves filter and project-scope derivation into useSidebarListFilters and reorganizes derived values, handlers, timer setup, shortcut wiring, and list-item rendering. Compiler coverage checks sidebar list derivation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to d9866

The reviewed change is mergeable based on the available evidence.

Security architecture risk: ⚪ Minimal · up to ba58c

The change preserves existing navigation and project-selection boundaries. Registration cleanup remains owned by each mounted consumer, and shortcuts use current selection inputs without repeatedly registering. No material security risk was found in the assessed changes.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed registration lifetime affects thread-jump selection in the existing HomeScreen and sidebar consumers. The traced paths do not add service authority or a new externally reachable command interface.

Trust Boundaries and Controls

  • observed — Sidebar actions continue through existing parent-owned navigation and pull-request-opening consumers. The refactoring does not replace those consumers or bypass their existing routing decisions.

Resilience and Maintainability Implications

  • inferred — Effect-owned registration and per-handler cleanup preserve independent ownership across overlapping consumers and unmount/remount transitions. Interrupted renders do not initiate registration. These conclusions follow from the implementation and React effect semantics; the added test directly covers rerender stability and latest-list selection, not every lifecycle transition.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly identifies the primary fix: preventing repeated mobile sidebar shortcut registration.
Description checkPassedThe description clearly explains the problem, implementation, verification, test results, and unperformed device testing. It does not use the template headings or include an explicit maintainer approv…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, implementation, verification, test results, and unperformed device testing. It does not use the template headings or include an explicit maintainer approval comment, but it provides sufficient focused-fix and scope context.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Synced from GitHub

Approvability

Verdict: Approved at d9866ac

Macroscope's review found this PR approvable — This is a localized mobile performance fix that stops existing sidebar and home-screen thread shortcuts from being re-registered on every render while retaining access to current thread data. The added tests verify stable registration and latest-list dispatch, and the other changes are limited to compiler coverage and test configuration.

Notes:

  • All code in this push has already been reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

Added 2 commits

  • fix(mobile): sidebar no longer re-registers shortcuts every render96efdba
  • test(mobile): pin the compiler cache for the sidebar thread listd9866ac