Repository navigation
Share the file without spending the click on downloading it - #273
Open
phanilkumar wants to merge 2 commits into
Open
phanilkumar wants to merge 2 commits into
phanilkumar wants to merge 2 commits into
Conversation
navigator.share() needs transient activation, and that expires five seconds after the click. share() fetched the attachment first, so the download was paid for out of that budget: small images squeaked through, a 30MB PDF never did, and the share failed with NotAllowedError and no feedback to the user at all. Fetch the file before the click instead — on pointerenter and pointerdown — and hold onto it. The click handler then calls navigator.share() synchronously, while the activation is still valid. Deliberately not awaited: an await there would hand the activation straight back. If the file is not ready when the click lands, don't call share() at all. That call is what throws. Mark the button busy, let the fetch finish, and the next click sends it. Also gate on canShare(data) rather than the bare presence of navigator.canShare. A platform can support sharing and still refuse a particular file type, and connect() was testing that a function exists — always true where the API is implemented — so the button rendered on platforms that could never use it. NOT YET VERIFIED AGAINST A RUNNING INSTANCE. There is no JS test harness in this repo, and the system tests drive headless Chrome, where no share sheet exists — so this needs a build and a manual check with both a large PDF and a small image before it goes anywhere near the deployed node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The reusable lightbox controller can miss preloading and share a previously cached attachment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preloads attachments so Web Share retains transient user activation.
Changes:
- Caches files on pointer interaction.
- Adds busy state and payload validation.
- Handles share failures asynchronously.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
app/javascript/controllers/web_share_controller.js |
Adds attachment preloading and synchronous sharing. |
Review details
Suppressed comments (1)
app/javascript/controllers/web_share_controller.js:34
- This cached file is not tied to
filesValue. The lightbox reuses this controller while changing that value for every opened attachment (lightbox_controller.js:16-22), so after one file finishes preparing, opening another attachment makes this condition pass and shares the previously cached file. Invalidate the cache infilesValueChangedand ensure an in-flight fetch only stores its result when its captured URL still matches the current value.
if (this.filesValue && !this.#file) {
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review caught two consequences of that reuse, both mine. The lightbox renders its share button with data-web-share-files-value="" and only fills the value in when an attachment opens (lightbox_controller.js:22), so gating listener registration on filesValue in connect() meant the listeners were never attached there at all — no preloading, and the first click only started the fetch. Register them unconditionally; #prepare already ignores an empty value. Worse, that is one persistent button whose filesValue is reassigned for every image opened, so a cached file would have been shared for the NEXT attachment too — the wrong file, silently. The cache is now tied to the URL it came from: filesValueChanged discards it, #ready checks it still matches, and a fetch that lands after the value moved on is dropped instead of cached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
navigator.share()requires transient activation, and that expires five seconds after the click.share()fetches the attachment first, so the download is paid for out of that budget — a small image squeaks through, a large PDF never does:Reported on a 36MB PDF attachment, Chrome/macOS, where
navigator.canShare({ files: [...application/pdf...] })returnstrue— so the platform accepts the file; the activation is simply gone by the timeshare()is reached.What this changes
Fetch the file on
pointerenter/pointerdownand cache it, so the click handler callsnavigator.share()synchronously while the activation is still valid. Deliberately not awaited — anawaitthere hands the activation straight back.If the file is not ready when the click lands,
share()is not called at all. That call is what throws. The button goesaria-busy, the fetch finishes, and the next click sends it.It also gates on
canShare(data)rather than the bare presence ofnavigator.canShare.connect()was testing that a function exists — always true wherever the API is implemented — so the button rendered on platforms that can never use it, and failed with the sameNotAllowedErrorfor an unrelated reason.Everything is inside the controller; no view changes.
Testing
There is no JS test harness in this repo, and the system tests drive headless Chrome, where no share sheet exists — so this was verified by hand on a self-hosted instance built from this branch: a 36MB PDF and a small image, both hover-then-click and click-without-hover, console clean throughout. Before the change the same 36MB PDF failed every time.