Repository navigation
Conversation
nicknisi
commented
May 19, 2026
nicknisi
left a comment
nicknisi
left a comment
Member
Author
There was a problem hiding this comment.
Code Review Findings
Warnings
- [Convention]
@types/react-dom@^19conflicts with@types/react@18.3.3in devDependencies — version mismatch may cause type-checking issues in development (package.json:33)
Info
- [Principle 10] Single commit bundles bug fix and test infrastructure setup (vitest, testing-library, jsdom). Ideally separate commits for fix vs test infra, but acceptable given focused scope.
Automated review by case/reviewer agent
 [Low impact] The PR appears safe to merge; no new actionable issues were found. SummaryAdds regression tests for
Both previous findings were addressed by removing the initialization-time token read. Reviews (2) · Last reviewed commit: "ci: run the tests" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Add vitest (jsdom), @testing-library/react, @testing-library/dom and @testing-library/jest-dom, plus react/react-dom 18 and @types/react-dom 18 as devDependencies so they match the pinned @types/react 18.3.3. Replace the placeholder test script with `vitest run`. The config is vitest.config.mts so Vite loads it as ESM in this CommonJS package. src/__tests__ is excluded from `files` so the published package contents stay the same. The lockfile starts from main's and keeps main's registry-agnostic style (no "resolved" fields). It was generated with npm 10 (Node 20, as in CI): npm 10's `npm ci` needs the nested esbuild 0.28.2 entry for vite 8's optional esbuild peer, which npm 11 would leave out.
Cover the init paths main's provider already handles, so they stay fixed: - an onRefresh that fires before createClient resolves keeps its organization (the #87 scenario, fixed on main by spreading prev); - an onRefresh after init still applies; - a token without org_id clears the organization claims; - createClient rejecting logs the error and settles signed out; - under StrictMode double effects only the second init runs; - a superseded init that resolves or rejects late is ignored. Only createClient is mocked; getClaims and LoginRequiredError are the real authkit-js exports.
nicknisi
force-pushed
the
fix/issue-87
branch
from
October 21, 2026 15:00
fc1ac7b to
50261e9
Compare
nicknisi
changed the title
Member
Author
|
@greptileai review |
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
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
After
createClient()resolves, the.then()callback was setting state with onlyisLoading: falseanduser, spreading frominitialStatewhich hasorganizationId: null. This overwrote anyorganizationId(and other auth metadata) that had been correctly set by theonRefresh/handleRefreshcallback moments earlier due to React state batching.The fix reads auth metadata (
organizationId,role,roles,permissions,featureFlags) directly from the access token's JWT claims in the.then()callback, using the existinggetClaims()helper. This ensures state is fully populated regardless of callback ordering. A try/catch handles unauthenticated sessions gracefully.Manual reproduction steps
AuthKitProviderwith anonRefreshcallback that logsresponse.organizationIduseAuth().organizationIdOn
main:organizationIdisnulleven thoughonRefreshreceived the correct value — the.then()callback overwrites itOn this branch:
organizationIdis correctly populated from the access token JWT claimsWhat was tested
Automated
3 vitest unit tests added and passing:
organizationIdis populated from JWT claims aftercreateClientresolvesinitialStatedefaultsonRefresh-setorganizationIdis not overwritten by the.then()callbackManual
npm linkuseAuth()fields are properly populated on the Account pagegetClaims()correctly extractsorg_idfrom JWT, provider source usesgetClaimsin.then()callback, old bug pattern removed, catch block handles unauthenticated sessionsVerification
Before
After
Issue
Closes #87
Follow-ups
@types/react-dom@^19in devDependencies conflicts with@types/react@18.3.3— version mismatch may cause type-checking issues in development (pre-existing, not introduced by this fix)