Skip to content

fix(ssr): isolate Redux store per request and clean up SSR utilities#274

Merged
ety001 merged 7 commits into
steemit:nextfrom
blazeapps007:next
May 17, 2026
Merged

fix(ssr): isolate Redux store per request and clean up SSR utilities#274
ety001 merged 7 commits into
steemit:nextfrom
blazeapps007:next

Conversation

@blazeapps007
Copy link
Copy Markdown
Contributor

Redux store SSR isolation — replaced the module-level singleton store with a makeStore factory. Providers now calls useState(() => makeStore()) so each server request renders with its own fresh store, preventing any user's auth state from leaking into another user's server-rendered HTML. On the client a single instance persists as before.
Type exports updated — AppStore, RootState, and AppDispatch are now derived from the factory's return type; all consumer files compile unchanged.
getCurrentTheme reads module store — removed the redundant localStorage.getItem call in favour of returning _theme directly, keeping a single source of truth in the module-level store.
Header comment — clarified the intentional no-op subscribe in useSyncExternalStore so future maintainers understand it is an SSR-safe initializer, not a live subscription.

Replace useSyncExternalStore with useState + useEffect in Header so the
remembered username is always null on the initial render (matching the
server), then updated after hydration. Apply the same deferred pattern
to useTheme, which was reading localStorage inside the useState
initializer — a value the server can never produce, causing React to
render different trees on server vs client.
Cross-tab sync — window.addEventListener('storage', ...) in the module init block. When another tab writes to THEME_KEY, _theme is updated and _notify() is called so all subscribers in this tab re-render.

Duplicated classList ops — both sites kept, with comments explaining why: changeTheme applies synchronously for FOUC prevention, useLayoutEffect is the safety net for mount and cross-tab updates that come through _notify().

Set mutation during iteration — all forEach calls now go through _notify(), which snapshots with Array.from(_listeners) before iterating.

cycleTheme stability — removed the theme dependency by reading _theme directly from the module store. cycleTheme is now stable (only depends on changeTheme, which is itself stable with []), so it won't invalidate memoized children if one is added later.
Replace the single shared store with a makeStore factory and create the store in Providers via a persistent useRef so each server request gets an isolated store (prevents auth state leaking) while the client keeps a single instance. Update store types to AppStore/RootState/AppDispatch. Clarify Header's useSyncExternalStore usage in a comment for SSR-safe avatar initialization. Make getCurrentTheme SSR-safe by returning the internal _theme instead of reading localStorage.
@blazeapps007
Copy link
Copy Markdown
Contributor Author

hi @ety001 when can you Review this one .

Copy link
Copy Markdown
Member

@ety001 ety001 left a comment

Choose a reason for hiding this comment

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

LGTM overall — the direction is correct and matches the official RTK + Next.js App Router pattern. Three non-blocking suggestions:

1. PR description slightly overstates the severity

The description says this prevents "any user's auth state from leaking into another user's server-rendered HTML." In the current codebase, every dispatch site is inside a client event handler (login/logout in use-auth.ts, onClick handlers, etc.) — there is no SSR-time dispatch today, so the module-level singleton was not actively leaking state between requests.

This change is still worth landing because it removes a footgun: the moment anyone adds server-side state hydration (e.g., dispatching setCredentials during render based on a request cookie), the singleton would silently turn into a cross-request leak. I'd suggest softening the wording to something like "future-proofs against per-request state leaking if we ever introduce server-side dispatch" so reviewers don't go hunting for an active bug.

2. Consider useRef instead of useState for the store

The official RTK Next.js App Router example uses:

const storeRef = useRef<AppStore | null>(null);
if (!storeRef.current) storeRef.current = makeStore();

useState(() => makeStore()) is functionally equivalent today, but useRef is the canonical pattern in the RTK docs and avoids any theoretical concern about React re-invoking a lazy initializer in future concurrent-rendering APIs. Not blocking — just a small alignment with the reference implementation.

3. Worth adding a tiny regression test

This PR converts a singleton to a factory, which is exactly the kind of architectural invariant a future contributor could quietly revert. The repo already has vitest.config.ts, so a 3-line test would lock it down:

import { makeStore } from '@/lib/store';

it('makeStore returns an isolated instance per call', () => {
  expect(makeStore()).not.toBe(makeStore());
});

Other observations (informational, no action requested)

  • header.tsx subscribe function() => () => {} allocates a new subscribe function on every render, so useSyncExternalStore re-subscribes each pass. Cleanup is a no-op so there's no functional impact, but hoisting it to a module-level const noopSubscribe = () => () => {}; would be slightly cleaner. This is pre-existing code; this PR only updated the comment.
  • getCurrentTheme() semantic shift — the old implementation read localStorage on every call; the new one returns _theme. Since _theme is initialized from localStorage at module load and kept in sync via the storage event, this is correct for the documented use case. The only behavioral difference would be visible if something in the same tab mutated localStorage[THEME_KEY] outside of changeTheme, but no caller in the repo does that — getCurrentTheme actually has zero call sites today, so this is purely defensive cleanup.
  • Consumer compatibility — confirmed by grepping src/: every consumer imports only RootState / AppDispatch as types (e.g., src/hooks/use-auth.ts, src/components/wallet/*-form.tsx, src/components/auth/login-form.tsx). No file imports the removed store singleton, so the rename is binary-compatible at the type level.

Happy to approve once (1) is addressed in the description; (2) and (3) are optional.

ety001 added 2 commits May 17, 2026 23:43
Lock in the SSR-isolation invariant introduced in this PR so a future
refactor cannot quietly revert makeStore back to a module-level singleton
without failing CI.
The repo's vitest config requires 80% line/statement coverage, but ui.ts
and wallet.ts had no tests, dragging the global stats to 79.54% lines /
77.30% statements and failing CI. Add reducer tests for every action in
both slices.

After this change:
- Lines:      79.54% -> 84.46%
- Statements: 77.30% -> 81.91%
@ety001 ety001 merged commit cda7c4c into steemit:next May 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants