feature: unified-shell-resolution (1/4) - #1120
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR adds typed terminal shell selection for automatic, profile, and explicit path modes. It adds extension-host messages, inline settings UI, buffered save behavior, validation tests, terminal status fields, and localization strings. ChangesTerminal shell selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant TerminalSettings
participant ExtensionHost
TerminalSettings->>ExtensionHost: requestTerminalShellOptions
ExtensionHost-->>TerminalSettings: terminalShellOptions payload
TerminalSettings->>SettingsView: onShellSelectionChange(selection)
SettingsView->>ExtensionHost: setTerminalShellSelection on save
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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
`@webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx`:
- Around line 33-43: Replace the any-typed capturedTerminalProps and mocked
TerminalSettings props with the existing TerminalSettingsProps shape, preserving
a nullable current value. Import or reference the webview-ui package’s
TerminalSettingsProps type using the correct package path, and type the mock
callback contract through onShellSelectionChange instead of suppressing it with
as any.
- Around line 93-110: Update the partial vi.mock factories for the Tab module
and ToggleSwitch module to be async, await vi.importActual (or use
importOriginal), and spread the resolved exports before applying the Tab* and
ToggleSwitch overrides. Preserve the existing mock implementations while
ensuring all unmodified exports remain available.
In
`@webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx`:
- Around line 164-175: Update the profile-selection test to use getByTestId for
the PowerShell option instead of queryByTestId, remove the conditional guard,
and execute the click plus onShellSelectionChange and
onTerminalProfilePickerOpened assertions unconditionally.
In `@webview-ui/src/components/settings/SettingsView.tsx`:
- Line 198: Update SettingsView.tsx lines 198-198 to hydrate
terminalShellSelection into cachedState without marking the form dirty; update
lines 463-477 to track the submitted selection and clear or reconcile the
pending value after host acknowledgement; update lines 917-926 to render the
reconciled cached selection. Add separate tests in
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx
lines 317-345 covering persisted-selection hydration with Save disabled and
host-state acknowledgement after Save.
In `@webview-ui/src/components/settings/TerminalSettings.tsx`:
- Around line 301-305: In TerminalSettings, add a typed response handler for the
requestCustomShellPath result that converts the selected path into a
TerminalShellSelection and passes it to onShellSelectionChange. Ensure the
existing SettingsView buffering flow persists the selection through
setTerminalShellSelection only when Save is used.
- Around line 317-327: Update the effective shell display in TerminalSettings to
render shellOptions.effectiveShell.label alongside the existing family and
source values, using the corresponding effectiveShell.label translation key. Add
an assertion in TerminalSettings.shell.spec.tsx covering the resolved executable
label.
- Around line 335-340: Update the shellError rendering in TerminalSettings to
use the unavailable-shell translation for TerminalShellOptionsPayload discovery
failures instead of the selection-specific invalid message. Preserve invalid
messaging only when the error represents an unsupported selected shell, and
update the relevant shell test to assert the unavailable message.
In `@webview-ui/src/i18n/locales/de/settings.json`:
- Line 851: Update the German settings strings around the description and the
related entries at the referenced symbols to use the feminine grammatical forms:
“die Shell”, “Aktive Shell”, and “Die ausgewählte Shell”. Preserve the existing
meanings and surrounding localization text.
In `@webview-ui/src/i18n/locales/pt-BR/settings.json`:
- Line 860: The fallbackDescription field in the pt-BR settings translation uses
"os comandos são reutilizados" (commands are reused) but should instead use "os
comandos são executados novamente" (commands are executed again) to accurately
describe the fallback behavior. Update the fallbackDescription string to replace
the phrase about reusing with the correct phrase about executing commands again.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd97182f-570b-4d50-a157-1e3fd8b44b8f
📒 Files selected for processing (26)
packages/types/src/__tests__/terminal-shell-settings.spec.tspackages/types/src/global-settings.tspackages/types/src/terminal.tspackages/types/src/vscode-extension-host.tswebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/components/settings/TerminalSettings.tsxwebview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsxwebview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
| // Capture the props SettingsView passes to TerminalSettings so tests can | ||
| // drive onShellSelectionChange directly. | ||
| const capturedTerminalProps = vi.hoisted(() => ({ current: null as any })) | ||
|
|
||
| vi.mock("../TerminalSettings", () => ({ | ||
| DEFAULT_PROFILE_VALUE: "__zoo_code_follow_vscode_sentinel__", | ||
| TerminalSettings: (props: any) => { | ||
| capturedTerminalProps.current = props | ||
| return <div data-testid="terminal-settings-stub" /> | ||
| }, | ||
| })) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 \
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsxRepository: Zoo-Code-Org/Zoo-Code
Length of output: 922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'repo files matching target:\n'
git ls-files | grep -F 'webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx' || true
printf '\npackage location:\n'
fd -a 'package.json$' . | sed 's#^\./##' | head -20
printf '\ntry lint from repo root with relative pattern:\n'
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 \
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx || true
printf '\ntry lint with file directly:\n'
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 \
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx || true
printf '\nsearch test file contents:\n'
sed -n '1,130p' webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx
printf '\nsearch related source exports/types:\n'
rg -n "type TerminalShellSelection|interface TerminalShellSelection|enum TerminalShellSelection|onShellSelectionChange|TerminalSettings" webview-ui/src/components/settings -SRepository: Zoo-Code-Org/Zoo-Code
Length of output: 14000
Replace the untyped terminal-settings test double.
The settings subdirectory is not inside package src, so ESLint needs the webview-ui package path. The file also uses as any on lines 36 and 39, which hides the SettingsView → TerminalSettings callback contract. Use the existing TerminalSettingsProps/onShellSelectionChange shape with a nullable current.
🤖 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
`@webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx`
around lines 33 - 43, Replace the any-typed capturedTerminalProps and mocked
TerminalSettings props with the existing TerminalSettingsProps shape, preserving
a nullable current value. Import or reference the webview-ui package’s
TerminalSettingsProps type using the correct package path, and type the mock
callback contract through onShellSelectionChange instead of suppressing it with
as any.
Source: Coding guidelines
| vi.mock("../../../components/common/Tab", () => ({ | ||
| ...vi.importActual("../../../components/common/Tab"), | ||
| Tab: ({ children }: any) => <div data-testid="tab-container">{children}</div>, | ||
| TabHeader: ({ children }: any) => <div data-testid="tab-header">{children}</div>, | ||
| TabContent: ({ children, "data-testid": dataTestId }: any) => ( | ||
| <div data-testid={dataTestId || "tab-content"}>{children}</div> | ||
| ), | ||
| TabList: ({ children, value, "data-testid": dataTestId }: any) => ( | ||
| <div data-testid={dataTestId} data-value={value}> | ||
| {children} | ||
| </div> | ||
| ), | ||
| TabTrigger: ({ children, value, "data-testid": dataTestId, onClick, isSelected }: any) => ( | ||
| <button data-testid={dataTestId} data-value={value} data-selected={isSelected} onClick={onClick}> | ||
| {children} | ||
| </button> | ||
| ), | ||
| })) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'vi\.mock\(|vi\.importActual\(' \
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 \
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsxRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'SettingsView\.shell-selection\.spec\.tsx$|package\.json$|vite.config|vitest|eslint'
echo
echo "Relevant mock locations:"
rg -n -C 5 'vi\.mock\(|vi\.importActual\(' webview-ui/src || true
echo
echo "ESLint config/package hints:"
rg -n '"eslint"|"vitest"|"import/no-unresolved"|"no-floating-promises"|no-floating-promises|plugin:`@typescript-eslint/recommended`|recommended' package.json webview-ui package.json webview-ui/package.json 2>/dev/null || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
🌐 Web query:
Vitest vi.mock vi.importActual async factory importOriginal documentation
💡 Result:
In Vitest, vi.mock is used to substitute modules with mocks [1][2]. The factory function provided to vi.mock allows for dynamic module creation and accepts an importOriginal helper as its first argument [1][3][2][4]. vi.mock factory and importOriginal When you use vi.mock(path, factory), the factory is a function that can receive an importOriginal parameter [1][2]. This parameter is a helper function you must call (and await) to retrieve the original, unmocked module [3][5][4]. This is particularly useful when you want to partially mock a module—by spreading the original module's exports and overriding specific ones [6][7][2]. Example: vi.mock('./example.js', async (importOriginal) => { const originalModule = await importOriginal // Get the original module return {...originalModule, // Keep original functionality methodToMock: vi.fn, // Override specific export } }) vi.importActual vi.importActual(path) is a standalone function that imports the original module, bypassing any mocks [1][7][2]. While you can use it inside a vi.mock factory, the importOriginal helper provided directly by the factory is the preferred approach, especially when using dynamic imports (e.g., vi.mock(import('./path'))) because it can automatically inherit types and handle paths more effectively [2][5]. Key Differences and Notes - Hoisting: Both vi.mock calls are hoisted to the top of the file and executed before other imports [1][6][8][2]. - Dynamic Imports: Using dynamic imports (vi.mock(import('./path'))) in the vi.mock call is recommended for better IDE support and type safety [2][5]. - Asynchronicity: The importOriginal helper and the vi.importActual function are asynchronous and must be awaited [3][2][4]. The documentation emphasizes using vi.spyOn if you only need to spy on specific methods without replacing the entire module, as it is often a simpler alternative [3][6][5].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/mocking/modules.md
- 4: https://vitest.dev/guide/mocking/modules
- 5: https://tangled.org/vitest.dev/vitest/blob/9be6121eee4bd636149c863aeabd0a82a0a2990d/docs/guide/mocking/modules.md
- 6: https://github.com/vitest-dev/vitest/blob/main/docs/guide/mocking.md
- 7: https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/integrations/vi.ts
- 8: https://tangled.org/vitest.dev/vitest/blob/cad08ee19b0cabe96a23b038a21723f64437d0e4/docs/api/vi.md
🌐 Web query:
Vitest importOriginal helper types return Promise or actual module
💡 Result:
In Vitest, the importOriginal helper function provided within the vi.mock factory is indeed an asynchronous function that returns a Promise [1][2]. When using vi.mock, you must await the result of importOriginal to access the original module's exports [1][2]. Example Usage: vi.mock(import('./your-module.js'), async (importOriginal) => { // importOriginal returns a Promise, so you must await it const originalModule = await importOriginal; return {...originalModule, // Add your mocks here someExport: vi.fn, }; }); By using dynamic imports (e.g., import('./path')) in your vi.mock call, Vitest can automatically infer the types for importOriginal based on the actual module being mocked, providing full TypeScript support and IDE autocompletion for the returned module object [3][4][5].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/mocking/modules.md
- 2: https://vitest.dev/guide/mocking/modules
- 3: https://vitest.dev/api/vi
- 4: https://tangled.org/vitest.dev/vitest/blob/c3b2757cd3d0c9c32a23be4ab49b4e48586c5e0f/docs/api/vi.md
- 5: https://github.com/vitest-dev/vitest/blob/v4.1.9/docs/api/vi.md
Await the actual module in partial vi.mock() factories.
Make both factory functions async, await vi.importActual() or use importOriginal, then spread the resolved exports before overriding Tab* and ToggleSwitch. This keeps unmodified exports from being replaced and prevents the factory promise from being ignored.
🤖 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
`@webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx`
around lines 93 - 110, Update the partial vi.mock factories for the Tab module
and ToggleSwitch module to be async, await vi.importActual (or use
importOriginal), and spread the resolved exports before applying the Tab* and
ToggleSwitch overrides. Preserve the existing mock implementations while
ensuring all unmodified exports remain available.
Source: Coding guidelines
| const profileButton = screen.queryByTestId("option-profile:PowerShell") | ||
| if (profileButton) { | ||
| act(() => { | ||
| fireEvent.click(profileButton) | ||
| }) | ||
|
|
||
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | ||
| kind: "profile", | ||
| profileName: "PowerShell", | ||
| }) | ||
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the profile-selection assertion unconditional.
Line 164 uses queryByTestId, and the if block skips every assertion when the profile option is absent. A regression that removes profile options will pass this test. Use getByTestId and run the click assertions unconditionally.
Proposed fix
- const profileButton = screen.queryByTestId("option-profile:PowerShell")
- if (profileButton) {
- act(() => {
- fireEvent.click(profileButton)
- })
-
- expect(onShellSelectionChange).toHaveBeenCalledWith({
- kind: "profile",
- profileName: "PowerShell",
- })
- expect(onTerminalProfilePickerOpened).toHaveBeenCalled()
- }
+ const profileButton = screen.getByTestId("option-profile:PowerShell")
+ act(() => {
+ fireEvent.click(profileButton)
+ })
+
+ expect(onShellSelectionChange).toHaveBeenCalledWith({
+ kind: "profile",
+ profileName: "PowerShell",
+ })
+ expect(onTerminalProfilePickerOpened).toHaveBeenCalled()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const profileButton = screen.queryByTestId("option-profile:PowerShell") | |
| if (profileButton) { | |
| act(() => { | |
| fireEvent.click(profileButton) | |
| }) | |
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | |
| kind: "profile", | |
| profileName: "PowerShell", | |
| }) | |
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() | |
| } | |
| const profileButton = screen.getByTestId("option-profile:PowerShell") | |
| act(() => { | |
| fireEvent.click(profileButton) | |
| }) | |
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | |
| kind: "profile", | |
| profileName: "PowerShell", | |
| }) | |
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() |
🤖 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 `@webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx`
around lines 164 - 175, Update the profile-selection test to use getByTestId for
the PowerShell option instead of queryByTestId, remove the conditional guard,
and execute the click plus onShellSelectionChange and
onTerminalProfilePickerOpened assertions unconditionally.
| terminalZshP10k, | ||
| terminalZdotdir, | ||
| terminalProfile, | ||
| terminalShellSelection, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Hydrate and reconcile terminal shell selection state.
For an initial state message that changes only terminalShellSelection, cachedState retains its pre-hydration value. At Line 228, the cache refresh exits unless the API configuration or mode changes. Line 917 can then send undefined to TerminalSettings instead of the persisted selection. After Save, the still-pending value remains preferred and can be posted again on a later save.
webview-ui/src/components/settings/SettingsView.tsx#L198-L198: synchronize the persisted selection intocachedStateduring hydration without setting the dirty flag.webview-ui/src/components/settings/SettingsView.tsx#L463-L477: track the submitted selection and clear or reconcile the pending value after the extension host acknowledges it.webview-ui/src/components/settings/SettingsView.tsx#L917-L926: render the reconciled cached value after acknowledgement.webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx#L317-L345: add separate tests for persisted-selection hydration with Save disabled and host-state acknowledgement after Save.
As per coding guidelines, “For SettingsView, keep inputs in local cachedState until save, and test automatic initialization separately from genuine user edits.”
📍 Affects 2 files
webview-ui/src/components/settings/SettingsView.tsx#L198-L198(this comment)webview-ui/src/components/settings/SettingsView.tsx#L463-L477webview-ui/src/components/settings/SettingsView.tsx#L917-L926webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx#L317-L345
🤖 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 `@webview-ui/src/components/settings/SettingsView.tsx` at line 198, Update
SettingsView.tsx lines 198-198 to hydrate terminalShellSelection into
cachedState without marking the form dirty; update lines 463-477 to track the
submitted selection and clear or reconcile the pending value after host
acknowledgement; update lines 917-926 to render the reconciled cached selection.
Add separate tests in
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx
lines 317-345 covering persisted-selection hydration with Save disabled and
host-state acknowledgement after Save.
Source: Coding guidelines
| onClick={() => { | ||
| // Ask the extension host to open a native file picker | ||
| // for selecting a shell executable path. | ||
| vscode.postMessage({ type: "requestCustomShellPath" }) | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline webview-ui/src/components/settings/TerminalSettings.tsx --items all
ast-grep outline webview-ui/src/components/settings/SettingsView.tsx --items all
rg -n -C 8 --glob '*.ts' --glob '*.tsx' \
'requestCustomShellPath|customShellPath|terminalShellSelection|onShellSelectionChange|setTerminalShellSelection' .Repository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TerminalSettings relevant handler =="
sed -n '76,131p' webview-ui/src/components/settings/TerminalSettings.tsx | cat -n
echo "== TerminalSettings picker options/paths =="
sed -n '239,310p' webview-ui/src/components/settings/TerminalSettings.tsx | cat -n
echo "== SettingsView TerminalSettings props =="
sed -n '909,926p' webview-ui/src/components/settings/SettingsView.tsx | cat -n
echo "== exact requestCustomShellPath occurrences =="
rg -n -C 3 'requestCustomShellPath|customShell' .Repository: Zoo-Code-Org/Zoo-Code
Length of output: 8670
Wire the custom shell picker result into TerminalSettings buffering.
SettingsView buffers shell changes through TerminalSettings.onShellSelectionChange, but TerminalSettings has no response handler for requestCustomShellPath. Add a typed picker-result message handler that converts the chosen path to TerminalShellSelection, calls onShellSelectionChange, and lets SettingsView persist it via setTerminalShellSelection on Save.
🤖 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 `@webview-ui/src/components/settings/TerminalSettings.tsx` around lines 301 -
305, In TerminalSettings, add a typed response handler for the
requestCustomShellPath result that converts the selected path into a
TerminalShellSelection and passes it to onShellSelectionChange. Ensure the
existing SettingsView buffering flow persists the selection through
setTerminalShellSelection only when Save is used.
| <div className="font-medium"> | ||
| {t("settings:terminal.inlineShell.effectiveShell.label")} | ||
| </div> | ||
| <div> | ||
| {t("settings:terminal.inlineShell.effectiveShell.family")}:{" "} | ||
| {shellOptions.effectiveShell.family} | ||
| </div> | ||
| <div> | ||
| {t("settings:terminal.inlineShell.effectiveShell.source")}:{" "} | ||
| {shellOptions.effectiveShell.source} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the effective shell executable.
TerminalShellOptionsPayload.effectiveShell.label contains the display label for the resolved executable. These lines show only the heading, family, and source. Add the label and assert it in webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx Lines 178-198.
Proposed fix
<div className="font-medium">
{t("settings:terminal.inlineShell.effectiveShell.label")}
</div>
+<div>{shellOptions.effectiveShell.label}</div>
<div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="font-medium"> | |
| {t("settings:terminal.inlineShell.effectiveShell.label")} | |
| </div> | |
| <div> | |
| {t("settings:terminal.inlineShell.effectiveShell.family")}:{" "} | |
| {shellOptions.effectiveShell.family} | |
| </div> | |
| <div> | |
| {t("settings:terminal.inlineShell.effectiveShell.source")}:{" "} | |
| {shellOptions.effectiveShell.source} | |
| </div> | |
| <div className="font-medium"> | |
| {t("settings:terminal.inlineShell.effectiveShell.label")} | |
| </div> | |
| <div>{shellOptions.effectiveShell.label}</div> | |
| <div> | |
| {t("settings:terminal.inlineShell.effectiveShell.family")}:{" "} | |
| {shellOptions.effectiveShell.family} | |
| </div> | |
| <div> | |
| {t("settings:terminal.inlineShell.effectiveShell.source")}:{" "} | |
| {shellOptions.effectiveShell.source} | |
| </div> |
🤖 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 `@webview-ui/src/components/settings/TerminalSettings.tsx` around lines 317 -
327, Update the effective shell display in TerminalSettings to render
shellOptions.effectiveShell.label alongside the existing family and source
values, using the corresponding effectiveShell.label translation key. Add an
assertion in TerminalSettings.shell.spec.tsx covering the resolved executable
label.
| {shellError && ( | ||
| <div | ||
| className="mt-2 text-vscode-errorForeground text-sm" | ||
| data-testid="terminal-inline-shell-error"> | ||
| {t("settings:terminal.inlineShell.error.invalid")} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the unavailable-shell message for discovery failures.
TerminalShellOptionsPayload.error represents shell-option discovery failure. This block always renders error.invalid. A discovery error such as Service unavailable incorrectly tells the user that the selected shell is unsupported. Render error.unavailable, or add a discriminated error type before using selection-specific text. Update the test at webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx Lines 200-215 to assert the message.
Proposed fix
- {t("settings:terminal.inlineShell.error.invalid")}
+ {t("settings:terminal.inlineShell.error.unavailable")}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {shellError && ( | |
| <div | |
| className="mt-2 text-vscode-errorForeground text-sm" | |
| data-testid="terminal-inline-shell-error"> | |
| {t("settings:terminal.inlineShell.error.invalid")} | |
| </div> | |
| {shellError && ( | |
| <div | |
| className="mt-2 text-vscode-errorForeground text-sm" | |
| data-testid="terminal-inline-shell-error"> | |
| {t("settings:terminal.inlineShell.error.unavailable")} | |
| </div> |
🤖 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 `@webview-ui/src/components/settings/TerminalSettings.tsx` around lines 335 -
340, Update the shellError rendering in TerminalSettings to use the
unavailable-shell translation for TerminalShellOptionsPayload discovery failures
instead of the selection-specific invalid message. Preserve invalid messaging
only when the error represents an unsupported selected shell, and update the
relevant shell test to assert the unavailable message.
| }, | ||
| "inlineShell": { | ||
| "label": "Inline-Terminal-Shell", | ||
| "description": "Wähle das Shell für die Inline-Terminal-Befehlsausführung. Auto folgt deinem vertrauenswürdigen VS Code Terminal-Profil. Benutzerdefinierte Pfade werden vom Extension-Host validiert.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the German grammatical gender for “Shell”.
“Shell” is feminine in standard German. These strings use neuter forms. Use die Shell, Aktive Shell, and Die ausgewählte Shell.
Proposed fix
- "description": "Wähle das Shell für die Inline-Terminal-Befehlsausführung. Auto folgt deinem vertrauenswürdigen VS Code Terminal-Profil. Benutzerdefinierte Pfade werden vom Extension-Host validiert.",
+ "description": "Wähle die Shell für die Inline-Terminal-Befehlsausführung. Auto folgt deinem vertrauenswürdigen VS Code Terminal-Profil. Benutzerdefinierte Pfade werden vom Extension-Host validiert.",
- "label": "Aktives Shell",
+ "label": "Aktive Shell",
- "invalid": "Das ausgewählte Shell wird nicht unterstützt. Wähle ein vertrauenswürdiges Profil oder ein gültiges Shell-Programm.",
+ "invalid": "Die ausgewählte Shell wird nicht unterstützt. Wähle ein vertrauenswürdiges Profil oder ein gültiges Shell-Programm.",Also applies to: 856-856, 863-863
🤖 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 `@webview-ui/src/i18n/locales/de/settings.json` at line 851, Update the German
settings strings around the description and the related entries at the
referenced symbols to use the feminine grammatical forms: “die Shell”, “Aktive
Shell”, and “Die ausgewählte Shell”. Preserve the existing meanings and
surrounding localization text.
| "family": "Família", | ||
| "source": "Fonte", | ||
| "fallback": "Comportamento de fallback", | ||
| "fallbackDescription": "Se a integração do shell falhar, os comandos são reutilizados com a mesma família de shell." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the fallback action description.
“Os comandos são reutilizados” says that commands are reused. The fallback executes the commands again. Replace it with “os comandos são executados novamente”.
🤖 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 `@webview-ui/src/i18n/locales/pt-BR/settings.json` at line 860, The
fallbackDescription field in the pt-BR settings translation uses "os comandos
são reutilizados" (commands are reused) but should instead use "os comandos são
executados novamente" (commands are executed again) to accurately describe the
fallback behavior. Update the fallbackDescription string to replace the phrase
about reusing with the correct phrase about executing commands again.
github-actions
Bot
added
the
awaiting-review
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@codecov.yml`:
- Around line 1-57: Convert the entire codecov.yml file to LF line endings,
preserving all existing coverage, flag, component, and comment configuration
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79f735b6-6b50-439d-8956-6a77ca2a733a
📒 Files selected for processing (22)
codecov.ymlpackages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tswebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (21)
- webview-ui/src/i18n/locales/en/settings.json
- webview-ui/src/i18n/locales/id/settings.json
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/i18n/locales/nl/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- packages/types/src/vscode-extension-host.ts
- packages/types/src/global-settings.ts
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/hi/settings.json
- webview-ui/src/components/settings/SettingsView.tsx
- webview-ui/src/i18n/locales/es/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/ko/settings.json
f1ca0e8 to
952963b
Compare
github-actions
Bot
added
awaiting-review
1 participant
Stack Position
feature/unified-shell-resolutionDescription
https://youtube.com/shorts/-cm4pnaoXD0
Full Feature Description
feature/unified-shell-resolutionterminal.ts,global-settings.ts,vscode-extension-host.ts, the settings UITerminalSettings.tsxandSettingsView.tsx, the backend terminal layersrc/integrations/terminal, and the task/tool/API wiringTask.ts,ExecuteCommandTool.ts,api.ts.SettingsView.tsxbind tocachedState, not live extension state.terminal-profile.test.ts. Manually run the same command in default, PowerShell, Command Prompt, and where available WSL/POSIX profiles, comparing the selected executable, output, exit code, cancellation, and cleanup.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds typed shell selection/profile/default contracts, backward-compatible serialization, settings UI and locale. The UI uses
SettingsView.tsx'scachedStateand does not change command execution.Included Files
packages/types/src/terminal.tspackages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tswebview-ui/src/components/settings/TerminalSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxExclusion Scope
Summary by CodeRabbit
Summary by CodeRabbit