Repository navigation
fix(sdk): browser compat — fetch binding + Web Crypto PKCE — 2.0.0 (breaking: async PKCE) - #1948
Conversation
Two independent browser-only bugs found building a real end-to-end demo (external npm install, Playwright-driven Chromium), invisible to the SDK's all-Node vitest suite: - client.ts captured a detached `fetch` reference (`options.fetch ?? fetch`), later called in method position through an options object. Browsers require fetch's receiver to be the real Window object; Node's undici fetch has no such check, so every call threw `TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation` before ever reaching the network. Fixed by binding to globalThis at construction time. - auth/pkce.ts used node:crypto's createHash and Buffer directly, breaking any browser app implementing its own OAuth login flow (the functions are SDK public surface). Rewritten on the Web Crypto API (crypto.subtle.digest, btoa), which both Node 20+ and every browser provide. deriveCodeChallenge is now async — its one caller (packages/cli/src/auth/loopback-flow.ts) now awaits it. Output is byte-for-byte identical to the old Buffer-based encoding, verified by the existing drift-guard test against @pagespace/lib's canonical copy. Bumps @pagespace/sdk to 1.5.2.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfa738bc80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| export function deriveCodeChallenge(verifier: string): string { | ||
| return createHash('sha256').update(verifier, 'ascii').digest('base64url'); | ||
| export async function deriveCodeChallenge(verifier: string): Promise<string> { |
There was a problem hiding this comment.
Publishing this as @pagespace/sdk 1.5.2 changes the exported deriveCodeChallenge contract from string to Promise<string>. Existing consumers that accept patch/minor updates will pick this up without source changes; in this repo, the published @pagespace/cli package still declares @pagespace/sdk: ^1.5.0, and its pre-change loopback flow called this synchronously, so a fresh install can send [object Promise] as the OAuth code_challenge and break login. Please avoid changing this public API in a patch release, or coordinate a compatible CLI/major release path.
Useful? React with 👍 / 👎.
Codex review flagged (P1, correctly): deriveCodeChallenge's sync -> async signature change is a breaking change to public SDK surface, and the already-published @pagespace/cli@1.5.0 depends on "@pagespace/sdk": "^1.5.0" with a loopback-flow.ts that (pre-this-PR) calls it synchronously. Publishing this fix as 1.5.2 (a patch) would have meant every EXISTING CLI install silently picked up the new async SDK on its next `npm install`/reinstall and started sending "[object Promise]" as the OAuth code_challenge — breaking `pagespace login` for users who did nothing wrong, with no code change on their end at all. Confirmed live: @pagespace/cli@1.5.0 is the current npm-published version, and it does declare that ^1.5.0 range. Fix: ship as 2.0.0. npm's ^1.5.0 range on the already-published CLI does not resolve to 2.x, so existing installs stay on the safe, synchronous 1.5.1 until they explicitly opt in. This repo's own packages/cli is the one consumer that DOES opt in — bumped its @pagespace/sdk dependency to ^2.0.0 and its own version to 1.5.1 (a patch from the CLI's own consumer-facing perspective; its public behavior is unchanged, only its internal SDK call is now awaited). - packages/sdk: package.json + version.ts -> 2.0.0; CHANGELOG.md rewritten with an explicit Breaking Changes section and migration note (await the previously-sync deriveCodeChallenge call). - packages/cli: package.json's sdk dependency -> ^2.0.0, own version -> 1.5.1 (CLI_VERSION + package.json, kept in lockstep per the existing version.test.ts guard); CHANGELOG.md entry explaining why. Full suites re-verified after the bump: packages/sdk (761/761), packages/cli (999/999), both typecheck clean.
2witstudios
changed the title
|
@chatgpt-codex-connector Confirmed and fixed — good catch. Verified live: `@pagespace/cli@1.5.0` is the current npm-published version and does declare `"@pagespace/sdk": "^1.5.0"`, with a pre-fix `loopback-flow.ts` that calls `deriveCodeChallenge` synchronously. Publishing this as a `1.5.2` patch would indeed have broken every existing CLI install's login flow on the next reinstall. Fixed in `4c38a6a1e`: shipping the SDK as `2.0.0` instead of `1.5.2`. npm's `^1.5.0` range on the already-published CLI does not resolve to `2.x`, so existing installs stay safely on `1.5.1` (synchronous) until they explicitly opt in. This repo's own `packages/cli` is the one consumer that does opt in — bumped its `@pagespace/sdk` dependency to `^2.0.0` and its own version to `1.5.1`. Both packages' CHANGELOG.md now have explicit breaking-change/migration notes. Both suites re-verified after the bump (761/761 SDK, 999/999 CLI). |
Summary
Testing
|
|
@chatgpt-codex-connector Thanks for re-verifying the fix. On the typecheck warning: investigated but could not reproduce it. Ran the exact command you cited (`bun run --filter '@pagespace/sdk' typecheck`) from repo root — passes clean, exit 0. This repo's actual installed/pinned compiler (per `bunx tsc --version` and package.json's `"typescript": "^5.8.3"`) is TypeScript 5.8.3, not 6.x, so the TS5101 `baseUrl`-deprecation enforcement you saw doesn't apply here — that sounds like your sandbox resolved a different (newer/canary) TypeScript than this repo's lockfile pins. The real CI's `ci / Lint & TypeScript Check` job also passed clean on this exact commit (9m20s). No action taken since I can't reproduce a real failure against this repo's actual toolchain. |
Resolves the only conflict (bun.lock — packages/sdk version drift, 1.5.1 -> 2.0.0 from master's SDK browser-compat release #1948) by regenerating the lockfile via bun install. No other conflicts. Rebuilt @pagespace/db and @pagespace/lib; re-ran affected tests (39 pass) and apps/web typecheck (clean) against the merged tree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRk3t3kn4hjYz2WfSUQHWj
Vite/React app exercising the SDK (drives/pages/activity/calendar/ collaborators/search/roles/members/agents/conversations/export/workflows/ commands/tokens, plus a create+trash write-path check) as a real npm consumer would use it. Built to root-cause and verify the browser fetch-binding + CORS/CORP fixes (SDK 2.0.0, PR #1948/#1950, and PageSpace-Deploy #17/#18) end-to-end against production. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TaPmsPMN99P5awqV9qsEqV
1 participant
Summary
packages/sdk/src/client.ts: bindfetchtoglobalThisat construction time. The previousoptions.fetch ?? fetchcaptured a detached reference, later invoked in method position (options.fetch(url, ...)) — Chromium/real-browserfetchrequires its receiver to be the realWindowobject and throwsTypeError: Failed to execute 'fetch' on 'Window': Illegal invocationotherwise. Node's undicifetchhas no such receiver check, so this was invisible to the all-Node vitest suite and only surfaced building a real browser demo.packages/sdk/src/auth/pkce.ts: replacenode:crypto'screateHash/Bufferwith the Web Crypto API (crypto.subtle.digest,btoa) so the SDK's public PKCE helpers (generateCodeVerifier/deriveCodeChallenge) work in a browser bundle, not just Node.deriveCodeChallengeis nowasync; updated its one in-repo caller,packages/cli/src/auth/loopback-flow.ts, toawaitit. Output is byte-for-byte identical to the previousBuffer-based encoding — verified by the existing drift-guard test against@pagespace/lib's canonical (Node-only) copy.Update after review: Codex correctly flagged (P1) that the async signature change is a breaking change to public SDK surface, and the already-published
@pagespace/cli@1.5.0on npm depends on@pagespace/sdk: ^1.5.0with a pre-fixloopback-flow.tsthat calls this synchronously — publishing as a1.5.2patch would have meant every existing CLI install silently brokepagespace login(code_challengebecomes"[object Promise]") on its next reinstall, with zero code change on the user's end. Fixed by shipping this as2.0.0instead (npm's^1.5.0on the published CLI does not resolve to2.x) and bumping this repo's ownpackages/clito depend on^2.0.0+ its own version to1.5.1. See both packages'CHANGELOG.mdfor the full breaking-change/migration notes.This is fix 1 and 2 of a 4-part browser-compat plan (root-caused via a real Playwright-driven npm-installed demo app). Fixes 3 and 4 (CORS + CORP, needed for the cross-origin case) are separate PRs — #1950 in this repo and a companion PR in
PageSpace-Deploy— since they touch different packages/repos and need their own deploys (Fly / npm publish are independent release processes).Test plan
packages/sdk:bun run typecheck,bun run build,bun run test— 761/761 passpackages/cli:bun run typecheck,bun run build,bun run test— 999/999 pass (including the loopback-flow PKCE integration test, now awaiting the asyncderiveCodeChallenge)npm publish@pagespace/sdk@2.0.0and@pagespace/cli@1.5.1together, rerun the Playwright-driven browser demo against a freshly-built local SDK to confirm same-origin calls now dispatch real network requests instead of throwing before reaching the network layer🤖 Generated with Claude Code
https://claude.ai/code/session_01UbwJXkonbbrzXmXcKpEsND