Repository navigation
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor cleanup suggestion inline.
Reviewed changes
- Skip probes after the settings pane closes —
loadEnvironmentsnow returns early once the saved-server list has resolved if the pane unmounted, and again after the verified-status snapshot wait, so a closed pane no longer starts status probes or follow-up snapshot reads while still publishing the shared catalog for mutation reconciliation. - Lifetime test suite — new happy-dom suite driving the real hook, covering a late list after close, a late mutation-driven reload, a verified wait that outlives the pane, preservation of active probes (verified/ephemeral skipping, exact call counts), an already-started probe after close, and late-error suppression.
The behavior change is narrow and matches the linked issue's documented trade-off (catalog publication is kept; status freshness for a closing pane defers to later events/refresh/reopen). Tests are meaningful — the three closed-pane cases assert getStatus/readRuntimeHostStatusSnapshots are not called, which the unguarded code fails. Focused suite verified passing locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe hook stores catalog results after unmount but stops pane-dependent work when the pane is no longer mounted. It checks mount state before verified-status snapshot work and again after the snapshot read. Tests cover mutation reloads after close, probe behavior across unmounts, and late catalog errors. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The pane can close during loading without starting late status probes, while saved-server updates remain available. No actionable merge-blocking risk is identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Closing settings can now leave saved-server status unchecked until another refresh or reopening. The saved-server list still updates, and the change does not establish a new external entrypoint or permission path. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…t-only assertions The suppression is keyed to useMountedRef, which is per-open-session, so a reopened pane reloads and reprobes from scratch. Nothing tested that, and the guard now sits in front of every probe: if the mounted flag ever failed to re-arm on mount, the pane would issue zero probes and sit on 'loading' forever instead of merely skipping a setState. Added a close-then-reopen test and a StrictMode double-invoked-effects test (both fail if the flag is not re-armed), including the assertion that a host whose probe was suppressed stays 'loading' (never checked) rather than 'error' — a probe that did not run is not evidence the host is down. Also removed the now-dead mounted re-check (no suspension point between it and the preceding guard) and relaxed the incidental snapshot-read count assertion, which would have flagged coalescing those reads as a regression.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Added reopen + StrictMode lifetime tests — a close-then-reopen test and a
StrictModedouble-invoked-effects test confirm theuseMountedRefsuppression does not persist across mounts, that the reopened pane probes from scratch, and that a host whose probe was skipped staysloadingrather thanerror. - Removed the now-dead mounted re-check — the outer
if (mountedRef.current)aroundsetEnvironments/setDetailsByEnvironmentIdis gone, since the two preceding early returns already guarantee the pane is mounted (no suspension point between them). This addresses the prior review's inline thread. - Relaxed the incidental snapshot-read count — the active-probing test now asserts
readRuntimeHostStatusSnapshotswas called rather than exactly twice; the probe path's shared-status sync is still independently asserted by the already-started-probe test, so no coverage is lost.
The only prior Pullfrog thread (dead mountedRef wrapper) is resolved by 12e0ced. Verified this run: the 8-case suite passes and oxlint is clean on both changed files. The documented freshness trade-off is unchanged and remains accepted.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Closing Remote Orca Servers stops new server checks from starting when its saved-server list arrives late.
What Changed
The catalog hook checks whether the pane is still open after the list arrives and after a verified-status wait. In a controlled 40-server case, closing before the list resolves reduces status probes and follow-up snapshot reads from 40 each to 0.
Counts use mocked bridge/store boundaries, not physical socket opens or measured UI latency.
Why
The hook keeps shared catalog updates so an add/remove operation that finishes after closing still updates other host pickers. Already-started checks finish normally. Canceling the whole load would also discard those catalog updates. The trade-off is that closing before probing begins defers status freshness to normal status events, another refresh, or reopening.
Separate trade-off PR: status freshness can be delayed with no bounded refresh deadline. The app feed does not guarantee immediate recovery: a new host's event may arrive before its catalog entry and be ignored. A later event, hydration, refresh or reopening may be needed. Already-admitted add/connect/remove operations still finish; connection can begin sooner after closing because the extra status wait is removed.
Linked Issue
Fixes #23150
Visual Proof
N/A
Visible rendering and controls are unchanged; this removes background work after the pane closes. Rendered application and physical server behavior were not exercised.
Testing
Six lifecycle tests pass; the original code fails three. Tests use actual React hooks in happy-dom with fake server and store calls, including late list/snapshot completion, mutation catalog reconciliation, active filtering, already-started probes and failures. Renderer typecheck, focused lint/format and the full changed-code quality gate (157 files) pass on macOS. Tests used background-launch mode. No native app, actual IPC/network, SSH/WSL or Windows/Linux runtime test; broader CI/build checks remain pending.
Review
Independent review approved the exact source/test hashes. Ten additional actual-hook tests pass (original five fail), including real add/remove callback paths, web-connect failure rollback, StrictMode effect replay, separate remount ownership and admitted probe success/failure after close. The permanent six-case suite was independently reproduced (original three fail).
Merge risk: low implementation complexity, moderate freshness risk for shared server status. Active request generation and late shared catalog overwrite remain unchanged; this is not general stale-load protection.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
No host routing, wire formats, process-liveness verdicts or mutations change. Overlapping-refresh generation and cancellation of already-started requests remain outside this change.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)