Skip to content

fix(vscode): warn on workspace-overridden settings + validate Direct Connect host URL#1263

Open
EdwardLien0426 wants to merge 1 commit into
rocketride-org:developfrom
EdwardLien0426:fix/RR-1257-settings-workspace-override
Open

fix(vscode): warn on workspace-overridden settings + validate Direct Connect host URL#1263
EdwardLien0426 wants to merge 1 commit into
rocketride-org:developfrom
EdwardLien0426:fix/RR-1257-settings-workspace-override

Conversation

@EdwardLien0426

@EdwardLien0426 EdwardLien0426 commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1257.

When a settings key is overridden in the workspace's .vscode/settings.json, ConfigManager.applyAllSettings() wrote every key to ConfigurationTarget.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" + empty hostUrl, 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.json from the panel.

Changes

  • applyAllSettings (config.ts) — each key is written through a small helper that uses config.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 a warning (which persists, unlike the auto-dismissed success) explaining the value is overridden by this workspace's .vscode/settings.json and won't take effect here, instead of a misleading success.
  • Direct Connect validation — on-prem mode now requires a non-empty Host URL on save (both development and deployment groups), mirroring the existing cloud teamId check. (The enum value is onprem; there is no direct.)
  • New pure util isShadowedByWorkspace + unit tests, kept side-effect-free so it's testable without a live VS Code instance (mirrors connectionModeAuth).

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.
  • Manual repro in an Extension Development Host: with 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

  • The VS Code extension's src/test/*.test.ts files don't appear to be wired into builder test, and the existing connectionModeAuth.test.ts is currently failing on develop (impl/test drift). New tests here pass via tsx --test but aren't run by CI — worth a separate follow-up to wire up the harness.
  • Out of scope here: writing to the effective (workspace) target — can be a follow-up if the team prefers the panel to edit .vscode/settings.json directly.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced detection to warn when workspace-local settings override your global configuration changes.
    • Added validation for Direct Connect mode requiring a non-empty host URL for both development and deployment connections.
  • Tests

    • Added comprehensive test coverage for workspace settings override detection, including edge cases and array/boolean handling.

…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>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 14e0ef02-e314-4bc6-9e57-81ca8619117a

📥 Commits

Reviewing files that changed from the base of the PR and between d5237d9 and 5e939fc.

📒 Files selected for processing (4)
  • apps/vscode/src/config.ts
  • apps/vscode/src/providers/SettingsProvider.ts
  • apps/vscode/src/shared/util/workspaceOverride.ts
  • apps/vscode/src/test/workspaceOverride.test.ts

📝 Walkthrough

Walkthrough

This PR implements workspace settings override detection to fix issue #1257. When users save settings in the RocketRide VS Code extension, the system now detects if workspace-level .vscode/settings.json values shadow the global configuration writes, adds Direct Connect host URL validation, and provides conditional UI feedback indicating whether the save succeeded or was partially overridden by workspace settings.

Changes

Workspace Override Detection and Save Feedback

Layer / File(s) Summary
Workspace override detection utility and tests
apps/vscode/src/shared/util/workspaceOverride.ts, apps/vscode/src/test/workspaceOverride.test.ts
Exports WorkspaceInspection<T> interface and isShadowedByWorkspace function to determine whether a desired global config value is masked by workspace or workspace-folder overrides. Uses structural equality checking (strict === with JSON fallback for objects/arrays). Includes comprehensive unit tests covering undefined inputs, conflicting vs. matching overrides, folder-level precedence, array equality, and falsy value semantics.
ConfigManager integration with shadow detection
apps/vscode/src/config.ts
applyAllSettings now imports isShadowedByWorkspace and inspects each configuration key before writing. Return type changes from Promise<void> to Promise<{ shadowedKeys: string[] }>, accumulating masked keys (formatted as ${section}.${key}) and returning them after a single cache refresh from the final state.
Settings Provider validation and conditional feedback
apps/vscode/src/providers/SettingsProvider.ts
Adds pre-save validation requiring Direct Connect (onprem mode) configurations to include non-empty trimmed hostUrl for both development and deployment targets. The save workflow now awaits applyAllSettings to consume its shadowedKeys result, showing a success message when no keys are overridden or a warning message when workspace settings shadow the saved values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

module:vscode

Suggested reviewers

  • jmaionchi
  • stepmikhaylov
  • Rod-Christensen

Poem

🐰 A workspace once shadowed the global decree,
Settings saved gracefully, but you couldn't see!
Now we detect when workspace says "no,"
And warn when your changes can't truly flow.
Direct Connect gains teeth—hostUrl won't hide,
The shadow's exposed; the truth can't slide! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: detecting workspace-overridden settings and validating Direct Connect host URLs.
Linked Issues check ✅ Passed All coding requirements from issue #1257 are met: workspace override detection with warnings, settings divergence prevention, and Direct Connect validation.
Out of Scope Changes check ✅ Passed All changes directly address the requirements in issue #1257 with no extraneous modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the module:vscode VS Code extension label Jun 12, 2026
@github-actions

Copy link
Copy Markdown
🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:vscode VS Code extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VSCode extension: Save All Settings shows success but the UI reverts when workspace settings override saved keys

1 participant