feat(desktop): open new terminals in the active tab directory#2215
Open
kbdevs wants to merge 1 commit into
Open
feat(desktop): open new terminals in the active tab directory#2215kbdevs wants to merge 1 commit into
kbdevs wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a feature enabling users to configure whether new terminals open in the current tab's directory. It includes database schema updates, tRPC endpoints for reading and writing the setting, UI components for the settings page, utility functions to resolve terminal directories, and search configuration for the new setting. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as NewTerminalDirectorySetting<br/>Component
participant Cache as React Query<br/>Cache
participant tRPC as tRPC Client
participant Server as Settings Router
participant DB as Database
User->>UI: Toggle switch
UI->>Cache: Apply optimistic update
UI->>tRPC: setNewTerminalInCurrentTabDirectory.mutate()
tRPC->>Server: POST /settings.setNewTerminalInCurrentTabDirectory
Server->>DB: UPDATE settings table
DB-->>Server: Confirmed
Server-->>tRPC: Response
alt Mutation Success
UI->>Cache: Invalidate query
Cache->>tRPC: Refetch getNewTerminalInCurrentTabDirectory
tRPC->>Server: GET /settings.getNewTerminalInCurrentTabDirectory
Server->>DB: SELECT from settings
DB-->>Server: Row data
Server-->>tRPC: Setting value
tRPC->>Cache: Update cache
Cache->>UI: Render updated state
else Mutation Error
UI->>Cache: Rollback to previous value
UI->>User: Show error state
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a feature to open new terminal tabs in the active tab's current working directory. When a user creates a new terminal tab, the app now resolves the CWD from the currently focused pane — using the terminal's live CWD, the file viewer's parent directory, or falling back to the workspace root. A new boolean setting (enabled by default) controls this behavior, backed by a local-db migration and full settings UI integration.
Changes:
- Add
resolveNewTerminalInitialCwdutility to derive an initial CWD from the active tab's focused pane (terminal CWD, file viewer dirname, or workspace root fallback) - Integrate the CWD resolution into
useTabsWithPresets.addTab, passing it asfallbackInitialCwdto presets so explicit preset CWDs take precedence - Add
newTerminalInCurrentTabDirectorysetting with schema migration, tRPC get/set endpoints, settings search entry, and toggle UI component
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/local-db/src/schema/schema.ts |
Add newTerminalInCurrentTabDirectory boolean column to settings table |
packages/local-db/drizzle/meta/_journal.json |
Register migration 0036 in the journal |
packages/local-db/drizzle/meta/0036_snapshot.json |
Auto-generated schema snapshot for migration 0036 |
packages/local-db/drizzle/0036_add_new_terminal_current_directory_setting.sql |
SQL migration to add the new column |
apps/desktop/src/shared/constants.ts |
Add DEFAULT_NEW_TERMINAL_IN_CURRENT_TAB_DIRECTORY = true |
apps/desktop/src/renderer/stores/tabs/utils.ts |
Add resolveNewTerminalInitialCwd function with terminal/file-viewer/fallback CWD resolution |
apps/desktop/src/renderer/stores/tabs/utils.test.ts |
Unit tests for resolveNewTerminalInitialCwd covering terminal, file-viewer, and webview cases |
apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts |
Integrate CWD resolution into addTab and openPreset, add fallbackInitialCwd to preset options |
apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts |
Add search entry for the new terminal directory setting |
apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts |
Test that "directory" search returns the new setting |
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/NewTerminalDirectorySetting.tsx |
Settings toggle UI with optimistic update pattern |
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx |
Render the new setting in the terminal settings page |
apps/desktop/src/lib/trpc/routers/settings/index.ts |
Add getNewTerminalInCurrentTabDirectory and setNewTerminalInCurrentTabDirectory tRPC endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts (1)
66-70: Rename or broaden this test suite.This case now reports under
"settings search - font settings", which is misleading when it fails and makes the search tests harder to scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts` around lines 66 - 70, The test suite name is misleading ("settings search - font settings") so rename or broaden the surrounding describe block to accurately reflect these search tests; locate the suite that contains the test with the it(...) checking searchSettings("directory") and update the describe string to a more general name like "settings search" or "settings search - general" so failures for searchSettings, getIds, or SETTING_ITEM_ID.TERMINAL_NEW_TERMINAL_DIRECTORY are reported under the correct group.apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts (1)
377-388: LGTM! Correct integration of current-tab-directory feature.The logic properly respects the user setting and falls back gracefully. The
resolveNewTerminalInitialCwdis called only when the feature is enabled.Consider consolidating the multiple
useTabsStore.getState()calls into a single snapshot for slightly cleaner code:♻️ Optional refactor
const sourceInitialCwd = openInCurrentTabDirectory ? resolveNewTerminalInitialCwd({ workspaceId, worktreePath: utils.workspaces.get.getData({ id: workspaceId }) ?.worktreePath, - tabs: useTabsStore.getState().tabs, - panes: useTabsStore.getState().panes, - activeTabIds: useTabsStore.getState().activeTabIds, - tabHistoryStacks: useTabsStore.getState().tabHistoryStacks, - focusedPaneIds: useTabsStore.getState().focusedPaneIds, + ...useTabsStore.getState(), }) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts` around lines 377 - 388, The multiple useTabsStore.getState() calls when computing sourceInitialCwd should be replaced with a single snapshot to avoid repeated state reads; capture const tabsState = useTabsStore.getState() (or a similarly named variable) and use tabsState.tabs, tabsState.panes, tabsState.activeTabIds, tabsState.tabHistoryStacks, and tabsState.focusedPaneIds when calling resolveNewTerminalInitialCwd (keep workspaceId and worktreePath retrieval the same).
🤖 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/desktop/src/renderer/stores/tabs/utils.test.ts`:
- Around line 422-489: Tests hard-code POSIX paths causing Windows failures;
update the three assertions around resolveNewTerminalInitialCwd to build
platform-neutral expected values using Node's path utilities: import path and
for the focused terminal case call path.normalize("/repo/packages/ui") or
path.join parts, for the file-viewer case compute expected via
path.dirname(path.join("/repo", "packages/ui/src/button.tsx")) (or using the
same worktreePath and pane.fileViewer.filePath values from the test) and for the
fallback case use path.normalize(worktreePath) or path.join(...) so assertions
compare path-normalized values instead of hard-coded POSIX strings; reference
resolveNewTerminalInitialCwd, pane.fileViewer.filePath, and worktreePath to
locate where to change the expected values.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts`:
- Around line 66-70: The test suite name is misleading ("settings search - font
settings") so rename or broaden the surrounding describe block to accurately
reflect these search tests; locate the suite that contains the test with the
it(...) checking searchSettings("directory") and update the describe string to a
more general name like "settings search" or "settings search - general" so
failures for searchSettings, getIds, or
SETTING_ITEM_ID.TERMINAL_NEW_TERMINAL_DIRECTORY are reported under the correct
group.
In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts`:
- Around line 377-388: The multiple useTabsStore.getState() calls when computing
sourceInitialCwd should be replaced with a single snapshot to avoid repeated
state reads; capture const tabsState = useTabsStore.getState() (or a similarly
named variable) and use tabsState.tabs, tabsState.panes, tabsState.activeTabIds,
tabsState.tabHistoryStacks, and tabsState.focusedPaneIds when calling
resolveNewTerminalInitialCwd (keep workspaceId and worktreePath retrieval the
same).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fee76df-7059-4b88-8c06-3e403ae95676
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/NewTerminalDirectorySetting.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tsapps/desktop/src/renderer/stores/tabs/utils.test.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/constants.tspackages/local-db/drizzle/0036_add_new_terminal_current_directory_setting.sqlpackages/local-db/drizzle/meta/0036_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
Comment on lines
+422
to
+489
| it("uses the focused terminal cwd when available", () => { | ||
| expect( | ||
| resolveNewTerminalInitialCwd({ | ||
| workspaceId: "ws-1", | ||
| worktreePath: "/repo", | ||
| tabs: [createTab({ id: "tab-1", workspaceId: "ws-1" })], | ||
| panes: { | ||
| "pane-1": { | ||
| id: "pane-1", | ||
| tabId: "tab-1", | ||
| type: "terminal", | ||
| name: "Terminal", | ||
| cwd: "/repo/packages/ui", | ||
| }, | ||
| }, | ||
| activeTabIds: { "ws-1": "tab-1" }, | ||
| tabHistoryStacks: {}, | ||
| focusedPaneIds: { "tab-1": "pane-1" }, | ||
| }), | ||
| ).toBe("/repo/packages/ui"); | ||
| }); | ||
|
|
||
| it("falls back to the file viewer's directory", () => { | ||
| expect( | ||
| resolveNewTerminalInitialCwd({ | ||
| workspaceId: "ws-1", | ||
| worktreePath: "/repo", | ||
| tabs: [createTab({ id: "tab-1", workspaceId: "ws-1" })], | ||
| panes: { | ||
| "pane-1": { | ||
| id: "pane-1", | ||
| tabId: "tab-1", | ||
| type: "file-viewer", | ||
| name: "Button.tsx", | ||
| fileViewer: { | ||
| filePath: "packages/ui/src/button.tsx", | ||
| viewMode: "raw", | ||
| isPinned: false, | ||
| diffLayout: "inline", | ||
| }, | ||
| }, | ||
| }, | ||
| activeTabIds: { "ws-1": "tab-1" }, | ||
| tabHistoryStacks: {}, | ||
| focusedPaneIds: { "tab-1": "pane-1" }, | ||
| }), | ||
| ).toBe("/repo/packages/ui/src"); | ||
| }); | ||
|
|
||
| it("falls back to the workspace root when no directory can be resolved", () => { | ||
| expect( | ||
| resolveNewTerminalInitialCwd({ | ||
| workspaceId: "ws-1", | ||
| worktreePath: "/repo", | ||
| tabs: [createTab({ id: "tab-1", workspaceId: "ws-1" })], | ||
| panes: { | ||
| "pane-1": { | ||
| id: "pane-1", | ||
| tabId: "tab-1", | ||
| type: "webview", | ||
| name: "Browser", | ||
| }, | ||
| }, | ||
| activeTabIds: { "ws-1": "tab-1" }, | ||
| tabHistoryStacks: {}, | ||
| focusedPaneIds: { "tab-1": "pane-1" }, | ||
| }), | ||
| ).toBe("/repo"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the implementation of resolveNewTerminalInitialCwd
rg -l "resolveNewTerminalInitialCwd" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Get details about the function implementation
rg -A 30 "function resolveNewTerminalInitialCwd|export.*resolveNewTerminalInitialCwd" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find the test file and implementation
fd -e ts -e tsx | grep -E "(utils\.test|utils\.ts).*tabs"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for the function without file type constraints
rg "resolveNewTerminalInitialCwd" -A 30Repository: superset-sh/superset
Length of output: 17355
🏁 Script executed:
# Get the full implementation of resolveNewTerminalInitialCwd
cat apps/desktop/src/renderer/stores/tabs/utils.ts | head -200 | tail -100Repository: superset-sh/superset
Length of output: 2479
🏁 Script executed:
# Let's search for the function and get more lines
rg -A 50 "export function resolveNewTerminalInitialCwd" apps/desktop/src/renderer/stores/tabs/utils.tsRepository: superset-sh/superset
Length of output: 1159
🏁 Script executed:
# Check the imports at the top of utils.ts
head -30 apps/desktop/src/renderer/stores/tabs/utils.tsRepository: superset-sh/superset
Length of output: 878
🏁 Script executed:
# Check if there's any platform-specific path handling elsewhere
rg "process.platform|win32|darwin|linux" apps/desktop/src/renderer/stores/tabs/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look at the test file to confirm the exact line numbers match the review
sed -n '420,495p' apps/desktop/src/renderer/stores/tabs/utils.test.tsRepository: superset-sh/superset
Length of output: 1900
🏁 Script executed:
# Double-check: does test 2 actually use the implementation path with join/dirname?
sed -n '453,467p' apps/desktop/src/renderer/stores/tabs/utils.test.tsRepository: superset-sh/superset
Length of output: 421
🏁 Script executed:
# Verify: on what platform would these tests be run?
# Check if there's any CI/testing setup
fd -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|ci|test)" | head -5Repository: superset-sh/superset
Length of output: 294
🏁 Script executed:
# Check for CI configuration files
fd -e yml -e yaml | grep -i -E "(github|gitlab|ci|test)"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Also check package.json to see what test command is used
cat apps/desktop/package.json | grep -A 5 "test"Repository: superset-sh/superset
Length of output: 200
Make these CWD assertions platform-neutral.
These test cases hard-code POSIX paths, but the file-viewer branch uses dirname(join(worktreePath, pane.fileViewer.filePath)) which is platform-dependent. On Windows, path separators become backslashes, causing assertions to fail.
Suggested test adjustment
+import { dirname, join } from "node:path";
import { describe, expect, it } from "bun:test";
import type { MosaicNode } from "react-mosaic-component";
import type { Tab } from "./types";
import {
buildMultiPaneLayout,
@@
describe("resolveNewTerminalInitialCwd", () => {
+ const repoRoot = join("/", "repo");
+
const createTab = ({
id,
workspaceId,
}: {
@@
resolveNewTerminalInitialCwd({
workspaceId: "ws-1",
- worktreePath: "/repo",
+ worktreePath: repoRoot,
tabs: [createTab({ id: "tab-1", workspaceId: "ws-1" })],
panes: {
"pane-1": {
@@
- cwd: "/repo/packages/ui",
+ cwd: join(repoRoot, "packages", "ui"),
},
},
@@
- ).toBe("/repo/packages/ui");
+ ).toBe(join(repoRoot, "packages", "ui"));
});
@@
resolveNewTerminalInitialCwd({
workspaceId: "ws-1",
- worktreePath: "/repo",
+ worktreePath: repoRoot,
@@
- ).toBe("/repo/packages/ui/src");
+ ).toBe(
+ dirname(join(repoRoot, "packages", "ui", "src", "button.tsx")),
+ );
});
@@
resolveNewTerminalInitialCwd({
workspaceId: "ws-1",
- worktreePath: "/repo",
+ worktreePath: repoRoot,
@@
- ).toBe("/repo");
+ ).toBe(repoRoot);
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/tabs/utils.test.ts` around lines 422 - 489,
Tests hard-code POSIX paths causing Windows failures; update the three
assertions around resolveNewTerminalInitialCwd to build platform-neutral
expected values using Node's path utilities: import path and for the focused
terminal case call path.normalize("/repo/packages/ui") or path.join parts, for
the file-viewer case compute expected via path.dirname(path.join("/repo",
"packages/ui/src/button.tsx")) (or using the same worktreePath and
pane.fileViewer.filePath values from the test) and for the fallback case use
path.normalize(worktreePath) or path.join(...) so assertions compare
path-normalized values instead of hard-coded POSIX strings; reference
resolveNewTerminalInitialCwd, pane.fileViewer.filePath, and worktreePath to
locate where to change the expected values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by cubic
Open new terminals in the active tab’s directory by default. Adds a toggle in Terminal settings, respects preset CWD, and falls back to the workspace root when no directory can be resolved.
new_terminal_in_current_tab_directory.initialCwd; when absent, use afallbackInitialCwdderived from the active tab.Written for commit d7f152c. Summary will update on new commits.
Summary by CodeRabbit