Skip to content

chore: Revert "feat: CLI: Native DuckDB file loading (#409)" - #470

Merged
ilyabo merged 2 commits into
mainfrom
revert-cli-native-file-upload
Mar 16, 2026
Merged

ilyabo merged 2 commits into
mainfrom
revert-cli-native-file-upload

Conversation

@ilyabo

@ilyabo ilyabo commented Mar 15, 2026 •

Copy link
Copy Markdown
Collaborator

This reverts commit bc32e25 which doesn't work properly.

Summary by CodeRabbit

  • New Features

    • Dragged files are now uploaded to the server and referenced by server path instead of using local file references.
  • Documentation

    • Clarified drag-and-drop behavior and documented the upload endpoint and persistence behavior in the README.

This reverts commit bc32e25.

Signed-off-by: Ilya Boyandin <[email protected]>
@ilyabo
ilyabo marked this pull request as ready for review March 15, 2026 16:59
@coderabbitai

coderabbitai Bot commented Mar 15, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c248e81f-d2bf-4d9e-9c13-9228f6998095

📥 Commits

Reviewing files that changed from the base of the PR and between ce3837f and 5295814.

📒 Files selected for processing (1)
  • apps/sqlrooms-cli-ui/src/serverApi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/sqlrooms-cli-ui/src/serverApi.ts

📝 Walkthrough

Walkthrough

The changes add server-side file upload to the CLI UI: a new API helper uploads files via FormData to /api/upload, validates returned paths, and the file-loading workflow now uploads dropped files and uses the server-returned path when invoking existing loaders.

Changes

Cohort / File(s) Summary
Server API Integration
apps/sqlrooms-cli-ui/src/serverApi.ts
Add uploadFileToServer(file, config), getApiBaseUrl(config), and validateServerPath() to normalize base URL, upload files to /api/upload, parse/validate response paths, and surface errors.
File Loading Workflow
apps/sqlrooms-cli-ui/src/store.ts
Modify loadFile() to call uploadFileToServer() for File inputs, create a renamed File using the server path, and pass it to the existing loader flow.
Documentation
python/sqlrooms-cli/README.md
Update drag-and-drop and data persistence text to document uploads to /api/upload and that dropped files are uploaded and referenced by server path.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant UI as "CLI UI (Browser)"
    participant API as "Server API (/api/upload)"
    participant FS as "Server FS / uploads"
    UI->>API: POST /api/upload (FormData with file)
    API->>FS: store file, generate path
    FS-->>API: return stored path
    API-->>UI: 200 { "path": "/sqlrooms_uploads/..." }
    UI->>UI: validateServerPath() && construct File(name=path)
    UI->>Loader: invoke existing loader with server-referenced File
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰📦 I hopped a file into FormData's nest,
Through POST and path it found its rest,
A tiny server trail to guide the way,
Now uploads dance where droppings used to play,
Hooray for hops that save the day! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.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 accurately describes the PR as a revert of a previous feature (PR #409) that introduced native DuckDB file loading, which aligns with the actual changes removing file upload functionality.

✏️ 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 revert-cli-native-file-upload
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/sqlrooms-cli-ui/src/serverApi.ts`:
- Around line 159-160: The uploaded-path response is trusted via a type
assertion on data.path; validate and sanitize it before returning to prevent SQL
injection when WebSocketDuckDbConnector later interpolates filePath into "SELECT
* FROM read_ipc('${filePath}')". In serverApi.ts, after const data = (await
res.json()) as {path: string}; verify typeof data.path === 'string', normalize
and reject or sanitize any characters outside an allowlist (for example enforce
a regex like /^[A-Za-z0-9_\-./]+$/), and either throw/return an error for
invalid values or return an encoded/escaped safe path (e.g., percent-encode or
remove/escape single quotes) so downstream code using filePath cannot break the
SQL string. Ensure you update callers to handle the validation error
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 282fdfd4-651f-4037-83d0-a78eea6bc6a3

📥 Commits

Reviewing files that changed from the base of the PR and between adbfec4 and ce3837f.

📒 Files selected for processing (3)
  • apps/sqlrooms-cli-ui/src/serverApi.ts
  • apps/sqlrooms-cli-ui/src/store.ts
  • python/sqlrooms-cli/README.md

Comment thread apps/sqlrooms-cli-ui/src/serverApi.ts Outdated
Signed-off-by: Ilya Boyandin <[email protected]>
@ilyabo
ilyabo merged commit c0de1db into main Mar 16, 2026
6 checks passed
@ilyabo
ilyabo deleted the revert-cli-native-file-upload branch March 16, 2026 09:41
dmitriy-kostianetskiy pushed a commit that referenced this pull request Mar 17, 2026
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.

1 participant