fix: resolve @file virtual-file URLs to absolute in HTML renderer - #9440
fix: resolve @file virtual-file URLs to absolute in HTML renderer#9440VishakBaddur wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/plugins/core/RenderHTML.tsx">
<violation number="1" location="frontend/src/plugins/core/RenderHTML.tsx:64">
P2: `replaceVirtualFileSrc` recreates `<audio>/<video>` without children, which drops nested `<source>/<track>` and fallback content.</violation>
</file>
Architecture diagram
sequenceDiagram
participant BE as Marimo Backend
participant RHTML as RenderHTML Component
participant RM as RuntimeManager
participant Browser as Browser DOM
Note over BE, Browser: User loads notebook (e.g., /notebooks/nb_123)
BE->>RHTML: Send HTML (mo.image, mo.pdf, etc.)
Note right of BE: contains src="./@file/..."
RHTML->>RHTML: parseHtml()
loop For each img, audio, video, source, iframe
RHTML->>RHTML: NEW: Check if src contains "/@file/"
opt Match Found
RHTML->>RM: getRuntimeManager().httpURL
RM-->>RHTML: Return base URL
RHTML->>RHTML: NEW: resolveVirtualFileUrl(src)
Note right of RHTML: 1. Ensure base URL has trailing slash "/"<br/>2. Resolve relative src against base<br/>3. Convert to Absolute URL
end
end
alt CHANGED: Tag is media (img/audio/video/source)
RHTML->>RHTML: NEW: replaceVirtualFileSrc()
RHTML-->>Browser: Render element with absolute src
else CHANGED: Tag is iframe
RHTML->>RHTML: CHANGED: replaceValidIframes() (absolute src injection)
RHTML-->>Browser: Render iframe with absolute src
else Default (External URL or Data URI)
RHTML-->>Browser: Render element with original src
end
Note over Browser, BE: Network Request Resolution
alt Original Behavior (Buggy in Edit Mode)
Browser-xBE: GET /notebooks/@file/... (404: nb_123 segment lost)
else NEW: Fixed Behavior
Browser->>BE: GET /notebooks/nb_123/@file/...
BE-->>Browser: 200 OK (Asset data)
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
Thanks! LGTM, but I think we need to make sure this is general to base URL. I'll look at it in a bit more detail but pinged @mchav who should be able to determine a bit more molab context |
Bundle ReportChanges will increase total bundle size by 486 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
Files in
|
| ); | ||
|
|
||
| const title = cell.getHoverTitle?.() ?? undefined; | ||
| const isCellSelected = cell.getIsSelected?.() || false; |
There was a problem hiding this comment.
How is this related to the current change?
There was a problem hiding this comment.
You're right, that's unrelated, removed it.
| // Resolve a virtual file URL (./@file/... or @file/...) to an absolute URL | ||
| // using the runtime base, ensuring a trailing slash so the notebook-ID path | ||
| // segment is never dropped during relative resolution. | ||
| function resolveVirtualFileUrl(src: string): string { |
There was a problem hiding this comment.
This seems similar to
Can we reuse that instead of creating a new function?
There was a problem hiding this comment.
I've restructured it to mirror asRemoteURL's pattern (blob: handling + same base resolution). The reason I kept it as a separate function rather than calling asRemoteURL directly is that asRemoteURL doesn't guarantee a trailing slash, and without that, new URL("./@file/...", base) drops the last path segment when the base URL has no trailing slash (the exact bug we're fixing). Added a comment in the code explaining this.
| }); | ||
| }); | ||
|
|
||
| describe("replaceVirtualFileSrc - virtual file URL rewriting", () => { |
There was a problem hiding this comment.
These tests don't actually repro the bug. We shoudl mock the runtime manager and check against complete strings rather than substrings.
There was a problem hiding this comment.
Updated the tests to mock getRuntimeManager with a base URL that has no trailing slash (http://localhost:2718/notebooks/nb_xxx), which reproduces the actual molab edit-mode scenario. Assertions now check complete URL strings instead of substrings.
When mo.image() (or mo.audio(), mo.pdf()) renders output, the backend generates a relative URL like ./@file/SIZE-filename. On molab in edit mode, the page URL is /notebooks/nb_xxx (no trailing slash), so the browser resolves ./@file/... to /notebooks/@file/... — dropping the notebook ID — causing a 404 and broken image. App mode works because the URL ends with /app, giving an extra path segment for relative resolution. Fix: add resolveVirtualFileUrl() helper that gets the runtime base URL and guarantees a trailing slash before resolving the @file path to absolute. Apply it in replaceVirtualFileSrc (img/audio/video/source) and replaceValidIframes (iframe, for mo.pdf()). Fixes marimo-team#9432
for more information, see https://pre-commit.ci
… mo.ui.table" This reverts commit 4cb9496.
13f40f3 to
8a82c51
Compare
for more information, see https://pre-commit.ci
|
Going to close this so it does not get accidentally review more. the issue is on molab and it is already fixed in the latest molab that we are currently doing a slow rollout of. |
Problem
mo.image()(andmo.audio(),mo.pdf()) fails to render in edit mode on molab but works in app mode. Closes #9432.Root cause: The backend intentionally generates relative virtual file URLs like
./@file/SIZE-filename(it can't know the mount path at creation time). When the page URL has no trailing slash — e.g., molab edit mode at/notebooks/nb_xxx— the browser resolves./@file/...to/notebooks/@file/..., dropping the notebook ID and causing a 404.App mode works because the URL ends with
/app, providing an extra path segment that makes relative resolution correct.Verified empirically:
Fix
Add
resolveVirtualFileUrl()helper inRenderHTML.tsxthat resolves@file/URLs against the runtime base URL with a guaranteed trailing slash, making the URL absolute and unambiguous regardless of the page URL's trailing slash.Apply it in:
replaceVirtualFileSrc— handles<img>,<audio>,<video>,<source>(coversmo.image(),mo.audio())replaceValidIframes— handles<iframe>(coversmo.pdf())Data URLs and external
https://URLs are untouched.Testing
./@file/rewriting,@file/without./, non-@fileURLs (unchanged),data:URLs (unchanged)