feat(core): add "Run with agent" action for inspector comments#101
Open
virang-nimrobo wants to merge 3 commits into
Open
feat(core): add "Run with agent" action for inspector comments#101virang-nimrobo wants to merge 3 commits into
virang-nimrobo wants to merge 3 commits into
Conversation
Adds a Vite dev-server plugin exposing /__superconnector/* endpoints for starting, polling, and canceling agent runs scoped to a single comment. The inspector comment widget gains a "Run with agent" button that dispatches the comment through @nimrobo/superconnector to a connected coding agent, with live status (running / done / error / canceled) streamed back over HMR. Localized strings added for en, ja, zh-CN, zh-TW.
|
@virang-nimrobo is attempting to deploy a commit to the Yiwei Ho Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a Vite dev /__superconnector API and client integration: a useAgentRuns hook, AgentLogLine/AgentRunButton UI, comment-widget wiring, localization, tests, Vite plugin implementation with run registry/streaming/cancel, Vite plugin registration, package dependency, and a changeset. ChangesSuperconnector Agent Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/core/src/app/components/inspector/comment-superconnect.tsx (4)
87-91: 💤 Low valueCleanup only runs on unmount;
pollTimerskeeps growing across runs.The unmount cleanup clears active timers, but completed runs are never removed from
pollTimers.currentbecausestopPollingis called fromfinishRun(which does delete the entry) — that part is fine. However, ifslideIdchanges during the component's lifetime (e.g., navigating between slides while the inspector stays mounted), in-flight polls and pending state for the previous slide are not reset;status/log/runsMaps will continue to carry stale entries keyed by old comment IDs. Consider resetting state and clearing timers whenslideIdchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 87 - 91, The component only cleans timers on unmount, so when slideId changes stale timers and entries in pollTimers.current and the Maps (status, log, runs) persist; add a useEffect that depends on slideId which iterates pollTimers.current and calls stopPolling (or clearInterval) for each active timer, clears pollTimers.current, and resets the Maps (status, log, runs) to empty states so in-flight polls and pending state for the previous slide are removed whenever slideId changes; reference the existing pollTimers, stopPolling, finishRun, and the Maps (status/log/runs) and ensure this new effect runs before starting any new polls for the new slideId.
162-175: ⚡ Quick win
setIntervalcan stack overlapping polls; prefer recursivesetTimeout.If a
pollStatusrequest takes longer than 2 s (slow network, server warm-up),setIntervalwill dispatch additional fetches in parallel rather than waiting for the previous one. This can lead to out-of-order log/status updates and unnecessary load. A recursivesetTimeoutensures one in-flight poll at a time and is the standard pattern for this case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 162 - 175, Replace the setInterval polling loop with a recursive setTimeout pattern so only one pollStatus(runId) is in-flight at a time: implement an async function (e.g., pollLoop) that calls pollStatus(runId), updates setLogMap with the last message for commentId, calls finishRun(commentId, status, error) when status !== 'pending', and on success schedules itself with setTimeout(..., 2000); on errors call finishRun(commentId, 'error') and stop or schedule retry as appropriate; ensure you clear the timer/stop the loop when the component unmounts or run completes.
181-192: 💤 Low value
canceldoes not give immediate feedback or update local state.When the user clicks Stop, the request is sent but
statusis not changed locally — the UI continues showing the in-flight stop button until either an HMR'canceled'event arrives or the next poll picks it up. On a slow connection, this looks unresponsive. Also, on failure the toast is shown but the run keeps polling; consider either optimistically setting status to a transient "canceling" state, or at least leaving the button enabled with a clearer error path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 181 - 192, The cancel handler (cancel) currently only calls cancelAgentRun(runId) and waits for a server event to update UI; update it to optimistically update local state so the UI gives immediate feedback: when a runId is found in runs, set that run's status to a transient "canceling" (or similar) in the same state/store that the UI reads (the runs map or run status state) before awaiting cancelAgentRun, and if cancelAgentRun throws revert that status and show toast.error(t.inspector.agentError); also ensure the stop button stays enabled/disabled appropriately by reading this transient status so the button reflects canceling and allows retry if the API fails.
26-65: 💤 Low valueNo request timeouts on dev-server fetches.
checkAgentAvailable,startAgentRun,pollStatus, andcancelAgentRunall fetch without anAbortController/timeout. If the dev-server endpoint hangs (broken plugin, agent stuck), the polling chain will block indefinitely and the cancel button itself can hang. Adding a modestAbortSignal.timeout(...)(or a manualAbortController) per call would make the UX more resilient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 26 - 65, All four network helpers (checkAgentAvailable, startAgentRun, pollStatus, cancelAgentRun) must use an AbortSignal with a modest timeout to avoid hanging — wrap each fetch with AbortSignal.timeout(<ms>) or create an AbortController and set a timer, pass its signal into fetch, and ensure you handle the abort by catching the DOMException/AbortError and returning a sensible fallback (false for checkAgentAvailable, throw a descriptive Error for startAgentRun/pollStatus/cancelAgentRun). Update the fetch calls inside checkAgentAvailable, startAgentRun, pollStatus, and cancelAgentRun to include the signal and clear any timers after completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/superconnector-integration.md:
- Line 5: Replace the current multi-sentence, implementation-heavy changeset
entry with a single present-tense, user-facing line describing the change; e.g.,
"Adds a 'Run with agent' action to inspector comments." Ensure the new line is
concise, present-tense, and does not mention implementation details like
`@nimrobo/superconnector` or HMR.
In `@packages/core/src/app/components/inspector/comment-superconnect.tsx`:
- Around line 148-179: The run function can leak stale intervals because
pollTimers.current.set(commentId, timer) overwrites without clearing an existing
timer; before setting a new timer, check pollTimers.current.get(commentId) and
if present call clearInterval(existing) and delete that entry (or call an
existing stopPolling helper) so the old interval is stopped; then set the new
timer and store it. Also add stopPolling (or the function you introduce that
clears timers) to the useCallback dependency array along with finishRun,
setStatus, slideId, and t.inspector.* so closures reference the latest helper.
- Around line 129-179: The race happens because both the HMR handler (handler in
the useEffect for import.meta.hot) and the poll interval inside run can call
finishRun for the same commentId; update both places to guard terminal
transitions by checking whether the run is still active before calling finishRun
(use pollTimers.current.has(commentId) or another active-run map like runsMap to
detect activity), and ensure you clear/delete the timer entry immediately when
handling a terminal event so the other path will skip calling finishRun; modify
the handler and the interval callback to first if
(!pollTimers.current.has(commentId)) return; (or the inverse check) and then
proceed to clear the timer and call finishRun.
- Around line 258-269: The retry button for status === 'canceled' uses the wrong
icon: it calls runs.run(commentId, line, note) but renders StopCircle (a "stop"
affordance) instead of a "play/retry" icon; update the JSX in the canceled
branch to render a Play (or RotateCw) icon component instead of StopCircle so
the visual matches the action triggered by runs.run (locate the conditional
block checking status === 'canceled' and replace the StopCircle element with the
chosen Play or RotateCw icon).
In `@packages/core/src/vite/superconnector-plugin.ts`:
- Around line 123-124: The Map runs (Map<string, AgentRun>) currently retains
every AgentRun forever; add bounded eviction by removing completed runs after a
configurable TTL (e.g., 5–30 minutes) and/or enforce a max size (LRU) to prevent
unbounded growth. Implement this by: when an AgentRun completes (all code paths
that call success/failure/abort in the run lifecycle), mark it completed and
schedule a delayed removal (setTimeout) that deletes runs.delete(runId) after
the TTL; also add a periodic cleanup/compaction routine or an LRU eviction
policy to prune oldest entries when the map exceeds maxEntries. Update all
places that create/finish runs to use the same finalize hook so completed runs
are consistently removed, and expose TTL/maxEntries as configuration constants
near the runs declaration. Ensure delete logic is idempotent and does not affect
still-active runs.
- Around line 149-154: The POST /runs handler currently assumes readBody(req)
succeeds and lets parse errors bubble to the global catch (500); update the
block that checks if (method === 'POST' && url.pathname === '/runs') to wrap the
readBody(req) call in a try/catch (or check its rejection) and, on JSON
parse/malformed errors, return json(res, 400, { error: 'malformed JSON' })
instead of throwing; do the same fix for the other occurrence around the second
readBody usage (the lines referenced 239-240). Ensure you reference the same
helper symbols: readBody, RunBody, and json so the handlers return 400 for
client-side malformed JSON.
- Around line 140-154: Add an origin/host check to the superconnector middleware
that rejects mutating requests unless they come from an allowed origin/host (or
have a valid CSRF token); specifically update the handler registered via
server.middlewares.use('/__superconnector', ...) and the POST branches for
'/runs' and '/runs/:runId/cancel' to verify req.headers.origin or
req.headers.host (or a configured allowlist) and return 403 for disallowed
requests before reading the body or performing actions; ensure the same guard is
applied to the other POST branch around lines handling cancel (the POST
'/runs/:runId/cancel' handler) so cross-site requests cannot trigger run
creation or cancellation.
---
Nitpick comments:
In `@packages/core/src/app/components/inspector/comment-superconnect.tsx`:
- Around line 87-91: The component only cleans timers on unmount, so when
slideId changes stale timers and entries in pollTimers.current and the Maps
(status, log, runs) persist; add a useEffect that depends on slideId which
iterates pollTimers.current and calls stopPolling (or clearInterval) for each
active timer, clears pollTimers.current, and resets the Maps (status, log, runs)
to empty states so in-flight polls and pending state for the previous slide are
removed whenever slideId changes; reference the existing pollTimers,
stopPolling, finishRun, and the Maps (status/log/runs) and ensure this new
effect runs before starting any new polls for the new slideId.
- Around line 162-175: Replace the setInterval polling loop with a recursive
setTimeout pattern so only one pollStatus(runId) is in-flight at a time:
implement an async function (e.g., pollLoop) that calls pollStatus(runId),
updates setLogMap with the last message for commentId, calls
finishRun(commentId, status, error) when status !== 'pending', and on success
schedules itself with setTimeout(..., 2000); on errors call finishRun(commentId,
'error') and stop or schedule retry as appropriate; ensure you clear the
timer/stop the loop when the component unmounts or run completes.
- Around line 181-192: The cancel handler (cancel) currently only calls
cancelAgentRun(runId) and waits for a server event to update UI; update it to
optimistically update local state so the UI gives immediate feedback: when a
runId is found in runs, set that run's status to a transient "canceling" (or
similar) in the same state/store that the UI reads (the runs map or run status
state) before awaiting cancelAgentRun, and if cancelAgentRun throws revert that
status and show toast.error(t.inspector.agentError); also ensure the stop button
stays enabled/disabled appropriately by reading this transient status so the
button reflects canceling and allows retry if the API fails.
- Around line 26-65: All four network helpers (checkAgentAvailable,
startAgentRun, pollStatus, cancelAgentRun) must use an AbortSignal with a modest
timeout to avoid hanging — wrap each fetch with AbortSignal.timeout(<ms>) or
create an AbortController and set a timer, pass its signal into fetch, and
ensure you handle the abort by catching the DOMException/AbortError and
returning a sensible fallback (false for checkAgentAvailable, throw a
descriptive Error for startAgentRun/pollStatus/cancelAgentRun). Update the fetch
calls inside checkAgentAvailable, startAgentRun, pollStatus, and cancelAgentRun
to include the signal and clear any timers after completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fce1461-efb7-4989-b460-8da1258c1350
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/superconnector-integration.mdpackages/core/package.jsonpackages/core/src/app/components/inspector/comment-superconnect.tsxpackages/core/src/app/components/inspector/comment-widget.tsxpackages/core/src/locale/en.tspackages/core/src/locale/ja.tspackages/core/src/locale/types.tspackages/core/src/locale/zh-cn.tspackages/core/src/locale/zh-tw.tspackages/core/src/vite/config.tspackages/core/src/vite/superconnector-plugin.test.tspackages/core/src/vite/superconnector-plugin.ts
There was a problem hiding this comment.
🧹 Nitpick comments (6)
packages/core/src/vite/superconnector-plugin.ts (2)
192-196: 💤 Low valueConsider
unref()on the cleanup timer.The 10-minute
setTimeoutkeeps a Node timer active and can hold the event loop open. While Vite's dev server lifecycle will usually keep the process alive anyway, callingunref()makes the cleanup non-blocking on graceful shutdown.Suggested tweak
- run.cleanupTimer = setTimeout(() => { + run.cleanupTimer = setTimeout(() => { const current = runs.get(run.runId); if (current && current.status !== 'pending') runs.delete(run.runId); }, COMPLETED_RUN_TTL_MS); + run.cleanupTimer.unref?.();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/superconnector-plugin.ts` around lines 192 - 196, The setTimeout assigned to run.cleanupTimer should be unref'd so the 10-minute cleanup timer doesn't keep the Node event loop alive; after creating the timer in the run.cleanupTimer = setTimeout(...) block (the code around run.cleanupTimer, COMPLETED_RUN_TTL_MS and pruneCompletedRuns), call run.cleanupTimer.unref() if that method exists (guard for environments where unref may be undefined) so the cleanup becomes non-blocking on graceful shutdown.
281-291: 💤 Low value
GET /runs/:runIdpath parsing also matches/runs/abc/cancel.
url.pathname.slice('/runs/'.length)greedily captures everything after/runs/, so a GET to/runs/foo/cancelwould setrunId = 'foo/cancel'and return 404 (rather than fall through or 405). Not exploitable, but tightening the match keeps the route surface predictable:- if (method === 'GET' && url.pathname.startsWith('/runs/')) { - const runId = url.pathname.slice('/runs/'.length); + if (method === 'GET' && /^\/runs\/[^/]+$/.test(url.pathname)) { + const runId = url.pathname.slice('/runs/'.length);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/superconnector-plugin.ts` around lines 281 - 291, The current GET handler uses url.pathname.startsWith('/runs/') and then slices everything after '/runs/' into runId, which lets paths like '/runs/foo/cancel' match; update the route guard to only accept a single-segment runId (no extra slashes) before using runId and runs.get. Concretely, replace the startsWith + slice logic with a stricter check (e.g., test url.pathname against /^\/runs\/[^/]+$/ or compute const runId = url.pathname.slice('/runs/'.length); if (runId.includes('/')) return to let other routes handle it) and then use runs.get(runId) and the existing json responses.packages/core/src/app/components/inspector/comment-superconnect.tsx (4)
305-338: ⚡ Quick winAdd
aria-labelto icon-only buttons for screen reader parity.The stop button at line 286 sets both
titleandaria-label, but the error retry (line 307), canceled retry (line 319), and default run (line 330) buttons rely ontitleonly.titleis not consistently exposed as the accessible name by all screen readers/browsers, so icon-only buttons can end up announced as "button" with no purpose.Suggested fix
<button type="button" onClick={() => runs.run(commentId, line, note)} className="rounded p-1 text-red-600 hover:bg-muted" title={t.inspector.agentError} + aria-label={t.inspector.agentError} > <XCircle className="size-3.5" /> </button> ... <button type="button" onClick={() => runs.run(commentId, line, note)} className="rounded p-1 text-muted-foreground hover:bg-muted hover:text-foreground" title={t.inspector.agentCanceled} + aria-label={t.inspector.agentCanceled} > <Play className="size-3.5" /> </button> ... <button type="button" onClick={() => runs.run(commentId, line, note)} className="rounded p-1 text-muted-foreground hover:bg-muted hover:text-foreground" title={t.inspector.runWithAgent} + aria-label={t.inspector.runWithAgent} > <Play className="size-3.5" /> </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 305 - 338, The icon-only buttons in comment-superconnect.tsx for status 'error' (the XCircle retry button), 'canceled' (the Play retry button), and the default run button are missing aria-label attributes; add aria-label props to each button using the same localized strings already used in title (t.inspector.agentError, t.inspector.agentCanceled, t.inspector.runWithAgent) to ensure screen readers announce the button purpose (leave existing title intact).
28-43: 💤 Low value
fetchWithTimeoutsilently drops a caller-suppliedsignal.Spreading
initfirst then overwritingsignal: controller.signalmeans any externalAbortSignalpassed by a caller would be ignored. No caller currently passes one, but if future code wants composable cancellation, this will silently break. Worth either documenting or composing signals viaAbortSignal.any([controller.signal, init?.signal])(Node 20+/modern browsers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 28 - 43, fetchWithTimeout currently overwrites any caller-supplied AbortSignal (init?.signal) by spreading init then setting signal: controller.signal; change it to compose the signals instead of discarding the caller's: create the timeout controller (controller) and combine controller.signal with init?.signal using AbortSignal.any([controller.signal, init?.signal]) when available, or implement a small fallback that forwards abortion from either signal to a new controller; pass the composed signal to fetch and keep the existing timeout/error behavior (throw `${label} timed out` when the timeout controller aborts) while still clearing the timer in finally. Ensure you reference fetchWithTimeout, controller, init?.signal, and FETCH_TIMEOUT_MS when making the change.
237-250: 💤 Low valueCancel error path can desync UI from server state.
On a failed
cancelAgentRun, status is reverted from'canceling'back to'pending'. If the cancel actually reached the server but the response failed (timeout, transient 5xx), the server may still abort the run while the UI shows'pending'— eventually the HMRcanceledevent or a poll will reconcile it, but the user just saw an error toast for a "failed" cancel that succeeded. Consider keeping'canceling'and letting the terminal event reconcile, or only reverting on explicit non-cancellable responses (e.g. 404).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 237 - 250, The cancel handler (cancel) should not blindly revert status to 'pending' on any cancelAgentRun error; instead keep the status as 'canceling' so the HMR-event/poll can reconcile unless the error explicitly indicates a non-cancellable outcome (e.g., a 404 or an error payload saying the run doesn't exist). Modify the catch block around cancelAgentRun(runId) to inspect the thrown error (status/code/message) and only call setStatus(commentId, 'pending') when the error clearly means "not found" or "already finished"; for all other errors keep the 'canceling' status and still call toast.error(t.inspector.agentError) so the user is informed without desync. Ensure you reference cancel, cancelAgentRun, setStatus, runs and t.inspector.agentError when updating the logic.
168-191: ⚡ Quick winRefactor to use the
statusfield instead ofmsgTypefor terminal detection.The HMR handler currently mixes
AgentMessageType(from agent stream messages) andAgentRunStatus(terminal statuses) in themsgTypefield. While collision risk is low in practice (typical agent SDKs use message types like'text','tool_use'rather than lifecycle terms), a cleaner approach is available: theSuperconnectorEventalready includes a separatestatusfield that directly indicates terminal states. Replace themsgType === 'done' | 'error' | 'canceled'checks withstatus !== 'pending'to be explicit about lifecycle logic, removing the overloading altogether.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/app/components/inspector/comment-superconnect.tsx` around lines 168 - 191, The HMR handler currently checks msgType for terminal events; switch it to use the SuperconnectorEvent.status field instead: in the handler (inside useEffect) replace the msgType === 'done'|'error'|'canceled' branches with a single branch that tests if status !== 'pending' and, if so, verifies pollTimers.current.has(commentId), calls stopPolling(commentId) and then finishRun(commentId, status, status === 'error' ? text : undefined); otherwise (status === 'pending') keep the existing log/status update path that sets setLogMap and setStatus; keep the import.meta.hot.on/off registration and all referenced symbols (handler, pollTimers, stopPolling, finishRun, setLogMap, setStatus) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/app/components/inspector/comment-superconnect.tsx`:
- Around line 305-338: The icon-only buttons in comment-superconnect.tsx for
status 'error' (the XCircle retry button), 'canceled' (the Play retry button),
and the default run button are missing aria-label attributes; add aria-label
props to each button using the same localized strings already used in title
(t.inspector.agentError, t.inspector.agentCanceled, t.inspector.runWithAgent) to
ensure screen readers announce the button purpose (leave existing title intact).
- Around line 28-43: fetchWithTimeout currently overwrites any caller-supplied
AbortSignal (init?.signal) by spreading init then setting signal:
controller.signal; change it to compose the signals instead of discarding the
caller's: create the timeout controller (controller) and combine
controller.signal with init?.signal using AbortSignal.any([controller.signal,
init?.signal]) when available, or implement a small fallback that forwards
abortion from either signal to a new controller; pass the composed signal to
fetch and keep the existing timeout/error behavior (throw `${label} timed out`
when the timeout controller aborts) while still clearing the timer in finally.
Ensure you reference fetchWithTimeout, controller, init?.signal, and
FETCH_TIMEOUT_MS when making the change.
- Around line 237-250: The cancel handler (cancel) should not blindly revert
status to 'pending' on any cancelAgentRun error; instead keep the status as
'canceling' so the HMR-event/poll can reconcile unless the error explicitly
indicates a non-cancellable outcome (e.g., a 404 or an error payload saying the
run doesn't exist). Modify the catch block around cancelAgentRun(runId) to
inspect the thrown error (status/code/message) and only call
setStatus(commentId, 'pending') when the error clearly means "not found" or
"already finished"; for all other errors keep the 'canceling' status and still
call toast.error(t.inspector.agentError) so the user is informed without desync.
Ensure you reference cancel, cancelAgentRun, setStatus, runs and
t.inspector.agentError when updating the logic.
- Around line 168-191: The HMR handler currently checks msgType for terminal
events; switch it to use the SuperconnectorEvent.status field instead: in the
handler (inside useEffect) replace the msgType === 'done'|'error'|'canceled'
branches with a single branch that tests if status !== 'pending' and, if so,
verifies pollTimers.current.has(commentId), calls stopPolling(commentId) and
then finishRun(commentId, status, status === 'error' ? text : undefined);
otherwise (status === 'pending') keep the existing log/status update path that
sets setLogMap and setStatus; keep the import.meta.hot.on/off registration and
all referenced symbols (handler, pollTimers, stopPolling, finishRun, setLogMap,
setStatus) unchanged.
In `@packages/core/src/vite/superconnector-plugin.ts`:
- Around line 192-196: The setTimeout assigned to run.cleanupTimer should be
unref'd so the 10-minute cleanup timer doesn't keep the Node event loop alive;
after creating the timer in the run.cleanupTimer = setTimeout(...) block (the
code around run.cleanupTimer, COMPLETED_RUN_TTL_MS and pruneCompletedRuns), call
run.cleanupTimer.unref() if that method exists (guard for environments where
unref may be undefined) so the cleanup becomes non-blocking on graceful
shutdown.
- Around line 281-291: The current GET handler uses
url.pathname.startsWith('/runs/') and then slices everything after '/runs/' into
runId, which lets paths like '/runs/foo/cancel' match; update the route guard to
only accept a single-segment runId (no extra slashes) before using runId and
runs.get. Concretely, replace the startsWith + slice logic with a stricter check
(e.g., test url.pathname against /^\/runs\/[^/]+$/ or compute const runId =
url.pathname.slice('/runs/'.length); if (runId.includes('/')) return to let
other routes handle it) and then use runs.get(runId) and the existing json
responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c54dc6cc-6323-4aa0-b693-992f3ed97ee5
📒 Files selected for processing (4)
.changeset/superconnector-integration.mdpackages/core/src/app/components/inspector/comment-superconnect.tsxpackages/core/src/vite/superconnector-plugin.test.tspackages/core/src/vite/superconnector-plugin.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/superconnector-integration.md
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.
Summary
/__superconnector/*) for starting, polling, and canceling agent runs scoped to a single inspector comment.@nimrobo/superconnector ^0.1.0as a runtime dependency.Notes for reviewers
pnpm typecheckfails onpackages/core/src/app/components/player.tsx:53— this is pre-existing onmainand not introduced by this PR.Test plan
pnpm checkcleanpnpm test— 168 tests pass (incl. 4 new superconnector-plugin tests)Summary by CodeRabbit
New Features
Localization
Tests
Chores