Add "Cancel Connection" button to VS Code extension UI#718
Add "Cancel Connection" button to VS Code extension UI#718EdwardLien0426 wants to merge 2 commits intorocketride-org:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 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)
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 |
|
No description provided. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/vscode/src/connection/local-manager.ts (1)
94-110:⚠️ Potential issue | 🟡 MinorCancellation errors during backoff should bypass attempt logging.
When
delayWithTokenrejects due to cancellation, the rejection is caught and treated as a retry attempt (line 95:attempts++), which logs misleading "waiting" UI (lines 105-106) before exiting on the next loop iteration's cancellation guard (lines 86-89).🐛 Proposed fix
} catch (error: unknown) { + if (error instanceof Error && error.message === 'Connection cancelled') { + this.logger.output(`${icons.warning} Connection cancelled by user`); + throw error; + } + attempts++; if (attempts >= MAX_CONNECT_ATTEMPTS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/local-manager.ts` around lines 94 - 110, The catch block currently increments attempts and logs retry info before checking for cancellation, causing cancellation rejections from delayWithToken to be treated as retries; change the flow in the catch around delayWithToken so you first detect a cancellation error (e.g., inspect the caught error for token.isCancellationRequested / token.aborted / AbortError / a CancellationError) and if it is a cancellation rethrow immediately without incrementing attempts, emitting status, or logging; only for non-cancellation errors increment attempts, log via this.logger.output and emit('status'), compute LocalManager.getBackoffDelayMs(attempts - 1) and await delayWithToken as before. Ensure references: delayWithToken, attempts, LocalManager.getBackoffDelayMs, this.logger.output, and emit('status') are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/connection/local-manager.ts`:
- Around line 146-153: The delayWithToken promise currently registers
token?.onCancellationRequested without disposing the returned subscription,
causing listener accumulation; change delayWithToken to capture the Disposable
returned by token.onCancellationRequested (e.g., const subscription =
token?.onCancellationRequested(...)), then ensure you call
subscription?.dispose() in every completion path: when the timeout resolves,
when the timeout is cleared on cancellation, and when the promise rejects, and
still clear the timeout via clearTimeout(timeout) on cancellation so both the
timer and the subscription are cleaned up.
In `@apps/vscode/src/providers/views/PageConnection/PageConnection.tsx`:
- Line 341: Replace the hard-coded cancel colors on the button in
PageConnection.tsx with VS Code theme CSS variables: instead of background
'#f44336' and 'white', use appropriate theme tokens (for example
var(--vscode-inputValidationErrorBackground) for the red background and
var(--vscode-inputValidationErrorForeground) or var(--vscode-button-foreground)
for the text) and wire them through the existing btnStyle('cancel') usage so
hover/active states use theme vars too; update btnStyle (and any inline style
merge around btnStyle('cancel')) to accept/emit CSS variable values rather than
literal hex strings so sendMessage, setHoveredBtn and the button element keep
the same behavior but use theme tokens.
In `@package.json`:
- Around line 93-94: The pnpm devDependency is using a caret range ("pnpm":
"^10.33.0") which can drift from exact pins in CI; update the package.json
devDependencies entry for "pnpm" to the exact version string "10.33.0" (replace
^10.33.0 with 10.33.0) so it matches the packageManager and workflow pins.
---
Outside diff comments:
In `@apps/vscode/src/connection/local-manager.ts`:
- Around line 94-110: The catch block currently increments attempts and logs
retry info before checking for cancellation, causing cancellation rejections
from delayWithToken to be treated as retries; change the flow in the catch
around delayWithToken so you first detect a cancellation error (e.g., inspect
the caught error for token.isCancellationRequested / token.aborted / AbortError
/ a CancellationError) and if it is a cancellation rethrow immediately without
incrementing attempts, emitting status, or logging; only for non-cancellation
errors increment attempts, log via this.logger.output and emit('status'),
compute LocalManager.getBackoffDelayMs(attempts - 1) and await delayWithToken as
before. Ensure references: delayWithToken, attempts,
LocalManager.getBackoffDelayMs, this.logger.output, and emit('status') are
updated accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1d65fee-ef04-4c9c-923e-0e68aafb66c9
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (11)
.github/copilot-instructions.mdapps/.github/copilot-instructions.mdapps/.gitignoreapps/vscode/.github/copilot-instructions.mdapps/vscode/.gitignoreapps/vscode/.vscode/launch.jsonapps/vscode/package.jsonapps/vscode/src/connection/local-manager.tsapps/vscode/src/providers/SidebarConnectionProvider.tsapps/vscode/src/providers/views/PageConnection/PageConnection.tsxpackage.json
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
|
@coderabbitai resolve merge conflict |
|
❌ An unexpected error occurred while resolving merge conflicts: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/connection/local-manager.ts`:
- Around line 86-89: Replace the generic throws of Error('Connection cancelled')
in local-manager.ts with vscode.CancellationError so cancellation flows are
recognized by engine-manager.ts; specifically update the throw in the
token?.isCancellationRequested branch (and the other two places that currently
throw Error('Connection cancelled')) to throw new vscode.CancellationError(),
and add/import the vscode namespace if not already imported so the symbol
resolves.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cde5f1b6-435b-4599-9c32-bbb6cef78153
📒 Files selected for processing (3)
apps/vscode/.vscode/launch.jsonapps/vscode/src/connection/local-manager.tsapps/vscode/src/providers/views/PageConnection/PageConnection.tsx
| if (token?.isCancellationRequested) { | ||
| this.logger.output(`${icons.warning} Connection cancelled by user`); | ||
| throw new Error('Connection cancelled'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== LocalManager cancellation throws ==="
rg -n "Connection cancelled|CancellationError" apps/vscode/src/connection/local-manager.ts -C2
echo
echo "=== Existing CancellationError handling patterns ==="
rg -n "instanceof\\s+vscode\\.CancellationError" apps/vscode/src -C2Repository: rocketride-org/rocketride-server
Length of output: 1830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking LocalManager file for cancellation error patterns ==="
# First, locate and display the specific lines mentioned
if [ -f "apps/vscode/src/connection/local-manager.ts" ]; then
echo "File found. Checking lines 86-89:"
sed -n '86,89p' apps/vscode/src/connection/local-manager.ts || true
echo
echo "Checking lines 149-150:"
sed -n '149,150p' apps/vscode/src/connection/local-manager.ts || true
echo
echo "Checking lines 168-169:"
sed -n '168,169p' apps/vscode/src/connection/local-manager.ts || true
echo
echo "=== All 'Connection cancelled' references in local-manager.ts ==="
rg -n "Connection cancelled" apps/vscode/src/connection/local-manager.ts || true
else
echo "File not found at expected location"
fi
echo
echo "=== Checking for CancellationError usage patterns in vscode connection code ==="
rg -n "CancellationError" apps/vscode/src/connection/ -t ts || true
echo
echo "=== Checking imports in local-manager.ts ==="
head -20 apps/vscode/src/connection/local-manager.ts || trueRepository: rocketride-org/rocketride-server
Length of output: 2528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking vscode import in local-manager.ts ==="
grep -n "import.*vscode" apps/vscode/src/connection/local-manager.ts || echo "No vscode import found in first pass"
echo
echo "=== Full import section of local-manager.ts ==="
sed -n '1,50p' apps/vscode/src/connection/local-manager.ts | grep -E "^import|^from" || true
echo
echo "=== Check for vscode usage elsewhere in local-manager.ts ==="
rg -n "vscode\." apps/vscode/src/connection/local-manager.ts | head -10 || trueRepository: rocketride-org/rocketride-server
Length of output: 869
Standardize cancellation exceptions to vscode.CancellationError.
Lines 88, 149, and 168 use generic Error('Connection cancelled'), while engine-manager.ts checks instanceof vscode.CancellationError for cancellation handling. Throw vscode.CancellationError() instead so cancellation exceptions remain distinguishable from real failures across connection flows.
♻️ Proposed change
- throw new Error('Connection cancelled');
+ throw new vscode.CancellationError();
...
- return reject(new Error('Connection cancelled'));
+ return reject(new vscode.CancellationError());
...
- reject(new Error('Connection cancelled'));
+ reject(new vscode.CancellationError());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/vscode/src/connection/local-manager.ts` around lines 86 - 89, Replace
the generic throws of Error('Connection cancelled') in local-manager.ts with
vscode.CancellationError so cancellation flows are recognized by
engine-manager.ts; specifically update the throw in the
token?.isCancellationRequested branch (and the other two places that currently
throw Error('Connection cancelled')) to throw new vscode.CancellationError(),
and add/import the vscode namespace if not already imported so the symbol
resolves.
|
PR is not ready.
|
e1f837b to
88c5192
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/vscode/src/connection/local-manager.ts (1)
86-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
vscode.CancellationErrorfor user-triggered aborts.These paths still reject with plain
Error('Connection cancelled'), so upstream code cannot reliably distinguish a deliberate cancel from a real connection failure. Since this file already depends onvscode, emitvscode.CancellationErrorhere instead.♻️ Proposed fix
if (token?.isCancellationRequested) { this.logger.output(`${icons.warning} Connection cancelled by user`); - throw new Error('Connection cancelled'); + throw new vscode.CancellationError(); } ... if (token?.isCancellationRequested) { - return reject(new Error('Connection cancelled')); + return reject(new vscode.CancellationError()); } ... sub = token?.onCancellationRequested(() => { cleanup(); - reject(new Error('Connection cancelled')); + reject(new vscode.CancellationError()); });#!/bin/bash set -euo pipefail echo "=== Current cancellation throws ===" rg -n --type=ts 'Connection cancelled|CancellationError' apps/vscode/src/connection -C2Also applies to: 148-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/local-manager.ts` around lines 86 - 89, Replace throwing a plain Error when the cancellation token indicates user abort with vscode.CancellationError so upstream can detect deliberate cancels: where you check token?.isCancellationRequested (the branch that calls this.logger.output(`${icons.warning} Connection cancelled by user`)), throw new vscode.CancellationError() instead of new Error('Connection cancelled'), and ensure the file imports vscode (or uses the existing vscode namespace). Repeat the same change for the other cancellation branches with the same pattern (the similar token checks around the later block noted in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/providers/views/PageConnection/PageConnection.tsx`:
- Line 26: TRANSITIONAL_STATES currently includes 'stopping-engine', causing the
UI and cancel logic to treat an in-progress disconnect as a connect/cancel
transitional state; remove 'stopping-engine' from TRANSITIONAL_STATES (or create
a separate DISCONNECTING_STATES set) and update any checks that rely on
TRANSITIONAL_STATES (e.g., the UI rendering and the handlers that determine
whether to show "Connecting…" or the "Cancel Connection" action) so that
'stopping-engine' is treated as a disconnect/teardown state and does not permit
dispatching a second disconnect while teardown is in progress; ensure references
to ConnectionState['state'] and any conditional logic around connect/cancel use
the new separation.
- Around line 51-52: The UI sends sendMessage({ type: 'openDashboard' }) but
PageConnectionProvider's incoming message handler (the switch over
PageConnectionOutgoingMessage in the provider) has no case for 'openDashboard',
so the Monitor button is a no-op; add 'openDashboard' to the
PageConnectionOutgoingMessage union and implement a matching case in
PageConnectionProvider's message dispatcher (the method that handles incoming
outgoing-page messages) that forwards the intent to the host (call the existing
host bridge method used for other open* actions or emit the same platform
message used by openSettings/openDocs/openDeploy) so the host actually opens the
dashboard/monitor; also apply the same addition for the other UI instance
referenced around lines 359-360 where sendMessage({ type: 'openDashboard' }) is
used.
---
Duplicate comments:
In `@apps/vscode/src/connection/local-manager.ts`:
- Around line 86-89: Replace throwing a plain Error when the cancellation token
indicates user abort with vscode.CancellationError so upstream can detect
deliberate cancels: where you check token?.isCancellationRequested (the branch
that calls this.logger.output(`${icons.warning} Connection cancelled by user`)),
throw new vscode.CancellationError() instead of new Error('Connection
cancelled'), and ensure the file imports vscode (or uses the existing vscode
namespace). Repeat the same change for the other cancellation branches with the
same pattern (the similar token checks around the later block noted in the
review).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d46c002f-a515-4071-9c0e-8c9cf52d1602
📒 Files selected for processing (2)
apps/vscode/src/connection/local-manager.tsapps/vscode/src/providers/views/PageConnection/PageConnection.tsx
|
Hi @EdwardLien0426, thank you for update. But the branch is still not ready. |
d536f9c to
5fd5c2c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/vscode/src/connection/local-manager.ts (1)
86-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
vscode.CancellationErrorin all cancellation paths.Line 88, Line 149, and Line 168 still use generic
Error('Connection cancelled'). This can blur cancellation vs real connection failures in upstream handlers.♻️ Proposed fix
- throw new Error('Connection cancelled'); + throw new vscode.CancellationError(); ... - return reject(new Error('Connection cancelled')); + return reject(new vscode.CancellationError()); ... - reject(new Error('Connection cancelled')); + reject(new vscode.CancellationError());#!/bin/bash set -euo pipefail echo "=== local-manager cancellation throw sites ===" rg -n "Connection cancelled|CancellationError" apps/vscode/src/connection/local-manager.ts -C2 echo echo "=== existing cancellation handling patterns in connection managers ===" rg -n "instanceof\\s+vscode\\.CancellationError|CancellationError" apps/vscode/src/connection -C2Also applies to: 149-150, 166-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/local-manager.ts` around lines 86 - 89, Replace generic throws of new Error('Connection cancelled') with vscode.CancellationError so cancellation is distinguishable from real failures: locate the cancellation checks (e.g., the token?.isCancellationRequested branch and the other two throw sites that use the exact message "Connection cancelled") and change them to throw new vscode.CancellationError() (or new vscode.CancellationError('Connection cancelled') for message consistency); ensure the module imports/uses the vscode symbol where these changes are made (e.g., in local-manager methods that reference token and this.logger.output).apps/vscode/src/providers/views/PageConnection/PageConnection.tsx (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSeparate “disconnecting” from “connecting” state handling.
Including
'stopping-engine'inTRANSITIONAL_STATESmakes the UI show Connecting... and Cancel Connection while teardown is already in progress.♻️ Proposed fix
-const TRANSITIONAL_STATES: ReadonlySet<ConnectionState['state']> = new Set(['connecting', 'downloading-engine', 'starting-engine', 'stopping-engine']); +const CONNECTING_STATES: ReadonlySet<ConnectionState['state']> = new Set(['connecting', 'downloading-engine', 'starting-engine']); +const DISCONNECTING_STATES: ReadonlySet<ConnectionState['state']> = new Set(['stopping-engine']); ... -const isTransitional = connectionData?.connectionState.state && TRANSITIONAL_STATES.has(connectionData.connectionState.state); +const isTransitional = connectionData?.connectionState.state + ? CONNECTING_STATES.has(connectionData.connectionState.state) || DISCONNECTING_STATES.has(connectionData.connectionState.state) + : false; ... -if (TRANSITIONAL_STATES.has(state)) return `Connecting${getAnimatedDots()}`; +if (CONNECTING_STATES.has(state)) return `Connecting${getAnimatedDots()}`; +if (DISCONNECTING_STATES.has(state)) return `Disconnecting${getAnimatedDots()}`; ... -const isConnecting = connectionData?.connectionState.state ? TRANSITIONAL_STATES.has(connectionData.connectionState.state) : false; +const isConnecting = connectionData?.connectionState.state ? CONNECTING_STATES.has(connectionData.connectionState.state) : false;Also applies to: 233-238, 281-282, 340-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageConnection/PageConnection.tsx` at line 26, TRANSITIONAL_STATES currently includes 'stopping-engine', which causes teardown to be treated as a "connecting" transition; remove 'stopping-engine' from the TRANSITIONAL_STATES set and add an explicit check (e.g., const isDisconnecting = connection.state === 'stopping-engine') in PageConnection.tsx where UI decisions are made so you can render disconnecting-specific labels/buttons (like "Disconnecting..." / "Cancel Disconnection") instead of the connecting ones; update all places that relied on TRANSITIONAL_STATES (the render/conditional logic referencing TRANSITIONAL_STATES, and any derived booleans) to use the new isDisconnecting check so teardown state is handled separately from connecting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/vscode/src/connection/local-manager.ts`:
- Around line 86-89: Replace generic throws of new Error('Connection cancelled')
with vscode.CancellationError so cancellation is distinguishable from real
failures: locate the cancellation checks (e.g., the
token?.isCancellationRequested branch and the other two throw sites that use the
exact message "Connection cancelled") and change them to throw new
vscode.CancellationError() (or new vscode.CancellationError('Connection
cancelled') for message consistency); ensure the module imports/uses the vscode
symbol where these changes are made (e.g., in local-manager methods that
reference token and this.logger.output).
In `@apps/vscode/src/providers/views/PageConnection/PageConnection.tsx`:
- Line 26: TRANSITIONAL_STATES currently includes 'stopping-engine', which
causes teardown to be treated as a "connecting" transition; remove
'stopping-engine' from the TRANSITIONAL_STATES set and add an explicit check
(e.g., const isDisconnecting = connection.state === 'stopping-engine') in
PageConnection.tsx where UI decisions are made so you can render
disconnecting-specific labels/buttons (like "Disconnecting..." / "Cancel
Disconnection") instead of the connecting ones; update all places that relied on
TRANSITIONAL_STATES (the render/conditional logic referencing
TRANSITIONAL_STATES, and any derived booleans) to use the new isDisconnecting
check so teardown state is handled separately from connecting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff0ee75d-0c70-484f-b55f-e22fabeca5eb
📒 Files selected for processing (2)
apps/vscode/src/connection/local-manager.tsapps/vscode/src/providers/views/PageConnection/PageConnection.tsx
|
Heads up — the Lin/Win/Mac builds here are red because of a TypeScript compile error, not the CI flake everyone else is hitting: You're passing |
d4b3210 to
7606b82
Compare
|
@EdwardLien0426 I can get behind this being added, but I'd like to see how this is displayed in the UI. Would you please add a screen recording of you showing this change in the extension UI? |
PoHsu.Edward.Lien_PR_CancelConnectionButton.mp4Hello! This is the demo! |
7606b82 to
3c93241
Compare

Description
Currently, when the extension enters an infinite 401 loop or network timeout, users are stuck in the 'Connecting' state without a way to abort. This PR provides a manual override to improve UX in such scenarios.
This PR introduces a "Cancel Connection" button in the VS Code extension's Connection page. This allows users to interrupt an ongoing connection attempt, improving the user experience during connection states.
Changes
VS Code Extension UI (PageConnection.tsx):
Logic Adjustments:
How to Test
Note to the team
My name is Po-Hsu Lien, and I am currently an applicant for an internship at your company. While exploring the codebase, I encountered this UX issue and wanted to contribute a adjustment. I'm excited about the work you're doing and hope this PR is helpful!
Here is my email: POHSU@csu.fullerton.edu
Summary by CodeRabbit
New Features
Bug Fixes
UX