Limit visible Roadmap and Project Timeline items on About page#3080
Limit visible Roadmap and Project Timeline items on About page#30800xgaurav wants to merge 27 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors ToggleableList to a generic render-prop API and makes ShowMoreButton controlled; updates About page to limit Story (3) and Project Timeline (4) visible items using ToggleableList; updates CardDetailsPage usages; and adapts unit tests and mocks to the new APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-21T12:21:32.372ZApplied to files:
🔇 Additional comments (4)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/app/about/page.tsx (2)
279-298: Missing expand/collapse implementation for Project Timeline.According to the PR objectives, the "Project Timeline" section should display only the first 6 items by default with a "Show more/Show less" toggle. Currently, all timeline items are displayed without limiting.
The
timelineExpandedstate (declared on line 66) should be used here to implement the same expand/collapse pattern as the "Our Story" section.🔎 Proposed implementation following the "Our Story" pattern
<SecondaryCard icon={FaClock} title={<AnchorTitle title="Project Timeline" />}> <div className="space-y-6"> - {[...projectTimeline].reverse().map((milestone, index) => ( + {[...projectTimeline].reverse() + .slice(0, timelineExpanded ? projectTimeline.length : 6) + .map((milestone, index) => ( <div key={`${milestone.year}-${milestone.title}`} className="relative pl-10"> {index !== projectTimeline.length - 1 && ( <div className="absolute top-5 left-[5px] h-full w-0.5 bg-gray-400"></div> )} <div aria-hidden="true" className="absolute top-2.5 left-0 h-3 w-3 rounded-full bg-gray-400" ></div> <div> <h3 className="text-lg font-semibold text-blue-400">{milestone.title}</h3> <h4 className="mb-1 font-medium text-gray-400">{milestone.year}</h4> <p className="text-gray-600 dark:text-gray-300">{milestone.description}</p> </div> </div> ))} </div> + + {projectTimeline.length > 6 && ( + <button + onClick={() => setTimelineExpanded(!timelineExpanded)} + className="mt-2 flex items-center bg-transparent px-0 text-blue-400" + > + {timelineExpanded ? "Show less" : "Show more"} + </button> + )} </SecondaryCard>Note: You may need to adjust the connecting line logic (line 283) when items are hidden, as the conditional check uses
projectTimeline.lengthwhich should account for the visible items only.
257-298: Reuse the ShowMoreButton component instead of manual implementation.The codebase has an established
ShowMoreButtoncomponent used consistently inTopContributorsList,RepositoryCard, andMenteeContributorsListto handle expand/collapse patterns. The current implementation (lines 257-298) manually reinvents this pattern with custom state and onClick handlers, contradicting the PR objective to reuse existing UI patterns.Replace the manual button implementation with
<ShowMoreButton onToggle={() => setRoadmapExpanded(!roadmapExpanded)} />to maintain consistency and avoid duplicate logic.
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
257-277: Implementation looks good, but improve key generation.The expand/collapse logic for "Our Story" is implemented correctly. However, the key generation on line 261 could produce duplicate keys if multiple story items start with the same text.
🔎 Safer key generation using index
- {projectStory - .slice(0, roadmapExpanded ? projectStory.length : 3) - .map((text) => ( + {projectStory + .slice(0, storyExpanded ? projectStory.length : 3) + .map((text, index) => ( <div - key={`story-${text.substring(0, 50).replaceAll(' ', '-')}`} + key={`story-${index}`} className="mb-4" > <div> <Markdown content={text} /> </div> </div> ))} {projectStory.length > 3 && ( <button - onClick={() => setRoadmapExpanded(!roadmapExpanded)} + onClick={() => setStoryExpanded(!storyExpanded)} className="mt-2 flex items-center bg-transparent px-0 text-blue-400" > - {roadmapExpanded ? "Show less" : "Show more"} + {storyExpanded ? "Show less" : "Show more"} </button> )}Note: This also incorporates the variable rename from the previous comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/about/page.tsx (1)
281-296: Fix the vertical line logic for collapsed timeline.The condition on line 283 checks against the original array length (
projectTimeline.length - 1), butindexcomes from the sliced array. When the timeline is collapsed (showing 6 items), the vertical line will incorrectly appear after the last visible item, creating a visual artifact suggesting more content below.🔎 Proposed fix
Extract the displayed timeline to a variable and check against its length:
<SecondaryCard icon={FaClock} title={<AnchorTitle title="Project Timeline" />}> <div className="space-y-6"> - {[...projectTimeline].reverse().slice(0, timelineExpanded ? projectTimeline.length : 6).map((milestone, index) => ( + {(() => { + const displayedTimeline = [...projectTimeline] + .reverse() + .slice(0, timelineExpanded ? projectTimeline.length : 6); + return displayedTimeline.map((milestone, index) => ( <div key={`${milestone.year}-${milestone.title}`} className="relative pl-10"> - {index !== projectTimeline.length - 1 && ( + {index !== displayedTimeline.length - 1 && ( <div className="absolute top-5 left-[5px] h-full w-0.5 bg-gray-400"></div> )} <div aria-hidden="true" className="absolute top-2.5 left-0 h-3 w-3 rounded-full bg-gray-400" ></div> <div> <h3 className="text-lg font-semibold text-blue-400">{milestone.title}</h3> <h4 className="mb-1 font-medium text-gray-400">{milestone.year}</h4> <p className="text-gray-600 dark:text-gray-300">{milestone.description}</p> </div> </div> - ))} + )); + })()} </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (2)
projectStory(56-60)projectTimeline(61-138)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(5-20)
🔇 Additional comments (3)
frontend/src/app/about/page.tsx (3)
65-66: LGTM! State variables are properly initialized.The state variables are well-named, correctly initialized to
falseto show collapsed views by default, and align with the PR objectives.
257-278: LGTM! Our Story expand/collapse implementation is correct.The implementation correctly:
- Limits display to 3 items when collapsed
- Generates unique keys for each paragraph
- Shows the toggle button only when there are more than 3 items
- Updates button text based on state
298-305: LGTM! Project Timeline toggle button is correctly implemented.The toggle button:
- Shows only when there are more than 6 items
- Correctly toggles the state
- Displays appropriate text based on expansion state
- Uses consistent styling with the Our Story toggle
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
This is half-way done:
- You didn't run make check-test.
- You should reuse ShowMoreButton component.
- The milestones part is not implemented.
- You should add tests to your code.
|
I ran All checks passed except frontend-related steps that require Node 24 and a running backend (pnpm format, lint, and GraphQL codegen). These failed locally due to environment constraints, but CI should have the correct Node version and backend setup to validate them. Please let me know if you’d like me to take any further action. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
This code still doesn't address my comments from #3080 (review)
You didn't implement the milestones requirement and I don't see any tests.
|
Please also resolve the conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ToggleableList.tsx (1)
24-29: Remove dead code:useRouterandhandleButtonClickare unused.After the refactor to use
renderItem, thehandleButtonClickfunction is never called, and consequentlyrouteris unused. Remove these along with theuseRouterimport on line 1.🔎 Proposed fix
Remove the import on line 1:
-import { useRouter } from 'next/navigation'Remove the dead code inside the component:
}) => { const [showAll, setShowAll] = useState(false) - const router = useRouter() const toggleShowAll = () => setShowAll(!showAll) - const handleButtonClick = ({ item }: { item: string }) => { - router.push(`/projects?q=${encodeURIComponent(item)}`) - } return (
🧹 Nitpick comments (1)
frontend/src/components/ToggleableList.tsx (1)
40-43: Ensure callers provide keys inrenderItem.The
.map(renderItem)pattern is correct, but callers must return elements with uniquekeyprops to avoid React warnings. Consider adding a brief JSDoc comment on therenderItemprop to document this expectation:/** Must return a React element with a unique `key` prop */ renderItem: (item: T, index: number, items: T[]) => React.ReactNode
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ToggleableList.tsx
🔇 Additional comments (1)
frontend/src/components/ToggleableList.tsx (1)
8-21: Clean generic refactoring.The generic type parameter
<T,>with trailing comma is correct TypeScript syntax to disambiguate from JSX, and therenderItemcallback signature is well-typed. This makes the component properly reusable for different item types.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @frontend/__tests__/unit/pages/About.test.tsx:
- Around line 684-691: The test in About.test.tsx asserts at most 6 milestones
but the About component (frontend/src/app/about/page.tsx) is rendered with
limit={4} and the jest.mock('utils/aboutData', ...) used in the test only
contains 2 timeline items, so update the test and mocks to align with the
implementation: change the expectation in the test to expect <= 4 (or update the
component to use limit={6} if 6 is the intended behavior), and expand the mocked
projectTimeline inside the jest.mock('utils/aboutData', ...) to include at least
5–7 items so the limit logic is actually exercised; refer to the About component
rendering in the test and the limit prop in page.tsx when making the change.
In @frontend/src/app/about/page.tsx:
- Around line 260-262: The Timeline limit is inconsistent with the PR objectives
(default should be first 6 items) — update the ToggleableList usage to set limit
to 6 (change the prop on the ToggleableList instance that passes
items={[...projectTimeline].reverse()}), and also update any related
documentation or PR description if you intentionally want to keep a different
default; ensure the ToggleableList prop name limit is the one you adjust so the
component shows six items by default.
- Around line 265-280: The connector visibility logic uses the full items array
length instead of the visible slice, causing the last visible item to still
render a connector; update the ToggleableList usage so the renderItem callback
is passed visibleItems.length (not items.length) as the third parameter, and
adjust the renderItem in this file (the anonymous renderItem that references
index !== items.length - 1) to compare index !== visibleCount - 1 (or use the
passed visible length) so the connector is hidden for the final visible
milestone.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/unit/components/ToggleableList.test.tsxfrontend/__tests__/unit/pages/About.test.tsxfrontend/src/app/about/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ToggleableList.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/CardDetailsPage.tsx
- frontend/tests/unit/components/ToggleableList.test.tsx
- frontend/src/components/ToggleableList.tsx
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@0xgaurav Could you address new Issues found by Sonar that were introduced in this pr?
Also please address code rabbit comments. You're required to address all comments from Rabbit and get it's approval before we (leaders) review.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @frontend/__tests__/unit/pages/About.test.tsx:
- Line 696: The test currently uses screen.findAllByText(/20\d{2}/) which
matches any year on the page; scope the lookup to the Project Timeline container
instead (e.g., locate the timeline element via a role/label or data-testid and
then use within(timelineElement).findAllByText(/20\d{2}/)) so milestoneYears
only includes years in the Project Timeline section; update the test to get the
timeline element (e.g., via screen.getByRole or screen.getByTestId) and call
within(...) around the existing findAllByText call.
- Around line 692-699: The test, its comment, and the implementation disagree on
the timeline item limit: About.test.tsx names "6 items by default" but asserts
<=4 while page.tsx sets limit={4}; update the implementation and test to be
consistent with PR #3057 by changing the timeline component's prop from
limit={4} to limit={6} in page.tsx (the component rendering the Project
Timeline) and adjust the unit test in About.test.tsx to expect
milestoneYears.length toBeLessThanOrEqual(6) and update the test comment to
"limits project timeline milestones to 6 items by default".
In @frontend/src/app/about/page.tsx:
- Around line 260-262: The ToggleableList is currently rendered with limit={4}
which conflicts with PR objective #3057 and the test expecting 6 items; update
the limit prop on the ToggleableList instantiation (the component that passes
items={[...projectTimeline].reverse()}) to limit={6} so the default shows the
first six timeline items instead of four.
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
248-258: Consider using ToggleableList for consistency.While the current slice + ShowMoreButton approach works correctly, you could use
ToggleableListhere as well (similar to the timeline section) for consistency across the codebase. This would centralize the show more/less logic.♻️ Optional refactor using ToggleableList
- <SecondaryCard icon={FaScroll} title={<AnchorTitle title="Our Story" />}> - {projectStory.slice(0, storyExpanded ? projectStory.length : 3).map((text) => ( - <div key={`story-${text.substring(0, 50).replaceAll(' ', '-')}`} className="mb-4"> - <div> - <Markdown content={text} /> - </div> - </div> - ))} - - {projectStory.length > 3 && ( - <ShowMoreButton onToggle={() => setStoryExpanded(!storyExpanded)} /> - )} - </SecondaryCard> + <SecondaryCard icon={FaScroll} title={<AnchorTitle title="Our Story" />}> + <ToggleableList + items={projectStory} + limit={3} + renderItem={(text) => ( + <div className="mb-4"> + <Markdown content={text} /> + </div> + )} + /> + </SecondaryCard>This would also allow you to remove the
storyExpandedstate (lines 66-67).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/About.test.tsxfrontend/src/app/about/page.tsxfrontend/src/components/ToggleableList.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/pages/About.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/About.test.tsx
🧬 Code graph analysis (2)
frontend/src/components/ToggleableList.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/app/about/page.tsx (3)
frontend/src/utils/aboutData.ts (2)
projectStory(56-60)projectTimeline(61-138)backend/apps/slack/models/message.py (1)
text(83-85)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(5-20)
🔇 Additional comments (6)
frontend/__tests__/unit/pages/About.test.tsx (1)
130-133: Good addition of explicit MarkdownWrapper mock.The explicit mock for MarkdownWrapper helps isolate the component behavior in tests and provides consistent rendering across test scenarios.
frontend/src/app/about/page.tsx (2)
248-258: "Our Story" section correctly implements the 3-item limit.The implementation properly limits the story paragraphs to 3 items and shows the ShowMoreButton only when there are more than 3 items, which aligns with PR objective #3057.
8-8: LGTM: Clean integration of new components and state.The imports and state management for the Show More/Less functionality are properly structured and integrate well with the existing component.
Also applies to: 32-34, 66-67
frontend/src/components/ToggleableList.tsx (3)
6-13: Excellent refactor to a generic, reusable component.The migration to TypeScript generics with a
renderItemcallback is a well-designed improvement that:
- Provides type safety for any item type
- Enables flexible, custom rendering per use case
- Maintains the existing Show More/Less behavior
- Makes the component highly reusable across the codebase
The API design is clean and intuitive, with
visibleCountparameter inrenderItemallowing renderers to adjust based on visible items (e.g., for connectors in timeline views).Also applies to: 15-22
41-41: Using index as key is acceptable here.While using
key={index}is generally discouraged in React, it's acceptable in this case because:
- Items don't have reordering operations
- The component is display-only (no editable state per item)
- Items are sliced from a stable source array
If items ever have unique IDs in the future, consider updating consumers to pass a
getItemKeyprop for better key stability.
29-36: Good: label is now optional for flexible usage.Making the label optional allows parent components to provide their own title structure (like
SecondaryCardin the About page), which improves component composition flexibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/__tests__/unit/pages/About.test.tsx:
- Around line 692-697: The test "limits project timeline milestones to 4 items
by default" and the ToggleableList usage in about/page.tsx are inconsistent with
the PR objective of showing 6 items by default; update the ToggleableList
instantiation in about/page.tsx to use limit={6} (or the desired canonical
value) and modify the unit test in frontend/__tests__/unit/pages/About.test.tsx
to assert milestones.length isLessThanOrEqual(6) so both the implementation
(ToggleableList limit) and the test (limits project timeline milestones to 6
items by default) match the PR objective.
In @frontend/src/app/about/page.tsx:
- Around line 260-285: The Project Timeline is missing its header and wrapper
because ToggleableList is rendered without a label/icon like other sections; fix
by either passing a label="Project Timeline" and appropriate icon prop into the
ToggleableList invocation (so it renders the heading consistently) or wrap the
ToggleableList inside the same SecondaryCard + AnchorTitle structure used
elsewhere (use SecondaryCard and AnchorTitle around ToggleableList) to restore
styling and the section header; update the JSX around ToggleableList accordingly
(referencing ToggleableList, SecondaryCard, and AnchorTitle).
🧹 Nitpick comments (2)
frontend/src/app/about/page.tsx (1)
8-9: Consider combining useState and useEffect imports.Minor style suggestion: these can be combined into a single import statement.
Suggested change
-import { useState } from 'react' -import { useEffect } from 'react' +import { useState, useEffect } from 'react'frontend/src/components/ToggleableList.tsx (1)
40-48: Usingindexas key may cause rendering issues.Using array index as
keycan lead to incorrect component reconciliation when items are reordered, filtered, or when list contents change. Since items are generic typeT, consider requiring anidfield or accepting akeyExtractorprop.Suggested approach with keyExtractor
type ToggleableListProps<T> = { items: T[] label?: React.ReactNode icon?: IconType limit?: number isDisabled?: boolean renderItem: (item: T, index: number, visibleCount: number) => React.ReactNode + keyExtractor?: (item: T, index: number) => string | number } const ToggleableList = <T,>({ items, label, icon, limit = 10, isDisabled = false, renderItem, + keyExtractor = (_, index) => index, }: ToggleableListProps<T>) => { // ... {visibleItems.map((item, index) => ( <div - key={index} + key={keyExtractor(item, index)} aria-disabled={isDisabled}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/About.test.tsxfrontend/src/app/about/page.tsxfrontend/src/components/ToggleableList.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/ToggleableList.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (2)
projectStory(56-60)projectTimeline(61-138)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(5-20)
🔇 Additional comments (9)
frontend/__tests__/unit/pages/About.test.tsx (3)
69-80: LGTM!Mock data for
projectStoryandprojectTimelineis properly structured to support testing the show more/less functionality. The 6 timeline items provide enough data to test the limit behavior.
129-133: LGTM!The MarkdownWrapper mock correctly isolates the component by rendering content in a testable way with a
data-testidattribute.
170-205: LGTM!The ShowMoreButton mock correctly implements toggle state behavior, matching the real component's interface with
onTogglecallback and expanded/collapsed UI states.frontend/src/app/about/page.tsx (2)
248-258: LGTM!The "Our Story" section correctly limits to 3 items by default and conditionally renders the ShowMoreButton when there are more than 3 items. The implementation aligns with PR objectives.
66-67: LGTM!The
storyExpandedstate is properly initialized and scoped to control the "Our Story" section expansion.frontend/src/components/ToggleableList.tsx (4)
6-13: LGTM!The generic type definition is well-structured. The
renderItemcallback providingvisibleCountis a thoughtful addition that enables rendering of connectors between items (as used in the timeline).
15-26: LGTM!The generic component signature and visible items logic are correctly implemented. The slice operation properly limits items when
showAllis false.
30-37: LGTM!The optional label rendering with icon support provides good flexibility. The conditional rendering prevents empty headers when no label is provided.
51-53: LGTM!The ShowMoreButton is correctly hidden when
isDisabledis true or when all items fit within the limit, preventing unnecessary UI elements.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/src/app/about/page.tsx:
- Around line 259-278: The Project Timeline ToggleableList is missing the
section heading and icon—pass a human-readable label and an icon component to
the ToggleableList props (e.g., label="Project Timeline" and icon={FaClock} or
another timeline icon) and import the chosen icon from react-icons (e.g., import
{ FaClock } from 'react-icons/fa'); update the ToggleableList usage to include
these props so the section header renders consistently with other sections.
In @frontend/src/components/ToggleableList.tsx:
- Around line 40-48: The current ToggleableList uses JSON.stringify(item) as the
React key in visibleItems.map, which is unstable and inefficient; add a
keyExtractor prop to the ToggleableList component (e.g., keyExtractor: (item: T,
index: number) => string) and use it to compute the key inside the map instead
of JSON.stringify, falling back to index only if no keyExtractor is provided;
update the ToggleableList props/interface and component signature and replace
the key generation in the map with a call to keyExtractor(item, index) so keys
are stable and performant.
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
247-257: Consider using ToggleableList for consistency.The manual expand/collapse implementation works correctly and limits "Our Story" to 3 items as intended. However, since you've already refactored the Project Timeline to use the new
ToggleableListcomponent, consider applying the same pattern here to maintain consistency and reduce duplication.♻️ Proposed refactor using ToggleableList
- <SecondaryCard icon={FaScroll} title={<AnchorTitle title="Our Story" />}> - {projectStory.slice(0, storyExpanded ? projectStory.length : 3).map((text) => ( - <div key={`story-${text.substring(0, 50).replaceAll(' ', '-')}`} className="mb-4"> - <div> - <Markdown content={text} /> - </div> - </div> - ))} - - {projectStory.length > 3 && ( - <ShowMoreButton onToggle={() => setStoryExpanded(!storyExpanded)} /> - )} - </SecondaryCard> + <ToggleableList + items={projectStory} + label={<AnchorTitle title="Our Story" />} + icon={FaScroll} + limit={3} + renderItem={(text) => ( + <div className="mb-4 w-full"> + <Markdown content={text} /> + </div> + )} + />This would eliminate the
storyExpandedstate and make both sections use the same component pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/About.test.tsxfrontend/src/app/about/page.tsxfrontend/src/components/ToggleableList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/pages/About.test.tsx
🔇 Additional comments (8)
frontend/src/app/about/page.tsx (4)
8-8: LGTM: Import additions are correct.The new imports for
useState,ShowMoreButton, andToggleableListare properly structured and align with the refactoring objectives.Also applies to: 31-31, 33-33
65-65: LGTM: State initialization is correct.The
storyExpandedstate is properly initialized withfalseto show collapsed content by default.
261-261: Verify the timeline item limit matches requirements.The implementation uses a limit of 4 items, but the linked issue #3057 requested showing the first 6 Project Timeline items by default. Please confirm whether 4 items is the intended behavior or if this should be increased to 6 to match the original requirements.
262-277: LGTM: Timeline rendering with connectors is well-implemented.The
renderItemcallback correctly handles:
- Dynamic connector visibility based on
visibleCount(Line 264)- Proper absolute positioning for both the connector line and timeline dot
- Appropriate
aria-hidden="true"for decorative elementsThe connector logic will correctly adjust when toggling between collapsed and expanded states.
frontend/src/components/ToggleableList.tsx (4)
1-13: LGTM: Generic type definition is well-designed.The refactoring to make
ToggleableListgeneric is clean and type-safe. TherenderItemcallback signature with(item: T, index: number, visibleCount: number)provides consumers with all necessary information for conditional rendering (as demonstrated in the timeline connector logic).
23-26: LGTM: Visibility state management is correct.The
showAllstate andvisibleItemscalculation cleanly handle the expand/collapse behavior. ExposingvisibleCountto therenderItemcallback is a thoughtful design that enables conditional rendering based on the current visible set.
30-37: LGTM: Optional label rendering is implemented correctly.The conditional rendering of the header section allows the component to be flexible—consumers can omit the label for use cases that don't need a heading (though in practice, adding labels improves consistency and accessibility).
51-53: LGTM: ShowMoreButton rendering is correct.The conditional rendering logic correctly shows the toggle button only when there are more items than the limit and the list is not disabled. The functional state update
(v) => !vis the proper pattern for toggling boolean state.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @frontend/src/components/ToggleableList.tsx:
- Around line 32-39: The h2 in ToggleableList contains a block-level div which
is invalid HTML; replace the inner div with a phrasing element (e.g., a span) or
move the flex container outside the h2 so the heading only contains phrasing
content. Update the JSX around the label rendering (the h2 and its children that
reference IconWrapper and {label}) to use a span with className="flex
items-center" (or move that span/div outside the h2) and ensure IconWrapper and
the label remain correctly nested and styled.
- Around line 6-14: The component allows a possibly-negative or NaN "limit"
which makes items.slice(0, limit) and the visible-toggle check items.length >
limit behave incorrectly; clamp and validate limit inside the ToggleableList
logic (e.g., normalize in the component that consumes ToggleableListProps.limit)
by coercing to a safe integer: if limit is missing/NaN/<=0 treat as 0, and if
limit > items.length set to items.length, then use that normalizedLimit for
items.slice(0, normalizedLimit) and for computing visibleCount/hasMore; update
references to limit, items.slice, and any computation that passes visibleCount
to renderItem accordingly.
- Around line 53-55: ToggleableList is toggling visibility with setShowAll but
ShowMoreButton keeps its own internal isExpanded state, causing possible
mismatch on external re-renders; update ToggleableList to pass the current
showAll state into ShowMoreButton (e.g., expanded={showAll} or
showAll={showAll}) and modify ShowMoreButton to accept and use that prop instead
of its internal state (remove or fallback its internal isExpanded), keeping the
onToggle callback as-is so the button text/icon stays in sync with
ToggleableList's showAll state.
- Around line 41-50: The wrapper around each rendered item in ToggleableList
currently uses aria-disabled and pointer-events-none but leaves interactive
descendants focusable; when isDisabled is true, add the inert attribute on the
item wrapper (so descendants are unfocusable/uninteractive) and only render
aria-disabled when isDisabled is true (don't emit aria-disabled="false"); keep
the existing className toggle (pointer-events-none) and preserve the key logic
(keyExtractor/item index) and renderItem/visibleCount usage.
🧹 Nitpick comments (1)
frontend/src/components/ToggleableList.tsx (1)
42-45: Strongly prefer stable keys; index fallback is risky.If items can reorder (sort/filter, reverse, new data),
indexkeys can cause remounting and state bugs insiderenderItem. Consider makingkeyExtractorrequired (or at least defaulting to index via a localgetKeyto make the fallback explicit and easy to change later).Low-impact cleanup
- {visibleItems.map((item, index) => ( + const getKey = keyExtractor ?? ((_: T, i: number) => i) + {visibleItems.map((item, index) => ( <div - key={keyExtractor ? keyExtractor(item, index) : index} + key={getKey(item, index)} aria-disabled={isDisabled} className={isDisabled ? 'pointer-events-none opacity-60' : undefined} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/about/page.tsxfrontend/src/components/ToggleableList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/about/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
Applied to files:
frontend/src/components/ToggleableList.tsx
🧬 Code graph analysis (1)
frontend/src/components/ToggleableList.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/ToggleableList.tsx (2)
6-14: Consider exporting the props type.The
ToggleableListProps<T>type definition is well-structured. Consider exporting it for consumers who may want to reference it when creating wrapper components or for documentation purposes.📤 Export the props type
-type ToggleableListProps<T> = { +export type ToggleableListProps<T> = { items: T[] label?: React.ReactNode icon?: IconType limit?: number isDisabled?: boolean keyExtractor?: (item: T, index: number) => string | number renderItem: (item: T, index: number, visibleCount: number) => React.ReactNode }
56-56: Consider removing redundantaria-disabledattribute.The
aria-disabledattribute on adivis non-standard and potentially confusing. Since theinertattribute (set via theuseEffecton Line 32-40) already disables all interactions and provides the necessary accessibility semantics, this attribute is redundant.🧹 Remove aria-disabled
<div ref={containerRef} className="flex flex-wrap gap-2" - aria-disabled={isDisabled} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/app/about/page.tsxfrontend/src/components/ShowMoreButton.tsxfrontend/src/components/ToggleableList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/about/page.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
Applied to files:
frontend/src/components/ToggleableList.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ToggleableList.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ToggleableList.tsx
🧬 Code graph analysis (2)
frontend/src/components/ToggleableList.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/ShowMoreButton.tsx (1)
frontend/src/types/button.ts (1)
Button(5-11)
🔇 Additional comments (7)
frontend/src/components/ShowMoreButton.tsx (2)
4-9: LGTM! Clean controlled component refactor.The conversion from uncontrolled to controlled component is well-executed. The type definition is clear, and the component signature properly accepts both
expandedandonToggleprops, enabling parent components to manage state.
15-26: LGTM! Proper event handling and rendering.The button correctly uses
onPress(per HeroUI API) and conditionally renders based on theexpandedprop. Accessibility is preserved witharia-hiddenicons and proper focus styles.frontend/src/components/ToggleableList.tsx (5)
16-30: LGTM! Solid generic component foundation.The generic component declaration and visibility logic are well-implemented. The
safeLimitcalculation properly guards against invalid limits, and the slicing logic correctly implements the show more/less behavior.
32-40: Verify browser support requirements for theinertattribute.The
inertattribute is a modern accessibility feature that disables all interactions within an element. While it's well-supported in current browsers (Chrome 102+, Firefox 112+, Safari 15.5+), confirm this aligns with the project's browser support requirements. If older browsers need support, consider a polyfill or alternative approach.
44-51: LGTM! Clean header implementation.The optional header rendering is well-structured with proper semantics and icon integration.
68-73: LGTM! Proper ShowMoreButton integration.The conditional rendering logic is correct, and the ShowMoreButton is properly wired with the controlled
expandedprop andonTogglecallback.
63-63: ThevisibleCountparameter is actively used and should not be removed.The parameter is necessary for at least one consumer: the milestone timeline in
frontend/src/app/about/page.tsxuses it to conditionally render a vertical connector line (by checkingindex !== visibleCount - 1). Removing it would break this functionality.While most simple list items don't require this information, the parameter enables consumers to implement conditional rendering based on visibility position, which is a valid use case.
|
|
@0xgaurav Hi!
Keeping in mind that deadline for this issue was on Dec 31 - I'm afraid I have to close this PR and reassign the issue to someone else. |
4 participants
Proposed change
Resolves #3057
This PR improves the readability of the About page by limiting the number of items shown by default:
Note:
The About page depends on seeded project metadata. When running locally, the page may show a 404 if
make load-dataandmake index-dataare not executed. The UI logic works as expected once data is available.Checklist
make check-testlocally and all tests passed