Repository navigation
perf(staged): build branch timelines from fewer git spawns and paint the last persisted timeline on launch - #1009
Merged
Merged
Conversation
…ox round trip
Every branch card's first timeline after launch was built by a chain of
git subprocesses: the local arm of build_branch_timeline ran up to 16
spawns in five sequential stages (11 on the critical path, two of them
the same merge-base), and the Blox arm paid five ws_exec round trips at
roughly 3 s each. Collapse both.
Local arm (merged queries, no fetch):
- One `git config -z --get-regexp '^(branch.<b>.|user.)'` replaces the
two upstream-config reads and the two identity reads. It runs through
run_lite so a no-match exit 1 never triggers the shell-env capture
retry; the user identity is remembered per worktree as a fallback for
builds whose config read fails.
- One `git rev-parse HEAD --symbolic-full-name HEAD` replaces rev-parse
HEAD + branch --show-current (detached HEAD comes out as `HEAD`; only
an unborn branch falls back to branch --show-current).
- One `git for-each-ref` resolves origin/<b> and origin/<base> instead
of two `rev-parse --verify` calls. Missing refs are simply absent, and
only exact refnames match so nested refs cannot be mistaken for them.
- `merge-base HEAD origin/<base>` runs exactly once per build and feeds
both the base-state count and `git log <mb>..HEAD` (the second
merge-base in get_commits_since_base is gone). The upstream's merge
base is derived from the --left-right counts; only a diverged
upstream still asks git for it.
- The build is two std::thread::scope phases: {rev-parse, status,
for-each-ref, merge-base} then {three rev-list counts, git log}. When a
fetch is planned (Ttl/Force callers) the remote-ref reads still wait
for it, so refresh semantics are unchanged.
Git spawns per local timeline build (with upstream / without):
critical path 11 -> 3 (config -> merge-base -> log)
total 16 -> 9 (13 -> 7 without an upstream; +1 when diverged)
Remote (Blox) arm: BATCH_GIT_STATE_SCRIPT takes a ninth argument, the
log format, and when set appends user.name / user.email and the commit
log (from the merge base it already computed, or origin/<base> when
there is none) behind a LOG_START delimiter as the final section, one
commit per line, so a subject that spells a key or delimiter cannot be
misread. compute_branch_git_snapshot_batched parses state, identity and
commits from that single ws_exec result; the state-only entry point
leaves the argument empty and the refresh path's script output is
byte-for-byte what it was. FetchMode::Never still passes skip_fetch.
Blox round trips per build 5 cold / 3 warm -> 1
BranchTimeline's serialized shape is unchanged; no frontend changes.
Tests: new branch_config parser/snapshot tests (NUL output, empty and
malformed input, regexp escaping, real repos with origin / other-remote
/ no upstream); for-each-ref, rev-parse and extended batch-output
parsers (full, missing upstream, empty log, failed log, no log section,
delimiter text in subjects); recording-runner fixtures asserting the
exact spawn set for in-sync, ahead, behind, diverged, no upstream, no
origin/<base>, zero commits ahead, detached HEAD, unborn branch and
fetching builds; batched snapshot runs of the real script; and timeline
local-arm tests for own-commit marking, upstream selection, zero
commits, missing base and detached HEAD.
The `[perf] git` / `branch-timeline` spans referenced by the audit note
exist only on the performance-audit branch (567a0fca), not here, so
there was none to keep and none was added to the web_server.rs arm.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ktop) Every launch of the desktop app showed each branch card on "Looking for changes…" for one to four seconds, because nothing survived a relaunch: the in-memory timeline map is per process, the IndexedDB SWR layer in cache.ts returned early under Tauri for every command, and the localStorage boot snapshot is web-only. This is the frontend variant of "Recommended fix 2" from the timeline audit note; the backend SQLite variant is deliberately not implemented. cache.ts — a Tauri persistence allowlist instead of a blanket early return: - `get_branch_timeline` is the one command that goes through IndexedDB under Tauri; `cachedInvoke`, `cachedCommand` and the three invalidators keep their network-only / no-op path for every other command. - `readCachedEntries(command)` reads every usable entry for a command in one key-range `getAll`, for seeding an in-memory cache at boot. Entries from another `CACHE_SCHEMA_VERSION` are skipped (and swept), so a `BranchTimeline` shape change is handled by the existing bump. - `CacheConfig.maxEntries` bounds a command's persisted entries: after a write pushes it over the cap, the oldest by `fetchedAt` are evicted. The count is one `getAllKeys`; values are only read when over the cap, which happens once per newly seen key, not on every refetch. One trim runs per command at a time so a project open with 40 cards shares it. - `CacheConfig.scope` is an opaque identity string stored with the entry and handed back on read; the cache does not interpret it. - Every IndexedDB failure is non-fatal: a store that cannot open reads as a miss, writes are swallowed, the invalidators and sweep catch, and `sweepCache` / `clearAllCache` now also run under Tauri. commands.ts — seed, identity and reconciliation: - At module init under Tauri, the in-memory timeline cache is seeded from IndexedDB with each entry's ORIGINAL `fetchedAt`, so a card's first `getBranchTimelineWithRevalidation` paints it and starts exactly one forced refetch in the same tick (the same contract the web snapshot already relies on). The later result and `git-state-updated` events overwrite the paint through the card's existing `applyRevalidation` and merge paths, and the existing git-state spinner covers "possibly stale" — no new UI. A seed entry is skipped when a card already fetched or is fetching that branch, so it can never roll a fresher timeline back. - Each write is stamped with the branch's identity (project, repo, type, name, base, Blox workspace, worktree path), captured as branch lists pass through `listBranchesForProject`. On the next launch an entry is shown only while nothing contradicts that identity: once the project list or the branch list for its project arrives without the branch, or with the branch under a different identity, the entry is dropped from memory (if still the seeded one) and from IndexedDB. Lists that land before the seed settles are honoured too. - `MAX_PERSISTED_TIMELINES = 200` bounds the store; the boot seed reads all of them in one request, so it also bounds time to first paint. - Web behaviour is unchanged: the seed, like the snapshot, is gated on platform, and `warmProjectTimelines` / `persistTimelineSnapshot` keep their Tauri early returns. Tests: cache.test.ts gains a desktop block (persist then stale-while- revalidate across a module reload, other commands stay network-only, per-branch keys, broken-IndexedDB fallback for reads/writes/maintenance, oldest-first eviction at `maxEntries`, schema-version skip + sweep). commands.test.ts gains a desktop block driven through the real cache.ts and fake-indexeddb as consecutive launches: seeded paint with one forced refetch per card, miss-then-persist, broken-IndexedDB fallback, drop on missing branch / changed identity / missing project, a branch list that lands before the seed, and no rollback of a fetch that beat the seed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…h, and only a silent exit 1 is "no config" Addresses every finding of the review of 5018995 and a227928. Frontend (commands.ts): - Plain `getBranchTimeline` treated any in-memory hit as an answer. Under Tauri that map now holds entries seeded from the previous launch's IndexedDB, so hashtagItems and DiffCommitSessionLauncher could resolve with a days-old commit list and no refetch. A seeded entry is now a paint, never an answer: while `timelineCache` still holds the exact entry the boot seed placed, a plain read treats it as a miss, joins the card's forced refetch when one is in flight, and otherwise fetches with `bypassRead` (the IndexedDB entry is the very thing being skipped). `getBranchTimelineWithRevalidation` is unchanged: it paints the seeded entry and issues the one forced refetch as before. Of the two options the review offered, this one was chosen over a `TIMELINE_FRESH_MS` age rule for plain reads, which would have turned every hashtag-menu or launcher open more than ten seconds after a card's fetch into another one-to-four-second git chain on desktop. - The boot seed skips, and removes from IndexedDB, entries older than `MAX_SEED_AGE_MS` (24 h). Anything younger paints and is refetched. - `MAX_PERSISTED_TIMELINES` (200) stays in force on the web as well as on desktop, deliberately. The store previously grew unbounded until the seven-day sweep; the cap evicts least-recently-fetched entries, so a web (iOS) card beyond it misses IndexedDB and fetches exactly as it did before anything was persisted for it, at the cost of one `getAllKeys` per write. The constant's comment records this. Backend: - `cli::run_lite_output` returns the exit status, stdout and stderr of a finished git run instead of judging it (spawn failures, a non-repo path and non-UTF-8 stdout are still errors); `run_with_env` shares the new `spawn_git` with it. - `read_branch_config_snapshot` treats only exit 1 with nothing on stderr — git's "no key matched" — as an empty snapshot. An unreadable config file (exit 3 / 128), a killed child, or an exit 1 that came with a complaint is now `Err`, so the timeline keeps the identity it last read instead of overwriting the cache with None/None. - The local arm's fallback is `timeline::local_identity_and_remote_branch`: a failed read compares against the branch's own name and returns the remembered identity without touching the cache; a successful empty read still clears it. Tests filling the gaps the review listed: - branch_config: synthetic outputs (exit 0, silent exit 1, exit 1 with stderr, exit 128 with stderr, silent exit 3, signal) and a real repo whose `.git/config` cannot be parsed. - cli: `run_lite_output` on a hit, a no-match exit 1 and a non-repo path. - timeline: a config read failing with non-empty stderr keeps the remembered identity and the local branch name; a later successful empty read clears it. - state: `upstream_counts_and_merge_base` with a failing `rev-list` (merge-base spawn, and `None` when that fails too) plus the derived case; a recording-runner build whose upstream count fails and asks git for the upstream merge base; a batched Blox snapshot with the cached `upstream_missing` verdict that still carries identity and log. - commands.test.ts: a plain read of a seeded entry resolves with this launch's timeline from one fetch and shares an in-flight card refetch; entries older than a day are not seeded and are removed from IndexedDB while a younger one still paints. Verified: cargo fmt, cargo clippy -- -D warnings, cargo test for git::cli, git::branch_config, git::state, git::state_tests and timeline; pnpm run check (0 errors); pnpm test (126 files, 1715 tests). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… under ten seconds old Closes the one remaining finding of the review of c95cfaf. getBranchTimelineWithRevalidation judged freshness by the entry's fetchedAt alone. A desktop boot seed keeps the previous launch's original fetchedAt, so relaunching within TIMELINE_FRESH_MS (10 s) of a card's last fetch painted that launch's timeline with `fresh: null` and no refetch. The backend timeline map is per process, so nothing would refresh the card until a git-state-updated event or a remount, and the "paint once, refetch exactly once" contract of a227928 did not hold in that window. The fresh check now also requires that the entry is not the one the seed placed (`isSeededTimeline`), so a seeded entry is always painted and always followed by exactly one forced refetch regardless of age. Plain reads already treated a seeded entry as a miss and are unchanged. Test: commands.test.ts relaunches with the fake clock untouched, so the seeded entry's fetchedAt equals now, and asserts the paint, the single forced refetch resolving to this launch's timeline, and no further fetch once that result is in. Verified: pnpm run check (0 errors, 0 warnings); pnpm test (126 files, 1716 tests). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93df99f177
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| local: &str, | ||
| ) -> Result<BranchConfigSnapshot, GitError> { | ||
| let pattern = branch_config_regexp(local); | ||
| let output = cli::run_lite_output(worktree, &["config", "-z", "--get-regexp", &pattern])?; |
There was a problem hiding this comment.
When the desktop process cannot find git on its inherited PATH but the captured login-shell environment can (for example, a Hermit-managed installation), run_lite_output returns GitNotFound and the timeline falls back to an empty identity. The state and log reads still succeed because their Lite path uses run_smart, which retries with the captured environment in cli.rs:83-95, so the timeline renders but every actual commit is marked isOwnCommit = false; the replaced identity lookups previously used cli_run_smart. Preserve the special handling for a silent exit 1 while retrying spawn/environment failures with the captured environment.
Useful? React with 👍 / 👎.
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
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.
Summary
Branch cards on desktop took one to four seconds to leave "Looking for changes…" after every launch. This branch cuts the cost of building a timeline and paints the previous launch's timeline immediately while the fresh one is fetched.
Fewer git spawns and one Blox round trip (
5018995a)git config --get-regexpreplaces four config reads, onerev-parsereplacesrev-parse+branch --show-current, onefor-each-refreplaces tworev-parse --verifycalls, andmerge-baseruns once and feeds both the base count andgit log. The build runs as twostd::thread::scopephases. Critical-path spawns drop from 11 to 3 and total spawns from 16 to 9 (13 to 7 without an upstream).BATCH_GIT_STATE_SCRIPTtakes a log-format argument and emits identity and the commit log behind a delimiter, so a build costs onews_execinstead of five cold / three warm.BranchTimeline's serialized shape is unchanged.Paint the last persisted timeline on launch (
a2279285)cache.tsgains a Tauri persistence allowlist: onlyget_branch_timelinegoes through IndexedDB on desktop. AddsreadCachedEntries(one key-rangegetAllfor boot seeding),maxEntrieseviction (oldest byfetchedAt), and an opaquescopeidentity string. Every IndexedDB failure is non-fatal.commands.tsseeds the in-memory timeline cache from IndexedDB at module init under Tauri with each entry's originalfetchedAt, so a card's first revalidating read paints it and issues exactly one forced refetch. Entries are stamped with branch identity and dropped once a project or branch list contradicts it.MAX_PERSISTED_TIMELINES = 200.Review fixes (
c95cfaf3,93df99f1)getBranchTimelinereads treat it as a miss and join or start a forced refetch. Revalidating reads always refetch a seeded entry regardless of age.cli::run_lite_outputreturns status/stdout/stderr;read_branch_config_snapshottreats only a silent exit 1 as "no config", so a failing read keeps the remembered identity instead of clearing the cache.Testing
cargo fmt,cargo clippy -- -D warnings,cargo testforgit::cli,git::branch_config,git::state,git::state_tests,timeline.pnpm run check(0 errors),pnpm test(126 files, 1716 tests).🤖 Generated with Claude Code