fix(mcp): restore lightweight device presence heartbeat#3171
Open
rogalio wants to merge 2 commits into
Open
Conversation
Since PR superset-sh#2904 removed the 30-second heartbeat to cut Vercel costs, devices register only once at startup. The MCP `list_devices` tool still checks a 60-second online window, so every device appears offline after one minute and MCP commands can no longer reach it. - Add a 5-minute periodic re-registration in useDevicePresence (288 calls/device/day vs the old 2,880 — 90% cost reduction kept) - Bump the online window from 60s to 10 minutes (2x the heartbeat interval to tolerate network hiccups) - Update tests and documentation to match Closes superset-sh#3091
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtended device “recently seen” window from 60 seconds to 10 minutes; added periodic device registration every 5 minutes in the desktop client; updated docs and tests to reflect the new online/offline threshold. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummaryThis PR restores a lightweight device presence heartbeat (every 5 minutes) to fix a regression introduced in #2904, where removing the 30-second heartbeat caused all devices to appear offline after 60 seconds, breaking MCP command routing entirely. The fix is minimal and well-targeted: a
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Desktop as Desktop App (useDevicePresence)
participant API as Superset API (registerDevice)
participant DB as DB (device_presence)
participant MCP as MCP Client (list_devices)
Desktop->>API: registerDevice (on mount)
API->>DB: upsert lastSeenAt
loop Every 5 minutes
Desktop->>API: registerDevice (heartbeat)
API->>DB: upsert lastSeenAt
end
MCP->>DB: query devices WHERE lastSeenAt >= now - 10min
DB-->>MCP: device list with isOnline flags
Reviews (1): Last reviewed commit: "fix(mcp): restore lightweight device pre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| deviceName: deviceInfo.deviceName, | ||
| deviceType: "desktop", | ||
| }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
The previous implementation reset registeredScopeRef.current = null on failure so the next render/launch would retry. The new code replaces that with a 5-minute interval retry, which is fine — but swallowing the error entirely with an empty catch makes it impossible to diagnose persistent failures (e.g., auth errors, network outages, broken API endpoint). Consider at least logging to the console in dev:
Suggested change
| .catch(() => {}); | |
| .catch((err) => { | |
| if (process.env.NODE_ENV === "development") { | |
| console.warn("[useDevicePresence] registration failed:", err); | |
| } | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.ts (1)
29-29: Silent error handling may obscure persistent failures.The
.catch(() => {})swallows all errors, including network failures, auth issues, and server errors. While this is acceptable for a fire-and-forget heartbeat, consider logging atdebuglevel or tracking consecutive failures to surface persistent registration problems during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.ts` at line 29, The silent .catch(() => {}) in useDevicePresence should not swallow all errors; update the promise rejection handler in the function (the chain ending with .catch(() => {})) to log the error at debug level (e.g., console.debug or the existing logger) and increment/reset a consecutive-failure counter stored in the useDevicePresence hook state so persistent failures can be detected and surfaced (e.g., emit a debug-level warning or set an isRegistrationHealthy flag after N failures). Ensure the handler references the caught error (err) and ties into existing hook state or telemetry so transient vs persistent failures are distinguishable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.ts`:
- Line 29: The silent .catch(() => {}) in useDevicePresence should not swallow
all errors; update the promise rejection handler in the function (the chain
ending with .catch(() => {})) to log the error at debug level (e.g.,
console.debug or the existing logger) and increment/reset a consecutive-failure
counter stored in the useDevicePresence hook state so persistent failures can be
detected and surfaced (e.g., emit a debug-level warning or set an
isRegistrationHealthy flag after N failures). Ensure the handler references the
caught error (err) and ties into existing hook state or telemetry so transient
vs persistent failures are distinguishable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e1e52b4-135c-4dd2-a3cc-bb787ea92f76
📒 Files selected for processing (4)
apps/api/MCP_TOOLS.mdapps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.tspackages/mcp/src/tools/devices/list-devices/list-devices.test.tspackages/mcp/src/tools/devices/list-devices/list-devices.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useDevicePresence/useDevicePresence.ts:29">
P2: `registerDevice` failures are silently swallowed by an empty catch block, removing actionable diagnostics in the periodic heartbeat path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
Since #2904 removed the 30-second heartbeat to reduce Vercel API costs, devices register only once at startup. The MCP
list_devicestool still checks a 60-second online window, so every device appears offline after one minute and MCP commands can no longer reach it. Multiple users reporting this (#3091).What this PR does
Restores a lightweight 5-minute periodic re-registration in
useDevicePresence, and bumps the online window from 60s to 10 minutes.Alternative: increase the window to 24h instead
If the cost reduction from #2904 is more important than real-time accuracy, a simpler fix would be to just bump the online window to 24 hours with zero additional API calls. Trade-off: a device closed 23h ago would still appear online, but
executeOnDevicealready handles unreachable devices via its timeout mechanism.@Kitenite — would you prefer the 24h window approach (zero cost, less accurate) or the 5-minute heartbeat (288 calls/day, accurate)? Happy to adjust either way.
Changes
useDevicePresence: Add 5-minutesetIntervalre-registrationlist-devices.ts: Bump online window from 60s to 10 minutesMCP_TOOLS.mdTest plan
bun testinpackages/mcp/— list-devices tests passlist_devicesstill shows device as onlineCloses #3091
Summary by CodeRabbit