Revamp About Page: Thematic Section Enhancements, Dynamic Content Structuring, and Comprehensive Testing Integration #1584
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe About page was extensively restructured to introduce new sections: mission, key features, target audience, ways to get involved, and a project history timeline. Existing tests were updated and expanded to validate the presence and content of these new sections, with both unit and end-to-end coverage reflecting the revised page structure and content. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes identified. Possibly related PRs
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: 2
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
227-228: Consider more specific selectors for year indicators.The current approach of checking for text "23" and "24" could be fragile if these numbers appear elsewhere in the page. Consider using more specific selectors or data attributes.
- // Check for year indicators - expect(screen.getAllByText('23')).toHaveLength(2) // 2023 entries - expect(screen.getAllByText('24')).toHaveLength(3) // 2024 entries + // Check for year indicators within the timeline + const timelineSection = screen.getByText('History').closest('div') + const yearIndicators = within(timelineSection).getAllByText(/^\d{2}$/) + const year23Count = yearIndicators.filter(el => el.textContent === '23').length + const year24Count = yearIndicators.filter(el => el.textContent === '24').length + expect(year23Count).toBe(2) // 2023 entries + expect(year24Count).toBe(3) // 2024 entries
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/e2e/pages/About.spec.ts(1 hunks)frontend/__tests__/unit/pages/About.test.tsx(5 hunks)frontend/src/app/about/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/about/page.tsx (1)
frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)
🔇 Additional comments (5)
frontend/__tests__/e2e/pages/About.spec.ts (1)
39-39: Good practice: Using exact match for heading assertion.The addition of
exact: trueensures the test specifically targets the "History" heading without matching partial text. This prevents false positives and aligns well with the restructured About page content.frontend/__tests__/unit/pages/About.test.tsx (2)
46-100: LGTM! Mock data aligns with new technologies displayed.The expanded mock technology data correctly includes all the new tools mentioned in the About page restructuring.
165-211: Well-structured tests for new content sections.The new test cases comprehensively cover the mission, key features, and get involved sections with appropriate assertions for both headings and content.
frontend/src/app/about/page.tsx (2)
337-364: Excellent timeline implementation!The new project history timeline with visual indicators, gradient line, and year badges provides a much better user experience compared to the previous static content. The implementation is clean and accessible.
332-332:✅ Verification successful
Verify Markdown component sanitization for external links.
The Markdown component is being used to render content that includes links to external resources. Ensure the component properly sanitizes content and handles external links securely.
🏁 Script executed:
#!/bin/bash # Description: Verify the MarkdownWrapper component's security configuration # Check the MarkdownWrapper implementation for proper sanitization cat frontend/src/components/MarkdownWrapper.tsx # Search for any security-related configurations rg -A 5 "DOMPurify|sanitize|linkify" frontend/src/components/Length of output: 1727
Markdown content is properly sanitized
- The
markdown-itparser is initialized withhtml: false, preventing raw HTML in the source.- All rendered output is passed through
DOMPurify.sanitize(), neutralizing any malicious markup.- Linkification (
linkify: true) only converts plain URLs to safe<a>tags without introducing XSS vectors.No further action needed here—sanitization is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/utils/aboutData.ts (3)
11-13: Consider a clearer key thanwhoItsFor
whoItsForis a little awkward to read and stylistically differs from the other nouns (mission,keyFeatures, …).
audienceortargetAudiencewould be easier to scan. Rename here and where it’s consumed.
50-78: Storeyearas a number, not a string
yearis a numeric concept; keeping it as a number avoids ad-hoc parsing when you need to sort or perform arithmetic.- year: '2024', + year: 2024,Repeat for every history item.
82-86: Giveleadersan explicit typeWithout a declaration TS infers
leadersas{ [x: string]: string }, which is fine but implicit.
A quickRecord<string, string>(or a dedicated enum/union of known handles if you prefer) makes intent crystal clear.-export const leaders = { +export const leaders: Record<string, string> = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/About.test.tsx(6 hunks)frontend/src/app/about/page.tsx(7 hunks)frontend/src/utils/aboutData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/pages/About.test.tsx
- frontend/src/app/about/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/app/about/page.tsx (1)
79-87:⚠️ Potential issueLoading gate misses
loadingflags & may show “Data not found” prematurely
useQueryalways returns an object, soprojectMetadataResponse/topContributorsResponseare never falsy.
During the first renderloadingis stilltrue, butisLoadingevaluates tofalse, causing the component to drop into the 404/Data not found branch for a split-second and then re-render. This flashes the error UI and breaks SSR.-const isLoading = - !projectMetadataResponse || - !topContributorsResponse || - (projectMetadataRequestError && !projectMetadata) || - (topContributorsRequestError && !topContributors) +const isLoading = + projectMetadataResponse?.loading || + topContributorsResponse?.loading || + (projectMetadataRequestError && !projectMetadata) || + (topContributorsRequestError && !topContributors)
🧹 Nitpick comments (3)
frontend/src/app/about/page.tsx (2)
296-297: Don’t always append “+” to stats – misleading for exact countsIf a repository has exactly 0 forks or 1 star the UI will still show
0+,1+.
Only add the suffix when the number is rounded/abbreviated.-<AnimatedCounter end={stat.value} duration={2} />+ +<AnimatedCounter end={stat.value} duration={2} /> +{stat.value > 0 && '+'}
308-316: Surface GraphQL errors consistently viahandleAppErrorInside
LeaderData, GraphQL errors are rendered as plain text while other errors are piped through the global toaster. For UX consistency delegate to the same handler:-if (error) return <p>Error loading {username}'s data</p> +if (error) { + handleAppError(error) + return <p>Error loading {username}'s data</p> +}frontend/__tests__/unit/pages/About.test.tsx (1)
237-268: Repetitive render boilerplate – extract a helperEvery test repeats:
await act(async () => { render(<About />) })Create a
renderAbouthelper inbeforeEach(or use RTL’srender()inside a wrapper). This shortens each test and centralises default setup.No code diff provided as change spans multiple blocks.
Also applies to: 251-283, 285-297
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/e2e/pages/About.spec.ts(2 hunks)frontend/__tests__/unit/pages/About.test.tsx(6 hunks)frontend/src/app/about/page.tsx(8 hunks)frontend/src/utils/aboutData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/e2e/pages/About.spec.ts
- frontend/src/utils/aboutData.ts
🔇 Additional comments (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
298-301:⚠️ Potential issueFuzzy text match makes the test brittle – use exact match
getAllByText('23')also matches “2023”, so the expected length will break if the copy changes. Make the query exact or target the badge element instead.-expect(screen.getAllByText('23')).toHaveLength(2) -expect(screen.getAllByText('24')).toHaveLength(3) +expect(screen.getAllByText('23', { exact: true })).toHaveLength(2) +expect(screen.getAllByText('24', { exact: true })).toHaveLength(3)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/app/about/page.tsx (1)
121-129: 🛠️ Refactor suggestionArray index still used as React key – use a stable identifier instead.
Using
indexbreaks reconciliation ifkeyFeaturesorder changes (e.g., translations, A/B tests).
Use the feature title (unique) or add an explicitid.- {keyFeatures.map((feature, index) => ( - <div key={index} className="rounded-lg bg-gray-100 p-4 dark:bg-gray-700"> + {keyFeatures.map((feature) => ( + <div key={feature.title} className="rounded-lg bg-gray-100 p-4 dark:bg-gray-700">
🧹 Nitpick comments (3)
frontend/src/app/about/page.tsx (3)
249-254: Minor: replace index with the string itself for list keys.Each entry in
getInvolvedContent.waysis already a unique string; using it avoids unnecessary DOM churn.- {getInvolvedContent.ways.map((way, index) => ( - <li key={index} className="flex items-start gap-2"> + {getInvolvedContent.ways.map((way) => ( + <li key={way} className="flex items-start gap-2">
267-286: Use deterministic key for timeline milestones.Relying on
indexcan cause flicker if history order is updated. Combineyear&titlefor a stable key.- {projectHistory.map((milestone, index) => ( - <div key={index} className="relative flex gap-6"> + {projectHistory.map((milestone) => ( + <div key={`${milestone.year}-${milestone.title}`} className="relative flex gap-6">
301-302: Consistency: consider dropping the hard-coded “+” after the animated count.
AnimatedCounteralready millifies values (e.g.,1.2K). Appending+yields1.2K+, which may be misleading when the exact count is shown (e.g.,73+). Decide on either exact or “at least” semantics and apply consistently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/e2e/pages/About.spec.ts(2 hunks)frontend/src/app/about/page.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/pages/About.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/about/page.tsx (3)
frontend/src/utils/aboutData.ts (4)
missionContent(25-30)keyFeatures(32-52)getInvolvedContent(54-66)projectHistory(68-97)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/AnimatedCounter.tsx (1)
AnimatedCounter(10-35)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)
11-15:❓ Verification inconclusive
Verify that
faTimeline& other newly-added icons exist in the installed free icon pack.
faTimeline(and, depending on the Font Awesome version,faHandsHelping) are not part of the free solid set in some releases. If they are missing at runtime the page will crash with: “icon not found”.
🏁 Script executed:
#!/bin/bash # Check if the requested icons exist in @fortawesome/free-solid-svg-icons grep -R --line-number -e "faTimeline" -e "faHandsHelping" "$(npm root)/@fortawesome" || echo "Icon(s) not found"Length of output: 235
Verify newly added FontAwesome icons exist in your free‐solid package
Ensure you’ve installed your front-end dependencies and then confirm that both
faTimelineandfaHandsHelpingare exported by your@fortawesome/free-solid-svg-iconspackage. Missing icons will throw an “icon not found” error at runtime.• Locations to check
- frontend/src/app/about/page.tsx (lines 11–15): references to
faTimeline,faHandsHelping• Verification steps (run from project root):
# 1. Install dependencies (if you haven’t already) cd frontend npm install # 2. Confirm icon exports grep -R -n "faTimeline" node_modules/@fortawesome/free-solid-svg-icons || echo "❌ faTimeline not found" grep -R -n "faHandsHelping" node_modules/@fortawesome/free-solid-svg-icons || echo "❌ faHandsHelping not found"If either icon is missing, swap in a supported free icon or bump your FontAwesome version before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/app/about/page.tsx (1)
123-128: Array index still used as React key – re-introduces the reconciliation bug
The earlier review already flagged this; the fix seems to have been reverted.-{keyFeatures.map((feature, index) => ( - <div key={index} … +{keyFeatures.map((feature) => ( + <div key={feature.title} …
🧹 Nitpick comments (3)
frontend/src/app/about/page.tsx (3)
249-255: Minor: use a stable key forgetInvolvedContent.waysUsing the list item text as the key avoids potential reordering issues and removes the need for the
indexparam.-{getInvolvedContent.ways.map((way, index) => ( - <li key={index} … +{getInvolvedContent.ways.map((way) => ( + <li key={way} …
267-286: Timeline items keyed by array index
projectHistoryprovides bothtitleandyear; prefer a composite key for stability.-{projectHistory.map((milestone, index) => ( - <div key={index} … +{projectHistory.map((milestone) => ( + <div key={`${milestone.year}-${milestone.title}`} …
301-302:+suffix may contradict the new exact countersSince
AnimatedCounternow animates to the precisestat.value, appending “+” can mislead users (e.g., “42+ Stars”). Remove the suffix or change the counter to round down if the “+” is desired.-<AnimatedCounter end={stat.value} duration={2} />+ +<AnimatedCounter end={stat.value} duration={2} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/about/page.tsx (3)
frontend/src/utils/aboutData.ts (4)
missionContent(25-30)keyFeatures(32-52)getInvolvedContent(54-66)projectHistory(68-97)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/AnimatedCounter.tsx (1)
AnimatedCounter(10-35)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)
11-15:❓ Verification inconclusive
Verify that
faTimelineexists in the Free-Solid icon set
faTimelineis part of Font Awesome 6 Pro and is not available in the free solid bundle used elsewhere in the project. Importing an icon that is absent at runtime will crash the page with
icon.lookup === undefined.Run this check to confirm availability:
If
faTimelineis missing, switch to a free alternative (e.g.faBarsStaggered) or add the Pro package.
🏁 Script executed:
#!/bin/bash # List all exported icon names from @fortawesome/free-solid-svg-icons node - <<'JS' const { ...icons } = require('@fortawesome/free-solid-svg-icons') console.log(Object.keys(icons).filter(k => k.startsWith('fa'))) JSLength of output: 872
Verify that
faTimelineexists in the Free-Solid icon set
The automated script failed (module not installed locally), so please manually confirm thatfaTimelineis exported by@fortawesome/free-solid-svg-icons. Importing a non-existent icon at runtime will causeicon.lookup === undefinedand crash the page.If
faTimelineisn’t available in the free bundle:
- Swap it out for a free alternative (e.g.
faBarsStaggered)- Or install the Pro package (
@fortawesome/pro-solid-svg-icons)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/app/about/page.tsx (4)
111-119: Consider semantic<section>wrappers for top-level blocksThe Mission / Who-It’s-For cards are visually distinct but still reside in a generic
<div>.
Wrapping each major area in a semantic<section>(with anaria-labelledbypointing to theAnchorTitle) will slightly improve accessibility and document outline without changing layout.
123-131: Use a guaranteed-unique key forkeyFeaturesitems
key={${feature.title}-${feature.description.slice(0, 10)}}is better than an index, yet still risks collisions if two features share a title prefix or similar opening text.
If the data model allows it, expose an explicitid/slugfield and use that instead:- key={`${feature.title}-${feature.description.slice(0, 10)}`} + key={feature.id}
252-257: Minor: bullet list keys have the same collision risk
key={way.slice(0, 32)}can duplicate when two items start with the same phrase.
Either use the full string or (preferred) store anidalongside each entry inaboutData.ts.
265-295: Decorative timeline spine should be hidden from screen readersThe absolutely-positioned gradient line is purely visual.
Addaria-hidden="true"(orrole="presentation") to the element to avoid it being announced as an empty “separator”.-<div className="absolute bottom-0 left-4 top-8 w-0.5 bg-gradient-to-b ..."></div> +<div + aria-hidden="true" + className="absolute bottom-0 left-4 top-8 w-0.5 bg-gradient-to-b ..." +/>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(8 hunks)
🔇 Additional comments (2)
frontend/src/app/about/page.tsx (2)
11-16: Verify newly-imported FontAwesome icons exist in the bundled library
faHandsHelpingandfaTimelinewere historically PRO-only / renamed across FontAwesome major versions. If the project is still on@fortawesome/free-solid-svg-iconsv5.x or an early 6.x release, this import will break the build at runtime (undefinedicon).
Please double-check the installed package version and, if needed, upgrade or swap in the closest free alternative (e.g.faHandHoldingHeart,faStream).
30-37: 👍 Content extraction improves separation of concernsImporting
missionContent,keyFeatures, etc. fromutils/aboutDatakeeps the page component lean and easier to maintain. Nicely done.
|
@kasya any more update required in this pr? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/app/about/page.tsx (2)
123-132: Key generator still fragile.
key={${feature.title}-${feature.description.slice(0, 10)}}improves on the array-index key, yet it can still collide if different features share the same title/first-10-chars combination (translation variants, AB tests, etc.).- key={`${feature.title}-${feature.description.slice(0, 10)}`} + key={feature.id ?? feature.title}Ideally expose a stable
idinkeyFeaturesand use that here.
304-305: Literal"+"may double-count visual cue.
AnimatedCounteralready counts to the exact value; appending a literal plus (+) could render1234+, which might be misleading (implies “and more” while you’re showing the exact count).Options:
- Remove
+altogether if accuracy is intended.- If “and more” is desired, round the value (e.g., to
1.2K) and keep the+.Decision depends on UX, but current combo mixes semantics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/e2e/pages/About.spec.ts(3 hunks)frontend/__tests__/unit/pages/About.test.tsx(7 hunks)frontend/src/app/about/page.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/e2e/pages/About.spec.ts
- frontend/tests/unit/pages/About.test.tsx
rishyym0927
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/app/about/page.tsx (1)
3-17:⚠️ Potential issueBroken import block – file will not compile.
The first
import { …statement is missing the closing}and thefrom '@fortawesome/free-solid-svg-icons'clause, so every line that follows is parsed as garbage. The linter error you saw (expected "as" but instead found "{") comes from this. Merge the two blocks into one well-formed import:-import { - faCircleCheck, - faClock, - faUserGear, - faMapSigns, - faUsers, - faTools, - faArrowUpRightFromSquare, -import { - faBullseye, - faUserGroup, - faRocket, - faHandshake, // alternative to faHandsHelping - faHistory, // alternative to faTimeline -} from '@fortawesome/free-solid-svg-icons' +import { + faCircleCheck, + faClock, + faUserGear, + faMapSigns, + faUsers, + faTools, + faArrowUpRightFromSquare, + faBullseye, + faUserGroup, + faRocket, + faHandshake, // alternative to faHandsHelping + faHistory, // alternative to faTimeline +} from '@fortawesome/free-solid-svg-icons'This single change removes the syntax error and restores the intended imports.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: expected
asbut instead found{Remove {
(parse)
[error] 16-17: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 17-17: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 11-17: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
274-276: Guard against non-stringyeartype.
milestone.year.slice(-2)assumesyearis a string. It is today, but one accidental change to a number will explode. Coerce defensively:- {milestone.year.slice(-2)} + {String(milestone.year).slice(-2)}Tiny change, but eliminates a fragile runtime assumption.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/about/page.tsx (3)
frontend/src/utils/aboutData.ts (4)
missionContent(25-30)keyFeatures(32-52)getInvolvedContent(54-66)projectHistory(68-97)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/AnimatedCounter.tsx (1)
AnimatedCounter(10-35)
🪛 Biome (1.9.4)
frontend/src/app/about/page.tsx
[error] 11-11: expected as but instead found {
Remove {
(parse)
[error] 11-17: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/app/about/page.tsx (1)
86-91: 🛠️ Refactor suggestion
isLoading/ error-handling logic misses the “contributors query failed” case
topContributorsis initialised as[](truthy).
IftopContributorsRequestErroris non-null,!topContributorsisfalse, so the error path is skipped and the component renders with an empty contributors list, silently swallowing the failure.Consider simplifying the flags:
-const isLoading = - !projectMetadataResponse || - !topContributorsResponse || - (projectMetadataRequestError && !projectMetadata) || - (topContributorsRequestError && !topContributors) +const isLoading = !projectMetadataResponse || !topContributorsResponse + +if (projectMetadataRequestError) handleAppError(projectMetadataRequestError) +if (topContributorsRequestError) handleAppError(topContributorsRequestError)…and render a dedicated
<ErrorDisplay>when either error is present.
This prevents silent data loss and improves UX.
♻️ Duplicate comments (1)
frontend/src/app/about/page.tsx (1)
11-16:⚠️ Potential issueMissing & obsolete Font-Awesome imports – will crash at runtime
faHandshakeandfaHistoryare used below (lines 248 & 265) but are not imported, whilefaHandsHelpingandfaTimelineare no longer used (and don’t exist in the free-solid set). This results inReferenceError: faHandshake is not defined.-import { - faCircleCheck, - faClock, - faUserGear, - faMapSigns, - faUsers, - faTools, - faArrowUpRightFromSquare, - faBullseye, - faUserGroup, - faRocket, - faHandsHelping, - faTimeline, -} from '@fortawesome/free-solid-svg-icons' +import { + faCircleCheck, + faClock, + faUserGear, + faMapSigns, + faUsers, + faTools, + faArrowUpRightFromSquare, + faBullseye, + faUserGroup, + faRocket, + faHandshake, + faHistory, +} from '@fortawesome/free-solid-svg-icons'
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
124-127: Unstable React keys – risk of duplicate key warningsUsing a slice of
descriptionis fragile; two features with identical first 10 chars collide. Prefer a stable, intrinsic identifier.- key={`${feature.title}-${feature.description.slice(0, 10)}`} + key={feature.title}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(8 hunks)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)
270-275:milestone.year.slice()assumes string – possible runtime errorIf
yearcomes in as a number (e.g.,2020) you’ll getyear.slice is not a function.- {milestone.year.slice(-2)} + {String(milestone.year).slice(-2)}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/app/about/page.tsx (1)
296-306: 🛠️ Refactor suggestionAnimated counter can render “0+” for small projects.
Rounding down with
Math.floor(stat.value / 10) * 10makes counts below 10 display as0+, which looks broken.- <AnimatedCounter end={Math.floor(stat.value / 10) * 10} duration={2} />+ + <AnimatedCounter + end={stat.value < 10 ? stat.value : Math.floor(stat.value / 10) * 10} + duration={2} + />+This preserves the “rounded-with-plus” style while avoiding a misleading zero.
♻️ Duplicate comments (1)
frontend/src/app/about/page.tsx (1)
270-276: Repeat of earlier slice-on-possibly-numeric issue.
milestone.year.slice(-2)assumesyearis a string. A future data refactor that stores it as a number (2024) will crash this section (year.slice is not a function).Same concern was raised in the previous review but remains unfixed.
- {milestone.year.slice(-2)} + {String(milestone.year).slice(-2)}
🧹 Nitpick comments (1)
frontend/src/app/about/page.tsx (1)
252-257:keygeneration may still collide.Using
way.slice(0, 32)increases the risk of duplicate keys if the first 32 chars of two list items match. React will silently reuse the DOM node, causing subtle UI bugs.- <li key={way.slice(0, 32)} ... + <li key={way} ...Each string in
getInvolvedContent.waysis already unique and stable, so the full string is sufficient.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
missionContent(25-30)keyFeatures(32-52)getInvolvedContent(54-66)projectHistory(68-97)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)
11-16: Icon choice fixed – looks good.The previous build-breaking icons (
faTimeline,faHandsHelping) have been replaced with free-tier equivalents that exist in@fortawesome/free-solid-svg-icons. 👍
|
@kasya I have made all the necessary changes which were requested |
|
Hey @arkid15r ! Does the content require any changes? |
|
Hey @arkid15r I have fixed all the requested changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/utils/aboutData.ts (2)
16-21: Prefersatisfiesfor literal-type safety without redundant castsYou already have an explicit annotation (
MissionContent) and anas constcast.
as constfreezes the literal types, but the explicit annotation immediately widens them back tostring, so the benefit is lost.
Using thesatisfiesoperator lets you keep literal inference while still validating against the interface.-export const missionContent: MissionContent = { +export const missionContent = { mission: 'OWASP Nest is ...', whoItsFor: "OWASP Nest is designed for ...", -} as const +} as const satisfies MissionContent
96-165: Minor name capitalisation issues (“Typescript”, “PlayWright”)Small consistency nit: the official branding is “TypeScript” and “Playwright” (lower-case “w”). Helps avoid broken links / icon paths later.
- Typescript: { + TypeScript: { icon: '/images/icons/typescript.svg', url: 'https://www.typescriptlang.org/', }, ... - PlayWright: { + Playwright: { icon: '/images/icons/playwright.svg', url: 'https://playwright.dev/', },frontend/src/app/about/page.tsx (1)
254-258: Use the item itself, not the array index, as React key
indexas the key can cause incorrect reconciliation if the list ever re-orders.
Since everywaystring is unique, it can act as a stable key.- {getInvolvedContent.ways.map((way, index) => ( - <li key={index} className="flex items-start gap-2"> + {getInvolvedContent.ways.map((way) => ( + <li key={way} className="flex items-start gap-2">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/app/about/page.tsx(7 hunks)frontend/src/types/about.ts(1 hunks)frontend/src/utils/aboutData.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/types/about.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/utils/aboutData.ts (1)
frontend/src/types/about.ts (6)
MissionContent(18-21)KeyFeature(1-4)GetInvolved(12-16)ProjectHistory(6-10)Leaders(23-25)TechnologySection(27-35)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
missionContent(16-21)keyFeatures(23-43)getInvolvedContent(45-57)projectHistory(59-88)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)
🔇 Additional comments (3)
frontend/src/utils/aboutData.ts (1)
90-95: Typed leaders map looks goodThe
Leadersmap aligns with the newly introducedLeaderstype and is used downstream for filtering and display. No issues spotted.frontend/src/app/about/page.tsx (2)
30-37: Clean data import keeps component leanGood call extracting all static content to
utils/aboutData.tsand importing it here—this keeps the page component focused on presentation and data-fetching logic.
111-119: Mission / Audience section renders correctlyLayout and accessibility (semantic headings, readable text colour in dark mode) look solid. No action required.
|
@arkid15r any update? |
| @@ -35,6 +35,78 @@ jest.mock('utils/aboutData', () => ({ | |||
| 'This is a test paragraph about the project.', | |||
| 'This is another paragraph about the project history.', | |||
| ], | |||
| leaders: { | |||
There was a problem hiding this comment.
These look like exact copies of what we have in aboutData.ts. Can we avoid the duplication?
| 'Leverage AI-powered insights to better understand project relationships and recommendations.', | ||
| }, | ||
| ], | ||
| getInvolvedContent: { |
There was a problem hiding this comment.
Was it intended to look like this?
The list item markers are misaligned and the links color isn't the one we normally use.
| callToAction: | ||
| 'To get started, visit the OWASP Nest Repository, explore the Contributing Guidelines, and review the Code of Conduct.', | ||
| }, | ||
| projectHistory: [ |
There was a problem hiding this comment.
This one looks inconsistent w/ the rest of the content too. I don't get your point of having multiple styles on the same page 🤷♂️
There was a problem hiding this comment.
Same for the white theme
|
|
@rishyym0927 do you plan to address these comments? Just checking in if this is still WIP. |
📝 Description
Fixes #1385
This PR introduces a significant enhancement to the
Aboutpage with a focus on structure, scalability, and theme alignment.✅ New Sections Added
🌗 Theme Compatibility
All newly added sections are fully responsive and aligned with both light and dark themes for a consistent visual experience.
🧩 Content Management
🧪 Testing Improvements