Ensure current user is updated #1540
Conversation
|
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. WalkthroughClient loader now validates and persists session state before fetching currentUser and config; shouldRevalidate always invokes session validation and returns session staleness OR the default decision; loader hydration flag is enabled; the HydrateFallback export was removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Router
participant L as clientLoader
participant SV as sessionValid
participant CU as getCurrentUser
participant CF as getConfig
rect rgba(230,245,255,0.6)
note over L: Loader hydration enabled (`hydrate = true`)
end
R->>L: invoke loader
L->>SV: validate & persist session
SV-->>L: session state ensured
L->>CU: fetch currentUser
CU-->>L: user data
L->>CF: fetch config
CF-->>L: config
L-->>R: loader data
sequenceDiagram
autonumber
participant R as Router (nav/update)
participant SR as shouldRevalidate
participant SV as sessionValid
participant GS as getSessionStale
participant DR as defaultShouldRevalidate
R->>SR: check revalidation
SR->>SV: validate session
SV-->>SR: session ensured
SR->>GS: compute sessionIsStale
GS-->>SR: boolean
SR->>DR: compute defaultShouldRevalidate
DR-->>SR: boolean
SR-->>R: return sessionIsStale OR defaultShouldRevalidate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Oksamies
force-pushed
the
09-17-ensure_current_user_is_updated
branch
from
fce63a7 to
b8891d1
Compare
![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
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
183-184: Enabling clientLoader hydration looks good.This ensures currentUser/config are fetched on initial load. No concerns.
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/root.tsx (2)
145-150: Tighten the comment; fix typos and wording.Clarify the rationale and remove typos (“the the”, stray comma). LocalStorage writes are sync; the issue is visibility/ordering relative to clientLoader.
- // We need to run this here too in addition to the, shouldRevalidate function, - // as for some reason the commits to localStorage are not done before the the clientLoader is run + // Also run this here (in addition to shouldRevalidate) because localStorage updates + // done during navigation may not be observed before clientLoader executes.
172-180: Avoid deep imports; reuse one storage instance; guard missing env.
- Don’t pass empty strings into sessionValid; short‑circuit to defaultShouldRevalidate if env is missing.
- Reuse a single storage instance for both calls.
- Prefer package‑level exports over “/src/*” deep imports to prevent coupling to internals and type mismatches.
Apply within-range changes:
- sessionValid( - new StorageManager(SESSION_STORAGE_KEY), - publicEnvVariables.VITE_API_URL || "", - publicEnvVariables.VITE_COOKIE_DOMAIN || "" - ); - const sessionIsStale = getSessionStale( - new NamespacedStorageManager(SESSION_STORAGE_KEY) - ); - return sessionIsStale || defaultShouldRevalidate; + const { VITE_API_URL: apiHost, VITE_COOKIE_DOMAIN: cookieDomain } = + publicEnvVariables; + if (!apiHost || !cookieDomain) { + return defaultShouldRevalidate; + } + const storage = new NamespacedStorageManager(SESSION_STORAGE_KEY); + sessionValid(storage, apiHost, cookieDomain); + const sessionIsStale = getSessionStale(storage); + return sessionIsStale || defaultShouldRevalidate;And update imports (outside this hunk) to avoid deep paths:
- import { - getSessionContext, - getSessionStale, - SESSION_STORAGE_KEY, - sessionValid, - } from "@thunderstore/ts-api-react/src/SessionContext"; - import { StorageManager } from "@thunderstore/ts-api-react/src/storage"; + import { + getSessionContext, + getSessionStale, + SESSION_STORAGE_KEY, + sessionValid, + StorageManager as NamespacedStorageManager, // or keep the existing alias import + } from "@thunderstore/ts-api-react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cyberstorm-remix/app/root.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/root.tsx (3)
packages/ts-api-react/src/SessionContext.tsx (3)
sessionValid(121-158)SESSION_STORAGE_KEY(46-46)getSessionStale(82-84)packages/ts-api-react/src/index.ts (2)
sessionValid(7-7)StorageManager(13-13)packages/ts-api-react/src/storage.ts (1)
StorageManager(1-69)
|
ESLint and CodeQL checks are failing because Graphite created a temporary branch when I was fixing and submitting code in each PR in order. (someone has limited the maximum amount of pushes to 2 in a single command and graphite normally tries to do the whole stack at once to prevent needing to create temporary branches) |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
145-150: Fix typos/wording in the comment.Small cleanup for readability.
- // We need to run this here too in addition to the, shouldRevalidate function, - // as for some reason the commits to localStorage are not done before the the clientLoader is run + // We need to run this here too, in addition to shouldRevalidate, + // because for some reason localStorage writes may not be committed before the clientLoader runs.
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
41-47: Consolidate imports to package entrypointpackages/ts-api-react/src/index.ts re-exports the SessionContext exports and StorageManager (aliased as NamespacedStorageManager). In apps/cyberstorm-remix/app/root.tsx (lines ~41–52) replace the deep import with a single import from the package root:
import {
NamespacedStorageManager,
getSessionContext,
getSessionStale,
SESSION_STORAGE_KEY,
sessionValid,
} from "@thunderstore/ts-api-react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cyberstorm-remix/app/root.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/root.tsx (3)
packages/ts-api-react/src/SessionContext.tsx (3)
sessionValid(121-158)SESSION_STORAGE_KEY(46-46)getSessionStale(82-84)packages/ts-api-react/src/index.ts (2)
sessionValid(7-7)StorageManager(13-13)packages/ts-api-react/src/storage.ts (1)
StorageManager(1-69)
⏰ 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). (6)
- GitHub Check: Test
- GitHub Check: ESLint
- GitHub Check: CodeQL
- GitHub Check: Test
- GitHub Check: Generate visual diffs
- GitHub Check: CodeQL
🔇 Additional comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
183-183: EnablingclientLoader.hydratelooks good.This ensures currentUser is fetched on initial hydration as intended.
Oksamies
force-pushed
the
09-17-ensure_current_user_is_updated
branch
from
bb9c261 to
ec751c6
Compare
For some reason the localStorage set's in shouldRevalidate are not commited before the clientLoader is run. So run the sessionValid function in both, to ensure currentUser is updated. This does lead to some extra processing, but acceptable cost for now.
Remove HydrateFallback from root to prevent the root loaders from holding back page specific skeleton loaders
Oksamies
force-pushed
the
09-17-ensure_current_user_is_updated
branch
from
ec751c6 to
1d4ac35
Compare
2 participants
For some reason the localStorage set's in shouldRevalidate are not
commited before the clientLoader is run. So run the sessionValid
function in both, to ensure currentUser is updated. This does lead to
some extra processing, but acceptable cost for now.
Summary by CodeRabbit
Bug Fixes
Performance
Other