Skip to content

Add downloadIfMissing parameter to media GetFileStream APIs. - #2459

Merged
hahn-kev merged 1 commit into
developfrom
t3code/media-service-conditional-download
Jul 20, 2026
Merged

Add downloadIfMissing parameter to media GetFileStream APIs.#2459
hahn-kev merged 1 commit into
developfrom
t3code/media-service-conditional-download

Conversation

@hahn-kev

Copy link
Copy Markdown
Collaborator

Let callers opt out of implicit remote downloads so the media manager can preview only locally cached files, while entry-editor callers still auto-fetch by passing true explicitly from JS.

This is required for the picture UI to show a load button.

Let callers opt out of implicit remote downloads so the media manager can preview only locally cached files, while entry-editor callers still auto-fetch by passing true explicitly from JS.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Jul 20, 2026
@hahn-kev
hahn-kev requested a review from rmunn July 20, 2026 07:46
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The media stream APIs now accept a downloadIfMissing flag. Backend retrieval returns NotFound for uncached files when disabled, while frontend callers explicitly choose whether missing media should be downloaded.

Media download control

Layer / File(s) Summary
Backend media retrieval contract
backend/FwLite/...
Backend APIs propagate downloadIfMissing; uncached files return NotFound when downloading is disabled.
JavaScript bridge propagation
backend/FwLite/FwLiteShared/..., frontend/viewer/src/lib/dotnet-types/..., frontend/viewer/src/project/demo/...
Invokable services, generated interfaces, and the demo API accept and forward the new parameter.
Frontend request policies
frontend/viewer/src/lib/components/..., frontend/viewer/src/lib/entry-editor/..., frontend/viewer/src/lib/media-manager/...
Picture and audio loading enable downloads, while media manager previews disable implicit downloads.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: rmunn

Poem

A rabbit hops through streams anew,
With download flags in every queue.
Cached files stay snug in their bed,
Missing ones say “NotFound” instead.
Pictures bloom and audio plays—
Carrots cheer these clearer days! 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a downloadIfMissing parameter to GetFileStream APIs.
Description check ✅ Passed The description is directly related to the changes and explains the new opt-out behavior and picture UI impact.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/media-service-conditional-download

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@argos-ci

argos-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Jul 20, 2026, 7:50 AM
e2e (Inspect) ✅ No changes detected - Jul 20, 2026, 7:57 AM

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)

1960-1967: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Honor the new downloadIfMissing contract.

The parameter is accepted but ignored, so downloadIfMissing=true still returns NotFound for uncached files. This conflicts with IMiniLcmReadApi and LcmMediaService, where true permits downloading missing media. Implement the download path for this backend, or explicitly make this implementation cache-only and prevent callers from relying on the default behavior.

🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs` around lines 1960
- 1967, The GetFileStream method ignores downloadIfMissing; implement its
true-path behavior to retrieve missing media consistently with IMiniLcmReadApi
and LcmMediaService, while retaining cached-file reads. If this backend cannot
download media, explicitly enforce cache-only semantics and ensure callers
cannot rely on the default true value.
🤖 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.

Outside diff comments:
In `@backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1960-1967: The GetFileStream method ignores downloadIfMissing;
implement its true-path behavior to retrieve missing media consistently with
IMiniLcmReadApi and LcmMediaService, while retaining cached-file reads. If this
backend cannot download media, explicitly enforce cache-only semantics and
ensure callers cannot rely on the default true value.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 96efefd3-e29c-4c84-89ab-e11cf4116202

📥 Commits

Reviewing files that changed from the base of the PR and between 31da37e and c3ce240.

📒 Files selected for processing (13)
  • backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
  • backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs
  • backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs
  • backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
  • backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
  • backend/FwLite/MiniLcm/IMiniLcmReadApi.cs
  • frontend/viewer/src/lib/components/field-editors/audio-input.svelte
  • frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts
  • frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts
  • frontend/viewer/src/lib/entry-editor/field-editors/EditPictureDialog.svelte
  • frontend/viewer/src/lib/entry-editor/field-editors/PictureImage.svelte
  • frontend/viewer/src/lib/media-manager/MediaFileDetail.svelte
  • frontend/viewer/src/project/demo/in-memory-demo-api.ts

@rmunn rmunn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hahn-kev
hahn-kev merged commit 55c38b7 into develop Jul 20, 2026
35 of 36 checks passed
@hahn-kev
hahn-kev deleted the t3code/media-service-conditional-download branch July 20, 2026 08:47
@coderabbitai coderabbitai Bot mentioned this pull request Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants