Repository navigation
Resolve TypeScript .ts files as text previews - #4924
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFilePreviewKindResolver defers resolution for certain known-text extensions (e.g., ChangesFile Type Ambiguity Handling for Text/Media Extensions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b4df44d. Configure here.
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the change is tightly scoped to file-preview kind resolution and the previously broken NUL-byte ordering in sniffLooksLikeText is now correct. All three issues raised in previous review rounds have been addressed: the NUL-byte guard now precedes the UTF-8 decode, the binary fixture uses NUL-padded MPEG-TS-sized packets that bypass the ASCII-range UTF-8 path, and the binary .ts initial-mode assertion is present. The new helpers are logically correct, the fallback chain is well-ordered (text wins over MPEG-TS detection), and the six new regression tests cover the full decision surface including sync-byte-like TypeScript content and M2TS 192-byte packets. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[URL with .ts extension] --> B{needsSniffBeforeTextOrMedia?}
B -- Yes --> C[initialResolution returns .needsSniff\ninitialMode returns .quickLook]
B -- No --> Z[existing UTType / knownTextFile path]
C --> D[resolvedResolution sniff block]
D --> E{sniffLooksLikeText?}
E -- UTF-16 BOM valid UTF-16 true --> F[.resolved .text]
E -- NUL bytes present false --> G{looksLikeMPEGTransportStream?}
E -- valid UTF-8 no NULs true --> F
G -- sync pattern found --> H[.resolved .media]
G -- no pattern or too small --> I{contentTypes mediaMode?}
I -- UTType maps to MPEG-TS --> H
I -- no media type --> J[.needsSniff returns .quickLook]
Reviews (7): Last reviewed commit: "Handle m2ts preview sniffing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try Data([0x47, 0x40, 0x00, 0x10, 0x00, 0x47, 0x41, 0x00, 0x10, 0x00]) | ||
| .write(to: url, options: .atomic) | ||
|
|
||
| XCTAssertEqual(FilePreviewKindResolver.mode(for: url), .media) |
There was a problem hiding this comment.
true, mode resolves to .text
Every byte in [0x47, 0x40, 0x00, 0x10, 0x00, 0x47, 0x41, 0x00, 0x10, 0x00] is in the 0x00–0x7F single-byte UTF-8 range, including the null bytes (0x00 encodes U+0000, a valid Unicode scalar). sniffLooksLikeText calls String(data: data, encoding: .utf8) first, and this returns a non-nil String for all-ASCII-range data. That causes an immediate return true, bypassing the data.contains(0) guard entirely. As a result resolvedResolution returns .resolved(.text) and the assertion XCTAssertEqual(… .media) fails. Including at least one byte ≥ 0x80 (e.g. 0xFF) would make the UTF-8 decode fail and let the null-byte or content-type path classify the file as media correctly.
There was a problem hiding this comment.
Fixed. The binary regression fixture now uses two MPEG-TS-sized packets with sync bytes and NULs, which catches the previous valid-UTF-8 false positive.
— Claude Code
| func testBinaryTransportStreamFileKeepsMediaPreview() throws { | ||
| let url = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent(UUID().uuidString) | ||
| .appendingPathExtension("ts") | ||
| defer { try? FileManager.default.removeItem(at: url) } | ||
|
|
||
| try Data([0x47, 0x40, 0x00, 0x10, 0x00, 0x47, 0x41, 0x00, 0x10, 0x00]) | ||
| .write(to: url, options: .atomic) | ||
|
|
||
| XCTAssertEqual(FilePreviewKindResolver.mode(for: url), .media) | ||
| } |
There was a problem hiding this comment.
initialMode for a binary .ts file is not tested
After this PR, initialResolution routes ALL .ts files through .needsSniff (because knownTextFileNeedsSniffBeforeMedia returns true for any .ts), so a binary MPEG-TS file now shows .quickLook initially rather than .media. A XCTAssertEqual(FilePreviewKindResolver.initialMode(for: url), .quickLook) assertion in the binary test would confirm this is intentional and guard against future regressions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed. The binary .ts regression now asserts the intentional initial Quick Look mode before resolved media mode.
— Claude Code
There was a problem hiding this comment.
Covered in the transport-stream tests: both binary .ts fixtures now assert initialMode is quickLook before resolving to media.
— Claude Code
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/FilePreviewReviewFeedbackTests.swift`:
- Around line 86-101: Add a regression test that writes a tiny ".ts" fixture
containing one or two NUL (\0) bytes and asserts that the sniffing rejects it
before the UTF-8 text path; specifically, add a new test (e.g.,
testTypeScriptWithNulBytesRejectsUtf8) in FilePreviewReviewFeedbackTests.swift
that builds a temporary .ts file with embedded \0 bytes, uses
FilePreviewKindResolver.initialMode(for: url) and
FilePreviewKindResolver.mode(for: url) to verify the sample is not treated as
printable text (expect quickLook or non-.text result), and clean up the temp
file in defer—this pins the boundary ensuring NUL-containing samples are
rejected prior to the UTF-8 text branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1bb997-9679-425f-a646-e61421190fb6
📒 Files selected for processing (2)
Sources/Panels/FilePreviewPanel.swiftcmuxTests/FilePreviewReviewFeedbackTests.swift
This branch was successfully deployed
1 participant
Summary
Testing
Issues
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Changes are isolated to file preview kind resolution with broad unit tests; wrong sniffing could mis-route rare
.tsor other ambiguous extensions but does not touch auth or persistence.Overview
File preview routing now defers choosing text vs media when an extension is ambiguous—especially
.ts, where macOS often classifies the file as MPEG transport stream instead of TypeScript.FilePreviewKindResolverforces a content sniff up front for those cases (initial mode stays Quick Look, then async resolution). After sniffing, text wins if the bytes look like text; otherwise it checks for real transport streams via0x47sync-byte patterns (188/192/204-byte packets) before falling back to media. Text sniffing is stricter: NUL bytes reject text, UTF-16 BOM is handled explicitly, and valid UTF-8 is required for the plain-UTF-8 path.Regression tests cover TypeScript
.ts(including UTF-8 BOM and sync-byte-like ASCII), extensionless ANSI/UTF-16 text, and binary/M2TS.tsstaying on media preview.Reviewed by Cursor Bugbot for commit 05f22dc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Ensures
.tsfiles open as TypeScript text instead of MPEG transport streams. Initial preview stays Quick Look; final resolves to text for source and to media for real transport streams..ts); text sniff rejects NUL bytes, allows ANSI, and accepts UTF‑16/UTF‑8 BOM; initial mode is Quick Look, then resolves to text if content is textual..tswith sync-like bytes still resolve to text; tests cover ANSI/extensionless text, UTF‑8 BOM, NUL‑byte.ts, plain textual.ts, and binary transport streams.Written for commit db19dd1. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests