Repository navigation
Display pictures in senses - #2412
Merged
Merged
Conversation
Display a sense's pictures in the FwLite viewer, below Semantic Domains. - Register a new `pictures` sense field (entity-config + FW Lite view) so it shows for every sense. - PictureImage loads each picture via MiniLcmJsInvokable.GetFileStream (MediaUri -> stream -> blob URL), shows its caption (best analysis alternative), and handles not-found/offline/error states. Object URLs are revoked on teardown. No try/catch around async, per viewer conventions. - PictureCarousel wraps the images in an embla carousel (embla-carousel-svelte) that auto-advances every 10s when there is more than one picture, with prev/next + dot navigation. Single-picture senses just show the image. - PicturesEditor shows the carousel when pictures exist, otherwise a disabled "+ Picture" button styled like "+ Component" (adding pictures is not yet possible: MiniLcmJsInvokable exposes no create-picture API to the frontend). - Export IPicture from the dotnet-types barrel (was omitted when generated). - Demo: give the first "nyumba" sense two pictures and serve them as inline SVG blobs from the demo getFileStream, so the carousel is demonstrable in the in-browser demo project. - Add a viewer Playwright test covering the populated (image + caption) and empty (disabled add button) states. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up after running svelte-check, eslint and the i18n extractor: - PictureCarousel: use the `onemblaInit` event attribute (Svelte 5 forbids mixing the legacy `on:` directive with `on*` handlers, and the embla package types this attribute) and pass the required `plugins: []` to the action. Convert the dot-sync arrow to a function declaration (func-style). - Extract the new picture UI strings into the locale catalogs and add translator-context comments in en.po per the i18n context guide. svelte-check: 0 errors / 0 warnings. eslint: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Running the new Playwright test against the real demo data surfaced two issues:
- The demo pictures were added to `_entries`, which is dead code; the served
data is `entries`, whose first item is replaced by `allWsEntry`. Move the two
demo pictures onto `allWsEntry`'s sense (the live "nyumba" entry) and revert
the `_entries` change.
- PicturesEditor crashed ("Cannot read properties of undefined (reading
'length')") on senses whose `pictures` is absent at runtime — the bulk demo
data (and legacy data) omit the field even though the type marks it required.
Default it to an empty array before use.
- Update the test's empty-state case to use a single-sense entry ("ambuka")
since the picture-bearing entry now has only one sense.
Both Playwright tests pass; svelte-check and eslint are clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It's time to implement picture manipulation in the frontend, at least basic stuff (uploading pictures, etc). No editing planned yet.
Wire up the previously-disabled "+ Picture" button in the sense editor so users can add a picture to a sense. - PicturesEditor: clicking the button opens a file picker limited to JPG/PNG, uploads the chosen file via saveFile, then calls createPicture with the returned mediaUri. Upload results are handled by branching on the result enum (no try/catch), matching PictureImage. - Too-large files: the client does not hardcode the size limit (it may change server-side); it handles the server's TooBig result and shows a helpful message — suggest lowering JPEG quality for a JPG, or reducing resolution for a PNG. - MiniLcmApiNotifyWrapper: notify on Create/Update/Move/DeletePicture (this was missing), so the entry reloads and the new picture renders, like every other write op. - Demo API: implement saveFile (stores the blob, enforces the same 10 MB server limit) plus create/update/deletePicture, so the flow round-trips offline. - Playwright: the empty-state button is now enabled; add coverage for uploading a picture through it and seeing it render. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Loading a not-yet-cached media file could crash with "SQLite Error 19: 'UNIQUE constraint failed: LocalResource.Id'". LcmMediaService.GetFileStream checks GetLocalResource, and on a miss calls ResourceService.DownloadResource, which inserts a LocalResource keyed by the file id. When the UI requests the same file more than once before it is cached (e.g. on a picture's first render), two calls both see no local resource and both download and insert, so the second insert violates the primary key. Coalesce concurrent downloads of the same file into one shared task, kept in a ConcurrentDictionary keyed by file id: the first caller starts the download and every concurrent caller awaits that same task, which is removed once it completes. Two races are handled: - GetOrAdd's factory overload can run more than once under contention (which would start the download twice), so the not-yet-started TaskCompletionSource is added via the atomic value overload — only the caller whose task is stored starts the download. - A caller can miss in GetLocalResource just before another caller commits the download. The shared task re-checks GetLocalResource before downloading, and the entry is removed only after the download commits, so a later fresh task finds the committed resource instead of inserting a duplicate key. Different files still download in parallel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GetFileStream threw when a download failed (e.g. the media server/gateway returns 504 Gateway Timeout, or the file is missing on the server). The exception propagated all the way to the JS caller, so instead of the picture UI showing an error state, an unhandled exception surfaced after a long wait. Callers like PictureImage are written to expect failures via the ReadFileResult enum, not exceptions. Catch download failures, log them, and return ReadFileResult.Error with the message so the UI can show a graceful error (and a later navigation can retry) rather than crashing. Note: this does not make an unreachable/slow media server succeed — it only makes the client fail gracefully. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A cold request to the proxied media endpoint (lexbox -> FwHeadless) can exceed the proxy's timeout on the first touch — e.g. before the backend's code paths are JIT-warmed — and come back as 504 (or 502/503) even though the backend then warms up and serves the file in milliseconds. Previously the concurrent-download bug masked this: several requests per file meant a later, warm one succeeded. Now that downloads are coalesced into one, a single cold 504 was terminal. Retry transient failures (408/502/503/504) up to 3 times with a short delay. A failed fetch never reaches AddLocalResource, so this cannot reintroduce the duplicate-key insert; non-transient failures still fail immediately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- PictureImage: left-justify the image (and caption) instead of centering it. - PicturesEditor: size the "+ Picture" button to its (translatable) label and right-align it, matching the "+ Component" button, rather than stretching to the full grid-column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
💻 FW Lite
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end picture CRUD support with backend APIs, viewer editing components, demo storage, localization, and Playwright coverage. Separately, media downloads now coalesce concurrent requests and retry selected transient HTTP failures. ChangesPicture CRUD feature
Media download reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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: 3
🧹 Nitpick comments (3)
frontend/viewer/tests/sense-pictures.test.ts (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fragile selector across all three tests.
[style*="grid-area: pictures"]is repeated verbatim in each test, coupling all of them to an inline-style implementation detail. Consider adding apicturesFieldhelper (e.g., onBrowsePage/EntryViewComponent) so the selector lives in one place.♻️ Proposed helper extraction
// browse-page.ts (or EntryViewComponent) + picturesField(page: Page) { + return page.locator('[style*="grid-area: pictures"]').first(); + }- const picturesField = page.locator('[style*="grid-area: pictures"]').first(); + const picturesField = browsePage.entryView.picturesField(page);Also applies to: 46-46, 62-62
🤖 Prompt for 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. In `@frontend/viewer/tests/sense-pictures.test.ts` at line 26, The three tests are repeating the same fragile pictures selector, which is tightly coupled to an inline style detail. Add a shared helper such as picturesField on BrowsePage or EntryViewComponent and use it in each test instead of inlining page.locator('[style*="grid-area: pictures"]'). Keep the selector logic in one place so the tests reference the helper rather than duplicating the locator.frontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.svelte (1)
43-80: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo client-side file-type validation before upload.
uploadPicturerelies entirely on the server'sNotSupportedresult to reject non-image files; theacceptattribute on the file input is only a UI hint and can be bypassed (e.g., "All Files" in the OS picker). This is already handled by the server, so it's a minor defense-in-depth gap rather than a functional bug.🤖 Prompt for 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. In `@frontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.svelte` around lines 43 - 80, Add a client-side image-type check in uploadPicture before calling api.saveFile, since the file input accept hint can be bypassed and the current flow only relies on the server’s UploadFileResult.NotSupported response. Use the existing uploadPicture function in PicturesEditor.svelte to reject non-image File types early with the same user-facing notification, while still keeping the server-side validation as the source of truth.frontend/viewer/src/locales/en.po (1)
1569-1579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale translator comment: button is no longer disabled.
The comment describes the "Picture" label as being on a "currently-disabled '+ Picture' add button," but per the PicturesEditor.svelte implementation, the button is wired to
onclick={selectFile}and actively triggers file upload/api.createPicture. This context comment appears to predate the upload-wiring commit and should be updated to avoid confusing translators about the button's actual behavior.✏️ Suggested comment fix
-#. Two uses: (1) alt text for a picture image when no caption is available; (2) label on a currently-disabled "+ Picture" add button in the sense editor +#. Two uses: (1) alt text for a picture image when no caption is available; (2) label on the "+ Picture" add button in the sense editor🤖 Prompt for 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. In `@frontend/viewer/src/locales/en.po` around lines 1569 - 1579, The translator comment for the “Picture” msgid is stale and incorrectly says the add button is currently disabled. Update the comment next to the Picture/Pictures entries in the locale file so it matches the current behavior in PictureImage.svelte and PicturesEditor.svelte, describing the label as the add/upload picture button that opens file selection and triggers picture creation rather than a disabled control.
🤖 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 `@backend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.cs`:
- Around line 150-161: The JS bridge is missing the picture reordering
operation, so add MovePicture exposure alongside the existing picture
create/update/delete paths. Update the MiniLcmJsInvokable and
IMiniLcmJsInvokable contract to include a MovePicture method that forwards to
the underlying write API, matching the existing patterns used for picture
actions. Ensure the new bridge method uses the same identifiers and parameters
as IMiniLcmWriteApi.MovePicture so the viewer can invoke reordering directly.
In `@frontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.svelte`:
- Around line 92-113: The PicturesEditor.svelte upload control is only rendered
in the empty-state branch, so once the carousel shows existing pictures the add
button and file input become unreachable. Update the conditional rendering
around PictureCarousel and the “Picture” Button/file input so the add action
remains available whenever !readonly, even when pictures.length > 0; use the
existing PictureCarousel, selectFile, fileInputElement, and uploading symbols to
place the button alongside the carousel.
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts`:
- Around line 561-601: The demo upload flow is losing the original filename
because saveFile only stores the Blob in `#uploadedFiles` and getFileStream later
reconstructs fileName from mediaUri. Update the in-memory demo API to persist
metadata.filename alongside the uploaded Blob, and have getFileStream return
that stored name for uploaded files instead of deriving it from demo-upload ids.
Keep the existing demo-picture fallback behavior unchanged in getFileStream and
preserve the current upload/saveFile contract.
---
Nitpick comments:
In `@frontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.svelte`:
- Around line 43-80: Add a client-side image-type check in uploadPicture before
calling api.saveFile, since the file input accept hint can be bypassed and the
current flow only relies on the server’s UploadFileResult.NotSupported response.
Use the existing uploadPicture function in PicturesEditor.svelte to reject
non-image File types early with the same user-facing notification, while still
keeping the server-side validation as the source of truth.
In `@frontend/viewer/src/locales/en.po`:
- Around line 1569-1579: The translator comment for the “Picture” msgid is stale
and incorrectly says the add button is currently disabled. Update the comment
next to the Picture/Pictures entries in the locale file so it matches the
current behavior in PictureImage.svelte and PicturesEditor.svelte, describing
the label as the add/upload picture button that opens file selection and
triggers picture creation rather than a disabled control.
In `@frontend/viewer/tests/sense-pictures.test.ts`:
- Line 26: The three tests are repeating the same fragile pictures selector,
which is tightly coupled to an inline style detail. Add a shared helper such as
picturesField on BrowsePage or EntryViewComponent and use it in each test
instead of inlining page.locator('[style*="grid-area: pictures"]'). Keep the
selector logic in one place so the tests reference the helper rather than
duplicating the locator.
🪄 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: 2e80063c-04e1-4921-8d8e-4e30ad159fca
📒 Files selected for processing (23)
backend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/LcmCrdt/MediaServer/LcmMediaService.csfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/field-editors/PictureCarousel.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureImage.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/views/entity-config.tsfrontend/viewer/src/lib/views/view-data.tsfrontend/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/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/tests/sense-pictures.test.ts
💤 Files with no reviewable changes (1)
- frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.ts
The picture carousel filled the full field width, so its centered prev/next/dot controls floated far to the right of the left-justified picture. Shrink the carousel to the width of its content (the picture) with w-fit, so it stays left-justified and the centered controls land directly under the picture. max-w-full keeps a very wide picture from overflowing the field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The picture carousel filled the full field width, so its centered prev/next/dot controls floated far to the right of the left-justified picture. When there are multiple pictures (the only time controls show), bound the carousel to a fixed width and center each picture within it, so the centered controls sit directly under the picture. Sizing the carousel to the picture itself isn't possible here — embla lays its slides out in a flex row, so a shrink-to-fit width would span the sum of all slides. A single picture is unchanged (natural size, left-justified) since it has no controls to align. A bounded box also has a definite width, so the loading placeholder fills it instead of collapsing — the box stays put while the image loads (which can take a second or two on slow mobile networks) rather than reflowing once it arrives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For the future, if we decide to allow rearranging the order of pictures in the FW Lite UI.
Collaborator
|
Some thoughts:
Things I'd do in a follow up PR:
|
Collaborator
|
When picking a photo in FLEx it allows png, tiff, jpg and bmp. I think it's fine that we just match that for now. |
Collaborator
|
I would say that by default we don't want to load images automatically (we could let the user enable that if they want), and especially we don't want to load multiple images automatically, so maybe a carousel is a way to do that? but it's definitely not the only way obviously. I think removing an image can be done with a trash can icon in the top right corner (with a prompt). |
Previously the "+ Picture" button only appeared for a sense with no pictures. Now the picture editor always offers "+ Picture" (so more can be added), and when at least one picture exists it also offers "Replace Picture" and "Delete Picture", both acting on the picture currently shown in the carousel. - PictureCarousel exposes its current slide via a bindable `selectedIndex`, so PicturesEditor knows which picture Replace/Delete target. - Replace uploads a new file (same saveFile flow as add) and calls UpdatePicture with the same picture id/order/caption but the new mediaUri. - Delete confirms via the standard delete dialog, then calls DeletePicture. - Both changes surface via the entry-changed event (the notify wrapper fires on Update/DeletePicture), which reloads the entry so the carousel updates. - Demo API already implements update/deletePicture, so this round-trips offline. - Playwright: cover the always-present add button, delete-with-confirmation, and in-place replace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The picture carousel auto-advances every 10s, so the "current" picture could change between the user reaching for Replace/Delete and clicking it. Suspend auto-advance while the Replace/Delete button group is hovered or focused. - PictureCarousel gains a `paused` prop; its auto-advance effect bails (clearing the interval) while paused and resumes when unpaused. - PicturesEditor wraps the Replace/Delete buttons in a role="group" that toggles the paused state on mouseenter/leave and focusin/focusout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hahn-kev
reviewed
Jul 7, 2026
hahn-kev
reviewed
Jul 7, 2026
The "Expose MovePicture to frontend API" change added movePicture to the IMiniLcmJsInvokable interface but not to the in-memory demo API, so the demo class no longer satisfied the interface (svelte-check failed). Implement it as a simple reorder of the sense's pictures between the given neighbours. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uploading an image that already exists on the server is not an error in this app: one image file (mediaUri) can back many Picture objects across different senses and entries. A Picture object is owned by a single sense and stored as JSON, but the mediaUri it holds refers to a shared image file. PicturesEditor no longer rejects an AlreadyExists upload; it treats it like a success and creates a new Picture pointing at the mediaUri the server returns for the existing file. No backend change was needed — UploadFileResponse already carries the existing file's MediaUri on the AlreadyExists result, and that handling stays media-type-agnostic (mediaUri covers images and audio). The demo API now mirrors the server's filename-based dedup: a repeat upload of the same filename returns AlreadyExists with the existing mediaUri. Removes the now-unused "That picture has already been uploaded" string, and adds a Playwright test covering the reuse path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@CodeRabbit fullreview |
Contributor
|
The PR description images show the current state nicely — the single-picture layout, the disabled/enabled "+ Picture" button, and the carousel with dot indicators for multiple pictures. I'll keep the earlier design discussion (carousel vs. showing all images, replace/delete UX, accepted file formats) in mind while reviewing, in case any of that feedback still needs to be reflected in the code. ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
150-171: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFixed retry delay without backoff/jitter.
RequestMediaFileretries transient 408/502/503/504 responses with a flat 200 ms delay. For a cold-start proxy scenario this is probably fine given only 3 attempts, but a short exponential backoff (e.g. 200ms/400ms) would be more resilient if multiple clients retry concurrently against a still-warming backend.🤖 Prompt for 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. In `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs` around lines 150 - 171, RequestMediaFile currently retries transient DownloadFile failures with a fixed 200 ms pause, which can cause synchronized retries under cold-start conditions. Update the retry loop in RequestMediaFile to use a small exponential backoff (for example, increasing the delay on each attempt) while keeping the transient status handling and MaxDownloadAttempts logic unchanged, and preserve the existing response disposal/error path behavior.
🤖 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 `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs`:
- Around line 73-85: The catch block in LcmMediaService.GetOrStartDownload is
leaking raw exception details to the UI by returning e.Message in
ReadFileResponse.ErrorMessage. Keep the existing logger.LogError(e, ...)
server-side, but change the returned ReadFileResponse in this failure path to
use a generic user-facing error string instead of the exception message so
PictureImage only shows a localized safe message.
In
`@frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts`:
- Around line 79-82: Regenerate the IMiniLcmJsInvokable.movePicture signature so
the position arguments can be omitted, matching MiniLcmJsInvokable.MovePicture’s
nullable Guid parameters. Update the generated TypeScript declaration to make
previousPictureId and nextPictureId optional in movePicture, while keeping the
existing entryId, senseId, and pictureId parameters unchanged. Use the
movePicture method in IMiniLcmJsInvokable as the target for this change.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs`:
- Around line 150-171: RequestMediaFile currently retries transient DownloadFile
failures with a fixed 200 ms pause, which can cause synchronized retries under
cold-start conditions. Update the retry loop in RequestMediaFile to use a small
exponential backoff (for example, increasing the delay on each attempt) while
keeping the transient status handling and MaxDownloadAttempts logic unchanged,
and preserve the existing response disposal/error path 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: e68d9464-d580-4567-9b8d-9ec128698c3e
📒 Files selected for processing (23)
backend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/LcmCrdt/MediaServer/LcmMediaService.csfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/field-editors/PictureCarousel.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureImage.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/views/entity-config.tsfrontend/viewer/src/lib/views/view-data.tsfrontend/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/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/tests/sense-pictures.test.ts
💤 Files with no reviewable changes (1)
- frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.ts
Add the shadcn-svelte carousel component and rewrite PictureCarousel to use its Carousel.Root/Content/Item primitives, capturing the Embla api via setApi. All existing behavior is preserved: bindable selectedIndex, paused, 10s auto-advance, loop, the bounded-width/centered layout for multiple pictures, and the i18n'd dot indicators + prev/next controls. Keep our own control chrome instead of Carousel.Previous/Next: those sit outside the frame with hardcoded English labels and offer no dot indicator, whereas our controls are centered below the image with translated labels. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This will be needed if we want the frontend to be able to allow people to move pictures to the front/top or end/bottom (and if users want to move pictures, nearly every time they're going to be moving one of them to the top or bottom.)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/viewer/src/locales/en.po (1)
1576-1587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShared "Picture" msgid spans two different UI roles.
The same translation string is reused as both alt text and a disabled "+ Picture" button label. For some target languages this dual-purpose string may not translate naturally in both contexts (label vs. button caption). Consider a distinct msgid/context for the button label if translator feedback surfaces issues.
🤖 Prompt for 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. In `@frontend/viewer/src/locales/en.po` around lines 1576 - 1587, The "Picture" translation is shared between image alt text and the disabled add-button label. Introduce a distinct translation msgid or context for the "+ Picture" button in PicturesEditor.svelte, update its usage and locale entry, while retaining "Picture" for PictureImage.svelte alt text.
🤖 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 `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs`:
- Around line 103-122: Update GetOrStartDownload to return the
Task<LocalResource> stored in DownloadTasks directly instead of awaiting it;
preserve the existing producer logic and return the local task variable so
callers receive the same task instance used by TryRemove in GetFileStream,
allowing completed downloads to be evicted correctly.
In `@frontend/viewer/src/lib/entry-editor/field-editors/EditPictureDialog.svelte`:
- Around line 136-150: Guard Submit and Replace Picture against an in-flight
delete by including deleting in their disabled conditions. Update the buttons
associated with fileInputElement?.click() and submit() so they cannot be
activated while deleting is true, matching the existing Delete button
concurrency protection.
---
Nitpick comments:
In `@frontend/viewer/src/locales/en.po`:
- Around line 1576-1587: The "Picture" translation is shared between image alt
text and the disabled add-button label. Introduce a distinct translation msgid
or context for the "+ Picture" button in PicturesEditor.svelte, update its usage
and locale entry, while retaining "Picture" for PictureImage.svelte alt text.
🪄 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: 8b8faf01-efaa-44d1-8af8-4b736bc26da0
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
backend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/LcmCrdt/MediaServer/LcmMediaService.csfrontend/viewer/package.jsonfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/field-editors/EditPictureDialog.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PictureImage.sveltefrontend/viewer/src/lib/entry-editor/field-editors/PicturesEditor.sveltefrontend/viewer/src/lib/entry-editor/field-editors/picture-formats.tsfrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/views/entity-config.tsfrontend/viewer/src/lib/views/view-data.tsfrontend/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/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/tests/sense-pictures.test.ts
💤 Files with no reviewable changes (2)
- frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.ts
- frontend/viewer/package.json
Very unlikely to happen, but costs basically nothing to guard against
By inverting the logic of the if statement, which simply returns if the condition was false, we can dedent most of the function. As a bonus, this also allows us to fix a subtle bug where the task wouldn't have been removed from the ConcurrentDictionary, which would have resulted in a memory leak.
Contributor
Author
|
Okay, I'm reasonably happy with the job that Claude did at this point. I'll probably go through and delete some of the overly-chatty comments, or at least rephrase them, but this is looking good. Time for actual code review, I think. |
Ran `pnpm i18n:extract` again to resolve *.po merge conflicts
hahn-kev
reviewed
Jul 10, 2026
hahn-kev
left a comment
hahn-kev
left a comment
Collaborator
There was a problem hiding this comment.
If I try to edit a picture when opening a flex project I get an error:
Browse view failed
each_key_duplicate Keyed each block has duplicate key en at indexes 0 and 2 https://svelte.dev/e/each_key_duplicate
A writing system can belong to both the vernacular and analysis lists (common in FieldWorks projects), so allWritingSystems()'s concatenation could contain the same wsId twice. The edit-picture caption editor (RichMultiWsInput) keys its rows by wsId and threw each_key_duplicate on such projects. Add WritingSystemService.uniqueWritingSystems (same input as allWritingSystems, de-duplicated keeping the first occurrence) and use it for the caption editor. Includes a unit test covering dedup, the no-duplicate case, and selection order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Try now; commit 46ae678 dedupes the writing systems presented in the edit-picture dialog. |
hahn-kev
requested changes
Jul 10, 2026
This was referenced Jul 12, 2026
The `picture` object was being refreshed/updated by a different part of the code, which was triggering the `$effect` to run again. By moving the `mediaUri` into a `$derived`, we avoid the `$effect` running twice: because although it's a different `picture` object, it has the same `mediaUri` and therefore the `$derived` doesn't re-run its dependents.
Now it's the caller's responsibility to provide an empty list if sense.pictures was undefined (as might happen with historical data)
hahn-kev
approved these changes
Jul 15, 2026
hahn-kev
left a comment
hahn-kev
left a comment
Collaborator
There was a problem hiding this comment.
looks good to me. Don't worry about the argos errors, but you should fix the linting issues.
The E2E test reorg on develop (b5e8ecb) removed tests/browse-page.ts and moved UI tests under tests/ui/ with a ProjectPage / DemoProjectPage page-object hierarchy. Move sense-pictures.test.ts into tests/ui/ (where the ui playwright config discovers it) and swap the deleted BrowsePage for DemoProjectPage. goto()/selectEntryByFilter() map one-to-one, so only the setup lines change; every picture/dialog/upload assertion is unchanged. All 10 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Linting issues turned out to be caused by recent changes to E2E tests on develop (the tests that Claude wrote were relying on a file that had moved to a new location, for example). So I merged develop into the branch and had Claude rewrite the tests since that was faster than doing it by hand. Should work now; currently waiting to see if all the checks turn green. If they do, I'll merge the PR. |
This was referenced Jul 20, 2026
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
3 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.
UI mostly coded by Claude; I just gave it design pointers.
Tested locally by doing Send/Receive and/or Sync in both directions. The "+ Picture" button successfully uploads a picture and then the sync can send it to FieldWorks Classic. Currently limited to JPG, PNG, BMP, and TIFF formats, same as FW Classic.
UI shows pictures in a flex row, wrapping, so that on phones they will end up in a column (unless the phone is held landscape-style). Screenshots below.
Some features will be bumped to a later PR; see #2442 for those.
Design decisions to discuss
Design decisions we made
Screenshots
One picture:
No pictures:
Two pictures:
Edit Picture dialog: