🧵 app: fix onesignal prompt persistence race condition#669
Conversation
🦋 Changeset detectedLatest commit: 1aa1fe6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @cruzdanilo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a timing issue where OneSignal in-app message prompts might be evaluated using stale or unhydrated data. By ensuring that the application's query client has fully restored its state from persistence before checking notification dismissal status, the system now accurately determines when to display in-app messages, leading to more reliable user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds client-side hydration promise and a persist() API to the query client; OneSignal onboarding prompt gating now awaits hydration and persists dismissal state; adds a changeset noting the OneSignal fix; updates local Android emulator startup flags and readiness waits. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant QueryClient
participant Persister
participant OneSignal
Browser->>QueryClient: import `hydrated` (Promise)
Browser->>Persister: client-only restore/subscribe after hydration
Persister-->>QueryClient: restored state
OneSignal->>QueryClient: enablePrompt() called
OneSignal->>QueryClient: await hydrated.then(...).catch(reportError)
QueryClient-->>OneSignal: hydrated resolved / restored data available
OneSignal->>QueryClient: read lastDismiss from restored state
alt appId present and not dismissed
OneSignal->>OneSignal: show onboarding prompt
end
OneSignal->>QueryClient: onDismiss -> call persist().catch(reportError)
QueryClient->>Persister: persister.persistClient(dehydrated state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a potential race condition by ensuring that the queryClient is fully hydrated before attempting to read data for the OneSignal notification dismiss check. The introduction of the hydrated promise in queryClient.ts and its subsequent use in onesignal.ts provides a robust and asynchronous way to manage this dependency. The changes are well-implemented and include appropriate error handling, improving the reliability of the notification prompt logic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #669 +/- ##
==========================================
+ Coverage 60.27% 61.46% +1.19%
==========================================
Files 169 169
Lines 5226 5240 +14
Branches 1479 1481 +2
==========================================
+ Hits 3150 3221 +71
+ Misses 1908 1858 -50
+ Partials 168 161 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/onesignal.ts`:
- Around line 75-81: Extract the multi-line inline callback passed to
hydrated.then into a named async (or regular) function—e.g., create a function
like handleHydration() that performs the current logic: read lastDismiss via
queryClient.getQueryData<number>(["onesignal","dismiss"]) ?? 0, check appId and
DISMISS_EXPIRY against Date.now(), and call
OneSignal.InAppMessages.addTrigger("onboard","1") when appropriate; then replace
hydrated.then(() => { ... }).catch(reportError) with
hydrated.then(handleHydration).catch(reportError) (and keep reportError usage
unchanged). Ensure the new function is defined in the same module and references
the same symbols (queryClient, appId, DISMISS_EXPIRY, OneSignal, reportError).
cruzdanilo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.changeset/good-tips-care.md:
- Line 5: Update the changeset entry that currently reads "🧵 fix onesignal
prompt persistence race condition" by replacing the thread emoji (🧵) with the
patch/adhesive bandage emoji (🩹) so the header becomes "🩹 fix onesignal prompt
persistence race condition"; locate the changeset file containing that header
string and edit the first token to the correct emoji to match the PR title and
semantic meaning.
In `@src/utils/queryClient.ts`:
- Around line 35-38: The hydrated promise can resolve to void on success but to
the value returned by reportError on error; change the catch to explicitly
return void so the promise always resolves to void. Update the expression that
builds hydrated (which currently calls persistQueryClientRestore({ queryClient,
persister, maxAge: ... }).catch(reportError)) to instead call .catch((err) => {
reportError(err); /* return void */ }) or chain a .then(() => undefined) after
the call so that hydrated consistently resolves to void; reference hydrated,
persistQueryClientRestore, reportError, queryClient and persister when locating
the code to change.
- Around line 47-55: Add an inline comment in the persist() function explaining
that buster is intentionally an empty string to disable version-based cache
invalidation because the codebase relies on maxAge (30 days) for staleness;
update the comment near the persister.persistClient call (and mention the buster
field) so future maintainers know this is a deliberate design choice and not a
placeholder.
♻️ Duplicate comments (1)
src/utils/onesignal.ts (1)
76-82: Extract the multi-line hydration handler into a named function.The inline callback passed to
hydrated.thenis multi-line; prefer a function declaration to align with the coding guidelines about using function declarations for multi-line functions.♻️ Proposed refactor
OneSignal.InAppMessages.addEventListener("didDismiss", () => { queryClient.setQueryData(["onesignal", "dismiss"], Date.now()); persist().catch(reportError); }); + function triggerOnboardingPromptIfAllowed() { + const lastDismiss = queryClient.getQueryData<number>(["onesignal", "dismiss"]) ?? 0; + if (!appId || lastDismiss + DISMISS_EXPIRY >= Date.now()) return; + OneSignal.InAppMessages.addTrigger("onboard", "1"); + } return { enablePrompt: () => { - hydrated - .then(() => { - const lastDismiss = queryClient.getQueryData<number>(["onesignal", "dismiss"]) ?? 0; - if (!appId || lastDismiss + DISMISS_EXPIRY >= Date.now()) return; - OneSignal.InAppMessages.addTrigger("onboard", "1"); - }) - .catch(reportError); + hydrated.then(triggerOnboardingPromptIfAllowed).catch(reportError); },
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.eas/workflows/local.yaml:
- Line 59: The YAML inline comment after the "name: start emulator" entry has
only one space before the '#' which violates lint rules; update the line
containing the name: start emulator key so there are two spaces before the
inline comment (i.e., ensure "name: start emulator # cspell:ignore ..."),
preserving the existing comment text and spacing elsewhere.
♻️ Duplicate comments (3)
src/utils/queryClient.ts (2)
35-38: Hydration promise implementation is correct for the race condition fix.The
hydratedpromise ensures persistence restoration completes before any code reads persisted query data. The server-side check (typeof window === "undefined") correctly short-circuits to an immediate resolution.Note: A previous review flagged that the promise type resolves inconsistently (void vs string from
reportError). That's still applicable here.
47-55: ThePromise.resolve()wrapper may be redundant.If
persister.persistClient()already returns a promise (which it does based on the async-storage persister API), wrapping it inPromise.resolve()is unnecessary. However, this is a minor nitpick and doesn't affect correctness.The empty
bustercomment concern was flagged in a previous review.♻️ Optional simplification
export function persist() { - return Promise.resolve( - persister.persistClient({ - timestamp: Date.now(), - buster: "", - clientState: dehydrate(queryClient, dehydrateOptions), - }), - ); + return persister.persistClient({ + timestamp: Date.now(), + buster: "", + clientState: dehydrate(queryClient, dehydrateOptions), + }); }src/utils/onesignal.ts (1)
80-86: Consider extracting the multi-line callback (per past review).A previous review suggested extracting the inline callback passed to
shouldPrompt.then()into a named function, per coding guidelines preferring function declarations for multi-line functions. This is a style nitpick.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/queryClient.ts`:
- Around line 55-63: The persist function wraps persister.persistClient(...) in
an unnecessary Promise.resolve; modify persist to return the promise directly by
calling persister.persistClient with the same payload (timestamp: Date.now(),
buster: "", clientState: dehydrate(queryClient, dehydrateOptions)). Locate the
persist function and replace the Promise.resolve(...) wrapper so the function
directly returns persister.persistClient(...) (keep dehydrate, queryClient, and
dehydrateOptions usage unchanged).
♻️ Duplicate comments (1)
src/utils/onesignal.ts (1)
76-84: Core race condition fix looks correct.Awaiting
hydratedbefore checking the dismiss timestamp ensures the persisted state is loaded before making the prompt decision. The logic correctly suppresses the prompt if the user dismissed within the last 30 days.
Extract the inline callback to a named function.
Per coding guidelines, prefer function declarations for multi-line functions.
♻️ Proposed refactor
+ function maybeShowOnboardingPrompt() { + const lastDismiss = queryClient.getQueryData<number>(["onesignal", "dismiss"]) ?? 0; + if (!appId || lastDismiss + DISMISS_EXPIRY >= Date.now()) return; + OneSignal.InAppMessages.addTrigger("onboard", "1"); + } return { enablePrompt: () => { - hydrated - .then(() => { - const lastDismiss = queryClient.getQueryData<number>(["onesignal", "dismiss"]) ?? 0; - if (!appId || lastDismiss + DISMISS_EXPIRY >= Date.now()) return; - OneSignal.InAppMessages.addTrigger("onboard", "1"); - }) - .catch(reportError); + hydrated.then(maybeShowOnboardingPrompt).catch(reportError); },
cruzdanilo
force-pushed
the
onesignal
branch
2 times, most recently
from
a11e772 to
bac2cd9
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.