canvas: what the review of #97 found, fixed - #98
Jing-yilin wants to merge 6 commits into
Conversation
The board's frame says `color-scheme: light` on the element, and the
`:root{color-scheme:dark;color:#000}` injected after every board's doctype
is gone. The PR's measurement had tried the element-side value that matches
the canvas, not the one that matches the board; measured again in Chrome
153, `light` on the `<iframe>` composites transparent. Nothing is written
into a board's markup now, and an injected class-level rule no longer
outranks a board's own `html{}`. The design note says so.
Cmd+/ no longer flips tldraw to its light theme under a black rail: the
`toggle-dark-mode` action is deleted from the overrides, since the ground
remap, the welcome board and the panel tokens are all dark only.
Attachments are the four image types the CLIs read, on both sides of the
wire. `image/svg+xml` was accepted and served back on the dev server's
origin, where every /__sp endpoint writes, and the strip opens each tile as
a top-level document.
The /run body is collected as bytes and decoded once: a character split
across two chunks decoded to U+FFFD, and a CJK message or file name sits
after megabytes of base64. The cap is bytes too, and its message says the
same 24 MB the panel does. Per-run image folders live under one folder per
server named by its pid: removed with the server, and a folder left by a
server that was killed is swept at the next start.
In the panel: a send is held until the server answers, so a repeated Enter
no longer posts the message and its images twice; a fetch that rejects
outright surfaces as the send error instead of an unhandled rejection; the
tray checks the server's 20-image and size limits over the whole tray and
reports a file that could not be read; a box emptied by typing drops the
`<br>` that kept the placeholder away; Enter inside an IME composition is
the editor's; and the data URL the reader produced is kept whole rather
than rebuilt from megabytes of base64 on every keystroke.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deploying super-prototyping with
|
| Latest commit: |
8f1bfe2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e10a64fb.super-prototyping.pages.dev |
| Branch Preview URL: | https://fix-canvas-geist-ui-review.super-prototyping.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db61e6cfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (attached.length + picked.length > 20) | ||
| return setSendError("that is too many to attach; keep it to 20 images"); | ||
| const held = attached.reduce((n, i) => n + i.url.length * 0.75, 0); | ||
| if (held + picked.reduce((n, f) => n + f.size, 0) > 24_000_000) |
There was a problem hiding this comment.
When a second paste, drop, or picker action occurs before the first batch's FileReader.onload callbacks finish, attached still excludes that first batch, so both the 20-image and 24 MB checks can pass independently. The completed reads can then populate an oversized tray, and sending it fails only after processing the files when the server returns 400 or 413; track pending count and bytes immediately when reads start so subsequent additions include them.
Useful? React with 👍 / 👎.
| } catch { | ||
| fs.rmSync(path.join(os.tmpdir(), name), { |
There was a problem hiding this comment.
On a multi-user Unix host, process.kill(pid, 0) throws EPERM when the PID is alive but owned by another user, not only when the process is gone. This catch consequently treats another user's live canvas as stale and attempts to remove its directory; that normally throws EACCES and aborts this server's startup. Only ESRCH should trigger deletion, while EPERM should leave the folder alone.
Useful? React with 👍 / 👎.
The tray keeps the reader's data URL, and upstream's dedupe of a picture pressed twice compares that instead of the sliced base64. Two things Codex found on #98 are folded in: the limits count what is still being read, so a second paste during the first one's read sees the whole tray, and the tmp sweep removes a folder only when its pid is gone (ESRCH), not when it belongs to another user (EPERM). And the tray is merged in a functional update, so a pick that finishes reading second no longer overwrites the one that finished first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b6e586f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const already = | ||
| attached.find((i) => i.url === url) ?? fresh.find((i) => i.url === url); | ||
| const image = already ?? { | ||
| n: nextN.current++, |
There was a problem hiding this comment.
When two concurrent attachment reads contain identical bytes, both invocations inspect the stale attached snapshot and assign different numbers. The functional updater then drops one duplicate against the current tray, but said still contains the discarded number; for canvas-originated attachments, this inserts a chip such as #2 while only image #1 is sent to the agent. Resolve the existing image from the current tray atomically before creating the chip and allocating its number.
Useful? React with 👍 / 👎.
| // server's goes with the server, and at start the folders of servers that were killed | ||
| // rather than closed go too: a pid nothing answers on is a server that is gone, and a | ||
| // run's images have no use past the server holding its events. | ||
| const chatDir = path.join(os.tmpdir(), `sp-chat-${process.pid}`); |
There was a problem hiding this comment.
On a shared host, this predictable path can be pre-created by another user as a writable directory or symlink before the server receives its first image. Because the later recursive mkdirSync does not verify ownership or reject symlinks, attachment files can be written beneath the attacker's directory and become readable to that user. Create the server directory atomically with a private mode, such as via mkdtemp, rather than deriving an unchecked path solely from the PID.
Useful? React with 👍 / 👎.
| if ((error as NodeJS.ErrnoException).code === "ESRCH") | ||
| fs.rmSync(path.join(os.tmpdir(), name), { | ||
| recursive: true, | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
On a multi-user system temp directory, a killed canvas belonging to another user produces ESRCH, but its sp-chat-* directory is still not removable by this process. The new ESRCH check therefore reaches this synchronous rmSync, which throws EACCES/EPERM and aborts Vite startup. Even after distinguishing live foreign processes, stale foreign folders must be skipped when deletion is not permitted.
Useful? React with 👍 / 👎.
Attaching runs one add at a time: two picks of one picture in flight together each numbered it, and the second put a chip in the sentence for a tile the tray had dropped. The tray an add reads is a ref written as it is set, so the add queued behind it sees it. The pending counters and the updater's second dedup go with that, and the tray weighs its files by size rather than by data-URL length. A drop or paste of a type the agents cannot read now says so instead of doing nothing, and the four types and the two limits are said once, in agents.ts, for both sides. Safari reports the Enter that confirms an IME candidate after composition has ended, with only keyCode 229 to mark it; the guard reads that too. The placeholder keys off the draft state through a data attribute rather than the box being wiped in onInput, which was throwing away native undo and eating a leading Shift+Enter. A run's image files go when its child closes or fails to spawn, and the image route serves the bytes the run already holds. The server's close hook went: Vite restarts by building the new server, same pid, before closing the old, so the hook was deleting the running server's folder; Ctrl+C never fired it at all. The dead-pid sweep is the cleanup, and skips a folder it cannot remove rather than failing the server on a shared tmp. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9dfd4c8 to
2b19654
Compare
A path made of the pid alone is one anyone on a shared tmp could have put a folder or a link at first, and the run's files would then have gone under theirs. mkdtemp makes it private and atomically, and the sweep reads the pid off the name it makes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc449a0be1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const picked = given.filter((f) => IMAGE_TYPES.includes(f.type)); | ||
| // Both limits are the server's (agents.ts), said here before anything is read or sent, | ||
| // and over the whole tray, since the server sees the whole tray and not this pick. | ||
| if (tray.current.length + picked.length > MAX_IMAGES) |
There was a problem hiding this comment.
When the tray already contains 20 images, reattaching one of those exact images—such as pressing the canvas attach button again to cite an existing tile—is rejected here as image 21 before the data-URL deduplication at line 472 runs. This breaks the documented one-number-per-picture behavior even though the request would add no image; apply the limit only to images that are actually fresh.
Useful? React with 👍 / 👎.
| if (size > MAX_IMAGE_BYTES * 2) | ||
| return send( | ||
| 413, | ||
| "too much attached; keep the images under about 32 MB", | ||
| `too much attached; keep the images under about ${MAX_IMAGE_BYTES / 1_000_000} MB together`, |
There was a problem hiding this comment.
When a caller bypasses the current composer, this 48 MB request-body check does not enforce MAX_IMAGE_BYTES: for example, roughly 32 MB of decoded images becomes about 43 MB of base64 and passes, after which the server writes and retains the oversized payload. The handler claims the shared 24 MB limit also protects non-browser clients, so it should sum the decoded image sizes and reject totals above MAX_IMAGE_BYTES after parsing.
Useful? React with 👍 / 👎.
…server counts the bytes The limits are over what a pick adds: the same picture pointed at again on a full tray kept its number rather than being refused as one more, so the reads come first and the count and the weight are of what is new. The server's body cap was the panel's limit in base64; a client that is not the panel now meets the same limit in the bytes the files decode to. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e34d264f1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // twenty-first, so the limits are over what is new. Still numbered in the order they | ||
| // were picked rather than the order the reads came back in, so three files chosen at | ||
| // once are #1, #2, #3 as they appear in the dialog. | ||
| const urls = await Promise.all(picked.map(readFile)); |
There was a problem hiding this comment.
When a user selects or drops a very large image, Promise.all(picked.map(readFile)) loads and base64-expands the entire file before the 24 MB check runs. A several-hundred-megabyte file can therefore consume substantially more memory and freeze or crash the tab even though it will always be rejected afterward; reject any individually oversized file, and other batches that cannot fit, before invoking FileReader.
Useful? React with 👍 / 👎.
A read is the whole file in memory, a third larger, and a drop of a few hundred megabytes was read in full to be refused after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Fixes for the findings of the review of #97, targeted at its branch so they land with it. Synced with the branch's later commits (attach-from-canvas), and reviewed a second time after the sync; the second review's fixes are in the last two commits.
Board backdrop.
CanvasFileShapeUtilsetscolor-scheme: lighton the<iframe>element and no longer injects:root{color-scheme:dark;color:#000}after each board's doctype. The PR's measurement had tried the element-side value that matches the canvas rather than the one that matches the board. Measured again in Chrome 153 (lightandnormalon the element composite transparent;darkand no rule paint white), so the design note is corrected. Nothing is written into a board's markup, a board's ownhtml{}rule is no longer outranked by an injected class-level one, and the doctype is no longer an anchor.Theme toggle.
toggle-dark-modeis deleted from the UI overrides. Cmd+/ flipped tldraw to light while the ground remap, the welcome board and the panel tokens stayed dark, until the next reload forced dark again.Attachments. Accepted types are
png,jpeg,gifandwebp, which is what the CLIs read; an SVG was accepted and served back on the dev server's origin, where every/__spendpoint writes. The four types, the 20-image cap and the 24 MB cap are said once, inagents.ts, and both sides read them. A drop, paste or canvas hand-over of another type says so instead of silently doing nothing.Server. The
/runbody is collected asBuffers and decoded once, so a multi-byte character across a chunk boundary no longer decodes to U+FFFD; the cap is in bytes and its 413 says the same 24 MB the panel does. A run's image files exist for its child only: written under a private per-server folder made withmkdtemp, removed when the child closes or fails to spawn, and the image route serves the bytes the run already holds. Folders left by a killed server are swept at the next start (a dead pid; another user's live one is kept, and one this process cannot remove is skipped). There is no hook on server close: Vite restarts by building the new server, same pid, before closing the old, so such a hook deleted the running server's folder, and Ctrl+C never fired it anyway.Panel. Adds run one at a time, reading the tray from a ref written as it is set: two picks of one picture in flight together each numbered it, and the second put a chip in the sentence for a tile the tray had dropped. A send is held until the server answers (no double post on a repeated Enter); a rejected fetch in send, history, the agents list and the commands probe surfaces as the send error; the placeholder keys off the draft through a
data-emptyattribute rather than the box being wiped inonInput, which threw away native undo and ate a leading Shift+Enter; Enter during IME composition is left to the editor, including Safari's confirming Enter that arrives aftercompositionendwith onlykeyCode 229to mark it; the tray weighs its files by size; and the reader's data URL is kept whole rather than rebuilt per keystroke.Also: the stale
FigmaMark.tsxreference in the chat-panel note, its paragraph on where image files live, and anapplyFrametest forstart.imagesandtool_done.shots.Verified:
tsc -b,oxlintandvitest(141 tests) pass. Smoke-tested against a running dev server: SVG and a 21st image refused, a 600 KB CJK body parses, a 49 MB body gets the 413, dead-pid folder swept and a live one kept; with a stub CLI and with no CLI on PATH, the run's folder is gone after the child and/image/1still serves the exact bytes.Not changed, from the cut lists: the collapsed panel's
display:none, hand-drawn icons vsgeist-icons,no-storeon the image routes, shots pinned in memory,run.imagesholding base64 (codex's stdin needs the bytes, and the image route now serves from it), theShotunion, the three older hand-rolled body readers outside this PR's routes, and a multipart transport for attachments.🤖 Generated with Claude Code