feat: import servers from ~/.ssh/config
#873
Merged
Conversation
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
Reviewer's GuideImplements SSH config import by adding an SSHConfig parser, server deduplication utilities, UI integration for triggering imports (both initial prompt and manual import in the server edit form), corresponding settings and localization, and extensive tests covering the new import workflow and core utilities. Sequence diagram for manual SSH config import in server edit pagesequenceDiagram
actor User
participant ServerEditPage
participant SSHConfig
participant ServerDeduplication
participant ServerStore
User->>ServerEditPage: Tap "Import from SSH config" button
ServerEditPage->>SSHConfig: parseConfig()
SSHConfig-->>ServerEditPage: List<Spi> (parsed servers)
ServerEditPage->>ServerDeduplication: deduplicateServers(servers)
ServerDeduplication->>ServerStore: fetch() (existing servers)
ServerStore-->>ServerDeduplication: List<Spi> (existing)
ServerDeduplication-->>ServerEditPage: deduplicated servers
ServerEditPage->>ServerDeduplication: resolveNameConflicts(deduplicated)
ServerDeduplication-->>ServerEditPage: resolved servers
ServerEditPage->>User: Show import summary dialog
User->>ServerEditPage: Confirm import
loop For each resolved server
ServerEditPage->>ServerStore: addServer(server)
end
ServerEditPage->>User: Show success message
Entity relationship diagram for server deduplication and importerDiagram
SERVER {
id string
name string
ip string
port int
user string
keyId string
jumpId string
}
IMPORT_SUMMARY {
total int
duplicates int
toImport int
}
SERVER ||--o{ IMPORT_SUMMARY: "imported servers"
SERVER ||--o{ SERVER: "deduplication (existing vs imported)"
Class diagram for SSHConfig and ServerDeduplication utilitiesclassDiagram
class SSHConfig {
+parseConfig(configPath)
+configExists(configPath)
-_parseSSHConfig(content)
-_extractJumpHost(value)
}
class ServerDeduplication {
+deduplicateServers(importedServers)
+resolveNameConflicts(importedServers)
+getImportSummary(originalList, deduplicatedList)
-_isDuplicate(imported, existing)
}
class ImportSummary {
+total: int
+duplicates: int
+toImport: int
+hasDuplicates: bool
+hasItemsToImport: bool
}
SSHConfig ..> Spi
ServerDeduplication ..> Spi
ServerDeduplication ..> ImportSummary
Class diagram for Spi changes (server_private_info.dart)classDiagram
class Spi {
+parseId(id)
+name: String
+ip: String
+port: int
+user: String
+keyId: String
+jumpId: String
+alterUrl: String
+custom: Custom
}
class Spix {
+isSameAs(other: Spi): bool
+shouldReconnect(old: Spi): bool
+parseAlterUrl(): (String, String, int)
+isRoot: bool
+toJsonString(): String
}
Spi <|.. Spix
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/core/utils/server_dedup.dart:12` </location>
<code_context>
+ final deduplicated = <Spi>[];
+
+ for (final imported in importedServers) {
+ if (!_isDuplicate(imported, existingServers)) {
+ deduplicated.add(imported);
+ }
+ }
+
</code_context>
<issue_to_address>
Deduplication logic may not handle case sensitivity in server names and users.
Normalization of case for server names and usernames during comparison is recommended to ensure accurate deduplication.
Suggested implementation:
```
for (final imported in importedServers) {
if (!_isDuplicateNormalized(imported, existingServers)) {
deduplicated.add(imported);
```
```
static List<Spi> deduplicateServers(List<Spi> importedServers) {
final existingServers = ServerStore.instance.fetch();
final deduplicated = <Spi>[];
for (final imported in importedServers) {
if (!_isDuplicateNormalized(imported, existingServers)) {
deduplicated.add(imported);
}
}
}
/// Checks for duplicates by normalizing case for server name and username
static bool _isDuplicateNormalized(Spi imported, List<Spi> existingServers) {
final importedName = imported.serverName.toLowerCase();
final importedUser = imported.username.toLowerCase();
for (final existing in existingServers) {
final existingName = existing.serverName.toLowerCase();
final existingUser = existing.username.toLowerCase();
if (importedName == existingName && importedUser == existingUser) {
return true;
}
}
return false;
```
If the `Spi` class uses different property names for server name or username, adjust `serverName` and `username` accordingly.
Remove or update the original `_isDuplicate` method if it exists and is no longer used.
</issue_to_address>
### Comment 2
<location> `lib/view/page/server/edit.dart:760` </location>
<code_context>
+ void _checkSSHConfigImport() async {
</code_context>
<issue_to_address>
First-time SSH config import logic does not persist user choice.
Currently, the user's decision is not saved, leading to repeated prompts. If this behavior is intended, please document the reasoning; otherwise, update the logic to persist the user's choice.
</issue_to_address>
### Comment 3
<location> `lib/data/model/server/server_private_info.dart:91` </location>
<code_context>
String toJsonString() => json.encode(toJson());
+ /// Returns true if the connection info is the same as [other].
+ bool isSameAs(Spi other) {
+ return user == other.user &&
+ ip == other.ip &&
+ port == other.port &&
+ pwd == other.pwd &&
+ keyId == other.keyId &&
+ jumpId == other.jumpId;
+ }
+
</code_context>
<issue_to_address>
isSameAs does not compare alterUrl or custom commands.
This difference may cause inconsistent behavior if isSameAs is used in contexts where alterUrl or custom.cmds matter. Please document this or update the comparison logic for consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Returns true if the connection info is the same as [other].
bool isSameAs(Spi other) {
return user == other.user &&
ip == other.ip &&
port == other.port &&
pwd == other.pwd &&
keyId == other.keyId &&
jumpId == other.jumpId;
}
=======
/// Returns true if the connection info is the same as [other].
/// Compares user, ip, port, pwd, keyId, jumpId, alterUrl, and custom.cmds.
bool isSameAs(Spi other) {
return user == other.user &&
ip == other.ip &&
port == other.port &&
pwd == other.pwd &&
keyId == other.keyId &&
jumpId == other.jumpId &&
alterUrl == other.alterUrl &&
_customCmdsEquals(custom, other.custom);
}
/// Helper to compare custom.cmds lists for equality.
bool _customCmdsEquals(Custom? a, Custom? b) {
if (a?.cmds == null && b?.cmds == null) return true;
if (a?.cmds == null || b?.cmds == null) return false;
if (a!.cmds.length != b!.cmds.length) return false;
for (int i = 0; i < a.cmds.length; i++) {
if (a.cmds[i] != b.cmds[i]) return false;
}
return true;
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+12
to
+14
| if (!_isDuplicate(imported, existingServers)) { | ||
| deduplicated.add(imported); | ||
| } |
There was a problem hiding this comment.
suggestion: Deduplication logic may not handle case sensitivity in server names and users.
Normalization of case for server names and usernames during comparison is recommended to ensure accurate deduplication.
Suggested implementation:
for (final imported in importedServers) {
if (!_isDuplicateNormalized(imported, existingServers)) {
deduplicated.add(imported);
static List<Spi> deduplicateServers(List<Spi> importedServers) {
final existingServers = ServerStore.instance.fetch();
final deduplicated = <Spi>[];
for (final imported in importedServers) {
if (!_isDuplicateNormalized(imported, existingServers)) {
deduplicated.add(imported);
}
}
}
/// Checks for duplicates by normalizing case for server name and username
static bool _isDuplicateNormalized(Spi imported, List<Spi> existingServers) {
final importedName = imported.serverName.toLowerCase();
final importedUser = imported.username.toLowerCase();
for (final existing in existingServers) {
final existingName = existing.serverName.toLowerCase();
final existingUser = existing.username.toLowerCase();
if (importedName == existingName && importedUser == existingUser) {
return true;
}
}
return false;
If the Spi class uses different property names for server name or username, adjust serverName and username accordingly.
Remove or update the original _isDuplicate method if it exists and is no longer used.
Comment on lines
+760
to
+769
| void _checkSSHConfigImport() async { | ||
| final prop = Stores.setting.firstTimeReadSSHCfg; | ||
| // Only check if it's first time and user hasn't disabled it | ||
| if (!prop.fetch()) return; | ||
|
|
||
| try { | ||
| // Check if SSH config exists | ||
| final (_, configExists) = SSHConfig.configExists(); | ||
| if (!configExists) return; | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): First-time SSH config import logic does not persist user choice.
Currently, the user's decision is not saved, leading to repeated prompts. If this behavior is intended, please document the reasoning; otherwise, update the logic to persist the user's choice.
Comment on lines
+90
to
+98
| /// Returns true if the connection info is the same as [other]. | ||
| bool isSameAs(Spi other) { | ||
| return user == other.user && | ||
| ip == other.ip && | ||
| port == other.port && | ||
| pwd == other.pwd && | ||
| keyId == other.keyId && | ||
| jumpId == other.jumpId; | ||
| } |
There was a problem hiding this comment.
suggestion: isSameAs does not compare alterUrl or custom commands.
This difference may cause inconsistent behavior if isSameAs is used in contexts where alterUrl or custom.cmds matter. Please document this or update the comparison logic for consistency.
Suggested change
| /// Returns true if the connection info is the same as [other]. | |
| bool isSameAs(Spi other) { | |
| return user == other.user && | |
| ip == other.ip && | |
| port == other.port && | |
| pwd == other.pwd && | |
| keyId == other.keyId && | |
| jumpId == other.jumpId; | |
| } | |
| /// Returns true if the connection info is the same as [other]. | |
| /// Compares user, ip, port, pwd, keyId, jumpId, alterUrl, and custom.cmds. | |
| bool isSameAs(Spi other) { | |
| return user == other.user && | |
| ip == other.ip && | |
| port == other.port && | |
| pwd == other.pwd && | |
| keyId == other.keyId && | |
| jumpId == other.jumpId && | |
| alterUrl == other.alterUrl && | |
| _customCmdsEquals(custom, other.custom); | |
| } | |
| /// Helper to compare custom.cmds lists for equality. | |
| bool _customCmdsEquals(Custom? a, Custom? b) { | |
| if (a?.cmds == null && b?.cmds == null) return true; | |
| if (a?.cmds == null || b?.cmds == null) return false; | |
| if (a!.cmds.length != b!.cmds.length) return false; | |
| for (int i = 0; i < a.cmds.length; i++) { | |
| if (a.cmds[i] != b.cmds[i]) return false; | |
| } | |
| return true; | |
| } |
Owner
Author
|
@claude review |
|
Claude finished @lollipopkit's task —— View job Code Review: SSH Config Import FeatureExecutive SummaryThis is a well-implemented feature that successfully addresses issue #871. The implementation demonstrates strong engineering practices with comprehensive test coverage (1,083 lines), proper error handling, and thoughtful user experience design. Key Strengths ✅1. Robust SSH Config Parsing (
|
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.
Fixes #871
Summary by Sourcery
Enable importing SSH servers by parsing ~/.ssh/config, integrating a manual import button and auto-import prompt, deduplicating and resolving conflicts, extending the Spi model, and covering the new workflow with unit tests
New Features:
Enhancements:
Documentation:
Tests:
Chores: