FIXING THE CONTRIBUTE CAD #1606
Conversation
Summary by CodeRabbit
WalkthroughThe changes introduce support for displaying clickable labels and programming languages in the Card component, update type definitions to accommodate these new features, and adjust internal logic for search and loading states. Several props and types are restructured, and icon support is expanded to include label icons. Changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
frontend/src/hooks/useSearchPage.ts (1)
66-71:⚠️ Potential issue
sortBy[0] !== 'default'is a logic bug – compare the full stringIndexing the first character (
sortBy[0]) and comparing it with'default'always evaluates totrue, so the guard is ineffective. Simplify withstartsWith:-if (sortBy && sortBy !== 'default' && sortBy[0] !== 'default' && sortBy !== '') { +if (sortBy && !sortBy.startsWith('default')) {Replicate the same fix in the
ordercheck below to avoid inconsistent URL parameters.
🧹 Nitpick comments (3)
frontend/src/components/SearchPageLayout.tsx (1)
46-47: Inverting the prop solves the loading bug but may flash an empty bar
isLoaded={!isFirstLoad}means the search bar renders as not loaded for one render even when the data fetch has already completed (the render beforesetIsFirstLoad(false)resolves). Rare, but users on very fast connections could notice a one-frame skeleton flash.Consider moving
isFirstLoadinto the data hook or computingisLoadeddirectly from props to eliminate the extra render.frontend/src/components/Search.tsx (1)
68-93: Good inversion – but add accessibility label to the clear buttonThe conditional now shows the real input only when data is ready – nice!
Minor a11y nit: the clear-search button is unlabeled for screen readers.-<button +<button + aria-label="Clear search" className="absolute right-2 top-1/2 -translate-y-1/2 rounded-full p-1 hover:bg-gray-100 focus:outline-none focus:ring-2 focus:ring-gray-300" onClick={handleClearSearch} >frontend/src/utils/data.ts (1)
34-35:faTagregistration is harmless but redundantBecause
Card.tsximports thefaTagdefinition explicitly and passes it to<FontAwesomeIcon>, the icon does not need to be added to the global library.
Keeping it here is not wrong, but it bloats the bundle a little and breaks the “only-register-what-is-referenced-by-string” convention used elsewhere in this file (everything inlibrary.add()is later referenced via string class names). Consider removing it or switching the button render to use the string class instead for consistency.Also applies to: 64-66
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/app/contribute/page.tsx(2 hunks)frontend/src/components/Card.tsx(4 hunks)frontend/src/components/Search.tsx(1 hunks)frontend/src/components/SearchPageLayout.tsx(1 hunks)frontend/src/hooks/useSearchPage.ts(2 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/skeleton.ts(1 hunks)frontend/src/utils/data.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/components/Card.tsx (3)
frontend/src/types/card.ts (1)
CardProps(14-40)frontend/src/utils/data.ts (1)
Icons(68-109)frontend/src/components/DisplayIcon.tsx (1)
DisplayIcon(7-60)
frontend/src/types/card.ts (1)
frontend/src/types/contributor.ts (1)
Contributor(1-8)
🔇 Additional comments (4)
frontend/src/hooks/useSearchPage.ts (1)
104-104: RemovedpageTitlefrom deps – verify callers no longer rely on itThe data-fetching effect now ignores
pageTitle. Ensure all downstream callers that previously passed this prop have been updated, otherwise the hook change is silent-breaking.frontend/src/types/skeleton.ts (1)
11-13: Good extension of the skeleton API
showLabelsandshowLanguagesalign perfectly with the new Card props. No issues spotted.frontend/src/app/contribute/page.tsx (1)
34-39:SubmitButtonnow has bothonclickandurl– verify ActionButton behaviourIf
ActionButtonprioritisesurl, the modal may never open; if it prioritisesonClick, navigation will never occur. Confirm the precedence and remove the unnecessary field to prevent double navigation or unexpected no-ops.frontend/src/components/Card.tsx (1)
110-156: Nice UX addition – labels & languagesImplementation is clean, accessible, and uses proper
encodeURIComponent. Good job handling “show more/less” toggles with state.
| const sortByParam = searchParams.get('sortBy') || defaultSortBy | ||
| const orderParam = searchParams.get('order') || defaultOrder | ||
| const pageParam = parseInt(searchParams.get('page') || '1') | ||
|
|
||
| setSearchQuery(searchQueryParam) | ||
| setSortBy(sortByParam) | ||
| setOrder(orderParam) | ||
| setCurrentPage(pageParam) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify radix & guard against NaN when parsing the page param
parseInt without an explicit radix relies on implementation defaults and may yield NaN for unexpected inputs (e.g. ?page=). A tiny defensive tweak prevents edge-case crashes:
-const pageParam = parseInt(searchParams.get('page') || '1')
+const rawPage = searchParams.get('page') || '1'
+const pageParam = Number.isNaN(Number(rawPage)) ? 1 : parseInt(rawPage, 10)📝 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 sortByParam = searchParams.get('sortBy') || defaultSortBy | |
| const orderParam = searchParams.get('order') || defaultOrder | |
| const pageParam = parseInt(searchParams.get('page') || '1') | |
| setSearchQuery(searchQueryParam) | |
| setSortBy(sortByParam) | |
| setOrder(orderParam) | |
| setCurrentPage(pageParam) | |
| const sortByParam = searchParams.get('sortBy') || defaultSortBy | |
| const orderParam = searchParams.get('order') || defaultOrder | |
| const rawPage = searchParams.get('page') || '1' | |
| const pageParam = Number.isNaN(Number(rawPage)) | |
| ? 1 | |
| : parseInt(rawPage, 10) | |
| setSearchQuery(searchQueryParam) | |
| setSortBy(sortByParam) | |
| setOrder(orderParam) | |
| setCurrentPage(pageParam) |
🤖 Prompt for AI Agents
In frontend/src/hooks/useSearchPage.ts around lines 50 to 57, the parseInt call
for pageParam lacks an explicit radix and does not handle NaN values. Fix this
by adding a radix of 10 to parseInt and add a check to default pageParam to 1 if
the parsed value is NaN, ensuring robust handling of empty or invalid page query
parameters.
| icons={ | ||
| Object.fromEntries( | ||
| Object.entries(filteredIcons).map(([key, value]) => [key, Boolean(value)]) | ||
| ) as Record<string, boolean> | ||
| } | ||
| button={SubmitButton} |
There was a problem hiding this comment.
Boolean-casting the icon map breaks existing UI – numbers become “true” strings
getFilteredIcons() used to return the actual numeric counts (stars, forks, …).
After this cast every entry is true, and DisplayIcon still renders the value next to the icon; users will now see the literal text true.
- icons={Object.fromEntries(
- Object.entries(filteredIcons).map(([key, value]) => [key, Boolean(value)])
- ) as Record<string, boolean>}
+ // keep original numeric values (falsy entries filtered out)
+ icons={Object.fromEntries(
+ Object.entries(filteredIcons).filter(([_, value]) => Boolean(value))
+ ) as Record<string, number>} // adjust type accordinglyYou will also need to revert the icons type change (see types/card.ts) or make DisplayIcon ignore non-numeric truthy values.
📝 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.
| icons={ | |
| Object.fromEntries( | |
| Object.entries(filteredIcons).map(([key, value]) => [key, Boolean(value)]) | |
| ) as Record<string, boolean> | |
| } | |
| button={SubmitButton} | |
| icons={ | |
| // keep original numeric values (falsy entries filtered out) | |
| Object.fromEntries( | |
| Object.entries(filteredIcons).filter(([_, value]) => Boolean(value)) | |
| ) as Record<string, number> | |
| } | |
| button={SubmitButton} |
🤖 Prompt for AI Agents
In frontend/src/app/contribute/page.tsx around lines 56 to 61, the code
incorrectly casts all icon values to boolean, causing numeric counts to display
as the string "true" in the UI. To fix this, remove the Boolean cast so that the
original numeric counts from getFilteredIcons() are preserved and passed to the
icons prop. Additionally, revert any related type changes in types/card.ts to
accept numeric values again or update DisplayIcon to handle non-numeric truthy
values properly.
| {icons && Object.keys(Icons).some((key) => icons[key as keyof typeof icons]) && ( | ||
| <div className="mt-3 flex flex-wrap"> | ||
| {Object.keys(Icons).map((key, index) => | ||
| icons[key] ? ( | ||
| {Object.keys(Icons).map((key, index) => { | ||
| const iconKey = key as keyof typeof icons; | ||
| return icons[iconKey] ? ( | ||
| <DisplayIcon | ||
| key={`${key}-${index}`} | ||
| item={key} | ||
| icons={Object.fromEntries(Object.entries(icons).filter(([_, value]) => value))} | ||
| icons={Object.fromEntries( | ||
| Object.entries(icons).filter(([_, value]) => Boolean(value)) | ||
| )} | ||
| /> | ||
| ) : null | ||
| )} | ||
| ) : null; | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
icons are filtered but still passed with boolean/number ambiguity
DisplayIcon expects an object whose values are displayable (number/string).
Because we pass booleans, the component renders the literal text true. Either:
- Revert to the original numeric object (preferred – keeps counts visible), or
- Update
DisplayIconto suppress non-numeric values.
Example quick fix inside DisplayIcon:
-{typeof icons[item] === 'number'
- ? millify(icons[item], { precision: 1 })
- : icons[item]}
+{typeof icons[item] === 'number'
+ ? millify(icons[item], { precision: 1 })
+ : null}Choose one path and align types/card.ts accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/Card.tsx around lines 80 to 94, the icons object is
filtered but still contains boolean values, which causes DisplayIcon to render
literal "true" text. To fix this, revert to passing the original numeric values
for icons instead of booleans, ensuring DisplayIcon receives displayable numbers
or strings. Also, update the types in types/card.ts to reflect that icons hold
numeric or string values, not booleans.
| level?: { | ||
| level: string | ||
| color: string | ||
| icon: any | ||
| } | ||
| icons?: Record<string, boolean> | ||
| topContributors?: Contributor[] | ||
| button: { | ||
| url: string | ||
| onclick?: () => void | ||
| icon?: React.ReactNode | ||
| label?: string | ||
| } | ||
| projectName?: string | ||
| projectLink?: string | ||
| social?: { | ||
| title: string | ||
| url: string | ||
| }[] | ||
| tooltipLabel?: string | ||
| labels?: string[] | ||
| languages?: string[] |
There was a problem hiding this comment.
Type drift causing runtime/UI bugs
Changing icons from Record<string, number | string> to Record<string, boolean> unintentionally removed star/fork counts and introduced the “true” rendering bug. Decide which of these two designs you need:
• Counts visible → revert to the previous numeric union type.
• Icons only → keep boolean but update DisplayIcon to hide the value.
Synchronise page.tsx and Card.tsx with the chosen approach to restore type safety.
🤖 Prompt for AI Agents
In frontend/src/types/card.ts between lines 18 and 39, the type of the `icons`
property was changed from `Record<string, number | string>` to `Record<string,
boolean>`, which removed star/fork counts and caused UI bugs. Decide if you want
to keep counts visible or only icons: if counts are needed, revert `icons` to
`Record<string, number | string>`; if only icons are needed, keep
`Record<string, boolean>` but update `DisplayIcon` to not render values. Also,
update `page.tsx` and `Card.tsx` accordingly to maintain type safety and
consistent rendering.
Resolves #(put the issue number here)
Add the PR description here.