feat: jump server chain #1006
feat: jump server chain #1006
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds multi‑hop jump‑server support across model, storage, UI and SSH core: new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant API as genClient
participant Core as _genClientInternal
participant Store as SPI Resolver
participant JumpA as Jump Server A
participant JumpB as Jump Server B
participant Target as Target Server
App->>API: genClient(targetSpi, jumpChain=[A,B], jumpPrivateKeys=[kA,kB])
API->>Core: _genClientInternal(spi, jumpChain?, visited={})
Core->>Store: resolveMergedJumpChain(targetSpi) -> [A,B]
Core->>Core: validate visited & normalize IDs
Core->>JumpA: establish SSH (private key kA) -> clientA
Core->>JumpB: connect via clientA (private key kB) -> clientB
Core->>Target: connect via clientB -> final SSH client
Core-->>API: return final SSH client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Fixes #356
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be9b9b10b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (final hopId in queue) { | ||
| final extra = flattenHopIds(hopId, visited: visited); |
There was a problem hiding this comment.
The new containsCycleWithCandidate only checks for direct duplicates and then calls flattenHopIds with a visited set that already contains every hop ID from the first loop, so flattenHopIds immediately returns without traversing each hop’s existing jumpChainIds. This means a candidate hop that already routes back to the current server (or through another hop) is still selectable, and the cycle won’t be caught until runtime in genClient (triggering the SSHErr loop detection). To actually prevent circular configurations in the UI, call flattenHopIds with a fresh visited (or add hops after the recursive walk) so nested chains are inspected.
Useful? React with 👍 / 👎.
lib/core/utils/server.dart
Outdated
| final jumpSpi = spi.jumpId != null ? Stores.server.box.get(spi.jumpId) : null; | ||
| if (jumpSpi != null && !_hasKnownHostFingerprintForSpi(jumpSpi, cache)) { |
There was a problem hiding this comment.
ensureKnownHostKey still only walks spi.jumpId and ignores jumpChainIds, so in multi-hop setups only the legacy single hop is pre-accepted. This is problematic for flows that rely on pre-seeded fingerprints (e.g., SFTP in an isolate where the default host key prompt cannot show UI and returns false), because unknown host keys on intermediate hops will cause connection failure even though the target’s chain is configured. The function should iterate the full jumpChainIds list (and detect loops) to ensure all hop fingerprints are accepted before connecting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/provider/server/single.dart (1)
138-142: Add defensive check for emptyjumpChainIdslist to align with other code paths.The condition only checks for null, but other parts of the codebase defensively check for both null and empty list (e.g.,
lib/view/page/server/edit/jump_chain.dart:27andlib/core/utils/server.dart:116). While the save flow normalizes empty lists to null, the model'scopyWithdoes not enforce this, making an empty list theoretically possible.Suggested change
- if (spi.jumpChainIds == null && spi.jumpId == null) { + if ((spi.jumpChainIds?.isNotEmpty != true) && spi.jumpId == null) {
🤖 Fix all issues with AI agents
In `@lib/view/page/server/edit/actions.dart`:
- Around line 225-232: The confirmation dialog is shown for any edit when spi !=
null but should only trigger if the jump chain actually changed; update the
condition around the context.showRoundDialog call to compare the current
_jumpChain.value with the original spi.jumpChainIds (or equivalent stored
originalJumpChain) and only show the dialog when they differ, leaving the
existing dialog text (libL10n.askContinue('${l10n.jumpServer}
${libL10n.setting}')) and action handling untouched; alternatively, if you
prefer a generic confirmation, replace the message with a neutral string from
libL10n and keep the current spi != null condition.
🧹 Nitpick comments (2)
lib/data/model/server/server_private_info.dart (1)
102-110: Consider simplifyingisSameAsto only checkjumpChainIdspost-migration.The method checks both
jumpIdandjumpChainIdsfor equality. After migration completes,jumpIdshould be null andjumpChainIdsshould hold all hop data. Checking both fields may cause false negatives if one server was migrated and another wasn't, even though they represent the same jump configuration.If backward compatibility with pre-migration data is required in
isSameAs, this is fine. Otherwise, consider normalizing both sides before comparison or only comparingjumpChainIds.lib/view/page/server/edit/jump_chain.dart (1)
118-119: Consider responsive width for the editor dialog.The fixed
width: 320may not adapt well to different screen sizes (e.g., narrow phones or tablets). Consider using a responsive approach.♻️ Optional: Use responsive constraints
child: SizedBox( - width: 320, + width: MediaQuery.of(context).size.width * 0.8, child: _jumpChain.listenVal((hops) {Or use
ConstrainedBoxwithmaxWidthfor better flexibility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/core/utils/server.dartlib/data/model/server/server_private_info.dartlib/data/model/server/server_private_info.freezed.dartlib/data/model/server/server_private_info.g.dartlib/data/model/sftp/req.dartlib/data/provider/server/all.g.dartlib/data/provider/server/single.dartlib/data/provider/server/single.g.dartlib/data/store/server.dartlib/hive/hive_adapters.g.dartlib/hive/hive_adapters.g.yamllib/view/page/server/edit/actions.dartlib/view/page/server/edit/edit.dartlib/view/page/server/edit/jump_chain.dartlib/view/page/server/edit/widget.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/view/page/server/edit/widget.dart
- lib/core/utils/server.dart
🧰 Additional context used
📓 Path-based instructions (7)
lib/data/provider/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Riverpod providers by feature under lib/data/provider/
Files:
lib/data/provider/server/single.dartlib/data/provider/server/single.g.dartlib/data/provider/server/all.g.dart
**/*.{g,freezed}.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Do not manually edit generated Dart files (*.g.dart, *.freezed.dart)
Files:
lib/data/model/server/server_private_info.g.dartlib/data/provider/server/single.g.dartlib/data/model/server/server_private_info.freezed.dartlib/hive/hive_adapters.g.dartlib/data/provider/server/all.g.dart
lib/data/model/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
After modifying annotated model files, run: dart run build_runner build --delete-conflicting-outputs
Files:
lib/data/model/server/server_private_info.g.dartlib/data/model/sftp/req.dartlib/data/model/server/server_private_info.freezed.dartlib/data/model/server/server_private_info.dart
lib/data/model/server/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place server management models under lib/data/model/server/
Files:
lib/data/model/server/server_private_info.g.dartlib/data/model/server/server_private_info.freezed.dartlib/data/model/server/server_private_info.dart
lib/data/store/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Persist state using Hive stores placed under lib/data/store/
Files:
lib/data/store/server.dart
lib/view/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/view/**/*.dart: Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Prefer using libL10n strings before adding new ones to project l10n
Split UI into build, actions, and utils; use extension on to separate concerns
Files:
lib/view/page/server/edit/jump_chain.dartlib/view/page/server/edit/edit.dartlib/view/page/server/edit/actions.dart
lib/data/model/sftp/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place SFTP models under lib/data/model/sftp/
Files:
lib/data/model/sftp/req.dart
🧠 Learnings (8)
📚 Learning: 2026-01-11T10:26:03.064Z
Learnt from: GT-610
Repo: lollipopkit/flutter_server_box PR: 999
File: lib/data/provider/server/all.dart:0-0
Timestamp: 2026-01-11T10:26:03.064Z
Learning: In lib/data/provider/server/all.dart, the `_refreshInProgress` guard was removed because: (1) each server has its own `_isRefreshing` protection in single.dart that prevents concurrent refreshes per server, (2) the coordination overhead in all.dart is minimal (milliseconds) with no harm from concurrent calls, (3) no code awaits the completer, and (4) removing it enables true parallelism where servers refresh independently without artificial serialization at the coordination layer.
Applied to files:
lib/data/provider/server/single.dartlib/data/provider/server/single.g.dartlib/data/provider/server/all.g.dart
📚 Learning: 2026-01-11T05:42:59.995Z
Learnt from: GT-610
Repo: lollipopkit/flutter_server_box PR: 0
File: :0-0
Timestamp: 2026-01-11T05:42:59.995Z
Learning: In lib/data/provider/server/single.dart, the `_updateServer()` method is kept as a separate method (rather than inlined) for abstraction and future extensibility, clearly separating the refresh lock logic from the actual data fetching operation.
Applied to files:
lib/data/provider/server/single.dartlib/data/provider/server/single.g.dartlib/data/provider/server/all.g.dartlib/data/model/server/server_private_info.dart
📚 Learning: 2026-01-11T05:42:59.995Z
Learnt from: GT-610
Repo: lollipopkit/flutter_server_box PR: 0
File: :0-0
Timestamp: 2026-01-11T05:42:59.995Z
Learning: In lib/data/provider/server/single.dart, the Boolean flag `_isRefreshing` is used intentionally for concurrency control instead of a Completer pattern. This is because all.dart uses `Future.wait(refreshFutures)` to coordinate parallel refreshes across multiple independent server notifiers, and the Boolean flag provides per-server concurrency protection while maintaining parallelism across servers.
Applied to files:
lib/data/provider/server/single.g.dartlib/data/provider/server/all.g.dartlib/data/model/server/server_private_info.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/server/**/*.dart : Place server management models under lib/data/model/server/
Applied to files:
lib/data/store/server.dartlib/data/provider/server/all.g.dartlib/data/model/server/server_private_info.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/sftp/**/*.dart : Place SFTP models under lib/data/model/sftp/
Applied to files:
lib/data/model/sftp/req.dartlib/data/model/server/server_private_info.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/ssh/**/*.dart : Place SSH models under lib/data/model/ssh/
Applied to files:
lib/data/model/sftp/req.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/view/**/*.dart : Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Applied to files:
lib/data/model/server/server_private_info.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/view/**/*.dart : Prefer using libL10n strings before adding new ones to project l10n
Applied to files:
lib/data/model/server/server_private_info.dart
🔇 Additional comments (17)
lib/data/provider/server/all.g.dart (1)
44-44: Generated hash update looks correct.This hash change reflects the updated
ServersNotifierimplementation for the jump chain feature. As per coding guidelines, this generated file should not be manually edited.lib/data/provider/server/single.g.dart (1)
61-61: Generated hash update looks correct.This hash change reflects the updated
ServerNotifierimplementation. As per coding guidelines, this generated file should not be manually edited.lib/hive/hive_adapters.g.yaml (1)
28-65: Schema changes for jump chain feature look correct.The
jumpChainIdsfield is added with the correct index (16), andnextIndexis properly incremented to 17. Retaining the legacyjumpIdfield (index 9) ensures backward compatibility for existing data during migration.lib/view/page/server/edit/edit.dart (3)
28-29: Part file addition for jump chain UI looks good.The modular approach of separating jump chain UI into its own part file follows the coding guidelines for organizing view layer code.
70-70: State migration from single hop to chain-based list is correct.The
.vnextension properly creates aValueNotifier<List<String>>for the jump chain state.
104-104: Jump chain UI integration is already in place.The
_buildJumpChain()is properly integrated in the widget part file (widget.dart, line 135) and the disposal at line 104 is correctly updated. No action required.lib/data/model/server/server_private_info.dart (2)
39-47: Well-documented field addition with clear migration path.The documentation clearly explains the relationship between legacy
jumpIdand the newjumpChainIds, making the migration strategy transparent for future maintainers.
90-93: Migration logic correctly promotes jumpId to jumpChainIds.The fallback
jumpChainIds ?? (jumpId == null ? null : [jumpId!])properly handles legacy single-hop configurations by wrapping them in a list.lib/data/store/server.dart (1)
90-98: Jump chain migration logic is correct.The migration properly:
- Derives the chain from
jumpChainIdsor falls back to wrappingjumpId- Remaps IDs through
idMapwhile preserving unmapped IDs- Clears the legacy
jumpIdfield after migrationThe fallback
idMap[e] ?? eensures that jump targets referencing servers that weren't migrated (already had new-format IDs) are preserved.lib/view/page/server/edit/actions.dart (2)
289-290: Correct transition to chain-based jump configuration.Setting
jumpId: nulland populatingjumpChainIdsensures new/updated servers use the chain model exclusively.
434-434: Initialization correctly handles both legacy and new formats.The fallback logic mirrors the migration pattern used elsewhere, ensuring consistent behavior when loading existing server configurations.
lib/data/model/sftp/req.dart (2)
18-43: Jump chain construction with loop detection looks correct.The loop detection using a
visitedset is well-implemented. One observation: on line 26, if a hop server is not found (hopSpi == null), the loop silently breaks, resulting in a partial chain. This may be intentional (graceful degradation), but consider logging a warning or throwing an error if a configured hop is missing, as it could indicate data corruption or misconfiguration.💡 Optional: Add warning for missing hop
final hopSpi = Stores.server.box.get(hopId); - if (hopSpi == null) break; + if (hopSpi == null) { + Loggers.app.warning('Jump hop server not found: $hopId'); + break; + }
115-115: Good typo fix.Correcting
sshConnectted→sshConnectedimproves code clarity.lib/view/page/server/edit/jump_chain.dart (3)
21-35: Recursive flattening with cycle protection is well-implemented.The
flattenHopIdsfunction correctly uses avisitedset to prevent infinite recursion when traversing nested jump chains. The early return on cycle detection (line 22) is appropriate.
37-53: Cycle detection for candidates is thorough.The two-phase approach correctly validates both direct cycles (candidate equals self or duplicates in chain) and transitive cycles (through nested jump chains). The shared
visitedset ensures consistent tracking across both phases.
92-113: Clear UX flow for adding and managing hops.The pattern of adding a hop first (line 113), then opening the editor dialog for further management is intuitive. Users can quickly add a single hop or continue to reorder/remove as needed.
lib/data/model/server/server_private_info.freezed.dart (1)
1-10: Generated file — no manual review required.This is auto-generated by Freezed. Per coding guidelines, do not manually edit
*.freezed.dartfiles. After modifying the annotated model inserver_private_info.dart, ensure the code is regenerated with:dart run build_runner build --delete-conflicting-outputs
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/core/utils/server.dart`:
- Around line 240-264: Wrap the hop/chain-building loop in a try-catch-finally
and maintain a List<SSHClient> (e.g., named createdClients) that you append each
currentClient returned from _genClientInternal (and the initial client) to; if
any await (forwardLocal or _genClientInternal) throws, iterate createdClients
and call close() on each to avoid leaks, rethrow the error, and on success close
any intermediate clients that are no longer needed before returning the final
forwarded socket; ensure you reference currentClient, _genClientInternal,
forwardLocal and resolveHopPrivateKey when updating the code so intermediate
SSHClient instances are properly closed in both error and success paths.
🧹 Nitpick comments (5)
lib/view/page/server/edit/actions.dart (1)
225-247: Jump chain change detection looks correct.The logic properly addresses the previous review concern by only showing the confirmation dialog when the jump chain actually changes. The comparison handles empty lists, length differences, and element-by-element checks.
Consider using
listEqualsfrompackage:flutter/foundation.dartfor cleaner comparison:♻️ Optional simplification
+import 'package:flutter/foundation.dart' show listEquals; + final oldSpi = this.spi; if (oldSpi != null) { final originalJumpChain = oldSpi.jumpChainIds ?? (oldSpi.jumpId == null ? const <String>[] : [oldSpi.jumpId!]); final currentJumpChain = _jumpChain.value; - final jumpChainChanged = () { - if (originalJumpChain.isEmpty && currentJumpChain.isEmpty) return false; - if (originalJumpChain.length != currentJumpChain.length) return true; - for (var i = 0; i < originalJumpChain.length; i++) { - if (originalJumpChain[i] != currentJumpChain[i]) return true; - } - return false; - }(); + final jumpChainChanged = !listEquals(originalJumpChain, currentJumpChain); if (jumpChainChanged) {lib/view/page/server/edit/jump_chain.dart (3)
55-66: Potential duplicate display if chain contains repeated hops.If
_jumpChain.valuecontains the same hop ID twice (e.g., through manual data manipulation),buildTextNearToFarwould add it toflattwice but only traverse its nested chain once due to thevisitedset. The UI should prevent duplicates viacontainsCycleWithCandidate, but this edge case could cause confusing display text.Consider adding deduplication or a defensive check:
♻️ Optional defensive check
String? buildTextNearToFar() { if (_jumpChain.value.isEmpty) return null; final flat = <String>[]; final visited = <String>{selfId}; for (final hopId in _jumpChain.value) { + if (visited.contains(hopId)) continue; // skip duplicates flat.add(hopId); flat.addAll(flattenHopIds(hopId, visited: visited)); }
121-167: Consider using ReorderableListView for better UX.The manual up/down arrow buttons work but Flutter's
ReorderableListViewprovides a more intuitive drag-and-drop experience for reordering items. This would reduce button clutter and improve touch accessibility.♻️ Suggested alternative with ReorderableListView
ReorderableListView.builder( shrinkWrap: true, itemCount: hops.length, onReorder: (oldIndex, newIndex) { if (newIndex > oldIndex) newIndex--; final list = [..._jumpChain.value]; final item = list.removeAt(oldIndex); list.insert(newIndex, item); _jumpChain.value = list; }, itemBuilder: (context, index) { final id = hops[index]; return ListTile( key: ValueKey(id), title: Text(serverNameOrId(id)), subtitle: Text(id, style: UIs.textGrey), trailing: IconButton( icon: const Icon(Icons.delete, size: 18), onPressed: () { final list = [..._jumpChain.value]..removeAt(index); _jumpChain.value = list; }, ), ); }, )
86-90: Clarify the distinction between "Route" and "Path" labels.The UI shows both
l10n.route(near-to-far) andlibL10n.path(far-to-near), but both display the same chain in opposite orders. Users may find this confusing without context about why both are shown.Consider either:
- Adding tooltips or help text explaining the difference
- Showing only one representation (typically the connection order: far-to-near)
- Using clearer labels like "Connection order" vs "Return path"
lib/core/utils/server.dart (1)
182-215: DuplicatedresolveSpilogic.The
resolveSpifunction at lines 197-208 duplicates the logic from lines 53-64 inresolveMergedJumpChain. Consider extracting a shared helper to reduce duplication and ensure consistent behavior.♻️ Suggested extraction
Spi Function(String) _buildSpiResolver({ required Map<String, Spi> injectedSpiMap, required bool strictMode, // true when jumpChain != null }) { return (String id) { final injected = injectedSpiMap[id]; if (injected != null) return injected; if (strictMode) { throw SSHErr(type: SSHErrType.connect, message: 'Jump server not found in provided chain: $id'); } final fromStore = Stores.server.box.get(id); if (fromStore == null) { throw SSHErr(type: SSHErrType.connect, message: 'Jump server not found: $id'); } return fromStore; }; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/core/utils/server.dartlib/view/page/server/edit/actions.dartlib/view/page/server/edit/jump_chain.darttest/jump_server_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/jump_server_test.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/view/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/view/**/*.dart: Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Prefer using libL10n strings before adding new ones to project l10n
Split UI into build, actions, and utils; use extension on to separate concerns
Files:
lib/view/page/server/edit/jump_chain.dartlib/view/page/server/edit/actions.dart
🔇 Additional comments (7)
lib/view/page/server/edit/actions.dart (2)
304-305: Migration from single jumpId to jumpChainIds is handled correctly.Setting
jumpId: nulland populatingjumpChainIdsaligns with the new chain-based model. The conditional null assignment when the chain is empty avoids storing unnecessary empty lists.
449-449: Initialization properly handles backward compatibility.The fallback logic
spi.jumpChainIds ?? (spi.jumpId == null ? const <String>[] : [spi.jumpId!])correctly migrates existing servers with singlejumpIdto the new chain representation.lib/view/page/server/edit/jump_chain.dart (2)
9-15: Good guard for new server creation.When
selfIdis null (creating a new server), showing a placeholder tile is appropriate since jump chains can only reference existing servers. The user must save the server first before configuring jump chains.
37-53: Cycle detection at UI level is limited to cycles involving the current server; transitive cycles among other servers are only detected at runtime.The implementation checks for:
- Direct duplicates in the proposed chain (line 43)
- Paths that loop back to
selfId(lines 46-51)However, it doesn't prevent cycles formed entirely among jump servers that don't include
selfId. For example, if server A's chain includes B and B's chain includes A, adding either as a candidate would pass this check whenselfIdis neither A nor B.This is mitigated by runtime detection in
genOrderedJumpChain()which uses DFS with stack-based cycle detection and catches all cycles regardless of whether they involveselfId. The error will be visible to the user as a connection failure. If immediate UI-level feedback is important for usability, the UI check could be enhanced to use the same graph-wide detection, but the current approach is acceptable given the runtime protection.lib/core/utils/server.dart (3)
69-108: LGTM - Well-structured DFS with proper cycle detection.The jump chain resolution correctly handles both
jumpChainIdsand legacyjumpIdfallback, with proper cycle detection using separateseenandstacksets. The topological ordering ensures dependencies are resolved before dependents.Minor note: Line 98's
seen.add(norm)in the condition is slightly redundant since line 89 already guards against re-entry, but this is a harmless defensive pattern.
110-148: LGTM - Clean public API with internal delegation.The updated signature correctly introduces
jumpChainandjumpPrivateKeysfor isolate contexts while maintaining backward compatibility. Initializingvisitedat the public entry point ensures cycle detection across the entire connection tree.
478-501: Addresses past review concern: now traverses full jump chain for host key verification.The updated implementation correctly uses
resolveMergedJumpChainto iterate all hops and verify their host keys before connecting to the target. This resolves the previously flagged issue where onlyspi.jumpIdwas checked.Note:
_ensureKnownHostKeyForSinglecallsgenClientwithout passingjumpChain/jumpPrivateKeys, so this function will not work correctly in isolate contexts whereStoresis unavailable. If isolate support is needed for host key pre-verification, consider adding these parameters toensureKnownHostKey.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai Why still blocking? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1567 🏁 Script executed: Length of output: 1260
Here are the pending review comments that need attention:
To unblock the PR, you can:
Would you like me to take another look at the specific issues, or would you prefer to address them first? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/data/model/sftp/req.dart`:
- Around line 26-29: Replace the silent truncation when a hop is missing:
instead of calling Loggers.app.warning('Jump server not found: $hopId,
truncating chain') and break, throw an SSHErr (the same error type used in
server.dart) including a clear message with hopId so callers can handle the
failure consistently; update the surrounding code paths that build/validate the
hop chain to propagate or catch this SSHErr as appropriate.
♻️ Duplicate comments (1)
lib/core/utils/server.dart (1)
222-286: Resource leak on success path persists.The error-path cleanup (lines 273-282) is now implemented, but the fundamental issue remains: on success,
createdClientsgoes out of scope and the intermediateSSHClientinstances become unreachable while still holding resources.The comment at lines 284-285 correctly explains the constraint—the socket tunnels through these clients, so they must remain open. However, without returning or tracking these clients, they can never be properly closed when the caller is finished.
Consider returning a wrapper that includes both the socket and the intermediate clients, enabling the caller to manage their lifecycle:
class TunneledConnection { final SSHSocket socket; final List<SSHClient> _intermediateClients; void close() { for (final client in _intermediateClients) { client.close(); } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/core/utils/server.dartlib/data/model/sftp/req.dart
🧰 Additional context used
📓 Path-based instructions (2)
lib/data/model/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
After modifying annotated model files, run: dart run build_runner build --delete-conflicting-outputs
Files:
lib/data/model/sftp/req.dart
lib/data/model/sftp/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place SFTP models under lib/data/model/sftp/
Files:
lib/data/model/sftp/req.dart
🧠 Learnings (2)
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/sftp/**/*.dart : Place SFTP models under lib/data/model/sftp/
Applied to files:
lib/data/model/sftp/req.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/ssh/**/*.dart : Place SSH models under lib/data/model/ssh/
Applied to files:
lib/data/model/sftp/req.dart
🔇 Additional comments (7)
lib/data/model/sftp/req.dart (3)
9-10: LGTM! Clean replacement of single-hop fields with chain-based structure.The new
jumpChainandjumpPrivateKeysfields properly support multi-hop traversal while maintaining alignment by index.
30-36: LGTM! Loop detection logic is correct.The visited set properly tracks both the target server and all processed hops, throwing
SSHErrwhen a cycle is detected.
117-117: LGTM! Good typo fix.Correcting
sshConnectted→sshConnectedimproves API consistency.lib/core/utils/server.dart (4)
41-110: LGTM! Well-implemented DFS-based chain resolution with cycle detection.The algorithm correctly:
- Uses
stackfor detecting cycles during DFS traversal- Uses
seenfor deduplication of fully processed nodes- Produces topologically-sorted output (dependencies before dependents)
- Handles both
jumpChainIdsand legacyjumpIdfallback
112-150: LGTM! Clean public API extension.The public
genClientmaintains backward compatibility while properly initializing thevisitedset for cycle detection at the entry point.
499-522: LGTM! ensureKnownHostKey now properly traverses full jump chain.This addresses the previous review concern—the function now uses
resolveMergedJumpChain(spi)to resolve all hops (includingjumpChainIds), ensuring all intermediate host keys are verified before connecting.
524-549: LGTM! Clean helper for per-hop host key verification.The early return optimization (lines 530-532) avoids unnecessary connections, and the cache accumulation pattern (line 547) ensures newly accepted fingerprints are available for subsequent hops.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
lib/data/model/sftp/req.dart
Outdated
| if (hopSpi == null) { | ||
| Loggers.app.warning('Jump server not found: $hopId, truncating chain'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: silent truncation vs throwing in server.dart.
When a hop is not found, this code logs a warning and truncates the chain, while server.dart throws SSHErr. This inconsistency could cause confusing failures—if a user configures chain [A, B, C] and B is missing, the chain silently becomes [A], leading to unexpected connection behavior.
Consider throwing SSHErr here for consistency:
Suggested fix
if (hopSpi == null) {
- Loggers.app.warning('Jump server not found: $hopId, truncating chain');
- break;
+ throw SSHErr(
+ type: SSHErrType.connect,
+ message: 'Jump server not found: $hopId',
+ );
}📝 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.
| if (hopSpi == null) { | |
| Loggers.app.warning('Jump server not found: $hopId, truncating chain'); | |
| break; | |
| } | |
| if (hopSpi == null) { | |
| throw SSHErr( | |
| type: SSHErrType.connect, | |
| message: 'Jump server not found: $hopId', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@lib/data/model/sftp/req.dart` around lines 26 - 29, Replace the silent
truncation when a hop is missing: instead of calling Loggers.app.warning('Jump
server not found: $hopId, truncating chain') and break, throw an SSHErr (the
same error type used in server.dart) including a clear message with hopId so
callers can handle the failure consistently; update the surrounding code paths
that build/validate the hop chain to propagate or catch this SSHErr as
appropriate.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6738ac94f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/data/model/sftp/req.dart
Outdated
| final hopIds = spi.jumpChainIds ?? (spi.jumpId == null ? const <String>[] : [spi.jumpId!]); | ||
| for (final hopId in hopIds) { | ||
| final hopSpi = Stores.server.box.get(hopId); | ||
| if (hopSpi == null) { | ||
| throw SSHErr( |
There was a problem hiding this comment.
When an SFTP download/upload runs in the isolate, genClient will resolve the full merged jump chain and, if jumpChain is provided, will throw when a referenced hop is missing. Here the request only collects the immediate jumpChainIds/jumpId from the target and never expands hops that themselves have jumpChainIds, so any multi-hop configuration like “target -> A -> B” (where A has its own jump chain) will fail with “Jump server not found in provided chain” during isolate connections. This breaks SFTP for nested jump chains; consider using resolveMergedJumpChain (and aligning keys) or recursively collecting all hop dependencies.
Useful? React with 👍 / 👎.
|
@codex review |
|
@coderabbitai Major issue in lib/core/utils/server.dart already has been solved, re-check it |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 26505
I can confirm that the current implementation (lines 228-281) now properly handles resource cleanup:
This implementation matches exactly what was suggested in the review and properly addresses the resource leak concern. Remaining item: There's still one minor issue pending in 🧠 Learnings used |
|
@coderabbitai Describe the minor issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16497
🟡 Original Issue: Jump Chain Confirmation DialogLocation: The Concern: Expected Behavior:
✅ Current Implementation ReviewHowever, looking at the current code (lines 208-227), I can see that you've actually implemented proper checking: final jumpChainChanged = () {
if (originalJumpChain.isEmpty && currentJumpChain.isEmpty) return false;
if (originalJumpChain.length != currentJumpChain.length) return true;
for (var i = 0; i < originalJumpChain.length; i++) {
if (originalJumpChain[i] != currentJumpChain[i]) return true;
}
return false;
}();
if (jumpChainChanged) {
// Only show dialog when jump chain actually changed
final ok = await context.showRoundDialog<bool>(...)
}This logic correctly:
It appears this issue has already been properly addressed in the current implementation! The dialog will only trigger when the jump chain is actually modified, not for unrelated edits. Would you like me to verify this is working as expected, or can I mark this issue as resolved? 🤔 🧠 Learnings used |
|
@coderabbitai Why still blocking? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3150
1. ❌ Failing CI CheckThe
This needs to be fixed before the PR can be merged. 2. 📝 Unresolved Review Comment ThreadsThere are 5 unresolved review comment threads keeping the review status at From CodeRabbit (me):
From Codex:
✅ To Unblock:
Would you like me to resolve my comment thread on 🧠 Learnings used |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eeb55c1d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| final hops = resolveMergedJumpChain(spi); | ||
| for (final hop in hops) { | ||
| cache = await _ensureKnownHostKeyForSingle( | ||
| hop, |
There was a problem hiding this comment.
Here ensureKnownHostKey iterates the merged hop list and calls _ensureKnownHostKeyForSingle for each hop, but that helper uses genClient(hop, ...) with no jump-chain context. In a multi-hop setup where a hop is only reachable through earlier hops (e.g., chain A→B and B has no jumpChainIds of its own), this will attempt a direct TCP connection to B, time out, and the SFTP host‑key precheck fails even though the chain itself is valid. Consider passing the appropriate preceding hops (or a socket override) so each hop’s host-key verification uses the same jump route as the actual connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/core/utils/server.dart`:
- Around line 188-197: The loop unconditionally writes entries for s.oldId into
injectedSpiMap (and injectedKeyMap) which can add an empty-string key; update
the loop that iterates jumpChain so you only set injectedSpiMap[s.oldId] = s and
injectedKeyMap[s.oldId] = jumpPrivateKeys[i] when s.oldId.isNotEmpty (mirror the
guard used in resolveMergedJumpChain) to avoid introducing an empty-string key
that breaks resolveSpi lookups.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/core/utils/server.dartlib/data/model/sftp/worker.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/data/model/sftp/worker.dart
🔇 Additional comments (6)
lib/core/utils/server.dart (6)
41-69: LGTM: Jump chain resolution with proper error handling.The
resolveMergedJumpChainfunction correctly builds a map from the providedjumpChainand handles bothidandoldIdlookups. The nestedresolveSpifunction properly distinguishes between isolate mode (wherejumpChainis required) and normal mode (whereStorescan be accessed).
71-110: LGTM: DFS-based chain resolution with proper cycle detection.The implementation correctly uses a two-set approach (
seenfor visited nodes,stackfor current DFS path) to detect cycles while avoiding duplicate processing. The fallback fromjumpChainIdstojumpId(line 75, 94) maintains backward compatibility with single-hop configurations.
112-150: LGTM: Clean public API with internal delegation.The
genClientpublic function properly initializes thevisitedset and delegates to_genClientInternal. The documentation clearly explains the purpose ofjumpChainandjumpPrivateKeysparameters for isolate scenarios whereStoresis unavailable.
220-286: Resource cleanup issue addressed.The multi-hop chain building now properly tracks intermediate clients in
createdClients(lines 245, 269) and closes them all on error (lines 275-280). The comment at lines 284-285 correctly explains that intermediate clients must remain open on success because the returned socket tunnels through them.This addresses the previously flagged resource leak concern.
499-539: Host-key verification now properly traverses the full jump chain.The
ensureKnownHostKeyfunction now:
- Uses
resolveMergedJumpChain(spi)(line 506) to resolve bothjumpChainIdsand the legacyjumpId— addressing the previously raised concern about ignoringjumpChainIds.- Routes each hop's host-key check through its preceding hops by passing
precedingHopsasjumpChain(lines 512-523) — addressing the concern about attempting direct TCP connections to intermediate hops that are only reachable through earlier hops.This implementation correctly ensures all hop fingerprints are verified before establishing the full chain.
541-570: LGTM: Per-hop host-key verification with proper cleanup.The
_ensureKnownHostKeyForSinglehelper correctly:
- Short-circuits if the fingerprint is already cached (line 549-551)
- Passes
jumpChain/jumpPrivateKeysto route the connection through prior hops- Uses try-finally to ensure the client is closed after verification (lines 562-566)
- Returns the updated cache for use by subsequent iterations
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c80795f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/core/utils/server.dart
Outdated
| } | ||
|
|
||
| return await currentClient!.forwardLocal(spi.ip, spi.port); |
There was a problem hiding this comment.
In the multi-hop branch, hop clients are added to createdClients but on success the function immediately returns the forwarded socket without retaining any reference to those clients. As a result, when the caller later closes the target SSHClient, the intermediate hop sessions remain open with no way to close them, leaking SSH connections for every hop in the chain. This occurs whenever a user connects through a jump chain and then disconnects. Consider tying hop client lifetimes to the returned client (e.g., close them when the target client closes) or returning a wrapper that can dispose the hop chain.
Useful? React with 👍 / 👎.
|
@coderabbitai Why still blocking? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2862
However, the PR is still blocked because there are 6 unresolved review comment threads, keeping the review status at 📝 Unresolved Review ThreadsFrom Codex (chatgpt-codex-connector):
From CodeRabbit (me):
✅ To Unblock the PR:You need to either:
The Codex comments appear to be the primary blockers (especially the P1 priority ones). Would you like me to take a closer look at any specific thread to help you understand what changes are needed? |
Fixes #356
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.