Conversation
…percentage above bar. bold distribution "Value of..." header
…the size properly
Provider view
Implement breadcrumbs
Fix errors
reduce card opacity when loading
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@CodeRabbit please try again |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request restructures the application from client-ID-based routing to resource-based routing, consolidates data fetching into a unified module, updates UI components to use new base-ui primitives, and introduces new styling with typography font changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ 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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ActionButtons.tsx (1)
209-234:⚠️ Potential issue | 🟠 MajorFix the Commons URL query field format from underscore to hyphen.
The URL at line 224 constructs
${resource.type}_id:${resource.id}(e.g.,consortium_id:123), but DataCite Commons expects the field name with a hyphen:${resource.type}-id:${resource.id}(e.g.,consortium-id:123). Update the query parameter to use hyphens to match the DataCite API field naming convention that Commons recognizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ActionButtons.tsx` around lines 209 - 234, In ViewInCommons, the Commons query uses an underscore field name (`${resource.type}_id:${resource.id}`) but DataCite Commons expects a hyphenated field (e.g., `consortium-id:123`); update the doi.org branch that builds href so the query uses `${resource.type}-id:${resource.id}` (adjust the string interpolation where href is constructed) while keeping the rest of the URL/params intact.
🟡 Minor comments (12)
src/components/ui/combobox.tsx-281-293 (1)
281-293:⚠️ Potential issue | 🟡 Minor
childrendestructured but never rendered inComboboxChipsInput.
childrenis extracted from props on Line 283 but is not used in the returned JSX (Lines 287-291). This silently drops any children passed by consumers. If children are intentionally unsupported, remove the destructuring so they flow through...props; if they should be rendered, add them to the JSX.Fix: remove unused destructuring
function ComboboxChipsInput({ className, - children, ...props }: ComboboxPrimitive.Input.Props) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/combobox.tsx` around lines 281 - 293, The ComboboxChipsInput function destructures children but never uses them, which drops any passed children; fix by removing children from the parameter destructuring so children remain in ...props and flow through to <ComboboxPrimitive.Input>, i.e. change the function signature to accept ({ className, ...props }: ComboboxPrimitive.Input.Props) and leave the returned JSX for ComboboxPrimitive.Input unchanged; alternatively, if children should be rendered, include {children} inside the returned JSX—refer to the ComboboxChipsInput function and the <ComboboxPrimitive.Input> call to implement the chosen fix.src/app/globals.css-39-44 (1)
39-44:⚠️ Potential issue | 🟡 MinorRemove unused
--color-primary-light-blueand--color-primary-dark-bluetokens.Lines 39-40 define
--color-primary-light-blue(#00b1e2) and--color-primary-dark-blue(#243b54), but these tokens are not referenced anywhere in the codebase. The active replacements—--color-datacite-blue-lightand--color-datacite-blue-darkon lines 41-42—are used insrc/constants.tsandsrc/components/Badges.tsx. Remove the unusedprimary-*tokens to eliminate dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 39 - 44, Remove the two unused CSS custom properties --color-primary-light-blue and --color-primary-dark-blue from src/app/globals.css; they are dead code and should be deleted so only the active tokens --color-datacite-blue-light and --color-datacite-blue-dark remain (these are referenced by src/constants.ts and src/components/Badges.tsx). Verify no other files reference --color-primary-light-blue or --color-primary-dark-blue and run a quick repo-wide search to confirm before committing.src/app/[[...id]]/Header.tsx-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor
useIdshadows React 19's built-inuseId.React 19 exports its own
useIdhook (for stable accessibility IDs). The localuseIdfrom@/hooksextracts URL params — a completely different contract. Developers relying on auto-imports or muscle memory could silently import the wrong hook. Consider renaming to something unambiguous, e.g.useResourceIdoruseSlugId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[[...id]]/Header.tsx at line 5, The local hook useId from "@/hooks" conflicts with React 19's built-in useId; rename the local hook import to an unambiguous name (e.g., useResourceId or useSlugId) and update all usages in Header.tsx (and any other files) to that new identifier to avoid accidental auto-import collisions with React's useId; optionally, update the original export in the source hooks module to export the hook under the new name (or add a named alias) so callers can import the clarified name consistently.src/app/layout.tsx-29-31 (1)
29-31:⚠️ Potential issue | 🟡 MinorHardcoded Usersnap project key — consider using an environment variable.
The key
b4393e90-ec13-4338-b299-7b6f122b7de3is embedded directly in source. While it's a public client-side key, externalising it to an env var (e.g.NEXT_PUBLIC_USERSNAP_KEY) makes it easier to swap keys across environments (staging vs. production) without a code change and avoids leaking the production key into branches/forks.♻️ Proposed refactor
- <Script id="feedback-button"> - {`(function(){window.onUsersnapCXLoad=function(e){e.init()};var e=document.createElement("script");e.defer=1,e.src="https://widget.usersnap.com/global/load/b4393e90-ec13-4338-b299-7b6f122b7de3?onload=onUsersnapCXLoad",document.getElementsByTagName("head")[0].appendChild(e)})();`} - </Script> + {process.env.NEXT_PUBLIC_USERSNAP_KEY && ( + <Script id="feedback-button"> + {`(function(){window.onUsersnapCXLoad=function(e){e.init()};var e=document.createElement("script");e.defer=1,e.src="https://widget.usersnap.com/global/load/${process.env.NEXT_PUBLIC_USERSNAP_KEY}?onload=onUsersnapCXLoad",document.getElementsByTagName("head")[0].appendChild(e)})();`} + </Script> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/layout.tsx` around lines 29 - 31, The inline Usersnap script in the Script element (id="feedback-button") hardcodes the project key; update it to read from an environment variable (e.g. NEXT_PUBLIC_USERSNAP_KEY via process.env.NEXT_PUBLIC_USERSNAP_KEY) and interpolate that value into the script string used in layout.tsx instead of the literal GUID, adding a simple guard or fallback behavior if the env var is missing so the page doesn't break; locate the Script with id="feedback-button" and replace the hardcoded key with the env variable reference.src/components/ui/card.tsx-45-50 (1)
45-50:⚠️ Potential issue | 🟡 MinorReplace
text-mdwithtext-base— it's not a standard or custom Tailwind utility.Tailwind's default font-size scale goes
text-xs,text-sm,text-base,text-lg, etc., with notext-md. The globals.css file uses Tailwind CSS v4's@themesyntax and defines custom colors, spacing, and shadows but no font-size tokens. Sincetext-mdgenerates no CSS,text-smon the preceding line silently wins, which is likely unintended.Fix
className={cn( "text-muted-foreground text-sm", - "font-normal text-md", + "font-normal text-base", barlow.className, className, )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 45 - 50, In the Card component's className composition (the cn(...) call) replace the non-existent Tailwind utility "text-md" with the correct "text-base" so the intended font-size wins over "text-sm"; update the string literal inside the cn call (in src/components/ui/card.tsx where className is composed) to use "text-base" alongside "text-muted-foreground text-sm", Barlow's class, and the passed-in className.src/components/Badges.tsx-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorInvalid Tailwind classes:
p-y-0andp-x-1should bepy-0andpx-1.Tailwind CSS uses
py-0/px-1for vertical/horizontal padding, notp-y-0/p-x-1. These classes will be silently ignored, resulting in no padding being applied.🐛 Proposed fix
const DEFAULT_CLASS = - "rounded-[40px] text-[0.8em] text-datacite-blue-dark bg-datacite-blue-light/20 p-y-0 p-x-1 border-none"; + "rounded-[40px] text-[0.8em] text-datacite-blue-dark bg-datacite-blue-light/20 py-0 px-1 border-none";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Badges.tsx` around lines 4 - 5, The DEFAULT_CLASS string in Badges.tsx uses invalid Tailwind tokens `p-y-0` and `p-x-1`; update the class constant DEFAULT_CLASS to replace those with the correct Tailwind utilities `py-0` and `px-1` so vertical and horizontal padding are applied (keep the rest of the class string intact).src/app/[[...id]]/page.tsx-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorRemove debug
console.log.This logs the full search params object on every page load and appears to be a leftover debug statement.
Proposed fix
- console.log(await searchParams); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[[...id]]/page.tsx at line 15, Remove the debug console.log statement that prints the search params in the page component: delete the line containing "console.log(await searchParams)" from src/app/[[...id]]/page.tsx (or replace with appropriate analytics/logging if intentional), ensuring no leftover debug prints remain in the page rendering logic.src/components/Breadcrumbs.tsx-38-38 (1)
38-38:⚠️ Potential issue | 🟡 MinorRemove unused
BreadcrumbDatatype export.
BreadcrumbDatais not imported or referenced anywhere in the codebase. SinceBreadcrumbsno longer accepts apagesprop, this exported type is dead code and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Breadcrumbs.tsx` at line 38, Remove the unused exported type BreadcrumbData from the Breadcrumbs component file: locate the exported type declaration "export type BreadcrumbData = { title: string; href?: string }" and delete it (since Breadcrumbs no longer accepts a pages prop and the type is not referenced anywhere), then run a quick TypeScript build or search to confirm no remaining references to BreadcrumbData exist.src/app/[[...id]]/page.tsx-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorThrow an
Errorobject instead of a string literal.
throw "Incorrect ID format"throws a raw string. Error boundaries and logging tools expectErrorinstances (with stack traces). Usethrow new Error("Incorrect ID format")or callnotFound()if a malformed slug should yield a 404.Proposed fix
- if (slug && slug.length > 1) throw "Incorrect ID format"; + if (slug && slug.length > 1) notFound();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[[...id]]/page.tsx at line 12, The current slug validation in page.tsx uses a string literal in the throw (if (slug && slug.length > 1) throw "Incorrect ID format";), which should be replaced with an Error object for proper stack traces and tooling; update that check to either throw new Error("Incorrect ID format") or, if a malformed slug should return a 404, call Next.js's notFound() instead, keeping the same condition that inspects slug and slug.length.src/components/cards/Cards.tsx-554-563 (1)
554-563:⚠️ Potential issue | 🟡 MinorTypo: "Fomat documentation" → "Format documentation".
Line 559:
text="Fomat documentation"is missing the 'r'.Proposed fix
<LearnMore - text="Fomat documentation" + text="Format documentation" href="https://datacite-metadata-schema.readthedocs.io/en/4/properties/format/" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/cards/Cards.tsx` around lines 554 - 563, The string in the FormatsDescription component has a typo: update the LearnMore component's text prop from "Fomat documentation" to "Format documentation" inside the FormatsDescription JSX (referencing the FormatsDescription constant and the LearnMore component/text prop) so the displayed label is correct.src/components/cards/Cards.tsx-191-200 (1)
191-200:⚠️ Potential issue | 🟡 MinorCopy-paste error: Publisher description says "funding sources" instead of "publishers".
Line 193 reads "The Publisher property connects resources to funding sources" — this is copied from the FundingReferences description. It should describe the Publisher property.
Proposed fix
const PublisherDescription = ( <> - The Publisher property connects resources to funding sources. Add + The Publisher property identifies the entity responsible for making the resource available. Add publisherIdentifiers with ROR IDs for the highest impact.{" "}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/cards/Cards.tsx` around lines 191 - 200, The PublisherDescription JSX constant contains incorrect copy ("connects resources to funding sources"); update the text in the PublisherDescription constant so it correctly refers to publishers (e.g., "The Publisher property connects resources to publishers. Add publisherIdentifiers with ROR IDs for the highest impact.") and keep the existing LearnMore link and structure intact; edit the PublisherDescription definition to replace the erroneous "funding sources" wording with "publishers".src/data/fetch.ts-58-58 (1)
58-58:⚠️ Potential issue | 🟡 MinorRemove
console.info(resource)— debug logging left in production code.This logs the entire resource object on every fetch, which leaks internal data structures to the browser console and adds noise.
Proposed fix
- console.info(resource); return resource;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data/fetch.ts` at line 58, Remove the stray debug logging call console.info(resource); — delete that statement or replace it with a controlled debug log (e.g., use a logger.debug or wrap in a NODE_ENV check) so the full resource object is not printed in production; locate the console.info(resource) invocation and either remove it or guard it with a conditional (process.env.NODE_ENV !== 'production') or use the project's logger at debug level.
🧹 Nitpick comments (27)
src/components/ui/item.tsx (4)
43-43: Trailing space indefaultsize value
"p-4 gap-4 "has a stray trailing space.cn/clsxwill handle it silently, but it's worth cleaning up.🧹 Proposed fix
- default: "p-4 gap-4 ", + default: "p-4 gap-4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/item.tsx` at line 43, The default size string in src/components/ui/item.tsx contains a trailing space ("p-4 gap-4 "); remove the stray space so the value becomes "p-4 gap-4". Locate the size/default mapping (the `default` key in the size variants or `itemVariants`/size object) and update the string, then run linters/formatters to ensure no leftover whitespace remains.
156-180:ItemHeaderandItemFootershare identical class stringsBoth apply
"flex basis-full items-center justify-between gap-2"— onlydata-slotdiffers. If the visual treatment ever diverges they'll need to be updated in two places. A tiny shared constant makes the intentional parity explicit.♻️ Suggested refactor
+const itemEdgeClasses = "flex basis-full items-center justify-between gap-2" + function ItemHeader({ className, ...props }: React.ComponentProps<"div">) { return ( <div data-slot="item-header" - className={cn( - "flex basis-full items-center justify-between gap-2", - className - )} + className={cn(itemEdgeClasses, className)} {...props} /> ) } function ItemFooter({ className, ...props }: React.ComponentProps<"div">) { return ( <div data-slot="item-footer" - className={cn( - "flex basis-full items-center justify-between gap-2", - className - )} + className={cn(itemEdgeClasses, className)} {...props} /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/item.tsx` around lines 156 - 180, ItemHeader and ItemFooter duplicate the same class string; extract the shared class list into a single constant (e.g., ITEM_CONTAINER_CLASSES) and use that constant in both ItemHeader and ItemFooter to avoid duplication and make future style changes in one place; update the JSX in both components to reference the new constant while keeping the data-slot and props behavior unchanged.
43-43: Stray trailing space in thedefaultsize value
"p-4 gap-4 "has a trailing space thatcnsilently tolerates but is worth cleaning up.🧹 Proposed fix
- default: "p-4 gap-4 ", + default: "p-4 gap-4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/item.tsx` at line 43, The default size value in the sizes object (the `default` key used by the Item component in src/components/ui/item.tsx) contains a stray trailing space ("p-4 gap-4 "); remove the trailing space so the value is "p-4 gap-4" to keep class strings tidy and avoid silent inconsistencies when passed into `cn` or similar utilities.
156-180:ItemHeaderandItemFootershare identical styling — consider a shared baseBoth components produce
"flex basis-full items-center justify-between gap-2"with only thedata-slotattribute differing. If the visual language for headers and footers ever diverges, it's easy to forget to update both. A single shareditemEdgeVariantsstring (or inline constant) would make the relationship explicit.♻️ Suggested refactor
+const itemEdgeClasses = + "flex basis-full items-center justify-between gap-2" function ItemHeader({ className, ...props }: React.ComponentProps<"div">) { return ( <div data-slot="item-header" - className={cn( - "flex basis-full items-center justify-between gap-2", - className - )} + className={cn(itemEdgeClasses, className)} {...props} /> ) } function ItemFooter({ className, ...props }: React.ComponentProps<"div">) { return ( <div data-slot="item-footer" - className={cn( - "flex basis-full items-center justify-between gap-2", - className - )} + className={cn(itemEdgeClasses, className)} {...props} /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/item.tsx` around lines 156 - 180, Extract the shared class string into a single constant (e.g., itemEdgeVariants) and use it in both ItemHeader and ItemFooter to avoid duplication; update ItemHeader and ItemFooter to call cn(itemEdgeVariants, className) and keep their distinct data-slot attributes unchanged so only the class list is centralized for future updates.src/app/error.tsx (1)
3-12: Missingresetprop leaves users with no recovery pathNext.js
error.tsxreceives areset: () => voidcallback alongsideerror. Without it, a user hitting this boundary can only hard-reload the page. Adding a "Try again" button is the conventional pattern.♻️ Proposed fix
-export default function ErrorPage({ - error, -}: { - error: Error & { digest?: string }; -}) { +export default function ErrorPage({ + error, + reset, +}: { + error: Error & { digest?: string }; + reset: () => void; +}) { return ( <div> <h2>{error.message}</h2> + <button onClick={reset}>Try again</button> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/error.tsx` around lines 3 - 12, The ErrorPage component currently only accepts { error } and renders the message, but Next.js passes a reset: () => void which should be used to let users retry; update the ErrorPage signature to accept the second prop reset (i.e., function ErrorPage({ error, reset }: { error: Error & { digest?: string }; reset: () => void })) and add a "Try again" button that calls reset on click, ensuring the button is rendered alongside the error message in the component returned by ErrorPage.src/components/ui/input-group.tsx (2)
71-76: Click-to-focus:querySelector("input")could match a nested input unexpectedly.
e.currentTarget.parentElement?.querySelector("input")finds the first<input>inside the parent, which works when there is a singleInputGroupInput. If someone ever nests multiple inputs (e.g., an inline search input inside a chip addon), this would focus the wrong element. UsingquerySelector('[data-slot="input-group-control"]')would be more precise and self-documenting.Suggested tweak
- e.currentTarget.parentElement?.querySelector("input")?.focus() + e.currentTarget.parentElement + ?.querySelector<HTMLElement>('[data-slot="input-group-control"]') + ?.focus()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/input-group.tsx` around lines 71 - 76, In the onClick handler inside the InputGroup component replace the broad e.currentTarget.parentElement?.querySelector("input") lookup with a precise selector that targets the input slot (e.g., e.currentTarget.parentElement?.querySelector('[data-slot="input-group-control"]')) so clicks focus the intended InputGroupInput rather than any nested input; update the selector in the onClick callback and ensure the InputGroupInput renders the matching data-slot attribute.
100-117: Button receives nosizeprop — relies on class merging to override its default.
InputGroupButtonomitssizefrom Button's props and only passes sizing viaclassName. Button will apply its own default size classes, thencn(inputGroupButtonVariants({ size }), className)overrides them. This works becausecnusestwMerge, but it's subtle. A brief comment explaining the intentional override would aid future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/input-group.tsx` around lines 100 - 117, The InputGroupButton intentionally omits the Button "size" prop and passes sizing only via className so that inputGroupButtonVariants({ size }) merged by cn/twMerge overrides Button's default size classes; add a concise inline comment above the InputGroupButton function (or above the Button JSX) stating that omission is intentional, referencing InputGroupButton, inputGroupButtonVariants, Button, and cn (twMerge) to clarify that class merging is relied upon to override Button's default size behavior for future maintainers.src/hooks.ts (2)
9-12:useIdname shadows React's built-inuseIdhook.React 18+ exports a
useIdhook. While this custom hook is in a different module and won't technically conflict at runtime, the name collision can cause confusion during imports and code review. Consider renaming to something more descriptive likeuseRouteIdoruseSlugId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks.ts` around lines 9 - 12, The custom hook useId shadows React's built-in useId; rename the function (and its exports/imports) to a clearer name such as useRouteId or useSlugId to avoid confusion—update the function declaration export function useId() to export function useRouteId() (or chosen name), adjust internal references (the useParams<{ id?: string[] }>() destructuring and slug handling remain the same), and update all imports where useId is consumed to the new name so callers reference useRouteId (or the chosen identifier) instead of useId.
48-61: Stale closure risk:resource!is captured outside the query key.
resourceis used insidequeryFnbut is not part ofqueryKey([id, filters, key]). TanStack Query determines when to refetch based on the query key, so ifresourcewere to change shape whileidstays the same, the cached result would not invalidate. In practice this is likely safe sinceresourceis derived fromid, but the non-null assertion on Line 57 is fragile — if theenabledguard is ever changed,resource!silently becomes a runtime error.Consider a safer pattern:
Proposed improvement
export function useQueryResource<R>( key: string, fetch: (resource: Resource, filters: Filters) => Promise<R>, initialData?: R, ) { const { data: resource } = useResource(); return useQueryId<R>( key, - (_, filters) => fetch(resource!, filters), + (_, filters) => { + if (!resource) throw new Error("resource unexpectedly null"); + return fetch(resource, filters); + }, initialData, !!resource, ); }This narrows the type without a non-null assertion, making any future
enabled-guard regressions surface as an explicit error rather than undefined behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks.ts` around lines 48 - 61, The useQueryResource function captures resource with a non-null assertion (resource!) inside the queryFn which risks stale-closure or runtime errors; update useQueryResource (and its call to useQueryId) to avoid the non-null assertion by either including resource in the queryKey or narrowing resource before creating the query: call useResource(), if resource is falsy return useQueryId with enabled=false (or throw) else create the queryFn that closes over the already-narrowed resource (no !), keep enabled set to !!resource, and ensure the queryKey passed to useQueryId includes resource (or a stable identifier derived from it) so TanStack Query revalidates when resource shape changes; adjust symbols: useQueryResource, useResource, useQueryId, queryFn, and queryKey accordingly.src/app/globals.css (1)
55-89: Missing--card-titlein.darktheme.
--card-titleis defined in:root(Line 61) and mapped to--color-card-titlein the theme (Line 37), but the.darkblock (Lines 91–123) doesn't override it. If dark mode should use a different card-title color, add an override; if the light-mode value is intentional for both themes, a brief comment would clarify that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 55 - 89, The .dark theme is missing an override for the --card-title variable: add a --card-title declaration inside the .dark block (next to other overrides like --card, --card-foreground) to set the dark-mode card title color, or if the :root value is intentional for both themes, add a short comment inside the .dark block referencing --card-title and --color-card-title to indicate the shared value so reviewers know it was deliberate.src/components/ui/combobox.tsx (2)
98-133: Extremely longclassNamestring reduces readability.The
classNameon Lines 124-125 is a single line with many Tailwind utilities and data-attribute selectors. Consider extracting subsets into namedcvavariants or breaking the string across multiple lines (using template literals or an array joined by spaces) for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/combobox.tsx` around lines 98 - 133, The ComboboxContent component has an unreadably long className string; refactor it by extracting the bulky Tailwind/data-attribute rules into a named constant (e.g., comboboxContentClass) using a class-variance-authority (cva) definition or into an array/template literal joined by spaces, then replace the inline string in the ComboboxPrimitive.Popup className with that constant combined with the incoming className; target the ComboboxContent function and the ComboboxPrimitive.Popup call, and move grouped rules (animations, positioning, data-[slot] modifiers, and chips/min-width logic) into cva variants or separate variables so the JSX line becomes concise and maintainable.
233-248: Remove redundant type intersection inComboboxChipsprops.
React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & ComboboxPrimitive.Chips.Propsis redundant.ComboboxPrimitive.Chips.Propsalready extendsBaseUIComponentProps<'div'>, which includes all React div component props with ref support. Additionally,ComboboxChipsis not aforwardRefwrapper, so the ref is not actually forwarded. UseComboboxPrimitive.Chips.Propsalone to match the pattern used consistently throughout the file (e.g.,ComboboxSeparator,ComboboxTrigger,ComboboxClear).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/combobox.tsx` around lines 233 - 248, The ComboboxChips component signature should stop using the redundant intersection of React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & ComboboxPrimitive.Chips.Props; update the props type to just ComboboxPrimitive.Chips.Props (since it already includes div props and ref support) and ensure the component remains a plain function (not forwardRef) to match other components like ComboboxSeparator/ComboboxTrigger/ComboboxClear; remove the unnecessary ComponentPropsWithRef usage and any now-unused imports if present.src/components/RadialChart.tsx (1)
45-48: Remove the commented-outChartTooltipblock.This dead code has been commented out since at least the initial implementation and is unlikely to be re-enabled. If tooltip support is genuinely planned, a follow-up issue is a better tracking mechanism.
♻️ Proposed cleanup
- {/* <ChartTooltip - cursor={false} - content={<ChartTooltipContent hideLabel />} - /> */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RadialChart.tsx` around lines 45 - 48, Remove the dead/commented-out tooltip JSX from RadialChart.tsx: delete the commented ChartTooltip block that references ChartTooltip and ChartTooltipContent (the three commented lines) so the file no longer contains leftover commented code; if tooltip work is planned, create a separate issue instead of keeping commented markup.src/app/Header.tsx (1)
11-11: Remove the emptyclassName=""fromImage.The prop has no effect and adds noise.
♻️ Proposed cleanup
- <Image src={logo} alt="DataCite logo" height={75} className="" /> + <Image src={logo} alt="DataCite logo" height={75} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/Header.tsx` at line 11, The Image element in Header.tsx includes an empty prop className="" which is no-op and should be removed; edit the JSX containing <Image src={logo} alt="DataCite logo" height={75} className="" /> to drop the className attribute (or only pass className when a non-empty value is needed), keeping the Image import/usage and other props unchanged.src/app/[[...id]]/Header.tsx (1)
12-13: Redundanttext-4xland inconsistent className composition.
H2fromHeadings.tsxalready appliestext-4xlas part of its base classes viacn(). Repeating it here is a no-op. Additionally, the template literal diverges from thecn()pattern used throughout the rest of the codebase for conditional class composition.♻️ Proposed cleanup
- <H2 className={`text-4xl w-full ${resource ? "" : "opacity-70"}`}> + <H2 className={cn("w-full", !resource && "opacity-70")}>Add the
cnimport:+"use client"; + +import { cn } from "@/lib/utils"; import { H2 } from "@/components/datacite/Headings";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[[...id]]/Header.tsx around lines 12 - 13, Remove the redundant "text-4xl" from the H2 usage and switch to using the shared cn() helper for class composition: import cn and rewrite the H2 className to call cn("w-full", resource ? "" : "opacity-70") (or equivalent with conditional entry) so the base classes from Headings.tsx are not duplicated and the conditional class follows the cn() pattern; reference the H2 component and the resource/id props when making this change.src/components/cards/OverviewCard.tsx (1)
14-15: Error display may render[object Object]instead of a readable message.
errorfrom React Query is typically anErrorinstance. The template literal`Error: ${error}`will call.toString()on it, which may produceError: Error: actual message(doubled prefix). Consider usingerror.messagefor a cleaner display, or rendering a proper error UI component.♻️ Suggested fix
- if (isError) return `Error: ${error}`; + if (isError) return `Error: ${error instanceof Error ? error.message : error}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/cards/OverviewCard.tsx` around lines 14 - 15, The current error branch in OverviewCard returns a template string which can render unhelpful "[object Object]" or duplicate "Error:" prefixes; update the isError branch in the OverviewCard component to return a proper React node that displays the underlying error message (use error?.message or extract message from the React Query error object) or render a small Error UI component instead of `\`Error: ${error}\`` so the message is readable and safe.src/components/Badges.tsx (1)
19-21: Defaultshow = trueis dead code sinceshowis a required prop.The type
{ show: boolean }makesshowrequired, so the default value= trueon Line 20 never triggers. Either make the prop optional (show?: boolean) or remove the default.♻️ Suggested fix — make `show` optional
-export function HighImpactBadge(props: { show: boolean } & BadgeProps) { +export function HighImpactBadge(props: { show?: boolean } & BadgeProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Badges.tsx` around lines 19 - 21, The default assignment "show = true" in the HighImpactBadge component is dead because the prop is typed as required; change the prop type from "{ show: boolean } & BadgeProps" to "{ show?: boolean } & BadgeProps" so the default will take effect (leave the destructuring "const { show = true, className, ...badgeProps } = props" as is); alternatively, if you prefer to keep show required, remove the "= true" default in the destructuring—update either the HighImpactBadge prop type or its destructuring accordingly.src/components/ResourceTypesChart.tsx (1)
62-83: Legend truncation silently hides entries with no visual cue.When
limit={5}is applied (Line 69), any resource types beyond the first 5 are silently hidden from the legend. Consider adding a summary item like "+ N more" so users know additional types exist in the chart but aren't listed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ResourceTypesChart.tsx` around lines 62 - 83, ChartLegendContent currently slices props.payload and hides entries without any indicator; compute the filtered list (filtering out item.type === "none") into a variable, apply the slice using props.limit, and if filtered.length > (props.limit ?? filtered.length) append a final legend entry that displays a "+ N more" summary (use a unique key like `more-${N}` and match the existing styling, e.g., a small square followed by the text) so users see how many entries were omitted; ensure you don't render the summary when hidden count is <= 0.src/components/DistributionChart.tsx (1)
8-11: Props type accepts all div attributes but onlyclassNameis forwarded.
PropsextendsReact.HTMLAttributes<HTMLDivElement>, implying callers can passstyle,onClick, etc. However, onlyclassNameis read fromprops(Line 24) — other attributes are silently discarded. Consider either spreading the rest onto the outer<div>or narrowing the type to only acceptclassName.♻️ Option: spread remaining props
export default function DistributionChart(props: Props) { - const { property, data } = props; + const { property, data, className, ...divProps } = props; const [displayAll, setDisplayAll] = useState(false); const toggleDisplayAll = () => setDisplayAll(!displayAll); if (data.length === 0) return null; const displayedData = displayAll ? data : data.slice(0, 3); return ( - <div className={cn("w-full flex flex-col h-min gap-2", props.className)}> + <div className={cn("w-full flex flex-col h-min gap-2", className)} {...divProps}>Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DistributionChart.tsx` around lines 8 - 11, The Props interface currently extends React.HTMLAttributes<HTMLDivElement> but the DistributionChart component only reads className and drops other attributes; fix by changing the component signature to destructure className and the rest (e.g., ({ property, data, className, ...rest }: Props)) and spread ...rest onto the outer <div> (the container rendered by DistributionChart) so that style, onClick, aria-*, etc. are forwarded; alternatively, if you want to restrict accepted props, narrow Props to only include className instead of extending HTMLAttributes.src/components/datacite/Headings.tsx (1)
4-6: H1, H5, H6 have no base styles ordmSansfont — intentional placeholders?H2–H4 apply typography classes and the
dmSansfont, but H1, H5, and H6 pass through withcn("", className), making them bare HTML elements. If these are intentional stubs for future use, a brief comment would clarify intent; otherwise they should get consistent base styling.Also applies to: 39-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/datacite/Headings.tsx` around lines 4 - 6, H1, H5, and H6 in Headings.tsx are currently passthroughs (cn("", className)) and lack the base typography and dmSans font used by H2–H4; either add the same base classes plus the dmSans font to these components (mirror the pattern used in H2/H3/H4) — e.g., include the shared base class tokens and the dmSans class in the cn call for H1, H5, and H6 — or, if they are intentionally left unstyled, add a brief comment above the H1/H5/H6 functions stating they are deliberate placeholders so reviewers know it’s by design.src/data/fetch.ts (4)
125-132: Formatters use positional array indices — brittle coupling to field order inCOMPLETENESS_FIELDS.All formatters (e.g.,
formatCreatorsline 126:p[0],p.slice(1, 5),p[5]) depend on the exact ordering of fields in the correspondingCOMPLETENESS_FIELDSentry. If someone reorders a field inCREATOR_FIELDS.present, the formatter silently produces wrong mappings. This is a maintenance risk worth documenting at minimum.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data/fetch.ts` around lines 125 - 132, formatCreators relies on positional indices into the parsed array (p[0], p.slice(1,5), p[5]) which tightly couples it to the ordering of COMPLETENESS_FIELDS/CREATOR_FIELDS.present and will break if field order changes; change createFormat/formatCreators to map by field name instead of fixed indices: update createFormat to accept a schema (e.g., the CREATOR_FIELDS.present array) or a name->index map and use indexOf or a precomputed lookup to reference fields by name (e.g., "creators", "affiliation", "nameIdentifier") when constructing the returned object, and update formatCreators to use those named lookups rather than hardcoded numeric indices.
62-63:getDatahas no error handling — non-200 responses or network errors will throw cryptic errors.If
fetchDatacitereturns a non-200 response,.json()may still parse (e.g., an error body), and the cast toTwill silently produce a misshapen object. If the response isn't JSON at all,.json()throws a generic parse error. Consider adding a response status check.Proposed fix
-const getData = async <T extends { data: unknown }>(url: string) => - ((await (await fetchDatacite(url)).json()) as T).data as T["data"]; +const getData = async <T extends { data: unknown }>(url: string) => { + const res = await fetchDatacite(url); + if (!res.ok) throw new Error(`API request failed: ${res.status} ${res.statusText}`); + return ((await res.json()) as T).data as T["data"]; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data/fetch.ts` around lines 62 - 63, getData currently assumes fetchDatacite succeeds and returns valid JSON; add robust error handling by awaiting the response object from fetchDatacite, checking response.ok (and if not, read text/json body for details) and throwing a clear, typed error that includes status and body; wrap the .json() call in try/catch to catch parse errors and rethrow a descriptive error mentioning the URL and response status; update the getData<T> function to perform these checks before casting to T["data"] so you never silently return a malformed object.
37-51: Children fetch assumesgetDataalways returns an array forApiResource<true>— a 404 or error response could crash.map().If the children endpoint returns a non-array (e.g., the provider has no associated clients endpoint or the API returns an error),
.map()on line 43 will throw a runtime error.Proposed fix
children: data.type !== "clients" - ? ( + ? (( await getData<ApiResource<true>>( `${data.attributes.memberType === "consortium" ? "providers" : "clients"}?page[size]=1000&${data.attributes.memberType === "consortium" ? "consortium" : "provider"}-id=${data.id}`, ) - ).map((c) => ({ + ) || []).map((c) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data/fetch.ts` around lines 37 - 51, The children mapping assumes getData<ApiResource<true>>(...) always returns an array; to prevent runtime crashes when the API returns 404/non-array or an error, await getData(...) into a local (e.g., childrenData), check Array.isArray(childrenData) (or coerce with childrenData ?? []), and only call .map(...) when it's an array; otherwise return an empty array and optionally log or surface the error. Update the expression around getData<ApiResource<true>>(...) used in the children property so it safely handles non-array responses (preserve the existing mapping of c.id, c.attributes.name, and subtype logic that uses data.attributes.memberType and c.type).
14-60: RecursivefetchResourcehas no depth guard — a malformed API response could cause infinite recursion.
fetchResourcecalls itself recursively (line 52) to resolve the parent chain. The DataCite hierarchy is shallow (≤3 levels), but there's no protection against cycles or unexpectedly deep chains. A simple depth counter would safeguard against this.Proposed fix sketch
export async function fetchResource( id: string | null, + depth = 0, ): Promise<Resource | null> { if (!id) return null; + if (depth > 5) return null; // ... - parent: await fetchResource( + parent: await fetchResource( data.type === "clients" ? data.relationships.provider.data.id : data.relationships.consortium?.data.id || null, + depth + 1, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data/fetch.ts` around lines 14 - 60, The recursive fetchResource call can loop indefinitely on malformed API data; add a depth guard to fetchResource (e.g., add an optional parameter like depth: number = 0 and a MAX_DEPTH constant) and immediately return null (or stop resolving parent) when depth >= MAX_DEPTH; increment depth when calling fetchResource for the parent (and also thread depth through any internal helper calls like the parent resolution branch) so cycles or unexpectedly deep chains are cut off. Optionally, you can also add a simple visited-ids set passed through recursion to detect cycles by id (use the id from ApiResource) and bail out if an id repeats.src/components/PresentBar.tsx (1)
53-64: Consider removing redundanttickFormatterandtickprops on the hidden YAxis.Since
hideis set (line 63), thetickFormatter,tick,interval, andorientationprops on YAxis have no visible effect. IfincludeHiddenis only needed for layout spacing, you can simplify by removing the formatting props.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PresentBar.tsx` around lines 53 - 64, In PresentBar's YAxis component the props tickFormatter, tick, interval and orientation are redundant because hide is true (and includeHidden is used only for spacing); remove tickFormatter, tick, interval and orientation from the <YAxis> element (leave dataKey, type, axisLine, tickLine, includeHidden and hide) so the hidden axis remains for layout without unnecessary formatting props.src/util.ts (1)
127-139:toPresentProps/toDistributionPropswill throw if the API omits a requested field.
findInPresent/findInDistributionreturnundefinedwhen the completeness API doesn't include a requested field in its response (nodefaultValueis passed tofindBuilder). This propagates totoPresentProps(undefined)which throws"Present item is undefined". While fail-fast is reasonable, a single missing field will crash the entire card.Consider providing a fallback so missing fields degrade to 0% instead of throwing:
Proposed fix
const findInPresent = findBuilder( json.present, (item, desired: string) => item.field === desired, + { field: "", percent: 0, count: 0, absent_count: 0 } as Present, ); const findInDistribution = findBuilder( json.distribution, (item, desired: string) => item.field === desired, + { field: "", values: [] } as Distribution, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.ts` around lines 127 - 139, findInPresent/findInDistribution can return undefined and passing that into toPresentProps/toDistributionProps throws; ensure missing fields degrade to 0% by supplying a safe fallback before mapping. Change the pipeline that builds present and distribution so that after mapping with findInPresent/findInDistribution you coalesce undefined to a default object (e.g. { field: desiredField, completeness: 0 } or equivalent shape) or supply a defaultValue into findBuilder; then pass the safe object into toPresentProps/toDistributionProps so a missing API field becomes a 0% entry rather than throwing.src/constants.ts (1)
17-59:Otherkey inPALETTE_OTHERis silently overridden byPALETTE_RESOURCE_TYPE.
PALETTE_OTHER.Otheris"gray"(line 19), but it's overridden byPALETTE_RESOURCE_TYPE.Other="#C59088"(line 58). If the"gray"value inPALETTE_OTHERis intentionally a generic fallback for non-resource-type contexts, this is fine. But if any consumer expectsPALETTE_RESOURCE_TYPEto include the original"gray"forOther, it won't. Just flagging for awareness since the override is easy to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants.ts` around lines 17 - 59, PALETTE_OTHER.Other is being shadowed by PALETTE_RESOURCE_TYPE.Other when you spread PALETTE_OTHER into PALETTE_RESOURCE_TYPE; decide which value should win and fix accordingly: either remove or rename the duplicate key in PALETTE_RESOURCE_TYPE (e.g., delete the "Other" entry in PALETTE_RESOURCE_TYPE) or rename PALETTE_OTHER.Other to a distinct fallback key (e.g., Fallback or Generic) and update all consumers, or change the spread order to have PALETTE_OTHER spread last so PALETTE_OTHER.Other overrides; reference PALETTE_OTHER and PALETTE_RESOURCE_TYPE to locate the change.
| if (Array.isArray(value)) | ||
| for (const v in value) urlSearchParams.append(key, v); |
There was a problem hiding this comment.
Bug: for...in iterates array indices, not values.
for (const v in value) yields the string indices "0", "1", … — not the actual array elements. This corrupts multi-valued query parameters during the lowercase redirect (e.g., ?foo=a&foo=b becomes ?foo=0&foo=1).
🐛 Fix: use `for...of`
if (Array.isArray(value))
- for (const v in value) urlSearchParams.append(key, v);
+ for (const v of value) urlSearchParams.append(key, v);
else urlSearchParams.append(key, value);📝 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.
| if (Array.isArray(value)) | |
| for (const v in value) urlSearchParams.append(key, v); | |
| if (Array.isArray(value)) | |
| for (const v of value) urlSearchParams.append(key, v); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[[...id]]/page.tsx around lines 23 - 24, The loop that appends
multi-valued query params uses `for (const v in value)` which iterates indices
not elements, corrupting values; update the loop in the code that builds
`urlSearchParams` to iterate array elements (use `for...of` or equivalent) so
`urlSearchParams.append(key, v)` receives the actual element values; locate the
block around `urlSearchParams.append` in the page rendering/redirect logic in
src/app/[[...id]]/page.tsx and replace the `for...in` with a `for...of` over
`value`.
| const BreadcrumbWrapper = (wrapperProps: { children: ReactNode }) => ( | ||
| <Breadcrumb> | ||
| <BreadcrumbList> | ||
| {pages.map((page, index) => ( | ||
| <React.Fragment key={page.title + page.href}> | ||
| {index > 0 && <BreadcrumbSeparator />} | ||
| <BreadcrumbItem> | ||
| {page.href && ( | ||
| <BreadcrumbLink href={page.href}>{page.title}</BreadcrumbLink> | ||
| )} | ||
| {!page.href && <BreadcrumbPage>{page.title}</BreadcrumbPage>} | ||
| </BreadcrumbItem> | ||
| </React.Fragment> | ||
| ))} | ||
| <BreadcrumbLink href="/"> | ||
| <Home /> | ||
| </BreadcrumbLink> | ||
| <Separator /> | ||
| {wrapperProps.children} {isPending && <Spinner />} | ||
| </BreadcrumbList> | ||
| </Breadcrumb> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Anti-pattern: component defined inside render causes remount on every render.
BreadcrumbWrapper is declared as a new function component on every render of Breadcrumbs. React sees a new component type each cycle, so it unmounts and remounts all children — destroying DOM and any child state (e.g., the Combobox open state, search input text).
Refactor to render the wrapper inline instead:
♻️ Proposed refactor — use inline JSX wrapping
Extract the common wrapper as plain JSX and use it directly in the two return paths:
- const BreadcrumbWrapper = (wrapperProps: { children: ReactNode }) => (
- <Breadcrumb>
- <BreadcrumbList>
- <BreadcrumbLink href="/">
- <Home />
- </BreadcrumbLink>
- <Separator />
- {wrapperProps.children} {isPending && <Spinner />}
- </BreadcrumbList>
- </Breadcrumb>
- );
-
- if (!resource)
- return (
- <BreadcrumbWrapper>
- <BreadcrumbContent resource={{ id, name: id, subtype: "" }} />
- </BreadcrumbWrapper>
- );
+ const breadcrumbContent = !resource ? (
+ <BreadcrumbContent resource={{ id, name: id, subtype: "" }} />
+ ) : (
+ <>
+ {pages.map((page, index) => (
+ <React.Fragment key={page.id || index}>
+ {index > 0 && <Separator />}
+ <ChildrenSelect resource={page} items={page.parent?.children || []}>
+ <BreadcrumbContent resource={page} />
+ </ChildrenSelect>
+ </React.Fragment>
+ ))}
+ {resource.children.length > 0 && (
+ <>
+ <Separator />
+ <ChildrenSelect resource={resource} items={resource.children} className="opacity-70">
+ Select {resource.subtype === "consortium" ? "organization" : "repository"}...
+ </ChildrenSelect>
+ </>
+ )}
+ </>
+ );
+
+ return (
+ <Breadcrumb>
+ <BreadcrumbList>
+ <BreadcrumbLink href="/">
+ <Home />
+ </BreadcrumbLink>
+ <Separator />
+ {breadcrumbContent} {isPending && <Spinner />}
+ </BreadcrumbList>
+ </Breadcrumb>
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Breadcrumbs.tsx` around lines 44 - 54, BreadcrumbWrapper is
currently defined inside the Breadcrumbs render which creates a new component
type each render causing remounts; remove the inner function and instead render
the wrapper inline (or move it to a stable outer component) by replacing usages
of <BreadcrumbWrapper>{...}</BreadcrumbWrapper> with the equivalent JSX
structure using Breadcrumb, BreadcrumbList, BreadcrumbLink, Home, Separator and
conditionally render {isPending && <Spinner />}; ensure any props (children) are
forwarded directly and delete the BreadcrumbWrapper declaration so children like
the Combobox keep their state.
| const BreadcrumbPageLink = (wrapperProps: { children: ReactNode }) => | ||
| props.resource.id === id ? ( | ||
| <BreadcrumbPage {...wrapperProps} className={className} /> | ||
| ) : ( | ||
| <BreadcrumbLink | ||
| {...wrapperProps} | ||
| href={`/${props.resource.id}`} | ||
| className={className} | ||
| onMouseDown={(e) => e.stopPropagation()} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same anti-pattern: BreadcrumbPageLink component defined inside render.
BreadcrumbPageLink is redefined on every render of BreadcrumbContent, causing the same remount problem. Since it captures closure variables (props.resource.id, id, className), convert it to a conditional JSX expression or extract it as a separate component that receives these as props.
♻️ Proposed refactor — inline the conditional
return (
<BreadcrumbItem>
- <BreadcrumbPageLink>
+ {props.resource.id === id ? (
+ <BreadcrumbPage className={className}>
+ <Item className="p-0 pt-5">
+ <ItemContent className="gap-0">
+ <ItemTitle className={props.resource.id === id ? "bg-black/0 font-semibold" : ""}>
+ {props.resource.name} <ResourceBadge resource={props.resource} />
+ </ItemTitle>
+ <ItemDescription className="text-muted-foreground/75 text-start">
+ {props.resource.id}
+ </ItemDescription>
+ </ItemContent>
+ </Item>
+ </BreadcrumbPage>
+ ) : (
+ <BreadcrumbLink
+ href={`/${props.resource.id}`}
+ className={className}
+ onMouseDown={(e) => e.stopPropagation()}
+ >
- <Item className="p-0 pt-5">
- <ItemContent className="gap-0">
- <ItemTitle
- className={
- props.resource.id === id ? "bg-black/0 font-semibold" : ""
- }
- >
- {props.resource.name} <ResourceBadge resource={props.resource} />
- </ItemTitle>
- <ItemDescription className="text-muted-foreground/75 text-start">
- {props.resource.id}
- </ItemDescription>
- </ItemContent>
- </Item>
- </BreadcrumbPageLink>
+ <Item className="p-0 pt-5">
+ <ItemContent className="gap-0">
+ <ItemTitle>
+ {props.resource.name} <ResourceBadge resource={props.resource} />
+ </ItemTitle>
+ <ItemDescription className="text-muted-foreground/75 text-start">
+ {props.resource.id}
+ </ItemDescription>
+ </ItemContent>
+ </Item>
+ </BreadcrumbLink>
+ )}
</BreadcrumbItem>
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Breadcrumbs.tsx` around lines 110 - 120, BreadcrumbPageLink is
being redefined on every render inside BreadcrumbContent and closes over
props.resource.id, id, and className; extract the conditional into either (A) an
inline JSX expression in the render where you choose between <BreadcrumbPage ...
/> and <BreadcrumbLink ... /> using the existing condition (props.resource.id
=== id) or (B) move BreadcrumbPageLink out as a new pure component that accepts
propsResourceId, id, className, children and the onMouseDown handler, then
render that component from BreadcrumbContent so it no longer gets recreated each
render; ensure the onMouseDown={(e) => e.stopPropagation()} behavior is
preserved and pass through children to BreadcrumbPage/BreadcrumbLink.
| const registrationYears = doisMeta.registered || []; | ||
|
|
||
| return { | ||
| total: doisMeta.total, | ||
| resourceTypeData, | ||
| registrationYears, | ||
| registrationsData: registrationYears.reverse().map((f) => ({ | ||
| year: f.id, | ||
| count: f.count, | ||
| })), | ||
| }; |
There was a problem hiding this comment.
.reverse() mutates registrationYears in place — both returned fields share the same reversed array.
Array.prototype.reverse() mutates the original array. After line 89 executes, the registrationYears reference on line 88 points to the same (now reversed) array. This means the Combobox in FilterByRegistrationYear will display years in ascending order (oldest first) — which may or may not be intended. Use a non-mutating approach for clarity:
Proposed fix
return {
total: doisMeta.total,
resourceTypeData,
registrationYears,
- registrationsData: registrationYears.reverse().map((f) => ({
+ registrationsData: [...registrationYears].reverse().map((f) => ({
year: f.id,
count: f.count,
})),
};📝 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 registrationYears = doisMeta.registered || []; | |
| return { | |
| total: doisMeta.total, | |
| resourceTypeData, | |
| registrationYears, | |
| registrationsData: registrationYears.reverse().map((f) => ({ | |
| year: f.id, | |
| count: f.count, | |
| })), | |
| }; | |
| const registrationYears = doisMeta.registered || []; | |
| return { | |
| total: doisMeta.total, | |
| resourceTypeData, | |
| registrationYears, | |
| registrationsData: [...registrationYears].reverse().map((f) => ({ | |
| year: f.id, | |
| count: f.count, | |
| })), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/data/fetch.ts` around lines 83 - 93, The returned object mutates
registrationYears by calling registrationYears.reverse(), causing both
registrationYears and registrationsData to reference the same reversed array;
instead create a non-mutating copy before reversing (e.g., use a shallow copy
like [...registrationYears] or registrationYears.slice()) when building
registrationsData so registrationYears remains in its original order; update the
code that constructs registrationsData (the registrationsData mapping derived
from registrationYears) to operate on the copied-and-reversed array to avoid
in-place mutation of the doisMeta.registered value.
2 participants
Purpose
Addresses the "Should Do" todos.
Notable updates:
closes: #6 and #7
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
Release Notes
New Features
Style
Refactor
Chores