diff --git a/apps/desktop/src/lib/trpc/routers/changes/utils/apply-numstat.ts b/apps/desktop/src/lib/trpc/routers/changes/utils/apply-numstat.ts index 5c6b6c81334..9894c4a1546 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/utils/apply-numstat.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/utils/apply-numstat.ts @@ -18,6 +18,7 @@ export async function applyNumstatToFiles( if (fileStat) { file.additions = fileStat.additions; file.deletions = fileStat.deletions; + file.isBinary = fileStat.isBinary; } } } catch {} diff --git a/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts b/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts index e9a481ca4cc..07b3285e20c 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts @@ -98,8 +98,16 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); - expect(stats.get("src/file1.ts")).toEqual({ additions: 10, deletions: 5 }); - expect(stats.get("src/file2.ts")).toEqual({ additions: 20, deletions: 3 }); + expect(stats.get("src/file1.ts")).toEqual({ + additions: 10, + deletions: 5, + isBinary: false, + }); + expect(stats.get("src/file2.ts")).toEqual({ + additions: 20, + deletions: 3, + isBinary: false, + }); }); test("handles binary files with dash markers", () => { @@ -108,8 +116,16 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); - expect(stats.get("image.png")).toEqual({ additions: 0, deletions: 0 }); - expect(stats.get("src/code.ts")).toEqual({ additions: 10, deletions: 5 }); + expect(stats.get("image.png")).toEqual({ + additions: 0, + deletions: 0, + isBinary: true, + }); + expect(stats.get("src/code.ts")).toEqual({ + additions: 10, + deletions: 5, + isBinary: false, + }); }); test("handles renamed files with arrow format", () => { @@ -118,8 +134,16 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); // Should be accessible by both old and new paths - expect(stats.get("new/path.ts")).toEqual({ additions: 5, deletions: 2 }); - expect(stats.get("old/path.ts")).toEqual({ additions: 5, deletions: 2 }); + expect(stats.get("new/path.ts")).toEqual({ + additions: 5, + deletions: 2, + isBinary: false, + }); + expect(stats.get("old/path.ts")).toEqual({ + additions: 5, + deletions: 2, + isBinary: false, + }); }); test("handles copied files with arrow format", () => { @@ -127,8 +151,16 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); - expect(stats.get("copy.ts")).toEqual({ additions: 0, deletions: 0 }); - expect(stats.get("source.ts")).toEqual({ additions: 0, deletions: 0 }); + expect(stats.get("copy.ts")).toEqual({ + additions: 0, + deletions: 0, + isBinary: false, + }); + expect(stats.get("source.ts")).toEqual({ + additions: 0, + deletions: 0, + isBinary: false, + }); }); test("handles paths with spaces in rename format", () => { @@ -136,8 +168,16 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); - expect(stats.get("new file.ts")).toEqual({ additions: 3, deletions: 1 }); - expect(stats.get("old file.ts")).toEqual({ additions: 3, deletions: 1 }); + expect(stats.get("new file.ts")).toEqual({ + additions: 3, + deletions: 1, + isBinary: false, + }); + expect(stats.get("old file.ts")).toEqual({ + additions: 3, + deletions: 1, + isBinary: false, + }); }); test("returns empty map for empty input", () => { @@ -153,7 +193,11 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); expect(stats.size).toBe(1); - expect(stats.get("valid/path.ts")).toEqual({ additions: 20, deletions: 3 }); + expect(stats.get("valid/path.ts")).toEqual({ + additions: 20, + deletions: 3, + isBinary: false, + }); }); test("handles non-numeric additions/deletions gracefully", () => { @@ -161,7 +205,11 @@ describe("parseDiffNumstat", () => { const stats = parseDiffNumstat(numstatOutput); - expect(stats.get("file.ts")).toEqual({ additions: 0, deletions: 0 }); + expect(stats.get("file.ts")).toEqual({ + additions: 0, + deletions: 0, + isBinary: false, + }); }); }); diff --git a/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.ts b/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.ts index 598f6676252..cd643188900 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.ts @@ -118,8 +118,11 @@ export function parseGitLog(logOutput: string): CommitInfo[] { export function parseDiffNumstat( numstatOutput: string, -): Map { - const stats = new Map(); +): Map { + const stats = new Map< + string, + { additions: number; deletions: number; isBinary: boolean } + >(); for (const line of numstatOutput.trim().split("\n")) { if (!line.trim()) continue; @@ -132,7 +135,11 @@ export function parseDiffNumstat( const additions = addStr === "-" ? 0 : Number.parseInt(addStr, 10) || 0; const deletions = delStr === "-" ? 0 : Number.parseInt(delStr, 10) || 0; - const statEntry = { additions, deletions }; + const statEntry = { + additions, + deletions, + isBinary: addStr === "-" && delStr === "-", + }; const renameMatch = rawPath.match(/^(.+) => (.+)$/); if (renameMatch) { diff --git a/apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts b/apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts index 1b8ca618cd8..08a62aba7a6 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/workers/git-task-handlers.ts @@ -1,5 +1,9 @@ -import { readFile, realpath, stat } from "node:fs/promises"; +import { open, realpath, stat } from "node:fs/promises"; import { isAbsolute, relative, resolve, sep } from "node:path"; +import { + BINARY_SNIFF_BYTES, + isBinaryMediaFile, +} from "@superset/shared/media-files"; import type { ChangedFile, GitChangesStatus } from "shared/changes-types"; import type { SimpleGit, StatusResult } from "simple-git"; import { getStatusNoLock } from "../../workspaces/utils/git"; @@ -30,6 +34,9 @@ interface TrackingStatus { } const MAX_LINE_COUNT_SIZE = 1 * 1024 * 1024; +// Chunk size for streaming untracked files when counting lines, so a file is +// never fully held in memory. Comfortably covers the binary sniff window. +const LINE_COUNT_CHUNK_SIZE = 64 * 1024; const WORKER_DEBUG = process.env.SUPERSET_WORKER_DEBUG === "1"; function logWorkerWarning(message: string, error: unknown): void { @@ -89,17 +96,63 @@ async function applyUntrackedLineCount( if (!isPathWithinWorktree(worktreeReal, fileReal)) continue; const stats = await stat(fileReal); - if (!stats.isFile() || stats.size > MAX_LINE_COUNT_SIZE) continue; - - const content = await readFile(fileReal, "utf-8"); - const lineCount = - content === "" - ? 0 - : content.endsWith("\n") - ? content.split(/\r?\n/).length - 1 - : content.split(/\r?\n/).length; - file.additions = lineCount; - file.deletions = 0; + if (!stats.isFile()) continue; + + if (isBinaryMediaFile(file.path)) { + file.isBinary = true; + file.additions = 0; + file.deletions = 0; + continue; + } + + // Stream in fixed chunks rather than slurping: readFile would pin + // the whole file in memory and, decoded as utf-8, would turn binary + // into U+FFFDs and report a bogus line count. Reuse one buffer, + // sniff the first 8KB for NULs (git's binary heuristic), and tally + // newlines as we go. + const handle = await open(fileReal, "r"); + try { + const buf = Buffer.allocUnsafe(LINE_COUNT_CHUNK_SIZE); + let { bytesRead } = await handle.read(buf, 0, buf.length, 0); + + const sniffEnd = Math.min(bytesRead, BINARY_SNIFF_BYTES); + let isBinary = false; + for (let i = 0; i < sniffEnd; i++) { + if (buf[i] === 0) { + isBinary = true; + break; + } + } + if (isBinary) { + file.isBinary = true; + file.additions = 0; + file.deletions = 0; + continue; + } + + // Over the budget: skip the LOC signal without reading the rest. + if (stats.size > MAX_LINE_COUNT_SIZE) continue; + + let newlines = 0; + let lastByte = -1; + let offset = 0; + while (bytesRead > 0) { + for (let i = 0; i < bytesRead; i++) { + if (buf[i] === 0x0a) newlines++; + } + lastByte = buf[bytesRead - 1] ?? lastByte; + offset += bytesRead; + ({ bytesRead } = await handle.read(buf, 0, buf.length, offset)); + } + // Match `content.split(/\r?\n/)`: a trailing newline doesn't add + // a line, but a final non-empty line without one does. (\r\n + // shares the \n, so counting \n bytes is equivalent.) + file.additions = + lastByte === -1 ? 0 : lastByte === 0x0a ? newlines : newlines + 1; + file.deletions = 0; + } finally { + await handle.close(); + } } catch (error) { logWorkerDebug( `failed untracked line count for "${file.path}" in "${worktreePath}"`, diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/types.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/types.ts index 69200b2e0a6..42453c90264 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/types.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/types.ts @@ -17,5 +17,6 @@ export interface ChangesetFile { status: FileStatus; additions: number; deletions: number; + isBinary?: boolean; source: DiffFileSource; } diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.ts index d2636b5da56..cbf54f85c5f 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.ts @@ -43,6 +43,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "commit", commitHash: ref.commitHash, @@ -62,6 +63,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "unstaged" }, })), ...status.staged.map((file) => ({ @@ -70,6 +72,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "staged" }, })), ]; @@ -86,6 +89,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "unstaged" }, }); } @@ -97,6 +101,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "staged" }, }); } @@ -108,6 +113,7 @@ export function useChangeset({ status: file.status as FileStatus, additions: file.additions, deletions: file.deletions, + isBinary: file.isBinary, source: { kind: "against-base", baseBranch: ref.baseBranch }, }); } diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx index c879b25c199..e52005f9458 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx @@ -5,7 +5,9 @@ import type { } from "@pierre/diffs"; import { CodeView, type CodeViewHandle } from "@pierre/diffs/react"; import type { RendererContext } from "@superset/panes"; +import { Button } from "@superset/ui/button"; import { useCallback, useMemo, useRef } from "react"; +import { LuFileCode } from "react-icons/lu"; import type { DiffPaneData, PaneViewerData } from "../../../../types"; import { type ChangesetFile, useChangeset } from "../../../useChangeset"; import { useOpenInExternalEditor } from "../../../useOpenInExternalEditor"; @@ -181,9 +183,15 @@ export function DiffPane({ | DiffLineAnnotation, item: CodeViewItem, ) => { - if (item.type !== "diff") return null; const m = annotation.metadata; + if (m.kind === "binary-placeholder") { + if (item.type !== "file") return null; + const file = fileByItemId.get(item.id); + if (!file) return null; + return ; + } if (m.kind === "composer") { + if (item.type !== "diff") return null; return ( ); } + if (m.kind !== "thread") return null; const annotationSide = "side" in annotation ? annotation.side : undefined; + const focusLine = m.sourceLine ?? annotation.lineNumber; const focused = item.id === targetItemId && data.focusLine != null && - annotation.lineNumber === data.focusLine && + focusLine === data.focusLine && (data.focusSide == null || annotationSide === data.focusSide); return ( @@ -221,6 +231,8 @@ export function DiffPane({ data.focusTick, clearComposer, submitComposer, + fileByItemId, + onOpenFile, ], ); @@ -257,3 +269,29 @@ export function DiffPane({ /> ); } + +function BinaryDiffPlaceholder({ + file, + onOpenFile, +}: { + file: ChangesetFile; + onOpenFile: (path: string, openInNewTab?: boolean) => void; +}) { + const canOpen = file.status !== "deleted"; + + return ( +
+ +

Binary file hidden

+ {canOpen ? ( + + ) : null} +
+ ); +} diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts index 5667db9021b..5c43c605324 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffAnnotations/useDiffAnnotations.ts @@ -18,6 +18,7 @@ export interface DiffCommentThread { isResolved: boolean; isOutdated: boolean; url?: string; + sourceLine?: number; } /** Local-only metadata for a draft composer pinned to the end of a selection. */ @@ -31,7 +32,8 @@ export interface DiffAgentComposer { export type DiffAnnotationMetadata = | ({ kind: "thread" } & DiffCommentThread) - | ({ kind: "composer" } & DiffAgentComposer); + | ({ kind: "composer" } & DiffAgentComposer) + | { kind: "binary-placeholder" }; interface UseDiffAnnotationsByPathOptions { workspaceId: string; diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts index 58c76099796..6cf729a8aed 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts @@ -1,6 +1,7 @@ import { type CodeViewItem, type DiffLineAnnotation, + type LineAnnotation, parseDiffFromFile, } from "@pierre/diffs"; import type { AppRouter } from "@superset/host-service"; @@ -49,10 +50,12 @@ export function useDiffCodeViewItems({ const diffRequests = useMemo( () => - files.map((file) => ({ - file, - input: createGetDiffInput(workspaceId, file), - })), + files + .filter((file) => !file.isBinary) + .map((file) => ({ + file, + input: createGetDiffInput(workspaceId, file), + })), [files, workspaceId], ); @@ -84,14 +87,74 @@ export function useDiffCodeViewItems({ const items = useMemo[]>(() => { const nextItems: CodeViewItem[] = []; + const queryByItemId = new Map( + diffRequests.map((request, index) => [ + getDiffItemId(request.file), + diffQueries[index], + ]), + ); - for (let index = 0; index < diffRequests.length; index++) { - const request = diffRequests[index]; - const query = diffQueries[index]; - if (!request || !query?.data) continue; - - const { file } = request; + for (const file of files) { const itemId = getDiffItemId(file); + const collapsed = collapsedSet.has(file.path); + + if (file.isBinary) { + // The placeholder item only has a single line, so re-anchor any + // existing review threads onto line 1 — otherwise they'd point at + // diff lines that don't exist here and silently disappear. + const threadAnnotations = ( + getAnnotationsForFile(annotationsByPath, file) ?? [] + ).map((annotation): LineAnnotation => { + const metadata = annotation.metadata; + if (metadata.kind === "thread") { + return { + lineNumber: 1, + metadata: { + ...metadata, + sourceLine: annotation.lineNumber, + }, + }; + } + if (metadata.kind === "composer") { + return { lineNumber: 1, metadata }; + } + return { lineNumber: 1, metadata }; + }); + const annotations: LineAnnotation[] = [ + { + lineNumber: 1, + metadata: { kind: "binary-placeholder" }, + }, + ...threadAnnotations, + ]; + nextItems.push({ + id: itemId, + type: "file", + file: { + name: file.path, + contents: " ", + }, + annotations, + collapsed, + version: hashString( + [ + file.path, + file.oldPath ?? "", + file.status, + file.additions, + file.deletions, + "binary", + collapsed ? "1" : "0", + getAnnotationsVersion(annotations), + ].join("\0"), + ), + }); + continue; + } + + const query = queryByItemId.get(itemId); + if (!query?.data) continue; + const baseAnnotations = getAnnotationsForFile(annotationsByPath, file); const extra = extraAnnotationsByItemId?.get(itemId); const annotations = @@ -108,7 +171,6 @@ export function useDiffCodeViewItems({ name: file.path, }, ); - const collapsed = collapsedSet.has(file.path); const version = hashString( [ query.dataUpdatedAt, @@ -134,6 +196,7 @@ export function useDiffCodeViewItems({ return nextItems; }, [ + files, diffRequests, diffQueries, annotationsByPath, @@ -207,16 +270,22 @@ function getAnnotationsForFile( } function getAnnotationsVersion( - annotations: DiffLineAnnotation[] | undefined, + annotations: + | ( + | DiffLineAnnotation + | LineAnnotation + )[] + | undefined, ): string { if (!annotations?.length) return ""; return annotations .map((annotation) => { const m = annotation.metadata; + const side = "side" in annotation ? annotation.side : "file"; if (m.kind === "composer") { return [ "c", - annotation.side, + side, annotation.lineNumber, m.startLine, m.endLine, @@ -224,9 +293,10 @@ function getAnnotationsVersion( m.endSide, ].join(","); } + if (m.kind !== "thread") return "local"; return [ "t", - annotation.side, + side, annotation.lineNumber, m.threadId, m.isResolved ? "1" : "0", diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.ts index 104e099c21e..dd64a77f981 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.ts @@ -56,8 +56,9 @@ export function useDiffCodeViewScroll({ ].join(":"); if (lastScrollTargetRef.current === scrollKey) return; + const targetItem = itemById.get(targetItemId); const target: CodeViewScrollTarget = - data.focusLine != null + data.focusLine != null && targetItem?.type === "diff" ? { type: "line", id: targetItemId, diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts index ed0e9f4e8df..4ed0277e913 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.ts @@ -3,11 +3,13 @@ import { binaryWarningView } from "./views/BinaryWarningView"; import { codeView } from "./views/CodeView"; import { imageView } from "./views/ImageView"; import { markdownPreviewView } from "./views/MarkdownPreviewView"; +import { videoView } from "./views/VideoView"; // Order is preserved as a stable tiebreaker for equal-priority views. -// Exclusives (image, binary-warning) short-circuit resolution when matched. +// Exclusive views short-circuit resolution when matched. export const ALL_VIEWS: FileView[] = [ imageView, + videoView, binaryWarningView, markdownPreviewView, codeView, diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx new file mode 100644 index 00000000000..8efb59846cf --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/VideoView.tsx @@ -0,0 +1,47 @@ +import { useEffect, useState } from "react"; +import { getBaseName } from "renderer/lib/pathBasename"; +import { getVideoMimeType } from "shared/file-types"; +import type { ViewProps } from "../../types"; + +export function VideoView({ document, filePath }: ViewProps) { + const [source, setSource] = useState<{ + key: string; + url: string; + } | null>(null); + + const sourceKey = + document.content.kind === "bytes" + ? `${filePath}\0${document.content.revision}` + : null; + + useEffect(() => { + if (document.content.kind !== "bytes") { + setSource(null); + return; + } + const mimeType = getVideoMimeType(filePath) ?? "video/mp4"; + const url = URL.createObjectURL( + new Blob([document.content.value as BlobPart], { type: mimeType }), + ); + setSource({ key: `${filePath}\0${document.content.revision}`, url }); + return () => URL.revokeObjectURL(url); + }, [document.content, filePath]); + + if (!source || source.key !== sourceKey) { + return null; + } + + return ( +
+ +
+ ); +} diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/index.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/index.ts new file mode 100644 index 00000000000..00eadd2c8a0 --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/VideoView/index.ts @@ -0,0 +1,12 @@ +import { isPreviewableVideoFile } from "shared/file-types"; +import type { FileView } from "../../types"; +import { VideoView } from "./VideoView"; + +export const videoView: FileView = { + id: "video", + label: "Video", + match: (filePath) => isPreviewableVideoFile(filePath), + priority: "exclusive", + documentKind: "bytes", + Renderer: VideoView, +}; diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.ts index ce75ee071f5..b693b1ab066 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.ts @@ -151,9 +151,8 @@ export function useWorkspaceFileNavigation({ [store, worktreePath, recordView], ); - // Sidebar tree clicks layer the VS-Code-style "click an already-active row - // to pin it" pattern on top of openFilePane. The picker and other callers - // stay on plain openFilePane so re-picks just refocus without pinning. + // User-facing file opens from the workspace sidebar layer the VS-Code-style + // "click an already-active row to pin it" pattern on top of openFilePane. const openFilePaneFromTreeClick = useCallback( (filePath: string, openInNewTab?: boolean) => { if (openInNewTab) { diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx index 42364f1b4b0..a03fb11c52a 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx @@ -155,7 +155,6 @@ function V2WorkspaceContent() { }); const { - openFilePane, openFilePaneFromTreeClick, revealPath, selectedFilePath, @@ -169,7 +168,7 @@ function V2WorkspaceContent() { }); const paneRegistry = usePaneRegistry({ - onOpenFile: openFilePane, + onOpenFile: openFilePaneFromTreeClick, onRevealPath: revealPath, launcher, store, @@ -207,15 +206,14 @@ function V2WorkspaceContent() { [closeQuickOpen], ); // Picking a file from Quick Open should surface the sidebar/Files tab so - // the reveal (expand + highlight + scroll) is actually visible. Tree - // clicks and other openFilePane callers already have the sidebar open. + // the reveal (expand + highlight + scroll) is actually visible. const handleQuickOpenSelectFile = useCallback( (filePath: string, openInNewTab?: boolean) => { setRightSidebarOpen(true); setRightSidebarTab("files"); - openFilePane(filePath, openInNewTab); + openFilePaneFromTreeClick(filePath, openInNewTab); }, - [openFilePane, setRightSidebarOpen, setRightSidebarTab], + [openFilePaneFromTreeClick, setRightSidebarOpen, setRightSidebarTab], ); const defaultPaneActions = useDefaultPaneActions({ launcher }); const onBeforeCloseTab = useDirtyTabCloseGuard(); diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts index 7e1120d7c70..3acb18ef373 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts @@ -1,6 +1,6 @@ import type { workspaceTrpc } from "@superset/workspace-client"; import type { FsWatchEvent } from "@superset/workspace-fs/client"; -import { isImageFile } from "shared/file-types"; +import { isImageFile, isVideoFile } from "shared/file-types"; import type { ConflictResolution, ConflictState, @@ -91,7 +91,8 @@ async function loadEntry( options: { unlimited?: boolean } = {}, ): Promise { const client = entry.trpcClient; - const readAsBinary = isImageFile(entry.absolutePath); + const readAsBinary = + isImageFile(entry.absolutePath) || isVideoFile(entry.absolutePath); const maxBytes = options.unlimited ? undefined : DEFAULT_MAX_BYTES; try { const result = await client.filesystem.readFile.query({ @@ -102,6 +103,7 @@ async function loadEntry( }); entry.byteSize = result.byteLength; + entry.isBinary = readAsBinary ? true : entry.isBinary; entry.orphaned = false; entry.hasExternalChange = false; diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx index 8d920e73a5b..862eb21ab03 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx @@ -11,6 +11,7 @@ import { useChangesStore } from "renderer/stores/changes"; import { toAbsoluteWorkspacePath } from "shared/absolute-paths"; import type { ChangeCategory, ChangedFile } from "shared/changes-types"; import { detectLanguage } from "shared/detect-language"; +import { isVideoFile } from "shared/file-types"; import { getStatusColor, getStatusIndicator, @@ -123,9 +124,11 @@ export function FileDiffSection({ }); const totalChanges = file.additions + file.deletions; + const isVideo = isVideoFile(file.path); + const isBinaryFile = file.isBinary === true || isVideo; const isLargeDiff = totalChanges > LARGE_DIFF_THRESHOLD; const isGenerated = isGeneratedFile(file.path); - const isHiddenByDefault = isLargeDiff || isGenerated; + const isHiddenByDefault = !isBinaryFile && (isLargeDiff || isGenerated); const fileKey = createFileKey(file, category, commitHash, worktreePath); const isViewed = viewedFiles.has(fileKey); @@ -241,9 +244,12 @@ export function FileDiffSection({ const statusBadgeColor = getStatusColor(file.status); const statusIndicator = getStatusIndicator(file.status); - const showStats = file.additions > 0 || file.deletions > 0; + const showStats = !isBinaryFile && (file.additions > 0 || file.deletions > 0); const canShowDiffBody = - isExpanded && (!isHiddenByDefault || loadHiddenDiff) && !!worktreePath; + isExpanded && + !isBinaryFile && + (!isHiddenByDefault || loadHiddenDiff) && + !!worktreePath; const shouldLoadDiff = canShowDiffBody && hasBeenVisible && (isInLoadRange || isEditing); @@ -406,6 +412,20 @@ export function FileDiffSection({ ); + const binaryFilePreview = ( +
+ + Binary file — cannot display diff + + + Use the file header to open this file outside the diff viewer. + +
+ ); + return (
- {isHiddenByDefault && !loadHiddenDiff ? ( + {isBinaryFile ? ( + binaryFilePreview + ) : isHiddenByDefault && !loadHiddenDiff ? (

diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts index e33ca423e61..b17fa14ca5a 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.test.ts @@ -61,4 +61,22 @@ describe("getEstimatedFileDiffSectionHeight", () => { }), ).toBe(420); }); + + it("uses the unsupported diff placeholder for binary files", () => { + expect( + getEstimatedFileDiffSectionHeight({ + file: createFile("assets/logo.png", { isBinary: true }), + isCollapsed: false, + }), + ).toBe(340); + }); + + it("uses the unsupported diff placeholder for videos", () => { + expect( + getEstimatedFileDiffSectionHeight({ + file: createFile("assets/demo.mp4"), + isCollapsed: false, + }), + ).toBe(340); + }); }); diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.ts index b5280523c08..fcf583da576 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.ts +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/VirtualizedFileList/utils/getEstimatedFileDiffSectionHeight.ts @@ -1,4 +1,5 @@ import type { ChangedFile } from "shared/changes-types"; +import { isVideoFile } from "shared/file-types"; import { FILE_DIFF_SECTION_COLLAPSED_HEIGHT, FILE_DIFF_SECTION_PLACEHOLDER_HEIGHT, @@ -42,6 +43,14 @@ function isGeneratedFile(filePath: string): boolean { function getPlaceholderHeight(file: ChangedFile): number { const changedLineCount = file.additions + file.deletions; + if (isVideoFile(file.path)) { + return LARGE_PLACEHOLDER_HEIGHT; + } + + if (file.isBinary) { + return LARGE_PLACEHOLDER_HEIGHT; + } + if (isGeneratedFile(file.path)) { return XL_PLACEHOLDER_HEIGHT; } diff --git a/apps/desktop/src/shared/changes-types.ts b/apps/desktop/src/shared/changes-types.ts index 8318d4cc7fc..b8c4bc2d130 100644 --- a/apps/desktop/src/shared/changes-types.ts +++ b/apps/desktop/src/shared/changes-types.ts @@ -25,6 +25,7 @@ export interface ChangedFile { status: FileStatus; additions: number; deletions: number; + isBinary?: boolean; } /** A commit summary for the committed changes section */ diff --git a/apps/desktop/src/shared/file-types.test.ts b/apps/desktop/src/shared/file-types.test.ts index b96852ae227..188b9786ce8 100644 --- a/apps/desktop/src/shared/file-types.test.ts +++ b/apps/desktop/src/shared/file-types.test.ts @@ -2,6 +2,10 @@ import { describe, expect, test } from "bun:test"; import { getImageExtensionFromMimeType, getImageMimeType, + getVideoMimeType, + isImageFile, + isPreviewableVideoFile, + isVideoFile, parseBase64DataUrl, } from "./file-types"; @@ -11,9 +15,16 @@ describe("file-types", () => { test("maps image file paths to MIME types", () => { expect(getImageMimeType("logo.svg")).toBe("image/svg+xml"); expect(getImageMimeType("logo.ico")).toBe("image/x-icon"); + expect(getImageMimeType("logo.tiff")).toBe("image/tiff"); expect(getImageMimeType("logo.unknown")).toBeNull(); }); + test("detects supported image file paths", () => { + expect(isImageFile("sample.bmp")).toBe(true); + expect(isImageFile("sample.tiff")).toBe(true); + expect(isImageFile("sample.txt")).toBe(false); + }); + test("maps image MIME types to preferred extensions", () => { expect(getImageExtensionFromMimeType("image/jpeg")).toBe("jpg"); expect(getImageExtensionFromMimeType("image/vnd.microsoft.icon")).toBe( @@ -23,6 +34,29 @@ describe("file-types", () => { expect(getImageExtensionFromMimeType("image/avif")).toBeNull(); }); + test("detects supported video file paths", () => { + expect(isVideoFile("demo.mp4")).toBe(true); + expect(isVideoFile("clips/intro.webm")).toBe(true); + expect(isVideoFile("clips/INTRO.MOV")).toBe(true); + expect(isVideoFile("clips/intro.avi")).toBe(true); + expect(isVideoFile("clips/intro.mkv")).toBe(true); + expect(isVideoFile("archive.zip")).toBe(false); + }); + + test("detects browser-previewable video file paths", () => { + expect(isPreviewableVideoFile("demo.mp4")).toBe(true); + expect(isPreviewableVideoFile("demo.webm")).toBe(true); + expect(isPreviewableVideoFile("demo.avi")).toBe(false); + expect(isPreviewableVideoFile("demo.mkv")).toBe(false); + }); + + test("maps video file paths to MIME types", () => { + expect(getVideoMimeType("demo.mp4")).toBe("video/mp4"); + expect(getVideoMimeType("demo.webm")).toBe("video/webm"); + expect(getVideoMimeType("demo.mov")).toBe("video/quicktime"); + expect(getVideoMimeType("demo.avi")).toBeNull(); + }); + test("parses base64 data URLs with extra MIME parameters", () => { expect( parseBase64DataUrl( diff --git a/apps/desktop/src/shared/file-types.ts b/apps/desktop/src/shared/file-types.ts index ac6bfe3f71a..a6b53a9f743 100644 --- a/apps/desktop/src/shared/file-types.ts +++ b/apps/desktop/src/shared/file-types.ts @@ -3,6 +3,12 @@ * Used by both main and renderer processes. */ +import { getFileExtension, isVideoFile } from "@superset/shared/media-files"; + +// Re-exported so renderer/main code can keep importing extension helpers from +// `shared/file-types`; the canonical definitions live in `@superset/shared`. +export { getFileExtension, isVideoFile }; + /** Supported image extensions */ const IMAGE_EXTENSIONS = new Set([ "png", @@ -13,6 +19,8 @@ const IMAGE_EXTENSIONS = new Set([ "svg", "bmp", "ico", + "tif", + "tiff", ]); /** MIME types for supported image extensions */ @@ -25,6 +33,26 @@ const IMAGE_MIME_TYPES: Record = { svg: "image/svg+xml", bmp: "image/bmp", ico: "image/x-icon", + tif: "image/tiff", + tiff: "image/tiff", +}; + +/** Browser-playable video extensions */ +const PREVIEWABLE_VIDEO_EXTENSIONS = new Set([ + "m4v", + "mov", + "mp4", + "ogv", + "webm", +]); + +/** MIME types for supported video extensions */ +const VIDEO_MIME_TYPES: Record = { + mp4: "video/mp4", + m4v: "video/mp4", + mov: "video/quicktime", + ogv: "video/ogg", + webm: "video/webm", }; /** Extensions for supported image MIME types */ @@ -36,6 +64,7 @@ const IMAGE_MIME_TYPE_EXTENSIONS: Record = { "image/webp": "webp", "image/svg+xml": "svg", "image/bmp": "bmp", + "image/tiff": "tiff", "image/x-icon": "ico", "image/vnd.microsoft.icon": "ico", }; @@ -43,18 +72,11 @@ const IMAGE_MIME_TYPE_EXTENSIONS: Record = { /** Markdown extensions */ const MARKDOWN_EXTENSIONS = new Set(["md", "markdown", "mdx"]); -/** - * Gets the file extension from a path (lowercase, without dot) - */ -function getExtension(filePath: string): string { - return filePath.split(".").pop()?.toLowerCase() ?? ""; -} - /** * Checks if a file is an image based on extension */ export function isImageFile(filePath: string): boolean { - return IMAGE_EXTENSIONS.has(getExtension(filePath)); + return IMAGE_EXTENSIONS.has(getFileExtension(filePath)); } /** @@ -62,10 +84,26 @@ export function isImageFile(filePath: string): boolean { * Returns null if not a supported image type */ export function getImageMimeType(filePath: string): string | null { - const ext = getExtension(filePath); + const ext = getFileExtension(filePath); return IMAGE_MIME_TYPES[ext] ?? null; } +/** + * Checks if a video file can be previewed by the browser video element. + */ +export function isPreviewableVideoFile(filePath: string): boolean { + return PREVIEWABLE_VIDEO_EXTENSIONS.has(getFileExtension(filePath)); +} + +/** + * Gets the MIME type for a video file + * Returns null if not a supported video type + */ +export function getVideoMimeType(filePath: string): string | null { + const ext = getFileExtension(filePath); + return VIDEO_MIME_TYPES[ext] ?? null; +} + /** * Gets the preferred file extension for an image MIME type. * Returns null if not a supported image type. @@ -102,7 +140,7 @@ export function parseBase64DataUrl(dataUrl: string): { * Checks if a file is markdown based on extension */ export function isMarkdownFile(filePath: string): boolean { - return MARKDOWN_EXTENSIONS.has(getExtension(filePath)); + return MARKDOWN_EXTENSIONS.has(getFileExtension(filePath)); } /** diff --git a/packages/host-service/src/trpc/router/git/types.ts b/packages/host-service/src/trpc/router/git/types.ts index 90a80e558d8..7a6ddf78bda 100644 --- a/packages/host-service/src/trpc/router/git/types.ts +++ b/packages/host-service/src/trpc/router/git/types.ts @@ -122,6 +122,7 @@ export interface ChangedFile { status: FileStatus; additions: number; deletions: number; + isBinary?: boolean; } export interface Commit { diff --git a/packages/host-service/src/trpc/router/git/utils/git-helpers.test.ts b/packages/host-service/src/trpc/router/git/utils/git-helpers.test.ts index 4ce2ecc6b12..f7d2a88de64 100644 --- a/packages/host-service/src/trpc/router/git/utils/git-helpers.test.ts +++ b/packages/host-service/src/trpc/router/git/utils/git-helpers.test.ts @@ -1,32 +1,68 @@ -import { describe, expect, test } from "bun:test"; -import { parseNameStatus, parseNumstat } from "./git-helpers"; +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { ChangedFile } from "../types"; +import { + countUntrackedFileLines, + parseNameStatus, + parseNumstat, +} from "./git-helpers"; describe("parseNumstat", () => { test("regular file entry", () => { const raw = "5\t2\tsrc/foo.ts\0"; const result = parseNumstat(raw); - expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); + expect(result.get("src/foo.ts")).toEqual({ + additions: 5, + deletions: 2, + isBinary: false, + }); }); test("multiple regular entries", () => { const raw = "5\t2\tsrc/foo.ts\x003\t0\tsrc/bar.ts\x00"; const result = parseNumstat(raw); - expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); - expect(result.get("src/bar.ts")).toEqual({ additions: 3, deletions: 0 }); + expect(result.get("src/foo.ts")).toEqual({ + additions: 5, + deletions: 2, + isBinary: false, + }); + expect(result.get("src/bar.ts")).toEqual({ + additions: 3, + deletions: 0, + isBinary: false, + }); }); test("exact rename with edits indexes both paths", () => { const raw = "4\t3\t\x00src/old.ts\x00src/new.ts\x00"; const result = parseNumstat(raw); - expect(result.get("src/new.ts")).toEqual({ additions: 4, deletions: 3 }); - expect(result.get("src/old.ts")).toEqual({ additions: 4, deletions: 3 }); + expect(result.get("src/new.ts")).toEqual({ + additions: 4, + deletions: 3, + isBinary: false, + }); + expect(result.get("src/old.ts")).toEqual({ + additions: 4, + deletions: 3, + isBinary: false, + }); }); test("pure rename with zero line changes", () => { const raw = "0\t0\t\x00src/old.ts\x00src/new.ts\x00"; const result = parseNumstat(raw); - expect(result.get("src/new.ts")).toEqual({ additions: 0, deletions: 0 }); - expect(result.get("src/old.ts")).toEqual({ additions: 0, deletions: 0 }); + expect(result.get("src/new.ts")).toEqual({ + additions: 0, + deletions: 0, + isBinary: false, + }); + expect(result.get("src/old.ts")).toEqual({ + additions: 0, + deletions: 0, + isBinary: false, + }); }); test("binary file with dash markers", () => { @@ -35,6 +71,7 @@ describe("parseNumstat", () => { expect(result.get("assets/image.png")).toEqual({ additions: 0, deletions: 0, + isBinary: true, }); }); @@ -44,12 +81,25 @@ describe("parseNumstat", () => { "4\t3\t\x00src/old.ts\x00src/new.ts\x00" + "-\t-\tassets/image.png\x00"; const result = parseNumstat(raw); - expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); - expect(result.get("src/new.ts")).toEqual({ additions: 4, deletions: 3 }); - expect(result.get("src/old.ts")).toEqual({ additions: 4, deletions: 3 }); + expect(result.get("src/foo.ts")).toEqual({ + additions: 5, + deletions: 2, + isBinary: false, + }); + expect(result.get("src/new.ts")).toEqual({ + additions: 4, + deletions: 3, + isBinary: false, + }); + expect(result.get("src/old.ts")).toEqual({ + additions: 4, + deletions: 3, + isBinary: false, + }); expect(result.get("assets/image.png")).toEqual({ additions: 0, deletions: 0, + isBinary: true, }); }); @@ -63,20 +113,33 @@ describe("parseNumstat", () => { expect(result.get("weird\tpath.ts")).toEqual({ additions: 1, deletions: 1, + isBinary: false, }); }); test("rename where both paths contain tabs", () => { const raw = "2\t1\t\x00weird\told.ts\x00weird\tnew.ts\x00"; const result = parseNumstat(raw); - expect(result.get("weird\told.ts")).toEqual({ additions: 2, deletions: 1 }); - expect(result.get("weird\tnew.ts")).toEqual({ additions: 2, deletions: 1 }); + expect(result.get("weird\told.ts")).toEqual({ + additions: 2, + deletions: 1, + isBinary: false, + }); + expect(result.get("weird\tnew.ts")).toEqual({ + additions: 2, + deletions: 1, + isBinary: false, + }); }); test("non-ASCII path (raw UTF-8)", () => { const raw = "3\t1\tsrc/日本語.ts\0"; const result = parseNumstat(raw); - expect(result.get("src/日本語.ts")).toEqual({ additions: 3, deletions: 1 }); + expect(result.get("src/日本語.ts")).toEqual({ + additions: 3, + deletions: 1, + isBinary: false, + }); }); }); @@ -137,3 +200,73 @@ describe("parseNameStatus", () => { expect(parseNameStatus("")).toEqual([]); }); }); + +describe("countUntrackedFileLines", () => { + let dir: string; + + beforeAll(async () => { + dir = await mkdtemp(join(tmpdir(), "untracked-lines-")); + }); + + afterAll(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + const SENTINEL = -999; + function makeFile(path: string): ChangedFile { + return { path, status: "untracked", additions: SENTINEL, deletions: 0 }; + } + + async function count(name: string, contents: Buffer | string) { + await writeFile(join(dir, name), contents); + const file = makeFile(name); + await countUntrackedFileLines(dir, [file]); + return file; + } + + test("counts lines with a trailing newline", async () => { + const file = await count("trailing.txt", "a\nb\nc\n"); + expect(file.additions).toBe(3); + expect(file.isBinary).toBeUndefined(); + }); + + test("counts a final line with no trailing newline", async () => { + const file = await count("no-trailing.txt", "a\nb\nc"); + expect(file.additions).toBe(3); + }); + + test("empty file is zero lines", async () => { + const file = await count("empty.txt", ""); + expect(file.additions).toBe(0); + }); + + test("CRLF counts the same as LF", async () => { + const file = await count("crlf.txt", "a\r\nb\r\nc\r\n"); + expect(file.additions).toBe(3); + }); + + test("counts correctly across read-chunk boundaries", async () => { + // 64KB chunk size; 20000 short lines spans several chunks. + const lineCount = 20000; + const file = await count("big.txt", `${"x".repeat(8)}\n`.repeat(lineCount)); + expect(file.additions).toBe(lineCount); + }); + + test("flags a NUL byte in the first 8KB as binary", async () => { + const file = await count("nul.bin", Buffer.from([0x61, 0x00, 0x62, 0x0a])); + expect(file.isBinary).toBe(true); + expect(file.additions).toBe(0); + }); + + test("flags by media extension without reading content", async () => { + const file = await count("clip.mp4", "not really a video\n"); + expect(file.isBinary).toBe(true); + expect(file.additions).toBe(0); + }); + + test("skips the LOC signal for files over the size budget", async () => { + const file = await count("huge.txt", "a\n".repeat(600_000)); // ~1.2MB + expect(file.additions).toBe(SENTINEL); + expect(file.isBinary).toBeUndefined(); + }); +}); 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 index 93b71bc436e..943619fee3b 100644 --- a/packages/host-service/src/trpc/router/git/utils/git-helpers.ts +++ b/packages/host-service/src/trpc/router/git/utils/git-helpers.ts @@ -1,13 +1,10 @@ -import { - copyFile, - mkdtemp, - readFile, - realpath, - rm, - stat, -} from "node:fs/promises"; +import { copyFile, mkdtemp, open, realpath, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; +import { + BINARY_SNIFF_BYTES, + isBinaryMediaFile, +} from "@superset/shared/media-files"; import type { SimpleGit } from "simple-git"; import { resolveUpstream } from "../../../../runtime/git/refs"; import { createUserSimpleGit } from "../../../../runtime/git/simple-git"; @@ -23,6 +20,11 @@ const MAX_UNTRACKED_LINE_COUNT_SIZE = 1 * 1024 * 1024; // doesn't exhaust the process file-descriptor limit. const UNTRACKED_IO_CONCURRENCY = 64; +// Chunk size for streaming untracked files when counting lines. Bounds +// per-file memory to this × UNTRACKED_IO_CONCURRENCY instead of the full +// file size, and comfortably covers the 8KB binary sniff window. +const UNTRACKED_READ_CHUNK_SIZE = 64 * 1024; + async function mapWithConcurrency( items: T[], limit: number, @@ -72,8 +74,11 @@ export function mapGitStatus(code: string): FileStatus { */ export function parseNumstat( raw: string, -): Map { - const result = new Map(); +): Map { + const result = new Map< + string, + { additions: number; deletions: number; isBinary: boolean } + >(); const entries = raw.split("\0"); for (let i = 0; i < entries.length; i++) { const entry = entries[i]; @@ -87,6 +92,7 @@ export function parseNumstat( const stats = { additions: add === "-" ? 0 : Number.parseInt(add || "0", 10), deletions: del === "-" ? 0 : Number.parseInt(del || "0", 10), + isBinary: add === "-" && del === "-", }; if (pathMaybe === "") { const oldPath = entries[++i] ?? ""; @@ -266,26 +272,61 @@ export async function countUntrackedFileLines( if (!isPathWithinWorktree(worktreeReal, fileReal)) return; const stats = await stat(fileReal); - if (!stats.isFile() || stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) { + if (!stats.isFile()) { return; } - // `readFile(file, "utf-8")` happily turns binary into U+FFFDs - // and returns a non-zero line count, so sniff first 8KB for - // NULs the way git's own binary heuristic does. - 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 (isBinaryMediaFile(file.path)) { + file.isBinary = true; + file.additions = 0; + file.deletions = 0; + return; } - const content = buf.toString("utf-8"); - file.additions = - content === "" - ? 0 - : content.endsWith("\n") - ? content.split(/\r?\n/).length - 1 - : content.split(/\r?\n/).length; + // Stream the file in fixed-size chunks rather than slurping it: + // readFile would pin the whole file in memory (×UNTRACKED_IO_CONCURRENCY) + // and, read as utf-8, would turn binary into U+FFFDs and report a + // bogus line count. We reuse one buffer, sniff the first 8KB for NULs + // (git's binary heuristic), and tally newlines as we go. + const handle = await open(fileReal, "r"); + try { + const buf = Buffer.allocUnsafe(UNTRACKED_READ_CHUNK_SIZE); + let { bytesRead } = await handle.read(buf, 0, buf.length, 0); + + const sniffEnd = Math.min(bytesRead, BINARY_SNIFF_BYTES); + for (let i = 0; i < sniffEnd; i++) { + if (buf[i] === 0) { + file.isBinary = true; + file.additions = 0; + file.deletions = 0; + return; + } + } + + // Over the budget: skip the LOC signal without reading the rest. + if (stats.size > MAX_UNTRACKED_LINE_COUNT_SIZE) { + return; + } + + let newlines = 0; + let lastByte = -1; + let offset = 0; + while (bytesRead > 0) { + for (let i = 0; i < bytesRead; i++) { + if (buf[i] === 0x0a) newlines++; + } + lastByte = buf[bytesRead - 1] ?? lastByte; + offset += bytesRead; + ({ bytesRead } = await handle.read(buf, 0, buf.length, offset)); + } + // Match `content.split(/\r?\n/)`: a trailing newline doesn't add a + // line, but a final non-empty line without one does. (\r\n shares + // the \n, so counting \n bytes is equivalent.) + file.additions = + lastByte === -1 ? 0 : lastByte === 0x0a ? newlines : newlines + 1; + } finally { + await handle.close(); + } } catch {} }); } @@ -296,6 +337,7 @@ export interface DetectedRename { status: "renamed"; additions: number; deletions: number; + isBinary: boolean; } /** @@ -354,13 +396,18 @@ export async function detectUnstagedRenames( if (!entry.oldPath) continue; const code = entry.status[0]; if (code !== "R") continue; - const stats = numstat.get(entry.path) ?? { additions: 0, deletions: 0 }; + 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, }); } return result; @@ -395,6 +442,7 @@ export async function getChangedFilesForDiff( status: mapGitStatus(f.status), additions: (numstat.get(f.path) ?? { additions: 0 }).additions, deletions: (numstat.get(f.path) ?? { deletions: 0 }).deletions, + isBinary: (numstat.get(f.path) ?? { isBinary: false }).isBinary, })); } catch { return []; 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 index 071bf222eaa..90a889ccf77 100644 --- a/packages/host-service/src/trpc/router/git/utils/git-status.ts +++ b/packages/host-service/src/trpc/router/git/utils/git-status.ts @@ -77,6 +77,7 @@ export async function getGitStatusSnapshot({ const stats = stagedNumstat.get(file.path) ?? { additions: 0, deletions: 0, + isBinary: false, }; staged.push({ path: file.path, @@ -84,6 +85,7 @@ export async function getGitStatusSnapshot({ status: mapGitStatus(idx), additions: stats.additions, deletions: stats.deletions, + isBinary: stats.isBinary, }); } } @@ -108,12 +110,14 @@ export async function getGitStatusSnapshot({ const stats = unstagedNumstat.get(file.path) ?? { additions: 0, deletions: 0, + isBinary: false, }; unstaged.push({ path: file.path, status: mapGitStatus(wd), additions: stats.additions, deletions: stats.deletions, + isBinary: stats.isBinary, }); } } @@ -149,6 +153,7 @@ export async function getGitStatusSnapshot({ status: rename.status, additions: rename.additions, deletions: rename.deletions, + isBinary: rename.isBinary, }); } } diff --git a/packages/shared/package.json b/packages/shared/package.json index dd63b3df656..15e52e66b36 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -12,6 +12,10 @@ "types": "./src/dev-credentials.ts", "default": "./src/dev-credentials.ts" }, + "./media-files": { + "types": "./src/media-files.ts", + "default": "./src/media-files.ts" + }, "./v2-only-user": { "types": "./src/v2-only-user.ts", "default": "./src/v2-only-user.ts" diff --git a/packages/shared/src/media-files.test.ts b/packages/shared/src/media-files.test.ts new file mode 100644 index 00000000000..043948b0792 --- /dev/null +++ b/packages/shared/src/media-files.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, test } from "bun:test"; +import { + getFileExtension, + isBinaryMediaFile, + isVideoFile, +} from "./media-files"; + +describe("media-files", () => { + test("extracts lowercase extensions and ignores directory dots", () => { + expect(getFileExtension("clips/INTRO.MOV")).toBe("mov"); + expect(getFileExtension("a.b.c/file.TS")).toBe("ts"); + expect(getFileExtension("noext")).toBe(""); + expect(getFileExtension(".gitignore")).toBe(""); + expect(getFileExtension("trailing.")).toBe(""); + }); + + test("detects video files across container formats", () => { + expect(isVideoFile("demo.mp4")).toBe(true); + expect(isVideoFile("clips/intro.mkv")).toBe(true); + expect(isVideoFile("archive.zip")).toBe(false); + }); + + test("flags raster images and videos as binary media", () => { + expect(isBinaryMediaFile("logo.png")).toBe(true); + expect(isBinaryMediaFile("photo.JPG")).toBe(true); + expect(isBinaryMediaFile("demo.webm")).toBe(true); + expect(isBinaryMediaFile("notes.txt")).toBe(false); + }); + + test("treats svg as text so its diff stays visible", () => { + expect(isBinaryMediaFile("icon.svg")).toBe(false); + }); +}); diff --git a/packages/shared/src/media-files.ts b/packages/shared/src/media-files.ts new file mode 100644 index 00000000000..94decf895e6 --- /dev/null +++ b/packages/shared/src/media-files.ts @@ -0,0 +1,80 @@ +/** + * Single source of truth for extension-based file classification shared by the + * host-service git path and the desktop main/renderer code. Keeping these here + * stops the two sides from drifting on questions like whether `.svg` counts as + * binary (it does not — it is text and should stay diffable). + */ + +/** Gets the file extension from a path (lowercase, without dot). */ +export function getFileExtension(filePath: string): string { + const fileName = filePath.split(/[\\/]/).pop() ?? filePath; + const dotIndex = fileName.lastIndexOf("."); + if (dotIndex <= 0 || dotIndex === fileName.length - 1) { + return ""; + } + return fileName.slice(dotIndex + 1).toLowerCase(); +} + +/** + * Raster image extensions. Excludes `svg`, which is text: it renders as an + * image but is better shown as a diff than hidden behind a binary placeholder. + */ +export const RASTER_IMAGE_EXTENSIONS: ReadonlySet = new Set([ + "bmp", + "gif", + "ico", + "jpeg", + "jpg", + "png", + "tif", + "tiff", + "webp", +]); + +/** Video container extensions. */ +export const VIDEO_EXTENSIONS: ReadonlySet = new Set([ + "3g2", + "3gp", + "avi", + "m4v", + "mkv", + "mov", + "mp4", + "mpeg", + "mpg", + "ogv", + "webm", + "wmv", +]); + +/** + * Extensions that are always binary media (raster images + videos). These are + * never diffable as text, so the changes pane shows a placeholder/preview + * instead of attempting a line diff. + */ +export const BINARY_MEDIA_EXTENSIONS: ReadonlySet = new Set([ + ...RASTER_IMAGE_EXTENSIONS, + ...VIDEO_EXTENSIONS, +]); + +/** + * Number of leading bytes to sniff for NUL when deciding whether an untracked + * file is binary, mirroring git's own heuristic. + */ +export const BINARY_SNIFF_BYTES = 8192; + +/** + * Checks if a file is a video based on extension. Covers all known video + * extensions, not just browser-playable ones. + */ +export function isVideoFile(filePath: string): boolean { + return VIDEO_EXTENSIONS.has(getFileExtension(filePath)); +} + +/** + * Checks if a file is binary media (raster image or video) purely from its + * extension, so a line diff is never attempted for it. + */ +export function isBinaryMediaFile(filePath: string): boolean { + return BINARY_MEDIA_EXTENSIONS.has(getFileExtension(filePath)); +}