Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
- PDF recognized as previewable in
files.open—openMobileFilegains anisRuntimePreviewablePdfPathbranch, ordered before the binary check, so a.pdfnow flows tohost.openFileand returnsopened: true. RuntimeFileOpenResult['kind']widened to include'pdf'— informational result field; no consumer does an exhaustive switch, and mobile's reply schema is a loose object, so old clients are unaffected.- New test — asserts the PDF reaches
openFileand returns{ kind: 'pdf', opened: true }; it would fail without the branch, so it pins the fix.
The mechanism is sound: the desktop renderer already renders PDFs off isImage: true + mimeType: 'application/pdf' (EditorEditFileSurface → ImageViewer → PdfViewer), and both the local readFileExplorerPreview and the relay mime map supply those fields for .pdf, so routing through the existing openFile path reaches the same viewer the file explorer uses. The leading-dot guard (dotIndex > 0) and case-insensitive match are correct, and a missing PDF still fails the RPC before any tab is created.
ℹ️ files.list / files.searchPaths still report PDFs as binary
The new kind: 'pdf' is only produced by files.open. The list and search methods still classify .pdf through isMobileBinaryPath, so the mobile file explorer row remains disabled (canPreviewMobileFileRow returns false for .pdf), meaning the mobile tap path described in the PR description is reachable for PDFs only from terminal path taps, not the explorer. Nothing blocking — flagging in case wiring the explorer was intended, since it would be a one-line follow-up.
Technical details
# PDF open lists as binary in list/search
## Affected sites
- `src/main/runtime/runtime-file-commands-constructor.ts:176-184` — `openMobileFile` now resolves `.pdf` to `'pdf'`.
- `src/main/runtime/runtime-file-commands-constructor.ts:61` and `:116`/`:157` — `listMobileFiles` / `searchMobileFilePaths` / `searchQuickOpenFilePaths` still use `isMobileBinaryPath`, so `.pdf` is reported `'binary'`.
- `mobile/src/files/mobile-file-preview-navigation.ts:33` — `canPreviewMobileFileRow` rejects non-previewable binaries, so a `.pdf` row is disabled.
## Required outcome
- Decide whether the mobile explorer should surface PDFs. If yes, align the list/search kind with `openMobileFile`; if no, the divergence is intentional and no change is needed.
## Open questions for the human
- Is opening PDFs from the mobile file explorer in scope, or is the CLI/terminal-tap path the only intended entry point?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 6d624bc29d2f4129a36692b9f3164891c7953a99 and 09b4752. 📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe runtime now classifies PDF paths separately and opens PDFs when desktop PDF opening is enabled. Capability negotiation preserves unopened binary results for clients that do not advertise support. Directory paths ending in Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to PDF opening and client handling appear consistent, while unsupported direct CLI opens now report errors. No actionable merge-blocking issue remains in the supplied evidence; normal checks should complete before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to PDF opening retains the existing file checks and preserves the previous response for clients without support. No introduced security vulnerability was established, but remote authentication and end-to-end viewer failure recovery were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, scope, behavior changes, regression tradeoffs, linked issue, and one automated test run. It does not follow the repository template because it omits the required ELI5, What Changed, Why, Visual Proof, Review, Agent skill upstream boundary, Notes, and Checklist sections. It also does not state the required visual-proof N/A rationale or broader test and platform coverage. Resolution Add the missing template sections. Include an ELI5 explanation, detailed user-facing and implementation changes, rationale and alternatives, visual proof or an exact N/A statement with a reason, testing platforms and results, AI disclosure if applicable, review notes, the Agent skill upstream boundary checkbox, security/cross-platform/remote/mobile/backwards-compatibility notes, and the required checklist items. Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 15 files. (1 skipped: 1 unsupported.)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7c95af37-7570-4ad7-9dd9-d3769c2359e8
📥 Commits
Reviewing files that changed from the base of the PR and between 94f6c53 and ad4128791e700b0c06f3181f6647fb4a3999ff42.
📒 Files selected for processing (3)
src/main/runtime/orca-runtime-files-mobile-explorer-reads.test.tssrc/main/runtime/runtime-file-commands-constructor.tssrc/shared/runtime-file-contracts.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
ad41287 to
6d624bc
Compare
Sync update (
|
There was a problem hiding this comment.
ℹ️ No critical issues — one mixed-version note inline.
Reviewed changes
- Mobile no longer re-reads a PDF tab the desktop already opened —
openMobileFileTapreturns early whenfiles.openreportskind: 'pdf', skippingscheduleOpenedWorktreeTabActivationand thefiles.readthat fails withbinary_file. fileTapOpenedSchemadeclares an optionalkind— so the tap can branch onkind: 'pdf'while a host that omits it still activates the tab.- Tests — a mobile test that fails without the early return, plus a schema read assertion for
kind.
The mobile fix is correct and well-scoped: the PDF branch on the host is reached only for worktree-contained taps, triggerOpenFeedback already fired before the early return, and the guard reads the same kind the host now publishes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
files.open treated .pdf as a non-previewable binary and reported success with opened:false. The editor already previews that mime.
A tap that gets opened:true was switching to the synced file tab, and that tab calls files.read, which rejects PDFs as binary_file. The desktop open stays. Co-authored-by: Cursor <cursoragent@cursor.com>
Direct `orca file open` kept ok:true when the runtime declined a binary, so callers treated that as success. A PDF reply that flips opened to true makes an already-installed mobile client activate the file tab and files.read it. Clients that advertise files.open.pdf-desktop.v1, and in-process callers, still open the desktop viewer. file diff and file open-changed keep their skipped-result output.
6d624bc to
a7d38c9
Compare
Sync update (
|
There was a problem hiding this comment.
ℹ️ No critical issues — one minor cleanup inline.
Reviewed changes (the increment since the prior pullfrog review, commit a7d38c9162)
- Gated the PDF desktop open behind
files.open.pdf-desktop.v1—supportsFilesPdfDesktopOpenreads the caller's advertised capabilities, andfiles.openpassespdfDesktopOpen: falsefor a negotiated client that does not advertise it, so the host returns the oldkind: 'binary', opened: falseanswer and opens no tab. This closes the mixed-version skew flagged in the prior review: an already-installed mobile client never seesopened: truefor a.pdf, and the gate is correctly scoped because a paired phone always carries a definedclientKindfromdevice.scope. openMobileFilegained an optionalpdfDesktopOpenparameter — defaults totrueso in-process callers keep the viewer, and returnskind: 'binary'when a PDF is declined.- CLI
file opennow fails on a declined open — a runtime result ofopened: falseis thrown asunsupported-type(exit 1,ok: falseunder--json), whilefile diffandfile open-changedkeep their skipped-row output. Note this is broader than the PR body's PDF-only framing: any declined open (zip/audio/video) now exits nonzero. - The capability is advertised by the CLI and mobile from
remoteRuntimeClientCapabilities, with a worked example added todocs/reference/remote-wire-compatibility.md. - Tests — capability function and dispatcher coverage, a host-level declined-PDF test, and two CLI decline tests.
Verified in this run: the touched unit files pass (136 tests), typecheck:node and typecheck:cli are clean, and the changed-code quality gate reports 0 findings.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
A directory named notes.pdf passed the existence check and the CLI reported success. The open now requires a file. The declined-open error uses the same RuntimeClientError the rest of the handler throws.
Sync update (
|
There was a problem hiding this comment.
Important
The new remote-directory test adds an unchecked as never cast on a changed line, which fails the required check:code-quality:changed CI step.
Reviewed changes
- Refuses to open a directory target, not just a missing one —
assertMobileOpenTargetExistsnow keeps thestatresult and throwsEISDIRwhenisDirectory()is true, so a.pdf(or text/markdown/image) path that names a directory no longer returnsopened: trueand creates a tab that cannot load it. - Consolidated the duplicate
RuntimeClientErrorimport —file.tsnow imports the class once from../runtime/types, retiring the prior review's inline nit. - Tests — new local and SSH tests for a directory whose name ends in
.pdf; the existing missing-fileENOENTtests are unchanged.
Verified in this run: the three touched test files pass (35 tests), typecheck:cli and typecheck:node are clean. The changed-code quality gate reports one new finding, anchored below.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The new as never is the only way to stub the SSH provider method this command reads, and the changed-line quality gate rejects it without a scoped reason. Co-authored-by: Cursor <cursoragent@cursor.com>
The changed-line gate reports the mockReturnValue call, so the exemption has to sit on that line. Co-authored-by: Cursor <cursoragent@cursor.com>
Sync update (
|
…Explorer (#24445) files.open gated the desktop open on the mobile preview list, so PDFs returned opened:false with ok:true. The host now opens every file after an existence/directory check; the CLI fails loudly if an older host still declines. Fixes #24328. Builds on #24331. Co-authored-by: Wooseong Kim <innocarpe@gmail.com>
|
Thanks @innocarpe for the fix and the clear diagnosis! We landed this in #24445, which builds on your approach (it opens every binary type, not just PDFs, and makes the CLI fail if an older app declines the file). You're credited as a co-author on the merge commit. |
2 participants
Description
orca file openon a PDF returnedok: truewithkind: "binary"andopened: false, and no tab opened. The same PDF opens in the desktop PDF viewer from the file explorer. Markdown in the same command opens a tab.Focused fix
files.opentreats a.pdfpath as previewable and calls the same rendereropenFilepath images already use, withkind: "pdf"andopened: true.opened: false. Diff opening is unchanged.Preserves
.PDFis included. A name that only contains.pdfin the middle is not.openFile.Evidence
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/runtime/orca-runtime-files-mobile-explorer-reads.test.tsUser-regression-tradeoffs
files.openfor a PDF now getsopened: trueand opens the desktop viewer tab, the same way an image opened through this RPC does. Zip files still decline.Fixes #24328