Fix copy_dir in storage.py to do proper remote -> remote copies#1431
Fix copy_dir in storage.py to do proper remote -> remote copies#1431
Conversation
📝 WalkthroughWalkthroughDependencies updated in both API and SDK packages to version 0.0.85. Remote-URI reconstruction logic added to the storage copy function. App layout restructured from grid to flex-based container with reorganized component hierarchy. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lab-sdk/src/lab/storage.py (1)
529-553: Add a regression test forcopy_dirwith protocol-lessfind()outputs.This path is easy to regress. A focused test for remote→remote copy where
find()returns keys without scheme would lock in the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lab-sdk/src/lab/storage.py` around lines 529 - 553, The copy_dir logic must handle src_fs.find returning protocol-less keys for remote→remote copies; add a regression test that exercises copy_dir (the function that uses src_fs.find, is_remote_path, and copy_file) where src_fs.find returns paths without a scheme and src_protocol is set (simulating an S3-like source) to ensure the code prefixes src_protocol correctly before computing rel_path and calling copy_file; mock or stub src_fs.find to return bare keys, call copy_dir with a remote dest, and assert that makedirs and copy_file are invoked with fully-qualified URIs (e.g., "s3://...") preserving directory structure.
🤖 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/App.tsx`:
- Around line 185-194: The IconButton used to toggle logs lacks an accessible
label; update the IconButton (the component instance that calls
setLogsDrawerOpen and reads logsDrawerOpen) to include an aria-label prop (e.g.,
aria-label={logsDrawerOpen ? "Close logs" : "Open logs"}) so screen readers can
identify the action while preserving the existing onClick and icon swap logic.
- Around line 133-156: The cast "as any" on setLogsDrawerOpen must be removed
and the prop typed consistently: update AppContentProps to declare
setLogsDrawerOpen as (open: boolean) => void (matching Sidebar's expected
signature), add a proper props interface to MainAppPanel (e.g.,
MainAppPanelProps with setLogsDrawerOpen: (open: boolean) => void) and annotate
MainAppPanel's component with that interface, and ensure Sidebar's prop type
remains (open: boolean) => void so both Sidebar and MainAppPanel accept the same
typed callback; then remove the two "as any" casts where setLogsDrawerOpen is
passed to Sidebar and MainAppPanel.
---
Nitpick comments:
In `@lab-sdk/src/lab/storage.py`:
- Around line 529-553: The copy_dir logic must handle src_fs.find returning
protocol-less keys for remote→remote copies; add a regression test that
exercises copy_dir (the function that uses src_fs.find, is_remote_path, and
copy_file) where src_fs.find returns paths without a scheme and src_protocol is
set (simulating an S3-like source) to ensure the code prefixes src_protocol
correctly before computing rel_path and calling copy_file; mock or stub
src_fs.find to return bare keys, call copy_dir with a remote dest, and assert
that makedirs and copy_file are invoked with fully-qualified URIs (e.g.,
"s3://...") preserving directory structure.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/pyproject.tomllab-sdk/pyproject.tomllab-sdk/src/lab/storage.pysrc/renderer/App.tsx
Summary by CodeRabbit
Bug Fixes
Improvements
Chores