Update all dependencies to latest versions and fix several linter issues #108
Conversation
…console: Using UNSAFE_componentWillMount in strict mode is not recommended and may indicate bugs in your code. See https://react.dev/link/unsafe-component-lifecycles for details. * Move code with side effects to componentDidMount, and set initial state in the constructor. Please update the following components: SideEffect(NullComponent2) ReactStrictModeWarnings.flushPendingUnsafeLifecycleWarnings @ react-dom-client.development.js:25805
…h a spinner while loading and to avoid errors in the console
…oColorSchemeType and updateUI functions outside the component.
…esn't depend on component state) and add sortIndex to the dependency array.
…nd add it to all the dependency arrays.
…ed parameter in DateConverter.ts + fix optional chain in ManifestJson.ts
…he dependency array
WalkthroughUpgrades project toolchain to Node 24 and pnpm/corepack, replaces ESLint/Prettier with Biome, bumps many dependencies (notably React 17→19 and react-router 6→7), converts numerous imports to type-only, tightens typings, adds fetch cancellation in Buckets, and updates CI/devcontainer and Vite build config. Changes
Sequence Diagram(s)%%{init: {"themeVariables": {"actorBorder":"#334155","signalColor":"#60A5FA","noteBg":"#F8FAFC","noteText":"#0F172A"}}}%%
sequenceDiagram
participant UI as User
participant Buck as Buckets component
participant Net as fetch()
Note over Buck: mount or criteria change triggers fetch
UI->>Buck: request fetch (criteria)
Buck->>Net: fetch(..., { signal: controller.signal })
alt unmount or new fetch occurs
UI->>Buck: new fetch / unmount
Buck->>Net: controller.abort()
Note right of Net: promise rejects with AbortError
end
Net-->>Buck: response (or error)
alt not aborted
Buck->>UI: setResults / setSearching(false)
else aborted
Note right of Buck: ignore results, no state updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Search.tsx (1)
144-146: Bug:setSortIndexcalled with sort direction value.Line 145 incorrectly calls
setSortIndexwith the result ofgetSortDirectionFromSearchParams(). This should besetSortDirection.if (getSortDirectionFromSearchParams(getSortIndexFromSearchParams()) !== sortDirection) { - setSortIndex(getSortDirectionFromSearchParams(getSortIndexFromSearchParams())); + setSortDirection(getSortDirectionFromSearchParams(getSortIndexFromSearchParams())); }src/components/SearchResult.tsx (1)
45-56: Apply HTML sanitization before rendering highlighted content to the DOM.Azure Cognitive Search returns highlighted fragments as HTML and does not automatically HTML-escape the original field content, so you must treat highlights as untrusted content and HTML-encode or sanitize before inserting into a page to avoid XSS.
The current implementation passes
highlightedName,highlightedDescription,highlightedLicense, andhighlightedVersiondirectly todisplayInnerHtml()→dangerouslySetInnerHTMLwith no sanitization applied. Additionally,formattedHighlightedRepository(src/components/SearchResult.tsx, line 116-117) uses.replace()on the highlighted content without escaping replacement strings, which could introduce XSS vectors.Actions:
- Install and use a sanitization library (e.g., DOMPurify) to sanitize all highlighted fields before rendering, or
- Ensure Azure Search content is HTML-encoded server-side before indexing, or
- Implement a safe rendering pattern (e.g., extract plain text, then apply styling with safe DOM methods).
🧹 Nitpick comments (14)
src/components/SearchBar.tsx (2)
1-76: Optional: use namedmemoimport instead of defaultReactThe default
Reactimport is only used forReact.memo. In React 17+ / 19 with the modern JSX transform, you can importmemodirectly and drop the default import for slightly cleaner and more tree‑shakable code.Example diff:
-import React, { type JSX, useCallback, useEffect, useRef } from 'react'; +import { memo, type JSX, useCallback, useEffect, useRef } from 'react'; @@ -export default React.memo(SearchBar); +export default memo(SearchBar);
17-55: Timer ref typing is Node‑specific; considerReturnType<typeof setTimeout>The debounced submit logic and the
clearDelayBeforeSubmitTimeoutcallback look correct and the cleanup is wired properly throughuseCallbackanduseEffect. The only concern is typing the ref asNodeJS.Timeout, which ties this component to Node typings; in browser‑focused React apps,setTimeoutcommonly returns anumber.Using
ReturnType<typeof setTimeout>keeps this compatible across environments (Node + DOM) without depending onNodeJSglobals.Suggested change:
- const delayBeforeSubmit = useRef<NodeJS.Timeout | undefined>(undefined); + const delayBeforeSubmit = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);This should be type‑safe for both Node 24 and the browser build as long as your TS lib settings include the appropriate timer definitions.
README.md (1)
19-37: Docs now match Node 24 + pnpm toolchainThe updated prerequisites and commands correctly reflect the move to Node 24 and pnpm (
pnpm install,pnpm run dev/build/preview, etc.), which should keep contributors aligned with the new setup.If you care about a clean markdownlint run, consider wrapping the localhost URLs in angle brackets (e.g.,
<http://localhost:3000>) to avoid MD034 “bare URL” warnings, but that’s purely cosmetic..devcontainer/devcontainer.json (1)
2-21: Devcontainer Node 24 / pnpm setup looks consistent; consider lockfile-strict installNode 24 image, Biome extension, and pnpm commands align with the rest of the PR. If you want devcontainers to strictly honor the lockfile, you could make the install step deterministic:
- "updateContentCommand": "corepack enable && pnpm install", + "updateContentCommand": "corepack enable && pnpm install --frozen-lockfile",Please confirm this matches how CI installs dependencies (npm/pnpm flags, lockfile usage).
src/serialization/SearchResultsJson.ts (1)
9-11: Cast to object is fine; initializer can be simplifiedThe
jsonObject as objectcast inCreateis a reasonable way to satisfydeserializeObjectwhile keeping the public APIunknown.For the property, you can drop the redundant assertion since the field is already typed:
- @JsonProperty('value', [ManifestJson]) - results: ManifestJson[] = [] as ManifestJson[]; + @JsonProperty('value', [ManifestJson]) + results: ManifestJson[] = [];Please ensure Biome/TS config doesn’t rely on the explicit
as ManifestJson[]before applying this.Also applies to: 16-18
vite.config.ts (1)
1-1: Visualizer always-on is okay; consider gating it by env flagImporting
execSyncfromnode:child_processand movingvisualizerinto the mainpluginsarray are fine. If you’d prefer to only generate stats on demand, you could gate it on an env var:- plugins: [ - react(), - visualizer({ - filename: 'build_stats/stats_treemap.html', - open: false, - gzipSize: true, - template: 'treemap', - }), - ], + plugins: [ + react(), + ...(process.env.ANALYZE === 'true' + ? [ + visualizer({ + filename: 'build_stats/stats_treemap.html', + open: false, + gzipSize: true, + template: 'treemap', + }), + ] + : []), + ],Verify this matches how you intend to run bundle analysis (local vs CI) before changing behavior.
Also applies to: 15-23
.github/workflows/build-deploy.yml (1)
12-12: pnpm + Node 24 workflow looks correct; consider locking installs for reproducibilityThe Node 24 and pnpm 10 setup with
setup-nodecaching is wired correctly, and the command switches topnpm run …are consistent. For tighter CI reproducibility you might want to usepnpm install --frozen-lockfile(or the equivalent pnpm CI command) so builds fail if the lockfile and package.json drift instead of silently updating.Also applies to: 24-37, 39-39, 42-42, 53-53
src/utils.ts (1)
1-3: Clarify or align theseparatorparameter usage
extractPathFromUrlcurrently does:url.split('/').slice(-2).join(separator);so
separatoronly affects the join, not the split. That’s slightly misleading given the parameter name and default. Consider either:
- Using
separatorconsistently:-export const extractPathFromUrl = (url: string, separator = '/'): string => { - return url.split('/').slice(-2).join(separator); -}; +export const extractPathFromUrl = (url: string, separator = '/'): string => { + return url.split(separator).slice(-2).join(separator); +};or
- Renaming the parameter (e.g.,
joinSeparator) if you intentionally always split on'/'.src/components/Home.tsx (3)
38-62: Consider extractingSyntaxHighlighteroutside the component.Defining
SyntaxHighlighterinsideHomerecreates the component identity on every render, which can cause unnecessary re-mounts and lose internal state. Since it only depends oncolorStyle, consider memoizing or extracting it.+const SyntaxHighlighter = React.memo(({ colorStyle, children, ...rest }: SyntaxHighlighterProps & { colorStyle: typeof darkStyle }) => { + const customStyle = { lineHeight: '1.4', fontSize: '0.9em', margin: '0' }; + return ( + <SyntaxHighlighterBase {...rest} style={colorStyle} customStyle={customStyle} codeTagProps={{ style: customStyle }}> + {children} + </SyntaxHighlighterBase> + ); +});Alternatively, wrap the current definition with
useMemokeyed oncolorStyle.
64-70: Minor: Unhandled clipboard errors are silently swallowed.The
.finally(() => {})prevents unhandled rejection warnings but also swallows any clipboard errors. Consider adding basic error handling if clipboard access fails.
76-95: Same concern:SyntaxHighlighterWithCopyBtnrecreated on every render.This inner component depends on
handleCopyToClipboardwhich is stable (viauseCallback), but the component identity still changes each render.src/components/Search.tsx (1)
141-155: Avoid setting state during render.These conditional state updates in the render body can cause extra re-renders. Consider moving them into a
useEffectthat depends onsearchParamsto synchronize state changes properly.src/colorscheme/ColorSchemeProvider.tsx (2)
11-21: Handle malformed string values intoColorSchemeTypeto avoidNaN
parseInt(value, 10)can returnNaNfor unexpected or corrupted values in localStorage, which then flows through as aColorSchemeTypeand is compared against the enum and passed toupdateUI. It will currently fall through as “not dark” but the enum invariant is broken.You can harden this by guarding explicitly:
const toColorSchemeType = (value: boolean | string | null): ColorSchemeType => { if (value === null) { return ColorSchemeType.Auto; } if (typeof value === 'boolean') { return value ? ColorSchemeType.Dark : ColorSchemeType.Light; } - return parseInt(value, 10) as ColorSchemeType; + const parsed = Number.parseInt(value, 10); + return Number.isNaN(parsed) ? ColorSchemeType.Auto : (parsed as ColorSchemeType); };
87-95: Add cleanup formatchMediaevent listener in layout effectThe
changelistener registered onbrowserColorSchemeMediaQueryListis never removed, so if this provider is ever unmounted/remounted you accumulate listeners.Consider capturing the handler and returning a cleanup function:
- useLayoutEffect(() => { - const browserColorSchemeMediaQueryList = window.matchMedia('(prefers-color-scheme: dark)'); - browserColorSchemeMediaQueryList.addEventListener('change', (e) => { - setBrowserColorSchemePreference(toColorSchemeType(e.matches)); - }); + useLayoutEffect(() => { + const browserColorSchemeMediaQueryList = window.matchMedia('(prefers-color-scheme: dark)'); + const handleChange = (e: MediaQueryListEvent) => { + setBrowserColorSchemePreference(toColorSchemeType(e.matches)); + }; + browserColorSchemeMediaQueryList.addEventListener('change', handleChange); setBrowserColorSchemePreference(toColorSchemeType(browserColorSchemeMediaQueryList.matches)); setUserColorSchemePreference(toColorSchemeType(localStorage.getItem(PREFERRED_COLOR_SCHEME_STORAGE))); - }, []); + return () => { + browserColorSchemeMediaQueryList.removeEventListener('change', handleChange); + }; + }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.devcontainer/devcontainer.json(1 hunks).eslintrc.json(0 hunks).github/workflows/build-deploy.yml(3 hunks).nvmrc(1 hunks).prettierrc.json(0 hunks)README.md(1 hunks)biome.json(1 hunks)package.json(2 hunks)src/App.css(5 hunks)src/App.tsx(1 hunks)src/colorscheme/ColorSchemeProvider.tsx(1 hunks)src/components/AsciinemaCasts.tsx(3 hunks)src/components/BucketTypeIcon.tsx(2 hunks)src/components/Buckets.tsx(4 hunks)src/components/CopyToClipboardButton.tsx(1 hunks)src/components/Footer.tsx(1 hunks)src/components/Home.tsx(7 hunks)src/components/NavBar.tsx(2 hunks)src/components/Root.tsx(1 hunks)src/components/Search.tsx(5 hunks)src/components/SearchBar.tsx(4 hunks)src/components/SearchPagination.tsx(3 hunks)src/components/SearchProcessor.tsx(3 hunks)src/components/SearchResult.tsx(4 hunks)src/components/SearchStatus.tsx(1 hunks)src/components/asciinema-customthemes.css(1 hunks)src/index.tsx(1 hunks)src/reportWebVitals.ts(1 hunks)src/request-idle-callback.ts(1 hunks)src/serialization/BucketsResultsJson.ts(1 hunks)src/serialization/DateConverter.ts(1 hunks)src/serialization/ManifestJson.ts(2 hunks)src/serialization/SearchResultsJson.ts(2 hunks)src/utils.ts(1 hunks)tsconfig.eslint.json(0 hunks)tsconfig.json(2 hunks)tsconfig.node.json(1 hunks)vite.config.ts(2 hunks)
💤 Files with no reviewable changes (3)
- .eslintrc.json
- .prettierrc.json
- tsconfig.eslint.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/Home.tsx (1)
src/colorscheme/ColorSchemeContext.ts (1)
ColorSchemeContext(19-19)
src/components/Buckets.tsx (1)
src/utils.ts (1)
extractPathFromUrl(1-3)
src/components/AsciinemaCasts.tsx (1)
src/types/asciinema-player.d.ts (1)
Player(4-6)
src/components/SearchResult.tsx (1)
src/utils.ts (1)
extractPathFromUrl(1-3)
🪛 ast-grep (0.40.0)
src/components/SearchResult.tsx
[warning] 49-49: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 markdownlint-cli2 (0.18.1)
README.md
27-27: Bare URL used
(MD034, no-bare-urls)
27-27: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (35)
src/components/asciinema-customthemes.css (1)
313-313: CSS syntax fix looks good.The stray closing brace has been removed and the final closing brace is now properly positioned. The file structure is valid and all theme selectors are correctly formatted.
src/request-idle-callback.ts (1)
4-4: Nice type safety improvement!Replacing
anywithIdleRequestCallbackproperly types the callback parameter and aligns with the PR's modernization goals. The callback invocation on line 8 correctly matches theIdleDeadlineinterface.biome.json (3)
17-42: Verify that permissive configuration aligns with PR objective.The PR objective states "Fix all linter errors and warnings," but the Biome configuration takes a permissive stance by disabling or downgrading several rules (
noExcessiveCognitiveComplexity,noArguments, accessibility checks) and setting important rules to"warn"rather than error level. This may allow violations to accumulate if CI does not fail on warnings.Clarify whether this configuration represents your team's intended standards or if it was a pragmatic workaround during the ESLint → Biome migration.
Consider also verifying:
- The exact Biome version in
package.jsonmatches schema 2.3.7.- Whether your CI/CD pipeline is configured to fail on warnings as well as errors.
11-16: Formatter configuration looks appropriate.The formatter settings (2-space indentation, 120-character line width, single quotes, always semicolons, ES5 trailing commas) are consistent with common web development practices and align well with the previous Prettier setup.
25-29: DisablingnoExcessiveCognitiveComplexityis intentional configuration during tool migration with no evidence of violations.The rule was deliberately disabled as part of the clean biome.json migration from eslint/prettier (commit 38395c7). Code inspection of the codebase (2086 LOC, 34 files) reveals no excessive cognitive complexity: nested conditionals are 2-3 levels deep (typical), switch statements are simple (2-3 cases), and the largest components (Search.tsx: 348 lines, SearchProcessor.tsx: 325 lines) show standard React component patterns. The original review assumes violations would surface if the rule were enabled, but no such violations are present in the codebase. This was a deliberate configuration choice during migration, not a workaround for existing problems.
Likely an incorrect or invalid review comment.
.nvmrc (1)
1-1: Node version pin looks goodPinning Node to major version 24 in
.nvmrcmatches the rest of the toolchain changes and should help keep local dev aligned with CI. Just ensure all workflows/devcontainers also target Node 24 to avoid drift.src/components/SearchStatus.tsx (1)
1-1: JSX type import is appropriate for React 19Using a type‑only
JSXimport and returningJSX.Elementkeeps the component well‑typed under the React 19 type model and doesn’t affect runtime behavior. The rest of the component is unchanged and remains sound.If you bump
@types/reactagain later, re‑run type‑check to confirm this import style (JSX.Element) still matches the recommended pattern for that version, as React’s JSX typing has evolved across 18.x → 19.x.src/serialization/ManifestJson.ts (1)
83-88: Optional chaining refactor preserves behaviorUpdating the predicate to
this.highlights?.[value]simplifies the null‑check while keeping the same semantics as the previousthis.highlights && this.highlights[value]. The final return still safely falls back when there’s no match orhighlightsis undefined.src/components/CopyToClipboardButton.tsx (1)
1-5: Import clean‑up is safeReordering the React hooks and icon imports is cosmetic and doesn’t affect the component’s behavior or public API. The button’s copy/feedback logic remains unchanged.
src/components/BucketTypeIcon.tsx (1)
1-5: Type import & formatting adjustments look correctBringing in a type‑only
JSXimport and returningJSX.Elementkeeps this component consistent with the React 19 typing pattern used elsewhere in the repo. The tooltip ternary is just re-indented; the actual conditions and text for official vs (popular) community buckets are unchanged.After bumping React/TS types, it’s worth a quick
pnpm run lint && pnpm run buildto confirm all JSX typings (including thisJSX.Elementusage) are fully compatible with the new versions.Also applies to: 29-33
tsconfig.json (1)
4-21: No Node globals detected in src/ — tsconfig changes are safeThe verification confirms the concern from the original review:
src/contains no direct usage of Node APIs (process,Buffer,__dirname,__filename,require), so the shift to"moduleResolution": "bundler"and removal of implicit Node typings will not cause unintended type errors.The tsconfig changes are consistent with a browser-only, Vite-based setup and are safe to proceed.
tsconfig.node.json (1)
2-8: The review comment is based on a false premise and should be disregarded.The
node:builtin specifier (e.g.,import fs from "node:fs") is a Node runtime convention — TypeScript will treat those as normal module specifiers and will resolve types via your ambient typings (e.g., @types/node or the Node builtin lib), not by any special bundler-only rule. ThemoduleResolutionsetting does not control hownode:protocol imports are handled at runtime; if your output runs under a bundler (Vite, Rollup, webpack), usemoduleResolution: "bundler".moduleResolution: "bundler" is recommended for modern bundlers (Vite/esbuild/Rollup) and is the standard configuration for tsconfig.node.json. The four bare imports in vite.config.ts—
node:child_process,@vitejs/plugin-react,rollup-plugin-visualizer, andvite—are all resolvable under bundler semantics. The change aligns with Vite best practices and poses no risk.Likely an incorrect or invalid review comment.
src/components/AsciinemaCasts.tsx (1)
1-1: Typed refs and stable Nav keys are a solid cleanupAnnotating the component with
JSX.Element, typingasciiPlayerRefasAsciinemaPlayer.Player | undefined, and switching Nav item keys from index toitem.keyall improve type safety and React reconciliation without changing behavior. No changes needed here.Also applies to: 25-26, 62-64
src/serialization/DateConverter.ts (1)
1-1: Type-only JsonCustomConvert import and_daterename look goodUsing a type-only import for
JsonCustomConvertand renaming the unuseddateparameter to_dateis a tidy way to satisfy typings and linters without changing runtime behavior (still “deserialize-only”). No further changes needed.Also applies to: 5-7
src/components/SearchProcessor.tsx (1)
1-7: Search result handling and UI tweaks are consistent with the new types
- Importing
SearchResultsJsonup front and usingSearchResultsJson.Create(data)keeps the fetch pipeline strongly typed and matches theonResultsChange(value?: SearchResultsJson)contract (includingundefinedon failures/abort).- Swapping to
FaCogand factoringSortIconoverIconBasePropsis a straightforward UI/typing improvement.- Marking the
Form.Switch.Inputelements asreadOnlywhile toggling via the surroundingDropdown.Itemclick handlers is a reasonable pattern to keep them controlled without React warnings.I don’t see functional regressions here.
Also applies to: 148-237, 281-314
src/components/SearchPagination.tsx (1)
1-1: Null return path and JSX typing are appropriate hereTyping
SearchPaginationasJSX.Element | nulland returningnullwhen there’s nothing to paginate is idiomatic React and slightly cheaper than rendering an empty fragment. The rest of the pagination logic is unchanged.Also applies to: 14-15, 73-74
package.json (1)
3-3: Pipeline fails at linting—fix formatting incoderabbit.markdownlint-cli2.jsoncbefore proceedingThe verification confirmed dependencies install correctly with pnpm v10.23.0 and the tooling stack is compatible. However, the lint check failed due to formatting errors in
coderabbit.markdownlint-cli2.jsonc. Biome requires:
- Collapse lines 2–6 from multi-line to single-line format
- Add a trailing newline at end of file
{ - "outputFormatters": [ - [ - "markdownlint-cli2-formatter-json" - ] - ], + "outputFormatters": [["markdownlint-cli2-formatter-json"]], "config": { "default": true, ... } + }Additionally, the CI uses pnpm v10.23.0 (review suggested v9; either version appears functional, but align if consistency is required).
Likely an incorrect or invalid review comment.
src/components/Root.tsx (1)
1-1: Typed JSX return and imports are clean and consistentUsing a type-only JSX import and annotating
Root’s return type aligns with the rest of the TS/React codebase and doesn’t alter runtime behavior of the provider tree.Also applies to: 5-5, 9-21
src/index.tsx (1)
1-1: createRoot + HelmetProvider integration matches modern React patternsThe explicit root element check,
createRoot(container), and wrappingAppwithReact.StrictModeandHelmetProvideris the expected setup for React 18/19 and the@dr.pogodin/react-helmetAPI. This should be a drop-in replacement for the previousReactDOM.render+react-helmetcombo.Please double-check against the versions of
react-domand@dr.pogodin/react-helmetinpackage.jsonto ensure this is the recommended initialization signature for those exact versions.Also applies to: 3-3, 8-18
src/components/NavBar.tsx (1)
1-1: Stricter color-scheme comparisons and JSX typing look goodSwitching to
===forColorSchemeTypecomparisons and adding an explicitJSX.Elementreturn type make the component safer and clearer without changing behavior.Also applies to: 3-4, 37-38, 43-44, 49-50
src/serialization/BucketsResultsJson.ts (1)
1-4: Type-only facet import and deserializeObject typing are reasonableImporting
BucketsResultsFacetJsonas a type-only symbol and constrainingjsonObjecttoobjectfordeserializeObjectkeep runtime behavior the same while tightening TS types. Theresultsdictionary type remains clear and keyed correctly by facet name.If you run into typing friction here, it may be worth confirming the current
json2typescriptJsonConvert.deserializeObjectsignature matches thisobjectconstraint for your installed version.Also applies to: 9-11, 16-18
src/components/Footer.tsx (1)
1-1: Footer icon updates and commit link are solid; confirm APP_COMMIT_HASH wiringSwitching to
FaGithub/FaRegHandshakeand adding the{APP_VERSION}/{APP_COMMIT_HASH}link improves clarity and traceability, and the JSX typing is consistent with the rest of the codebase. Just ensureAPP_COMMIT_HASHis defined in your build-time globals and included in the relevant TypeScript declaration file so this doesn’t rely on an implicitanyor cause runtimeReferenceErrors.Also applies to: 3-4, 12-23
src/App.tsx (1)
1-1: App’s JSX typing and Helmet migration align with the new entry setupAnnotating
Appas returningJSX.Elementand switchingHelmetto@dr.pogodin/react-helmetare consistent with the updatedHelmetProviderusage and React 19 toolchain; the router structure is unchanged.Confirm that the
defaultTitleandtitleTemplateprops used here match the current@dr.pogodin/react-helmetAPI for the version declared inpackage.json.Also applies to: 4-5, 12-27
src/App.css (1)
1-151: LGTM!The CSS changes are formatting normalizations (multi-selector line breaks,
line-height: 2.0→2,rgba()spacing) consistent with the Biome migration. No semantic style changes.src/components/Home.tsx (1)
1-14: LGTM!Import reorganization aligns with React 19 best practices: explicit
type JSXimport, type-only imports forSyntaxHighlighterPropsandAsciinemaCastItem, and proper ColorSchemeContext usage.src/components/Search.tsx (3)
1-10: LGTM!Import updates are correct:
@dr.pogodin/react-helmetfor React 19 compatibility, explicittype JSXimport, andtype SortDirectionimport.
46-46: Good: Explicit radix forparseInt.Adding the radix
10prevents unexpected behavior with leading zeros.
203-203: LGTM!Simplified cleanup effect is correct and ensures the idle callback is cancelled on unmount.
src/components/SearchResult.tsx (3)
4-11: LGTM!Import reorganization follows React 19 patterns with explicit type imports and the new
extractPathFromUrlutility aligns with the codebase-wide refactor.
26-26: LGTM!The
RefObject<HTMLDivElement | null>type is the correct typing for React 19, where all refs are mutable and includenullin the type.
124-129: LGTM!The
extractPathFromUrlusage and template literal forappCommandare clean and consistent with the utility's signature insrc/utils.ts.src/components/Buckets.tsx (3)
1-9: LGTM!Import updates are correct:
@dr.pogodin/react-helmet, explicittype JSX, andextractPathFromUrlfrom the new utility location.
64-139: Good: Proper AbortController pattern.The per-effect AbortController creation, abort-on-cleanup, and
AbortErrorhandling are well implemented for React 19's stricter Strict Mode behavior.
186-186: LGTM!Consistent usage of
extractPathFromUrlmatching the pattern inSearchResult.tsx.src/reportWebVitals.ts (1)
1-11: Web‑vitals integration looks consistent with the newer APIThe type-only import and the use of
onCLS,onINP,onFCP,onLCP, andonTTFBall wired to the same(metric: Metric) => voidcallback are coherent and keep the dynamic import lazy.Please double-check against the exact
web-vitalsversion you installed that:
Metricis the correct metric type, and- the functions
onCLS,onINP,onFCP,onLCP, andonTTFBall accept a(metric: Metric) => voidcallback.
If you like, you can confirm via the officialweb-vitalsdocumentation or its type definitions innode_modules/web-vitals.
| "a11y": { | ||
| "recommended": true, | ||
| "useKeyWithClickEvents": "off", | ||
| "useKeyWithMouseEvents": "off", | ||
| "noStaticElementInteractions": "off" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify accessibility rule disablement.
Three accessibility rules are explicitly disabled: useKeyWithClickEvents, useKeyWithMouseEvents, and noStaticElementInteractions. For a web project, disabling these rules is a significant regression in accessibility compliance. Confirm this is intentional and not a workaround to silence violations during migration.
If this was disabled due to existing violations in the codebase, consider re-enabling these rules and addressing the violations to maintain or improve accessibility posture.
🏁 Script executed:
#!/bin/bash
# Examine full biome.json to understand configuration context
echo "=== Full biome.json content ==="
find . -name "biome.json" -type f | head -1 | xargs cat
echo ""
echo "=== package.json (Biome version check) ==="
if [ -f "package.json" ]; then
jq '.devDependencies.biome // .dependencies.biome // "not found"' package.json
else
echo "package.json not found"
fiLength of output: 1183
Address disabled accessibility rules in biome.json.
Three accessibility rules are explicitly disabled (lines 38-40): useKeyWithClickEvents, useKeyWithMouseEvents, and noStaticElementInteractions. This directly contradicts the "recommended": true setting within the a11y section and represents a regression for a web project. Additionally, the cognitive complexity rule is disabled (noExcessiveCognitiveComplexity: "off" at line 26), and several other rules are downgraded to "warn" level.
Either document the business/technical justification for these exemptions, or re-enable these rules and address the underlying violations to maintain accessibility posture.
🤖 Prompt for AI Agents
In biome.json around lines 36 to 41 (and note the related cognitive complexity
rule at line 26 and other downgraded rules), the accessibility rules
useKeyWithClickEvents, useKeyWithMouseEvents, and noStaticElementInteractions
are explicitly set to "off" which contradicts "a11y.recommended": true; either
re-enable these rules (set them back to "error" or "warn" as per project policy)
and fix the underlying violations in the codebase, or if there is a justified
exception, add a brief documented justification in the config (or link to an
issue/PR) explaining why each rule is disabled and include an owner and a
remediation plan/timeframe; ensure the biome.json comment or adjacent README
notes the decision and align the cognitive complexity and other downgraded rules
consistently with project standards.
…th React's rendering cycle.
gpailler
force-pushed
the
update-latest-dependencies
branch
from
4c711ea to
2654bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Buckets.tsx (1)
64-66: Unused ref assignment.
abortControllerRef.currentis assigned on line 66 but never read elsewhere in the component. The localabortControllervariable is sufficient for the cleanup function closure. Consider removing the ref if it's not needed for external access.const Buckets = (): JSX.Element => { - const abortControllerRef = useRef<AbortController | null>(null); const [searching, setSearching] = useState<boolean>(false); const [results, setResults] = useState<Bucket[]>([]); // ... useEffect(() => { - // Create a new AbortController for this effect const abortController = new AbortController(); - abortControllerRef.current = abortController;src/components/Search.tsx (1)
141-171: Consider consolidating parameter synchronization.The separate
useEffectblocks for parameter synchronization work correctly, but the bidirectional pattern (including current state in dependencies to conditionally update) adds complexity. Each effect re-runs whenever its state changes, then conditionally updates if search params differ.While functionally correct, this could be simplified by either:
- Removing state from dependencies and relying solely on search param changes
- Using a single effect to synchronize all parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.devcontainer/devcontainer.json(1 hunks).github/workflows/build-deploy.yml(3 hunks)src/components/Buckets.tsx(4 hunks)src/components/Search.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/devcontainer.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Buckets.tsx (1)
src/utils.ts (1)
extractPathFromUrl(1-3)
🔇 Additional comments (10)
src/components/Buckets.tsx (5)
1-9: LGTM!The imports are properly updated for React 19 compatibility. Using
type JSXimport is the correct modern approach, and extractingextractPathFromUrlto a shared utility improves reusability.
19-51: LGTM!The array mutation issue has been properly addressed by creating a shallow copy before sorting. The sort logic is correct for all three modes, and the default case appropriately throws an error for unexpected sort modes.
53-61: LGTM!The
abortControllerRefis properly typed and the functional update pattern inhandleSortChangecorrectly avoids stale closure issues.
120-136: LGTM!The abort handling is well-implemented. Checking
abortController.signal.abortedbefore state updates prevents React warnings about updating unmounted components, and properly ignoringAbortErrorin the catch block is the correct pattern.
185-185: LGTM!Good refactoring to extract
extractPathFromUrlinto a shared utility. This improves reusability and testability..github/workflows/build-deploy.yml (1)
12-12: ✅ All verification checks passed—the migration to pnpm and Node 24 is correctly implemented.The review comment is accurate. Key confirmations:
- pnpm v10 officially supports Node.js 24 per pnpm documentation
- All npm commands have been replaced with pnpm equivalents throughout the workflow
- pnpm installation step correctly uses
pnpm/action-setup@v4with version 10 before Node setuppnpm install --frozen-lockfileis the proper CI/CD equivalent ofnpm ci- Cache strategy correctly configured to
pnpm- No other workflows exist that need updating
src/components/Search.tsx (4)
1-11: LGTM: Type-only imports improve tree-shaking.The migration to type-only imports (lines 2, 6, 7, 10) follows TypeScript best practices and helps with bundle optimization.
46-46: LGTM: Explicit radix prevents edge cases.Adding radix
10toparseIntprevents potential octal interpretation issues and follows best practices.Also applies to: 55-55
219-219: LGTM: Correct cleanup effect pattern.Removing
idleCallbackIdfrom dependencies is correct since refs don't need to be tracked. The cleanup function will properly cancel the callback on unmount.
317-317: LGTM: Strict equality prevents type coercion.Changing to
===improves type safety and follows modern JavaScript best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Buckets.tsx (2)
19-51:sortResultsnow preserves state immutability; minor optional enhancementCloning before sorting correctly avoids mutating React state, and the three sort branches match the
sortModesoptions with sensible ordering (official first, then name; name; manifests desc). As a small nicety, you could include the unexpected value in the error to aid debugging:- default: - throw new Error('Unexpected sort mode'); + default: + throw new Error(`Unexpected sort mode: ${sortOrder}`);
62-65: AbortController-based fetch lifecycle is solid; consider response checks and parallel fetchesThe per‑effect
AbortController, passingsignalintofetch, ignoringAbortError, and guardingsetResults/setSearchingon!signal.abortedare all correct and avoid setState‑after‑unmount issues.Two small robustness/perf tweaks you might consider:
- Check
response.okbeforeresponse.json()to surface HTTP failures more clearly:const response = await fetch(url, { method: 'POST', body: JSON.stringify({ @@ }, signal: abortSignal, }); - const data = (await response.json()) as unknown; + if (!response.ok) { + throw new Error( + `Azure Search request failed with ${response.status} ${response.statusText}`, + ); + } + const data = (await response.json()) as unknown;
- Fetch official and community buckets in parallel to reduce latency:
- const fetchAsync = async (abortSignal: AbortSignal): Promise<Bucket[]> => { - const officialBuckets = await fetchDataAsync(abortSignal, true); - const communityBuckets = await fetchDataAsync(abortSignal, false); + const fetchAsync = async (abortSignal: AbortSignal): Promise<Bucket[]> => { + const [officialBuckets, communityBuckets] = await Promise.all([ + fetchDataAsync(abortSignal, true), + fetchDataAsync(abortSignal, false), + ]);These are optional but would make the flow a bit more resilient and responsive.
Also applies to: 103-137
vite.config.ts (2)
15-23: Visualizer plugin placement looks good; consider gating it if builds get noisyUsing
visualizer({...})in the top‑levelpluginsarray is a standard Vite pattern and the options (filename,open: false,gzipSize,template: 'treemap') look correct for rollup-plugin-visualizer 6.x. If you only occasionally need bundle stats, you might optionally gate this plugin behind a mode/env flag (e.g., only enable whenANALYZE=true) so normal builds don’t always generate the treemap file.
35-51: manualChunks vendor grouping is reasonable; verify regex scope and path normalizationThe
manualChunkslogic to groupreact-bootstrap/popperintobootstrap-vendor, React‑related packages intoreact-vendor, andasciinema-playerintoasciinema-vendoris sound and should help produce more predictable vendor bundles. Two minor points to keep in mind:
- The
/\/(react)/regex will catch any dependency path containingreact(e.g.,@vitejs/plugin-react-swc, various React ecosystem packages), soreact-vendormay grow quite large; confirm this granularity matches your intended caching/splitting strategy.- Vite/Rollup usually normalize IDs to use
/, but if you ever see Windows-specific\paths leaking intomanualChunks, you may want to normalizeidfirst (e.g.,const normalized = id.replace(/\\\\/g, '/');) before running the regex checks.Functionally this is fine as‑is; these are just things to verify against real build outputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Buckets.tsx(4 hunks)vite.config.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Buckets.tsx (1)
src/utils.ts (1)
extractPathFromUrl(1-3)
🔇 Additional comments (2)
src/components/Buckets.tsx (1)
1-3: Imports, Helmet, and sharedextractPathFromUrlusage look goodType‑only JSX import, switch to
@dr.pogodin/react-helmet, and reusing the sharedextractPathFromUrlhelper in the table cell all look consistent and type‑safe. No changes requested here.Also applies to: 6-7, 183-183
vite.config.ts (1)
1-1: Node built‑in import style is appropriate for Node 24Switching to
'node:child_process'is the modern, recommended style for Node 20+/24 and should behave the same as the old'child_process'specifier in this context. Just ensure all environments that run this config (CI, dev containers, any tooling) are indeed on a Node version that supportsnode:specifiers.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.