Repository navigation
fix cross-domain client plugin types - #391
Conversation
|
@zbeyens is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/plugins/cross-domain/client.ts (1)
93-93: 💤 Low valueConsider explicitly accepting all parameters for clarity.
The implementation accepts two parameters
(_, $store), but the type signature (viaParameters<NonNullable<BetterAuthClientPlugin["getActions"]>>) and test calls (line 141, 157 in the test file) suggest Better Auth expects three parameters. While TypeScript allows ignoring trailing parameters, explicitly accepting all three would improve clarity:- getActions(_, $store) { + getActions(_, $store, _options) {This makes the signature more explicit and aligns with the test calls.
🤖 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 93, The getActions method in the client.ts file currently accepts only two parameters (_, $store) but the type signature and test calls indicate it should accept three parameters. Update the getActions method signature to explicitly accept all three parameters instead of omitting the trailing parameter, which will improve code clarity and ensure the implementation aligns with the expected type signature from BetterAuthClientPlugin and the test calls shown at lines 141 and 157.src/plugins/cross-domain/client.test.ts (1)
146-146: 💤 Low valueNon-null assertions on
fetchPluginsaccess.The non-null assertions (
plugin.fetchPlugins!) suggest thatfetchPluginsis optional in theBetterAuthClientPlugintype. While thecrossDomainClientimplementation always providesfetchPlugins, this assertion pattern could mask issues if:
- The plugin implementation changes in the future
- These helpers are used with other plugins
Consider adding a runtime check if the helpers might be used with other plugins:
if (!plugin.fetchPlugins) { throw new Error("Plugin does not provide fetchPlugins"); } return plugin.fetchPlugins[0].hooks!.onSuccess!;For test-only code specifically testing
crossDomainClient, the current approach is acceptable.Also applies to: 158-158
🤖 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` at line 146, The non-null assertions on plugin.fetchPlugins access are acceptable in this test file since the code is specifically testing the crossDomainClient implementation which always provides fetchPlugins. Since this is test-only code, no change is required. However, if similar helper patterns were to be extracted and used in production code with other plugin types, consider adding a runtime check to verify that fetchPlugins exists before accessing it, as suggested in the comment example.
🤖 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.types.test.ts`:
- Around line 1-5: Remove the `adminClient` import from the import statement in
the test file since it is not exported from the local repository's client
plugins module at `src/client/plugins/index.ts`. The import from
"better-auth/client/plugins" is unresolvable in this context and the symbol is
not being used in the test, so simply delete this import while keeping the other
imports intact (createAuthClient, expectTypeOf, it, convexClient, and
crossDomainClient).
---
Nitpick comments:
In `@src/plugins/cross-domain/client.test.ts`:
- Line 146: The non-null assertions on plugin.fetchPlugins access are acceptable
in this test file since the code is specifically testing the crossDomainClient
implementation which always provides fetchPlugins. Since this is test-only code,
no change is required. However, if similar helper patterns were to be extracted
and used in production code with other plugin types, consider adding a runtime
check to verify that fetchPlugins exists before accessing it, as suggested in
the comment example.
In `@src/plugins/cross-domain/client.ts`:
- Line 93: The getActions method in the client.ts file currently accepts only
two parameters (_, $store) but the type signature and test calls indicate it
should accept three parameters. Update the getActions method signature to
explicitly accept all three parameters instead of omitting the trailing
parameter, which will improve code clarity and ensure the implementation aligns
with the expected type signature from BetterAuthClientPlugin and the test calls
shown at lines 141 and 157.
🪄 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: f6342caf-2e33-4913-98e0-c693e0ec68a4
📒 Files selected for processing (3)
src/plugins/cross-domain/client.test.tssrc/plugins/cross-domain/client.tssrc/plugins/cross-domain/client.types.test.ts
|
@codex review |
commit: |
🐛 Fixes #195
🟢 95-100% confidence
/tmp/convex-ba-repro-j7CbNK npm run typecheckfailed before patch withbetter-auth@1.6.18:crossDomainClient()rejected asBetterAuthClientPlugin;getCookiemissingnpm run build;npm run test;npm run lint;npm run typecheck; repacked tarball repro passed againstbetter-auth@1.6.18✅ Outcome
crossDomainClient()now composes with current Better Auth 1.6.x client plugins and keeps inferred actions such asgetCookie.This does not fix Better Auth's broader inferred-client type explosion/OOM class. It fixes the
@convex-dev/better-authcompatibility bug that made the supported peer range lie.🏗️ Design
The fix is a type-boundary repair on
crossDomainClient(): use Better Auth's current client plugin contract, keep the currentgetActionsparameter shape, and preserve the concrete cross-domain server plugin/actions inference. Runtime object shape is unchanged.🧪 Verified
npm run buildnpx vitest run --typecheck src/plugins/cross-domain/client.types.test.ts src/plugins/cross-domain/client.test.tsnpm run testnpm run lintnpm run typecheck/tmp/convex-ba-repro-j7CbNK npm run typecheckwith repacked package +better-auth@1.6.18autoreview --mode local: clean, no accepted/actionable findings