Repository navigation
Conversation
The branch timeline's "…" menu gains an "Add image" item, below "Write note", that picks images and adds them straight to the branch timeline. Like "Write note" it is offered on every branch: images live in the store, not the worktree. macOS (Tauri) opens the native open panel through the dialog plugin's `open()`, multi-select and filtered to PNG/JPEG/GIF/WebP. A file `<input>` would not do here: wry's `runOpenPanelWithParameters` ignores `accept`, so its panel cannot be filtered to images. The plugin and its `dialog:default` capability (which grants `allow-open`) are already in place, and the unsandboxed app needs no entitlement or Info.plist key for NSOpenPanel, so no config changes. iOS has no Tauri target: Staged runs there as the web build in mobile Safari. The item clicks a hidden file input with an explicit image `accept` list, synchronously inside the menu item's click so the tap's user activation carries through. iOS then presents its own source sheet — Photo Library (PHPicker) or Files (the document picker, filtered to the accepted types), plus the Take Photo option iOS always adds for image inputs — so no custom action sheet is layered on top. Safari owns photo-library access, so no NSPhotoLibraryUsageDescription is involved. Picked images reuse the branch card's drag-and-drop pipeline, the existing way images reach a branch timeline directly: the drop handler's image branch is extracted into `addTimelineImages`, which both use. Desktop paths go through `create_image` exactly as a drop does; web files go through `create_image_from_data`, the endpoint the web build already uploads through (`create_image` is desktop-only). Both store via `store_sniffed_image` with no pending session, so a picked image appears on the timeline immediately, like a dropped one. The chat composer's attachment path was not reused: its images are pending, hidden until a session claims them. Cancelling either picker does nothing. Per-file storage failures toast like a rejected drop; failing to open the desktop panel goes through the card's `notifyError`. The chunked File→base64 loop, duplicated in ImageAttachment and SessionChatPane, moves to `fileToBase64` rather than gaining a third copy. `IMAGE_ACCEPT` is split out of `ATTACHMENT_ACCEPT` and `IMAGE_EXTENSIONS` is exported, so the picker filters come from the lists the store's other entry points already use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to the branch card's "Add image": the review found that `addTimelineImages`, shared by "Add image" and image drops, started every store at once, showed nothing until the whole batch had landed, and toasted once per rejected file. Images are now stored one at a time, in pick/drop order (`storeImagesInOrder`, new in timelineImages.ts). On the web build each store holds the whole file in memory several times over (ArrayBuffer, binary string, base64, JSON body). One at a time, a 10–20 photo pick in mobile Safari only holds one file's worth at once, not the whole batch. It also fixes timeline order: an image's position is the time the server inserted it, so parallel uploads landed in whichever order they finished, smallest first. Local `create_image` copies are fast, so drops lose little. Each image gets an "adding..." placeholder row as soon as the batch starts, like the rows text drops already get. Images publish no change event, so the single refresh after the batch is still what shows them, and on iOS that can take seconds. The placeholder makes the tap visibly do something, so the user doesn't pick again and store duplicates. A rejected image's row goes as soon as it fails. A stored one keeps its row until the refresh lands, so it never blinks out in between. I preferred this over refreshing after every store: that would re-fetch the timeline and re-run the git-state refresh once per image. To render them, the timeline's `pendingDropNotes` becomes `pendingAdds`. Each entry carries its row type: `generating-note` for text drops, as before, and a new `pending-image` TimelineRow type (spinner, image colours). The `isLocal` gate on the prop is gone. Only local branches can be dropped on, so the gate never hid anything, but "Add image" is offered on every branch, and remote branches need the placeholders too. Keys come from a counter, not the file name, because iOS can hand back several files all named "image.jpg". A batch's rejections are reported in one toast through `formatAttachErrors`, the composer's batch formatter, instead of one toast per file. This covers drops and picks, since they share the path. In the composer (`ImageAttachment`, `SessionChatPane`), the `fileToBase64` read moves inside the `try`. Before, a failed `file.arrayBuffer()` (e.g. `NotReadableError`) aborted the batch without setting `attachError` and skipped the input reset, so re-picking the same file did not fire `change`. Now it counts as that file's rejection. Tests cover the sequential store order, continuing past rejections with per-index callbacks, and the one-message batch report. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to d370f72 ("add timeline images one at a time, with placeholders"). That commit's design rested on a wrong premise: it said images publish no change event, so the single refresh after a batch was what showed them. They do publish one. `Store::create_image` publishes `StoreChange::Branch` for any image with a branch and no session, and the card's `timeline-invalidated` handler force-reloads on it, on desktop and over `/api/events` on the web. So each stored image already showed as a real row shortly after its own store, while its "adding..." placeholder stayed at the bottom until the whole batch finished. In a 10-photo iOS pick, every finished photo showed twice for the rest of the upload. The commit's trade-off was wrong too: it said it avoided a timeline and git-state refresh per image, but those refreshes were already happening through the change feed. Each placeholder now records its stored `Image` id (both `createImage` and `createImageFromData` already resolve to it). `pruneLandedImages` drops placeholders whose id is already in `timeline.images`. It runs wherever the card applies a timeline, next to `prunePendingSessionItems`, through a shared `prunePlaceholders`. So the reload that brings an image in also removes its placeholder, in the same update. It also runs once when the id is recorded, in case that reload landed first. As with pruned session placeholders, the timeline skips the slide-out for a row that leaves because its image arrived, so the image replaces it in place. Rejected images and finished text drops still slide out. `pendingAdds` moves above the card's synchronous cache hydration, which now prunes it. The batch-end removal stays as a fallback for an image whose row never arrives, but it no longer races. A load that a newer one supersedes returns early (the `timelineLoadVersion` check), so the change-feed reload for the last image could make the batch's `await loadTimeline()` return before anything was shown. The placeholders were then removed before the newer load landed, so they blinked out. `loadTimeline` now records its newest run, and the batch waits on `timelineLoadsSettled`, which follows superseding loads until the last one has settled. Only then does it drop what's left. Batches are also serialized per card. `createImageStoreQueue` chains each batch's `storeImagesInOrder` after the previous one. Before, a pick or drop made during an upload stored alongside it, so mobile Safari held two files' worth of base64 at once and the batches interleaved on the timeline. A queued batch's placeholders still appear as soon as it's picked. Only the storing is chained, so one batch's final refresh doesn't hold up the next batch. `storeImagesInOrder` takes `{ onStored, onRejected }` handlers, and `onStored` gets the stored image. Tests cover pruning (landed, stored but not landed, not yet stored, and returning the same array when nothing changed), cross-batch ordering, per-batch rejections, and the queue continuing past a failed batch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to 2da771b ("swap each image placeholder for its row as it lands"), applying that commit's four review suggestions. Behaviour is otherwise unchanged. `addTimelineImages` wraps the store and settle sequence in try/catch/finally. The queue already survived a batch that rejected (a handler throwing), but the card did not: the `await` threw past the toast and the batch-end removal. Every placeholder from the failing image on then spun forever, and because callers `void` the call, the rejection went unhandled. The placeholders are now removed in `finally`, and the error is caught and shown through the card's `notifyError`. The fallback's `await loadTimeline()` is now `void loadTimeline(); await timelineLoadsSettled();`. `loadTimeline` records its run as `latestTimelineLoad` synchronously, and the settle wait's first pass awaits that run or a newer one, so the extra await added nothing. The fallback relies on the settle wait, and the code now says so. The image store queue is module-level. `storeTimelineImages`, one `createImageStoreQueue()` exported from timelineImages.ts, replaces the per-card instance. The one-file-at-a-time guarantee used to end when a card remounted mid-upload, for example on a project switch. The old instance kept uploading, and a pick on the new one started a second, parallel queue, so mobile Safari held two files' base64 at once. The shared queue keeps the guarantee across remounts and across cards. timelineImages.ts exports a `PendingAdd` type for the "adding..." rows. BranchCard's `pendingAdds` state, BranchTimeline's `pendingAdds` prop and `pruneLandedImages` all use it, so the row shape is no longer written out twice and constrained structurally a third time. The `pruneLandedImages` tests now build full `PendingAdd` rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 4e5afedc57
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+45
to
+48
| for (const [index, { name, store }] of images.entries()) { | ||
| let image: Pick<Image, 'id'>; | ||
| try { | ||
| image = await store(); |
There was a problem hiding this comment.
When multiple small images are stored within the same second, awaiting each store sequentially does not preserve their visible pick order. The backend returns branch images in descending created_at order (store/images.rs), while BranchTimeline.svelte truncates each millisecond timestamp to seconds and assigns every image the same secondary order; the stable sort therefore keeps the backend's newest-first order, so a pick of A then B commonly renders as B then A. Preserve millisecond timestamps or provide an insertion-order tiebreaker.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an Add image item to the branch card's "…" menu, below "Write note". It picks images and adds them straight to the branch timeline. Like "Write note", it's offered on every branch, because images live in the store, not the worktree.
open(). It allows multi-select and is filtered to PNG/JPEG/GIF/WebP. A file<input>can't be used here because wry ignoresaccept. No config or capability changes are needed.acceptlist. iOS then shows its own Photo Library / Files / Take Photo sheet.Picked images go through the same pipeline as image drops. That pipeline is now
addTimelineImages, with the image-store logic intimelineImages.ts.Upload behaviour (picks and drops)
storeTimelineImages). Mobile Safari then only holds one file's base64 at a time, and images land on the timeline in pick order. Batches made during an upload wait their turn, and this still holds if a card remounts mid-upload.pending-imagerow right away. The timeline'spendingDropNotesbecomespendingAdds, which covers both text-drop and image rows. When the image's real row arrives through the change-feed reload, its placeholder is swapped for it in place (pruneLandedImages). A fallback removes any placeholder left over once all timeline loads have settled (timelineLoadsSettled).formatAttachErrors, not one toast per file. If the batch itself throws, its placeholders are cleaned up and the error goes tonotifyError.Shared cleanups
ImageAttachmentandSessionChatPanemoves tofileToBase64. In the composer, the read now happens inside thetry, so a failedarrayBuffer()counts as that file's rejection and still resets the input.IMAGE_ACCEPTis split out ofATTACHMENT_ACCEPT, andIMAGE_EXTENSIONSis exported, so the picker filters use the lists the store already uses.Testing
fileToBase64, the batch error formatting, sequential store order, rejections, queue ordering across batches and past a failed batch, and placeholder pruning.🤖 Generated with Claude Code