Add SSR modules#75
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughAdds a new ChangesSSR Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
src/lib/__tests__/jwt.test.ts (1)
12-25: ⚡ Quick winAdd malformed-token coverage for expiry decisions.
Current tests only cover a valid payload path. Please add a case for malformed token input to lock expected refresh behavior and prevent regressions.
🤖 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/lib/__tests__/jwt.test.ts` around lines 12 - 25, Add a unit test in the 'jwt helpers' suite that passes a clearly malformed JWT string (e.g., 'malformed.token') to getJwtExpiration and asserts the expected safe result (use null to lock the refresh decision) so malformed tokens don't throw or produce a Date; reference getJwtExpiration and add the test alongside the existing jwtWithPayload-based case to prevent regressions.
🤖 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 `@package.json`:
- Line 3: Update package.json so prerelease versions follow the recommended
-dev.X format and publishing scripts use the correct dist-tags: change the
"version" value from "1.3.0-ssr.3" to a dev prerelease like "1.3.0-dev.3" (or
add a short comment in package.json/README explaining why -ssr.3 is
intentionally dev if you must keep it), and ensure the npm scripts "publish:dev"
runs npm publish --tag dev while "publish:stable" runs npm publish (defaulting
to latest) so that dev prereleases are published with the dev dist-tag and
stable releases remain on latest.
In `@src/index.ts`:
- Around line 82-90: The current check only rejects missing apiKey but allows
whitespace-only values; update the validation around config.apiKey (used before
destructuring into { apiKey, ...clientConfig } and when instantiating
InsForgeClient) to reject blank/whitespace-only strings by using something like
String(config.apiKey).trim() === '' (or trim check) and throw the same
Error('Missing apiKey. Pass apiKey to createAdminClient().') when blank so
edgeFunctionToken never receives an empty token.
In `@src/lib/jwt.ts`:
- Around line 30-37: The current isJwtExpiredOrExpiring function treats a
malformed or unparsable JWT as valid because it returns false when
getJwtExpiration(token) yields null; change this behavior so that when
getJwtExpiration(token) returns null you treat the token as expired (return
true) so refresh paths won’t be skipped. Locate isJwtExpiredOrExpiring and
adjust the early-return logic (and any callers relying on it) so malformed
tokens trigger a true result while preserving the existing leewaySeconds
expiration check for valid expiration dates.
In `@src/ssr/browser-client.ts`:
- Around line 171-204: The ssrFetch wrapper must honor the skipAuthRefresh
opt-out for both the pre-request refresh and the post-401 retry: compute a local
flag (e.g., const skip = shouldSkipRefresh(input)) at the top of ssrFetch and
use it to short-circuit any refresh logic — do not call refreshFromRoute() or
attempt to attach refreshed tokens if skip is true; likewise, after reading
errorCode, if skip is true, return the original response even if
isRefreshableErrorCode(errorCode) is true (i.e., don't perform the retry/refresh
flow). Update logic around ssrFetch, shouldSkipRefresh, refreshFromRoute,
withAuthHeader, readErrorCode, isRefreshableErrorCode and client.setAccessToken
to respect that skip flag in both the pre-request and post-401 paths.
---
Nitpick comments:
In `@src/lib/__tests__/jwt.test.ts`:
- Around line 12-25: Add a unit test in the 'jwt helpers' suite that passes a
clearly malformed JWT string (e.g., 'malformed.token') to getJwtExpiration and
asserts the expected safe result (use null to lock the refresh decision) so
malformed tokens don't throw or produce a Date; reference getJwtExpiration and
add the test alongside the existing jwtWithPayload-based case to prevent
regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 418794ab-2543-4369-b3da-104db4a89d38
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdSDK-REFERENCE.mdpackage.jsonsrc/client.tssrc/index.tssrc/lib/__tests__/client.test.tssrc/lib/__tests__/jwt.test.tssrc/lib/jwt.tssrc/modules/auth/auth.tssrc/modules/realtime.tssrc/ssr.tssrc/ssr/__tests__/ssr.test.tssrc/ssr/browser-client.tssrc/ssr/cookies.tssrc/ssr/refresh.tssrc/ssr/server-client.tssrc/ssr/update-session.tssrc/types.tstsup.config.ts
Comment on lines
+171
to
+204
| const ssrFetch: typeof fetch = async (input, init) => { | ||
| if (!fetchImpl) { | ||
| throw new Error( | ||
| 'Fetch is not available. Please provide a fetch implementation.', | ||
| ); | ||
| } | ||
| if (shouldSkipRefresh(input)) { | ||
| return fetchImpl(input, init); | ||
| } | ||
|
|
||
| let requestInit = init; | ||
| if ( | ||
| (!accessToken && !sessionChecked) || | ||
| isJwtExpiredOrExpiring(accessToken, options.refreshLeewaySeconds) | ||
| ) { | ||
| const refreshed = await refreshFromRoute().catch(() => null); | ||
| if (refreshed?.accessToken) { | ||
| requestInit = withAuthHeader(init, refreshed.accessToken); | ||
| } | ||
| } | ||
|
|
||
| const response = await fetchImpl(input, requestInit); | ||
| const errorCode = await readErrorCode(response); | ||
| if (!isRefreshableErrorCode(errorCode)) { | ||
| return response; | ||
| } | ||
|
|
||
| const refreshed = await refreshFromRoute(); | ||
| if (!refreshed?.accessToken) { | ||
| client.setAccessToken(null); | ||
| return response; | ||
| } | ||
|
|
||
| return fetchImpl(input, withAuthHeader(init, refreshed.accessToken)); |
There was a problem hiding this comment.
Honor skipAuthRefresh in the SSR fetch wrapper.
ssrFetch currently refreshes/retries every refreshable 401, so auth calls that intentionally return 401s can still hit /api/auth/refresh even when the SDK request layer marked them to skip refresh. Please thread the same opt-out through both the pre-request refresh path and the post-401 retry path here.
Based on learnings: skipAuthRefresh: true must prevent legitimate auth 401s from triggering a refresh cycle.
🤖 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/ssr/browser-client.ts` around lines 171 - 204, The ssrFetch wrapper must
honor the skipAuthRefresh opt-out for both the pre-request refresh and the
post-401 retry: compute a local flag (e.g., const skip =
shouldSkipRefresh(input)) at the top of ssrFetch and use it to short-circuit any
refresh logic — do not call refreshFromRoute() or attempt to attach refreshed
tokens if skip is true; likewise, after reading errorCode, if skip is true,
return the original response even if isRefreshableErrorCode(errorCode) is true
(i.e., don't perform the retry/refresh flow). Update logic around ssrFetch,
shouldSkipRefresh, refreshFromRoute, withAuthHeader, readErrorCode,
isRefreshableErrorCode and client.setAccessToken to respect that skip flag in
both the pre-request and post-401 paths.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 20 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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.
Summary by cubic
Adds first-class SSR auth for Next.js via
@insforge/sdk/ssr, with server-owned refresh cookies and browser-readable access tokens. Also adds a server-only admin client and makes Realtime SSR-safe by dynamically importingsocket.io-client.New Features
@insforge/sdk/ssrhelpers:createBrowserClient(),createServerClient(),createRefreshAuthRouter(),refreshAuth(),updateSession(), and cookie utils (setAuthCookies(),clearAuthCookies(), options/getters).insforge_access_token(readable) andinsforge_refresh_token(httpOnly), both expiring at JWTexp.NEXT_PUBLIC_INSFORGE_URL,NEXT_PUBLIC_INSFORGE_ANON_KEY); explicit config wins.AUTH_TOKEN_EXPIRED; server client reads the access-token cookie per request.createAdminClient({ apiKey })for server-only admin access (bearer token; enables server mode).socket.io-clientto avoid SSR bundles; re-subscribes on reconnect.getJwtExpiration(),isJwtExpiredOrExpiring().InsForgeAdminConfig,InsForgeErrorCode; legacy server-mode refresh error standardized toAUTH_UNAUTHORIZED.@insforge/sdk/ssrexport andtsupentry; externalizesocket.io-client; bump to1.3.0; update@insforge/shared-schemasto^1.1.53.Migration
/api/auth/refreshviacreateRefreshAuthRouter(), usecreateBrowserClient()andcreateServerClient(), runupdateSession()in middleware/proxy, and callsetAuthCookies()after sign-in.isServerModeand manualrefreshSession({ refreshToken })with the new SSR helpers.NEXT_PUBLIC_INSFORGE_URLandNEXT_PUBLIC_INSFORGE_ANON_KEY, or passbaseUrl/anonKeyexplicitly.Written for commit e97eb07. Summary will update on new commits.
Review in cubic
Note
Add
@insforge/sdk/ssrsubpath with browser/server client factories and session refresh helpers./ssrexport subpath in src/ssr.ts exposingcreateBrowserClient,createServerClient,createRefreshAuthRouter,refreshAuth,updateSession, and cookie utilities for Next.js SSR integration.createBrowserClientreads access tokens from cookies, pre-refreshes expiring tokens via a configurable app route, and retries once on 401 with auth error codes.createServerClientbuilds a server-modeInsForgeClientusing per-request access tokens sourced from cookies.refreshAuthandcreateRefreshAuthRouterhandle server-side token refresh via httpOnly cookies, settingSet-Cookieheaders on the response and omittingrefreshTokenfrom the response body.updateSessionacts as middleware to proactively refresh sessions and sync cookies before a request is handled.createAdminClientin src/index.ts for server-only admin access using anapiKey.getJwtExpirationandisJwtExpiredOrExpiringin src/lib/jwt.ts usingatob/TextDecoderto avoid a NodeBufferdependency.socket.io-clientis now loaded lazily via dynamic import in src/modules/realtime.ts and is marked external in the build.Macroscope summarized e97eb07.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores