🐛 Fixed missing gift link option in the share modal for pages#28988
🐛 Fixed missing gift link option in the share modal for pages#28988jonatansberg wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 10m 56s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run-many -t test:unit -p @tryghost/posts,@tr... |
✅ Succeeded | 7m 27s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 4m 27s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 3m 12s | View ↗ |
nx run-many -t lint -p @tryghost/posts,@tryghos... |
✅ Succeeded | 1m 10s | View ↗ |
nx run @tryghost/activitypub:test:acceptance |
✅ Succeeded | 57s | View ↗ |
nx run ghost:build:assets |
✅ Succeeded | 2s | View ↗ |
nx run ghost:build:tsc |
✅ Succeeded | 6s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-30 12:47:55 UTC
jonatansberg
force-pushed
the
ber-3751-gift-link-share-modal-for-pages
branch
from
0121eeb to
26be980
Compare
closes https://linear.app/ghost/issue/BER-3751 - pages reach the same analytics share modal as posts, but the context resolved the resource only via the posts endpoint, which never returns pages - so a page came back undefined and the gift-link eligibility check (which reads status/visibility) silently failed, hiding the option - fall back to the pages endpoint when the post lookup is empty, and pass the resolved resource through to the gift-link modal
ref https://linear.app/ghost/issue/BER-3751 - pages share the post analytics screen, so its breadcrumb, share modal, edit/delete actions and delete dialog all referred to a page as a "post" - derive the noun from the resolved resource type and thread it through PostShareModal (new optional resourceNoun prop, defaulting to "post" so posts are unchanged) - also corrects the edit action to open the page editor route rather than the post editor
jonatansberg
force-pushed
the
ber-3751-gift-link-share-modal-for-pages
branch
from
26be980 to
29f1aaa
Compare
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
I don't really agree with this approach. Currently page analytics only works at all by accident, and I'd prefer a more holistic solution than falling back to the pages API if the posts request fails. Specifically, the route for page analytics should be /pages/analytics/{id} rather than /posts/analytics/{id}. This way we could use the routing itself to determine whether to hit the posts or pages endpoints, includes, etc. and save the additional round trip to the API and eliminate this fallback logic.
As I understand it, getting page analytics working isn't a blocker for shipping gift links, so I'd prefer not to merge this in favor of waiting for a more holistic solution that actually gets page analytics fully working properly.
|
This PR started as fix the modal, then fix the copy, and you could definitely expand from there to also fix the routing. That would make sense. I might have some time later this week or next week where I could get this fixed. Is there anything else beyond the routing that you feel is missing to make this work properly? |
2 participants
closes NY-1401
Problem
The "Share as a gift" option showed in the analytics share modal for posts but not for pages. As the reporter noted, there was no explicit guard — it was a side-effect of how the resource is fetched.
Pages reach the same React analytics screen as posts (the pages list links to
#/posts/analytics/:id). Butpost-analytics-contextresolved the resource only viauseBrowsePosts, and the/posts/endpoint never returns pages. So for a page,postcame backundefined→useCanManageGiftLink(post)short-circuited on!post→ the gift option (and the post title/preview) was hidden.Confirmed live:
GET /posts/?filter=id:<pageId>→ 0 results;GET /pages/?filter=id:<pageId>→ the page withstatus: published, visibility: members.Fix
post-analytics-context.tsx— when the posts lookup is empty, fall back touseBrowsePagesand exposepostType: 'post' | 'page'. The fallback only fires when the post query returns nothing, so post analytics is unaffected.constants.ts— addedPAGE_ANALYTICS_INCLUDE; the pages API rejects the post-only includes (email,newsletter, click/feedback counts), so the page lookup requests the allowed subset.post-analytics-header.tsx— passresourcetoGiftLinkModalso it queries the right endpoint (it defaulted toposts, which would have failed for a page even once shown).test-helpers.ts— replaced a junkpostTypeobject in the mock fixture (wrong shape, slipped in when the type had nopostType) with'post'.Verification
/private-page/?gift=…).post; empty posts → pages fallback resolves aspagewith visibility). Full posts suite passing (486), lint clean.Follow-up (not in this PR)
A few labels still hardcode "post" and read oddly for pages (the gift teaser "members-only post", the "Post analytics" breadcrumb, the "Your post is published" success title). These live in shared/shade components and are a pre-existing naming gap, so this PR stays scoped to the actual bug.