Repository navigation
fix:(cross-domain): support async storage implementations - #418
Choco-milk-for-u wants to merge 2 commits into
Conversation
|
@Choco-milk-for-u is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: get-convex/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe cross-domain client now accepts synchronous or asynchronous Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/plugins/cross-domain/client.types.test.ts (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the second async accessor too.
getSessionDataalso changed inCrossDomainActions, but this test only protectsgetCookie. Add an assertion for() => Promise<Record<string, unknown> | null>.🤖 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 `@src/plugins/cross-domain/client.types.test.ts` at line 18, Add a type assertion for CrossDomainActions.getSessionData alongside the existing authClient.getCookie assertion, verifying its type is () => Promise<Record<string, unknown> | null> and preserving coverage of both async accessors.src/plugins/cross-domain/client.test.ts (1)
162-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a runtime test with asynchronous
getItem.The fixture used by
getActions()still returnsstring | null, so these tests do not exercise the new Promise-returning storage path. Add at least one mock whosegetItemreturnsPromise.resolve(...).🤖 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 `@src/plugins/cross-domain/client.test.ts` around lines 162 - 189, Add a test case in the getSessionData suite that configures the getActions() storage fixture with an asynchronous getItem implementation returning Promise.resolve(...), then verifies the expected parsed session data or null result. Keep the existing synchronous storage coverage unchanged and ensure the async mock exercises the Promise-returning path.
🤖 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 `@src/plugins/cross-domain/client.ts`:
- Line 12: Update the examples using the Promise-returning accessors near the
getCookie and corresponding accessor declarations so their containing examples
are async and both accessor calls are awaited. Preserve the existing example
behavior while ensuring the calls are handled asynchronously.
- Around line 168-171: Serialize the cookie read-modify-write sequence around
getSetCookie in the success handlers so concurrent operations cannot read the
same stale cookie and overwrite each other. Apply the same synchronization to
both cookie mutation paths, including the corresponding logic near the second
occurrence, while preserving the existing merge and write behavior.
- Line 76: Update the storage interface alongside getItem so setItem returns
void or Promise<void>, then make the storage.setItem calls in the success/cache
path awaitable and await them before completing onSuccess. Preserve synchronous
storage compatibility while ensuring rejected async writes propagate instead of
being dropped.
---
Nitpick comments:
In `@src/plugins/cross-domain/client.test.ts`:
- Around line 162-189: Add a test case in the getSessionData suite that
configures the getActions() storage fixture with an asynchronous getItem
implementation returning Promise.resolve(...), then verifies the expected parsed
session data or null result. Keep the existing synchronous storage coverage
unchanged and ensure the async mock exercises the Promise-returning path.
In `@src/plugins/cross-domain/client.types.test.ts`:
- Line 18: Add a type assertion for CrossDomainActions.getSessionData alongside
the existing authClient.getCookie assertion, verifying its type is () =>
Promise<Record<string, unknown> | null> and preserving coverage of both async
accessors.
🪄 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: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c18f95f6-8b5f-42f6-9211-f216b066ffc8
📒 Files selected for processing (3)
src/plugins/cross-domain/client.test.tssrc/plugins/cross-domain/client.tssrc/plugins/cross-domain/client.types.test.ts
| storage?: { | ||
| setItem: (key: string, value: string) => any; | ||
| getItem: (key: string) => string | null; | ||
| getItem: (key: string) => Promise<string | null> | string | null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n 'setItem\s*:|setItem\s*\(' src/plugins/cross-domainRepository: get-convex/better-auth
Length of output: 1100
🏁 Script executed:
#!/usr/bin/env bash
sed -n '60,280p' src/plugins/cross-domain/client.tsRepository: get-convex/better-auth
Length of output: 6919
🏁 Script executed:
#!/usr/bin/env bash
rg -n 'CrossDomainClientPlugin|storage\?:|setItem:\s*\(|getItem:\s*\(' src/plugins src | sed -n '1,220p'Repository: get-convex/better-auth
Length of output: 1962
Type setItem as promise-capable and await the cache writes. getItem already accepts async storage, so setItem should return void | Promise<void> too; otherwise the storage.setItem(...) calls in the success/cache path can drop rejected writes and let onSuccess finish before persistence completes.
🤖 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 `@src/plugins/cross-domain/client.ts` at line 76, Update the storage interface
alongside getItem so setItem returns void or Promise<void>, then make the
storage.setItem calls in the success/cache path awaitable and await them before
completing onSuccess. Preserve synchronous storage compatibility while ensuring
rejected async writes propagate instead of being dropped.
| const prevCookie = await storage.getItem(cookieName); | ||
| const toSetCookie = getSetCookie( | ||
| setCookie || "", | ||
| prevCookie ?? undefined |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize cookie read-modify-write operations.
The new await storage.getItem(...) introduces a yield between reading and writing. Concurrent success handlers can therefore merge against the same stale cookie and let the last write discard another response’s session or challenge cookie. Serialize these mutations or use an atomic storage operation.
Also applies to: 210-213
🤖 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 `@src/plugins/cross-domain/client.ts` around lines 168 - 171, Serialize the
cookie read-modify-write sequence around getSetCookie in the success handlers so
concurrent operations cannot read the same stale cookie and overwrite each
other. Apply the same synchronization to both cookie mutation paths, including
the corresponding logic near the second occurrence, while preserving the
existing merge and write behavior.
storage methods were expected to be synchronous. This change allows them to return promises as well, so we can pass a storage implementation other than localstorage in crossdomain plugin.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.