Repository navigation
fix(vscode): warn on workspace-overridden settings + validate Direct Connect host URL - #1263
EdwardLien0426 wants to merge 6 commits into
Conversation
…Connect host URL applyAllSettings now detects keys shadowed by a conflicting value in the workspace's .vscode/settings.json (via config.inspect) and returns them so the save path shows a warning instead of a misleading "saved successfully" that silently reverts. Also validates that on-prem (Direct Connect) requires a non-empty Host URL, mirroring the cloud teamId check. Fixes rocketride-org#1257 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds Direct Connect host URL validation, workspace override detection, serialized settings writes, warning feedback, related tests, documentation, and compiled test execution in the VS Code build. ChangesVS Code Settings Persistence and Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsWebview
participant SettingsProvider
participant ConfigManager
participant VSCodeConfiguration
SettingsWebview->>SettingsProvider: submit settings snapshot
SettingsProvider->>SettingsProvider: validate Direct Connect host URLs
SettingsProvider->>ConfigManager: applyAllSettings(snapshot)
ConfigManager->>VSCodeConfiguration: inspect workspace and folder scopes
VSCodeConfiguration-->>ConfigManager: return scoped values
ConfigManager->>VSCodeConfiguration: write guarded global values
ConfigManager-->>SettingsProvider: return shadowedKeys
SettingsProvider-->>SettingsWebview: show success or override warning
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Thanks Edward — the core detection logic is right (inspect() precedence handled correctly, !== undefined presence checks, warning only on explicit save) and the bug still exists on develop, but requesting changes because the branch needs a real rebase: it conflicts with current develop in both config.ts and SettingsProvider.ts, and the drift is semantic — several instrumented keys (teamId, local.debugOutput, local.engineArgs) no longer exist, four new ones (pipelineTTL, pipelineTraceLevel, taskArguments, pipelineDebugOutput) would silently lack shadow detection, and the block the host-URL check anchors to was deleted. When rebasing, please also decide on the untouched-key clobber (saving any unrelated setting silently writes workspace-pinned values into the user's global settings — either skip those writes or call it out as an explicit non-goal, since the PR claims to fix silent divergence), and consider the smaller items: the WelcomeProvider save path bypasses both the warning and validation, the host-URL check only rejects empty strings ("asdf" saves fine — a permissive new URL() check or a reworded title would close the gap), getConfiguration() without a resource URI misses folder-level overrides in multi-root workspaces, and the new tests aren't wired into CI. Happy to re-review quickly once rebased. |
…ings-workspace-override # Conflicts: # apps/vscode/src/config.ts # apps/vscode/src/providers/SettingsProvider.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/vscode/src/config.ts`:
- Around line 457-461: Update SettingsProvider.loadAllSettings() and its write
flow so the webview supplies the keys the user actually changed, then restrict
wc.update calls to those changed keys rather than the full effective settings
snapshot. Keep isShadowedByWorkspace() warning behavior for keys being written,
and do not overwrite unrelated Global settings with workspace-pinned values.
- Around line 452-459: Update the configuration-save flow around the local write
helper and its getConfiguration call so multi-root workspaces inspect each
workspace folder using its vscode.WorkspaceFolder.uri scope before deciding
whether to record a shadowed key. Ensure every global write is checked against
the applicable folder-level override instead of relying on the unscoped wc
configuration, while preserving the existing shadowedKeys reporting behavior.
🪄 Autofix
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 Plus
Run ID: 4ee1b803-76ba-400a-add1-2b3451d2b22a
📒 Files selected for processing (4)
apps/vscode/src/config.tsapps/vscode/src/providers/SettingsProvider.tsapps/vscode/src/shared/util/workspaceOverride.tsapps/vscode/src/test/workspaceOverride.test.ts
…s, validate host URLs Addresses the review on rocketride-org#1263. - Never write a key whose submitted value already matches the effective one. The Settings page is populated with effective values, so every key round- tripped on save and a workspace-pinned value was copied into the user's Global settings, changing it for every other window. - Never write a key the form did not supply. The Welcome page renders a subset of the settings, so `update(key, undefined)` silently removed pipelineTTL, pipelineTraceLevel, taskArguments and pipelineDebugOutput. - Inspect every workspace folder, not just the unscoped configuration, so a folder-level override in a multi-root workspace is reported rather than silently masking the write. - Validate Direct Connect host URLs by parse rather than emptiness, mirroring RocketRideClient.normalizeUri so bare hosts ("localhost:5565") stay valid. - Apply both the validation and the shadowed-key warning on the Welcome save path, which bypassed them. The panel closes on save, so the warning goes to a window notification there. - Add vscode:test to the build so apps/vscode/src/test ran by CI at all, and correct connectionModeAuth.test.ts, stale since rocketride-org#678 moved cloud to OAuth and made on-prem the only manual-key mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
docs
There was a problem hiding this comment.
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/providers/SettingsProvider.ts (1)
363-369: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe workspace and workspace-folder overrides accurately.
shadowedKeysis surfaced when a workspace or workspace-folder value conflicts with the saved user value, but the warning says.vscode/settings.json. Users can hit this from a multi-root.code-workspaceworkspace value as well.
apps/vscode/src/providers/SettingsProvider.ts#L363-L369: replace the.vscode/settings.jsonwording with “workspace or workspace-folder settings”.apps/vscode/src/providers/WelcomeProvider.ts#L246-L250: use the same workspace-scope wording in the window notification.apps/vscode/src/config.ts#L444-L448: correct theshadowedKeysJSDoc description.apps/vscode/docs/installation.md#L63-L68: document both workspace-file and workspace-folder override locations.🤖 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 `@apps/vscode/src/providers/SettingsProvider.ts` around lines 363 - 369, Update the shadowed-key messaging and documentation to describe both workspace and workspace-folder overrides: in apps/vscode/src/providers/SettingsProvider.ts lines 363-369 and apps/vscode/src/providers/WelcomeProvider.ts lines 246-250, replace the .vscode/settings.json wording with “workspace or workspace-folder settings”; in apps/vscode/src/config.ts lines 444-448, correct the shadowedKeys JSDoc; and in apps/vscode/docs/installation.md lines 63-68, document workspace-file and workspace-folder override locations.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@apps/vscode/src/providers/SettingsProvider.ts`:
- Around line 363-369: Update the shadowed-key messaging and documentation to
describe both workspace and workspace-folder overrides: in
apps/vscode/src/providers/SettingsProvider.ts lines 363-369 and
apps/vscode/src/providers/WelcomeProvider.ts lines 246-250, replace the
.vscode/settings.json wording with “workspace or workspace-folder settings”; in
apps/vscode/src/config.ts lines 444-448, correct the shadowedKeys JSDoc; and in
apps/vscode/docs/installation.md lines 63-68, document workspace-file and
workspace-folder override locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 607c47c7-17d1-40f1-9ada-e6ebd7b3ed56
📒 Files selected for processing (10)
apps/vscode/docs/installation.mdapps/vscode/scripts/tasks.jsapps/vscode/src/config.tsapps/vscode/src/providers/SettingsProvider.tsapps/vscode/src/providers/WelcomeProvider.tsapps/vscode/src/shared/util/hostUrl.tsapps/vscode/src/shared/util/workspaceOverride.tsapps/vscode/src/test/connectionModeAuth.test.tsapps/vscode/src/test/hostUrl.test.tsapps/vscode/src/test/workspaceOverride.test.ts
Addresses CodeRabbit on rocketride-org#1263: the warning named .vscode/settings.json, but inspecting folder scopes means a shadowing value can equally come from a multi-root workspace's .code-workspace file. Pointing at one file sends the user to the wrong place. Both messages, the applyAllSettings JSDoc and the docs now name the workspace and workspace-folder scopes and where each lives, via a single shared constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/vscode/src/providers/WelcomeProvider.ts (1)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
HOST_URL_EXAMPLESin the Welcome flow.
SettingsProvider.saveAllSettings()uses the shared examples, butWelcomeProvider.saveAndConnect()duplicates them. If supported formats change, the two validation messages can become inconsistent. Import the shared constant and use it in this message.Also applies to: 231-239
🤖 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 `@apps/vscode/src/providers/WelcomeProvider.ts` around lines 38 - 42, Update WelcomeProvider.saveAndConnect to reuse the shared HOST_URL_EXAMPLES constant in its host URL validation message instead of duplicating example values. Import HOST_URL_EXAMPLES from the existing shared host URL utility and preserve the current validation behavior while keeping the message consistent with SettingsProvider.saveAllSettings.apps/vscode/src/config.ts (2)
467-487: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
applyAllSettings()transactional or remove the atomic-save contract.Each configuration update completes before the next one, and secure-storage writes occur afterward. If a later operation fails, earlier settings remain persisted. The catch block only refreshes the partial state, so the callers can report a save failure while development and deployment settings are inconsistent. Add rollback for completed writes, or use a persistence design that can commit all changes together.
Also applies to: 490-526
🤖 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 `@apps/vscode/src/config.ts` around lines 467 - 487, Update applyAllSettings() and its write flow to preserve the atomic-save contract: track completed configuration and secure-storage writes, and roll them back if any later operation fails before rethrowing the error. Ensure the catch path restores all previously persisted development and deployment settings, not merely refreshing partial state.
456-457: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialize overlapping
applyAllSettings()calls.
isBatchApplyingis a single boolean, so a secondSettingsProvider.saveAllSettings()orWelcomeProvider.saveAndConnect()call can run while the first call is still writing settings/API keys. The batch flag can clear before the first write set finishes, allowing config listeners to observe partial state. Use an async mutex/queue forapplyAllSettings().🤖 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 `@apps/vscode/src/config.ts` around lines 456 - 457, Serialize applyAllSettings() executions with an async mutex or promise queue so overlapping SettingsProvider.saveAllSettings() and WelcomeProvider.saveAndConnect() calls wait for the active batch to finish. Update the applyAllSettings() flow around isBatchApplying to acquire and release the serialization guard across the entire settings/API-key write set, preserving the existing flag cleanup on 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.
Outside diff comments:
In `@apps/vscode/src/config.ts`:
- Around line 467-487: Update applyAllSettings() and its write flow to preserve
the atomic-save contract: track completed configuration and secure-storage
writes, and roll them back if any later operation fails before rethrowing the
error. Ensure the catch path restores all previously persisted development and
deployment settings, not merely refreshing partial state.
- Around line 456-457: Serialize applyAllSettings() executions with an async
mutex or promise queue so overlapping SettingsProvider.saveAllSettings() and
WelcomeProvider.saveAndConnect() calls wait for the active batch to finish.
Update the applyAllSettings() flow around isBatchApplying to acquire and release
the serialization guard across the entire settings/API-key write set, preserving
the existing flag cleanup on completion.
In `@apps/vscode/src/providers/WelcomeProvider.ts`:
- Around line 38-42: Update WelcomeProvider.saveAndConnect to reuse the shared
HOST_URL_EXAMPLES constant in its host URL validation message instead of
duplicating example values. Import HOST_URL_EXAMPLES from the existing shared
host URL utility and preserve the current validation behavior while keeping the
message consistent with SettingsProvider.saveAllSettings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bcc37e3-2385-4adb-91fb-e9632e96eb00
📒 Files selected for processing (5)
apps/vscode/docs/installation.mdapps/vscode/src/config.tsapps/vscode/src/providers/SettingsProvider.tsapps/vscode/src/providers/WelcomeProvider.tsapps/vscode/src/shared/util/workspaceOverride.ts
…the atomic claim Addresses the second CodeRabbit pass on rocketride-org#1263. - Serialize applyAllSettings(). isBatchApplying is one boolean, so with the Settings page and the Welcome page both able to save, whichever finished first un-suppressed the config listeners while the other was still writing — the half-written state the flag exists to hide. SettingsProvider._isSaving was never read, and WelcomeProvider had no guard at all, so the overlap was reachable. The queue lives in a pure createSerialQueue() with its own tests. - Reuse HOST_URL_EXAMPLES in the Welcome flow instead of repeating the literal, so both validation messages quote what isValidHostUrl actually accepts. - Stop calling the save atomic. VS Code persists each update() as it completes, so a mid-batch failure leaves earlier keys written; the docs now say the batch isolates listeners rather than rolling back. Rollback is not implemented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/providers/WelcomeProvider.ts (1)
217-218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the operation as a batch.
applyAllSettings()can leave earlier writes persisted when a later write fails. ReplaceAtomic config writewithBatched config write.🤖 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 `@apps/vscode/src/providers/WelcomeProvider.ts` around lines 217 - 218, Update the operation description in applyAllSettings() from “Atomic config write” to “Batched config write” to accurately reflect that earlier writes may persist if a later write fails.
🤖 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 `@apps/vscode/src/config.ts`:
- Around line 462-467: Extend the save serialization beyond
applyAllSettingsBatch(): in apps/vscode/src/config.ts lines 462-467, keep the
applyQueue lease active through the complete save flow; in
apps/vscode/src/providers/SettingsProvider.ts lines 353-385, move
reload/reconcile work into that queued flow; and in
apps/vscode/src/providers/WelcomeProvider.ts lines 244-267, likewise serialize
initialization/reconciliation so no provider work overlaps another save.
In `@apps/vscode/src/shared/util/hostUrl.ts`:
- Line 38: Remove the local HOST_URL_EXAMPLES declarations from SettingsProvider
and WelcomeProvider, and import the shared HOST_URL_EXAMPLES export from
hostUrl.ts in both providers. Update each save flow to use the imported constant
so validation messages share one source of truth.
---
Outside diff comments:
In `@apps/vscode/src/providers/WelcomeProvider.ts`:
- Around line 217-218: Update the operation description in applyAllSettings()
from “Atomic config write” to “Batched config write” to accurately reflect that
earlier writes may persist if a later write fails.
🪄 Autofix
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 Plus
Run ID: 30b6de70-007b-47c1-983c-0836b5953279
📒 Files selected for processing (6)
apps/vscode/src/config.tsapps/vscode/src/providers/SettingsProvider.tsapps/vscode/src/providers/WelcomeProvider.tsapps/vscode/src/shared/util/hostUrl.tsapps/vscode/src/shared/util/serialQueue.tsapps/vscode/src/test/serialQueue.test.ts
| * Accepted-input examples for "this Host URL is unusable" messages. Shared so | ||
| * every entry point quotes the same formats as {@link isValidHostUrl} accepts. | ||
| */ | ||
| export const HOST_URL_EXAMPLES = 'localhost:5565 or https://engine.example.com'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use this export in both save flows.
HOST_URL_EXAMPLES is still declared locally in apps/vscode/src/providers/SettingsProvider.ts:38 and apps/vscode/src/providers/WelcomeProvider.ts:38. Remove those declarations and import this export in both providers. Otherwise, the validation messages can diverge and this utility is not the single source of truth.
🤖 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 `@apps/vscode/src/shared/util/hostUrl.ts` at line 38, Remove the local
HOST_URL_EXAMPLES declarations from SettingsProvider and WelcomeProvider, and
import the shared HOST_URL_EXAMPLES export from hostUrl.ts in both providers.
Update each save flow to use the imported constant so validation messages share
one source of truth.
…ings-workspace-override develop moved vscode:build to a policy of "unit tests run under test targets, never as build steps — a normal build must not stream test output", and dropped shared-ui:test from the steps accordingly. vscode:test followed that policy: it is out of the build steps and now chains vscode:compile-typescript itself so it stays safe to invoke on its own. develop also changed tsconfig rootDir from "src" to "../..", which moves the emitted tree from out/test to out/apps/vscode/src/test. The runner globbed a fixed path and would have found nothing while still reporting success, so it now globs **/test/*.test.js and fails loudly when the glob comes up empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@Leela8256 — all six addressed, ready for re-review.
Three things to push back on if you disagree:
Not verified: nothing here has been exercised in a running Extension Development Host — the banner, the Welcome notification, real multi-root URIs and concurrent saves are covered by reading the code and unit tests on the extracted pure logic only. Worth a manual pass before merge. |
|
I re-checked the staleness worry that drove the last round: One small thing that is still open from the bot pass and cheap to close out: |
|
@EdwardLien0426 this is conflicting with
Once it's resolved and CI is green, please ping here for re-review. Thanks! |
|
@EdwardLien0426 one more thing while you're updating this: please add screenshots to the PR description for any UI or visual changes. From the diff, that's at least:
Please take them after the rebase, so they show the current UI. |
This branch has not been deployed
4 participants
Summary
Fixes #1257.
When a settings key is overridden in the workspace's
.vscode/settings.json,ConfigManager.applyAllSettings()wrote every key toConfigurationTarget.Global, so the workspace value kept shadowing the just-written user value. The save then reported "Settings saved successfully!", reloaded the webview from the merged config, and the UI snapped back to the old value — while user-level settings silently diverged (e.g.connectionMode: "onprem"+ emptyhostUrl, breaking the extension in other workspaces).This PR takes the warning approach from the issue's two options (see the discussion comment): keep writing to user settings, but detect the conflict and tell the user, rather than writing into a (usually git-tracked)
.vscode/settings.jsonfrom the panel.Changes
applyAllSettings(config.ts) — each key is written through a small helper that usesconfig.inspect()to detect a conflicting workspace/workspace-folder value (one that differs from what we're saving) before the Global write. The method now returns{ shadowedKeys }.saveAllSettings(SettingsProvider.ts) — when any saved key is shadowed, show awarning(which persists, unlike the auto-dismissed success) explaining the value is overridden by this workspace's.vscode/settings.jsonand won't take effect here, instead of a misleading success.teamIdcheck. (The enum value isonprem; there is nodirect.)isShadowedByWorkspace+ unit tests, kept side-effect-free so it's testable without a live VS Code instance (mirrorsconnectionModeAuth).A matching workspace override (same value the user is saving) is intentionally not flagged, so the warning only fires on real conflicts.
Verification
npx tsc --noEmit— clean (other callers, e.g.WelcomeProvider, ignore the new return value and still compile).npx tsx --test src/test/workspaceOverride.test.ts— 8/8 pass.rocketride.development.connectionMode: "local"pinned in workspace settings, switching to Direct Connect + Save now shows the warning instead of a false success that reverts.Notes for reviewers
src/test/*.test.tsfiles don't appear to be wired intobuilder test, and the existingconnectionModeAuth.test.tsis currently failing ondevelop(impl/test drift). New tests here pass viatsx --testbut aren't run by CI — worth a separate follow-up to wire up the harness..vscode/settings.jsondirectly.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests