Add Tailwind CSS setup, theme system, and UI components / flow#13
Open
Casjb wants to merge 36 commits into
Open
Add Tailwind CSS setup, theme system, and UI components / flow#13Casjb wants to merge 36 commits into
Casjb wants to merge 36 commits into
Conversation
…ality. also added a proper loader as an upgrade to the old text based one
… key that changes when pagination is iterated or an arbitrary trigger is fired
📝 WalkthroughWalkthroughAdds Tailwind/PostCSS, a CSS-variable theme (dark/light), global error and loading Redux slices with providers/UI, new generic UI components and hooks (resource list, extendable context menu, global loader), numerous helper refactors, and a ThemeTest page and route. Changes
Sequence Diagram(s)sequenceDiagram
participant Page
participant DisplayResourceGeneric
participant useResource
participant FetchHelper
participant Redux
Page->>DisplayResourceGeneric: render(fetchFn, page)
DisplayResourceGeneric->>useResource: init(fetchFn, page)
DisplayResourceGeneric->>useResource: call load()
useResource->>FetchHelper: fetchFn(page)
alt success
FetchHelper-->>useResource: response(items,_meta)
useResource->>DisplayResourceGeneric: {items,totalPages}
DisplayResourceGeneric->>Page: render list & pagination
else failure
FetchHelper->>Redux: call throwError(msg)
Redux->>Redux: setError + stopLoad
FetchHelper-->>useResource: null
useResource->>DisplayResourceGeneric: ready=false / empty
end
sequenceDiagram
participant Component
participant Redux
participant ErrorProvider
participant ErrorBanner
Component->>Redux: call throwError(msg)
activate Redux
Redux->>Redux: dispatch setError(msg)
Redux->>Redux: dispatch stopLoad()
deactivate Redux
ErrorProvider->>Redux: useSelector(selectError)
Redux-->>ErrorProvider: error state
ErrorProvider->>ErrorBanner: render(message)
ErrorBanner->>Component: visible banner
Component->>ErrorBanner: click ✕
ErrorBanner->>Redux: clearError()
activate Redux
Redux->>Redux: dispatch resetError()
deactivate Redux
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/helpers/validation/checkUniqueness.ts (1)
6-13:⚠️ Potential issue | 🟠 MajorValidate the API response shape before returning uniqueness.
On Line 12, the function returns raw JSON without checking
response.okor type. If the backend returns an error object (truthy), callers can treat it asunique=trueand allow invalid input.Suggested fix
try { const response = await fetch( `${BASE_URL}/${tableName}/check-unique?name=${encodeURIComponent(fieldName)}`, { credentials: 'include', } ) - return await response.json() + if (!response.ok) return false + + const data: unknown = await response.json() + if (typeof data === 'boolean') return data + if ( + typeof data === 'object' && + data !== null && + 'unique' in data && + typeof (data as { unique: unknown }).unique === 'boolean' + ) { + return (data as { unique: boolean }).unique + } + return false } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/helpers/validation/checkUniqueness.ts` around lines 6 - 13, The function that fetches `${BASE_URL}/${tableName}/check-unique` currently returns raw JSON without checking response status or verifying the shape; update the fetch handling in checkUniqueness (use the existing response variable) to first verify response.ok, parse the JSON, and validate that the parsed object contains the expected boolean field (e.g., unique or isUnique) before returning; if response.ok is false or the shape is invalid, throw or return a clear error/false result so callers cannot treat an error object as unique.frontend/src/helpers/manageResource/createNewResource.ts (1)
17-37:⚠️ Potential issue | 🟠 MajorDo not swallow create failures before running
onReload.Current flow logs errors and continues, so failed creates still execute
onReloadand appear successful to callers.Suggested fix
import { BASE_URL } from '../exports/exportEnv.ts' +import { throwError } from '../exports/exportError.ts' @@ export const createNewResource = async ({ tableName, data, onReload }: ResourceCreation) => { - await fetch(`${BASE_URL}/${tableName}/create`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - name: data.name, - }), - credentials: 'include', - }) - .then(res => res.json()) - .then(data => { - console.log(data) - }) - .catch(err => console.log(err)) + try { + const response = await fetch(`${BASE_URL}/${tableName}/create`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ name: data.name }), + credentials: 'include', + }) + + if (!response.ok) { + throw new Error(`Create failed (${response.status})`) + } + + await response.json() + } catch (err: any) { + throwError(`Failed to create resource: ${err.message}`) + return + } @@ if (onReload) { await onReload() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/helpers/manageResource/createNewResource.ts` around lines 17 - 37, The createNewResource function currently swallows fetch errors and always calls onReload; update createNewResource so it checks the fetch response (use response.ok or parsed payload status) and throws or rethrows on failure instead of just console.logging, and only call await onReload() after a successful create. Locate the fetch block in createNewResource, ensure non-2xx responses and caught exceptions are propagated (or return an error) so callers know the create failed, and keep onReload invocation conditional on a successful create.
🧹 Nitpick comments (6)
frontend/src/Main.tsx (1)
10-14: Guard module-scope DOM access for non-browser execution.On Lines 10-14,
window/documentare accessed during module evaluation. A simple runtime guard makes this safer for SSR-like or DOM-less test environments.Suggested hardening
-const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches -// only apply light if system explicitly prefers it -if (!prefersDark) { - document.documentElement.classList.add('light') -} +if (typeof window !== 'undefined' && typeof document !== 'undefined') { + const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches + // only apply light if system explicitly prefers it + if (!prefersDark) { + document.documentElement.classList.add('light') + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Main.tsx` around lines 10 - 14, The module currently accesses window and document at top-level (prefersDark using window.matchMedia and document.documentElement.classList.add('light')), which breaks in non-browser environments; wrap that logic in a runtime guard such as checking typeof window !== 'undefined' && typeof document !== 'undefined' (and that window.matchMedia exists) before evaluating prefersDark or mutating document, so the code only runs in browsers and is safe for SSR/tests.frontend/src/App.tsx (1)
37-37: Gate/Test/Themebehind a dev-only condition.This route is marked temporary; consider only registering it in development builds to avoid exposing internal test surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` at line 37, The Test Theme route is currently always registered; wrap the Route that renders ThemeTest (the JSX line Route path="/Test/Theme" element={<ThemeTest />} inside App or the component that builds routes) in a dev-only conditional so it only mounts in development builds (e.g., guard with process.env.NODE_ENV === 'development' or your build's dev flag like import.meta.env.DEV). Keep the same Route JSX but render it only when the dev flag is true so production builds do not expose the /Test/Theme test page.frontend/src/components/load/GlobalLoader.tsx (1)
16-16: Prefer the theme overlay token for consistency.On Line 16, consider using
var(--color-bg-overlay)instead of hardcodedrgba(0, 0, 0, 0.5)so loader overlay stays fully theme-driven.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/load/GlobalLoader.tsx` at line 16, Replace the hardcoded overlay color in the GlobalLoader component by using the theme token: when computing the backgroundColor (the property currently set to opaque ? 'var(--color-bg)' : 'rgba(0, 0, 0, 0.5)'), swap the hardcoded rgba with 'var(--color-bg-overlay)' so the overlay uses the theme token; update the conditional expression where backgroundColor is assigned in GlobalLoader to use 'var(--color-bg-overlay)' for the non-opaque case.frontend/src/providers/LoadingProvider.tsx (1)
13-17: Use a real delay threshold for anti-flicker behavior.On Line 16,
setTimeout(..., 0)only defers to the next tick and usually won’t reduce flash on quick loads. Consider a small real threshold (e.g. 120–200ms).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/providers/LoadingProvider.tsx` around lines 13 - 17, The current useEffect in LoadingProvider defers showing the loader with setTimeout(..., 0) which doesn't prevent flicker; change the timeout to a small real threshold (e.g., 150ms) so the timer created when isLoading becomes true calls setShowLoader(true) after that delay, while keeping the existing cleanup that clears the timer; update references in the useEffect where isLoading, setShowLoader and the timer are used to use the new delay constant (120–200ms, e.g., 150ms).frontend/src/components/context/ExtendableContextMenu.tsx (2)
112-112: Simplify redundant submit label expression.Both branches return the same string.
💡 Suggested fix
- {isAnyValidating ? 'Submit' : 'Submit'} + Submit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/context/ExtendableContextMenu.tsx` at line 112, In ExtendableContextMenu.tsx inside the JSX rendering (the Submit label currently written as the redundant ternary {isAnyValidating ? 'Submit' : 'Submit'}), replace the ternary expression with the plain string 'Submit' to remove the unnecessary conditional and simplify the markup.
92-98: Associate labels/errors with inputs for accessibility.The input label is not programmatically connected, and error text is not announced to assistive tech.
💡 Suggested fix
if (field.type === 'input') { + const inputId = `context-menu-input-${i}` + const errorId = `context-menu-error-${i}` return ( <div key={i}> - <label>{field.label}</label> + <label htmlFor={inputId}>{field.label}</label> <input + id={inputId} placeholder={field.placeholder} value={values[i] ?? ''} onChange={e => handleChange(i, field, e.target.value)} + aria-invalid={Boolean(errors[i])} + aria-describedby={errors[i] ? errorId : undefined} /> - {errors[i] && <span style={{ color: 'red', fontSize: '12px' }}>{errors[i]}</span>} + {errors[i] && ( + <span id={errorId} role="alert" style={{ color: 'red', fontSize: '12px' }}> + {errors[i]} + </span> + )} </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/context/ExtendableContextMenu.tsx` around lines 92 - 98, The label and error text must be programmatically associated with the input in ExtendableContextMenu.tsx: give each input a unique id (e.g., derived from field.name or the index i), set the label's htmlFor to that id, add aria-describedby on the input pointing to the error span id when errors[i] exists, and set aria-invalid={!!errors[i]} so assistive tech announces validation; update the span to have a matching id (e.g., `${id}-error`) and keep handleChange/onChange usage as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 35-57: There are plain text nodes after Route elements inside
Routes (e.g., <Route path="/Login" element={<Login />} />, <Route
path="/Test/Theme" element={<ThemeTest />} />, ProtectedRoutes wrapper and
children like Home, Test, Leagues, League, Trainers, Trainer, Teams, Team,
DocsRoot, DocsHierarchy, DocsLeagues, DocsTrainers) which create invalid JSX
inside <Routes>; replace each trailing text description (e.g., " - login page")
with a JSX comment (/* ... */) so the Routes children contain only valid
elements and comments (e.g., change `... /> - login page` to `... /> {/* login
page */}`).
In `@frontend/src/components/api/DisplayResourceGeneric.tsx`:
- Around line 24-26: Normalize and validate the page query param before use:
when reading searchParams.get('page') in DisplayResourceGeneric, parse it into
an integer and if it is NaN or less than 1 default to 1 (assign to pageNum);
ensure setPageNum also only writes positive integer strings (e.g., coerce input
to Math.max(1, Math.floor(page))). Update the logic around useSearchParams,
pageNum, and setPageNum to enforce this normalization so invalid hand-edited
values like "foo" or "-1" become page 1.
- Around line 43-49: The current items.map callback nests a Link inside a button
which is invalid; change the outer element to be the Link (use Link as the
primary interactive element produced in items.map with key={item.id} and
to={path(item.id)}) and move the onContextMenu handler onto that Link
(onContextMenu={e => onContextMenu?.(e, item)}), or alternatively keep a button
and perform navigation imperatively using history/navigation in the button’s
onClick—do not nest Link inside a button and ensure only a single interactive
element per item (refer to items.map, onContextMenu, Link, path).
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Line 8: The onSubmit prop in ExtendableContextMenu is currently called
fire-and-forget which can cause unhandled rejections and duplicate submissions;
update the component to await the result of onSubmit and prevent concurrent
submits by adding an isSubmitting state flag (e.g., useState<boolean>
isSubmitting) and use it to short-circuit subsequent clicks, wrap the await in
try/catch to handle rejections and a finally to reset isSubmitting, and apply
this pattern to every call-site inside ExtendableContextMenu that invokes
props.onSubmit so submissions are serialized and errors are handled.
- Around line 72-73: The inline style in ExtendableContextMenu sets background:
'white' and border: '1px solid black', which bypasses theming; replace these
hardcoded values with theme tokens (e.g., use the app theme's background and
border/divider tokens via your theme provider or useTheme hook) so the menu uses
theme.palette/background and theme.palette/divider (or your project's equivalent
tokens) instead of 'white'/'black'; update the style object in
ExtendableContextMenu to read theme values for background and border and remove
hardcoded colors.
- Around line 41-47: The async validation block for field.validate can produce
stale/out-of-order results and doesn't handle thrown errors; wrap the await in a
try/catch and gate state updates with a validation token or value snapshot so
only the latest validation result mutates state: when starting validation for
field.validate (use the same index i and current val), generate a unique token
(or capture the current val) and store it locally, call setValidating as now,
then await field.validate inside try/catch, on success check the token/snapshot
still matches the current input before calling setValid and setErrors, on
failure setErrors appropriately (use field.validationError or the caught error
message), and in finally only clear setValidating for that index if the
token/snapshot still matches; reference field.validate, setValidating, setValid,
setErrors and the index i/val to locate where to apply this.
In `@frontend/src/components/error/ErrorBanner.tsx`:
- Around line 22-33: The close button in ErrorBanner (the button with
onClick={clearError}) has no accessible name; add an accessible label such as
aria-label="Close" or a more descriptive label (e.g., aria-label={`Dismiss
error: ${errorMessage}`}) to the button so screen readers get a meaningful
control name; alternatively include visually-hidden text inside the button for
accessibility while keeping the icon-only appearance.
In `@frontend/src/components/modals/CreationModal.tsx`:
- Around line 79-105: The submit handler (handleSubmit) needs an isSubmitting
flag to prevent duplicate requests: add a state variable (e.g., isSubmitting /
setIsSubmitting) and at the top of handleSubmit return early if isSubmitting is
true, then setIsSubmitting(true) just before calling await onSubmit({ tableName,
data: formData }) and ensure setIsSubmitting(false) in a finally block after
onSubmit/onReload/pipeCancel/setFormData to reset the flag on success or error;
update the submit button(s) (the buttons referenced around lines handling the
modal submit at the end of the file) to include disabled={isSubmitting || /*
existing disabled conditions */} so the UI prevents repeated clicks while the
create request is in flight.
- Around line 59-76: The uniqueness validation can race and apply stale results
in handleChange; introduce a ref (e.g., latestValidateValueRef) keyed by field
name to store the latest value/request id when starting validateUnique in
handleChange, then after await check that the stored value matches the current
value (or compare with formData[fieldName]) before calling setErrors or toggling
setIsValidating for that field; ensure you update the ref when new validations
start and only clear/set errors and setIsValidating when the check confirms the
result is for the latest value.
In `@frontend/src/components/ProtectedRoutes.tsx`:
- Around line 14-20: The useEffect in ProtectedRoutes currently calls
startLoading() and stopLoading() directly based on the auth `loading` boolean,
which can clear a global loader started by unrelated flows; update the logic to
use scoped or reference-counted loading operations instead of a single toggle:
when `loading` becomes true call a start method that accepts a unique key or
increments a counter (e.g., startLoading("ProtectedRoutes" or
incrementLoading)), and when `loading` becomes false call the matching stop
method that removes that key or decrements the counter (e.g.,
stopLoading("ProtectedRoutes" or decrementLoading)); modify the global loading
API and the ProtectedRoutes useEffect to use these keyed/incremental operations
so other concurrent loaders are not prematurely cleared.
In `@frontend/src/helpers/leagues/fetchLeaguesIndex.ts`:
- Around line 5-14: The current fetch in fetchLeaguesIndex only handles network
rejections and returns any HTTP error body as success; update the call that
assigns response (the fetch to `${BASE_URL}/categories/index?page=${pageNum}`)
to check response.ok after the await (or in a then) and treat non-OK responses
as errors: call throwError with a message including response.status and
statusText (or response.text()) and return null, so only successful 2xx
responses proceed to await response.json().
In `@frontend/src/helpers/leagues/fetchLeaguesSpecific.ts`:
- Around line 5-14: The current fetch call in fetchLeaguesSpecific.ts only
catches network errors but still calls response.json() for non-OK HTTP
responses; update the logic after the fetch (the response handling around the
response variable) to check response.ok and handle non-2xx statuses explicitly
(e.g., call throwError with a message containing response.status and
response.statusText and return null) before calling response.json(), so API
errors (404/500) don't get parsed as success.
In `@frontend/src/helpers/leagues/renameLeague.ts`:
- Around line 13-27: The renameLeague function is calling the wrong endpoint and
only handling network errors; change the fetch URL from
`/categories/update?id=${id}` to the controller-compatible
`/categories?id=${id}` and strengthen error handling: after the fetch (and still
catching network errors with the existing catch that calls throwError), check
response.ok and call throwError with status and statusText (and response text if
available) for non-2xx responses, then wrap the await response.json() in
try/catch to handle JSON parse errors and call throwError on parse failure;
reference the renameLeague function, the throwError helper, the fetch call,
response.ok check, and response.json() when making these changes.
In `@frontend/src/hooks/load/useResource.ts`:
- Around line 9-29: The load function is using stale values because fetchRef and
pageRef are never updated after mount; update the refs whenever inputs change
and keep load stable: add effects like useEffect(() => { fetchRef.current =
fetchFn }, [fetchFn]) and useEffect(() => { pageRef.current = pageNum },
[pageNum]) so fetchRef.current(pageRef.current) inside load uses the latest
values, leaving load memoized via useCallback as-is to keep the
DisplayResourceGeneric useEffect([load]) behavior intact.
In `@frontend/src/index.css`:
- Around line 1-3: Stylelint is flagging the Tailwind directives (`@tailwind`
base; `@tailwind` components; `@tailwind` utilities;) as unknown at-rules; fix by
updating Stylelint config to allow Tailwind at-rules—either extend a
Tailwind-aware config (e.g., add "stylelint-config-tailwindcss" to extends) or
set the scss rule to ignore these names (e.g., configure
"scss/at-rule-no-unknown": [true, { "ignoreAtRules":
["tailwind","apply","variants","responsive","screen"] }]); ensure the change is
applied so the `@tailwind` directives are no longer reported.
In `@frontend/src/pages/leagues/League.tsx`:
- Around line 19-24: Validate categoryId before calling startLoading, or wrap
the async fetch flow in a try/finally to guarantee stopLoading runs on every
exit; specifically, move or add the categoryId check (used with throwError) so
it executes before startLoading OR call stopLoading() immediately before each
early return, and after awaiting fetchLeaguesSpecific ensure leagueJson is not
null/undefined before using Object.hasOwn (e.g., check if (!leagueJson ||
!Object.hasOwn(leagueJson, 'user_id')) then call stopLoading() and
throwError('No league found')). In short: ensure startLoading/stopLoading are
paired on every path around the fetchLeagueSpecific call in League (functions:
startLoading, stopLoading, throwError, fetchLeaguesSpecific, setLeague).
In `@frontend/src/pages/leagues/Leagues.tsx`:
- Around line 39-42: The renameLeague handler always flips setReloadKey
regardless of API success; change renameLeague to await the result of
renameLeagueApi(contextMenuItem.id, name), store it in a variable, and only call
setReloadKey(k => k * -1) (or otherwise invalidate) when that result is truthy
(i.e., renameLeagueApi returned a non-null success value); keep the early return
when !contextMenuItem and ensure you reference the renameLeague function,
renameLeagueApi call, contextMenuItem, and setReloadKey when making the change.
In `@frontend/src/pages/test/ThemeTest.tsx`:
- Around line 9-15: The dark state is hardcoded to true causing a mismatch;
update the ThemeTest component to initialize dark from the DOM (useState(() =>
!document.documentElement.classList.contains('light'))) and add a useEffect that
watches for changes to document.documentElement.classList (MutationObserver) to
call setDark accordingly; keep the existing toggleTheme (setDark and
document.documentElement.classList.toggle('light')) but ensure the state and DOM
are synchronized on mount and when the html class changes (follow the pattern
used in Pokeball.tsx).
In `@frontend/src/styles/theme.css`:
- Line 6: Change the CSS property value for text-rendering from
"optimizeLegibility" to lowercase "optimizelegibility" so Stylelint passes;
locate the declaration using the text-rendering property in theme.css and
normalize the keyword to optimizelegibility.
---
Outside diff comments:
In `@frontend/src/helpers/manageResource/createNewResource.ts`:
- Around line 17-37: The createNewResource function currently swallows fetch
errors and always calls onReload; update createNewResource so it checks the
fetch response (use response.ok or parsed payload status) and throws or rethrows
on failure instead of just console.logging, and only call await onReload() after
a successful create. Locate the fetch block in createNewResource, ensure non-2xx
responses and caught exceptions are propagated (or return an error) so callers
know the create failed, and keep onReload invocation conditional on a successful
create.
In `@frontend/src/helpers/validation/checkUniqueness.ts`:
- Around line 6-13: The function that fetches
`${BASE_URL}/${tableName}/check-unique` currently returns raw JSON without
checking response status or verifying the shape; update the fetch handling in
checkUniqueness (use the existing response variable) to first verify
response.ok, parse the JSON, and validate that the parsed object contains the
expected boolean field (e.g., unique or isUnique) before returning; if
response.ok is false or the shape is invalid, throw or return a clear
error/false result so callers cannot treat an error object as unique.
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Line 37: The Test Theme route is currently always registered; wrap the Route
that renders ThemeTest (the JSX line Route path="/Test/Theme"
element={<ThemeTest />} inside App or the component that builds routes) in a
dev-only conditional so it only mounts in development builds (e.g., guard with
process.env.NODE_ENV === 'development' or your build's dev flag like
import.meta.env.DEV). Keep the same Route JSX but render it only when the dev
flag is true so production builds do not expose the /Test/Theme test page.
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Line 112: In ExtendableContextMenu.tsx inside the JSX rendering (the Submit
label currently written as the redundant ternary {isAnyValidating ? 'Submit' :
'Submit'}), replace the ternary expression with the plain string 'Submit' to
remove the unnecessary conditional and simplify the markup.
- Around line 92-98: The label and error text must be programmatically
associated with the input in ExtendableContextMenu.tsx: give each input a unique
id (e.g., derived from field.name or the index i), set the label's htmlFor to
that id, add aria-describedby on the input pointing to the error span id when
errors[i] exists, and set aria-invalid={!!errors[i]} so assistive tech announces
validation; update the span to have a matching id (e.g., `${id}-error`) and keep
handleChange/onChange usage as-is.
In `@frontend/src/components/load/GlobalLoader.tsx`:
- Line 16: Replace the hardcoded overlay color in the GlobalLoader component by
using the theme token: when computing the backgroundColor (the property
currently set to opaque ? 'var(--color-bg)' : 'rgba(0, 0, 0, 0.5)'), swap the
hardcoded rgba with 'var(--color-bg-overlay)' so the overlay uses the theme
token; update the conditional expression where backgroundColor is assigned in
GlobalLoader to use 'var(--color-bg-overlay)' for the non-opaque case.
In `@frontend/src/Main.tsx`:
- Around line 10-14: The module currently accesses window and document at
top-level (prefersDark using window.matchMedia and
document.documentElement.classList.add('light')), which breaks in non-browser
environments; wrap that logic in a runtime guard such as checking typeof window
!== 'undefined' && typeof document !== 'undefined' (and that window.matchMedia
exists) before evaluating prefersDark or mutating document, so the code only
runs in browsers and is safe for SSR/tests.
In `@frontend/src/providers/LoadingProvider.tsx`:
- Around line 13-17: The current useEffect in LoadingProvider defers showing the
loader with setTimeout(..., 0) which doesn't prevent flicker; change the timeout
to a small real threshold (e.g., 150ms) so the timer created when isLoading
becomes true calls setShowLoader(true) after that delay, while keeping the
existing cleanup that clears the timer; update references in the useEffect where
isLoading, setShowLoader and the timer are used to use the new delay constant
(120–200ms, e.g., 150ms).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: caf44dfa-bfef-4c6a-b21e-62af39d6e29f
⛔ Files ignored due to path filters (3)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/pokeballDark.svgis excluded by!**/*.svgfrontend/public/pokeballLight.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
frontend/package.jsonfrontend/postcss.config.jsfrontend/src/App.tsxfrontend/src/Main.tsxfrontend/src/components/Pokeball.tsxfrontend/src/components/ProtectedRoutes.tsxfrontend/src/components/api/DisplayResourceGeneric.tsxfrontend/src/components/context/ExtendableContextMenu.tsxfrontend/src/components/error/ErrorBanner.tsxfrontend/src/components/leagues/LeagueContextMenu.tsxfrontend/src/components/load/GlobalLoader.tsxfrontend/src/components/load/LoadResourceIndex.tsxfrontend/src/components/load/ShowLoader.tsxfrontend/src/components/modals/CreationModal.tsxfrontend/src/helpers/exports/exportError.tsfrontend/src/helpers/exports/exportLoading.tsfrontend/src/helpers/leagues/fetchLeaguesIndex.tsfrontend/src/helpers/leagues/fetchLeaguesSpecific.tsfrontend/src/helpers/leagues/renameLeague.tsfrontend/src/helpers/manageResource/createNewResource.tsfrontend/src/helpers/validation/checkUniqueness.tsfrontend/src/hooks/load/useResource.tsfrontend/src/index.cssfrontend/src/pages/leagues/League.tsxfrontend/src/pages/leagues/Leagues.tsxfrontend/src/pages/test/Test.tsxfrontend/src/pages/test/ThemeTest.tsxfrontend/src/providers/ErrorProvider.tsxfrontend/src/providers/LoadingProvider.tsxfrontend/src/redux/errorSlice.tsfrontend/src/redux/loadingSlice.tsfrontend/src/redux/store.tsfrontend/src/styles/theme.cssfrontend/tailwind.config.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/load/LoadResourceIndex.tsx
Casjb
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/pages/leagues/League.tsx (1)
27-27:⚠️ Potential issue | 🟠 MajorValidate
categoryIdas a strict integer before fetch.
parseInt(categoryId)can accept partial values (e.g."12abc"), which may fetch the wrong league ID. Reject invalid IDs explicitly before callingfetchLeaguesSpecific.Suggested fix
- const leagueJson = await fetchLeaguesSpecific(parseInt(categoryId)) + const id = Number(categoryId) + if (!Number.isInteger(id) || id <= 0) { + throwError('Invalid league id') + return + } + + const leagueJson = await fetchLeaguesSpecific(id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/leagues/League.tsx` at line 27, The code calls fetchLeaguesSpecific(parseInt(categoryId)) without strict validation, allowing inputs like "12abc"; update the League component to validate categoryId before calling fetchLeaguesSpecific by ensuring it matches a strict integer pattern (e.g. /^\d+$/) or by parsing with Number and verifying Number.isInteger and string equality to the original, then convert to an integer and call fetchLeaguesSpecific with that safe value; if validation fails, handle it explicitly (show error UI, return early, or redirect) so fetchLeaguesSpecific is only ever called with a proven integer id.frontend/src/components/context/ExtendableContextMenu.tsx (1)
69-81:⚠️ Potential issue | 🟠 MajorHandle submit rejections and guard re-entry inside
handleSubmit.
void handleSubmit()with no rejection handling can still produce unhandled promise rejections, and there’s no in-function guard against concurrent invocation.💡 Suggested fix
const handleSubmit = async () => { + if (submitting) return setSubmitting(true) try { - await Promise.all( + await Promise.all( fields.map((field, i) => field.type === 'input' && values[i] !== undefined ? Promise.resolve(field.onSubmit(values[i])) : Promise.resolve() ) ) + } catch { + // surface via app-level error handling if available } finally { setSubmitting(false) } }Also applies to: 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/context/ExtendableContextMenu.tsx` around lines 69 - 81, handleSubmit currently can be invoked concurrently and may produce unhandled rejections; add an in-function re-entry guard and proper rejection handling: at the top of handleSubmit check and return if submitting is true (use the submitting state), then setSubmitting(true), await Promise.all over fields.map calling field.onSubmit(values[i]) only when field.type === 'input' and values[i] !== undefined, wrap the await in try/catch to handle/log any errors from field.onSubmit, and finally setSubmitting(false) in a finally block; update both occurrences of handleSubmit (lines cited) to use this pattern.
🧹 Nitpick comments (2)
frontend/src/pages/leagues/League.tsx (1)
46-46: Set explicit button type for safer reuse.Add
type="button"to avoid accidental form submission if this markup is ever nested inside a<form>.Suggested fix
- <button onClick={() => navigate(-1)}>{'<- Leagues'}</button> + <button type="button" onClick={() => navigate(-1)}>{'<- Leagues'}</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/leagues/League.tsx` at line 46, The back-navigation button in the League component is missing an explicit type which can cause accidental form submission if this component is ever nested in a form; update the JSX button (the element with onClick={() => navigate(-1)} in the League component) to include type="button" so it is explicitly a non-submit button.frontend/src/components/context/ExtendableContextMenu.tsx (1)
133-133: Use a theme token for error color instead of hardcoded'red'.This keeps error styling consistent across light/dark themes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/context/ExtendableContextMenu.tsx` at line 133, The inline style in ExtendableContextMenu sets color: 'red' which hardcodes error color; replace it with the app's theme token instead (e.g. useTheme().colors.error or a CSS variable like 'var(--color-error)') by retrieving the theme via the project’s theme hook/utility and using that token for the style prop (or apply a className such as 'text-error'); update the element that currently has style={{ color: 'red', fontSize: '12px' }} to reference the theme token so error color respects light/dark themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Around line 46-66: When validation is skipped because the input is empty (the
condition is false for field.validate && val.length > 0), ensure you explicitly
clear the validating flag for that index so it can't remain true; update the
block around field.validate to add an else branch that calls setValidating(prev
=> ({ ...prev, [i]: false })) (optionally also clear errors via setErrors(prev
=> ({ ...prev, [i]: undefined })) or setValid if desired) referencing the same
index i and the existing state setters (setValidating, setErrors, setValid) and
latestValues.current logic.
In `@frontend/src/pages/leagues/League.tsx`:
- Around line 26-37: The loadLeague code calls fetchLeaguesSpecific but lacks a
catch, so thrown errors bypass the app's global UI error flow; wrap the fetch in
try/catch/finally (or add a catch after the existing try) and in the catch call
throwError(error) (or throwError(String(error)) for safe messaging) before
returning, keeping stopLoading() in the finally block; reference
fetchLeaguesSpecific, loadLeague (or the enclosing function), throwError,
setLeague and stopLoading to locate and update the code.
---
Duplicate comments:
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Around line 69-81: handleSubmit currently can be invoked concurrently and may
produce unhandled rejections; add an in-function re-entry guard and proper
rejection handling: at the top of handleSubmit check and return if submitting is
true (use the submitting state), then setSubmitting(true), await Promise.all
over fields.map calling field.onSubmit(values[i]) only when field.type ===
'input' and values[i] !== undefined, wrap the await in try/catch to handle/log
any errors from field.onSubmit, and finally setSubmitting(false) in a finally
block; update both occurrences of handleSubmit (lines cited) to use this
pattern.
In `@frontend/src/pages/leagues/League.tsx`:
- Line 27: The code calls fetchLeaguesSpecific(parseInt(categoryId)) without
strict validation, allowing inputs like "12abc"; update the League component to
validate categoryId before calling fetchLeaguesSpecific by ensuring it matches a
strict integer pattern (e.g. /^\d+$/) or by parsing with Number and verifying
Number.isInteger and string equality to the original, then convert to an integer
and call fetchLeaguesSpecific with that safe value; if validation fails, handle
it explicitly (show error UI, return early, or redirect) so fetchLeaguesSpecific
is only ever called with a proven integer id.
---
Nitpick comments:
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Line 133: The inline style in ExtendableContextMenu sets color: 'red' which
hardcodes error color; replace it with the app's theme token instead (e.g.
useTheme().colors.error or a CSS variable like 'var(--color-error)') by
retrieving the theme via the project’s theme hook/utility and using that token
for the style prop (or apply a className such as 'text-error'); update the
element that currently has style={{ color: 'red', fontSize: '12px' }} to
reference the theme token so error color respects light/dark themes.
In `@frontend/src/pages/leagues/League.tsx`:
- Line 46: The back-navigation button in the League component is missing an
explicit type which can cause accidental form submission if this component is
ever nested in a form; update the JSX button (the element with onClick={() =>
navigate(-1)} in the League component) to include type="button" so it is
explicitly a non-submit button.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c3324f3-143d-4aec-a380-c5500ef54a19
📒 Files selected for processing (11)
frontend/fixme.mdfrontend/src/App.tsxfrontend/src/Main.tsxfrontend/src/components/api/DisplayResourceGeneric.tsxfrontend/src/components/context/ExtendableContextMenu.tsxfrontend/src/components/load/GlobalLoader.tsxfrontend/src/helpers/manageResource/createNewResource.tsfrontend/src/helpers/validation/checkUniqueness.tsfrontend/src/pages/leagues/League.tsxfrontend/src/pages/leagues/Leagues.tsxfrontend/src/pages/test/ThemeTest.tsx
✅ Files skipped from review due to trivial changes (4)
- frontend/src/Main.tsx
- frontend/fixme.md
- frontend/src/components/load/GlobalLoader.tsx
- frontend/src/pages/test/ThemeTest.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/helpers/validation/checkUniqueness.ts
- frontend/src/App.tsx
- frontend/src/helpers/manageResource/createNewResource.ts
- frontend/src/components/api/DisplayResourceGeneric.tsx
- frontend/src/pages/leagues/Leagues.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/components/context/ExtendableContextMenu.tsx (1)
71-88: Consider surfacing submission errors to the user.When
onSubmitfails, the error is logged to console but no feedback is shown to the user. The menu remains open without any indication that submission failed.Additionally, since
Promise.allruns all field submissions in parallel, partial failures may occur (some succeed, some fail). Consider whether this behavior is acceptable for your use cases.💡 Optional: Add error state for submission failures
const [errors, setErrors] = useState<Record<number, string>>({}) const [submitting, setSubmitting] = useState(false) +const [submitError, setSubmitError] = useState<string | null>(null) // ... const handleSubmit = async () => { if (submitting) return setSubmitting(true) + setSubmitError(null) try { await Promise.all( fields.map((field, i) => field.type === 'input' && values[i] !== undefined ? Promise.resolve(field.onSubmit(values[i])) : Promise.resolve() ) ) } catch (error) { console.error(error) + setSubmitError('Submission failed. Please try again.') } finally { setSubmitting(false) } }Then render
submitErrornear the submit button.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/context/ExtendableContextMenu.tsx` around lines 71 - 88, handleSubmit currently swallows errors in console and leaves the menu ambiguous; add a submitError state (e.g., submitError and setSubmitError) and set it on catch so failures are surfaced to the UI, render that error text near the submit button in ExtendableContextMenu, and ensure setSubmitting(false) still runs; also consider replacing Promise.all with Promise.allSettled or sequential awaits inside handleSubmit (calling field.onSubmit for each input-type field in fields using values[i]) and treat any rejected result as a failure that sets submitError so the menu does not silently remain open on partial/total failure.frontend/src/pages/leagues/League.tsx (1)
13-13: Component name doesn't match file name or purpose.The file is
League.tsx(singular) and the docstring says "Individual league page," but the exported function is namedLeagues(plural). Consider renaming toLeaguefor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/leagues/League.tsx` at line 13, The exported component is named Leagues but the file and docstring indicate it represents a single league; rename the component/function from Leagues to League (and update any corresponding default export if explicitly named) to match the file purpose and docstring, and update any imports elsewhere that reference Leagues to import League instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/context/ExtendableContextMenu.tsx`:
- Around line 71-88: handleSubmit currently swallows errors in console and
leaves the menu ambiguous; add a submitError state (e.g., submitError and
setSubmitError) and set it on catch so failures are surfaced to the UI, render
that error text near the submit button in ExtendableContextMenu, and ensure
setSubmitting(false) still runs; also consider replacing Promise.all with
Promise.allSettled or sequential awaits inside handleSubmit (calling
field.onSubmit for each input-type field in fields using values[i]) and treat
any rejected result as a failure that sets submitError so the menu does not
silently remain open on partial/total failure.
In `@frontend/src/pages/leagues/League.tsx`:
- Line 13: The exported component is named Leagues but the file and docstring
indicate it represents a single league; rename the component/function from
Leagues to League (and update any corresponding default export if explicitly
named) to match the file purpose and docstring, and update any imports elsewhere
that reference Leagues to import League instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc6b6777-8bdb-4e19-bb54-441318f2d723
📒 Files selected for processing (2)
frontend/src/components/context/ExtendableContextMenu.tsxfrontend/src/pages/leagues/League.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation