Repository navigation
feat(documents): open CSV and TSV files from the OS - #24756
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOS-open handling now accepts Markdown, CSV, and TSV documents. The main process parses and resolves supported paths, then delivers resolved documents to the renderer, which opens them in the floating workspace after workspace-session readiness. macOS, Linux, and Windows installer associations now include CSV and TSV. Tests cover path handling, delivery, platform associations, and end-to-end OS-open flows. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to CSV and TSV OS-open support is ready for normal CI and platform validation; no established issue currently blocks merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The added formats retain the existing file checks, bounded queue, and local workspace opening behavior. Table cells are displayed as text rather than executed. No introduced security weakness was demonstrated, but native installation behavior and some caller-identity and recovery paths remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 23 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
|
Addressed the localization scope warning in The docstring-coverage warning is advisory. AGENTS.md requires concise comments explaining non-obvious reasoning and prohibits explaining obvious code. I kept comments aligned with that rule. Scoped feature and localization regression checks pass (131 tests across 15 files), along with the full typecheck and changed-code-quality gate. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Packaging associations: macOS
fileAssociationsnow generates Markdown + CSV/TSV entries, allrole: Editor/rank: Alternate; LinuxmimeTypesaddtext/csvandtext/tab-separated-values; Windows NSIS generalizes toORCA_REGISTER_DOCUMENT_*with a newOrca.TabularProgID, staying purely additive (OpenWithProgids/SupportedTypesonly) with a symmetric uninstall.win.fileAssociationsremains absent, preserving the "never claim the default handler" invariant. - Main process:
os-opened-markdown-files.tsbecomesos-opened-documents.ts;isOsOpenedDocumentName()= Markdown ∪.csv/.tsv(case-insensitive);documentPathsFromArguments,OsOpenedDocumentState, andresolveOsOpenedDocumentsreplace their Markdown-named counterparts.resolveOsOpenedDocumentsre-validates through the same argument filter beforestat/authorizeExternalPath, so the authorization contract is unchanged. Buffer cap (32), dedupe, and restore semantics are preserved. IPC channel names and theMarkdownDocumenttype alias are retained for compatibility. - Renderer:
openDocumentInFloatingWorkspace(language already resolves.csv/.tsvto their table languages). The OS-open bridge now defers both listener attachment and the pending pull untilworkspaceSessionReady, closing a cold-start race where startup tab restoration erased an early-opened tab.workspaceSessionReadyflips duringreconnectPersistedTerminals, which runs afterhydrateEditorSession, so the gate outlasts editor hydration as intended. - Tests: unit coverage is split cleanly across arguments/resolution/delivery/wiring/documents, and the new
tests/e2e/os-opened-documents.spec.tsasserts real table cells / Markdown headings on cold and warm delivery, one tab per file, denial of an unsupported sibling, and hidden/unfocused windows.
Verification
- 44 main-process unit tests and 42 renderer/i18n tests pass on this head.
verify:localization-extraction,verify:localization-catalog, andverify:localization-runtime-catalogall exit 0.- No dangling references to any removed symbol remain; only the generated
config/scripts/ci-shard-timings.jsonstill names the renamed test files, with no functional effect.
The one non-actionable observation: verify:localization-runtime-catalog reports the now-orphaned auto.hooks.ipc.events.os.markdown.file.open.bridge.1e9a1a63c4 entry as "no longer required (harmless; sync to drop)" — running pnpm run sync:localization-runtime-catalog would prune it, but the check explicitly tolerates it.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…/sweep-23225-csv-os-open
|
Final merge review at Extend existing OS document associations and delivery to CSV/TSV, preserving restoration and authorization. Merge risk and retained limits: Low–moderate: OS file associations/startup queue; cold-start files wait for workspace restoration. Contributor authors and existing co-author credit will be preserved explicitly in the squash commit. |
ELI5
Choose Orca in your file manager’s Open With menu for a CSV or TSV export, and see it as a table in the floating workspace.
What Changed
Before, Orca only registered Markdown files and ignored CSV/TSV paths supplied by the OS. This adds CSV/TSV to the same local document queue and file-tab opener, which already selects the table viewer. macOS uses Alternate, Linux adds the existing CSV/TSV MIME types, and Windows uses the existing additive Open With registration and uninstall mechanism. None changes the default app.
The renderer now leaves requests in that queue until saved tabs finish restoring. A hidden cold-start check exposed that restoration could erase a tab opened too early; I rebuilt the untouched baseline and reproduced that with Markdown before changing the timing. The 32-file queue limit, dedupe, existing file authorization, and Markdown-only discovery remain in place.
Why
The viewer already exists. Extending the shared entry point keeps startup and already-running behavior together, with the same file validation and limits, instead of introducing another CSV launch path or a default-handler takeover.
Linked Issue
Fixes #23225
Visual Proof
macOS, disposable profiles and files, rebuilt app, hidden renderer screenshots throughout. These prove delivery and rendered content; they do not prove native Finder/Explorer installation registration.
Already-running CSV and TSV delivery:
Testing
Reproduce the acceptance checks with:
ORCA_BACKGROUND_LAUNCH=1 pnpm exec electron-vite build --mode e2e ORCA_BACKGROUND_LAUNCH=1 SKIP_BUILD=1 pnpm run test:e2e tests/e2e/os-opened-documents.spec.ts --workers=1All three Markdown/CSV/TSV cases pass on macOS with no project required. Each starts with real argv paths plus a file URL, then delivers warm
second-instanceandopen-fileevents through Electron’s handlers. Assertions check actual table cells or Markdown headings, one tab per file, denied reads for an unsupported sibling file, and every native window hidden and unfocused.Scoped regression checks cover Windows drive/UNC paths and file URLs, malformed and relative inputs, dedupe, queue overflow, missing files/directories, authorization, Markdown discovery, renderer bridge cleanup, and all three platforms’ additive association declarations. Current
pnpm tc, formatting, changed-code-quality and localization checks pass. The preserved feature source has passed hidden cold/warm acceptance for Markdown, CSV and TSV on the preceding independently reviewed head. After bringing in the existing main fix for stale package-list contract checks (#24785), the current head0bf39c52ed3005f325f75daf17534351e98b216dpasses 194 scoped tests across 17 files, full production typecheck, formatting, changed-code-quality and localization checks. All 27 feature paths, all 26 content blobs, and the full feature diff are byte-for-byte identical to that reviewed head; original commits remain in history. Required CI on this exact published head passes: repository guards and final verify are SUCCESS. The changed E2E specs and both package jobs also pass; there are no pending or failed check contexts. This does not prove native file-manager registration. The prior Windows job timed out in an unchanged missing-hook test before packaging; a source-equivalent named-main control passed that case, and the new-head Windows job passes without a timeout or workflow workaround.AI Disclosure
Review
Independent read-only review found no actionable issues on exact head
bac18bd849fcf8d935db82cca991fd3d7b25ba5fagainstadf447d958b2e7eb9c85b0e9b12f634683c4ecbf. The reviewer passed 73 feature tests on the preceding head, confirmed those source changes are identical, and independently passed 36 localization tests plus the catalog verifier on this head. The prior local regression suite passed 131 tests across 15 files. The current main-integration head preserves the exact reviewed patch (SHA-2566ab65d18ea3396d16fc25fc9c2d29966f785815bcd3555b149017ecdc8b0c8ce) against named main5706d144b71fa09e23caf95c4f558901a45b9843; this is verified source equivalence to the genuine review, not a new independent review claim.Automatic CodeRabbit review on the preceding
b1a8c0cb3f60head reported no actionable code findings. Its unrelated localization scope warning is addressed in this revision; the docstring coverage advisory is explained under the repository’s concise-comments rule. Pullfrog reviewed the precedingbac18bdfeature head, all 27 files and both commits, and found no new issues. Its optional leftover localization-key observation is explicitly harmless under the verifier. Current-head required CI passes; the bot reviews themselves remain tied to the explicit earlier heads above.Requested by @momokun7; this reuses the Markdown OS-open flow from #10138. I compared recent contributor PRs #19894 by @kbsali (CSV delimiters) and #21125 by @BearMett (editor restoration records): neither adds OS entry points. No contributor commits were taken over, so no co-author trailer is claimed.
Agent skill upstream boundary
Notes
Merge risk: document associations and startup delivery. Regression checks preserve the Windows daemon uninstall guard and existing handler registrations. This is local desktop behavior; remote execution and wire contracts are untouched, and files remain pinned to this machine even when another runtime is selected.
Negative user-facing tradeoff: OS files now wait for normal session restoration during cold start, so slow restoration can delay the tab. No new setting, default-app change, or file-access expansion is introduced.
Validation gaps: the optional standalone E2E typecheck encounters pre-existing conflicting
Window.__paneManagersdeclarations; productionpnpm tcand rendered E2E checks pass. Native Finder/Explorer registration, Windows installer execution, Linux desktop installation, and native file-manager actions were not exercised. Cross-platform path and declared packaging behavior are covered by automated contracts, not a native installation claim. CodeRabbit’s automatic review covers the preceding head; fresh current-head review is subject to the coordinator’s shared quota slot, with no autonomous retry requested.Checklist
0bf39c52ed3005f325f75daf17534351e98b216d; local checks pass)