Add API domain override and update OTP endpoint #7994
Conversation
Introduces an 'api' domain override in domains.ts and updates the OTP authentication flow to use the new API endpoint for initiation. This change improves flexibility for API server configuration and aligns OTP requests with the updated backend route. Closes BLD-220
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an API domain option to domain configuration and updates in-app wallet OTP to call api.thirdweb.com for OTP initiation (sendOtp) with a typed request body; OTP verification (verifyOtp) URL construction and behavior remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as In-app OTP module
participant API as api.thirdweb.com
rect rgb(230,245,255)
Note over C,A: Initiate OTP
C->>A: sendOtp(email|phone)
A->>API: POST /v1/auth/initiate
Note right of A: body { method: "email"|"sms", email? , phone? }
API-->>A: 200 / 4xx
A-->>C: void on success / throws on error
end
rect rgb(245,255,235)
Note over C,A: Verify OTP
C->>A: verifyOtp(code, email|phone)
A->>API: POST getLoginCallbackUrl(...) (existing helper)
Note right of A: body { code, email? , phone? }
API-->>A: 200/4xx JSON
A-->>C: Parsed JSON or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/thirdweb/src/utils/domains.ts (2)
17-21: Clarify “base URL” vs. hostname; optionally accept full http(s) URLsDocs say “base URL” but defaults are hostnames. If callers pass an overridden value with
http(s)://,getThirdwebBaseUrlwill double‑prefix unless we normalize. Consider either clarifying the doc to “hostname (no protocol)” or accepting full URLs.Optional normalization:
export const getThirdwebBaseUrl = (service: keyof DomainOverrides) => { const origin = domains[service]; - if (origin.startsWith("localhost")) { + if (/^https?:\/\//i.test(origin)) { + return origin; + } + if (origin.startsWith("localhost")) { return `http://${origin}`; } return `https://${origin}`; };
85-99: Rename param to avoid shadowing theDomainOverridestypeMinor readability nit: parameter
DomainOverridesshadows the type name and breaks usual casing conventions.-export const setThirdwebDomains = (DomainOverrides: DomainOverrides) => { +export const setThirdwebDomains = (overrides: DomainOverrides) => { domains = { - analytics: DomainOverrides.analytics ?? DEFAULT_ANALYTICS_URL, - api: DomainOverrides.api ?? DEFAULT_API_URL, - bridge: DomainOverrides.bridge ?? DEFAULT_BRIDGE_URL, - bundler: DomainOverrides.bundler ?? DEFAULT_BUNDLER_URL, - engineCloud: DomainOverrides.engineCloud ?? DEFAULT_ENGINE_CLOUD_URL, - inAppWallet: DomainOverrides.inAppWallet ?? DEFAULT_IN_APP_WALLET_URL, - insight: DomainOverrides.insight ?? DEFAULT_INSIGHT_URL, - pay: DomainOverrides.pay ?? DEFAULT_PAY_URL, - rpc: DomainOverrides.rpc ?? DEFAULT_RPC_URL, - social: DomainOverrides.social ?? DEFAULT_SOCIAL_URL, - storage: DomainOverrides.storage ?? DEFAULT_STORAGE_URL, + analytics: overrides.analytics ?? DEFAULT_ANALYTICS_URL, + api: overrides.api ?? DEFAULT_API_URL, + bridge: overrides.bridge ?? DEFAULT_BRIDGE_URL, + bundler: overrides.bundler ?? DEFAULT_BUNDLER_URL, + engineCloud: overrides.engineCloud ?? DEFAULT_ENGINE_CLOUD_URL, + inAppWallet: overrides.inAppWallet ?? DEFAULT_IN_APP_WALLET_URL, + insight: overrides.insight ?? DEFAULT_INSIGHT_URL, + pay: overrides.pay ?? DEFAULT_PAY_URL, + rpc: overrides.rpc ?? DEFAULT_RPC_URL, + social: overrides.social ?? DEFAULT_SOCIAL_URL, + storage: overrides.storage ?? DEFAULT_STORAGE_URL, }; };packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (5)
16-23: AddAccept: application/jsonheaderHelps negotiate JSON responses and makes intent explicit.
const headers: Record<string, string> = { "Content-Type": "application/json", + "Accept": "application/json", "x-client-id": client.clientId, };
32-46: Make the strategy switch exhaustiveGuard against unexpected strategies to fail fast in dev.
const body = (() => { switch (args.strategy) { case "email": return { type: "email", email: args.email, }; case "phone": return { type: "phone", phone: args.phoneNumber, }; + default: { + const neverStrategy: never = args.strategy as never; + throw new Error(`Unsupported strategy: ${String(neverStrategy)}`); + } } })();
53-55: Improve error detailIncluding status (and optionally response text) eases debugging and Sentry grouping.
- if (!response.ok) { - throw new Error("Failed to send verification code"); - } + if (!response.ok) { + throw new Error(`Failed to send verification code (status ${response.status})`); + }
76-87: MirrorAccept: application/jsonhere tooStay consistent with
sendOtp.const headers: Record<string, string> = { "Content-Type": "application/json", + "Accept": "application/json", "x-client-id": client.clientId, };
110-112: Polish error messageMinor grammar and add status for debugging.
- if (!response.ok) { - throw new Error("Failed to verify verification code"); - } + if (!response.ok) { + throw new Error(`Failed to verify code (status ${response.status})`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/thirdweb/src/utils/domains.ts(4 hunks)packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/utils/domains.tspackages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/utils/domains.tspackages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/utils/domains.tspackages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
🧬 Code graph analysis (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (2)
packages/thirdweb/src/wallets/in-app/core/authentication/types.ts (2)
PreAuthArgsType(17-20)AuthStoredTokenWithCookieReturnType(145-151)packages/thirdweb/src/utils/domains.ts (1)
getThirdwebBaseUrl(111-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/thirdweb/src/utils/domains.ts (2)
62-62: DEFAULT_API_URL addition looks goodGood default and consistent with the other domain constants.
73-83: Addingapito the domains map is correctThe map remains exhaustive for
keyof DomainOverrides. LGTM.packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (3)
2-2: Correct: usegetThirdwebBaseUrl("api")for OTP initiationImport aligns with the new API domain override.
70-75: LGTM: verification URL computation unchangedKeeping verification on the existing callback path is consistent with cookie handling and existing flows.
57-58: AlignsendOtpsignature and behavior
Inpackages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts, replacereturn await response.json();with
return;so the
Promise<void>return type matches. Verified no callers consume the JSON response.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (2)
53-55: Surface server error details (status/body) in thrown errors.Current messages mask status and API error payloads, hindering debugging.
- if (!response.ok) { - throw new Error("Failed to send verification code"); - } + if (!response.ok) { + const errText = await response.text().catch(() => ""); + throw new Error( + `Failed to send verification code: ${response.status} ${response.statusText}${errText ? ` - ${errText}` : ""}`, + ); + }- if (!response.ok) { - throw new Error("Failed to verify verification code"); - } + if (!response.ok) { + const errText = await response.text().catch(() => ""); + throw new Error( + `Failed to verify verification code: ${response.status} ${response.statusText}${errText ? ` - ${errText}` : ""}`, + ); + }Also applies to: 110-112
47-51: Add cancellation/timeout support to fetch calls.Initiation/verification are network-bound; without AbortSignal, calls can hang and block UX.
Suggestion: plumb an optional signal through args and pass it to fetch:
// types.ts (example) export type WithSignal<T> = T & { signal?: AbortSignal };// usage export const sendOtp = async (args: WithSignal<PreAuthArgsType>): Promise<void> => { … const response = await fetch(url, { body: stringify(body), headers, method: "POST", signal: args.signal, }); }I can wire this through both functions if you confirm the types location.
Also applies to: 104-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
🧠 Learnings (1)
📚 Learning: 2025-05-30T17:14:25.332Z
Learnt from: MananTank
PR: thirdweb-dev/js#7227
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/modules/components/OpenEditionMetadata.tsx:26-26
Timestamp: 2025-05-30T17:14:25.332Z
Learning: The ModuleCardUIProps interface already includes a client prop of type ThirdwebClient, so when components use `Omit<ModuleCardUIProps, "children" | "updateButton">`, they inherit the client prop without needing to add it explicitly.
Applied to files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
🧬 Code graph analysis (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (1)
packages/thirdweb/src/utils/domains.ts (1)
getThirdwebBaseUrl(111-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Size
- GitHub Check: Unit Tests
🔇 Additional comments (2)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (2)
2-2: Confirm cross-domain flow (initiate on api, verify via login callback).Initiation now targets getThirdwebBaseUrl("api") while verification still uses getLoginCallbackUrl (likely login/iaw). If tokens/cookies are domain‑scoped, this split can introduce CORS/cookie domain issues in some environments. Verify this is intentional and covered by DomainOverrides, and that staging/prod behave correctly.
Do you want me to open a follow-up to unify both steps on the API domain (if supported) or add a regression test covering this cross-domain flow?
Also applies to: 4-4, 70-75
36-38: Good addition: explicit type discriminator in request body.Adding type: "email" | "phone" clarifies payload semantics and future‑proofs the API.
Also applies to: 41-43
Changed the OTP initiation payload to use 'method' instead of 'type', and set phone strategy to 'sms' instead of 'phone'. Also removed the unused return value from sendOtp to match updated API expectations.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Included 'api.thirdweb.com' in the domain list for test coverage in domain.test.ts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (1)
15-16: Resolved: return-type mismatch in sendOtp.Now returns void explicitly, matching the signature and addressing the earlier bot note.
Also applies to: 57-58
🧹 Nitpick comments (2)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (2)
53-55: Include status and server message in thrown errors.Helps debugging without needing network tracing.
if (!response.ok) { - throw new Error("Failed to send verification code"); + const details = await response.text().catch(() => ""); + throw new Error( + `Failed to send verification code: ${response.status} ${response.statusText}${details ? ` — ${details}` : ""}`, + ); }if (!response.ok) { - throw new Error("Failed to verify verification code"); + const details = await response.text().catch(() => ""); + throw new Error( + `Failed to verify code: ${response.status} ${response.statusText}${details ? ` — ${details}` : ""}`, + ); }Also applies to: 110-112
19-31: DRY the repeated auth headers into a small helper.Reduces duplication and keeps the two flows in lockstep.
+// local helper +const buildAuthHeaders = ({ + client, + ecosystem, +}: { + client: ThirdwebClient; + ecosystem?: Ecosystem; +}): Record<string, string> => { + const headers: Record<string, string> = { + "Content-Type": "application/json", + "x-client-id": client.clientId, + }; + if (ecosystem?.id) headers["x-ecosystem-id"] = ecosystem.id; + if (ecosystem?.partnerId) headers["x-ecosystem-partner-id"] = ecosystem.partnerId; + return headers; +};- const headers: Record<string, string> = { - "Content-Type": "application/json", - "x-client-id": client.clientId, - }; - if (ecosystem?.id) { - headers["x-ecosystem-id"] = ecosystem.id; - } - if (ecosystem?.partnerId) { - headers["x-ecosystem-partner-id"] = ecosystem.partnerId; - } + const headers = buildAuthHeaders({ client, ecosystem });- const headers: Record<string, string> = { - "Content-Type": "application/json", - "x-client-id": client.clientId, - }; - if (ecosystem?.id) { - headers["x-ecosystem-id"] = ecosystem.id; - } - if (ecosystem?.partnerId) { - headers["x-ecosystem-partner-id"] = ecosystem.partnerId; - } + const headers = buildAuthHeaders({ client, ecosystem });Also applies to: 76-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
🧠 Learnings (1)
📚 Learning: 2025-05-30T17:14:25.332Z
Learnt from: MananTank
PR: thirdweb-dev/js#7227
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/modules/components/OpenEditionMetadata.tsx:26-26
Timestamp: 2025-05-30T17:14:25.332Z
Learning: The ModuleCardUIProps interface already includes a client prop of type ThirdwebClient, so when components use `Omit<ModuleCardUIProps, "children" | "updateButton">`, they inherit the client prop without needing to add it explicitly.
Applied to files:
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts
🧬 Code graph analysis (1)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (1)
packages/thirdweb/src/utils/domains.ts (1)
getThirdwebBaseUrl(111-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (2)
2-4: API base URL adoption looks correct.Using getThirdwebBaseUrl("api") and dropping the legacy initiation path aligns with the PR goal. No issues spotted.
Also applies to: 17-17
63-75: No changes needed: OTP domain split is intentional
OTP initiation correctly targets the API domain (getThirdwebBaseUrl("api")/v1/auth/initiate) to send codes, while verification reuses the in-app-wallet login callback endpoint (getLoginCallbackUrl)—this matches other flows (guest, SIWE).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/thirdweb/src/utils/domain.test.ts (2)
35-47: Add an override test for api to validate DomainOverrides.api wiring.This directly exercises the new override path introduced in the PR.
describe("setThirdwebDomains", () => { it("should override specific domains while keeping others as default", () => { setThirdwebDomains({ analytics: "custom.analytics.com", rpc: "custom.rpc.com", }); expect(getThirdwebDomains()).toEqual({ ...defaultDomains, analytics: "custom.analytics.com", rpc: "custom.rpc.com", }); }); + it("should override the api domain", () => { + setThirdwebDomains({ api: "custom.api.com" }); + expect(getThirdwebDomains()).toEqual({ + ...defaultDomains, + api: "custom.api.com", + }); + });
59-76: Add base-URL tests for api (default and localhost override).Ensures HTTPS for default api host and HTTP for localhost, mirroring existing RPC coverage.
describe("getThirdwebBaseUrl", () => { it("should return an HTTPS URL for non-localhost domains", () => { const baseUrl = getThirdwebBaseUrl("rpc"); expect(baseUrl).toBe(`https://${DEFAULT_RPC_URL}`); }); + it("should return an HTTPS URL for the api domain by default", () => { + const baseUrl = getThirdwebBaseUrl("api"); + expect(baseUrl).toBe(`https://${DEFAULT_API_URL}`); + }); + it("should return an HTTP URL for localhost domains", () => { setThirdwebDomains({ rpc: "localhost:8545" }); const baseUrl = getThirdwebBaseUrl("rpc"); expect(baseUrl).toBe("http://localhost:8545"); }); + it("should return an HTTP URL for localhost api domain", () => { + setThirdwebDomains({ api: "localhost:3001" }); + const baseUrl = getThirdwebBaseUrl("api"); + expect(baseUrl).toBe("http://localhost:3001"); + });
🧹 Nitpick comments (1)
packages/thirdweb/src/utils/domain.test.ts (1)
1-7: Import DEFAULT_API_URL for parity with RPC tests.Use the exported constant to avoid literal drift in API-related expectations.
import { - DEFAULT_RPC_URL, + DEFAULT_RPC_URL, + DEFAULT_API_URL, getThirdwebBaseUrl, getThirdwebDomains, setThirdwebDomains, } from "./domains.js";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/thirdweb/src/utils/domain.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/utils/domain.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
**/*.test.{ts,tsx}: Co‑locate tests asfoo.test.ts(x)next to the implementation
Use real function invocations with stub data; avoid brittle mocks
Use MSW to intercept HTTP calls for network interactions; mock only hard‑to‑reproduce scenarios
Keep tests deterministic and side‑effect free; use Vitest
Files:
packages/thirdweb/src/utils/domain.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/utils/domain.test.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/utils/domain.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/utils/domain.test.ts (1)
21-22: Good: defaultDomains now includes api. Add focused tests for the new surface.The defaults assertion covers presence, but we’re missing behavior tests for api base URL and overrides. See follow-up comments with diffs to add them.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (55.55%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7994 +/- ##
=======================================
Coverage 56.63% 56.64%
=======================================
Files 904 904
Lines 58677 58683 +6
Branches 4161 4163 +2
=======================================
+ Hits 33231 33240 +9
+ Misses 25340 25337 -3
Partials 106 106
🚀 New features to boost your workflow:
|
Introduces an 'api' domain override in domains.ts and updates the OTP authentication flow to use the new API endpoint for initiation. This change improves flexibility for API server configuration and aligns OTP requests with the updated backend route.
Closes BLD-220
PR-Codex overview
This PR introduces the
apiURL configuration to the application, enhancing the domain management by adding a base URL for the API server. It also updates thesendOtpfunction to utilize this new base URL for API requests.Detailed summary
apifield to the domain configuration.DEFAULT_API_URL.sendOtpfunction to use the new API base URL for initiating authentication.getLoginUrl.Summary by CodeRabbit
New Features
Refactor
Tests