Skip to content

perf(mobile): branch picker stops rendering twice per update - #22132

Open
flamboh wants to merge 1 commit into
pingdotgg:mainfrom
flamboh:perf/mobile-branch-picker-memo
Open

flamboh wants to merge 1 commit into
pingdotgg:mainfrom
flamboh:perf/mobile-branch-picker-memo

Conversation

@flamboh

@flamboh flamboh commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Note

Part of a series fixing values the React Compiler stopped caching after the mobile migrations. Each PR restructures the code so the compiler can cache it, without adding useMemo or useCallback. Each is independent and can merge in any order:

Note

🤖 Claude Opus 5.5 on behalf of Oliver

Problem

The mobile React Compiler migration #21137 left usePaginatedBranches (apps/mobile/src/state/queries.ts) building its combined pagesAtom with Atom.make(...) during render. The compiler can't cache it: the value stays live across the later useAtomValue call, and a cache block can't span a hook call, so the compiler drops that scope without a build error. Every render therefore creates a new atom, useAtomValue resubscribes, and the caller renders a second time. This affects NewTaskFlowProvider and the scheduled-task branch picker.

Fix

Move the atom out of render. A module-level Atom.family keyed by a JSON string of [environmentId, cwd, query, cursors] returns the same atom for equal inputs, so render no longer needs a cache. This follows the repo's string-key family pattern (vcsEnvironment.listRefs already keys its family the same way):

const branchPagesAtom = Atom.family((key: string) => {
  const [environmentId, cwd, query, cursors] = JSON.parse(key) as [/* … */];
  const pageAtoms = cursors.map((cursor) => branchPageAtom(environmentId, cwd, query, cursor));
  return Atom.make((get) => pageAtoms.map((atom) => get(atom))).pipe(/* label */);
});

const results = useAtomValue(
  pagesKey === null ? EMPTY_BRANCH_PAGES_ATOM : branchPagesAtom(pagesKey),
);
  • No memo hooks. The hook has no useMemo, useCallback, or ref/state memo stand-ins.
  • Explicit initial cursor. The initial cursor is null instead of undefined, so it survives the JSON key unchanged.
  • Refresh. refresh looks up the first-page atom through the same branchPageAtom helper, so it no longer depends on a per-render array.
  • Memory. The combined atom isn't keepAlive, so the registry releases its node once no component subscribes. Atom.family holds entries by WeakRef, so an unused key is garbage-collected like the existing listRefs entries.

Verification

  • Compiler output. Compiled queries.ts with babel-preset-expo using Metro's iOS release caller flags and a compiler logger. The atom passed to useAtomValue now comes from the family, so it keeps the same identity across renders without a cache slot. refresh is now cached (deps query, setPagination, target, targetKey). On main it changed identity on every render. The one pruned block in usePaginatedBranches is unchanged from main: the string query/targetKey computation before the hooks, which doesn't need a cache. No new CompileError events, and no file was added to KNOWN_BAILOUTS.

  • Render counts. A jsdom harness compiled the real usePaginatedBranches source from each branch with the same compiler preset, then ran it under @effect/atom-react with a plain AtomRegistry.make() provided through RegistryContext, the same setup as App.tsx. Only vcsEnvironment.listRefs was stubbed. Results matched across 3 runs:

    main this PR
    Renders on mount 2 2
    Renders over 5 parent updates 10 5
    refresh identity changes 11 0
  • Checks. vp test run apps/mobile/src/reactCompilerCoverage.test.ts passed (887). queries.test.ts and queries.filesystem.test.ts passed. Mobile typecheck passed. vp lint on queries.ts is clean.

  • Not checked. No simulator or device run.

Claude Opus 5.5 via Claude Code in T3 Code

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 20, 2026
@github-actions github-actions Bot added the size:S 10-29 changed lines (additions + deletions). label Oct 20, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 20, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 5fea58d

Macroscope's review found this PR approvable — This is an isolated mobile performance refactor that stabilizes paginated branch-query atoms and removes redundant rendering without changing branch data, pagination semantics, or product defaults. The limited runtime impact and contained scope make it low risk.

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

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

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: 9d634c4d-9282-4349-a819-c41a76ff673e
📥 Commits

Reviewing files that changed from the base of the PR and between e0f853c and 5fea58d.

📒 Files selected for processing: 1
  • apps/mobile/src/state/queries.ts

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

📝 Walkthrough

usePaginatedBranches now uses shared keyed atoms for branch-page fetching and aggregation. Initial cursors use null. Refresh targets the first shared branch-page atom.

Changes: Branch pagination

Layer / File(s) Summary
Shared branch page atoms
apps/mobile/src/state/queries.ts
The pagination state uses null for initial cursors. Shared atoms fetch and aggregate branch pages by serialized target key. The hook uses the keyed atom family and refreshes the first shared page atom when the environment and working directory exist.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 5fea5

The pagination change is ready to merge after normal checks; no branch-loading or refresh regression was identified.

Security architecture risk: 🔵 Low · up to 5fea5

The shared state remains separated by environment, repository directory, search query, and page cursors. No introduced security defect was identified, and the existing request authority is preserved. Upstream access enforcement was not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated change affects branch-reference reads in the mobile new-task and scheduled-task pickers. Aggregate sharing is limited to identical environment, directory, query, and cursor keys; the inspected change adds no request authority or repository mutation. Upstream endpoint exposure and authorization remain unverified.

Trust Boundaries and Controls

  • observed — Page requests continue through the existing environment-scoped runtime to vcsListRefs. The aggregate key preserves the same environment and request-selection fields used by the underlying page family. This supports unchanged client-side isolation, but does not establish that upstream access enforcement is sufficient.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the mobile branch picker performance fix and the duplicate rendering behavior it addresses.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains the problem, the fix, and detailed verification results. The “Fix” section covers the required “Change” content. The required “Scope and approval” section is not expli…
Full details: Description check

Explanation

The description clearly explains the problem, the fix, and detailed verification results. The “Fix” section covers the required “Change” content. The required “Scope and approval” section is not explicitly included, and no maintainer approval or approval exemption is documented.

✨ 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.

@flamboh
flamboh force-pushed the perf/mobile-branch-picker-memo branch from e0f853c to 5fea58d Compare October 20, 2026 05:32
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 20, 2026 05:32

Dismissing prior approval to re-evaluate 5fea58d

@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 20, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant