Picture UI improvements - #2458
Open
rmunn wants to merge 35 commits into
Open
Conversation
Replace the pencil affordance on a picture with a reusable three-dots menu offering Edit, Download, and Delete, each wired to the existing behaviors (edit dialog, download, delete-with-confirmation). The menu is a new PictureActionsMenu component built on the responsive-menu primitive (dropdown on desktop, drawer on mobile) and is also opened by a long-press of the image so touch users don't have to hit the small target. Download logic is extracted into a shared picture-actions.ts (used by the menu and the edit dialog); PicturesEditor's delete-with-confirm is generalized so the menu and the dialog share it. The picture click still opens the edit dialog for now. Test infra: EntryViewComponent.waitForEntryLoaded() keyed off `.i-mdi-dots-vertical`, which is no longer unique now that each picture renders a three-dots menu; switch it to the lexeme-form field, a stable per-entry "loaded" signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clicking (or tapping) a sense picture now opens a new PictureViewerDialog instead of the edit dialog — the three-dots menu (added previously) already reaches the editor. The viewer shows the picture sized to the viewport but never larger than its native resolution (PictureImage gains a size="full" variant; the dialog shrinks to fit). It carries the same three-dots actions menu (Edit/Download/Delete) acting on the current picture, left/right arrows to move between pictures when the sense has more than one, and below the image the current picture's non-empty captions shown read-only with writing-system abbreviation labels. The shown picture is tracked by id so navigation and deletion stay in sync with the reloaded entry; the dialog closes if that picture is gone. PictureImage's click now calls onView (aria-label "View Picture"); the menu's Edit still uses onEdit. The viewer's actions menu is disabled while an operation is in flight, matching the edit dialog's double-invoke guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an entry-view-scoped image service (context-based) that fetches and decodes each picture mediaUri once and hands out a shared blob object URL. Two pictures with the same mediaUri — or the same picture shown in both the field and a dialog — now load a single time instead of once per PictureImage instance. The service is initialized in EntryEditor (the entry view, which remounts per entry), so its cached object URLs are revoked when that view is torn down. PictureImage loads through it (preload in an $effect, reactive read via a SvelteMap-backed get in a $derived), falling back to a component-local cache when rendered without an entry-view scope. EditPictureDialog and PictureViewerDialog get the cache transitively since they render PictureImage within the EntryEditor subtree. A #disposed guard prevents a load that finishes after teardown from minting an object URL that would never be revoked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `$effect` would run twice if the `picture` object got replaced by a different one that had the same `mediaUri`. By moving `mediaUri` into a `$derived` we avoid the double run of the `$effect`.
github-actions
Bot
added
the
💻 FW Lite
Contributor
📝 WalkthroughWalkthroughPicture editing now supports project-scoped image caching, on-demand loading, long-press actions, fullscreen viewing with navigation and captions, immediate local CRUD updates, shared downloads, updated demo behavior, localization, and expanded Playwright coverage. ChangesPicture experience
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Pictures no longer auto-load. The entry-scoped image cache is the sole gate: a picture whose mediaUri isn't cached shows a "Click to load" / "Tap to load" placeholder (styled like the error placeholder) and is fetched only when the user clicks/taps it. Once a mediaUri is cached, every picture sharing it — and the edit dialog / fullscreen viewer — display it immediately. No backend change. image-service: add a 'not-loaded' state; get() reports it for an uncached mediaUri; the former auto-preload is now an on-demand load() called from the click handler. PictureImage drops its auto-load effect, renders the placeholder, and its click loads when not-loaded else opens the viewer; the three-dots menu stays available even before the image loads. Also saves the investigation into true local-vs-remote detection (which would have required a 🔴 MiniLcm media-API change) in picture-loading-findings.md, explaining why the cache-based approach was chosen instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The cache lived on EntryEditor, which remounts per entry ({#key entry.id}), so it was
torn down whenever you switched entries — a picture you clicked to load reverted to
"click to load" after navigating away and back.
Make it a project-scoped singleton via projectContext.getOrAdd instead: one ImageService
per open project, created lazily on first use, kept for the project's lifetime, and
disposed (object URLs revoked) when the project context is destroyed via an $effect
cleanup riding getOrAdd's $effect.root. useImageService now resolves it from the project
context (falling back to a component-local cache only outside a project, e.g. stories);
EntryEditor no longer initializes it.
A loaded image now stays cached across entry navigation and displays without re-clicking.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In PictureViewerDialog the caption block is now a clickable toggle. Uncollapsed (the default, unchanged behavior) shows all non-empty captions; clicking collapses to just the first non-empty caption with its text line-clamped to one line, and clicking again expands again. Each picture starts expanded (navigation resets the state). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in new downloadIfMissing param to getFileStream API
Now that getFileStream takes a downloadIfMissing flag, implement the original design: a picture already cached locally loads automatically, and only a picture that would have to be fetched from the remote media service shows the "Click to load" / "Tap to load" placeholder. ImageService.ensureLocal (called from an $effect on render) probes with downloadIfMissing: false — a local file displays immediately; a NotFound result means "not downloaded" and surfaces the placeholder. Clicking it calls download() (downloadIfMissing: true) to fetch and display. The project-scoped cache still means a mediaUri loaded once shows everywhere. The in-memory demo API now simulates a remote media service: pre-seeded pictures aren't available locally until downloaded once, while uploaded files are local. Tests updated to match (uploads auto-load; the demo's pre-seeded pictures click-to-download). Also drops a stray third argument to the shared downloadPicture helper in EditPictureDialog, and updates picture-loading-findings.md to record the final design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was an accidental commit of a planning document, which doesn't belong in the PR.
`pictures` is now $bindable, so add/delete/update mutate the sense's picture list directly
for immediate UI feedback instead of waiting for the change to round-trip back through the
entry-changed reload:
- addPicture appends the new picture, deletePicture filters it out, submitEdits swaps it.
- Each reassigns `pictures` (not in-place push/splice) so the parent's bound setter fires.
SenseEditorPrimitive binds via a getter/setter (bind:pictures={() => sense.pictures ?? [],
(v) => (sense.pictures = v)}), matching the function-binding idiom used elsewhere; the
getter keeps the longstanding guard against a runtime-undefined sense.pictures. Pictures
still persist through their own api.createPicture/updatePicture/deletePicture calls (not
the sense onchange flow), so nothing double-persists.
The edit dialog now retains the last picture it showed (lastEditedPicture) so it stays
mounted for its close animation even though a delete removes the picture from `pictures`
sooner than the reload used to.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The caption block is clickable to collapse/expand but had no affordance signalling it. Add a disclosure chevron (i-mdi-chevron-down) at its trailing edge: it points down while collapsed (more captions available below) and rotates to point up while expanded, with a transition. Standard show-more/less indicator; the button keeps aria-expanded for AT. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rmunn
marked this pull request as ready for review
July 21, 2026 03:57
Contributor
Author
|
@CodeRabbit help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai rate limit |
Contributor
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 45 minutes. |
Contributor
Author
|
@coderabbitai Since this is the first PR I've modified today, I don't understand why I am hitting a rate limit. The explanation states "CodeRabbit enforces per-developer PR review limits for each organization", but the first PR I submitted today was this one. Why are you registering a rate limit? Previous pushes to this branch were while it was in draft status, which skips reviews. Did those commits still count against the review rate limit even though no review was triggered? That seems like a bug in your code. |
Contributor
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 42 minutes. |
Contributor
Author
|
Heh, the AI model wasn't programmed to respond to that kind of question. |
Brings in the picture sync improvments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/viewer/src/lib/entry-editor/field-editors/image-service.svelte.ts`:
- Around line 60-86: Update `#load` to wrap both api.getFileStream and
file.stream.stream processing in a try/catch. On any exception, set this.#cache
for mediaUri to an error state with the available error detail, then re-throw
the original error so global handling remains unchanged; preserve the existing
disposed checks and successful/not-downloaded paths.
In
`@frontend/viewer/src/lib/entry-editor/field-editors/PictureViewerDialog.svelte`:
- Around line 37-52: Reset the collapsed state when the viewer is opened for a
picture through the direct openViewer flow, not only in showPrevious and
showNext. Ensure every newly opened picture starts with collapsed set to false
while preserving the existing navigation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2a21187-26a8-4872-8c42-8ceaad04853b
📒 Files selected for processing (19)
frontend/viewer/src/lib/entry-editor/field-editors/EditPictureDialog.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureActionsMenu.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureImage.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureViewerDialog.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.sveltefrontend/viewer/src/lib/entry-editor/field-editors/image-service.svelte.tsfrontend/viewer/src/lib/entry-editor/field-editors/picture-actions.tsfrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/tests/pages/entry-view.component.tsfrontend/viewer/tests/ui/sense-pictures.test.ts
If getFileStream rejected or the stream/blob processing threw, #load let the exception propagate but left the cache on 'loading', so the picture spun forever. Wrap the fetch + stream processing in a try/catch that sets an error state (with the exception's message as detail) and then re-throws, so the UI recovers while the global handler still reports it. The disposed checks and the success / not-downloaded paths are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
image-service already permits the retry (download() proceeds from any state except in-flight/loaded, so it re-attempts from an error); clarified its doc to say so. The gap was in the UI: PictureImage only routed clicks to download from the 'not-downloaded' state. Make the error state clickable too — clicking (or tapping) an errored picture calls download() to retry — and show a "Click to retry" / "Tap to retry" affordance on the error placeholder. Works in the field, dialog, and viewer (retry is non-mutating, so allowed even when readonly). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
collapsed was only reset in showPrevious/showNext, so opening the viewer directly (via openViewer) after collapsing and closing reopened the picture collapsed. Reset collapsed whenever the viewer opens, so every newly opened picture starts expanded; the navigation resets (which fire while the viewer stays open) are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both locations where the `downloadPicture` function was used were locally renaming it in the import to `downloadPictureFile`. Simpler to just call it `downloadPictureFile` in the first place.
rmunn
commented
Jul 21, 2026
| : 'h-40 w-auto rounded-md object-contain', | ||
| ); | ||
|
|
||
| // Long-press opens the actions menu, so touch users don't have to hit the small three-dots target. |
Contributor
Author
There was a problem hiding this comment.
Is this code right? Or is there a longPress event that the LLM just completely missed? I would assume that its training data includes lots and lots of Javascript/Typescript aimed at mobile devices, so I assume it's right, but I don't actually know. Maybe there's a much simpler way of handling long-press events that the LLM missed, and which I would also miss because I don't know about it.
Whoever reviews this, please let me know if this chunk of code is correct or not.
Collaborator
There was a problem hiding this comment.
I'm guessing it should reuse something. E.g. our context-menu gives us long presses for free. If that's not the right component here, maybe bits-ui has something smaller?
Collaborator
There was a problem hiding this comment.
[Claude-drafted]
Done — dropped the hand-rolled long-press timer and let the actions menu handle it via its context-menu mode (<PictureActionsMenu contextMenu> → bits-ui ContextMenu), which gives touch long-press and right-click for free. No custom long-press code anymore.
Contributor
Author
|
I've done a self-review, and in theory I could commit this, but I'm not quite certain about one thing. The image-service.svelte.ts file has a comment (generated by Claude): // getOrAdd runs this inside the project context's $effect.root; its teardown revokes the URLs.
$effect(() => () => service.dispose());I understand what that The other part I'm uncertain of, I've flagged: the long-press code. Another place where I don't know enough to judge if the LLM-generated code is correct. |
hahn-kev
reviewed
Jul 22, 2026
| */ | ||
| export class ImageService { | ||
| readonly #getApi: () => IMiniLcmJsInvokable | undefined; | ||
| readonly #cache = new SvelteMap<string, ImageLoadState>(); |
Collaborator
There was a problem hiding this comment.
do we actually want to cache these across the whole project? that sounds a lot like a memory leak.
Contributor
Author
There was a problem hiding this comment.
If we cache them at the entry level then they go away when you switch to another entry and switch back, which I found to be very poor UX. I'll double check whether they get unloaded when you close the project.
Contributor
Author
There was a problem hiding this comment.
Actually, now that we have the downloadIfMissing parameter, then the UX is better even if we don't cache at the project level. Because we can query the backend to say "Do you have this?" and not trigger a download in the process. So I'll revert to the version that cached at the entry level.
hahn-kev
reviewed
Jul 22, 2026
| async #load(mediaUri: string, downloadIfMissing: boolean): Promise<void> { | ||
| const api = this.#getApi(); | ||
| if (!api) { | ||
| this.#cache.set(mediaUri, {status: 'error', reason: 'unknown'}); |
Collaborator
There was a problem hiding this comment.
I don't really like this. We should never get here, change the image service so that either the API is required (preferred) or it throws if you try to use it when it's not ready.
hahn-kev
reviewed
Jul 22, 2026
…IfMissing 36a0db3 moved the cache to project scope so a downloaded picture wouldn't revert to "click to load" after navigating away and back (EntryEditor remounts per entry via {#key entry.id}, tearing the cache down). The downloadIfMissing mechanism added since then makes that unnecessary: ensureLocal() (downloadIfMissing=false) probes for a locally-available file without touching the network, and the backend keeps a once-downloaded remote file locally — so a fresh entry-scoped cache re-loads it on its own when you return, no re-click and no extra download. So go back to the simpler entry-view Context scope (initImageService in EntryEditor) while keeping the click-to-download flow: images available locally show immediately, but a remote-only image stays a placeholder until the user clicks, keeping bandwidth under their control (metered connections). Update the navigation test accordingly: the object URL now differs (a fresh cache mints a new one), so it asserts the first picture re-displays as an <img> with no "click to load" placeholder rather than an identical src. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hahn-kev
reviewed
Jul 22, 2026
| * what's already local, a picture downloaded once stays visible when you navigate away and back: the | ||
| * fresh entry-scoped cache re-loads it locally, no re-click needed. | ||
| */ | ||
| export class ImageService { |
Collaborator
There was a problem hiding this comment.
I'm not sure how much you're planning to rework this. But right now it requires the consumer to deal with the fact that it's a cache. I'd much rather a simpler API, eg loadImage(uri, options) with some options like bypassCache and downloadIfMissing and it will return an ImageState the the caller, no loading state though, a promise should be returned.
Contributor
Author
- Replace the hand-rolled long-press with ResponsiveMenu's contextMenu flavor: right-click and touch long-press on the picture open the same actions menu, and desktop right-click is no longer suppressed. - Viewer: deleting the shown picture advances to the nearest remaining picture instead of closing; the title shows "Picture N of M" when a sense has several; ArrowLeft/ArrowRight navigate (window-level so the keys work regardless of what has focus). - Viewer: stop preventing the dialog's open autofocus, so focus moves into the dialog on open (APG dialog pattern). - Show a generic message in the error placeholder instead of raw exception text; unexpected errors already reach the global handler (copy-error button + .NET logging) via the rethrow. Document that convention in the viewer AGENTS.md. - Prune comments that narrated the code or duplicated the image-service rationale, and fix ones the above changes made stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hT9fAFxFEXNkMDEUTYNNV
useEventListener with a reactive target replaces the svelte:window +
early-return guard: the keydown listener now only exists while the
viewer is open (one viewer is mounted per sense's pictures field).
The position counter becomes a language-neutral "1 / 4" beside the
stable "Picture" title, dropping the just-added "Picture {0} of {1}"
msgid from all catalogs. Top-right placement was rejected: the dialog's
close button lives there.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015hT9fAFxFEXNkMDEUTYNNV
The earlier comment consolidation kept the mechanism but dropped the "mirrors a synced project fetching images on demand" rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hT9fAFxFEXNkMDEUTYNNV
Collaborator
|
"I"
I still think we should revisit the large collection of buttons in the edit dialog: |
- Fix the dialog to a fixed-height stage so paging between pictures no longer resizes it or moves the prev/next arrows. - Letterbox the image (object-contain) into a definite-height figure so a tall/large image caps to the stage instead of overflowing the dialog. - Reserve the caption band so caption-count differences don't shift the stage. - Move the actions menu out of the on-image overlay into the top-right chrome, paired with the close button in one flex cluster (both icon-xs) so they align without mirroring the close button's position. - Add a size prop to the responsive menu trigger for that pairing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Overlay the viewer's prev/next arrows on the stage edges (anchored to the
stage, not the image, so they hold across aspect ratios), show the caption
band only when captions exist, and let the stage bleed to the screen edges on
mobile. Simplify the placeholder/error wording ("Load picture", "Try again",
"You're offline"). Ignore image taps while an actions menu is open so a touch
ghost-click doesn't also open the viewer, covered by a new touch test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Kevin's review: replace the ensureLocal/download/get reactive-cache
surface with loadImage(uri, {downloadIfMissing, bypassCache}) that resolves to
the final ImageState (no 'loading' variant, that's the unresolved promise) and
keeps the object-URL cache internal. The api is now required (a getter that
throws if the project isn't ready) instead of silently caching an error state,
and the local fallback is only built when a project context actually exists.
PictureImage owns its own loading UI and awaits loadImage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swap the load $effect for runed's watch so the reload trigger (mediaUri) is explicit rather than relying on load() transitively reading it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
[Claude-drafted] Reworked the fullscreen picture viewer and thumbnail chrome — stable layout, touch-safe, good on desktop and mobile.
Screenshots — landscape + portrait, desktop + mobile:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
approved these changes
Jul 23, 2026
myieye
left a comment
myieye
left a comment
Collaborator
There was a problem hiding this comment.
I can't say that I went through the code thoroughly, but I tweaked the UX/UI to what I'm happy with.
Contributor
Author
|
There's a bug now: the caching is broken. If you open the edit dialog via the three-dots menu, then load the picture while inside the edit dialog, the picture in the main view doesn't "notice" that it has been loaded; its I'm working on fixing that bug right now. Claude suggested putting cache awareness back into the PictureImage, which complicates the API. I'll see if there's a simpler way to do it. |
Now if the picture is clicked in a dialog, the PictureImage component in the editor view will be notified that the picture has updated, and will pick up that newly-loaded picture.
Contributor
Author
|
Commit a149c92 fixes the caching bug; now when you load the image in the fullscreen or edit dialog, the PictureImage component in the main entry also updates itself. |
E2E test to verify that the cache bug fixed by the previous commit will stay fixed.
Removed most of Claude's overly-chatty comments; kept only the ones that were actually non-obvious to someone skimming the code.
Contributor
Author
Contributor
Author
|
IMPORTANT: When merging, please edit the merge commit and trim it down. Claude wrote more than 3/4 of the commit messages in this PR in its overly-chatty style. If you commit this with GitHub's default "smash all the commit messages together and call it good" message, it will become ridiculously long in a |
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
4 participants
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.
Fixes #2442.
IMPORTANT: When merging, please edit the merge commit and trim it down. Claude wrote more than 3/4 of the commit messages in this PR in its overly-chatty style. If you commit this with GitHub's default "smash all the commit messages together and call it good" message, it will become ridiculously long in a
git log.Done:
Screenshots, please bikeshed the UI.
Click-to-load placeholder:
"Image not found" placeholder if download fails (e.g. the picture was not moved into LinkedFiles originally so the Mercurial repo doesn't have it):
Fullscreen view with all captions visible:
Fullscreen view with captions collapsed:
Please bikeshed the captions and chevron indicator. Is the indicator too small? Should it be given a background to stand out more? Should it be over on the left? Should the captions be centered under the picture in this view like they are in the entry view? (My opinion there is that the captions should be left-justified in this view, but centered in the entry view, but I could be wrong).
Three-dots menu in corner: