Skip to content

feat(web-ui): add a Download button for completed files - #1216

Open
kno wants to merge 1 commit into
amule-org:masterfrom
kno:feat/webui-download-button
Open

feat(web-ui): add a Download button for completed files#1216
kno wants to merge 1 commit into
amule-org:masterfrom
kno:feat/webui-download-button

Conversation

@kno

@kno kno commented Aug 29, 2026

Copy link
Copy Markdown

Closes #659

Summary

The Web UI half of #659, now that #1203 has landed: a Download button that pulls a finished file straight to the browser. That was the motivation stated in the issue — the Web UI could tell you a file was complete and then leave you to fetch it over SFTP.

Generated with AI, flagged up front per the terms in #1177. Web UI only, no C++.

It appears in the Shared Files detail panel, and in the detail panel of a finished download — a completed download has been moved into Incoming, which is shared, so the same hash resolves under shared/{hash}/content. done gates it on the downloads side for exactly that reason; anything earlier is still a partfile and the endpoint has nothing servable for it.

A plain anchor, deliberately not a fetch()

The endpoint answers with Content-Disposition: attachment, so a navigation hands the response to the browser's own downloader: the page stays mounted and the bytes never enter JS. A fetch()+Blob would buffer the whole file in browser memory and undo the constant-memory streaming the endpoint exists for — a 600 MB transfer moves the daemon's RSS by kilobytes, and it would be absurd to spend that on the client instead.

No token in the URL. The session cookie is HttpOnly and same-origin, so a navigation carries the credential by itself. A token in a query string would land in browser history, the Referer header and every proxy log in between. The URL is built through a helper that shares the subpath derivation with the fetch client, so a reverse-proxy prefix keeps working.

Not admin-gated, because the route is GUEST-accessible like the listing it hangs off: a session allowed to see the file is allowed its bytes.

A partfile renders as a disabled button with a title saying why, rather than an anchor. An anchor cannot be disabled, and since this is a navigation rather than a fetch, clicking one would replace the page with the endpoint's 409 JSON instead of raising a toast. That flag is also why there is no per-row action in the list: incomplete is detail-only, so a row-level control could not tell a servable file from a partfile.

Screenshot

Download button in the Shared Files detail panel

Test plan

Driven in a browser against a live daemon running merged master (2e2a80e), so the endpoint under it is the shipped one, not a local branch.

Clicking the button issues:

GET /api/v0/shared/70fd34de…/content            200
sec-fetch-mode: navigate                        (a real navigation, not fetch)
cookie: amuleapi_token=…                        (credential by cookie)
  — no Authorization header, no token anywhere in the URL —

content-length:            3145728
content-disposition:       attachment; filename="Pelicula Terminada 2026.mkv";
                           filename*=UTF-8''Pelicula%20Terminada%202026.mkv
content-type:              application/octet-stream
x-accel-buffering:         no
content-security-policy:   default-src 'none'; sandbox
x-content-type-options:    nosniff
accept-ranges:             bytes

The filename has spaces, so the extended form exercises the percent-encoding rather than the trivial path.

  • The SPA is still mounted after the click — same route, detail panel still open. The download does not navigate the app away.
  • Byte fidelity, fetched the same way the browser does, by cookie alone: md5 686c163dd8f31d4e26b310fcf476e8d7 received, identical to the file on disk.
  • The rendered anchor carries no admin-only class and no admin-only ancestor.
  • Console clean — no errors, no warnings.
  • check-i18n.mjs clean; 4 new keys in both catalogues.

The headless browser reports net::ERR_ABORTED after the headers because it has no download directory configured; the response is fully formed, which is what the byte check above confirms independently.

@got3nks

got3nks commented Aug 29, 2026

Copy link
Copy Markdown

Read the whole change. The reasoning here is right and I checked the load-bearing claims rather than taking them: GET /shared/{hash}/content does Authenticate without RequireAdmin, so GUEST access is correct; the downloads panel gates on done and passes no incomplete, which is right because a completed download is in Incoming; the shared panel passes !!s.incomplete; the cookie is Path=/api/v0 and same-origin, so a navigation to that path carries it; and omitting the download attribute so the client cannot rename past the server's sanitised Content-Disposition is a good call, not an oversight.

The anchor-not-fetch() decision is clearly correct - buffering the file in a Blob would throw away the constant-memory streaming the endpoint exists for.

One gap: the navigation bypasses the session-death gate

You reasoned that a partfile must not be an anchor, because a navigation would replace the page with the endpoint's 409 JSON instead of raising a toast - and gated it with a disabled button. The same reasoning applies to three statuses you cannot gate, and they are unaddressed:

  • 401 - the cookie expired or was revoked between the panel rendering and the click
  • 404 - the file stopped being shared in that window (another client, a shared_reload)
  • 503 ec_unavailable

All three answer JSON with no Content-Disposition, so the browser renders them and the SPA is gone. 401 is the one that stings: markSessionDead() lives inside request(), so a navigation never triggers it. The user gets raw JSON instead of the login screen, hits Back, and only then discovers they were logged out.

The window is normally small - the SSE stream and polls notice a dead session within a tick and unmount the panel. It is not small for a tab left open overnight past the 24h Max-Age, backgrounded and throttled, then returned to.

I am not sure this is worth fixing, and that is the point of raising it. target="_blank" is the obvious lever and it is not free: errors land in a throwaway tab and the SPA survives, but Firefox has historically left an empty tab on the success path, which trades a common-path annoyance for a rare-path recovery. A preflight fetch before location.href costs a round trip and still races.

So: either take target="_blank" with that trade-off stated, or extend the comment that already explains the 409 case to say what happens on 401/404/503 and that it is accepted. What I would not leave is the current state, where the reasoning covers one status and is silent on three.

Nit: apiUrl's comment says "Absolute URL"; it returns a root-relative one (BASE starts from location.pathname). The behaviour is right, the word is not.

@kno
kno force-pushed the feat/webui-download-button branch from 19b083f to 164a17d Compare August 29, 2026 16:30
@kno

kno commented Aug 29, 2026

Copy link
Copy Markdown
Author

Both addressed in 164a17d.

The 401 / 404 / 503 gap. Documented as accepted rather than fixed, and I want to be explicit that this is a choice and not an oversight I papered over.

target="_blank" is the wrong trade here. It buys recovery on a path that needs one tick of bad luck to reach, and pays for it with an empty tab on the success path — which is the path, every time anyone uses this button. Trading a common-path annoyance for a rare-path recovery is the wrong direction, and a stray tab on every download is the kind of thing that gets a feature disliked rather than debugged. A preflight fetch, as you say, costs a round trip and still races.

So the comment now says what actually happens on all four statuses instead of only the one I had reasoned about:

// A navigation cannot be intercepted, so 401, 404 and 503 also render their
// JSON in place of the app; 401 additionally bypasses markSessionDead(), which
// only runs inside request(). Accepted rather than fixed: the window is one
// tick, since any poll or the SSE stream unmounts the panel on a dead session.
// target="_blank" would contain it, but Firefox can leave an empty tab on the
// success path — trading a common-path annoyance for a rare-path recovery.

Your overnight-tab case is the one that survives that reasoning, and I have not solved it — a backgrounded, throttled tab past the 24h Max-Age really can get raw JSON. If you would rather have target="_blank" and eat the Firefox behaviour, say so and I will switch it; the argument is close enough that I would not push back.

The nit. Fixed: apiUrl returns a root-relative URL, since BASE starts from location.pathname. Behaviour unchanged; only the word was wrong. Thank you for catching it — it is the same failure mode as the three false comments you found in #1203, and I would rather be told.

Re-verified after the change: the button still resolves to /api/v0/shared/{hash}/content, the click still leaves the SPA mounted on #/shared, and the i18n gate is clean.

Separately, @ngosang asked on #1215 for fewer comments in the code. I have not applied that here yet beyond tightening the two blocks above — the trimming pass is on that branch first, and I will bring the same standard back to this one before it lands.

@kno

kno commented Aug 29, 2026

Copy link
Copy Markdown
Author

Correcting a figure I cited here, in the last paragraph of my previous comment, when I referred to @ngosang's request on #1215.

I claimed upstream's Web UI sits at 9% comment lines. It does not. That was views/downloads.js, which is the least-commented file in the tree, and I was also comparing it against a ratio measured over diff-added lines rather than over a whole file — two different denominators.

Measured across all 24 non-vendor JS files over 50 lines, upstream ranges 5% to 35%, median 17%. components.js, which this PR touches, is 24% on master; on this branch it is 27%. Close to the house, not far above it.

The trim on #1215 still stands on its own merits, and I will bring the same standard here before this lands — but the justification I gave for it was not one that survives checking, and I would rather say so than leave the number standing. Full correction is on #1215.

@got3nks

got3nks commented Aug 29, 2026

Copy link
Copy Markdown

Agreed on target="_blank" - don't take it. A stray tab on every download to recover a one-tick race is the wrong trade, and the comment now covering all four statuses was the actual ask.

The overnight tab is not this button's problem: there is no visibilitychange or focus handler anywhere in static/js, so a backgrounded tab never revalidates. This button is just where the consequence is a page replacement rather than a fetch 401 reaching markSessionDead(). Worth its own change.

Both fixes confirmed in 164a17d. Looks good to me - but I'll let @ngosang give the final review before it merges; he's taking care of the Web UI and knows it better than I do.

This is what the endpoint was for. The Shared Files panel could tell you a
file was complete and then leave you to fetch it over SFTP; now there is a
button, and the finished-download panel has the same one, since a completed
download has been moved into Incoming and resolves under the same hash.

A plain anchor, never a fetch(). The endpoint answers with
Content-Disposition: attachment, so a navigation hands the response to the
browser's own downloader: the page stays mounted and the bytes never enter
JS. A fetch()+Blob would buffer the whole file in browser memory and throw
away the constant-memory streaming the endpoint exists for -- a 600 MB
transfer moves the daemon by kilobytes, and it would be absurd to undo that
in the client.

No token in the URL. The session cookie is HttpOnly and same-origin, so a
navigation carries the credential by itself; a token in a query string would
land in browser history, the Referer header and every proxy log in between.
The URL is built through a helper that shares the subpath derivation with
the fetch client, so a reverse-proxy prefix keeps working.

Not admin-gated: the route is GUEST-accessible, like the listing it hangs
off. A session allowed to see the file is allowed its bytes.

A partfile renders as a disabled button with a title saying why, rather than
an anchor -- an anchor cannot be disabled, and since this is a navigation
rather than a fetch, clicking one would replace the page with the endpoint's
409 JSON instead of raising a toast. That flag is also why there is no
per-row action in the list: `incomplete` is detail-only, so a row-level
control could not tell a servable file from a partfile.
@kno
kno force-pushed the feat/webui-download-button branch from e6e00c9 to 4278682 Compare September 4, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[amuleapi] Add an endpoint to download a shared file's contents over HTTP

3 participants