Skip to content

fix(blocks): handle each Share tap once when feeds are nested - #198

Merged
miguelpeixe merged 6 commits into
trunkfrom
fix/nested-feed-share
Oct 8, 2026
Merged

miguelpeixe merged 6 commits into
trunkfrom
fix/nested-feed-share

Conversation

@miguelpeixe

Copy link
Copy Markdown
Member

A Rolling Coverage feed can sit inside another feed's entry. When a reader tapped Share in the inner feed, both feeds handled the tap. On a device with a share sheet, the link was also copied, and a copy notice appeared while the sheet was still open. Without a share sheet, the link was copied twice. An entry with no title of its own, as in the Stream layout, could also share the title of an entry in the feed nested inside it.

What changes

Each Share tap is now handled once, by the feed closest to the button. Feeds that reach the page later, through the Load More button or new updates, keep working.

With this change:

  • Tapping Share in a nested feed opens the share sheet once, or copies the link once.
  • "Link copied." is announced in the tapped entry's own feed.
  • Sharing an entry without a title uses the page's title, never a nested entry's.

How to test

  1. Create two coverages, "Outer" and "Inner", each with two published entries.
  2. Add a Rolling Coverage block showing "Inner" to the older "Outer" entry, and update it.
  3. Add a Rolling Coverage block showing "Outer" to a page, choose the Bulletin layout, and publish.
  4. On trunk, run npm run build, open the page in Chrome on Android, and tap Share on an "Inner" entry.
    The share sheet opens, and a "Link copied." or "Couldn't copy the link." notice also appears.
  5. Check out this branch, run npm run build, and repeat step 4.
    Only the share sheet opens, with no copy notice.
  6. In a desktop browser without a share sheet, such as Firefox, tap Share on an "Inner" entry.
    One "Link copied." notice appears, and the button returns to "Share" after about two seconds.
  7. In the page's feed settings, set "Older entries" to "Load More button" and "Entries per page" to 1.
  8. Reload the page, load the older "Outer" entry, and tap Share on an "Inner" entry in it.
    It shares or copies once, and the page doesn't navigate.
  9. Switch the page's feed to the Stream layout, then share the older "Outer" entry from Chrome on Android.
    The shared title is the page's title, not an "Inner" entry's title.
Technical details

Root cause. src/blocks/share/view.ts adds click and keydown listeners to each feed present at load. A tap in a nested feed bubbles to every feed around it, and each listener ran handleShareClick(). With Web Share, the second run's navigator.share() is refused while the sheet is open, so it falls through to the copy. Two of its lookups also ignored nesting: root.querySelector() for the status region (a capped feed renders none, so it found a nested feed's), and closest( 'article' )?.querySelector() for the title (the Stream and Minute layouts render no Post Title).

The fix.

Two alternatives were set aside. Ignoring buttons whose closest feed isn't root breaks feeds added after load (load more, polls, the jump to the live feed), which get no Share listeners; their taps would follow the link. One document-level listener, as #137 did for Follow, would also work, but it misses taps when a wrapper stops propagation.

Verification. tsc --noEmit and lint pass. A Playwright check ran the built script in real Chromium on routed pages, with stand-ins for navigator.share and Clipboard.writeText(). It covers 19 scenarios: nested taps with and without Web Share, Space, the icon-only and legacy buttons, capped feeds, feeds added after load (one and two levels deep), untitled entries, and modified clicks. 11 fail on trunk; none fail on this branch. Not checked on a device.

With the stand-in writeText() settling in a later task, as on HTTPS, a nested copy on trunk also left the button reading "Copied!" for good. That isn't confirmed in a real browser.

The "Link copied." status region is used only without Newspack UI. With it, the notice is a snackbar.

Self-review: two rounds (Newspack WP expert (Claude Opus 5.5), deep reviewer (Claude Opus 5.5)), no blockers fixed.

🤖 Generated with Claude Code

@miguelpeixe
miguelpeixe requested a balanced review from Copilot October 7, 2026 23:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@miguelpeixe
miguelpeixe marked this pull request as ready for review October 8, 2026 12:14
@miguelpeixe
miguelpeixe requested a lite review from Copilot October 8, 2026 12:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Resolve the missing copy announcement for capped nested feeds and add the reader-facing README documentation.

1 open finding

🧠 Review effort: Lite


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread src/blocks/rolling-coverage/DEVELOPMENT.md
@miguelpeixe

Copy link
Copy Markdown
Member Author

On Copilot's review:

  • "Resolve the missing copy announcement for capped nested feeds": no code change. A capped feed renders no live region by design (DEVELOPMENT.md, "Capped feeds are previews"), so without Newspack UI a capped feed's own copy isn't announced, nested or not, the same as on trunk. With Newspack UI, the "Link copied." snackbar shows in every case. The one nested case this branch affected, a feed added after load inside a capped feed, announces in that feed's own region since 5a13ac1.
  • "Add the reader-facing README documentation": added in 9ab50f7.

@miguelpeixe
miguelpeixe merged commit 2fdd168 into trunk Oct 8, 2026
5 checks passed
@miguelpeixe
miguelpeixe deleted the fix/nested-feed-share branch October 8, 2026 12:23
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Hey @miguelpeixe, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

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.

2 participants