mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-02 15:09:26 +00:00
feat(stack-files): force-text override for misidentified binary files (#1215)
* feat(stack-files): force-text override for misidentified binary files
The binary-detection heuristic (30% non-printable / NUL in the first
8 KB) sometimes flags UTF-8 files that happen to carry an embedded NUL
or a high non-printable ratio, locking the user out of inline editing
with only a Download fallback.
readStackFile now accepts an optional { forceText: true } that bypasses
isBinaryBuffer on the small-file path and returns the bytes as UTF-8
content. The route exposes this as ?force=text on GET /files/content.
The oversized branch deliberately stays untouched: returning a multi-MB
file as JSON-encoded text is wasteful regardless of the heuristic.
SpecialFilePanel grows an optional extraAction slot. The viewer's
binary branch wires Open as text anyway, which refetches with the new
flag, clears isBinary, and routes the content through the existing
Monaco editor path. A failed override surfaces both an inline error
panel and a toast so the user knows why the click did nothing.
Backend tests pin the heuristic-vs-override behaviour pair on a file
with a literal NUL byte. The frontend test asserts that the second
readStackFile call carries forceText: true and that Monaco mounts.
Troubleshooting accordion entry updated to mention the new affordance.
* fix(stack-files): guard the binary-override path against oversized files
The override on the binary panel could open Monaco against an empty
content buffer if the backend's oversized branch ran (files past the
2 MB inline-preview cap intentionally carry no content even when
force=text is set). Saving that empty buffer would wipe the file on
disk.
Two reinforcing changes:
- Initial load now checks result.oversized before result.binary, so a
file that is both oversized and has binary bytes in the 8 KB probe
shows the Download panel rather than the binary panel. The size
signal stays in front of the operator and the override button never
surfaces for a file that cannot be safely opened inline.
- The handleForceText handler now respects result.oversized on the
refetch and transitions to the Download panel instead of clearing
isBinary and copying result.content ?? '' into Monaco.
Same handler also gains a stale-request guard via a selectedPathRef:
a slow override for file A no longer stomps on file B's state if the
user navigated away while the request was in flight.
Two regression tests pin the new behaviour: oversized+binary surfaces
the Download panel on initial load, and an oversized refetch from the
binary panel routes to the Download panel rather than Monaco.
This commit is contained in:
@@ -241,6 +241,40 @@ describe('GET /api/stacks/:stackName/files/content', () => {
|
||||
|
||||
await fs.unlink(bigPath);
|
||||
}, 15000);
|
||||
|
||||
it('returns binary:true by default for a file with a NUL byte in the probe window', async () => {
|
||||
const oddPath = path.join(stacksDir, STACK, 'force-text-default.txt');
|
||||
await fs.writeFile(oddPath, Buffer.from('hello\0world rest is utf8 text', 'utf-8'));
|
||||
try {
|
||||
const res = await request(app)
|
||||
.get(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'force-text-default.txt' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.binary).toBe(true);
|
||||
expect(res.body.content).toBeUndefined();
|
||||
} finally {
|
||||
await fs.unlink(oddPath);
|
||||
}
|
||||
});
|
||||
|
||||
it('returns the content as UTF-8 when ?force=text is set, even for files the binary heuristic rejects', async () => {
|
||||
const oddPath = path.join(stacksDir, STACK, 'force-text-on.txt');
|
||||
const raw = Buffer.from('hello\0world rest is utf8 text', 'utf-8');
|
||||
await fs.writeFile(oddPath, raw);
|
||||
try {
|
||||
const res = await request(app)
|
||||
.get(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'force-text-on.txt', force: 'text' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.binary).toBe(false);
|
||||
expect(typeof res.body.content).toBe('string');
|
||||
expect(res.body.content).toBe(raw.toString('utf-8'));
|
||||
} finally {
|
||||
await fs.unlink(oddPath);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ── GET /:stackName/files/download ────────────────────────────────────────────
|
||||
|
||||
@@ -1420,10 +1420,11 @@ stacksRouter.get('/:stackName/files/content', async (req: Request, res: Response
|
||||
if (!isValidRelativeStackPath(relPath)) {
|
||||
return res.status(400).json({ error: 'Invalid path', code: 'INVALID_PATH' });
|
||||
}
|
||||
const forceText = req.query.force === 'text';
|
||||
const startedAt = Date.now();
|
||||
logFileDiag('read start', { stackName, relPath, nodeId: req.nodeId });
|
||||
logFileDiag('read start', { stackName, relPath, nodeId: req.nodeId, forceText });
|
||||
try {
|
||||
const result = await FileSystemService.getInstance(req.nodeId).readStackFile(stackName, relPath);
|
||||
const result = await FileSystemService.getInstance(req.nodeId).readStackFile(stackName, relPath, undefined, { forceText });
|
||||
// ETag is the integer mtimeMs the file was stat'd with, so the matching
|
||||
// PUT can compare millisecond-equal even though some filesystems return
|
||||
// float mtimeMs.
|
||||
|
||||
@@ -675,7 +675,8 @@ export class FileSystemService {
|
||||
async readStackFile(
|
||||
stackName: string,
|
||||
relPath: string,
|
||||
maxBytes: number = 2 * 1024 * 1024
|
||||
maxBytes: number = 2 * 1024 * 1024,
|
||||
opts: { forceText?: boolean } = {},
|
||||
): Promise<{ content?: string; binary: boolean; oversized: boolean; size: number; mime: string; mtimeMs: number }> {
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
const mime = this.guessMime(safePath);
|
||||
@@ -701,7 +702,12 @@ export class FileSystemService {
|
||||
|
||||
const buf = await fh.readFile();
|
||||
|
||||
if (isBinaryBuffer(buf)) {
|
||||
// forceText bypasses the binary-detection heuristic so callers can
|
||||
// recover from false positives (a UTF-8 file that happens to carry a
|
||||
// NUL or a high non-printable ratio in its first 8 KB). The oversized
|
||||
// branch above still applies because returning a multi-megabyte file
|
||||
// as JSON-encoded text is wasteful regardless of the heuristic.
|
||||
if (!opts.forceText && isBinaryBuffer(buf)) {
|
||||
return { binary: true, oversized: false, size: stat.size, mime, mtimeMs };
|
||||
}
|
||||
|
||||
|
||||
@@ -169,7 +169,7 @@ The Permissions dialog opens for everyone; only users with stack edit permission
|
||||
|
||||
<AccordionGroup>
|
||||
<Accordion title="File shows as 'Binary file detected' but it is a text file">
|
||||
The binary detection heuristic flagged null bytes or a high proportion of non-printable characters in the file's first kilobytes. This can happen with certain encodings, non-UTF-8 byte sequences, or unusual line-ending mixes. Download the file, edit it locally or via a host terminal, then re-upload.
|
||||
The binary detection heuristic flagged null bytes or a high proportion of non-printable characters in the file's first kilobytes. This can happen with certain encodings, non-UTF-8 byte sequences, or unusual line-ending mixes. Click **Open as text anyway** on the binary panel to re-fetch the file as UTF-8 text and edit it in the inline viewer. Files above the 2 MB preview limit still need a Download.
|
||||
</Accordion>
|
||||
<Accordion title="Upload fails with a 413 error">
|
||||
The file exceeds the 25 MB per-file upload limit. Compress or split the file before uploading, or transfer it via `scp` or `rsync` directly to the stack directory on the host.
|
||||
|
||||
@@ -27,6 +27,7 @@ interface SpecialFilePanelProps {
|
||||
label: string;
|
||||
stackName: string;
|
||||
relPath: string;
|
||||
extraAction?: { label: string; onClick: () => void; disabled?: boolean };
|
||||
}
|
||||
|
||||
function SpecialFilePanel({
|
||||
@@ -35,6 +36,7 @@ function SpecialFilePanel({
|
||||
label,
|
||||
stackName,
|
||||
relPath,
|
||||
extraAction,
|
||||
}: SpecialFilePanelProps) {
|
||||
const [downloading, setDownloading] = useState(false);
|
||||
|
||||
@@ -69,19 +71,31 @@ function SpecialFilePanel({
|
||||
<p className="font-mono text-sm text-stat-title">{filename}</p>
|
||||
<p className="text-xs text-stat-subtitle">{label} · {formatBytes(size)}</p>
|
||||
</div>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() => void handleDownload()}
|
||||
disabled={downloading}
|
||||
>
|
||||
{downloading ? (
|
||||
<Loader2 className="w-4 h-4 mr-1.5 animate-spin" strokeWidth={1.5} />
|
||||
) : (
|
||||
<Download className="w-4 h-4 mr-1.5" strokeWidth={1.5} />
|
||||
<div className="flex items-center gap-2">
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() => void handleDownload()}
|
||||
disabled={downloading}
|
||||
>
|
||||
{downloading ? (
|
||||
<Loader2 className="w-4 h-4 mr-1.5 animate-spin" strokeWidth={1.5} />
|
||||
) : (
|
||||
<Download className="w-4 h-4 mr-1.5" strokeWidth={1.5} />
|
||||
)}
|
||||
Download
|
||||
</Button>
|
||||
{extraAction && (
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="sm"
|
||||
onClick={extraAction.onClick}
|
||||
disabled={extraAction.disabled}
|
||||
>
|
||||
{extraAction.label}
|
||||
</Button>
|
||||
)}
|
||||
Download
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -115,6 +129,15 @@ export function FileViewer({
|
||||
onDirtyChangeRef.current = onDirtyChange;
|
||||
}, [onDirtyChange]);
|
||||
|
||||
// Mirror selectedPath into a ref so async handlers fired from button clicks
|
||||
// (handleForceText) can detect a stale response after the user has already
|
||||
// navigated to a different file. A slow override for file A must not stomp
|
||||
// on file B's state.
|
||||
const selectedPathRef = useRef(selectedPath);
|
||||
useEffect(() => {
|
||||
selectedPathRef.current = selectedPath;
|
||||
}, [selectedPath]);
|
||||
|
||||
useEffect(() => {
|
||||
onDirtyChangeRef.current?.(hasChanges);
|
||||
}, [hasChanges]);
|
||||
@@ -156,10 +179,17 @@ export function FileViewer({
|
||||
if (cancelled) return;
|
||||
setSize(result.size);
|
||||
setLoadedMtimeMs(result.mtimeMs);
|
||||
if (result.binary) {
|
||||
setIsBinary(true);
|
||||
} else if (result.oversized) {
|
||||
// Check oversized BEFORE binary: the backend returns oversized:true
|
||||
// for files past the 2 MB inline-preview cap regardless of the binary
|
||||
// probe, and the body intentionally carries no content for those
|
||||
// files. Showing the binary panel for an oversized file would hide
|
||||
// the size signal and offer an override that resolves to an empty
|
||||
// editor (the backend keeps the oversized-no-content contract even
|
||||
// when force=text is set).
|
||||
if (result.oversized) {
|
||||
setIsOversized(true);
|
||||
} else if (result.binary) {
|
||||
setIsBinary(true);
|
||||
} else {
|
||||
const text = result.content ?? '';
|
||||
setContent(text);
|
||||
@@ -241,6 +271,40 @@ export function FileViewer({
|
||||
const language = extensionToLanguage(filename);
|
||||
|
||||
if (isBinary) {
|
||||
const handleForceText = async () => {
|
||||
const requestedPath = selectedPath;
|
||||
setLoading(true);
|
||||
setError(null);
|
||||
try {
|
||||
const result = await readStackFile(stackName, requestedPath, { forceText: true });
|
||||
// Stale-request guard: the user may have navigated to a different
|
||||
// file while the override request was in flight. Drop the response
|
||||
// rather than stomp on the new file's state.
|
||||
if (selectedPathRef.current !== requestedPath) return;
|
||||
setSize(result.size);
|
||||
setLoadedMtimeMs(result.mtimeMs);
|
||||
if (result.oversized) {
|
||||
// Backend keeps oversized files out of the inline editor even with
|
||||
// force=text set; the body has no content. Surface the Download
|
||||
// panel so the user does not get an empty Monaco buffer that they
|
||||
// could accidentally save back over the file.
|
||||
setIsBinary(false);
|
||||
setIsOversized(true);
|
||||
} else {
|
||||
const text = result.content ?? '';
|
||||
setContent(text);
|
||||
setOriginalContent(text);
|
||||
setIsBinary(false);
|
||||
}
|
||||
} catch (e) {
|
||||
if (selectedPathRef.current !== requestedPath) return;
|
||||
const message = e instanceof Error ? e.message : 'Failed to load file as text.';
|
||||
setError(message);
|
||||
toast.error(message);
|
||||
} finally {
|
||||
if (selectedPathRef.current === requestedPath) setLoading(false);
|
||||
}
|
||||
};
|
||||
return (
|
||||
<SpecialFilePanel
|
||||
filename={filename}
|
||||
@@ -248,6 +312,10 @@ export function FileViewer({
|
||||
label="Binary file"
|
||||
stackName={stackName}
|
||||
relPath={selectedPath}
|
||||
extraAction={{
|
||||
label: 'Open as text anyway',
|
||||
onClick: () => void handleForceText(),
|
||||
}}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -238,6 +238,66 @@ describe('FileViewer', () => {
|
||||
expect(opts).toEqual({ ifMatchMtimeMs: 1_700_000_000_000 });
|
||||
});
|
||||
|
||||
it('binary panel offers "Open as text anyway"; click refetches with forceText and renders Monaco', async () => {
|
||||
mockReadFile
|
||||
.mockResolvedValueOnce(binaryResult())
|
||||
.mockResolvedValueOnce(textResult('rescued as text'));
|
||||
|
||||
render(<FileViewer {...defaultProps} selectedPath="quirky-utf8.txt" />);
|
||||
await screen.findByText(/binary file/i);
|
||||
|
||||
const overrideBtn = screen.getByRole('button', { name: /open as text anyway/i });
|
||||
overrideBtn.click();
|
||||
|
||||
await waitFor(() => expect(screen.getByTestId('monaco-editor')).toBeInTheDocument());
|
||||
expect(mockReadFile).toHaveBeenCalledTimes(2);
|
||||
expect(mockReadFile).toHaveBeenNthCalledWith(2, 'my-stack', 'quirky-utf8.txt', { forceText: true });
|
||||
});
|
||||
|
||||
it('renders the oversized panel (not the binary panel) when both flags are set', async () => {
|
||||
// The backend can return `binary: true, oversized: true` when a >2 MB
|
||||
// file's first 8 KB probe also looks binary. Surfacing the binary panel
|
||||
// there would hide the size signal and offer an override that resolves
|
||||
// to an empty editor, so the oversized panel must win.
|
||||
mockReadFile.mockResolvedValue({
|
||||
binary: true,
|
||||
oversized: true,
|
||||
size: 5_000_000,
|
||||
mime: 'application/octet-stream',
|
||||
mtimeMs: 1_700_000_000_000,
|
||||
});
|
||||
|
||||
render(<FileViewer {...defaultProps} selectedPath="huge-binary.bin" />);
|
||||
|
||||
expect(await screen.findByText(/too large to preview/i)).toBeInTheDocument();
|
||||
expect(screen.queryByText(/binary file/i)).not.toBeInTheDocument();
|
||||
expect(screen.queryByRole('button', { name: /open as text anyway/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('override on an oversized response falls back to the Download panel, never Monaco', async () => {
|
||||
// Defence-in-depth: even if a future regression let the override button
|
||||
// surface on an oversized file, the handler must not open Monaco against
|
||||
// an empty content buffer. Save would then wipe the file on disk.
|
||||
mockReadFile
|
||||
.mockResolvedValueOnce(binaryResult())
|
||||
.mockResolvedValueOnce({
|
||||
binary: false,
|
||||
oversized: true,
|
||||
size: 5_000_000,
|
||||
mime: 'text/plain',
|
||||
mtimeMs: 1_700_000_000_000,
|
||||
});
|
||||
|
||||
render(<FileViewer {...defaultProps} selectedPath="huge-misclassified.txt" />);
|
||||
await screen.findByText(/binary file/i);
|
||||
|
||||
const overrideBtn = screen.getByRole('button', { name: /open as text anyway/i });
|
||||
overrideBtn.click();
|
||||
|
||||
expect(await screen.findByText(/too large to preview/i)).toBeInTheDocument();
|
||||
expect(screen.queryByTestId('monaco-editor')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('updates baseline on FileConflictError without discarding the user buffer; follow-up save uses new mtime', async () => {
|
||||
mockReadFile.mockResolvedValue(textResult('stale local copy'));
|
||||
mockWriteFile
|
||||
|
||||
@@ -86,10 +86,14 @@ export async function listStackDirectory(
|
||||
|
||||
export async function readStackFile(
|
||||
stackName: string,
|
||||
relPath: string
|
||||
relPath: string,
|
||||
options?: { forceText?: boolean }
|
||||
): Promise<FileContentResult> {
|
||||
assertSafeRelPath(relPath);
|
||||
const res = await apiFetch(stackFilesUrl(stackName, `/content?path=${encodeURIComponent(relPath)}`));
|
||||
const forceSuffix = options?.forceText ? '&force=text' : '';
|
||||
const res = await apiFetch(
|
||||
stackFilesUrl(stackName, `/content?path=${encodeURIComponent(relPath)}${forceSuffix}`)
|
||||
);
|
||||
if (!res.ok) throw new Error(await parseApiError(res));
|
||||
return res.json() as Promise<FileContentResult>;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user