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, and 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). (6)
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| @@ -141,6 +141,13 @@ export async function clientLoader() { | |||
| publicEnvVariables.VITE_API_URL, | |||
| publicEnvVariables.VITE_COOKIE_DOMAIN | |||
| ); | |||
|
|
|||
| // We need to run this here too in addition to the, shouldRevalidate function, | |||
apps/cyberstorm-remix/app/root.tsx
Outdated
| const sessionIsStale = getSessionStale( | ||
| new NamespacedStorageManager(SESSION_STORAGE_KEY) | ||
| ); | ||
| if (sessionIsStale || defaultShouldRevalidate) return true; |
There was a problem hiding this comment.
Minor: return sessionIsStale || defaultShouldRevalidate; would be better as it's less to read and would change the functions return type from awkward true | undefined to boolean.
| } | ||
|
|
||
| clientLoader.hydrate = true; |
There was a problem hiding this comment.
This seems like an odd place for this code, hidden between two exported functions. Which leads to question: is there a reason to define shouldRevalidate in root.tsx, an already lengthy file, if it's not even used here? This applies to other exported functions in the same file too. Disclaimer: I've no idea if root.tsx is supposed to be some special file with special responsibilities.
There was a problem hiding this comment.
I do not understand the question? shouldRevalidate and clientLoader.hydrate do not affect each other.
https://v2.remix.run/docs/route/client-loader#clientloaderhydrate
https://reactrouter.com/start/data/route-object#shouldrevalidate
Both are needed. shouldRevalidate to prevent unnecessary clientLoader calls and clientLoader.hydrate = true to ensure that the currentUser is fetched and set on the initial page load.
As the address the "lengthyness" of the root.tsx file; the file needs to have certain components/functions and loaders in it, as react-router calls the components from the Route files.
We can of course cleanup and organize the 750~ lines to multiple files, if we want that, but let's make a separate task for that.
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}
| 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; |
There was a problem hiding this comment.
Critical logic bug: The sessionValid() function call is no longer being used to determine revalidation behavior. In the original code, if sessionValid() returned false, the function would return true to trigger revalidation. Now sessionValid() is called but its return value is ignored, and revalidation only depends on sessionIsStale and defaultShouldRevalidate. This means invalid sessions will no longer trigger revalidation, potentially leaving users with stale data when their session becomes invalid. The fix should capture the sessionValid() return value and include it in the revalidation logic: const isSessionInvalid = !sessionValid(...); return sessionIsStale || defaultShouldRevalidate || isSessionInvalid;
| 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 isSessionValid = 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 || !isSessionValid; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
Guard against empty envs to avoid clobbering storage; reuse a single storage instance.
Passing "" into sessionValid can overwrite API_HOST_KEY/COOKIE_DOMAIN_KEY in localStorage if envs are missing momentarily. Also, construct one storage instance and reuse it for both calls.
Apply this diff within this block:
- 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 apiHost = publicEnvVariables.VITE_API_URL;
+ const cookieDomain = publicEnvVariables.VITE_COOKIE_DOMAIN;
+ if (!apiHost || !cookieDomain) {
+ // Don't mutate storage with empty values; fall back to router default.
+ return Boolean(defaultShouldRevalidate);
+ }
+ const storage = new NamespacedStorageManager(SESSION_STORAGE_KEY);
+ sessionValid(storage, apiHost, cookieDomain);
+ const sessionIsStale = getSessionStale(storage);
+ return sessionIsStale || defaultShouldRevalidate;Note: If you apply this, the StorageManager import becomes unused. Remove it (see separate note below).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 apiHost = publicEnvVariables.VITE_API_URL; | |
| const cookieDomain = publicEnvVariables.VITE_COOKIE_DOMAIN; | |
| if (!apiHost || !cookieDomain) { | |
| // Don't mutate storage with empty values; fall back to router default. | |
| return Boolean(defaultShouldRevalidate); | |
| } | |
| const storage = new NamespacedStorageManager(SESSION_STORAGE_KEY); | |
| sessionValid(storage, apiHost, cookieDomain); | |
| const sessionIsStale = getSessionStale(storage); | |
| return sessionIsStale || defaultShouldRevalidate; | |
| } |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/root.tsx around lines 172 to 181, avoid passing
empty strings into sessionValid (which can clobber
API_HOST_KEY/COOKIE_DOMAIN_KEY) and reuse a single storage instance: create one
NamespacedStorageManager (or StorageManager if appropriate) instance and pass
that same instance into both sessionValid and getSessionStale, and only pass the
env vars to sessionValid when they are non-empty (e.g., conditionally pass
undefined or omit them) so you don't overwrite stored values when envs are
missing; after updating usage, remove the now-unused StorageManager import.
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
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.
Summary by CodeRabbit
Bug Fixes
Performance
Other