Skip to content

feat(web): downloadFile.ts enhancements (downloadBlob, fileNameFromUri, isHttpUrl)#1586

Merged
cliffhall merged 1 commit into
1579-wave-1from
v2/1560-downloadfile
Jul 2, 2026
Merged

feat(web): downloadFile.ts enhancements (downloadBlob, fileNameFromUri, isHttpUrl)#1586
cliffhall merged 1 commit into
1579-wave-1from
v2/1560-downloadfile

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1560

Wave 1 of the #1579 decomposition (re-implementing PR #1510 as scoped issues). Generalizes clients/web/src/lib/downloadFile.ts beyond JSON so the Apps host's ui/download-file support (a later issue) can download arbitrary embedded resources and open http(s) resource links safely. No UI wiring in this change.

What changed

  • downloadBlob(filename, blob) — the temp-anchor download core, with a setTimeout(..., 0)-deferred URL.revokeObjectURL() so a synchronous revoke can't abort the scheduled download (Firefox/Safari, intermittently Chrome for larger blobs).
  • downloadJsonFile(filename, json) — refactored on top of downloadBlob; existing callers (App.tsx) are unchanged.
  • fileNameFromUri(uri) — derives a safe filename from a resource URI's last path segment: strips control/format chars, replaces disallowed filename chars with _, caps at 255 chars, falls back to "download".
  • isHttpUrl(url) — parses and returns the URL only for http:/https: schemes, else null (shared allowlist for opening/downloading server-supplied URLs).

Tests & coverage

  • Updated downloadFile.test.ts: deferred-revoke assertions via fake timers, a downloadBlob passthrough/type case, and fileNameFromUri / isHttpUrl coverage (backslash splitting, control-char stripping, 255-char truncation, scheme rejection).
  • downloadFile.ts: 100% statements / functions / lines, 92.3% branches — all four dimensions ≥90.
  • Full cd clients/web && npm run test:coverage passes (exit 0, 3245 tests); the per-file gate holds.

Scope

Only clients/web/src/lib/downloadFile.ts (+ its test) — no other Wave 1 files touched.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 1, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 53s —— View job


Code Review: downloadFile.ts enhancements

I reviewed the diff against origin/v2/main. This is a clean, well-scoped, well-tested change. downloadJsonFile's signature is unchanged, so the five App.tsx callers are unaffected. ✅

Todos

  • Read the changed files
  • Verify existing callers are unaffected
  • Review for bugs / security / correctness
  • Post review

Strengths

  • downloadBlob deferred revoke — moving URL.revokeObjectURL(url) into setTimeout(…, 0) inside the finally is the right fix for the intermittent Firefox/Safari/Chrome abort, and the try/finally means cleanup runs even if click() throws. Nicely factored out of downloadJsonFile.
  • isHttpUrl — a proper scheme allowlist (http:/https: only), returning null for javascript:, data:, file:, and unparsable input. This is exactly the right guard for opening/downloading server-supplied URLs, and the tests cover the dangerous schemes.
  • fileNameFromUri — sensible layering: last path segment → strip Cc/Cf control/format chars → replace filesystem-reserved chars → trim → cap → fallback. Good use of Unicode property escapes.
  • Tests are thorough (fake timers for the deferred revoke, the throw-in-click path, backslash splitting, control-char stripping, truncation, scheme rejection).

Minor observations (non-blocking)

  1. fileNameFromUri doesn't strip query strings or fragments (downloadFile.ts:46). For https://x/report.pdf?token=abc, the last segment is report.pdf?token=abc; the ? becomes _, yielding report.pdf_token=abc. Safe, but the suggested name leaks/garbles the query. If these URIs can carry queries, consider parsing with URL and using pathname (falling back to raw split for non-URL inputs like C:\...). Not required for this wave since there's no UI wiring yet.

  2. 255-char cap can split a multi-byte grapheme / drop the extension (downloadFile.ts:52). slice(0, 255) counts UTF-16 code units, and most filesystems cap at 255 bytes; a name of multi-byte chars can still exceed the byte limit, and truncation drops the trailing extension. Edge-casey and fine to leave — worth a one-line comment noting the cap is code-units, not bytes.

  3. Dead defensive branchuri.split(/[\\/]/).pop() ?? "" (downloadFile.ts:47): String.prototype.split always returns a non-empty array, so pop() is never undefined and the ?? "" fallback is unreachable. This is almost certainly the uncovered branch behind the reported 92.3%. Harmless defensiveness — no change needed, just noting it as the coverage gap.

Verdict

Looks good to merge. The observations above are optional polish; none are correctness or security blockers, and they can reasonably be deferred to the ui/download-file wiring issue where real URIs flow through these helpers.
· v2/1560-downloadfile

@cliffhall

Copy link
Copy Markdown
Member Author

📋 Smoke test & AGENTS.md audit for this PR: #1560 (comment) (posted on the linked issue #1560).

@cliffhall cliffhall changed the base branch from v2/main to 1579-wave-1 July 2, 2026 16:43
@cliffhall cliffhall merged commit 53cca10 into 1579-wave-1 Jul 2, 2026
1 check passed
@cliffhall cliffhall deleted the v2/1560-downloadfile branch July 2, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

web: downloadFile.ts enhancements (downloadBlob, fileNameFromUri, isHttpUrl)

1 participant