perf(gui): share one timer per interval and skip redundant revisit fetches - #1857
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lidge-jun
added
the
gui-screenshot-waived
|
✅ Deterministic PR hygiene checks passed. |
![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: 044ab41c83
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| initialData: cached ?? undefined, | ||
| initialDataCachedAt: seedCombosCachedAt(cacheKey), |
There was a problem hiding this comment.
When the Combos panel remains mounted but active becomes false, its resource store is evicted; on reactivation, cached is still the payload frozen by the mount-time useMemo, while seedCombosCachedAt(cacheKey) rereads the timestamp written by the latest refresh. After creating, deleting, or refreshing a combo when an older session entry existed, switching tabs and returning within 60 seconds therefore seeds the old payload with the new timestamp, skips revalidation, and lets state.data override the newer retainedData, making the completed mutation appear undone. Read the payload and timestamp from one current envelope, or retain both together after successful loads; ClaudeDesktop has the same split-read pattern.
Useful? React with 👍 / 👎.
044ab41 to
dbf5188
Compare
116bcae to
c75f88d
Compare
e2e9de2 to
7a6ba92
Compare
…tches Nine dashboard resources polling at 5s meant nine timers waking to do the same thing; poll scheduling now lives in interval buckets, so a cadence costs one wakeup no matter how many stores share it. Membership changes are bookkeeping, empty buckets are deleted, and the hidden-tab rule moves to bucket granularity (a bucket with no eligible member holds no timer, one visibility listener re-evaluates all buckets). Per-store skip rules still run inside the tick, so opt-out subscribers keep working while paused peers stay silent. Session-cache seeds now carry a write timestamp (legacy untimestamped values read as unknown age and self-heal), and staleAfterMs lets a revisit inside the window paint from cache with no request at all; past it the refetch is quiet, so cached content never flashes a skeleton. Wired into Combos, ApiKeys, ClaudeCode, ClaudeDesktop, Grok, and — via a new opt-in sessionCacheKey on useDataSurface — the ten Integrations resources that had no cache at all. New tests: client-resource-scheduler (4) and client-resource-revalidate (5).
…ne visibility listener Review caught a real regression in the revisit work: the Integrations overview and the per-client page describe the same connection through different cache keys, so a toggle on one could be contradicted by the other for up to 60s, with no request in flight to correct it. Those ten resources now seed from cache without a staleness window — a revisit still paints instead of flashing a skeleton, but it always revalidates. The window stays where a surface owns its truth (Combos, ApiKeys, ClaudeCode, ClaudeDesktop, Grok). A test pins the distinction so the next person adding a cache key has to decide which kind it is. Also from review: the per-store visibilitychange handlers collapse into one module listener (each was running the same global bucket sweep, N times per flip), and the session-cache seed read is memoized per key rather than re-parsed on every render of an eight-resource page.
c75f88d to
e1beb5e
Compare
be97ca6 to
17cfdcc
Compare
1 participant
Summary
Layer 4, the performance half: one timer per cadence, and a tab revisit that costs nothing.
Nine dashboard resources polling at 5s meant nine separate timers waking to do the same thing. Poll scheduling now lives in interval buckets — a cadence costs one wakeup no matter how many stores share it. Membership changes are bookkeeping, empty buckets are deleted rather than merely stopped, and layer 3's hidden-tab rule moves to bucket granularity with a single visibility listener re-evaluating every bucket. Per-store skip rules still run inside the tick, so opt-out subscribers keep working while paused peers stay silent.
The second half is where the request volume actually drops. Every route change evicts the store, so returning to a tab used to re-fetch everything — the Integrations overview alone re-issued eight requests. Session-cache seeds now carry a write timestamp (legacy untimestamped values read as unknown age and self-heal on first use), and
staleAfterMslets a revisit inside the window paint straight from cache with no request at all. Past the window the refetch is quiet, so cached content never flashes a skeleton.Ten Integrations resources had no session cache at all, so
useDataSurfacegained an opt-insessionCacheKeythat owns the seed-and-write-back pair instead of ten hand-written copies.Measured (sandboxed instance, CDP)
The visible-tab request count is deliberately unchanged: bucketing removes wakeups, not cadence, and a live dashboard is for freshness. No poll interval was retuned — the measurement does not show the Logs or Debug polls dominating (both are page-gated and now visibility-paused), and changing a freshness default without numbers behind it is the kind of regression this work exists to avoid.
Design and evidence:
devlog/_plan/260816_gui_loading_performance/040_phase4_poll_consolidation.mdStack (merge bottom-up): #1854 ← #1855 ← #1856 ← this PR (top).
Depends on #1856. Review this PR's diff only.
Verification
cd gui && bun test tests→ 922 pass / 0 failcd gui && bun run lint,bun run lint:i18n,bun run build→ greenbun run typecheck(root) → greengui/tests/client-resource-scheduler.test.tsx(4 cases: shared bucket, bucket move with empty-bucket deletion, hidden drop/visible re-arm, mixed opt-out membership) andclient-resource-revalidate.test.tsx(5 cases: fresh seed skips the fetch, stale seed revalidates quietly without a skeleton, legacy seed self-heals, no-staleAfterMskeeps today's behavior, cache envelope round-trip).Note:
startVisibilityPollschedules throughwindow.setIntervalwhen available — the migrated pollers all used the window timer and their tests intercept it there. The bare global bound to a different scope and broke nine tests, caught by running the full suite before commit.No visual change: scheduling and caching only.
Checklist