Repository navigation
8 React & Next.js. Server Side Rendering - #8
wingedseraph wants to merge 59 commits into
Conversation
✅ Deploy Preview for wingi-rs26 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
wingedseraph
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe project is migrated from Vite and React Router to Next.js with next-intl for internationalization. Build scripts, TypeScript configuration, PostCSS, ESLint, and Netlify configuration are updated accordingly. A new i18n layer is introduced covering locale routing, per-request message loading for English and Russian, and locale-aware navigation helpers. The Next.js app directory is structured with a root layout, locale-specific layouts, and parallel route slots for the search list and card details. RTK Query is replaced with plain 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)src/app/globals.cssFile contains syntax errors that prevent linting: Line 4: Tailwind-specific syntax is disabled.; Line 8: Tailwind-specific syntax is disabled.; Line 12: Tailwind-specific syntax is disabled.; Line 16: Tailwind-specific syntax is disabled.; Line 21: Tailwind-specific syntax is disabled.; Line 33: Tailwind-specific syntax is disabled.; Line 39: Tailwind-specific syntax is disabled.; Line 40: Tailwind-specific syntax is disabled.; Line 41: Tailwind-specific syntax is disabled.; Line 190: Tailwind-specific syntax is disabled.; Line 211: Tailwind-specific syntax is disabled. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/back-link.tsx (1)
8-18: 🎯 Functional Correctness | 🟠 MajorBug
The icon-only back link has no accessible name, so assistive technology users cannot reliably understand its purpose.
This violates WCAG 4.1.2 (Name, Role, Value), which requires that all interactive controls have a programmatically determinable accessible name.Proposed fix
<Link href={PATH.index} + aria-label='Back to home' className=' inline-block w-fit shrink-0 rounded-full bg-white p-1 text-stone-4 shadow-cloud outline-hidden transition-colors hover:text-black-tisa focus-visible:ring-1 focus-visible:ring-black ' > - <IconArrowBold /> + <IconArrowBold aria-hidden='true' /> </Link>For icon-only interactive controls with no visible text,
aria-labeldirectly on the element is the appropriate method. Addingaria-hidden='true'to the icon prevents screen readers from announcing irrelevant SVG or icon font content.Source: W3C: Names and Descriptions, W3C: Understanding WCAG 4.1.2
vitest.config.ts (1)
1-29: 📐 Maintainability & Code Quality | 🔴 CriticalBug: Test files using
@/path imports will fail at runtime because Vitest is not configured to resolve these aliases. TypeScript'stsconfig.jsonpaths are not automatically available to Vitest's module resolution.Vitest requires explicit alias configuration to map import paths at test runtime. Without it, imports like
import { Header } from '@/widgets/header/Header'in test files (e.g.,Header.test.tsx,Flyout.test.tsx) will break.Proposed fix
+import { resolve } from 'node:path' import { defineConfig } from 'vitest/config' export default defineConfig({ + resolve: { + alias: { + '@': resolve(__dirname, './src'), + }, + }, test: { root: __dirname, setupFiles: ['./vitest.setup.ts'],Source: alias | Config | Vitest
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17e68271-e03d-4142-ba53-8f14f14b7c26
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksrc/app/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (102)
.env.example.gitignore.husky/pre-push.oxlintrc.jsoneslint.config.jsindex.htmlmessages/en.jsonmessages/ru.jsonnetlify.tomlnext.config.tspackage.jsonpostcss.config.mjspublic/_redirectssrc/api/artwork.tssrc/api/typeguard.tssrc/app/[locale]/(search)/@details/card/[id]/page.tsxsrc/app/[locale]/(search)/@details/default.tsxsrc/app/[locale]/(search)/@details/loading.tsxsrc/app/[locale]/(search)/@details/page.tsxsrc/app/[locale]/(search)/@list/card/[id]/page.tsxsrc/app/[locale]/(search)/@list/default.tsxsrc/app/[locale]/(search)/@list/loading.tsxsrc/app/[locale]/(search)/@list/page.tsxsrc/app/[locale]/(search)/layout.tsxsrc/app/[locale]/(search)/page.tsxsrc/app/[locale]/[...rest]/page.tsxsrc/app/[locale]/about/page.tsxsrc/app/[locale]/error.tsxsrc/app/[locale]/layout.tsxsrc/app/[locale]/not-found.tsxsrc/app/actions.tssrc/app/api/csv/route.tssrc/app/globals.csssrc/app/layout.tsxsrc/app/not-found.tsxsrc/components/error-boundary/ErrorBoundary.test.tsxsrc/components/error-boundary/ErrorBoundary.tsxsrc/components/pagination/Pagination.tsxsrc/components/ui/back-link.tsxsrc/components/ui/spinner.tsxsrc/context/ThemeContext.tsxsrc/global.tssrc/hooks/useLocalStorage.tssrc/hooks/usePage.tssrc/i18n/navigation.tssrc/i18n/request.tssrc/i18n/routing.tssrc/lib/cardToCsv.tssrc/lib/const/router.tssrc/lib/tryCatch.tssrc/main.tsxsrc/pages/about-page/AboutPage.test.tsxsrc/pages/about-page/AboutPage.tsxsrc/pages/card-detailed-page/CardDetailedPage.test.tsxsrc/pages/card-detailed-page/CardDetailedPage.tsxsrc/pages/error-page/ErrorPage.test.tsxsrc/pages/error-page/ErrorPage.tsxsrc/pages/forms-page/FormsPage.tsxsrc/pages/forms-page/components/AgeField.tsxsrc/pages/forms-page/components/CountryField.tsxsrc/pages/forms-page/components/EmailField.tsxsrc/pages/forms-page/components/FileField.tsxsrc/pages/forms-page/components/GenderField.tsxsrc/pages/forms-page/components/Modal.test.tsxsrc/pages/forms-page/components/Modal.tsxsrc/pages/forms-page/components/NameField.tsxsrc/pages/forms-page/components/PasswordField.test.tsxsrc/pages/forms-page/components/PasswordField.tsxsrc/pages/forms-page/components/SubmissionCard.tsxsrc/pages/forms-page/components/TermsField.tsxsrc/pages/forms-page/forms/RHFForm.test.tsxsrc/pages/forms-page/forms/RHFForm.tsxsrc/pages/forms-page/forms/UncontrolledForm.test.tsxsrc/pages/forms-page/forms/UncontrolledForm.tsxsrc/pages/forms-page/schema/schema.test.tssrc/pages/forms-page/schema/schema.tssrc/pages/landing-page/LandingPage.test.tsxsrc/pages/landing-page/LandingPage.tsxsrc/proxy.tssrc/router.tsxsrc/store/StoreProvider.tsxsrc/store/index.tssrc/store/slices/submissionsSlice.test.tssrc/styles/styles.tssrc/tests/mocks/handlers.tssrc/tests/mocks/mocks.tssrc/tests/mocks/node.tssrc/tests/mocks/setupStore.tsxsrc/vite-env.d.tssrc/widgets/card-list/CardItem.tsxsrc/widgets/card-list/CardList.tsxsrc/widgets/card-list/CardListFooter.tsxsrc/widgets/combined-input/CombinedInput.tsxsrc/widgets/flyout/Flyout.tsxsrc/widgets/header/Header.tsxsrc/widgets/header/ThemeToggle.tsxsrc/widgets/layout/Layout.tsxtsconfig.app.jsontsconfig.jsontsconfig.node.jsonvitest.config.tsvitest.setup.ts
💤 Files with no reviewable changes (42)
- index.html
- tsconfig.app.json
- src/pages/landing-page/LandingPage.test.tsx
- src/pages/forms-page/components/GenderField.tsx
- src/pages/forms-page/components/PasswordField.tsx
- src/pages/forms-page/components/EmailField.tsx
- src/components/error-boundary/ErrorBoundary.tsx
- src/pages/forms-page/components/SubmissionCard.tsx
- .husky/pre-push
- src/pages/error-page/ErrorPage.test.tsx
- src/main.tsx
- src/pages/forms-page/components/NameField.tsx
- src/pages/about-page/AboutPage.test.tsx
- src/widgets/layout/Layout.tsx
- src/pages/forms-page/components/FileField.tsx
- src/pages/card-detailed-page/CardDetailedPage.tsx
- src/pages/forms-page/forms/UncontrolledForm.tsx
- public/_redirects
- src/hooks/usePage.ts
- src/pages/forms-page/forms/UncontrolledForm.test.tsx
- tsconfig.node.json
- src/pages/error-page/ErrorPage.tsx
- src/pages/forms-page/schema/schema.test.ts
- src/tests/mocks/mocks.ts
- src/pages/card-detailed-page/CardDetailedPage.test.tsx
- src/pages/about-page/AboutPage.tsx
- src/pages/forms-page/components/CountryField.tsx
- src/pages/forms-page/schema/schema.ts
- src/store/index.ts
- src/components/error-boundary/ErrorBoundary.test.tsx
- src/pages/forms-page/forms/RHFForm.tsx
- src/pages/forms-page/FormsPage.tsx
- src/pages/forms-page/forms/RHFForm.test.tsx
- src/pages/forms-page/components/PasswordField.test.tsx
- src/router.tsx
- src/pages/landing-page/LandingPage.tsx
- src/pages/forms-page/components/Modal.test.tsx
- src/vite-env.d.ts
- src/pages/forms-page/components/TermsField.tsx
- src/pages/forms-page/components/AgeField.tsx
- src/pages/forms-page/components/Modal.tsx
- src/store/slices/submissionsSlice.test.ts
| export async function getByQueryArtwork(query: string, page?: string) { | ||
| const response = await fetch(`${BASE}/objects/search?q=${query}&images_exist=${IMAGES_EXIST}&page_size=${PAGE_SIZE}&page=${page ?? 0}`) | ||
|
|
||
| export const artworkApi = createApi({ | ||
| keepUnusedDataFor: Number.isFinite(Number(import.meta.env.VITE_TTL)) ? Number(import.meta.env.VITE_TTL) : 20, | ||
| tagTypes: ['ArtworkByQuery', 'ArtworkById'], | ||
| reducerPath: 'artworkApi', | ||
| baseQuery: fetchBaseQuery({ baseUrl: BASE }), | ||
| endpoints: builder => ({ | ||
| getArtworkByName: builder.query<ValidResponse, Arguments>({ | ||
| query: ({ query, page }) => { | ||
| const parameters = new URLSearchParams({ | ||
| q: query, | ||
| images_exist: IMAGES_EXIST, | ||
| page_size: PAGE_SIZE, | ||
| page: page ?? '1', | ||
| }) | ||
| if (!response.ok) { | ||
| throw new Error('Issue with API') | ||
| } | ||
| const data: unknown = await response.json() | ||
|
|
||
| return `/objects/search?${parameters.toString()}` | ||
| }, | ||
| providesTags: [byQueryTag], | ||
| transformResponse: async (response) => { | ||
| if (!isValidResponse(response)) { | ||
| throw new Error('error in typeguard') | ||
| } | ||
| return response | ||
| }, | ||
| }), | ||
| getArtworkById: builder.query<ValidResponseSingleCard, string>({ | ||
| query: (id) => { | ||
| return `/museumobject/${id}` | ||
| }, | ||
| providesTags: [byIdTag], | ||
| transformResponse: async (response) => { | ||
| if (!isValidResponseSingleItem(response)) { | ||
| throw new Error('error in typeguard') | ||
| } | ||
| return response | ||
| }, | ||
| }), | ||
| }), | ||
| }) | ||
| if (!isValidResponse(data)) { | ||
| throw new Error('Issue with API Response') | ||
| } | ||
| const records = data.records | ||
| const recordsCount = data.info.record_count | ||
|
|
||
| export const { useGetArtworkByNameQuery, useGetArtworkByIdQuery } = artworkApi | ||
| return { records, recordsCount } | ||
| } | ||
| export async function getByIdArtwork(id: string) { | ||
| const response = await fetch(`${BASE}/museumobject/${id}`) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, find and read the src/api/artwork.ts file to verify the fetch calls
cat -n src/api/artwork.tsRepository: wingedseraph/rs26
Length of output: 1425
🌐 Web query:
AbortSignal.timeout() Node.js support version requirements
💡 Result:
The AbortSignal.timeout static method is supported in Node.js starting from versions 17.3.0 and 16.14.0 [1][2]. It was added to provide a convenient way to create an AbortSignal that automatically aborts after a specified number of milliseconds [1][3].
Citations:
- 1: https://r2.nodejs.org/docs/v20.6.0/api/globals.html
- 2: https://nodejs.org/api/globals.html
- 3: lib: add AbortSignal.timeout nodejs/node#40899
🏁 Script executed:
# Check for Node.js version requirements and runtime context
ls -la | head -20
cat package.json | grep -A 5 '"engines"' || echo "No engines specified"Repository: wingedseraph/rs26
Length of output: 1469
🔍 Additional research query:
Query Node.js AbortSignal.timeout polyfill compatibility fetch API timeout best practices
💡 Result:
Excellent! I have gathered comprehensive information. Let me provide the final answer based on the documentation.
Node.js AbortSignal.timeout with Fetch API Timeout Best Practices
What is AbortSignal.timeout?
AbortSignal.timeout(delay) is a static method that returns a new AbortSignal which will automatically abort after a specified delay in milliseconds.
Method Signature:
AbortSignal.timeout(delay)
// delay: number - The number of milliseconds to wait before triggering the AbortSignal
// Returns: AbortSignalUsing AbortSignal.timeout with Fetch API
The most straightforward way to implement timeouts for fetch requests is to pass AbortSignal.timeout() directly to the fetch signal option:
try {
const response = await fetch('https://example.com/api', {
signal: AbortSignal.timeout(5000) // 5 second timeout
});
const data = await response.json();
} catch (err) {
if (err.name === 'AbortError') {
console.error('Request timeout');
} else {
console.error('Request failed:', err);
}
}Best Practices for Timeout Implementation
-
Always handle AbortError: When a signal is aborted (including timeout), the fetch promise rejects with an
AbortError. Always checkerr.name === 'AbortError'to distinguish timeout errors from other failures. -
Use AbortSignal with Client Disconnects: In HTTP servers, use
req.signalto abort downstream fetch calls when clients disconnect mid-request, preventing resource waste:
import http from 'node:http';
http.createServer(async (req, res) => {
try {
const data = await fetch('https://example.com/api', { signal: req.signal });
res.end(JSON.stringify(await data.json()));
} catch (err) {
if (err.name === 'AbortError') return;
res.statusCode = 500;
res.end('Internal Server Error');
}
}).listen(3000);- Combine Multiple Signals with AbortSignal.any(): To handle both timeout and manual cancellation, use
AbortSignal.any():
const controller = new AbortController();
const timeoutSignal = AbortSignal.timeout(5000);
const combinedSignal = AbortSignal.any([controller.signal, timeoutSignal]);
const response = await fetch(url, { signal: combinedSignal });Polyfill Compatibility
AbortSignal.timeout() is a native Node.js API available in modern versions. No polyfill is required for Node.js environments. The API is also part of the standard AbortSignal specification, so it works consistently with the Fetch API in both Node.js and modern browsers.
Key Properties
- abortSignal.aborted: Boolean indicating if the signal has been aborted
- abortSignal.reason: The reason the signal was aborted (a
TimeoutErrorfor timeout-initiated aborts) - abortSignal.throwIfAborted(): Throws the abort reason if the signal is aborted
Add fetch timeout protection to prevent request processing stalls.
Line 8 and line 24 call external APIs without timeout, allowing upstream service latency to block SSR request processing until platform timeout occurs. This degrades availability by increasing tail latency and holding server resources.
Use AbortSignal.timeout() to enforce a fixed timeout on fetch requests:
Proposed fix
export const PAGE_SIZE = '6'
export const BASE = 'https://api.vam.ac.uk/v2'
const IMAGES_EXIST = '1'
+const API_TIMEOUT_MS = 8000
+
+function fetchWithTimeout(url: string) {
+ return fetch(url, { signal: AbortSignal.timeout(API_TIMEOUT_MS) })
+}
export async function getByQueryArtwork(query: string, page?: string) {
- const response = await fetch(`${BASE}/objects/search?q=${query}&images_exist=${IMAGES_EXIST}&page_size=${PAGE_SIZE}&page=${page ?? 0}`)
+ const response = await fetchWithTimeout(`${BASE}/objects/search?q=${query}&images_exist=${IMAGES_EXIST}&page_size=${PAGE_SIZE}&page=${page ?? 0}`)
@@
export async function getByIdArtwork(id: string) {
- const response = await fetch(`${BASE}/museumobject/${id}`)
+ const response = await fetchWithTimeout(`${BASE}/museumobject/${id}`)Source: MDN Web Docs — AbortSignal.timeout()
Source: Path instructions
| export function isValidCard(card: unknown): card is Card { | ||
| return ( | ||
| isObject(card) && 'systemNumber' in card && '_primaryTitle' in card && '_images' in card && 'objectType' in card | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -type f -name "typeguard.ts" | head -5Repository: wingedseraph/rs26
Length of output: 178
🏁 Script executed:
# Look at the actual implementation
cat -n src/api/typeguard.tsRepository: wingedseraph/rs26
Length of output: 1709
🏁 Script executed:
# Find the Card type definition
rg "type Card|interface Card" --type ts --type tsx -A 10Repository: wingedseraph/rs26
Length of output: 183
🏁 Script executed:
# Check how isValidCard is used in the CSV route
cat -n src/app/api/csv/route.ts 2>/dev/null || echo "File not found, searching for csv routes..."
rg "isValidCard" --type ts --type tsx -B 2 -A 2Repository: wingedseraph/rs26
Length of output: 1490
🏁 Script executed:
# Check the isObject and isArray helper functions
rg "export.*isObject|export.*isArray" --type ts --type tsx -A 5Repository: wingedseraph/rs26
Length of output: 183
🏁 Script executed:
# Find the Card type definition
rg "type Card|interface Card" -A 15Repository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Check what properties _images should have
rg "_images|_primaryTitle|systemNumber|objectType" --type ts -B 2 -A 2 | head -60Repository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Look at the complete types file
cat -n src/api/types.ts 2>/dev/null || find . -name "types.ts" -type f | head -3Repository: wingedseraph/rs26
Length of output: 712
🏁 Script executed:
# Verify the usage pattern - how is isValidCard called?
cat -n src/app/api/csv/route.ts | grep -A 5 "isArray(parsed.data, isValidCard)"Repository: wingedseraph/rs26
Length of output: 411
🌐 Web query:
TypeScript type predicates type narrowing soundness
💡 Result:
TypeScript type predicates are inherently unsound because the compiler does not verify that the implementation of a user-defined type guard matches its declared return type [1][2][3]. When you declare a function returning parameterName is Type, you are essentially providing an explicit instruction to the type system to perform a type assertion [4][2]. Key points regarding the soundness and risks of type predicates include: 1. Absence of Implementation Verification: The compiler trusts the type predicate signature completely [2][3]. A function could unconditionally return true, or implement logic that does not actually satisfy the promised Type, and the compiler will not issue an error [5][3]. This is similar to using a type assertion (as Type) [4][2]. 2. If-and-only-if Semantics: Type predicates are expected to behave with "if and only if" logic: if the function returns true, the parameter must be of the declared type, and if it returns false, it must not be [5][2]. Implementing a predicate that fails this—such as returning true for a subset of the type but also returning false when the type is present—leads to unsound narrowing in the calling code, which can result in runtime errors [5]. 3. Hidden Unsoundness in False Branches: A common pitfall is focusing only on the true branch while ignoring what happens when the predicate returns false [5]. If the false branch does not correctly narrow the type (or narrows it incorrectly), downstream code may operate on values that do not match the expected type, leading to silent failures [5]. 4. Preferred Alternatives: Because of these risks, TypeScript prioritizes native narrowing where possible [3]. As of TypeScript 5.5, the compiler can automatically infer type predicates for many simple functions (e.g., used in .filter()), which eliminates the need for manual, unsafe predicates [2]. These inferred predicates are checked by the compiler to ensure they match the actual logic, providing a sound alternative to manual is annotations [2]. For complex cases where manual predicates are required, best practice is to treat them with the same caution as type assertions [4]. Always ensure the internal logic rigorously validates the shape of the data against the promised type and consider inline narrowing if a standalone function is not strictly necessary [5][3].
Citations:
- 1: Type assertions & type predicates can have incorrect narrowing logic microsoft/TypeScript#57436
- 2: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-5.html
- 3: https://joshhunt.dev/posts/typescripts-user-defined-type-guards-are-not-safe
- 4: "Using type predicates" documentation promotes lying to the type system microsoft/TypeScript#63421
- 5: https://effectivetypescript.com/2024/02/27/type-guards/
Type predicate must validate value types, not just property presence.
Line 12 narrows card to Card by checking only key existence with the in operator, so objects with wrong property types pass validation and are treated as trusted typed data. Type predicates must be sound: the compiler trusts the implementation without verification, so any mismatch between the predicate logic and the declared type leaks unsafeThis is a critical validation boundary for untrusted CSV payloads.
Proposed fix
export function isValidCard(card: unknown): card is Card {
- return (
- isObject(card) && 'systemNumber' in card && '_primaryTitle' in card && '_images' in card && 'objectType' in card
- )
+ if (!isObject(card)) return false
+ const candidate = card as Record<string, unknown>
+ if (!isObject(candidate._images)) return false
+ const images = candidate._images as Record<string, unknown>
+
+ return (
+ typeof candidate.systemNumber === 'number'
+ && typeof candidate.objectType === 'string'
+ && typeof candidate._primaryTitle === 'string'
+ && typeof images._iiif_image_base_url === 'string'
+ )
}Source: TypeScript Handbook — Using type predicates
Source: Path instructions
| export type SearchParams = { | ||
| query: string | ||
| page: string | ||
| } | ||
|
|
||
| export default async function SearchPage({ searchParams }: { searchParams: Promise<SearchParams> }) { | ||
| const { query = 'Rome', page = '1' } = await searchParams | ||
| const data = await getByQueryArtwork(query, page) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Bug: SearchParams type is too narrow—repeated query parameters become arrays but the code treats them only as strings.
Next.js App Router passes searchParams as a Promise that resolves to { [key: string]: string | string[] | undefined }. When a URL contains repeated parameters (e.g., ?query=rome&query=italy), the value becomes an array. Typing query and page as required strings silently allows arrays to flow into getByQueryArtwork(), which expects string arguments.
Proposed fix
export type SearchParams = {
- query: string
- page: string
+ query?: string | string[]
+ page?: string | string[]
}
+const getFirstParam = (value: string | string[] | undefined, fallback: string) =>
+ Array.isArray(value) ? (value[0] ?? fallback) : (value ?? fallback)
+
export default async function SearchPage({ searchParams }: { searchParams: Promise<SearchParams> }) {
- const { query = 'Rome', page = '1' } = await searchParams
+ const params = await searchParams
+ const query = getFirstParam(params?.query, 'Rome')
+ const page = getFirstParam(params?.page, '1')
const data = await getByQueryArtwork(query, page)| 'use client' | ||
|
|
||
| import Error from 'next/error' | ||
|
|
||
| export default function RootNotFound() { | ||
| return ( | ||
| <html lang='en'> | ||
| <body> | ||
| <Error statusCode={404} /> | ||
| </body> | ||
| </html> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🔍 Additional research query:
Context7 library lookup: Next.js 14-15 App Router not-found file convention and next/error component documentation
💡 Result:
Next.js 14-15 App Router Documentation
not-found File Convention
The not-found.js or not-found.tsx file is used to render custom UI when the notFound() function is called within a route segment. Create a not-found.js or not-found.tsx file in the same route segment to define the custom UI displayed when notFound() is called.
Basic Example:
export default function NotFound() {
return <div>404 - Page Not Found</div>
}Full Example with Navigation:
import Link from 'next/link'
export default function NotFound() {
return (
<div>
<h2>Not Found</h2>
<p>Could not find requested resource</p>
<Link href="/">Return Home</Link>
</div>
)
}Using notFound() Function:
Invoking the notFound() function throws a NEXT_HTTP_ERROR_FALLBACK;404 error and terminates rendering of the route segment in which it was thrown. Specifying a not-found file allows you to gracefully handle such errors by rendering a Not Found UI within the segment.
import { notFound } from 'next/navigation'
async function fetchUser(id) {
const res = await fetch('https://...')
if (!res.ok) return undefined
return res.json()
}
export default async function Profile({ params }) {
const { id } = await params
const user = await fetchUser(id)
if (!user) {
notFound()
}
// ...
}Global Not Found Page:
Create this file in the root app directory to define a global 404 page for unmatched routes, ensuring it returns a full HTML document including <html> and <body> tags.
next/error Component and error.js Convention
The error.js or error.tsx file creates an error boundary for handling runtime errors in a route segment.
Key Requirements:
Use error.js as a Client Component to catch runtime errors in a route segment, log them, and provide a UI for users to retry an operation. Error boundaries must be Client Components.
Basic Implementation:
'use client' // Error boundaries must be Client Components
import { useEffect } from 'react'
export default function Error({
error,
retry,
}: {
error: Error & { digest?: string }
retry: () => void
}) {
useEffect(() => {
// Log the error to an error reporting service
console.error(error)
}, [error])
return (
<div>
<h2>Something went wrong!</h2>
<button onClick={() => retry()}>
Try again
</button>
</div>
)
}catchError Alternative:
The catchError function creates a component that wraps its children in an error boundary. It provides a programmatic alternative to the error.js file convention, enabling component-level error recovery anywhere in your component tree.
🌐 Web query:
Next.js App Router not-found.tsx documentation 2025 html body tags rendering
💡 Result:
In the Next.js App Router, the rendering behavior of not-found.tsx depends on whether it is a standard not-found.tsx file or the global-not-found.tsx (often referred to as global-not-found.js in documentation) [1]. Standard not-found.tsx A standard not-found.tsx file renders as part of the React component tree [1]. It is placed within the route segment where the notFound() function was called (or where the route matching failed within the segment) and is wrapped by the parent layout(s) [1][2][3]. Because it is rendered within the existing layout hierarchy, it does not include <html> or <body> tags itself; these tags are provided by the root layout (app/layout.tsx) which wraps the entire application [4][2]. Global not-found.tsx The global-not-found.tsx (which must be placed in the root app/ directory) is used when a request does not match any defined route at all [1]. Unlike the standard not-found.tsx, this file serves as the entire page and is responsible for rendering the full HTML document [1]. Documentation explicitly states that global-not-found.tsx must include its own <html> and <body> tags [1]. Summary of Key Differences: - Rendering: Standard not-found.tsx renders inside the application layout (wrapped by parent layout.tsx files); global-not-found.tsx replaces the page and renders as a standalone document [1]. - HTML/Body Tags: Standard not-found.tsx should not contain <html> or <body> tags as they are inherited from the root layout [2]. global-not-found.tsx must include <html> and <body> tags [1]. - Trigger: Standard not-found.tsx is triggered by the notFound() function or segment-level failures [1]. global-not-found.tsx is triggered by unmatched URLs across the entire application [1]. Next.js automatically injects the necessary <head> tags (such as for metadata) for pages returning a 404 status code, including those rendered via global-not-found.tsx [1].
Citations:
- 1: https://nextjs.org/docs/app/api-reference/file-conventions/not-found
- 2: https://nextjs.org/docs/app/api-reference/file-conventions/layout
- 3: https://vercel.com/academy/nextjs-foundations/not-found-and-error-surfaces
- 4: https://nextjs.org/docs/app/guides/migrating/app-router-migration
🌐 Web query:
Next.js next/error component Pages Router vs App Router usage
💡 Result:
The next/error component, traditionally used in the Pages Router to create custom error pages (like 500.js), is not part of the App Router's idiomatic error handling architecture. Pages Router Usage In the Pages Router, you typically customize error pages by creating a custom _error.js or _error.tsx file in the pages directory [1]. You can import the default error component from next/error to maintain consistent styling and logic while adding custom behavior or logging [1]. This component is designed to work with the Pages Router's data fetching methods, such as getInitialProps [1][2]. App Router Usage The App Router uses a completely different, file-convention-based approach that does not rely on the next/error component [3][4][5]. Instead, it utilizes specific file names within the route hierarchy to create React Error Boundaries automatically [3][2]: 1. error.tsx: Defines an error boundary for a specific route segment and its children [6][3]. It catches unexpected runtime errors during rendering [4][7]. It must be a Client Component ('use client') and accepts an error object and a reset function to attempt a re-render [3][8][5]. It does not catch errors in the layout of the same segment [3][5]. 2. global-error.tsx: Acts as a safety net for the entire application, specifically catching errors within the root layout (which error.tsx cannot reach) [3][4][7]. Because it replaces the entire application shell, it must define its own and tags [3][7][5]. 3. not-found.tsx: Specifically handles 404 cases, either when a URL does not match any route or when the notFound function is called from next/navigation [4][7][9]. While some internal Next.js utilities (such as unstable_catchError) are exported from next/error, the primary next/error component is not intended for use in the App Router [1][10][11]. Relying on it in the App Router is difficult because the component expects a statusCode property derived from Pages Router lifecycle methods, which do not exist in the same way in the App Router [1]. If no error or global-error components are defined in the App Router, Next.js may fall back to its internal error handling, but you should explicitly define these files to control the user experience [1].
Citations:
- 1: RFC: Default error component for App Router vercel/next.js#59055
- 2: https://www.sitepoint.com/next-js-error-handling-app-router/
- 3: https://nextjslaunchpad.com/article/nextjs-app-router-error-handling-complete-guide-error-tsx-not-found-global-error-boundaries
- 4: https://towardsdev.com/next-js-error-pages-explained-404-error-tsx-not-found-tsx-f5bd9f4701c3
- 5: https://www.matthewswong.com/en/blog/nextjs-error-handling-patterns/
- 6: https://vercel-next-js.mintlify.app/app/core-concepts/error-handling
- 7: https://www.honeybadger.io/blog/next-js-error-handling/
- 8: https://nextjs.org/learn/dashboard-app/error-handling
- 9: https://www.nazarboyko.com/articles/error-handling-in-nextjs-app-router
- 10: https://nextjs.org/docs/app/getting-started/error-handling
- 11: https://en.nextjs.im/docs/app/getting-started/error-handling
Bug
This app/not-found.tsx uses next/error and returns <html>/<body>, both Pages Router patterns that break App Router conventions. The next/error component is designed for Pages Router error handling, not for App Router not-found.tsx. Standard not-found.tsx files render within the layout hierarchy and must not include document tags—those are owned by the root layout. If this file is intended as a global 404 handler, it should be named global-not-found.tsx and exclude next/error while including document tags; otherwise, it should render segment UI only.
Proposed fix
-'use client'
-
-import Error from 'next/error'
-
export default function RootNotFound() {
return (
- <html lang='en'>
- <body>
- <Error statusCode={404} />
- </body>
- </html>
+ <main>
+ <h1>404</h1>
+ <p>Page not found.</p>
+ </main>
)
}Source: Next.js App Router not-found file convention
🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 7-7: This JSX crashes because React isn't in scope.
If you're on React 17+ with the new JSX transform, disable this rule. Otherwise import React at the top of the file.
(react-in-jsx-scope)
[warning] 8-8: This JSX crashes because React isn't in scope.
If you're on React 17+ with the new JSX transform, disable this rule. Otherwise import React at the top of the file.
(react-in-jsx-scope)
[warning] 9-9: This JSX crashes because React isn't in scope.
If you're on React 17+ with the new JSX transform, disable this rule. Otherwise import React at the top of the file.
(react-in-jsx-scope)
Source: Path instructions
| const pageNumber = Number(page) | ||
|
|
||
| const firstPage = pageNumber === 1 | ||
| const lastPage = Math.ceil(recordsCount / Number(PAGE_SIZE)) <= pageNumber | ||
| return ( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/pagination/Pagination.tsx | head -50Repository: wingedseraph/rs26
Length of output: 2405
Validate page parameter before using in pagination logic.
Using Number(page) without validation allows NaN or invalid values from malformed URL params, which breaks pagination state and generates invalid prev/next links. URL-derived state must be validated to keep pagination logic deterministic.
Proposed fix
function Pagination({ page, query, recordsCount }: PaginationProperties) {
- const pageNumber = Number(page)
+ const parsedPage = Number.parseInt(page, 10)
+ const pageNumber = Number.isFinite(parsedPage) && parsedPage > 0 ? parsedPage : 1Also applies to: 28, 41
Source: MDN - Number()
| const locales = ['en', 'ru'] | ||
| const defaultLocale = 'en' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Suggestion
Line 3 defines locales as a widened string[], which causes AppConfig.Locale in src/global.ts to lose the 'en' | 'ru' union and weakens compile-time locale safety.
This is a type-safety issue: widened literals reduce the value of your module augmentation contract and make invalid locale values easier to pass unnoticed.
Proposed fix
-const locales = ['en', 'ru']
+const locales = ['en', 'ru'] as constAs per path instructions, "Use the connected Context7 MCP server to look up current documentation for every library imported in the changed files" and "Every review comment MUST follow this structure."
Source: TypeScript Handbook: as const for literal narrowing
📝 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.
| const locales = ['en', 'ru'] | |
| const defaultLocale = 'en' | |
| const locales = ['en', 'ru'] as const | |
| const defaultLocale = 'en' |
Source: Path instructions
| export function cardToCsv(card: Card[], origin?: string) { | ||
| const header = 'id,name,description,details URL' | ||
|
|
||
| const data = card.map(element => [element.systemNumber, element.objectType, element._primaryTitle, `${window.location.origin}/card/${element.systemNumber}`].join(',')) | ||
| const data = card.map(element => [element.systemNumber, element.objectType, element._primaryTitle, `${origin}/card/${element.systemNumber}`].join(',')) | ||
| const csv = [header, ...data].join('\r\n') |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and matches the snippet
cat -n src/lib/cardToCsv.tsRepository: wingedseraph/rs26
Length of output: 592
🏁 Script executed:
# Check for CSV-related dependencies in package.json
git ls-files | grep -E 'package\.json$'Repository: wingedseraph/rs26
Length of output: 168
🏁 Script executed:
# Search for any existing CSV imports or usage
rg -i 'csv|papaparse|csv-stringify' --type jsonRepository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Check package.json contents for CSV-related dependencies
cat -n package.jsonRepository: wingedseraph/rs26
Length of output: 2745
🏁 Script executed:
# Search for where cardToCsv is called to understand usage context
rg 'cardToCsv' --type ts --type tsx -B 2 -A 2Repository: wingedseraph/rs26
Length of output: 183
🏁 Script executed:
# Check the Card type definition to understand the data
rg 'type Card' --type ts --type tsx -A 10Repository: wingedseraph/rs26
Length of output: 183
🏁 Script executed:
# Search for cardToCsv usage
rg 'cardToCsv' -A 2 -B 2Repository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Find Card type definition
rg 'type Card' -A 10Repository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Check if any files use this function or export it
rg 'export.*cardToCsv|import.*cardToCsv' -A 2Repository: wingedseraph/rs26
Length of output: 155
🏁 Script executed:
# Try broader search with case-insensitive
rg -i 'card' --type-list | head -20Repository: wingedseraph/rs26
Length of output: 545
🏁 Script executed:
# List all TypeScript/JavaScript files to understand structure
fd -e ts -e tsx | head -20Repository: wingedseraph/rs26
Length of output: 844
🏁 Script executed:
# Check api/types to find Card definition
find . -name 'types*' -o -name 'Card*' | head -10Repository: wingedseraph/rs26
Length of output: 381
🏁 Script executed:
# Check the Card type definition
cat -n src/api/types.tsRepository: wingedseraph/rs26
Length of output: 712
🏁 Script executed:
# Now search for cardToCsv usage in all files
rg 'cardToCsv' --no-type-listRepository: wingedseraph/rs26
Length of output: 599
🏁 Script executed:
# Search for cardToCsv usage
grep -r 'cardToCsv' --include='*.ts' --include='*.tsx' .Repository: wingedseraph/rs26
Length of output: 674
🏁 Script executed:
# Check if the function is actually used anywhere
grep -r 'cardToCsv\|card.*csv\|csv.*card' --include='*.ts' --include='*.tsx' . 2>/dev/null | head -20Repository: wingedseraph/rs26
Length of output: 948
🏁 Script executed:
# Check the CSV route to see how origin is used
cat -n src/app/api/csv/route.tsRepository: wingedseraph/rs26
Length of output: 1462
🏁 Script executed:
# Check the test file to see what's already being tested
cat -n src/lib/cardToCsv.test.tsRepository: wingedseraph/rs26
Length of output: 1049
🏁 Script executed:
# Check the mock data to understand what values are being tested
cat -n src/mocks/mocks.ts | head -100Repository: wingedseraph/rs26
Length of output: 206
🏁 Script executed:
# Find the mocks file
find . -name 'mocks*' -type fRepository: wingedseraph/rs26
Length of output: 182
🏁 Script executed:
# Search for CARD_WITH_PRIMARY_TITLE_MOCK definition
grep -r 'CARD_WITH_PRIMARY_TITLE_MOCK' --include='*.ts' --include='*.tsx'Repository: wingedseraph/rs26
Length of output: 2288
🏁 Script executed:
# Check the mock data
cat -n src/tests/mocks/mocks.ts | grep -A 20 'CARD_WITH_PRIMARY_TITLE_MOCK ='Repository: wingedseraph/rs26
Length of output: 1154
🌐 Web query:
RFC 4180 CSV format escaping rules double quotes
💡 Result:
According to RFC 4180, which defines the common format and MIME type for CSV files, the rules for handling double quotes are as follows: 1. Fields that contain line breaks (CRLF), double quotes, or commas must be enclosed in double quotes [1][2]. 2. If a field is enclosed in double quotes, any double quote character appearing inside that field must be escaped by preceding it with another double quote [1][2]. For example, a literal double quote inside a field would be represented as two double quotes in succession ("""") [1][2]. 3. If fields are not enclosed in double quotes, then double quotes are not permitted to appear anywhere inside those fields [1][2]. In summary, when a field is quoted, the only way to represent a double quote character is to double it up [1].
Citations:
🌐 Web query:
OWASP CSV injection formula attack prevention
💡 Result:
CSV injection (also known as Formula Injection) occurs when an application embeds untrusted, user-controlled input into CSV files, causing spreadsheet software like Microsoft Excel or LibreOffice Calc to misinterpret text as executable formulas [1][2]. Because there is no universal CSV standard that reliably disables formula execution across all spreadsheet applications, there is no single "bulletproof" mitigation [1][3]. To prevent or mitigate CSV injection, the OWASP Foundation and related security standards recommend the following strategies: Core Mitigation Strategy The primary goal is to ensure that no cell starts with a formula-triggering character (=, +, -, @, \t, or 0x0). If a field starts with any of these, it should be sanitized so the spreadsheet treats it as literal text [1][4][2]. Recommended Sanitization Techniques - Prefixing: Prepend a single quote (') to any cell that starts with a dangerous character [1][3]. Note that some spreadsheet applications may not reliably process this upon import or re-export [2][5]. - Excel-Resistant Mitigation: For files specifically intended for Microsoft Excel, prefixing the cell content with a tab character (0x09) inside a quoted field (e.g., "\t=1+1") is often effective [1][6][2]. - Defense-in-Depth: When generating CSVs, ensure proper RFC 4180 compliance, including wrapping fields in double quotes and escaping existing double quotes by doubling them (e.g., " becomes "") [4][2]. Important Considerations and Limitations - Excel Behavior: Microsoft Excel may remove quotes or escape characters when a file is saved and re-opened, potentially re-activating previously neutralized formulas [1][2]. - Data Integrity: Adding characters like tabs or single quotes becomes part of the actual data, which may cause issues for downstream applications that process the CSV programmatically [1][2]. - Alternative Formats: If CSV is not strictly required, consider using formats that do not support formulas, such as JSON or XML [7]. - Input Validation: As a defense-in-depth measure, sanitize and validate data at the point of entry to ensure it does not contain malicious payloads, though this does not replace the need for output sanitization [7]. Because of these limitations, always validate your chosen mitigation against the specific spreadsheet applications and workflows used by your organization [2]. Do not rely on a single, universal fix [1][3].
Citations:
- 1: https://owasp.org/www-community/attacks/CSV_Injection
- 2: https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/21-Testing_for_CSV_Injection
- 3: https://cwe.mitre.org/data/definitions/1236
- 4: https://asvs.dev/v5.0.0/V1-Encoding-and-Sanitization/
- 5: https://security.stackexchange.com/questions/279321/preventing-csv-injection
- 6: https://github.com/OWASP/www-community/blob/master/pages/attacks/CSV_Injection.md
- 7: https://www.securecodinghub.com/guides/csv-injection
Security: CSV serialization lacks RFC 4180 escaping and formula injection protection.
Line 6 generates CSV by joining raw field values with commas. This violates RFC 4180 (fields containing commas, line breaks, or quotes must be wrapped in quotes and internal quotes must be escaped) and creates a formula injection vulnerability—cells beginning with =, +, -, or @ will execute as formulas when opened in spreadsheet software.
Additionally, the optional origin parameter is used directly in a template string without a fallback, resulting in undefined/card/... URLs when origin is not provided.
Proposed fix
export function cardToCsv(card: Card[], origin?: string) {
const header = 'id,name,description,details URL'
+ const escapeCsvCell = (value: unknown) => {
+ const raw = String(value)
+ const neutralized = /^[=+\-@]/.test(raw) ? `'${raw}` : raw
+ return `"${neutralized.replace(/"/g, '""')}"`
+ }
+ const detailsUrl = (id: number) => (origin ? `${origin}/card/${id}` : `/card/${id}`)
- const data = card.map(element => [element.systemNumber, element.objectType, element._primaryTitle, `${origin}/card/${element.systemNumber}`].join(','))
+ const data = card.map(element =>
+ [
+ escapeCsvCell(element.systemNumber),
+ escapeCsvCell(element.objectType),
+ escapeCsvCell(element._primaryTitle),
+ escapeCsvCell(detailsUrl(element.systemNumber)),
+ ].join(','),
+ )
const csv = [header, ...data].join('\r\n')Source: RFC 4180 — Common Format and MIME Type for CSV Files, OWASP — CSV Injection
📝 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.
| export function cardToCsv(card: Card[], origin?: string) { | |
| const header = 'id,name,description,details URL' | |
| const data = card.map(element => [element.systemNumber, element.objectType, element._primaryTitle, `${window.location.origin}/card/${element.systemNumber}`].join(',')) | |
| const data = card.map(element => [element.systemNumber, element.objectType, element._primaryTitle, `${origin}/card/${element.systemNumber}`].join(',')) | |
| const csv = [header, ...data].join('\r\n') | |
| export function cardToCsv(card: Card[], origin?: string) { | |
| const header = 'id,name,description,details URL' | |
| const escapeCsvCell = (value: unknown) => { | |
| const raw = String(value) | |
| const neutralized = /^[=+\-@]/.test(raw) ? `'${raw}` : raw | |
| return `"${neutralized.replace(/"/g, '""')}"` | |
| } | |
| const detailsUrl = (id: number) => (origin ? `${origin}/card/${id}` : `/card/${id}`) | |
| const data = card.map(element => | |
| [ | |
| escapeCsvCell(element.systemNumber), | |
| escapeCsvCell(element.objectType), | |
| escapeCsvCell(element._primaryTitle), | |
| escapeCsvCell(detailsUrl(element.systemNumber)), | |
| ].join(','), | |
| ) | |
| const csv = [header, ...data].join('\r\n') |
Source: Path instructions
| <Image | ||
| loading='eager' | ||
| width={600} | ||
| height={600} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the CardItem.tsx file to understand the context
cat -n src/widgets/card-list/CardItem.tsx | head -60Repository: wingedseraph/rs26
Length of output: 2515
🏁 Script executed:
# Let's also check how CardItem is used in the codebase
rg "CardItem" --type tsx --type ts -A 2 -B 2 | head -50Repository: wingedseraph/rs26
Length of output: 183
🌐 Web query:
Next.js Image component loading lazy eager performance best practices grid list
💡 Result:
To optimize performance when using the Next.js Image component in grid lists or standard layouts, follow these best practices based on current framework standards (as of Next.js 16+): Core Loading Strategy 1. Use Lazy Loading by Default: The next/image component defaults to lazy loading (loading="lazy"), which is ideal for almost all images that are not immediately visible. This defers downloading until an image approaches the viewport, saving bandwidth and improving initial load speed [1][2][3]. 2. Identify LCP Elements: Only images that are part of the Largest Contentful Paint (LCP)—typically hero images or the primary image above the fold—should be loaded eagerly [4][2][5]. 3. Preload vs. Eager: - Use the preload prop for your primary LCP image to insert a link tag in the document head and fetch the image early [1][2]. - Use loading="eager" for critical above-the-fold content that is not the LCP (e.g., a logo or secondary hero element) [1][6]. - Note: The priority prop was deprecated in Next.js 16 in favor of the preload prop to clarify these behaviors [1][6]. Avoid combining preload, loading="eager", and fetchPriority="high" indiscriminately, as they serve specific purposes [1][6]. Grid List Optimization 1. Implement the sizes Prop: This is the most critical step for responsive grids. Without it, the browser assumes an image will be 100vw, often downloading significantly larger assets than necessary [4][7][5]. Match the sizes string to the rendered width of your image at each CSS breakpoint (e.g., sizes="(min-width: 808px) 50vw, 100vw") [7][8]. 2. Avoid Preloading Grids: Do not use preload or priority on images within a grid list unless they are guaranteed to be the LCP element [4][2][5]. Applying these to multiple images creates resource contention, where images compete for network priority, ultimately slowing down the page [4][5]. 3. Selective Eager Loading: In long grids, you may opt to set loading="eager" only for the first few images (e.g., the first 4 items in a grid) that are likely to be visible immediately upon page load [8]. All other images should remain lazy [4][8]. General Best Practices - Layout Stability: Always define width and height or use the fill prop to allow the framework to reserve space and prevent layout shifts (CLS) [7][3][5]. - Modern Formats: Ensure your Next.js configuration is set to support modern formats like AVIF or WebP to reduce file size [4][3][8]. - Static Imports: Use static imports for local images whenever possible; this automatically provides the image dimensions and allows for "blur-up" placeholders, significantly improving developer experience and performance [4][3][8]. - Validate with Tools: Use Lighthouse or browser dev tools to confirm which element is truly your LCP before applying performance props [4]. When everything is prioritized, nothing is [2][5].
Citations:
- 1: https://vercel-next-js.mintlify.app/api-reference/components/image
- 2: https://socialanimal.dev/blog/nextjs-image-optimization-core-web-vitals-2026/
- 3: https://nextjs.org/docs/13/app/building-your-application/optimizing/images
- 4: https://www.stanza.dev/concepts/nextjs-image-optimization
- 5: https://usamasoft.com/blog/nextjs-image-optimization-guide
- 6: https://nextjs.org/docs/app/api-reference/components/image
- 7: https://nextjs.org/docs/pages/api-reference/components/image
- 8: https://www.vidhyasagarthakur.engineer/blog/nextjs-image-optimization-deep-dive-performance-responsive-images-and-configuration
Remove eager loading from grid card thumbnails to avoid resource contention.
Card thumbnails in this grid should use lazy loading by default; only above-the-fold images that are guaranteed visible on initial page load should be eager-loaded. Applying loading='eager' to every card in the list forces the browser to download all thumbnails immediately, competing for network priority and slowing initial render.
Proposed fix
<Image
- loading='eager'
width={600}
height={600}Source: Next.js Image Component: loading
📝 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.
| <Image | |
| loading='eager' | |
| width={600} | |
| height={600} | |
| <Image | |
| width={600} | |
| height={600} |
🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 31-31: This JSX crashes because React isn't in scope.
If you're on React 17+ with the new JSX transform, disable this rule. Otherwise import React at the top of the file.
(react-in-jsx-scope)
| <Button | ||
| title='Unselect all selected cards' | ||
| title={t('unselectAllTitle')} | ||
| className='block h-fit cursor-pointer text-lg font-bold hover:bg-silver-mist-hover hover:no-underline' | ||
| onClick={() => dispatch(removeAll())} | ||
| > | ||
|
|
||
| Unselect all | ||
| {t('unselectAll')} | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bug: The "unselect all" Button has no explicit type, so inside this <form> it defaults to type="submit" and clicking it submits the CSV form (triggering a /api/csv download) in addition to dispatching removeAll().
Reasoning: Per the HTML spec, a <button> with a missing or invalid type defaults to submit. The shared Button sets no default type, so this control conflates two actions (separation of concerns) and fires an unintended network submission. Set type='button' to make it a non-submitting action.
<Button
title={t('unselectAllTitle')}
+ type='button'
className='block h-fit cursor-pointer text-lg font-bold hover:bg-silver-mist-hover hover:no-underline'
onClick={() => dispatch(removeAll())}
>
{t('unselectAll')}
</Button>Source: MDN: button type
📝 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.
| <Button | |
| title='Unselect all selected cards' | |
| title={t('unselectAllTitle')} | |
| className='block h-fit cursor-pointer text-lg font-bold hover:bg-silver-mist-hover hover:no-underline' | |
| onClick={() => dispatch(removeAll())} | |
| > | |
| Unselect all | |
| {t('unselectAll')} | |
| </Button> | |
| <Button | |
| title={t('unselectAllTitle')} | |
| type='button' | |
| className='block h-fit cursor-pointer text-lg font-bold hover:bg-silver-mist-hover hover:no-underline' | |
| onClick={() => dispatch(removeAll())} | |
| > | |
| {t('unselectAll')} | |
| </Button> |
🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 45-45: This JSX crashes because React isn't in scope.
If you're on React 17+ with the new JSX transform, disable this rule. Otherwise import React at the top of the file.
(react-in-jsx-scope)
Source: Path instructions
| ".next/types/**/*.ts", | ||
| ".next/dev/types/**/*.ts" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the tsconfig.json file
find . -name "tsconfig.json" -type f | head -20Repository: wingedseraph/rs26
Length of output: 171
🏁 Script executed:
# Check TypeScript documentation about include/exclude pattern precedence
# Also examine Next.js guidance on .next directory configurationRepository: wingedseraph/rs26
Length of output: 155
🌐 Web query:
TypeScript include exclude pattern precedence semantics how does exclude affect include patterns
💡 Result:
In TypeScript, the exclude property functions strictly as a filter for files found by the include property [1][2]. It is not a mechanism to prevent a file from entering the compilation process entirely; it only limits what the include setting discovers [1][3]. Key Semantics and Precedence: 1. Filter Mechanism: The exclude property specifically affects the files that would otherwise be included via the include property [1][4]. If a file is not matched by an include pattern, it is not "excluded" by an exclude pattern—it was never included to begin with [5]. 2. Limitations: Because exclude is only a filter on include, it cannot prevent a file from being included if that file is added via other means [1][2]. Specifically, files will still be part of your codebase if they are: - Explicitly listed in the files property [1][2]. - Imported or referenced by another file that is already included in the compilation (e.g., via import statements or triple-slash directives) [1][2][5]. - Included via types or reference types configuration [1][4]. 3. Precedence and Interactions: - Files specified in the files property are always included, regardless of any exclude patterns [2][5]. - When both files and include are specified, the compiler uses the union of files matched by both (filtered by exclude) [2]. - If neither files nor include is specified, the compiler defaults to including all files in the root directory and subdirectories, excluding node_modules, bower_components, and jspm_packages by default [2]. Specifying an exclude property in this scenario will filter this default set [5]. In summary, exclude is a narrow filter for include patterns and does not override other methods of file inclusion in the TypeScript program [1][4].
Citations:
- 1: https://www.typescriptlang.org/tsconfig/
- 2: https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/tutorials/tsconfig.json.md
- 3: https://aka.ms/tsconfig/
- 4: TypeScript shouldn't compile configured exclusions microsoft/TypeScript#39580
- 5: [nightly] using only exclude and include cancel each other out microsoft/TypeScript#9806
🌐 Web query:
Next.js .next directory tsconfig include exclude configuration best practices
💡 Result:
For a Next.js project, the best practice regarding the.next directory and tsconfig.json is to explicitly include the generated framework types while letting Next.js manage the configuration automatically [1][2][3]. Recommended Configuration Next.js automatically maintains your tsconfig.json to ensure proper type checking for App Router or Pages Router features [1][4]. The standard include array should be: { "include": [ "next-env.d.ts", ".next/types//.ts", "/.ts", "/.tsx" ], "exclude": ["node_modules"] } Key Practices 1. Include Generated Types: You must include.next/types//.ts in your include array [1][2]. This allows TypeScript to recognize framework-generated types, such as route parameters and metadata helpers, which are essential for type safety in Next.js applications [5][3]. 2. Let Next.js Manage It: When you run next dev or next build, Next.js automatically updates your tsconfig.json to include these paths if they are missing [2][3]. It is generally recommended to avoid manually removing these entries to prevent loss of type coverage [5]. 3. Handle next-env.d.ts: Do not modify next-env.d.ts, as it is automatically regenerated by Next.js [1][3]. It is best practice to add this file to your.gitignore [1][2]. 4. Exclude node_modules: Always keep node_modules in the exclude array to ensure the TypeScript compiler does not attempt to parse external library code, which can cause significant performance degradation [1][6]. 5. Troubleshooting Duplicates: If you notice duplicate paths (e.g., relative vs. absolute paths) appearing in your tsconfig.json after running commands like next lint, this is a known behavior of the Next.js CLI [7][8]. While these duplicates are generally harmless to the build, they can sometimes cause IDE noise; Next.js continuously works to refine this behavior [7][8]. If you are setting up a project manually or having issues with missing types, ensure these entries are present [1][2]. Avoid complex custom exclude patterns unless necessary, as the standard defaults provided by Next.js are optimized for most use cases [9].
Citations:
- 1: https://nextjs.org/docs/pages/api-reference/config/typescript
- 2: https://nextjs.im/docs/app/api-reference/config/typescript/
- 3: https://vercel-next-js.mintlify.app/api-reference/config/typescript
- 4: https://nextjs.org/docs/app/api-reference/config/typescript
- 5: https://frameworkkit.dev/guides/tsconfig-for-nextjs
- 6: https://man.hubwiz.com/docset/TypeScript.docset/Contents/Resources/Documents/www.typescriptlang.org/docs/handbook/tsconfig-json.html
- 7: [NEXT-792] dont add duplicate paths to tsconfig.json#include vercel/next.js#46845
- 8:
next lintadds unnecessary absolute pathdistDirtoincludein tsconfig.json vercel/next.js#62796 - 9: the "include" property in tsconfig.json is not nessecary (and can sometimes causes issues) vercel/next.js#46905
🏁 Script executed:
cat -n ./tsconfig.jsonRepository: wingedseraph/rs26
Length of output: 1662
Bug
The exclude pattern for .next filters out the explicitly included .next/types/**/*.ts, preventing TypeScript from type-checking Next.js-generated type definitions.
Reasoning: TypeScript's exclude property acts as a filter on patterns matched by include. The exclusion of .next will exclude files matching .next/types/**/*.ts, which breaks type-safety for Next.js framework-generated types like typed routes and metadata helpers. According to Next.js official guidance, only node_modules should be excluded.
Proposed fix
"exclude": [
- "./node_modules",
- ".next"
+ "./node_modules"
]Source: TypeScript TSConfig exclude behavior, Next.js TypeScript configuration guide
MielomankA
left a comment
MielomankA
left a comment
There was a problem hiding this comment.
Great job - this was a significant piece of work! 🥇 I've left a couple of comments. Also, please take a look at some of CodeRabbit's feedback, as there are a few points worth addressing
| const records = data.records | ||
| const recordsCount = data.recordsCount |
There was a problem hiding this comment.
destructuring can be used here
| const records = data.records | |
| const recordsCount = data.recordsCount | |
| const { records, recordsCount } = data; |
| <> | ||
| <div className='max-w-3xl outlet:max-w-none outlet:flex-1'> | ||
| <Header query={query} page={page} /> | ||
| <CardList data={records ?? FALLBACK_CARDS} query={query} page={page} /> | ||
| <Pagination query={query} page={page} recordsCount={recordsCount ?? FALLBACK_CARDS.length} /> | ||
| <Flyout /> | ||
| </div> | ||
| </> |
There was a problem hiding this comment.
You can get rid of the react fragment here
| <> | ||
| <StoreProvider> |
There was a problem hiding this comment.
Nit: The fragment is unnecessary here since there's already a single root element
2 participants
deploy
Done 23.06.2026 / deadline 23.06.2026
Score: 100/100