feat: display project path in sidebar and allow same remote path with different hosts#1690
Open
yuzhichang wants to merge 3 commits intogeneralaction:mainfrom
Open
feat: display project path in sidebar and allow same remote path with different hosts#1690yuzhichang wants to merge 3 commits intogeneralaction:mainfrom
yuzhichang wants to merge 3 commits intogeneralaction:mainfrom
Conversation
|
@yuzhichang is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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:
📝 WalkthroughWalkthroughRefines project save conflict logic to allow same-path remote projects when sshConnectionId matches; drops DB uniqueness on Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant DatabaseService
participant DB
Renderer->>DatabaseService: saveProject(project)
DatabaseService->>DB: SELECT id, sshConnectionId, isRemote WHERE path = project.path
alt no existing row
DB-->>DatabaseService: no rows
DatabaseService->>DB: INSERT project
DB-->>DatabaseService: insert result
DatabaseService-->>Renderer: resolve(insert result)
else existing row found
DB-->>DatabaseService: row {id, sshConnectionId, isRemote}
alt existing.id == project.id
DatabaseService->>DB: UPDATE project
DB-->>DatabaseService: update result
DatabaseService-->>Renderer: resolve(update result)
else different id
alt both remote and sshConnectionId equal
DatabaseService->>DB: INSERT project (allowed)
DB-->>DatabaseService: insert result
DatabaseService-->>Renderer: resolve(insert result)
else incompatible (non-remote mismatch or sshConnectionId differs)
DatabaseService-->>Renderer: reject(ProjectConflictError)
end
end
end
sequenceDiagram
participant Renderer
participant MainIPC
participant SSHService
participant Monitor
participant Telemetry
Renderer->>MainIPC: CONNECT("ssh-config:alias%20enc")
MainIPC->>MainIPC: decode alias -> resolveSshConfigHost(alias)
alt alias resolved
MainIPC->>SSHService: connect(in-memory SshConfig)
SSHService-->>MainIPC: connectionId
MainIPC->>Monitor: startMonitoring(connectionId)
MainIPC->>Telemetry: emit("ssh_connect_success", {authType})
MainIPC-->>Renderer: { success: true, connectionId }
else alias not found
MainIPC-->>Renderer: { success: false, error }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/main/services/DatabaseService.ts`:
- Around line 220-232: Add unit tests covering remote-vs-remote project path
collisions for DatabaseService.validate or the method that contains
isNewProjectRemote/isExistingProjectRemote logic: create two test cases—(1) both
projects marked isRemote with identical sshConnectionId values and the same path
and assert that ProjectConflictError is thrown with existingByPath.id/name/path
included, and (2) both projects marked isRemote with different sshConnectionId
values and the same path and assert that no error is thrown (operation
succeeds). Use the same setup pattern as the existing test (populate
existingByPath and project objects) and target the branch conditions involving
isNewProjectRemote, isExistingProjectRemote, and sameSshConnection to ensure the
new remote-specific behavior is covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe104978-5710-4f3d-adf7-2669e338fce3
📒 Files selected for processing (2)
src/main/services/DatabaseService.tssrc/renderer/components/sidebar/LeftSidebar.tsx
yuzhichang
force-pushed
the
feat/remote-project-path-display
branch
from
0493a9e to
1971bf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/sidebar/LeftSidebar.tsx`:
- Around line 89-94: The remote display label currently uses project.path inside
the useMemo for displayName, which can show the wrong path when a remote project
provides remotePath; update the useMemo (displayName) to use remotePath when
isRemote (fall back to project.path) and keep the host from remote.host;
specifically change the branch that builds `${project.name} (${remote.host ??
'...'}:${project.path})` to use `${remote.host ?? '...'}:${remote.remotePath ??
project.path}` (reference: useMemo -> displayName, project, isRemote,
remote.host, remote.remotePath).
In `@src/test/main/DatabaseService.saveProject.test.ts`:
- Around line 172-193: The test "throws ProjectConflictError when both remote
projects share the same sshConnectionId and path" is using mismatched paths so
it doesn't truly validate the same-path conflict; update the queued mock row
pushed into selectResults to use the same path as baseProject (i.e., change the
existing project's path from '/srv/project-one' to baseProject.path or
'/tmp/project-one') so the call to service.saveProject(baseProject) exercises
the intended same-path conflict branch referenced by selectResults and the
expected ProjectConflictError assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b86dde0-174e-43ab-a90d-ffc4c3247ee3
📒 Files selected for processing (3)
src/main/services/DatabaseService.tssrc/renderer/components/sidebar/LeftSidebar.tsxsrc/test/main/DatabaseService.saveProject.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/services/DatabaseService.ts
…same path but different hosts - Display project path in sidebar after project name: - Local projects show absolute path - Remote projects show hostname:path (e.g., tower01:/home/user/repo) - Fix ProjectConflictError for remote projects by allowing same path with different sshConnectionId (different SSH hosts) - Remove unique index on projects.path (with migration) since remote projects can share paths when connected via different SSH hosts - Add SSH config alias support: when connectionId starts with "ssh-config:", resolve via SSH config parser instead of DB lookup - Fix connection error display: show connectionId as fallback when remote.host is unavailable - Add unit tests for remote-vs-remote project path collisions Co-Authored-By: Claude Opus 4.6 <[email protected]>
yuzhichang
force-pushed
the
feat/remote-project-path-display
branch
from
bd49319 to
953c63e
Compare
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 `@src/main/ipc/sshIpc.ts`:
- Around line 465-474: The SshConfig creation sets username to an empty string
when sshConfigHost.user is undefined, which can cause SSH auth failures; update
the username assignment in the SshConfig object (constructed in sshIpc.ts) to
validate or default to the current OS user by using os.userInfo().username when
sshConfigHost.user is falsy, or alternatively throw/return an error if no
username can be determined—ensure the change touches the SshConfig construction
(username field) and any calling code that expects a non-empty username.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 137fb801-803e-4586-b248-8b20b66bc862
📒 Files selected for processing (8)
drizzle/0013_remove_projects_path_unique.sqldrizzle/meta/0013_snapshot.jsondrizzle/meta/_journal.jsonsrc/main/db/schema.tssrc/main/ipc/sshIpc.tssrc/main/services/DatabaseService.tssrc/renderer/components/sidebar/LeftSidebar.tsxsrc/test/main/DatabaseService.saveProject.test.ts
✅ Files skipped from review due to trivial changes (3)
- drizzle/meta/_journal.json
- drizzle/meta/0013_snapshot.json
- src/main/services/DatabaseService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/main/DatabaseService.saveProject.test.ts
…ig lacks user When connecting via SSH config alias (e.g., "ssh-config:tower02"), the username from sshConfigHost.user may be undefined. Previously defaulted to empty string which causes SSH auth failures. Now falls back to the current OS username via os.userInfo().username. Co-Authored-By: Claude Opus 4.6 <[email protected]>
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 `@src/main/ipc/sshIpc.ts`:
- Around line 466-475: The code constructs an SshConfig and currently forces
authType = sshConfigHost.identityFile ? 'key' : 'agent', which overrides the
resolved host intent; instead preserve the resolved auth choice from the host
object (e.g. use sshConfigHost.authType if present) and only fallback to a
sensible default when absent. Update the SshConfig creation (variable config) to
set authType = sshConfigHost.authType ?? (sshConfigHost.identityFile ? 'key' :
'agent') and set useAgent = sshConfigHost.useAgent ?? (authType === 'agent'), or
equivalent logic that prefers sshConfigHost.authType/identityAgent flags so you
don't collapse passphrase-protected-key or explicit agent intents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…SH config alias When connecting via SSH config alias, identityAgent (e.g. SSH_AUTH_SOCK) should take priority over identityFile for determining auth type and useAgent. Previously useAgent was always false when identityFile was set, ignoring identityAgent which represents explicit intent to use SSH agent forwarding. Co-Authored-By: Claude Opus 4.6 <[email protected]>
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
1 participant
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
Sidebar path display: Add path info after project name in sidebar
/home/user/project)tower01:/home/user/repo)remote.hostis unavailable, falls back to displayingconnectionIdFix ProjectConflictError: Allow remote projects with the same path but different SSH hosts
projects.path(with migration0013_remove_projects_path_unique.sql)sshConnectionId- two remote projects with the same path but differentsshConnectionIdare allowedssh-config:aliasformat is now resolved via SSH config parserTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Database
projects.pathto allow remote projects with the same path but different SSH connection IDs.Summary by CodeRabbit
New Features
Bug Fixes
Database
Tests