feat: update store function for web version#59
Conversation
WalkthroughKindeAuthProvider now chooses storage at runtime via a new Changes
Sequence DiagramsequenceDiagram
participant Init as Initialization
participant Platform as Platform Detection
participant Storage as Storage Layer
participant Session as Session Handler
Init->>Platform: Detect platform (web/native)
alt Web
Platform->>Storage: getStorageInstance()
alt localStorage usable
Storage->>Storage: return LocalStorage
else
Storage->>Storage: return MemoryStorage
end
Storage-->>Init: Storage instance
Init->>Session: maybeCompleteAuthSession()
Session-->>Init: Session status
else Native
Platform->>Storage: getStorageInstance()
Storage->>Storage: return ExpoSecureStore
Storage-->>Init: Storage instance
end
Init->>Init: setActiveStorage(instance)
Init-->>Init: Provider Ready
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/KindeAuthProvider.tsx (1)
149-198: Confirm stale closure risks in error handlers and add ref-based solution.The useEffect has an empty dependency array but captures
callbacksandcontextValuein the error handler (line 185), creating stale closure issues. This problem extends beyond the useEffect:
- Line 185 (useEffect error handler): If
callbacksprop changes after mount but before storage initialization fails, the error handler calls the stale callbacks object- Line 267 (setRefreshTimer callback): Same issue—the error handler captures
callbacksandcontextValueat the timeauthenticate()is called; if either changes before the refresh timer triggers and an error occurs, stale references are used- Lines 294, 351, 410: Similar callback captures elsewhere
Recommended fix: Use a ref to track the current
callbacksobject:const callbacksRef = useRef(callbacks); useEffect(() => { callbacksRef.current = callbacks; }, [callbacks]);Then replace
callbacks?.onError?.()calls withcallbacksRef.current?.onError?.(). ForcontextValue, either ensure it's stable or passcontextValuevia ref as well.Alternatively, require that the parent component memoize
callbackswithuseCallbackto maintain referential equality across renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/KindeAuthProvider.tsx(5 hunks)
🔇 Additional comments (3)
lib/KindeAuthProvider.tsx (3)
19-20: LGTM! Imports are appropriate for web compatibility.The new imports (LocalStorage, MemoryStorage, maybeCompleteAuthSession, Platform) are correctly added to support platform-aware storage and web auth flow completion.
Also applies to: 37-41, 61-61
73-75: LGTM! Proper web auth session initialization.Calling
maybeCompleteAuthSessionat module level is the correct pattern for handling OAuth redirects on web platforms. The platform and window checks ensure this only runs in the appropriate environment.
166-171: LGTM! Storage initialization flow is correct.The storage instance is properly initialized, registered globally via
setActiveStorage, and stored in component state. The type safety viaSessionManagerensures all storage implementations are compatible.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/KindeAuthProvider.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-08T06:21:44.638Z
Learnt from: didley
Repo: kinde-oss/expo PR: 5
File: lib/KindeAuthProvider.tsx:38-45
Timestamp: 2024-11-08T06:21:44.638Z
Learning: In `lib/KindeAuthProvider.tsx`, for the `KindeAuthProvider` component, the `config.domain` and `config.clientId` properties should be typed as `string | undefined` because users may pass `process.env` values, which can be `undefined`.
Applied to files:
lib/KindeAuthProvider.tsx
🔇 Additional comments (4)
lib/KindeAuthProvider.tsx (4)
19-20: LGTM!The imports for
LocalStorageandMemoryStorageare correctly added to support the new web storage strategy.
61-61: LGTM!The
Platformimport is correctly added to enable platform-specific storage selection.
177-182: LGTM!The
initializeStoragefunction correctly integrates the newgetStorageInstancehelper to enable platform-aware storage selection while maintaining the existing initialization flow.
73-75: Verification complete—module-level invocation is correct.The pattern of calling
maybeCompleteAuthSession()once at app startup on web is the recommended usage pattern forexpo-web-browser. Your implementation inKindeAuthProvider.tsxaligns with this guidance: the conditional platform check ensures it runs only on web, and the function is a no-op on native (iOS/Android), so there is no platform-specific risk. The code is sound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/KindeAuthProvider.tsx (2)
150-173: Runtime storage selection and web fallback behavior look solid.
getStorageInstancenow:
- Uses
Platform.OS === "web"to branch into a web-specific path.- Probes
window.localStoragein atry/catchto detect private-browsing / restricted-cookie scenarios before instantiatingLocalStorage.- Falls back to
MemoryStoragewith clear warnings when localStorage is unavailable orwindowis missing, avoiding hard failures on web/SSR.- Uses
ExpoSecureStore.default()on native to keep storing secrets in a secure store.Wiring this into
initializeStorageviaconst storageInstance = await getStorageInstance();and then callingsetActiveStorage(storageInstance)andsetStorage(storageInstance)is consistent with theSessionManagerabstraction.If you ever want to unit-test this selection logic in isolation, consider hoisting
getStorageInstanceto a module-level helper and injectingPlatform/windowvia parameters, but that’s purely optional.Also applies to: 177-177
175-207: Callbacks invoked during initialization see the first-render context; consider making that explicit or reworking.Because the storage-initialization
useEffecthas an empty dependency array and is declared beforeuseMemo(contextValue)anduseCallback(onRefresh), the effect runs with the first-render instances ofcontextValueandonRefresh. That means:
callbacks?.onErrorinside the effect, andcallbacks?.onEvent(AuthEvent.tokenRefreshed, data, contextValue)triggered viaonRefreshduring the initialrefreshTokencallreceive a context object whose
storage,isAuthenticated,isLoading, etc. still reflect the pre-initialization render.If consumers of these callbacks expect to call methods on the passed
contextduring bootstrap (e.g.,getAccessTokenright after a successful refresh), they’ll interact with a slightly stale view.Not urgent, but you might consider one of:
- Documenting that the context passed into these initialization-time callbacks may not be fully hydrated yet, or
- Refactoring so the effect depends on a stable
onRefreshthat itself is based on the post-storagecontextValue(for example, by guarding the effect with auseRef“initialized” flag and addingonRefresh/config as dependencies), or- Passing only primitives (
domain,clientId,data) into these callbacks during init, and relying on the normal context for subsequent events.Functionally this is a subtle edge case rather than a blocker, but worth being aware of.
Also applies to: 601-706
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/KindeAuthProvider.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-08T06:21:44.638Z
Learnt from: didley
Repo: kinde-oss/expo PR: 5
File: lib/KindeAuthProvider.tsx:38-45
Timestamp: 2024-11-08T06:21:44.638Z
Learning: In `lib/KindeAuthProvider.tsx`, for the `KindeAuthProvider` component, the `config.domain` and `config.clientId` properties should be typed as `string | undefined` because users may pass `process.env` values, which can be `undefined`.
Applied to files:
lib/KindeAuthProvider.tsx
🔇 Additional comments (2)
lib/KindeAuthProvider.tsx (2)
116-121: Config typing fordomain/clientIdstill correctly allows env-drivenundefined.
KindeAuthConfigkeepsdomainandclientIdasstring | undefinedand then explicitly guards againstundefinedat runtime, which matches the earlier guidance about supportingprocess.envvalues that may be missing. No changes needed here.
Based on learnings
37-41: Web auth-session completion is gated safely for browser-only environments.Importing
openAuthSessionAsync,openBrowserAsync, andmaybeCompleteAuthSessionfromexpo-web-browser, and then callingmaybeCompleteAuthSession()only whenPlatform.OS === "web"andtypeof window !== "undefined", keeps SSR/tests safe while enabling proper completion of web auth flows. This looks correct and aligns with Expo’s expectations.Also applies to: 61-61, 73-75
|
I'll look into this. |
2 participants
Explain your changes
Fix 51
#51
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.