Skip to content

Fix copy_dir in storage.py to do proper remote -> remote copies#1431

Merged
deep1401 merged 3 commits intomainfrom
fix/copy-dir
Mar 3, 2026
Merged

Fix copy_dir in storage.py to do proper remote -> remote copies#1431
deep1401 merged 3 commits intomainfrom
fix/copy-dir

Conversation

@deep1401
Copy link
Copy Markdown
Member

@deep1401 deep1401 commented Mar 3, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of remote file paths during copy operations.
  • Improvements

    • Redesigned application interface with updated layout system.
    • Reorganized footer area and logging interface for enhanced usability.
  • Chores

    • Updated dependencies to version 0.0.85.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 3, 2026

📝 Walkthrough

Walkthrough

Dependencies 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

Cohort / File(s) Summary
Dependency Updates
api/pyproject.toml, lab-sdk/pyproject.toml
Version bump from 0.0.84 to 0.0.85. Transformerlab dependency updated in API, project version incremented in SDK.
Storage URI Logic
lab-sdk/src/lab/storage.py
Added remote-URI reconstruction in copy_dir function to determine source protocol and reconstruct full URIs for remote paths, ensuring correct destination path resolution during file copying operations.
App Layout Restructuring
src/renderer/App.tsx
Converted main app container from grid to flex-based layout. Reorganized Header, Sidebar, and MainAppPanel components within new content wrapper. Refactored footer and log drawer UI with inlined InsecurePasswordBanner and restructured OutputTerminal rendering.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix copy_dir in storage.py to do proper remote -> remote copies' is specific and clearly describes the main change in the pull request, focusing on the storage.py fix which is the primary functional change across the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/copy-dir

Comment @coderabbitai help to get the list of available commands and usage tips.

@sentry
Copy link
Copy Markdown

sentry bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
lab-sdk/src/lab/storage.py (1)

529-553: Add a regression test for copy_dir with protocol-less find() 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

📥 Commits

Reviewing files that changed from the base of the PR and between 448e528 and 642c2cb.

📒 Files selected for processing (4)
  • api/pyproject.toml
  • lab-sdk/pyproject.toml
  • lab-sdk/src/lab/storage.py
  • src/renderer/App.tsx

@deep1401 deep1401 merged commit 73070cf into main Mar 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants