Skip to content

Ensure current user is updated #1540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ensure current user is updated #1540

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6fd4d2
Ensure current user is updated
Oksamies Sep 17, 2025
ec751c6
Remove HydrateFallback from root
Oksamies Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions apps/cyberstorm-remix/app/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: typos

// as for some reason the commits to localStorage are not done before the the clientLoader is run
sessionTools.sessionValid(
publicEnvVariables.VITE_API_URL,
publicEnvVariables.VITE_COOKIE_DOMAIN
);
const currentUser = await sessionTools.getSessionCurrentUser();
const config = sessionTools.getConfig(publicEnvVariables.VITE_API_URL);
return {
Expand All @@ -158,25 +165,22 @@ export type RootLoadersType = typeof loader | typeof clientLoader;
export function shouldRevalidate({
defaultShouldRevalidate,
}: ShouldRevalidateFunctionArgs) {
if (defaultShouldRevalidate) return true;
const publicEnvVariables = getPublicEnvVariables([
"VITE_API_URL",
"VITE_COOKIE_DOMAIN",
]);
if (
!sessionValid(
new StorageManager(SESSION_STORAGE_KEY),
publicEnvVariables.VITE_API_URL || "",
publicEnvVariables.VITE_COOKIE_DOMAIN || ""
)
)
return true;
return getSessionStale(new NamespacedStorageManager(SESSION_STORAGE_KEY));
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;
Comment on lines +172 to +180
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Suggested change
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

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

}
Comment on lines +172 to 181
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.


export function HydrateFallback() {
return <div style={{ padding: "32px" }}>Loading...</div>;
}
clientLoader.hydrate = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const adContainerIds = ["right-column-1", "right-column-2", "right-column-3"];

Expand Down
Loading