Sync user cookie less often#2713
Open
drew-harris wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR removes Reactor's automatic periodic user sync and changes syncUserToEndpoint to check the fetch response and throw on non-OK HTTP status; backend docs were updated with an on-demand sync example. ChangesUser sync behavior and error handling
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/www/app/docs/backend/page.md (1)
514-516: 🏗️ Heavy liftAvoid documenting private
_reactorAPIs as the supported path.This snippet encourages use of internals (
db.core._reactor), which are not a stable public contract. Consider adding a public method (for example ondb.auth) and documenting that instead.🤖 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 `@client/www/app/docs/backend/page.md` around lines 514 - 516, The docs currently show usage of the private internals db.core._reactor.getCurrentUser() and syncUserToEndpoint(), which should not be documented as a public API; add and document a stable public wrapper (for example a method on db.auth such as db.auth.syncCurrentUserToEndpoint or db.auth.getAndSyncCurrentUser) and update the snippet to call that public method instead of db.core._reactor.* so consumers use the supported contract; implement the public method to internally call getCurrentUser and syncUserToEndpoint so behavior is unchanged while hiding the private _reactor usage.
🤖 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 `@client/www/app/docs/backend/page.md`:
- Around line 513-517: The current code passes the entire AuthState returned by
db.core._reactor.getCurrentUser() into syncUserToEndpoint; instead, extract and
pass the actual user object (the "user" field) to syncUserToEndpoint. Locate
db.core._reactor.getCurrentUser() and change the call site so it awaits or .then
the result and forwards result.user (or the equivalent property) to
db.core._reactor.syncUserToEndpoint rather than the full AuthState wrapper.
---
Nitpick comments:
In `@client/www/app/docs/backend/page.md`:
- Around line 514-516: The docs currently show usage of the private internals
db.core._reactor.getCurrentUser() and syncUserToEndpoint(), which should not be
documented as a public API; add and document a stable public wrapper (for
example a method on db.auth such as db.auth.syncCurrentUserToEndpoint or
db.auth.getAndSyncCurrentUser) and update the snippet to call that public method
instead of db.core._reactor.* so consumers use the supported contract; implement
the public method to internally call getCurrentUser and syncUserToEndpoint so
behavior is unchanged while hiding the private _reactor usage.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51fa72eb-b5aa-4b67-8501-b86fc1372873
📒 Files selected for processing (2)
client/packages/core/src/Reactor.jsclient/www/app/docs/backend/page.md
Contributor
|
View Vercel preview at instant-www-js-drewh-user-sync-less-often-jsv.vercel.app. |
stopachka
reviewed
May 28, 2026
| this._oauthCallbackResponse = this._oauthLoginInit(); | ||
|
|
||
| // kick off a request to cache it | ||
| this.getCurrentUser().then((userInfo) => { |
Contributor
There was a problem hiding this comment.
SGTM, but I do wonder: how long will these cookies last?
Perhaps we should do something like once every 7 days. It begs the question: how will we know? I guess we could store something in local storage.
WDYT @dwwoelfel @drew-harris @nezaj ?
Contributor
Author
There was a problem hiding this comment.
oh good point the cookies do expire so we'd probably want a local storage to re-sync before they expire
drew-harris
force-pushed
the
drewh/user-sync-less-often
branch
from
bd94169 to
5c4efff
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/packages/core/src/Reactor.js (1)
2193-2206: ⚡ Quick winGood addition of HTTP status checking.
The change to capture the response and throw on non-OK status is correct—
fetch()only rejects on network errors, not HTTP 4xx/5xx responses.Optional: Include response body in error message for better debugging
}); if (!response.ok) { - throw new Error(`HTTP error! status: ${response.status}`); + const body = await response.text().catch(() => ''); + throw new Error(`HTTP error! status: ${response.status}${body ? `, body: ${body}` : ''}`); } } catch (error) {🤖 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 `@client/packages/core/src/Reactor.js` around lines 2193 - 2206, The HTTP error throw in the Reactor.js POST (the fetch block that posts type: 'sync-user' using this.config.firstPartyPath) should include the response body to aid debugging; after checking !response.ok, read the response text (or attempt json) and include that content and the response.status (and optionally response.statusText or request URL via this.config.firstPartyPath) in the thrown Error message so callers get both status and server error details.
🤖 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.
Nitpick comments:
In `@client/packages/core/src/Reactor.js`:
- Around line 2193-2206: The HTTP error throw in the Reactor.js POST (the fetch
block that posts type: 'sync-user' using this.config.firstPartyPath) should
include the response body to aid debugging; after checking !response.ok, read
the response text (or attempt json) and include that content and the
response.status (and optionally response.statusText or request URL via
this.config.firstPartyPath) in the thrown Error message so callers get both
status and server error details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fb88388-ec6d-4f5c-9e17-90c41a15d155
📒 Files selected for processing (2)
client/packages/core/src/Reactor.jsclient/www/app/docs/backend/page.md
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
2 participants
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.
Stop calling cookie sync with firstPartyPath every time reactor starts up and every minute. Should now only be called on state changes.
Includes a helpful snippet in the docs for when a user wants to manually sync.
Ex: the user could put their own setInterval in their _app.tsx that wouldn't run on page navigations.
Question:
Should we include a function on db.auth to run a manual sync?