React: Routing and Hooks - #3
rika-milew wants to merge 111 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMigrates to TanStack Router with file-based routes and RouterProvider. Converts class components to hooks. Adds SearchPage, NotFound, DetailView, and new hooks. Renames and hardens API services with timeouts and type guards. Updates tests, styles, and tooling (ESLint/Prettier/Stylelint/Vite/Vitest). ChangesRouter + Hooks Migration
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SearchPage/CardList
participant Router as TanStack Router
participant Hooks as useDataList/useDetailData
participant API as services/api
User->>UI: Enter search / paginate / open detail
UI->>Router: navigate({ search: { page } } or details)
UI->>Hooks: loadData(search, page-1)
Hooks->>API: getItems/getItemFull
API-->>Hooks: Result (success/not-found/error)
Hooks-->>UI: State update
UI-->>User: Renders list/pagination/detail/error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes |
| @@ -0,0 +1,11 @@ | |||
| import { ErrorBoundary } from '@/components/error-boundary/error-boundary'; | |||
|
|
|||
| export function ErrorBoundaryAdapter({ | |||
There was a problem hiding this comment.
Couldn't we write defaultErrorComponent: ErrorBoundary?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
package.json (1)
34-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
@types/p-limit(you’re onp-limit@7which already ships types)Informative React/TS student review: Lines 34 and 46:
p-limitis pinned to^7.3.0while@types/p-limitis installed at^2.1.0;p-limit@7.3.0includes its ownindex.d.ts(types are exported from the package), and@types/p-limit@2.1.0targets the legacyp-limitv2 API (export = pLimit,pLimit(concurrency: number)only), so this is version-mismatched and unnecessary. Action: remove@types/p-limitfromdevDependenciesand rely on the bundled types fromp-limit.src/components/error-state/error-state.tsx (1)
13-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLines 14-15: dynamically-rendered errors aren't announced to screen readers.
This block mounts only after a failed load, so assistive tech users get no notification that an error appeared. There's no semantic element that conveys "live" status, so this is a valid case for an ARIA live region: add
role="alert"to the wrapping<div>(or to the<p>). That makes screen readers announce the message the moment it renders. Keep the rest as-is.src/components/layout/footer/footer.module.css (1)
5-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLines 6, 11, 49, 51, 64, 66, 72, 74: stylelint
declaration-empty-line-beforewill fail CI.The blank lines you've inserted to visually group declarations (box-model / color / etc.) each trip
declaration-empty-line-before. These are reported as errors by Stylelint 17.12.0, so the lint step will break. Remove the empty lines that immediately precede a declaration, e.g.:align-items: center; - width: 100%;If the grouping whitespace is intentional, consider relaxing that rule in
stylelint.config.jsinstead — otherwise the build stays red.src/components/layout/header/header.module.css (1)
5-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLines 6, 10, 15, 17, 25, 27, 73: same stylelint
declaration-empty-line-beforefailures.The blank lines preceding declarations here will be flagged as errors by Stylelint 17.12.0 and break CI. Drop the empty line before each declaration, or relax the rule in
stylelint.config.jsif the grouping whitespace is deliberate. Nice work on the.skip-link/:focuspattern, though — that's a solid keyboard-accessibility addition.src/components/detail-view/detail-view.tsx (1)
21-50: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winLines 21-46: The keydown/click listeners and cleanup are wired correctly, and the
[data-detail]/[data-card]guards prevent self-closing. One thing to be aware of: the effect re-subscribes on every render becausecloseDetailViewhas a new identity each render (seeuse-detail-navigation.ts) — listeners get removed/re-added needlessly. The real fix belongs in the hook (memoize the callbacks), which I've noted there; once that lands, this[closeDetailView]dependency becomes stable. The rest of the rendering logic (Lines 48-95) reads cleanly.src/hooks/use-detail-navigation.ts (1)
9-29: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winLines 13-26:
openDetailViewandcloseDetailVieware recreated on every render, so any consumer that lists them in a dependency array (e.g.DetailView'suseEffectkeyed oncloseDetailView) re-runs each render. Wrap each callback inuseCallbackwith[navigate, search]as dependencies so their identities stay stable across renders. Also notenavigateandsearchfrom TanStack Router are already render-stable inputs, so this gives you a clean, predictable dependency set. TheuseSearch({ from: '/_layout' })adoption looks good — this is exactly the router-aware read suggested earlier.src/services/data-service.ts (1)
1-10: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueLines 1-10: Duplicate imports from
@/constants/constantsYou're importing from the same module twice (lines 2 and 5-10). Consolidating these into a single import statement improves readability and follows common TS conventions.
Combine into one import block from
@/constants/constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 195d0147-bf42-4a98-b1d5-9cde6ac5397b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.prettierrceslint.config.jspackage.jsonsrc/components/button/button.module.csssrc/components/button/button.tsxsrc/components/card-list/card-list.module.csssrc/components/card-list/card-list.test.tsxsrc/components/card-list/card-list.tsxsrc/components/card/card.module.csssrc/components/card/card.test.tsxsrc/components/card/card.tsxsrc/components/detail-view/detail-view.module.csssrc/components/detail-view/detail-view.test.tsxsrc/components/detail-view/detail-view.tsxsrc/components/error-boundary/error-boundary.module.csssrc/components/error-boundary/error-boundary.test.tsxsrc/components/error-button/error-button.test.tsxsrc/components/error-state/error-state.module.csssrc/components/error-state/error-state.test.tsxsrc/components/error-state/error-state.tsxsrc/components/layout/footer/footer.module.csssrc/components/layout/footer/footer.test.tsxsrc/components/layout/footer/footer.tsxsrc/components/layout/header/header.module.csssrc/components/layout/layout.module.csssrc/components/layout/layout.test.tsxsrc/components/layout/layout.tsxsrc/components/loader/loader.module.csssrc/components/loader/loader.tsxsrc/components/pagination/pagination.module.csssrc/components/pagination/pagination.test.tsxsrc/components/pagination/pagination.tsxsrc/components/search-bar/search-bar.module.csssrc/components/search-bar/search-bar.test.tsxsrc/components/search-bar/search-bar.tsxsrc/constants/constants.tssrc/hooks/use-data-list.tssrc/hooks/use-detail-data.tssrc/hooks/use-detail-navigation.tssrc/hooks/use-local-storage.tssrc/main.tsxsrc/pages/about/about-page.module.csssrc/pages/about/about-page.tsxsrc/pages/not-found/not-found.module.csssrc/pages/not-found/not-found.test.tsxsrc/pages/search/search-page.module.csssrc/pages/search/search-page.test.tsxsrc/pages/search/search-page.tsxsrc/routes/-__root-component.tsxsrc/routes/-_layout-component.tsxsrc/routes/-_layout.details.$detailId-component.tsxsrc/routes/__root.tsxsrc/routes/_layout.details.$detailId.tsxsrc/routes/_layout.tsxsrc/services/api.test.tsxsrc/services/api.tssrc/services/data-service.test.tssrc/services/data-service.tssrc/styles/index.csssrc/styles/variables.csssrc/test-utils/api-mock.tssrc/types/api.tssrc/types/type-guards.tsstylelint.config.jstsconfig.app.jsonvite.config.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- vitest.config.ts
| @import url('./variables.css'); | ||
| @import url('./reset.css'); |
There was a problem hiding this comment.
Lines 1-2: Stylelint import-notation violation
Stylelint expects string notation for @import rather than url() syntax:
-@import url('./variables.css');
-@import url('./reset.css');
+@import './variables.css';
+@import './reset.css';📝 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.
| @import url('./variables.css'); | |
| @import url('./reset.css'); | |
| `@import` './variables.css'; | |
| `@import` './reset.css'; |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 1-1: Expected "url('./variables.css')" to be "'./variables.css'" (import-notation)
(import-notation)
[error] 2-2: Expected "url('./reset.css')" to be "'./reset.css'" (import-notation)
(import-notation)
There was a problem hiding this comment.
I suppose we should add the rule 'import-notation': 'string', at the stylelint-config
There was a problem hiding this comment.
@ansivgit, good thinking — but note that stylelint-config-standard (which stylelint.config.js already extends) already enforces import-notation: string. That's precisely why Stylelint flagged Lines 1–2. Adding the rule explicitly would be redundant (though harmless for clarity).
The two actionable paths are:
Option A – Fix the CSS (recommended, aligns with stylelint-config-standard):
-@import url('./variables.css');
-@import url('./reset.css');
+@import './variables.css';
+@import './reset.css';Option B – Override the rule in stylelint.config.js to allow url() notation (if intentional):
rules: {
'import-notation': 'url', // allow url() syntax
// ...other rules
}Option A is the simpler fix and keeps the codebase consistent with the inherited standard. Option B makes sense only if there's a specific reason to prefer url() syntax here. Which would you prefer?
🧠 Learnings used
Learnt from: ansivgit
Repo: rika-milew/rs-react-app PR: 1
File: src/components/card/card.module.css:19-20
Timestamp: 2026-05-11T16:49:03.725Z
Learning: In this codebase, CSS relative color syntax (e.g., `hsl(from var(--color) h s l / alpha)`) is supported because the project targets modern browsers only (Chrome/Edge 131+, Firefox 133+, Safari 18+). When reviewing `.css` changes, do not require legacy fallback colors for relative color syntax unless the stylesheet explicitly opts into broader browser support.
There was a problem hiding this comment.
implemented in the app-state-management branch
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return ( | ||
| <section className={cx('section')}> | ||
| <h2 className={cx('title')}>Results</h2> | ||
| {status === 'loading' && <Loader />} |
There was a problem hiding this comment.
| {status === 'loading' && <Loader />} | |
| {isLoading && <Loader />} |
There was a problem hiding this comment.
implemented in the app-state-management branch
| "typescript-eslint": "^8.58.2", | ||
| "vercel": "^54.0.0", | ||
| "vite": "^8.0.10", | ||
| "vite-tsconfig-paths": "^6.1.1", |
There was a problem hiding this comment.
Yes, of course. Just keep in mind that this solution isn't optimal, so next time (for example, at the final task), it's better to look for a different solution to the issue.
| @import url('./variables.css'); | ||
| @import url('./reset.css'); |
There was a problem hiding this comment.
I suppose we should add the rule 'import-notation': 'string', at the stylelint-config
| globalThis.removeEventListener('keydown', handleKeyDown); | ||
| document.removeEventListener('click', handleOutsideClick); | ||
| }; | ||
| }, [closeDetailView]); |
There was a problem hiding this comment.
I think it's better to wrap this function in a useCallback (in the useDetailNavigation hook). See Rabbit's comment.
There was a problem hiding this comment.
implemented in the app-state-management branch. There I completely removed this hook and used useCallback
2 participants
Task: https://github.com/rolling-scopes-school/tasks/blob/master/react/modules/tasks/functional-routing.md
Screenshot:
Deployment: https://rika-milew-rs-react-app.vercel.app/
Done 18.05.2026 / deadline 19.05.2026
Score: 100 / 100