Fixed displayed gift link being incorrect on first modal open - #29019
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run-many -t test:unit -p @tryghost/posts,@tr... |
✅ Succeeded | 2m 16s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 55s | View ↗ |
nx run-many -t lint -p @tryghost/posts,@tryghos... |
✅ Succeeded | 35s | 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-07-08 12:30:37 UTC
kevinansfield
changed the title
c6b42f0 to
5c55cc1
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c55cc1c2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsx (1)
44-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the async callbacks on the current key (
apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsx:44-82)In dev StrictMode, the first effect cleanup flips
cancelledto true, the replayed effect returns early on the same ref key, and the only in-flight promise drops itsfinally, leavingensuringstuck. Compare againstlastEnsuredLinkKeyRef.currentat resolution time instead of a per-run cleanup flag so stale requests can be ignored without disabling the modal forever.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsx` around lines 44 - 82, The async callbacks in gift-link-modal’s useEffect are gated by a per-run cancelled flag, which breaks under StrictMode replay and can leave ensuring stuck. Update the effect in GiftLinkModal to validate against lastEnsuredLinkKeyRef.current at promise resolution time for ensureGiftLink, so stale requests are ignored without preventing the latest open target from clearing ensuring. Keep the existing key logic based on resource and postId, but use that current-key check in the then/catch/finally handlers instead of relying on cleanup-only cancellation.
🧹 Nitpick comments (1)
apps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx (1)
81-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify the token actually gets applied after StrictMode replay.
This test only checks
mockEnsureGiftLinkcall count/args, not that the resolved token is reflected in the UI (e.g., the copy button becoming enabled / "Generating link…" placeholder clearing). As noted in the review ofgift-link-modal.tsx, the current guard can callensureGiftLinkonce yet still discard its response under StrictMode replay — this test would pass even with that regression present.Consider asserting on
screen.getByTestId('copy-gift-link')(e.g., checking it becomes enabled, or that the resolved URL appears) after thewaitFor, so the test actually validates the fix's goal rather than just the call count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx` around lines 81 - 92, The StrictMode test for GiftLinkModal only verifies ensureGiftLink is called once, but it should also confirm the returned token is applied to the UI after replay. In the gift-link-modal.test.tsx case around GiftLinkModal and mockEnsureGiftLink, extend the assertion after waitFor to check the rendered state changes (for example, the copy-gift-link control becomes enabled or the loading placeholder disappears), so the test proves the resolved link is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsx`:
- Around line 44-82: The async callbacks in gift-link-modal’s useEffect are
gated by a per-run cancelled flag, which breaks under StrictMode replay and can
leave ensuring stuck. Update the effect in GiftLinkModal to validate against
lastEnsuredLinkKeyRef.current at promise resolution time for ensureGiftLink, so
stale requests are ignored without preventing the latest open target from
clearing ensuring. Keep the existing key logic based on resource and postId, but
use that current-key check in the then/catch/finally handlers instead of relying
on cleanup-only cancellation.
---
Nitpick comments:
In `@apps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx`:
- Around line 81-92: The StrictMode test for GiftLinkModal only verifies
ensureGiftLink is called once, but it should also confirm the returned token is
applied to the UI after replay. In the gift-link-modal.test.tsx case around
GiftLinkModal and mockEnsureGiftLink, extend the assertion after waitFor to
check the rendered state changes (for example, the copy-gift-link control
becomes enabled or the loading placeholder disappears), so the test proves the
resolved link is actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1e4ddd5-a21e-499a-8d21-27ae098c0f58
📒 Files selected for processing (2)
apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsxapps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx
5c55cc1 to
962ea5b
Compare
ref https://linear.app/ghost/issue/BER-3762/ Opening the gift link modal in React StrictMode could replay the ensure effect and issue competing upsert requests, leaving the UI with the wrong last-written token.
962ea5b to
4f1792d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsx`:
- Around line 38-64: The request dedup cache in gift-link-modal’s useEffect only
keys on postId, so a changed resource can incorrectly reuse a stale
ensureGiftLink request. Update the cached request shape around
ensureGiftLinkRequestRef and the reuse check in the effect to include resource
as part of the identity, so a resource change creates a fresh
ensureGiftLink({id: postId, resource}) request instead of reusing the previous
one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bf97b7c-115c-4093-b9db-79bb8cfcc79e
📒 Files selected for processing (2)
apps/posts/src/views/PostAnalytics/modals/gift-link-modal.tsxapps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/test/unit/views/post-analytics/gift-link-modal.test.tsx
ref https://linear.app/ghost/issue/BER-3762/ - the ensure effect re-runs when the resource prop changes, but the cached request was keyed by post id only, so a resource change with the same id would reuse a request made for the other resource - unreachable today because every modal host pins or remounts per resource, but the cache key now matches what the effect dependencies claim
2 participants
ref https://linear.app/ghost/issue/BER-3762/
Summary
Why
Opening the modal could fire two same-moment ensure requests. The API's last-write-wins tradeoff could then leave the modal displaying a token that was no longer the active gift link.