Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds shared SFTP timeout helpers, extracts Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/core/utils/sftp_timeout.dart`:
- Around line 14-19: The timeout wrapper in sftp_timeout.dart only rethrows a
TimeoutException and logs it, but the browser-session open flow in the SFTP open
path can time out before a SftpClient exists, leaving the underlying open
operation without cleanup. Update the open logic that calls _client.sftp() so it
either avoids wrapping the session-open future in timeout or wires a
cancellation/close path into the timeout handling, and make sure the cleanup is
tied to the SftpClient/session open code path rather than only to later
file/session close callers.
In `@lib/data/model/sftp/status.dart`:
- Around line 31-33: The SftpStatus constructor is firing `worker.init()`
without handling its async failure path, so initialization errors can be lost
and the request may never complete. Update the constructor/`SftpWorker` setup so
the `init` call is observed and any failure is caught and routed through the
existing request/status error handling path, using the `SftpStatus` and
`SftpWorker.init` symbols to locate the affected flow.
- Around line 41-44: The SFTP status cleanup in dispose() is not idempotent, so
repeated calls can hit completer?.complete(true) more than once and throw.
Update the Status.dispose method to guard against double disposal by checking
whether the completer has already been completed or by clearing/marking the
state after the first completion, while still disposing the worker exactly once.
In `@lib/view/page/storage/sftp.dart`:
- Around line 929-939: The _safeLocalPathPart helper in Sftp page path
construction only blocks traversal tokens and the platform separator, but it
still lets platform-invalid or reserved filename values through. Update the
remotePath-to-local-path mapping in the same code path to sanitize all invalid
characters and reserved device names for local filesystems, not just "."/".."
and Pfs.seperator, so downloads can be safely written on Windows and other
platforms. Keep the fix centered around _safeLocalPathPart and the pathParts
fold that builds the local path.
- Around line 743-744: The confirmation text built in the SFTP delete flow no
longer warns that directories may be removed recursively when
Stores.setting.sftpRmrDir.fetch() is already enabled. Update the message
construction around useRmr and libL10n.askContinue in the delete path so it
always includes recursive-delete wording for directory deletions, even when the
checkbox is hidden, and keep the confirmation copy aligned with the rm -r / sudo
recursive delete behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cdfbb78f-8c57-45a7-a1e0-e6ba2afa4f6f
📒 Files selected for processing (9)
lib/core/utils/sftp_timeout.dartlib/data/model/sftp/req.dartlib/data/model/sftp/status.dartlib/data/model/sftp/worker.dartlib/data/provider/sftp.dartlib/view/page/storage/local.dartlib/view/page/storage/sftp.dartlib/view/page/storage/sftp_helpers.dartlib/view/page/storage/sftp_mission.dart
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 by CodeRabbit
-instead of the Unix epoch.