Feat/ Navigation with React Router V6#901
Conversation
|
i cant posibly help for this bug right? |
There was a problem hiding this comment.
PR Review — React Router v6 Migration
Overview
Replaces the custom hash-based router (stremio-router) with React Router v6 (HashRouter). Routes consume params via useParams/useSearchParams instead of receiving urlParams/queryParams as props. Navigation moves from window.location/window.history to useNavigate. Adds useNavigateWithOrigin for origin-tracking on back navigation and replaces the route-stack-based useRouteFocused with a window-focus-based implementation.
+456 / -352 across 52 files. Significant structural change.
Branch is 512 commits behind development
The merge base is very old. Files like LibItem.js, KeyboardShortcuts.js, and others have diverged significantly. This will need a rebase before merging, and several conflicts are likely (e.g. onPlayClick in LibItem was added to development after this branch diverged).
Bugs
1. HorizontalNavBar.js — stale closure in backButtonOnClick
const backButtonOnClick = React.useCallback(() => {
if (originPath) {
navigate(originPath);
} else {
navigate(-1);
}
}, []); // ← empty depsoriginPath and navigate are used inside the callback but missing from the dependency array. The callback captures the initial values and never updates. Should be [originPath, navigate].
2. Routes.tsx — ref .current in useEffect deps
React.useEffect(() => {
// ...
previousAuthRef.current = profile.auth;
}, [location, profile.auth, navigate, previousAuthRef.current]);previousAuthRef.current should not be in the dependency array. Ref mutations don't trigger re-renders, and including .current in deps makes React compare stale snapshots. Remove it — the effect should depend on [location.pathname, profile.auth] only (navigate is stable from React Router).
3. Library.js — typeSelect.selected doesn't exist
if (!library.selected?.type && typeSelect.selected) {
navigate(typeSelect.selected[0].replace('#', ''));
}typeSelect from useSelectableInputs has a value property, not selected. The selected property belongs to sortChips. This condition is always falsy, so the auto-redirect to the first type is broken. Should remain typeSelect.value:
if (!library.selected?.type && typeSelect.value) {
navigate(typeSelect.value.replace('#', ''));
}4. SearchBar.js — invalid navigate options
navigate('/search', { search: '' });The second argument to navigate() is NavigateOptions ({ replace?, state?, preventScrollReset? }). { search: '' } is silently ignored. Use navigate('/search') or setSearchParams({}) to clear the query.
5. Discover/useSelectableInputs.js — catalogSelect.onSelect parameter change
- onSelect: (value) => {
- window.location =value;
+ onSelect: (event) => {
+ navigate(event.value.replace('#', ''));The parameter changed from value to event.value, but all other onSelect callbacks in this file and across Library/Addons use (value) => .... If the caller passes a plain string (like the other selects), event.value would be undefined. Verify the caller or keep it consistent.
Semantic change: useRouteFocused
This is the most impactful change in the PR beyond the routing mechanics.
Old behavior (deleted RouteFocusedContext): Returns whether this route is the topmost/active view in the custom router's view stack. Background routes return false.
New behavior (useRouteFocused.ts): Returns document.hasFocus() — whether the browser window/tab is focused.
These are fundamentally different signals. Key consumers affected:
useModelState.js— subscribes/unsubscribes to core state updates based on this. Old: pauses when route is backgrounded. New: pauses when window loses focus. With React Router only rendering one route, this is probably fine since backgrounded routes are unmounted.Popup.js— closes popups whenrouteFocusedbecomes false. Old: closes on route navigation. New: closes when alt-tabbing away from the browser. This is a UX change — dropdown menus and popups will close when clicking outside the browser window.Slider.js— controlsrequestAnimationFrame. Old: stops animating background routes. New: stops when window unfocused. Fine.
Since React Router unmounts inactive routes, the old "is this route on top" concept is replaced by mount/unmount lifecycle. The window-focus variant is reasonable but the Popup behavior change should be intentional.
useInsertionEffect → useLayoutEffect in useModelState.js
useInsertionEffect runs synchronously before all layout effects and was specifically chosen for ordering guarantees (dispatch/subscribe happens before any layout effect in child components). Changing to useLayoutEffect means these now run in the same phase as other layout effects, so ordering depends on component tree position. If this worked before with useInsertionEffect and there was a reason for it, this change could introduce subtle timing bugs. What motivated this change?
.replace('#', '') is fragile
Used ~20 times throughout the PR:
navigate(deepLinks.player.replace('#', ''));String.replace with a string argument only replaces the first occurrence, so it handles the leading # correctly by coincidence. But it would break if a deepLink doesn't have a leading # but has a # elsewhere (e.g., fragment in a URL). Consider a targeted strip:
const toPath = (link) => link.startsWith('#') ? link.slice(1) : link;The many TODO comments (// TODO: remove # from deeplinks in core) suggest this should eventually be fixed at the source (stremio-core). Until then, a small utility would be cleaner than 20 inline .replace calls.
Incomplete migration
These files still use window.location / window.history and bypass React Router:
| File | Usage |
|---|---|
App.js:104,106 |
window.location.href = '#/addons?...' |
DeepLinkHandler.js:14 |
window.location = deepLinks.metaDetailsVideos |
KeyboardShortcuts.js:54,56 |
window.history.forward() / window.history.back() |
GamepadNavigation |
window.history.back() |
These bypass React Router's state management. Navigation that skips useNavigate won't update router state correctly, which can cause the back button, useLocation, and route transitions to behave unexpectedly.
App.js still imports from stremio-router (const { Router } = require('stremio-router')) — this is fine since it's the local module, but the module name stremio-router is now misleading given it's just a thin wrapper around HashRouter.
useNavigateWithOrigin — sessionStorage coupling
sessionStorage persists across page refreshes within the same tab. If a user navigates to MetaDetails, refreshes the page, then clicks back, they'll be sent to the stored origin from before the refresh rather than getting normal back-button behavior. Consider whether location.state alone (which doesn't survive refresh) is more appropriate, or clearing the stored origin on page load.
Minor
- React Router v6 vs v7: v6 is at end of life (v7 has been stable since Dec 2024). The PR description mentions Node 14 compat — if that's the constraint, worth documenting. Otherwise v7 would give you the upgrade path.
withModel(Library, useLocation)in Library.js — passinguseLocationas a parameter to a HOC is unusual. It's used insidewithModelto calluseLocation()which works because it's called inside a component, but it creates an unnecessary indirection. Could just import and call it directly inside the component.
Summary
The direction is right — moving to React Router is a good call. But the branch needs a rebase (512 commits behind), and there are a few bugs (typeSelect.selected, stale closure, invalid navigate options) plus the useRouteFocused semantic change that should be verified intentional. The incomplete migration and fragile # stripping should also be addressed before merge.
|
@Botsy please resolve conflicts again |
This PR aims to replace the usage of
window.locationandwindow.historyfor navigation and to integrate React Router v6 (version 6 is used or older Node version compatibility >= Node 14)