Build App (Preview) / compute-version (pull_request) Successful in 5s
Build Container / build-container (pull_request) Successful in 37s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m40s
Build App (Preview) / build-windows (pull_request) Successful in 4m56s
Build App (Preview) / build-linux (pull_request) Successful in 5m11s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Two independent reviews of 2c9482a, one for correctness and one against the
threat model. Between them they found two ways to lose a file, one way for a
container to choose a Windows save destination, and a rule that made every
dotfile unsavable. Every finding below was demonstrated against a real
container before being fixed, and the fixes are demonstrated the same way.
## The download was accepting truncated reads and refusing whole ones
The `[ -f ]` bracket around the read was wrong in both directions.
It missed the case that loses data. Truncation *in place* — `> file`, log
rotation, `tar -x`, most build tools — leaves a regular file behind, so `dd`
stopped at the new EOF and exited 0. Measured: a 600 MB source truncated
mid-read delivered 34 MB, which was then renamed over the user's own earlier
copy and toasted as `Saved (34.1 MB)`. Truncate-to-zero did the same and needs
no adversary at all.
And it failed *good* downloads. `dd` already holds the fd, and neither `rm` nor
`mv` can touch an open one — the bytes are complete. But `rm` makes `[ -f ]`
false, so a finished 600 MB transfer of a file a bundler happened to unlink was
deleted, reporting "nothing was saved". The comment claiming the bracket caught
a "mix of two files" was simply wrong; an open fd cannot be a mix.
So the script now measures the file before reading it and puts the answer on
stderr, and Rust checks that at least that many bytes arrived. One rule,
subsuming everything the bracket was for. Verified against a real container:
truncation mid-read and the FIFO race are refused; deletion, rename-over, a
growing file, an empty file and an untouched 600 MB read all pass.
## On Windows the container, not the user, was naming the save destination
`suggested_save_name` split on `/` only, and it feeds the save dialog's
pre-filled name. Backslash is a legal Linux filename character and
`validate_container_path` has no reason to object — `..\..\Users\…` is one
POSIX segment. The Windows common file dialog parses its name box as a path on
Save, so a container-created file called
`..\..\..\Users\vic\AppData\Roaming\Microsoft\Word\STARTUP\x.dotm` put its own
bytes in an auto-loading Office directory on one un-read click. `resolve_host_path`
did not stop it: Word's `STARTUP` and Excel's `XLSTART` are not in the autorun
denylist, which this file's own docs already concede is "losing by
construction" and was never meant to be the boundary here.
The name is now sanitized of every separator, the drive colon and the rest of
what NTFS refuses, so it cannot be a path on any platform this ships to.
## No dotfile could be saved, and the app pre-filled the name that guaranteed it
The write policy judged the leaf for hiddenness, so `/workspace/.env` was
refused *after* the modal and the overwrite prompt — quoting the name the app
itself had suggested. `.gitignore`, `.dockerignore`, `.eslintrc.json`, `.nvmrc`:
all unsavable, while uploading them worked, so a dotfile could go in and never
come out.
`HostPathUse` gains a third mode. `WriteChosenName` drops the leaf check and
keeps every directory rule, and only `download_container_file` uses it — the
dialog is a real boundary for that caller and only that caller.
`download_container_backup` still takes its path over IPC as a string and keeps
the strict rule.
## Smaller, all found by the reviews
* A download had no ceiling. `dd` resolves through the container's `PATH`,
which its agent owns with passwordless sudo; a replacement writing forever
was measured at ~6 GB/s, so one click on a file listed as 2 KB filled the
host disk with no progress shown and no cancel. Bounded now by what the
file measured, with slack that is absolute for small files and
proportional for large ones, so an honest growing log is unaffected.
* "Framed rather than verbatim" did not stop container text reaching the
toast headline: `readableRefusal` matches with `includes`, and it has to,
because the app's own refusals carry those markers mid-sentence. Anchoring
would break them. The fix is at the injection point — container text is
clipped to one 200-character line with control characters stripped — plus a
`max-h-40` on the toast message, which the `detail` block always had and
this half did not. 8 KB of prose in a `z-[60]` card pushed its own dismiss
button off-screen.
* `savingPath` was a scalar while the design deliberately allows concurrent
saves. Starting a second freed the first's row mid-transfer, and whichever
finished first cleared both; dismissing the second dialog was enough. It is
a `Set` now.
* `setUploading(false)` fired when the command settled, not when the refresh
finished, so a second click landed mid-relisting.
* `upload_files_to_container` checked the container directory before the
picker and then used the unresolved path. A modal has no time limit. It
re-checks after, which is what `docker::exec`'s doc comment already claimed.
* `wait_for_exec_exit` flattened a missing exit code to `Some(0)`, which made
the new `!= Some(0)` check unreachable by construction.
* `normalize_host_path` did not collapse repeated separators, so the lexical
system-root rule was silently absent for `C:\\Windows\…`. Not exploitable —
the resolved pass catches it — but a documented layer that does nothing is
a trap for the next caller.
* README still carried the "no host path crosses IPC in either direction"
claim the previous commit narrowed everywhere else, and HOW-TO-USE
described the hidden rule without saying it applies to folders only.
480 Rust tests, 602 frontend, no new clippy warnings. Eleven mutations against
the new tests, all killed — two of the first round survived and were rewritten:
one because `split_whitespace` already handled the case I thought I was
testing, one because I had deleted a comment rather than the behaviour.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LHL9ty7arp8FHwvE77ne7y
281 lines
11 KiB
TypeScript
281 lines
11 KiB
TypeScript
import { useCallback, useRef, useState } from "react";
|
|
import type { FileEntry } from "../lib/types";
|
|
import * as commands from "../lib/tauri-commands";
|
|
import { useAppState } from "../store/appState";
|
|
import { errorText, readableRefusal } from "../lib/refusalText";
|
|
import { formatBytes } from "../lib/formatBytes";
|
|
|
|
/**
|
|
* ## Where failures are reported
|
|
*
|
|
* Two audiences, two places, and the split is deliberate.
|
|
*
|
|
* The **initial listing** failure stays in `error`, rendered inline above the
|
|
* (empty) grid. It is on screen, it is in context, it explains why there are
|
|
* no rows, and it is not transient — it stands until the directory lists.
|
|
*
|
|
* Every **transient operation** failure — rename, create folder, upload, save
|
|
* to host — goes to `ToastHost` instead. Those used to land in the same inline `error` div, which
|
|
* is the first child of the *scrolling* list: three hundred rows down, a
|
|
* refused rename produced no visible change at all, just a rename box that
|
|
* stayed open for no stated reason. The toast host is a persistent `aria-live`
|
|
* region at `z-[60]`, i.e. the one place in the app that is above a modal and
|
|
* does not scroll away.
|
|
*
|
|
* ## Where the current directory lives
|
|
*
|
|
* `currentPath` is state (the UI renders it) *and* a ref (async work reads it
|
|
* after an await). Every long operation captures the directory it targets at
|
|
* the start and compares it against the ref at the end: a slow rename in
|
|
* `/workspace` must not drag the pane back out of `src/` because that is where
|
|
* the closure happened to be created. The ref moves at the *start* of a
|
|
* navigation rather than when the listing lands, because the question being
|
|
* asked is "where is the user going", not "what is on screen right now" — and
|
|
* it is put back if that navigation fails.
|
|
*/
|
|
export function useFileManager(projectId: string) {
|
|
const [currentPath, setCurrentPath] = useState("/workspace");
|
|
const [entries, setEntries] = useState<FileEntry[]>([]);
|
|
const [loading, setLoading] = useState(false);
|
|
const [error, setError] = useState<string | null>(null);
|
|
/**
|
|
* What just finished, for the live region — a rename or a new folder is a
|
|
* change a sighted user sees in the grid and a screen reader user does not.
|
|
*/
|
|
const [completed, setCompleted] = useState<string | null>(null);
|
|
/**
|
|
* Which host transfers are in flight.
|
|
*
|
|
* Both actions open an OS dialog and can then run for a long time on a large
|
|
* file, with nothing on screen to say so. Without this the buttons stay live:
|
|
* a second click opens a second dialog and runs a second concurrent exec
|
|
* against the same file, and a multi-gigabyte save is indistinguishable from
|
|
* a click that did nothing.
|
|
*
|
|
* `savingPaths` is a **set**, not one path. Keeping only the row being
|
|
* disabled is what makes the pane usable during a big transfer — and that is
|
|
* precisely what makes a *second* save startable, so the state has to be able
|
|
* to hold two. As a scalar it could not: starting a save on `notes.txt` while
|
|
* `big.bin` was still streaming overwrote it, so `big.bin`'s button went live
|
|
* again mid-transfer; and whichever save finished first cleared the flag for
|
|
* both. Dismissing the second dialog was enough to do it.
|
|
*
|
|
* Paths are unique within a listing, so a path is a usable key — `FilesTab`
|
|
* relies on the same fact for its row keys.
|
|
*/
|
|
const [uploading, setUploading] = useState(false);
|
|
const [savingPaths, setSavingPaths] = useState<ReadonlySet<string>>(new Set());
|
|
|
|
const currentPathRef = useRef(currentPath);
|
|
|
|
/**
|
|
* A slow listing can land after a newer one and set both the rows and the
|
|
* breadcrumb back to a directory the user already left. Same generation
|
|
* guard `useContainerMigration` uses: every async write
|
|
* checks it is still the newest before it lands.
|
|
*/
|
|
const navGeneration = useRef(0);
|
|
|
|
/**
|
|
* Report a failed operation, given the headline this hook would write and the
|
|
* raw failure behind it.
|
|
*
|
|
* The headline is what the *hook* knows ("Could not rename …"); it is a
|
|
* category, not an explanation. Some backend refusals are already a finished
|
|
* sentence written for the person reading it — a container path outside the
|
|
* roots this panel may change, a name it will not create — and those used to
|
|
* arrive as the toast's `detail`, which `ToastHost` renders as collapsed
|
|
* monospace behind a "Details" button. So the sentence that said what was
|
|
* wrong and what to do about it was hidden under a headline that said
|
|
* neither. When there is such a sentence it becomes the headline, and there
|
|
* is nothing left to hide.
|
|
*/
|
|
const report = useCallback((message: string, cause: unknown) => {
|
|
const promoted = readableRefusal(cause);
|
|
useAppState.getState().pushToast({
|
|
kind: "error",
|
|
message: promoted ?? message,
|
|
detail: promoted ? undefined : errorText(cause),
|
|
});
|
|
}, []);
|
|
|
|
const navigate = useCallback(
|
|
async (path: string) => {
|
|
const mine = ++navGeneration.current;
|
|
const previous = currentPathRef.current;
|
|
currentPathRef.current = path;
|
|
setLoading(true);
|
|
setError(null);
|
|
try {
|
|
const result = await commands.listContainerFiles(projectId, path);
|
|
if (navGeneration.current !== mine) return;
|
|
setEntries(result);
|
|
setCurrentPath(path);
|
|
} catch (e) {
|
|
if (navGeneration.current !== mine) return;
|
|
// The move did not happen, so the pane is still where it was — the ref
|
|
// has to agree with the breadcrumb or the next operation will decide
|
|
// it targeted a directory nobody is looking at.
|
|
currentPathRef.current = previous;
|
|
setError(String(e));
|
|
} finally {
|
|
if (navGeneration.current === mine) setLoading(false);
|
|
}
|
|
},
|
|
[projectId],
|
|
);
|
|
|
|
const goUp = useCallback(() => {
|
|
const here = currentPathRef.current;
|
|
if (here === "/") return;
|
|
const parent = here.replace(/\/[^/]+$/, "") || "/";
|
|
navigate(parent);
|
|
}, [navigate]);
|
|
|
|
const refresh = useCallback(() => {
|
|
navigate(currentPathRef.current);
|
|
}, [navigate]);
|
|
|
|
/**
|
|
* Rename in place. `newName` is a bare name — Rust rejects anything with a
|
|
* `/` in it, so this can never turn into a move. Resolves true on success so
|
|
* the caller knows whether to leave edit mode.
|
|
*/
|
|
const renameEntry = useCallback(
|
|
async (entry: FileEntry, newName: string) => {
|
|
const trimmed = newName.trim();
|
|
if (!trimmed || trimmed === entry.name) return true;
|
|
const target = currentPathRef.current;
|
|
try {
|
|
await commands.renameContainerPath(projectId, entry.path, trimmed);
|
|
setCompleted(`Renamed "${entry.name}" to "${trimmed}".`);
|
|
if (currentPathRef.current === target) await navigate(target);
|
|
return true;
|
|
} catch (e) {
|
|
report(`Could not rename "${entry.name}"`, e);
|
|
return false;
|
|
}
|
|
},
|
|
[projectId, navigate, report],
|
|
);
|
|
|
|
const createFolder = useCallback(
|
|
async (name: string) => {
|
|
const trimmed = name.trim();
|
|
if (!trimmed) return true;
|
|
const target = currentPathRef.current;
|
|
try {
|
|
await commands.createContainerDirectory(projectId, target, trimmed);
|
|
setCompleted(`Created "${trimmed}".`);
|
|
if (currentPathRef.current === target) await navigate(target);
|
|
return true;
|
|
} catch (e) {
|
|
report(`Could not create "${trimmed}"`, e);
|
|
return false;
|
|
}
|
|
},
|
|
[projectId, navigate, report],
|
|
);
|
|
|
|
/**
|
|
* Copy host files into the directory on screen.
|
|
*
|
|
* The picker is opened by **Rust**, not here — `upload_files_to_container`
|
|
* shows it, reads what the user chose and never lets a host path near IPC.
|
|
* So this passes a directory and gets back an outcome; `null` means the user
|
|
* dismissed the dialog, which is not a failure and says nothing.
|
|
*
|
|
* One dialog can select several files and they need not agree, hence two
|
|
* lists. Every failure is reported, because "3 of 5 uploaded" without saying
|
|
* which two is not a report. The listing is refreshed once, at the end, and
|
|
* only if the user is still looking at the directory that was targeted.
|
|
*/
|
|
const uploadFiles = useCallback(async () => {
|
|
const target = currentPathRef.current;
|
|
setUploading(true);
|
|
try {
|
|
let outcome;
|
|
try {
|
|
outcome = await commands.uploadFilesToContainer(projectId, target);
|
|
} catch (e) {
|
|
// A failure *before* the picker: no container, not running, or a
|
|
// directory this pane may not write to. One toast, not one per file.
|
|
report("Could not upload", e);
|
|
return;
|
|
}
|
|
if (!outcome) return;
|
|
for (const failure of outcome.failures) {
|
|
useAppState.getState().pushToast({ kind: "error", message: failure });
|
|
}
|
|
if (outcome.uploaded.length === 0) return;
|
|
// The directory is named, not implied. `target` is captured at click time
|
|
// and the picker is a modal OS dialog — the user has all the time in the
|
|
// world to browse somewhere else while it is open, and the files land
|
|
// where they started. "Uploaded 2 files." in front of a grid that does
|
|
// not contain them is a worse answer than no message at all.
|
|
const count = outcome.uploaded.length;
|
|
setCompleted(
|
|
`Uploaded ${count === 1 ? "1 file" : `${count} files`} to ${target}.`,
|
|
);
|
|
if (currentPathRef.current === target) await navigate(target);
|
|
} finally {
|
|
// Around the *whole* body, refresh included. Clearing it the moment the
|
|
// command settled put the button back before the re-listing had run, so
|
|
// a second click landed mid-refresh on a grid that was still the old one.
|
|
setUploading(false);
|
|
}
|
|
}, [projectId, navigate, report]);
|
|
|
|
/**
|
|
* Save one file out to the host, with Rust opening the save dialog.
|
|
*
|
|
* No refresh: nothing in the container changed. The save dialog is also what
|
|
* asks about overwriting an existing host file, which is why the backend has
|
|
* no collision handling of its own to get wrong. `null` is a dismissal.
|
|
*/
|
|
const saveToHost = useCallback(
|
|
async (entry: FileEntry) => {
|
|
setSavingPaths((live) => new Set(live).add(entry.path));
|
|
try {
|
|
const bytes = await commands.downloadContainerFile(projectId, entry.path);
|
|
// `0` is a real answer — an empty file saved is a success — so this
|
|
// tests for the dismissal sentinel, not for falsiness.
|
|
if (bytes === null) return;
|
|
setCompleted(`Saved "${entry.name}" (${formatBytes(bytes)}).`);
|
|
} catch (e) {
|
|
report(`Could not save "${entry.name}"`, e);
|
|
} finally {
|
|
// Remove only this one. A save that finishes while another is still
|
|
// streaming must not re-enable the other's row.
|
|
setSavingPaths((live) => {
|
|
const next = new Set(live);
|
|
next.delete(entry.path);
|
|
return next;
|
|
});
|
|
}
|
|
},
|
|
[projectId, report],
|
|
);
|
|
|
|
return {
|
|
currentPath,
|
|
entries,
|
|
loading,
|
|
/** Inline, in-context: why the listing on screen is empty. */
|
|
error,
|
|
/** What the last operation finished doing, for the live region. */
|
|
completed,
|
|
setError,
|
|
navigate,
|
|
goUp,
|
|
refresh,
|
|
renameEntry,
|
|
createFolder,
|
|
uploadFiles,
|
|
saveToHost,
|
|
/** A host transfer is in flight — see the state declarations above. */
|
|
uploading,
|
|
savingPaths,
|
|
};
|
|
}
|