Repository navigation
fix(clusters): only instances on screen poll their status on the instances list - #1821
Merged
dawsontoth merged 18 commits intoOct 12, 2026
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces off-screen gating for the InstanceStatusCell component to optimize performance by only polling status requests for instances currently visible on screen (or within a 200px margin). This is achieved via a new custom hook useIsOnScreen utilizing the IntersectionObserver API, accompanied by a mock FakeIntersectionObserver for testing. Feedback includes addressing a potential memory leak in the useIsOnScreen hook where a reference to an unmounted or changed DOM element is retained in state, and improving accessibility by adding screen reader text to the loading spinner.
This was referenced Oct 11, 2026
Merged
dawsontoth
added this pull request to stack #1901
October 11, 2026 21:25
dawsontoth
marked this pull request as ready for review
October 12, 2026 16:38
…ances list Each row of a cluster's instances list polled get_status every 10s for as long as the tab was visible, so the list's cost grew with the cluster rather than with what anyone could see (one session sent 5,271 operation requests in a day). A row now polls only while it is within 200px of the viewport, through a small useIsOnScreen hook over IntersectionObserver: a visible row keeps its 10s cadence, a row scrolled out of view stops, and a row scrolling back in fetches at once if its status is stale. A row that has never loaded shows the spinner rather than an unknown dot. Browsers without IntersectionObserver poll every row as before. Refs #1603 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erved Review follow-up: useIsOnScreen kept the last reported visibility when its element was removed or replaced, so a row that went RESTARTING while on screen, was scrolled away, and came back RUNNING would fetch once before the observer reported its new element off-screen. The state now records which element it describes and reads false for any other. Refs #1603 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The hook kept the last observed element in state after the element went away, so an instances-list cell that switched to its muted branch held its detached status subtree for as long as the row stayed mounted. Reset the stale report during render, the documented pattern for adjusting state to a changed input; the return value is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… text The spinner icon is aria-hidden, so a row whose status had not loaded read as an empty cell. Off-screen rows now stay on the spinner until they scroll into view, which made that state common. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eds Harper 5.1
The deployment history list (2s/10s), the deployment detail (2s until
terminal) and the log auto-refresh (5s) were the instance-operation pollers
left outside the shared poll policy, so a 403 never stopped them and a 400 or
5xx never slowed them. The detail poll was the worst: a failed fetch has no
status to end on, so a deployment id the instance rejects ("No deployment found
with id …", a 400) was asked for every 2s for as long as the dialog stayed
open. All three now go through pollUnlessForbidden; the two whose interval
depends on the data compute it per tick and hand it over.
The deployments page is hidden from the nav below Harper 5.1, but a bookmark or
back-navigation still mounted it and polled list_deployments against an
instance that doesn't have it. It now reads the instance's version first and
says the feature needs Harper 5.1 instead of polling.
DESIGN.md records the polling convention.
Refs #1602
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on can't be read Review follow-up: if the registration_info read failed with nothing cached, the deployments page waited on the version forever behind a spinner. It now shows the existing "Could not load deployments" line instead. The log view's auto-refresh off path stays a plain `false` rather than a wrapper that always answers false. Refs #1602 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…certification A rolling deploy answers once the node that received it is done and hands the other nodes to a restart_service job (restartJobId). Studio never followed it, so a node that rejected the release read as a successful deploy. The deploy mutation now polls get_job until the job ends: a failed job rejects with DeployRolloutError naming each node, and a job Studio cannot follow resolves as unfollowable, which the callers never report as success. Also reads Harper 5.4's canary certification on a deploy's result and its rejection decision on a failure, keeps the deploy's streamed warning events, and links the progress panel to the deployment's record. Refs #1785 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n success A finished restart job with a result in neither shape Harper writes is now unknown rather than complete. The buffered redeploy and import paths report through the same rollout policy as the streamed one, so a node the legacy job could not reach, or a rollout Studio could not follow, is a warning there too, and their toasts link the deployment's record. Following the job aborts promptly when its signal fires mid-poll, and an unknown rollout settles the progress panel as inconclusive, not success. Refs #1785 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t-found grace followRestartJob gave up on a job once it had been missing for 20s, but measured that from the first miss even when every poll in between failed. The receiving node restarts its own workers before its peers (harper bin/restart.ts:336-355), so a cluster address that reached another node could read one miss, then errors while the job's node restarted, and end the rollout as not-found though the job was running. A miss now only counts across an unbroken run of misses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… how each node installed The list filters by component and status, pages by the server's total, and shows each record's id, source (package, payload hash, or the release an activation made live) and user. A staged record (Harper 5.3+) is terminal and reads as waiting to be activated, so the list no longer polls it every 2s. The detail view shows the origin's install source, the activation's source release, and each peer's install comparison (5.3.1+), and drops the Restorable and Rollback-of fields no Harper version sets. The editor's Application menu links to the component's deployments. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ilter where staging is absent The list no longer keeps the previous page's rows on screen, clickable, while the next page loads. A saved ?status=staged link opened against an instance older than 5.3 no longer asks for a status it cannot have. The component filter applies on Enter only, so leaving the field to click Next does not reset the page under the click, and it follows an outside change without remounting the field. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the URL A saved ?status=staged link opened against Harper older than 5.3 left the URL claiming a filter while the list showed every status. Once the version is known, the page replaces the URL without it. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Redeploy and Import offer Stage only where the instance reads 5.3.0 or later: it sends activate: false and no restart, and reports the staged deployment id without a health check. A staged record on the Deployments page has an Activate action, which sends deploy_component with its deployment_id and a rolling restart through the same mutation, so it is followed like any other rolling deploy; the progress panel ends a stage at Staged. All three request shapes come from one deployRequestBody, and the editor's Application menu links to a component's staged releases. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sent twice Activate awaits the mutation instead of passing it callbacks, which React Query drops once the detail unmounts, so the report and the refresh still happen for an activation someone closed the dialog on. The mutation is keyed by entity and deployment, so a detail reopened mid-rollout shows it pending and offers no second activation. A stream that ends early says the outcome is unknown instead of reporting a failure. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The activation reports every outcome itself, so it opts out of the global mutation toast, which would have called an early-ended stream a failure alongside the local 'may not be activated'. It logs the error with console.error as that handler did, so Datadog still sees it. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…after it may have started When the SSE deploy got no event stream, onDeployComponentSubmit fell back to the buffered deploy_component on any SSEUnsupportedError: a gateway 502/503/504, a 2xx that was not a stream, a dropped connection, or the 120s idle watchdog firing before headers. Harper starts an SSE operation as soon as it accepts it and answers with a 200 stream, so each of those can follow a deploy that is already running, and the fallback deployed the same package a second time (a second install and rolling restart, or two deploys of one project racing). SSEUnsupportedError now carries the response status. The deploy falls back only on a 4xx, which Harper answers for a request it refused before running it (authentication, permissions, validation). Anything else becomes SSEInconclusiveError, which the import, redeploy and activate callers already answer by checking instead of resending. Closes #1889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
deployComponentStream's contract still told every caller to retry on SSEUnsupportedError, which is the resend this branch removes. Also trims two comments that restated the design note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/1603-offscreen-status-polls
branch
from
October 12, 2026 16:45
119b604 to
e5c118e
Compare
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
1 participant
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.
⊙ Problem
Each row of a cluster's instances list runs its own
get_statuspoll every 10s for as long as the tab is visible, so the list's cost grows with the cluster, not with what anyone can see: a 28-instance cluster is ~170 requests a minute from one tab. RUM, last 7 days: 6,755 of the 7,870 proxiedPOST /HDBInstance/{id}/operationrequests (86%) came from/instancesviews, across 54 sessions; the issue's worst day had one session at 5,271.💡 Solution
A row now polls only while it is within 200px of the viewport, through a small
useIsOnScreenhook overIntersectionObserver. A visible row keeps its 10s cadence and its existing stagger; a row scrolled out of view stops; a row scrolling back in fetches at once if its status is stale (9sstaleTime), so the margin usually has it loaded before it appears. The existing per-row stagger (index × 500ms) still applies first, so a row far down a long list that is scrolled into view within its first few seconds waits out its stagger, as it did before. A row that has never loaded shows the spinner rather than an "unknown" dot, with "Loading status" for screen readers, since the icon itself isaria-hidden. Where the browser has noIntersectionObserver, every row polls as before. The cost of the list is now bounded by viewport height rather than cluster size.⚖️ Alternatives
visibilitychange, so a visible-but-unfocused window keeps polling. Not done: a second monitor showing the list is a legitimate use, and it would change the UX of a visible list.🔧 Changes
src/hooks/useIsOnScreen.ts— the hook; true whenIntersectionObserveris absent, false until the observer reports, disconnects on unmount, and drops a report whose element is gone so a detached element is not kept alive in state.src/features/cluster/InstanceStatusCell.tsx— the row's poll requiresonScreen; the observed element is the row's status container; spinner for a not-yet-loaded off-screen row.src/test/fakeIntersectionObserver.ts— a controllable stand-in (jsdom has none), used by the tests below.✅ Verification
End-to-end route: new component test against the real
getStatusQueryOptionsand a realQueryClient, counting requests (InstanceStatusCell.polling.test.tsx): with three rows and one on screen, 60s produces 7 requests for the visible row and 0 for the others; a row scrolled into view fetches immediately and then every 10s. AlsoInstanceStatusCell.test.tsx(enabled flips with visibility; spinner for an unloaded row) anduseIsOnScreen.test.ts, including a forced-GC test that the detached element is collected.onScreenfrom the poll gate fails both polling tests (the hidden rows send 7 requests each) and the two enable-flag tests; reverting the spinner change fails its test; reverting the hook's reset fails the GC test (3/3 runs), and reverting the screen-reader text fails its assertion.npx vitest run→ 397 files / 3,701 tests passed at 119b604 (exit 0, pre-commit hook).npx tsc -b,npx oxlint --format stylish .,npx dprint checkall exit 0.IntersectionObservergeometry in the real table layout is not exercised.Pre-push review (codex + gemini), one round. Fixed: the hook kept the last visibility when its element was replaced, so a row that went RESTARTING on screen, was scrolled away and came back RUNNING fetched once before being observed off-screen — state is now bound to the observed element (second commit, with a regression test);
entries.at(-1)→ index access. Declined on evidence: a row scrolled away mid-retry finishes its in-flight retry chain (≤3 requests, React Query's disabled-query contract; cancelling would also cancel the row menu's observer of the same key); one observer per row rather than a shared one (clusters are tens of instances — the largest in the RUM data is 28); a spinner for an errored row while it is off screen (nobody can see it, and scrolling it in refetches at once); dropping the stagger (it is what spreads the initial burst of visible rows). The cursor leg failed on the 1Password SSH agent and the domain (Harper adjudication) leg on an expired OAuth session. The follow-up commit was not re-reviewed.Round 2 (codex + gemini + Harper domain), after the gemini-code-assist fixes below: both round-1 findings resolved, nothing new against the fixes. Still open and unchanged in this PR: the in-flight retry chain and per-row observer, both declined above; a nit that the touched imports lack
.tsextensions (repo convention is extensionless;allowImportingTsExtensionsis off); and a nit asking to trim narrative comments elsewhere in the PR, applied only to the two comments the fixes added.gemini-code-assist feedback. Fixed:
useIsOnScreenkept a removed element in state (613a04d, with a forced-GC regression test); the spinner had no screen-reader text (858966c).Closes #1603
🤖 Generated with Claude Code
Related PRs: #1817 overlaps (stacked base), #1812 overlaps (base of #1817)
Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; blocked=cursor-composer(no-receipt); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 119b604
Review-Attention: read ~3m (decisions: do-less-alternative, stagger-kept-index-based, per-row-observer, offscreen-uncached-shows-spinner) @ 119b604