Repository navigation
[codex] Handle binary and video file changes - #5385
Merged
Merged
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR propagates binary-file metadata through git parsing, desktop changeset/status handling, and diff rendering, while adding shared video file utilities, a video preview view, and binary/video-specific diff UI. ChangesBinary detection and video preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Ready to review this PR? Stage has broken it down into 9 individual chapters for you: 
Chapters generated by Stage for commit 4f4ffbb on Jun 29, 2026 5:38pm UTC. |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
marked this pull request as ready for review
June 29, 2026 04:52
Contributor
Greptile SummaryThis PR adds binary and video handling to the desktop changes views. The main changes are:
Confidence Score: 4/5The binary DiffPane path needs a fix before merging.
DiffPane binary item annotations and host-service untracked file reads.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/git/utils/git-helpers.ts | Adds binary numstat parsing and untracked binary detection, but reads large untracked files before the size guard. |
| apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts | Adds 8KB binary sniffing and image/video extension checks before UTF-8 line counting. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts | Skips diff queries for binary files and emits placeholder file items, but binary items drop existing thread annotations. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx | Adds a video renderer using a Blob object URL with cleanup on content changes or unmount. |
| apps/desktop/src/shared/file-types.ts | Adds shared extension parsing plus image, video, and MIME helpers. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx | Treats binary and video files as non-diffable and disables text-diff editing for them. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/trpc/router/git/utils/git-helpers.ts:324-337
**Large Files Load Before Guard**
An untracked extensionless binary or large text file reaches this `readFile` before the size check, so the host service can load the whole file into memory even though it will later skip line counting. With the existing concurrent untracked-file processing, several large files can create avoidable memory pressure or crash the process.
```suggestion
if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) {
return;
}
const buf = await readFile(fileReal);
const sniffEnd = Math.min(buf.length, 8192);
for (let i = 0; i < sniffEnd; i++) {
if (buf[i] === 0) {
file.isBinary = true;
file.additions = 0;
file.deletions = 0;
return;
}
}
```
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts:107-113
**Binary Items Drop Threads**
When a binary file has existing review comments, this branch replaces the item annotations with only the placeholder annotation and never merges the fetched thread annotations for that path. The DiffPane can then show the binary placeholder while hiding comments that still exist on the file.
Reviews (1): Last reviewed commit: "Keep binary placeholder annotation ancho..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
324
to
337
| const buf = await readFile(fileReal); | ||
| const sniffEnd = Math.min(buf.length, 8192); | ||
| for (let i = 0; i < sniffEnd; i++) { | ||
| if (buf[i] === 0) return; | ||
| if (buf[i] === 0) { | ||
| file.isBinary = true; | ||
| file.additions = 0; | ||
| file.deletions = 0; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
An untracked extensionless binary or large text file reaches this readFile before the size check, so the host service can load the whole file into memory even though it will later skip line counting. With the existing concurrent untracked-file processing, several large files can create avoidable memory pressure or crash the process.
Suggested change
| const buf = await readFile(fileReal); | |
| const sniffEnd = Math.min(buf.length, 8192); | |
| for (let i = 0; i < sniffEnd; i++) { | |
| if (buf[i] === 0) return; | |
| if (buf[i] === 0) { | |
| file.isBinary = true; | |
| file.additions = 0; | |
| file.deletions = 0; | |
| return; | |
| } | |
| } | |
| if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) { | |
| return; | |
| } | |
| if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) { | |
| return; | |
| } | |
| const buf = await readFile(fileReal); | |
| const sniffEnd = Math.min(buf.length, 8192); | |
| for (let i = 0; i < sniffEnd; i++) { | |
| if (buf[i] === 0) { | |
| file.isBinary = true; | |
| file.additions = 0; | |
| file.deletions = 0; | |
| return; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 324-337
Comment:
**Large Files Load Before Guard**
An untracked extensionless binary or large text file reaches this `readFile` before the size check, so the host service can load the whole file into memory even though it will later skip line counting. With the existing concurrent untracked-file processing, several large files can create avoidable memory pressure or crash the process.
```suggestion
if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) {
return;
}
const buf = await readFile(fileReal);
const sniffEnd = Math.min(buf.length, 8192);
for (let i = 0; i < sniffEnd; i++) {
if (buf[i] === 0) {
file.isBinary = true;
file.additions = 0;
file.deletions = 0;
return;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+107
to
+113
| }, | ||
| annotations: [ | ||
| { | ||
| lineNumber: 1, | ||
| metadata: { kind: "binary-placeholder" }, | ||
| }, | ||
| ], |
Contributor
There was a problem hiding this comment.
When a binary file has existing review comments, this branch replaces the item annotations with only the placeholder annotation and never merges the fetched thread annotations for that path. The DiffPane can then show the binary placeholder while hiding comments that still exist on the file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts
Line: 107-113
Comment:
**Binary Items Drop Threads**
When a binary file has existing review comments, this branch replaces the item annotations with only the placeholder annotation and never merges the fetched thread annotations for that path. The DiffPane can then show the binary placeholder while hiding comments that still exist on the file.
How can I resolve this? If you propose a fix, please make it concise.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!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 24 files
Architecture diagram
sequenceDiagram
participant Git as Git Service
participant Numstat as Numstat Parser
participant Worker as Git Worker
participant Sniffer as Binary Sniffer
participant Changeset as Changeset Hook
participant DiffPane as v2 DiffPane
participant FilePane as v2 FilePane
participant ChangesView as Changes View (v1)
Note over Git,ChangesView: Binary file detection and diff handling
%% Git numstat parsing - binary markers
Git->>Numstat: git diff --numstat
Numstat->>Numstat: Parse "- -" markers
Numstat-->>Git: isBinary: true for binary files
%% Untracked file binary detection
Worker->>Sniffer: applyUntrackedLineCount()
Sniffer->>Sniffer: Read first 8KB for null bytes
alt isImageFile() or isVideoFile() detected
Sniffer->>Worker: Mark isBinary: true, stats 0
else Null byte found in sample
Sniffer->>Worker: Mark isBinary: true, stats 0
else UTF-8 content
Sniffer->>Worker: Count lines normally
end
%% Changeset propagation
Worker->>Changeset: ChangedFile { isBinary, ... }
Changeset->>Changeset: Map to ChangesetFile { isBinary, ... }
%% v2 DiffPane - skip diff requests for binaries
Changeset->>DiffPane: files list with isBinary flags
DiffPane->>DiffPane: Filter out binary files from diffRequests
alt File is binary
DiffPane->>DiffPane: Create file-type CodeViewItem
DiffPane->>DiffPane: Add binary-placeholder annotation (line 1)
DiffPane->>DiffPane: Render BinaryDiffPlaceholder component
DiffPane-->>User: Show "Binary file hidden" + Open file button
else File is not binary
DiffPane->>DiffPane: Normal diff request and rendering
end
%% v2 DiffPane - scroll behavior
DiffPane->>DiffPane: useDiffCodeViewScroll()
alt targetItem.type is "diff"
DiffPane->>DiffPane: Scroll to exact line
else targetItem.type is "file" (binary)
DiffPane->>DiffPane: Skip line-level scroll
end
%% v2 FilePane - video preview
FilePane->>FilePane: ALL_VIEWS: videoView registered
FilePane->>FilePane: isPreviewableVideoFile() check
opt Video file matched
FilePane->>FilePane: readFile as bytes (documentKind: "bytes")
FilePane->>FilePane: Create Blob with getVideoMimeType()
FilePane->>FilePane: Revoke object URL on cleanup
FilePane-->>User: Render <video> with controls
end
%% v1 ChangesView - binary handling
ChangesView->>ChangesView: FileDiffSection
alt isBinary or isVideo
ChangesView->>ChangesView: Show unsupported diff placeholder
ChangesView->>ChangesView: Disable edit toggle
ChangesView->>ChangesView: Hide file stats
ChangesView->>ChangesView: Set constant placeholder height
ChangesView-->>User: "Binary file — cannot display diff"
else Large/generated diff
ChangesView->>ChangesView: Normal collapsed behavior
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
readFile pinned the whole file in memory (up to 1MB x 64 concurrent reads) just to count newlines. Read in fixed 64KB chunks through one reused buffer: sniff the first 8KB for NULs (git's binary heuristic), tally \n bytes as we go, and skip the LOC signal for files over the size budget without reading past the first chunk. Caps per-file memory at the chunk size regardless of file size. Adds filesystem-backed tests for line counting (trailing newline, CRLF, chunk-boundary), binary sniffing, media-extension detection, and the size-budget skip.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts (1)
94-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
entry.isBinarybefore theexceededLimitreturn.For image/video paths,
readAsBinaryis already true here, but large files return at Line 109 beforeentry.isBinaryis updated. That leavesSharedFileDocument.isBinarynull/stale for oversized media files even though this loader already knows they are binary.🤖 Prompt for 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. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts around lines 94 - 123, In fileDocumentStore.loadFileDocument, ensure binary media files are marked correctly even when the read is truncated by the size limit. Move the assignment to entry.isBinary so it happens immediately after readAsBinary is computed (before the result.exceededLimit early return), or otherwise set it in the exceededLimit branch, so oversized image/video entries do not keep a null or stale SharedFileDocument.isBinary value.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts (1)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the exclusives comment in sync with the registry.
videoViewis alsopriority: "exclusive", so the note is already misleading about which views short-circuit resolution.🤖 Prompt for 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. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts around lines 8 - 12, The registry comment in ALL_VIEWS is out of sync with the actual exclusive views. Update the note near ALL_VIEWS so it accurately reflects every view with priority: "exclusive", including videoView, and keep it aligned with any future changes to the registry entries.
🤖 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 `@apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts`:
- Around line 96-105: The worker’s binary detection in git-task-handlers.ts is
not aligned with the shared helper used by git-helpers.ts, since it only checks
isImageFile and isVideoFile after sniffing. Update the binary classification in
the relevant untracked-file handling logic to use the same early short-circuit
as the shared path, including isBinaryMediaFile(file.path), so file.isBinary,
additions, and deletions are set consistently before any UTF-8 line counting.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx:
- Around line 282-285: The binary placeholder message in DiffPane is currently
not selectable because the renderer disables text selection globally. Update the
status copy or its immediate wrapper in DiffPane to include explicit select-text
cursor-text classes so the “Binary file hidden” text can be copied by users. Use
the existing DiffPane component markup around the <p> element to apply the
selectable text styling without changing the surrounding layout.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts:
- Around line 100-106: Preserve the original diff line for binary-file review
threads in useDiffCodeViewItems while still re-anchoring the displayed
annotation to line 1. Update the binary-file handling in
DiffPane/useDiffCodeViewItems so the annotation passed to the UI keeps the
original line in metadata (or otherwise lets the focus logic compare against the
original thread line), because DiffPane’s focus/highlight behavior currently
relies on annotation.lineNumber matching data.focusLine. Ensure the existing
mapping around getAnnotationsForFile and the threadAnnotations rewrite does not
lose the source line when file.isBinary is true.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx:
- Around line 7-20: The VideoView component is reusing an old blob URL for one
render after switching files, so the previous video can flash briefly. Update
the object URL handling in VideoView/useEffect to track the URL together with a
stable source key from the current document (for example filePath or a
revision/id) and only render the video when that key matches the current
document. Ensure the state is cleared or invalidated immediately on source
change so objectUrl from the prior file cannot be displayed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts`:
- Around line 74-80: The video placeholder test currently relies on isBinary:
true, so it does not directly cover the isVideoFile(file.path) branch. Update
the getEstimatedFileDiffSectionHeight test case to leave the file non-binary and
assert the unsupported diff height through the video-extension path in
getEstimatedFileDiffSectionHeight, using the existing createFile helper with the
.mp4 path so the test fails if isVideoFile stops matching videos.
In `@packages/host-service/src/trpc/router/git/utils/git-helpers.ts`:
- Around line 423-427: Propagate the binary flag through rename handling in
git-helpers so renamed files keep their binary status end-to-end. Update
DetectedRename and the logic in getGitStatusSnapshot() to carry stats.isBinary
from the numstat entry when building rename entries, and make sure any re-added
rename snapshots preserve that flag instead of defaulting to a plain text
change. Reference the DetectedRename type, numstat.get(entry.path), and
getGitStatusSnapshot() when applying the fix.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts:
- Around line 94-123: In fileDocumentStore.loadFileDocument, ensure binary media
files are marked correctly even when the read is truncated by the size limit.
Move the assignment to entry.isBinary so it happens immediately after
readAsBinary is computed (before the result.exceededLimit early return), or
otherwise set it in the exceededLimit branch, so oversized image/video entries
do not keep a null or stale SharedFileDocument.isBinary value.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts:
- Around line 8-12: The registry comment in ALL_VIEWS is out of sync with the
actual exclusive views. Update the note near ALL_VIEWS so it accurately reflects
every view with priority: "exclusive", including videoView, and keep it aligned
with any future changes to the registry entries.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54c4c29a-8f2e-4381-99ef-7e3259061268
📒 Files selected for processing (24)
apps/desktop/src/lib/trpc/routers/changes/utils/apply-numstat.tsapps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.tsapps/desktop/src/lib/trpc/routers/changes/utils/parse-status.tsapps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.tsapps/desktop/src/shared/changes-types.tsapps/desktop/src/shared/file-types.test.tsapps/desktop/src/shared/file-types.tspackages/host-service/src/trpc/router/git/types.tspackages/host-service/src/trpc/router/git/utils/git-helpers.test.tspackages/host-service/src/trpc/router/git/utils/git-helpers.tspackages/host-service/src/trpc/router/git/utils/git-status.ts
Comment on lines
+423
to
+427
| const stats = numstat.get(entry.path) ?? { | ||
| additions: 0, | ||
| deletions: 0, | ||
| isBinary: false, | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate isBinary through detected renames.
stats.isBinary is introduced here, but DetectedRename still drops it, and getGitStatusSnapshot() later re-adds rename entries without the flag. A renamed untracked binary therefore comes back as a plain text change and can bypass the binary placeholder flow.
Proposed fix
diff --git a/packages/host-service/src/trpc/router/git/utils/git-helpers.ts b/packages/host-service/src/trpc/router/git/utils/git-helpers.ts
@@
export interface DetectedRename {
oldPath: string;
newPath: string;
status: "renamed";
additions: number;
deletions: number;
+ isBinary: boolean;
}
@@
result.push({
oldPath: entry.oldPath,
newPath: entry.path,
status: "renamed",
additions: stats.additions,
deletions: stats.deletions,
+ isBinary: stats.isBinary,
});
diff --git a/packages/host-service/src/trpc/router/git/utils/git-status.ts b/packages/host-service/src/trpc/router/git/utils/git-status.ts
@@
for (const rename of renames) {
mergedUnstaged.push({
path: rename.newPath,
oldPath: rename.oldPath,
status: rename.status,
additions: rename.additions,
deletions: rename.deletions,
+ isBinary: rename.isBinary,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stats = numstat.get(entry.path) ?? { | |
| additions: 0, | |
| deletions: 0, | |
| isBinary: false, | |
| }; | |
| export interface DetectedRename { | |
| oldPath: string; | |
| newPath: string; | |
| status: "renamed"; | |
| additions: number; | |
| deletions: number; | |
| isBinary: boolean; | |
| } |
Suggested change
| const stats = numstat.get(entry.path) ?? { | |
| additions: 0, | |
| deletions: 0, | |
| isBinary: false, | |
| }; | |
| result.push({ | |
| oldPath: entry.oldPath, | |
| newPath: entry.path, | |
| status: "renamed", | |
| additions: stats.additions, | |
| deletions: stats.deletions, | |
| isBinary: stats.isBinary, | |
| }); |
Suggested change
| const stats = numstat.get(entry.path) ?? { | |
| additions: 0, | |
| deletions: 0, | |
| isBinary: false, | |
| }; | |
| for (const rename of renames) { | |
| mergedUnstaged.push({ | |
| path: rename.newPath, | |
| oldPath: rename.oldPath, | |
| status: rename.status, | |
| additions: rename.additions, | |
| deletions: rename.deletions, | |
| isBinary: rename.isBinary, | |
| }); | |
| } |
🤖 Prompt for 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.
In `@packages/host-service/src/trpc/router/git/utils/git-helpers.ts` around lines
423 - 427, Propagate the binary flag through rename handling in git-helpers so
renamed files keep their binary status end-to-end. Update DetectedRename and the
logic in getGitStatusSnapshot() to carry stats.isBinary from the numstat entry
when building rename entries, and make sure any re-added rename snapshots
preserve that flag instead of defaulting to a plain text change. Reference the
DetectedRename type, numstat.get(entry.path), and getGitStatusSnapshot() when
applying the fix.
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
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
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Line 171: Keep usePaneRegistry bound to openFilePane for all non-tree file
opens, and reserve openFilePaneFromTreeClick only for the sidebar tree row
handler. Update the workspace page wiring in the _dashboard v2 workspace page so
TerminalPane and DiffPane still call the generic openFilePane path, while the
tree click callback uses the tree-specific wrapper with pin/reveal behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeb8ac19-b21b-469a-aaa3-00669f1d9323
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/shared/file-types.tspackages/shared/package.jsonpackages/shared/src/media-files.test.tspackages/shared/src/media-files.ts
✅ Files skipped from review due to trivial changes (3)
- packages/shared/package.json
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.ts
- packages/shared/src/media-files.ts
|
|
||
| const paneRegistry = usePaneRegistry({ | ||
| onOpenFile: openFilePane, | ||
| onOpenFile: openFilePaneFromTreeClick, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- useWorkspaceFileNavigation ---'
fd -p 'useWorkspaceFileNavigation.ts$' 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId' -x sed -n '1,260p'
echo
echo '--- usePaneRegistry ---'
fd -p 'usePaneRegistry.ts$' 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId' -x sed -n '1,260p'
echo
echo '--- onOpenFile usage in the v2 workspace subtree ---'
rg -n -C3 --type=ts --type=tsx '\bonOpenFile\b' 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId'Repository: superset-sh/superset
Length of output: 6233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate usePaneRegistry ---'
fd -a 'usePaneRegistry\.(ts|tsx)$' apps/desktop/src | sed -n '1,20p'
echo
echo '--- locate onOpenFile references in desktop renderer ---'
rg -n -C3 '\bonOpenFile\b' apps/desktop/src/renderer apps/desktop/src | sed -n '1,240p'
echo
echo '--- locate openFilePaneFromTreeClick references ---'
rg -n -C3 '\bopenFilePaneFromTreeClick\b' apps/desktop/src/renderer apps/desktop/src | sed -n '1,240p'Repository: superset-sh/superset
Length of output: 50376
Keep usePaneRegistry on openFilePane: onOpenFile also comes from TerminalPane and DiffPane, so wiring the registry to openFilePaneFromTreeClick leaks tree-click pin/reveal behavior into non-tree file opens. Use the tree-click wrapper only for the sidebar row handler.
🤖 Prompt for 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.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx
at line 171, Keep usePaneRegistry bound to openFilePane for all non-tree file
opens, and reserve openFilePaneFromTreeClick only for the sidebar tree row
handler. Update the workspace page wiring in the _dashboard v2 workspace page so
TerminalPane and DiffPane still call the generic openFilePane path, while the
tree click callback uses the tree-specific wrapper with pin/reveal behavior.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts (1)
15-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPopulate
sourceLinewhen building thread annotations.
DiffCommentThreadnow exposessourceLine, but thekind: "thread"payload created here still never sets it (Lines 112-128). As written, the new field is a no-op in this hook, so downstream diff logic still cannot consume source-side line metadata from these annotations.🤖 Prompt for 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. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts around lines 15 - 21, The thread annotation payload built in useDiffAnnotations is not populating the new sourceLine field, so downstream diff logic can’t use source-side line metadata. Update the thread annotation creation path in useDiffAnnotations, especially the kind: "thread" payload builder, to set sourceLine from the underlying DiffCommentThread data when available and ensure the resulting annotation includes it consistently.
🤖 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.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts:
- Around line 15-21: The thread annotation payload built in useDiffAnnotations
is not populating the new sourceLine field, so downstream diff logic can’t use
source-side line metadata. Update the thread annotation creation path in
useDiffAnnotations, especially the kind: "thread" payload builder, to set
sourceLine from the underlying DiffCommentThread data when available and ensure
the resulting annotation includes it consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33aa95bc-736d-4fc3-9515-7673ebcd37b3
📒 Files selected for processing (9)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.tspackages/host-service/src/trpc/router/git/utils/git-helpers.tspackages/host-service/src/trpc/router/git/utils/git-status.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts
- packages/host-service/src/trpc/router/git/utils/git-status.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx
- packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Contributor
There was a problem hiding this comment.
1 issue found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:171">
P2: `onOpenFile` is consumed by `TerminalPane` and `DiffPane` in addition to the sidebar tree. Passing `openFilePaneFromTreeClick` here leaks the "click-active-row-to-pin" behavior into those non-tree contexts. Consider keeping `openFilePane` for the registry's `onOpenFile` and restricting `openFilePaneFromTreeClick` to the sidebar row handler only.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| const paneRegistry = usePaneRegistry({ | ||
| onOpenFile: openFilePane, | ||
| onOpenFile: openFilePaneFromTreeClick, |
Contributor
There was a problem hiding this comment.
P2: onOpenFile is consumed by TerminalPane and DiffPane in addition to the sidebar tree. Passing openFilePaneFromTreeClick here leaks the "click-active-row-to-pin" behavior into those non-tree contexts. Consider keeping openFilePane for the registry's onOpenFile and restricting openFilePaneFromTreeClick to the sidebar row handler only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, line 171:
<comment>`onOpenFile` is consumed by `TerminalPane` and `DiffPane` in addition to the sidebar tree. Passing `openFilePaneFromTreeClick` here leaks the "click-active-row-to-pin" behavior into those non-tree contexts. Consider keeping `openFilePane` for the registry's `onOpenFile` and restricting `openFilePaneFromTreeClick` to the sidebar row handler only.</comment>
<file context>
@@ -169,7 +168,7 @@ function V2WorkspaceContent() {
const paneRegistry = usePaneRegistry({
- onOpenFile: openFilePane,
+ onOpenFile: openFilePaneFromTreeClick,
onRevealPath: revealPath,
launcher,
</file context>
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds explicit binary-file handling for the desktop Changes view so binary blobs are not loaded into the text diff renderer. Videos are treated as non-diffable in Changes and can now be previewed in the v2 FilePane using the same registry pattern as images.
Details
--numstatbinary markers throughChangedFile.isBinary.VideoViewregistered alongsideImageView.Validation
bun test apps/desktop/src/shared/file-types.test.ts apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.tsbun run typecheck --filter=@superset/desktopbun run lintSummary by cubic
Adds binary handling to Changes and the v2 DiffPane with a single placeholder that preserves review threads, and adds video previews to the v2 FilePane. Media detection is unified in
@superset/shared/media-files, and untracked files are streamed to detect binaries and count lines with constant memory.New Features
--numstatbinary markers intoChangedFile.isBinary(including renames); shares extension-based media detection via@superset/shared/media-files(raster images incl. TIFF, all video containers; SVG remains text).VideoView); includes helpers (isVideoFile,isPreviewableVideoFile,getVideoMimeType,getFileExtension) and reads videos as bytes.Bug Fixes
isBinarypropagates through host-servicenumstat, status snapshots, rename detection, desktop change lists, and parse-status; removes duplicate binary placeholder text; aligns Quick Open with the tree-click open flow for consistent reveal/pinning.Written for commit 4f4ffbb. Summary will update on new commits.
Summary by CodeRabbit