Add async SSH connection management, reconnect logic, and security hardening to terminal screen#1
Conversation
SSH接続管理: - _reconnectTabが全タブの共有クライアントを破壊する問題を修正 - _getClientにCompleter排他制御を追加（競合状態防止） - closedなSSHClientのisClosed判定を追加 - _showAddTabDialogで共有クライアントを再利用 接続ライフサイクル: - build内のFuture.microtask副作用をsession.doneコールバックに移動 - StreamSubscriptionを保持しdispose時にcancel - 再接続にリトライ上限(5回)+backoff導入、手動再試行ボタン表示 - _currentTabの空配列アクセスガード追加 - _reconnectingフラグをtry-finallyで保護 UI・セキュリティ: - セッション名のバリデーション（シェルインジェクション防止） - onPointerMoveスクロールに距離・方向しきい値追加 - タブ×ボタンのタップ領域拡大(32x32) - マジックナンバー/マジックストリングを定数化 - _closeTab内のNavigator.popをaddPostFrameCallbackに移動 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors terminal SSH handling: introduces asynchronous shared client lifecycle, per-tab SSH sessions with subscription-managed stdout/stderr, reconnect with capped backoff, session-name validation, pointer-based scroll detection, improved cleanup/UI feedback, adds CI workflow, coderabbit config, and multiple unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as TerminalScreen
participant Client as SharedSSHClient
participant Tab as Per-Tab Session
participant Stream as Stdout/Stderr
User->>Screen: Open tab / Connect
activate Screen
Screen->>Client: check/reuse or create client
alt create
Screen->>Client: establish connection (async)
Client-->>Screen: connection ready
end
Screen->>Tab: create session bound to client
Tab->>Stream: subscribe stdout/stderr
Stream-->>Tab: deliver output
Screen-->>User: display terminal
deactivate Screen
User->>Screen: Input / Resize
activate Screen
Screen->>Tab: route input / send resize
Tab->>Client: write to SSH session
Client->>Stream: emit stdout/stderr
Stream-->>Tab: update display
deactivate Screen
Note over Screen,Tab: On disconnect/error
Tab->>Screen: notify error
alt attempt reconnect (<=5)
Screen->>Screen: backoff + retry
Screen->>Client: recreate/check client
Screen->>Tab: recreate session & resubscribe
else give up
Screen->>Tab: cancel subscriptions & close session
Screen->>Screen: if no tabs -> exit screen
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
- GitHub Actions CI: analyze, test, build APK - CodeRabbit: 日本語レビュー、auto_review有効 - 43 unit tests: HostConfig serialization, session name validation, SSH service path prefix and tmux output parsing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/screens/terminal_screen.dart (2)
177-185:⚠️ Potential issue | 🔴 CriticalDon't reconnect tabs that were intentionally closed.
Line 200 and Line 284 close the session during normal teardown, but the
donehandler on Lines 177-185 treats every completion as a dropped connection and calls_reconnectTab(tab). Closing a tab can therefore recreate an orphaned SSH session for a tab that's already been removed from_tabs, along with new listeners attached to it. Add a per-tab closed flag or a_tabs.contains(tab)guard before updating state/reconnecting, and re-check it after the backoff delay.Also applies to: 200-210, 279-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/screens/terminal_screen.dart` around lines 177 - 185, The done handler for tab.session (the block that calls setState and _reconnectTab(tab)) must not treat intentional closes as dropped connections; add a per-tab closed flag (e.g., tab.closed) or check `_tabs.contains(tab)` before mutating state and before calling `_reconnectTab(tab)`, and set that flag when you intentionally close the session (the paths that call session.close()/closeSession in the teardown around the other handlers). Also re-check the same guard after the backoff delay inside the reconnect logic so you never recreate sessions or listeners for tabs that were intentionally removed.
59-60:⚠️ Potential issue | 🟠 MajorAvoid accessing
ScaffoldMessengerduring theinitStatelifecycle phase.Line 59 calls
_addTab(widget.sessionName)frominitState(). If the session name is invalid, lines 121-125 attempt to show a snackbar viaScaffoldMessenger.of(context)before the first build completes. This violates Flutter's widget lifecycle and can fail since inherited widgets may not be available during initialization.Defer the validation feedback using
addPostFrameCallback(), or return a validation result from_addTab()and display UI feedback from a post-build handler instead (as already shown in_closeTab()at line 208).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/screens/terminal_screen.dart` around lines 59 - 60, _initState currently calls _addTab(widget.sessionName) which may trigger ScaffoldMessenger.of(context) to show a SnackBar before the first build; defer any UI feedback by wrapping the call that shows the snackbar in WidgetsBinding.instance.addPostFrameCallback or change _addTab to return a validation result and then call ScaffoldMessenger.of(context).showSnackBar from a post-frame callback (similar to _closeTab) instead of inside _addTab during initState so inherited widgets are available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/screens/terminal_screen.dart`:
- Around line 113-114: _reconnectTab resets tab._retryCount unconditionally
because _connectTab currently swallows all exceptions; change _connectTab (or
its callers) so failures are observable: either have _connectTab return a
success boolean or rethrow the caught exception after logging, and then update
_reconnectTab to only reset tab._retryCount when the connect call succeeded (or
catch the rethrown error to increment retry and schedule another attempt).
Target the _connectTab method and the _reconnectTab code path that awaits it,
and ensure tab._retryCount is only reset on a confirmed successful connection.
- Around line 201-205: When removing a tab in the setState block that mutates
_tabs (the code that calls _tabs.removeAt(index) and adjusts _currentIndex),
account for removals to the left of the active tab by decrementing _currentIndex
when index < previous _currentIndex; otherwise the selected tab will shift
incorrectly. Update the logic around _tabs.removeAt(index) to: capture the
previous _currentIndex, remove the tab, if the removed index is less than the
previous index decrement _currentIndex by 1, then clamp _currentIndex to 0 when
_tabs is empty or to _tabs.length - 1 if it’s out of range.
---
Outside diff comments:
In `@lib/screens/terminal_screen.dart`:
- Around line 177-185: The done handler for tab.session (the block that calls
setState and _reconnectTab(tab)) must not treat intentional closes as dropped
connections; add a per-tab closed flag (e.g., tab.closed) or check
`_tabs.contains(tab)` before mutating state and before calling
`_reconnectTab(tab)`, and set that flag when you intentionally close the session
(the paths that call session.close()/closeSession in the teardown around the
other handlers). Also re-check the same guard after the backoff delay inside the
reconnect logic so you never recreate sessions or listeners for tabs that were
intentionally removed.
- Around line 59-60: _initState currently calls _addTab(widget.sessionName)
which may trigger ScaffoldMessenger.of(context) to show a SnackBar before the
first build; defer any UI feedback by wrapping the call that shows the snackbar
in WidgetsBinding.instance.addPostFrameCallback or change _addTab to return a
validation result and then call ScaffoldMessenger.of(context).showSnackBar from
a post-frame callback (similar to _closeTab) instead of inside _addTab during
initState so inherited widgets are available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36d79722-66bc-4573-80b1-6c42d22f1f81
📒 Files selected for processing (1)
lib/screens/terminal_screen.dart
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6m10cmUK
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/session_name_validation_test.dart (1)
3-5: Pattern duplication creates a sync risk.The regex is duplicated from
lib/screens/terminal_screen.dart:15. If the production pattern changes, this test would still pass with the old pattern.Consider exposing the pattern for testability, or at minimum, add a comment referencing the exact source location to ease future maintenance:
/// Mirrors _kSessionNamePattern from lib/screens/terminal_screen.dart:15 /// Keep in sync if the production pattern changes. final _kSessionNamePattern = RegExp(r'^[a-zA-Z0-9_-]+$');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/session_name_validation_test.dart` around lines 3 - 5, The test duplicates the production regex _kSessionNamePattern (from lib/screens/terminal_screen.dart) causing a sync risk; either export the constant from terminal_screen.dart (make _kSessionNamePattern public or provide a getter) and import it in test_session_name_validation_test.dart, or if you cannot change visibility, add a clear comment above the duplicated final _kSessionNamePattern in the test that references "Mirrors _kSessionNamePattern from lib/screens/terminal_screen.dart:15 — keep in sync if production pattern changes" so reviewers/maintainers can find and update the source pattern.test/ssh_service_test.dart (1)
12-29: Consider extracting parsing logic to a testable helper.The test duplicates the parsing logic from
listTmuxSessions(seelib/services/ssh_service.dart:17-29) rather than testing the actual method. While the comment explains the SSHClient dependency, this approach is fragile—if the production logic changes, these tests won't detect regressions.Consider extracting the parsing logic into a separate static method that can be tested directly:
// In SshService: static List<String> parseTmuxOutput(String output) { if (output.isEmpty || output.contains('no server running') || output.contains('not found')) { return []; } return output.split('\n') .where((s) => s.isNotEmpty) .where((l) => l.contains(':')) .map((l) => l.split(':').first.trim()) .toList(); }Then the test can call
SshService.parseTmuxOutput(output)directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ssh_service_test.dart` around lines 12 - 29, Extract the tmux-output parsing out of the test into a new static helper on SshService (e.g., SshService.parseTmuxOutput) and update the test to call that helper instead of duplicating logic; implement parseTmuxOutput to return [] for empty output or when it contains "no server running" or "not found", otherwise split on '\n', filter non-empty lines that contain ':', and map each line to the trimmed name before the ':' so listTmuxSessions can delegate to this helper and the test directly verifies SshService.parseTmuxOutput(output).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 24-27: The workflow uses the wrong GitHub Action name
"subosio/flutter-action" causing CI to fail; update the action reference in the
YAML to the correct repository "subosito/flutter-action" wherever
"subosio/flutter-action" appears (in the job step using
flutter-version-file/channel), preserving the existing inputs
(flutter-version-file and channel) so the step behavior remains unchanged.
---
Nitpick comments:
In `@test/session_name_validation_test.dart`:
- Around line 3-5: The test duplicates the production regex _kSessionNamePattern
(from lib/screens/terminal_screen.dart) causing a sync risk; either export the
constant from terminal_screen.dart (make _kSessionNamePattern public or provide
a getter) and import it in test_session_name_validation_test.dart, or if you
cannot change visibility, add a clear comment above the duplicated final
_kSessionNamePattern in the test that references "Mirrors _kSessionNamePattern
from lib/screens/terminal_screen.dart:15 — keep in sync if production pattern
changes" so reviewers/maintainers can find and update the source pattern.
In `@test/ssh_service_test.dart`:
- Around line 12-29: Extract the tmux-output parsing out of the test into a new
static helper on SshService (e.g., SshService.parseTmuxOutput) and update the
test to call that helper instead of duplicating logic; implement parseTmuxOutput
to return [] for empty output or when it contains "no server running" or "not
found", otherwise split on '\n', filter non-empty lines that contain ':', and
map each line to the trimmed name before the ':' so listTmuxSessions can
delegate to this helper and the test directly verifies
SshService.parseTmuxOutput(output).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b704454-d60d-4d31-b867-a9257df9e915
📒 Files selected for processing (5)
.coderabbit.yaml.github/workflows/ci.ymltest/host_config_test.darttest/session_name_validation_test.darttest/ssh_service_test.dart
✅ Files skipped from review due to trivial changes (1)
- .coderabbit.yaml
| - uses: subosio/flutter-action@v2 | ||
| with: | ||
| flutter-version-file: pubspec.yaml | ||
| channel: stable |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
subosito flutter-action GitHub official
💡 Result:
The official GitHub repository for subosito/flutter-action is:
https://github.com/subosito/flutter-action
[1]
Fix typo in Flutter action name — workflow will fail.
The action subosio/flutter-action is incorrect. The correct action is subosito/flutter-action (note: subosito, not subosio).
Proposed fix
- - uses: subosio/flutter-action@v2
+ - uses: subosito/flutter-action@v2
with:
flutter-version-file: pubspec.yaml
channel: stable📝 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.
| - uses: subosio/flutter-action@v2 | |
| with: | |
| flutter-version-file: pubspec.yaml | |
| channel: stable | |
| - uses: subosito/flutter-action@v2 | |
| with: | |
| flutter-version-file: pubspec.yaml | |
| channel: stable |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 24 - 27, The workflow uses the wrong
GitHub Action name "subosio/flutter-action" causing CI to fail; update the
action reference in the YAML to the correct repository "subosito/flutter-action"
wherever "subosio/flutter-action" appears (in the job step using
flutter-version-file/channel), preserving the existing inputs
(flutter-version-file and channel) so the step behavior remains unchanged.
Local xterm package has deprecation infos that shouldn't fail CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Only reset _retryCount when connection actually succeeded (tab.connected) - Adjust _currentIndex when closing a tab to the left of current Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Retry logic: max retries, backoff capping, reset on success only - Tab index: left/right/self close, sequential close, bounds check - Scroll: threshold, multi-event, reverse direction - Pointer: min distance, axis locking Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/terminal_logic_test.dart (1)
52-59: Pipeline warning: Dead code is intentional here, but consider refactoring for clarity.The CI flagged line 55 as dead code because the
if (connected)block can never execute whenconnected = false. While this correctly tests that the retry count is not reset on failure, the dead code warning may cause confusion.Consider simplifying the test to make the intent clearer:
Suggested refactor
test('接続失敗時はリトライカウントがリセットされない', () { retryCount = 3; - final connected = false; - if (connected) { - retryCount = 0; - } + // Simulate failed connection - retryCount should NOT be reset + // (no reset logic executed when connection fails) expect(retryCount, 3); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/terminal_logic_test.dart` around lines 52 - 59, The test '接続失敗時はリトライカウントがリセットされない' contains dead code inside the if (connected) block because connected is explicitly false; simplify by removing the unreachable if and directly asserting that retryCount remains 3 when connected is false: set retryCount = 3, set connected = false (or omit if obvious), then call expect(retryCount, 3) (or expect(retryCount, equals(3))) so the intent is clear; update the test body in test(...) accordingly and keep the test name and variables retryCount and connected for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/terminal_logic_test.dart`:
- Around line 52-59: The test '接続失敗時はリトライカウントがリセットされない' contains dead code
inside the if (connected) block because connected is explicitly false; simplify
by removing the unreachable if and directly asserting that retryCount remains 3
when connected is false: set retryCount = 3, set connected = false (or omit if
obvious), then call expect(retryCount, 3) (or expect(retryCount, equals(3))) so
the intent is clear; update the test body in test(...) accordingly and keep the
test name and variables retryCount and connected for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d235c0f2-e5ad-4175-a0e3-0c0c6977004f
📒 Files selected for processing (3)
.github/workflows/ci.ymllib/screens/terminal_screen.darttest/terminal_logic_test.dart
…age in CI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX
Tests
Chores