LibreKAT/Ocideck

Watch

Fork 0

[Bug] Image carousel picker reads a blob: path on web, and its callers are ungated #526

New issue

Closed

opened 2026-07-22 10:26:32 +00:00 by brenno · 1 comment

brenno commented

2026-07-22 10:26:32 +00:00

Owner

Describe the bug

_browse() in lib/widgets/dialogs/parts/image_carousel_picker_actions.dart:612
calls FilePicker.pickFiles without withData: true and then reads
result.files.single.path. On web file_picker puts a blob: URL in path,
while the rest of the app expects mem: keys for browser-held images
(lib/services/image_service.dart:127, lib/models/asset_origin.dart:88).

That path is then handed to captionService.getCaption(path) and returned as an
ImagePickResult, so the blob URL travels into the deck.

Neither caller gates the picker on platform:

lib/widgets/editors/_editor_field.dart:418 (_openCarousel)
lib/widgets/app_shell_main_layout.dart:646

The two isWebPlatform references in app_shell_main_layout.dart (lines 306 and
945) are about other controls.

Same class as #506, different decision

#506 hid the logo picker, because a logo is stored as a filesystem path in the
theme profile and a browser has nothing to put there. That reasoning does not
carry over here. The carousel returns an image for a slide, and the app already
has a browser-side representation for exactly that — the mem: pipeline. So the
honest options are:

Make it work on web. withData: true, then route the bytes through the
same mem: path the rest of the web build already uses. This is the better
outcome if the carousel's other half (browsing searchPaths) is meaningful on
web at all.
Hide it on web, like the logo picker, if the carousel is fundamentally
about local image folders — in which case searchPaths is empty on web and
the whole dialog is already close to useless there.

Establish which before writing code: option 1 is more work and only worth it if
the dialog has a reason to exist in a browser.

Regression guard

Same structural limitation as #506: kIsWeb is always false under
flutter test, so the broken branch is unreachable and a widget test would sit
green around it. The guard belongs with #505, which is exactly the shape of
mistake it is meant to catch — a pickFiles result whose path is read without
withData.

Found while verifying #506, not by a search; there may be more.

**Describe the bug** `_browse()` in `lib/widgets/dialogs/parts/image_carousel_picker_actions.dart:612` calls `FilePicker.pickFiles` **without** `withData: true` and then reads `result.files.single.path`. On web `file_picker` puts a `blob:` URL in `path`, while the rest of the app expects `mem:` keys for browser-held images (`lib/services/image_service.dart:127`, `lib/models/asset_origin.dart:88`). That path is then handed to `captionService.getCaption(path)` and returned as an `ImagePickResult`, so the blob URL travels into the deck. Neither caller gates the picker on platform: - `lib/widgets/editors/_editor_field.dart:418` (`_openCarousel`) - `lib/widgets/app_shell_main_layout.dart:646` The two `isWebPlatform` references in `app_shell_main_layout.dart` (lines 306 and 945) are about other controls. **Same class as #506, different decision** #506 hid the logo picker, because a logo is stored as a filesystem path in the theme profile and a browser has nothing to put there. That reasoning does **not** carry over here. The carousel returns an image for a slide, and the app already has a browser-side representation for exactly that — the `mem:` pipeline. So the honest options are: 1. **Make it work on web.** `withData: true`, then route the bytes through the same `mem:` path the rest of the web build already uses. This is the better outcome if the carousel's other half (browsing `searchPaths`) is meaningful on web at all. 2. **Hide it on web**, like the logo picker, if the carousel is fundamentally about local image folders — in which case `searchPaths` is empty on web and the whole dialog is already close to useless there. Establish which before writing code: option 1 is more work and only worth it if the dialog has a reason to exist in a browser. **Regression guard** Same structural limitation as #506: `kIsWeb` is always `false` under `flutter test`, so the broken branch is unreachable and a widget test would sit green around it. The guard belongs with #505, which is exactly the shape of mistake it is meant to catch — a `pickFiles` result whose `path` is read without `withData`. Found while verifying #506, not by a search; there may be more.

brenno added the

triage

bug

labels

2026-07-22 10:26:32 +00:00

brenno referenced this issue

2026-07-22 10:32:10 +00:00

feat: bewaak dat een pad uit FilePicker achter een platformpoort staat #527

brenno referenced this issue

2026-07-22 10:32:28 +00:00

[Feature] Work filePickerPathBaseline down from 7 to 0 #528

brenno referenced this issue

2026-07-22 11:14:51 +00:00

fix: laat de FilePicker-poort per aanroep kijken in plaats van per bestand #535

brenno commented

2026-07-22 11:15:10 +00:00

Author

Owner

Two more instances of the same class, found while refining the #505 gate to look per call site (PR #535):

lib/services/image_service.dart:265 — pickVideo
lib/services/image_service.dart:276 — pickAudio

Both read result?.files.single.path with no platform gate, and both sit directly below pickImageDetailed, which does have a correct if (kIsWeb) branch using withData: true and the mem: store. The file therefore looked clean to the original file-level rule. Their callers are ungated too: video_slide_editor.dart:97, finding_editor.dart:655, audio_attachment_editor.dart:23, editor_panel_slide_settings.dart:806.

A fourth, different in kind: lib/services/file_service.dart:957 (pickPackageFile) reads .path and — unlike pickMarkdownFile, which has a bytes-based sibling for web right below it — has no web counterpart at all.

Why none of them are fixed yet. The decision is not the same for all four, and it is a product judgement rather than a mechanical one:

Video and audio almost certainly should not get a bytes path on web. pickImageDetailed can hold an image in memory; holding a video there is a different proposition, and the HTML export already excludes video deliberately. Hiding the buttons on web, as the logo picker was hidden in #506, looks right — but that is worth confirming against what the web build is actually for.
The carousel is the opposite case: its whole job is returning an image for a slide, and the mem: pipeline already exists. Delegating _browse() to ImageService.pickImageDetailed would make it work rather than disappear — and would remove a duplicated FilePicker call at the same time.
Hiding the carousel instead may remove the only image-insert route on web. Establishing that needs the app running, not a grep.

All four are recorded in filePickerPathBaseline with line numbers, so make check reports them and the list cannot rot silently.

Two more instances of the same class, found while refining the #505 gate to look per call site (PR #535): - `lib/services/image_service.dart:265` — `pickVideo` - `lib/services/image_service.dart:276` — `pickAudio` Both read `result?.files.single.path` with no platform gate, and both sit **directly below** `pickImageDetailed`, which does have a correct `if (kIsWeb)` branch using `withData: true` and the `mem:` store. The file therefore looked clean to the original file-level rule. Their callers are ungated too: `video_slide_editor.dart:97`, `finding_editor.dart:655`, `audio_attachment_editor.dart:23`, `editor_panel_slide_settings.dart:806`. A fourth, different in kind: `lib/services/file_service.dart:957` (`pickPackageFile`) reads `.path` and — unlike `pickMarkdownFile`, which has a bytes-based sibling for web right below it — has no web counterpart at all. **Why none of them are fixed yet.** The decision is not the same for all four, and it is a product judgement rather than a mechanical one: - **Video and audio** almost certainly should *not* get a bytes path on web. `pickImageDetailed` can hold an image in memory; holding a video there is a different proposition, and the HTML export already excludes video deliberately. Hiding the buttons on web, as the logo picker was hidden in #506, looks right — but that is worth confirming against what the web build is actually for. - **The carousel** is the opposite case: its whole job is returning an image for a slide, and the `mem:` pipeline already exists. Delegating `_browse()` to `ImageService.pickImageDetailed` would make it work rather than disappear — and would remove a duplicated `FilePicker` call at the same time. - Hiding the carousel instead may remove the **only** image-insert route on web. Establishing that needs the app running, not a grep. All four are recorded in `filePickerPathBaseline` with line numbers, so `make check` reports them and the list cannot rot silently.

brenno referenced this issue from a pull request that will close it,

2026-07-22 13:57:31 +00:00

fix: laat de beeldcarrousel via ImageService kiezen in plaats van zelf #559

brenno referenced this issue from a commit

2026-07-22 13:57:31 +00:00

fix(carrousel): laat het kiezen van een afbeelding via ImageService lopen

brenno closed this issue

2026-07-22 13:57:33 +00:00