Skip to content

Conversation

@anurag2787
Copy link
Contributor

@anurag2787 anurag2787 commented Sep 9, 2025

Resolves #1385

Description

This PR improves the /about page to make it more informative and welcoming.
I have added new sections that help explain the project purpose, features, and journey, while also guiding people on how they can get involved.

Sections Added

  • Mission
  • Key Features
  • Who It’s For
  • Get Involved
  • Project History Timeline

Checklist

  • I've read and followed the contributing guidelines.
  • I've run make check-test locally; all checks and tests passed.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

Warning

Rate limit exceeded

@kasya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 51 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 0da3ec7 and c3aa10c.

📒 Files selected for processing (1)
  • frontend/__tests__/e2e/pages/About.spec.ts (3 hunks)

Summary by CodeRabbit

  • New Features
    • Revamped About page with Our Mission and Who It’s For cards.
    • Added Key Features section highlighting capabilities.
    • Introduced Our Story with rich text content.
    • Added Project Timeline with a vertical, reverse-chronological view.
    • New Get Involved section with ways to participate and a call-to-action.
    • Retained Technologies & Tools, repositioned within the page.
  • Style
    • Updated icons and tooltips; refined grid/layout for improved readability and card organization.

Walkthrough

Replaces the History-focused About page with data-driven sections: Mission, Who It’s For, Key Features, Get Involved, Our Story, and a Project Timeline; adds typed content exports, updates icons/layout, and adjusts unit and e2e tests to the new data shape.

Changes

Cohort / File(s) Summary
About page restructuring
frontend/src/app/about/page.tsx
Replaces History rendering with separate cards: Mission & Who It’s For (two-column), Key Features (grid), Get Involved (description, list, Markdown CTA), Our Story (Markdown), and Project Timeline (vertical, reverse-chronological). Updates icons, markup, test-ids, and layout.
About-related types
frontend/src/types/about.ts
Adds exported types: MissionContent, KeyFeature, GetInvolved, and ProjectTimeline.
About content/data exports
frontend/src/utils/aboutData.ts
Removes aboutText export; adds and exports missionContent, keyFeatures, getInvolvedContent, projectStory, and projectTimeline (retains technologies).
Tests (unit & e2e)
frontend/__tests__/unit/pages/About.test.tsx, frontend/__tests__/e2e/pages/About.spec.ts
Replaces fixtures to match the new data model; adds assertions for Mission, Key Features, Get Involved, Our Story, and Project Timeline entries; updates selectors/headings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • arkid15r

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Enhance /about page with new sections" succinctly and accurately reflects the primary change of adding new informational sections to the about page without extraneous details or file listings.
Linked Issues Check ✅ Passed The changes implement all requested sections from issue #1385 by adding Mission, Key Features, Who It’s For, Get Involved, and Project History Timeline content and UI, updating types and data sources accordingly, and enhancing tests to verify each section.
Out of Scope Changes Check ✅ Passed All modifications are confined to the /about page and its related types, data, and tests with no unrelated functionality or modules altered.
Description Check ✅ Passed The pull request description clearly states that it resolves issue #1385 and outlines the purpose of making the /about page more informative and welcoming by listing the new sections added.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@anurag2787 anurag2787 changed the title Improved about page Enhance /about page with new sections Sep 9, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
frontend/src/utils/aboutData.ts (1)

132-135: Fix brand casing: Playwright

Key is misspelled as playWright; this renders “PlayWright”.

-      playWright: {
+      playwright: {
         icon: '/images/icons/playwright.svg',
         url: 'https://playwright.dev/',
       },
frontend/src/app/about/page.tsx (1)

85-96: Spinner persists on GraphQL error; show error UI instead

Current isLoading logic can loop on errors. Use query loading flags and a hasError gate.

-  const isLoading =
-    !projectMetadataResponse ||
-    !topContributorsResponse ||
-    (projectMetadataRequestError && !projectMetadata) ||
-    (topContributorsRequestError && !topContributors)
+  const isLoading = projectMetadataResponse?.loading || topContributorsResponse?.loading
+  const hasError = projectMetadataRequestError || topContributorsRequestError

Then:

-  if (isLoading) {
+  if (isLoading) {
     return <LoadingSpinner />
   }
 
-  if (!projectMetadata || !topContributors) {
+  if (hasError) {
+    return <ErrorDisplay statusCode={500} title="Unable to load data" message="Please try again later." />
+  }
+  if (!projectMetadata || !topContributors) {
     return (
       <ErrorDisplay
         statusCode={404}
         title="Data not found"
         message="Sorry, the page you're looking for doesn't exist"
       />
     )
   }

Additionally, destructure loading from both useQuery calls (outside this hunk):

const { data: projectMetadataResponse, error: projectMetadataRequestError, loading: projectMetadataLoading } = useQuery(...)
const { data: topContributorsResponse, error: topContributorsRequestError, loading: topContributorsLoading } = useQuery(...)
🧹 Nitpick comments (4)
frontend/src/types/about.ts (1)

23-31: Tighten TechnologySection typing for readability

Extract Tool and use Record to simplify the nested index signature.

-export type TechnologySection = {
-  section: string
-  tools: {
-    [toolName: string]: {
-      icon: string
-      url: string
-    }
-  }
-}
+export type Tool = { icon: string; url: string }
+export type TechnologySection = {
+  section: string
+  tools: Record<string, Tool>
+}
frontend/src/utils/aboutData.ts (1)

90-90: Annotate technologies with its exported type

Prevents drift and improves DX.

-export const technologies = [
+export const technologies: TechnologySection[] = [
frontend/src/app/about/page.tsx (2)

242-251: External links in Markdown open in same tab

Improve UX and security; add target and rel to Markdown-rendered anchors.

Update components/MarkdownWrapper.tsx (outside this hunk):

import mila from 'markdown-it-link-attributes'

const md = markdownit({ html: false, linkify: true, typographer: true })
  .use(mila, { attrs: { target: '_blank', rel: 'noopener noreferrer' } })

109-119: Meta: PR doesn’t address linked issue #1835 (tests for GeneralCompliantComponent)

This page refactor is solid, but it doesn’t add the required tests. Please add them or update the linked issue.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f038c59 and 84430b8.

📒 Files selected for processing (3)
  • frontend/src/app/about/page.tsx (3 hunks)
  • frontend/src/types/about.ts (1 hunks)
  • frontend/src/utils/aboutData.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/utils/aboutData.ts (1)
frontend/src/types/about.ts (4)
  • MissionContent (18-21)
  • KeyFeature (1-4)
  • GetInvolved (12-16)
  • ProjectHistory (6-10)
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 (1)
frontend/src/app/about/page.tsx (1)

311-318: No changes required for the onclick prop
The Button type in frontend/src/types/button.ts defines the handler as onclick, and UserCardProps.button correctly uses that; UserCard then maps it to onPress on the UI component—contract is honored.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

1-1: Remove “Resolves #1835” from PR description
This PR only modifies frontend/src/app/about/page.tsx and does not add or update tests for GeneralCompliantComponent (existing tests already cover it). Drop the auto‐close issue reference.

♻️ Duplicate comments (2)
frontend/src/app/about/page.tsx (2)

121-132: Resolved: stable keys for features.

Switched from index to feature.title. This addresses the earlier reconciliation issue.


252-269: Resolved: stable keys for timeline + decorative dot a11y.

Key composed from year-title and aria-hidden on the marker both look good.

🧹 Nitpick comments (3)
frontend/src/app/about/page.tsx (3)

109-119: Section UX is clear; minor icon nit.

Mission and Roadmap both use faMapSigns. Consider a distinct icon for one of them for quicker visual scanning (e.g., faCircleInfo for Mission or keep faMapSigns only for Roadmap).


121-132: Prefer deterministic IDs over titles for keys.

If titles ever collide or change, keys will churn. Consider adding an id field to KeyFeature in aboutData and use it as the key.


242-251: Avoid index keys; optional Markdown link behavior.

  • Use a stable key for ways; the string itself is fine.
  • Optional: external links in Markdown don’t open in a new tab; for consistency with other external links, consider adding target="_blank" + rel="noopener noreferrer" in MarkdownWrapper.

Apply within this hunk:

-            {getInvolvedContent.ways.map((way, index) => (
-              <li key={index} className="text-gray-600 dark:text-gray-300">{way}</li>
+            {getInvolvedContent.ways.map((way) => (
+              <li key={way} className="text-gray-600 dark:text-gray-300">{way}</li>

If you want the Markdown change, I can propose a small renderer hook in components/MarkdownWrapper.tsx to add target/rel to links.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84430b8 and 5d2da4b.

📒 Files selected for processing (3)
  • frontend/src/app/about/page.tsx (3 hunks)
  • frontend/src/types/about.ts (1 hunks)
  • frontend/src/utils/aboutData.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/src/types/about.ts
  • frontend/src/utils/aboutData.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
  • missionContent (14-19)
  • keyFeatures (21-41)
  • getInvolvedContent (43-55)
  • projectHistory (57-86)
frontend/src/components/MarkdownWrapper.tsx (1)
  • Markdown (4-17)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)

26-26: Imports aligned with new data model — looks good.

Switch to missionContent/keyFeatures/getInvolvedContent/projectHistory reads clean and keeps concerns in utils/aboutData.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
frontend/src/app/about/page.tsx (2)

121-132: Switched to stable keys for features — good upgrade.
Using feature.title instead of index keys addresses reconciliation issues noted earlier.


252-268: Deterministic timeline keys and decorative dot a11y — LGTM.
Good use of ${milestone.year}-${milestone.title} and aria-hidden on the dot.

🧹 Nitpick comments (4)
frontend/src/app/about/page.tsx (4)

109-119: Avoid duplicate headings; let AnchorTitle be the section heading.
Reduce heading noise by removing the extra h3s inside these cards. Also, confirm AnchorTitle slugifies "Who It's For" safely (apostrophe/punctuation).

Apply this diff to drop the redundant h3s:

-            <h3 className="mb-2 text-xl font-semibold text-blue-400">Our Mission</h3>
             <p className="text-gray-600 dark:text-gray-300">{missionContent.mission}</p>
...
-            <h3 className="mb-2 text-xl font-semibold text-blue-400">Target Audience</h3>
             <p className="text-gray-600 dark:text-gray-300">{missionContent.whoItsFor}</p>

242-251: Use stable keys for “Get Involved” list items.
Avoid index keys; the item string is deterministic and unique enough here.

Apply:

-            {getInvolvedContent.ways.map((way, index) => (
-              <li key={index} className="text-gray-600 dark:text-gray-300">{way}</li>
+            {getInvolvedContent.ways.map((way) => (
+              <li key={way} className="text-gray-600 dark:text-gray-300">{way}</li>
             ))}

257-258: Mark the vertical connector as decorative.
Add aria-hidden to the blue line to keep it out of the accessibility tree.

-                  <div className="absolute left-[11px] top-8 h-full w-0.5 bg-blue-400"></div>
+                  <div aria-hidden="true" className="absolute left-[11px] top-8 h-full w-0.5 bg-blue-400"></div>

109-119: PR/issue mismatch: resolving #1835 (tests for ) isn’t addressed here.
This change updates the About page but doesn’t add the required tests. Either drop “Resolves #1835” from the PR or include the tests before merge. I can help scaffold the test suite if you want.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d2da4b and 464d2c4.

📒 Files selected for processing (1)
  • frontend/src/app/about/page.tsx (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
  • missionContent (14-19)
  • keyFeatures (21-41)
  • getInvolvedContent (43-55)
  • projectHistory (57-86)
frontend/src/components/MarkdownWrapper.tsx (1)
  • Markdown (4-17)
🔇 Additional comments (2)
frontend/src/app/about/page.tsx (2)

26-26: Consolidated aboutData imports — LGTM.


276-277: Confirm the rounding strategy for stats.
Rounding down to the nearest 10 with a trailing “+” may understate small counts (e.g., 9 → “0+”). Confirm this is intentional for the UI.

@anurag2787
Copy link
Contributor Author

Hy @arkid15r @kasya could you please take a look and let me know if any further changes are needed or should I start writing tests for these updates?

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good!! Left a couple of requests ⬇️

@kasya
Copy link
Collaborator

kasya commented Sep 11, 2025

@anurag2787 could you also update description to connect correct issue?
And it seems that you need to run make check to address some issues with your next push.

@anurag2787 anurag2787 marked this pull request as draft September 11, 2025 04:22
@anurag2787
Copy link
Contributor Author

@anurag2787 could you also update description to connect correct issue? And it seems that you need to run make check to address some issues with your next push.

Hi, I haven’t written the tests yet i opened PR for UI review. I’ll update it as requested and add the tests for the same. Thanks!

@anurag2787 anurag2787 marked this pull request as ready for review September 11, 2025 15:11
@anurag2787 anurag2787 requested a review from kasya September 11, 2025 15:12
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (13)
frontend/__tests__/e2e/pages/About.spec.ts (3)

37-43: Align expectations with the new IA; drop legacy "History" heading.

With "Project History" added, the legacy "History" heading may no longer exist. Keep the exact match for "Project History" and remove "History" to avoid false failures.

   test('renders main sections correctly', async ({ page }) => {
     await expect(page.getByRole('heading', { name: 'About' })).toBeVisible()
     await expect(page.getByRole('heading', { name: 'Project History', exact: true })).toBeVisible()
-    await expect(page.getByRole('heading', { name: 'History' })).toBeVisible()
     await expect(page.getByRole('heading', { name: 'Leaders' })).toBeVisible()
     await expect(page.getByRole('heading', { name: 'Roadmap' })).toBeVisible()
   })

96-99: Prefer role-based heading locators for stability and a11y.

Use a heading role query instead of text to reduce brittleness and align with other assertions.

-  await expect(page.getByText('Key Features')).toBeVisible()
+  await expect(page.getByRole('heading', { name: 'Key Features', exact: true })).toBeVisible()
   await expect(page.getByText('Advanced Search Capabilities')).toBeVisible()

101-104: Use role-based query for "Project History" heading.

Keeps selectors consistent and resilient.

-  await expect(page.getByText('Project History')).toBeVisible()
+  await expect(page.getByRole('heading', { name: 'Project History', exact: true })).toBeVisible()
   await expect(page.getByText('Project Inception')).toBeVisible()
frontend/__tests__/unit/pages/About.test.tsx (10)

33-50: Good targeted mock; consider centralizing and adding type-safety.

Nice isolation from real content. To reduce duplication across tests and catch drift early, centralize this in mocks/utils/aboutData.ts and add compile-time checks with satisfies.

Add (outside this block):

import type { MissionContent, KeyFeature, GetInvolved, ProjectHistory } from 'types/about'

Then tighten the mock:

 jest.mock('utils/aboutData', () => ({
-  missionContent: {
+  missionContent: ({
     mission: 'Test mission statement',
     whoItsFor: 'Test target audience description',
-  },
-  keyFeatures: [
+  } as const satisfies MissionContent),
+  keyFeatures: [
     { title: 'Feature 1', description: 'Feature 1 description' },
     { title: 'Feature 2', description: 'Feature 2 description' },
-  ],
-  getInvolvedContent: {
+  ] as const satisfies KeyFeature[],
+  getInvolvedContent: ({
     description: 'Get involved description',
     ways: ['Way 1', 'Way 2'],
     callToAction: 'Test call to action',
-  },
-  projectHistory: [
+  } as const satisfies GetInvolved),
+  projectHistory: [
     { year: '2023', title: 'Test Event 1', description: 'Test description 1' },
     { year: '2024', title: 'Test Event 2', description: 'Test description 2' },
-  ],
+  ] as const satisfies ProjectHistory[],

154-167: Use role-based heading query; avoid brittle DOM traversal.

Query the heading directly and drop .closest('div').

-  const keyFeaturesSection = screen.getByText('Key Features').closest('div')
-  expect(keyFeaturesSection).toBeInTheDocument()
+  expect(screen.getByRole('heading', { name: 'Key Features' })).toBeInTheDocument()

169-181: Same here: prefer role-based query for "Get Involved".

-  const getInvolvedSection = screen.getByText('Get Involved').closest('div')
-  expect(getInvolvedSection).toBeInTheDocument()
+  expect(screen.getByRole('heading', { name: 'Get Involved' })).toBeInTheDocument()

188-197: Prefer heading role for "Project History" section title.

-  const projectHistorySection = screen.getByText('Project History').closest('div')
-  expect(projectHistorySection).toBeInTheDocument()
+  expect(screen.getByRole('heading', { name: 'Project History' })).toBeInTheDocument()

625-637: Tighten queries to headings; drop .closest('div').

-  const missionSection = screen.getByText('Mission').closest('div')
-  expect(missionSection).toBeInTheDocument()
+  expect(screen.getByRole('heading', { name: 'Mission' })).toBeInTheDocument()
   expect(screen.getByText('Test mission statement')).toBeInTheDocument()

-  const whoItsForSection = screen.getByText("Who It's For").closest('div')
-  expect(whoItsForSection).toBeInTheDocument()
+  expect(screen.getByRole('heading', { name: "Who It's For" })).toBeInTheDocument()
   expect(screen.getByText('Test target audience description')).toBeInTheDocument()

639-645: Redundant with the combined mission/Who It's For test above; remove to cut noise.

-  test('renders mission section', async () => {
-    await act(async () => {
-      render(<About />)
-    })
-    expect(screen.getByText('Mission')).toBeInTheDocument()
-    expect(screen.getByText('Test mission statement')).toBeInTheDocument()
-  })

647-653: Duplicate of coverage in the combined test; safe to delete.

-  test("renders 'Who It's For' section", async () => {
-    await act(async () => {
-      render(<About />)
-    })
-    expect(screen.getByText("Who It's For")).toBeInTheDocument()
-    expect(screen.getByText(/Test target audience description/)).toBeInTheDocument()
-  })

655-664: Duplicate of "renders key features section correctly" (Lines 154–167); remove one.

-  test('renders key features section', async () => {
-    await act(async () => {
-      render(<About />)
-    })
-    expect(screen.getByText('Key Features')).toBeInTheDocument()
-    expect(screen.getByText('Feature 1')).toBeInTheDocument()
-    expect(screen.getByText('Feature 2')).toBeInTheDocument()
-    expect(screen.getByText('Feature 1 description')).toBeInTheDocument()
-    expect(screen.getByText('Feature 2 description')).toBeInTheDocument()
-  })

666-675: Duplicate of "renders get involved section correctly" (Lines 169–181); remove one.

-  test('renders get involved section', async () => {
-    await act(async () => {
-      render(<About />)
-    })
-    expect(screen.getByText('Get Involved')).toBeInTheDocument()
-    expect(screen.getByText('Get involved description')).toBeInTheDocument()
-    expect(screen.getByText('Way 1')).toBeInTheDocument()
-    expect(screen.getByText('Way 2')).toBeInTheDocument()
-    expect(screen.getByText('Test call to action')).toBeInTheDocument()
-  })

677-686: Duplicate of "renders project history timeline correctly" (Lines 183–197); remove one.

-  test('renders project history timeline section', async () => {
-    await act(async () => {
-      render(<About />)
-    })
-    expect(screen.getByText('Project History')).toBeInTheDocument()
-    expect(screen.getByText('Test Event 1')).toBeInTheDocument()
-    expect(screen.getByText('Test Event 2')).toBeInTheDocument()
-    expect(screen.getByText('Test description 1')).toBeInTheDocument()
-    expect(screen.getByText('Test description 2')).toBeInTheDocument()
-  })
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 988066f and 3f6de0a.

📒 Files selected for processing (5)
  • frontend/__tests__/e2e/pages/About.spec.ts (2 hunks)
  • frontend/__tests__/unit/pages/About.test.tsx (3 hunks)
  • frontend/src/app/about/page.tsx (3 hunks)
  • frontend/src/types/about.ts (1 hunks)
  • frontend/src/utils/aboutData.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • frontend/src/app/about/page.tsx
  • frontend/src/types/about.ts
  • frontend/src/utils/aboutData.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
frontend/src/wrappers/testUtil.tsx (1)
  • render (14-14)

@kasya
Copy link
Collaborator

kasya commented Sep 12, 2025

@anurag2787 make check command that we have is not for the tests - it's for linters and other checks. Right now they fail on your PR and it's required to run it before any commit and push 👌🏼

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anurag2787 this looks good 👍🏼
One more request:
what if we move the Get involved block up and keep Roadmap and Project History next to each other? I feel like that would make more sense... 🤔 What do you think?

@anurag2787
Copy link
Contributor Author

Hy @kasya moving Get involved block up is a good idea but keeping Roadmap and Project History next to each other can create inconsistency in heights
image

@kasya
Copy link
Collaborator

kasya commented Sep 13, 2025

Hy @kasya moving Get involved block up is a good idea but keeping Roadmap and Project History next to each other can create inconsistency in heights

I'm sorry, I guess I wasn't clear in my previous message. What I meant is to just get the Get involved block higher - that would make those two other blocks (Roadmap and Project History) appear one after another. We didn't really need to combine them into a single block.
Just some reordering of things.. Is it clear now?

@anurag2787 anurag2787 requested a review from kasya September 15, 2025 18:58
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

49-66: Fix: spinner can hang forever on GraphQL errors; drive loading from Apollo and show an error state

Current isLoading depends on presence of response objects; when a query errors early, the spinner may never resolve. Use loading flags from useQuery and gate an explicit error render.

Apply:

-  const { data: projectMetadataResponse, error: projectMetadataRequestError } = useQuery(
+  const {
+    data: projectMetadataResponse,
+    loading: projectMetadataLoading,
+    error: projectMetadataRequestError,
+  } = useQuery(
     GET_PROJECT_METADATA,
     {
       variables: { key: projectKey },
     }
   )
 
-  const { data: topContributorsResponse, error: topContributorsRequestError } = useQuery(
+  const {
+    data: topContributorsResponse,
+    loading: topContributorsLoading,
+    error: topContributorsRequestError,
+  } = useQuery(
     GET_TOP_CONTRIBUTORS,
     {
       variables: {
         excludedUsernames: Object.keys(leaders),
         hasFullName: true,
         key: projectKey,
         limit: 24,
       },
     }
   )
-  const isLoading =
-    !projectMetadataResponse ||
-    !topContributorsResponse ||
-    (projectMetadataRequestError && !projectMetadata) ||
-    (topContributorsRequestError && !topContributors)
+  const isLoading = projectMetadataLoading || topContributorsLoading
-  if (!projectMetadata || !topContributors) {
+  if (projectMetadataRequestError || topContributorsRequestError) {
+    return (
+      <ErrorDisplay
+        statusCode={503}
+        title="Couldn't load data"
+        message="We were unable to load project metadata or contributors."
+      />
+    )
+  }
+
+  if (!projectMetadata || !topContributors) {
     return (
       <ErrorDisplay
         statusCode={404}
         title="Data not found"
         message="Sorry, the page you're looking for doesn't exist"
       />
     )
   }

Also applies to: 91-109

♻️ Duplicate comments (2)
frontend/src/app/about/page.tsx (2)

125-136: Stable keys for features: addressed

Switched from index to feature.title as key; matches prior guidance and avoids reorder glitches. Ensure titles remain unique in aboutData.


262-282: Timeline polish looks good and addresses prior feedback

Deterministic keys, smaller dots (h-4 w-4), added padding (pl-10), and aria-hidden on decorative bullets are all on point.

🧹 Nitpick comments (4)
frontend/src/app/about/page.tsx (4)

115-124: Verify anchor slug for “Who It’s For”

Apostrophes can produce awkward fragment IDs. Ensure AnchorTitle generates a clean slug (e.g., who-its-for) or allow passing one explicitly.

If supported:

-<SecondaryCard icon={faUsers} title={<AnchorTitle title="Who It's For" />}>
+<SecondaryCard icon={faUsers} title={<AnchorTitle id="who-its-for" title="Who It's For" />}>

189-200: Open Markdown links safely in a new tab (optional)

Markdown links currently open in the same tab. Consider adding target="_blank" with rel="noopener noreferrer" for external links within MarkdownWrapper while keeping sanitization.

In frontend/src/components/MarkdownWrapper.tsx:

 export default function Markdown({ content, className }: { content: string; className?: string }) {
   const md = markdownit({
     html: false,
     linkify: true,
     typographer: true,
   })
+  // Force external links to open in a new tab with safe rel attrs
+  const defaultRender = md.renderer.rules.link_open || ((tokens, idx, options, env, self) => self.renderToken(tokens, idx, options))
+  md.renderer.rules.link_open = (tokens, idx, options, env, self) => {
+    const aIndex = tokens[idx].attrIndex('target')
+    if (aIndex < 0) tokens[idx].attrPush(['target', '_blank'])
+    else tokens[idx].attrs![aIndex][1] = '_blank'
+    const relIndex = tokens[idx].attrIndex('rel')
+    if (relIndex < 0) tokens[idx].attrPush(['rel', 'noopener noreferrer'])
+    else tokens[idx].attrs![relIndex][1] = 'noopener noreferrer'
+    return defaultRender(tokens, idx, options, env, self)
+  }

249-254: Roadmap body rendering: consider formatting and truncation (optional)

milestone.body may contain long text or Markdown. For readability, either sanitize-render as Markdown or clamp lines to avoid tall cards.

Option A (render sanitized Markdown):

-<p data-testid="milestone-description" className="text-gray-600 dark:text-gray-300">
-  {milestone.body}
-</p>
+<div data-testid="milestone-description" className="prose prose-invert max-w-none dark:prose-invert">
+  <Markdown content={milestone.body || ''} />
+  {/* Markdown component already sanitizes */}
+</div>

Option B (keep text, clamp to 3 lines):

-<p data-testid="milestone-description" className="text-gray-600 dark:text-gray-300">
+<p data-testid="milestone-description" className="text-gray-600 line-clamp-3 dark:text-gray-300">
   {milestone.body}
 </p>

289-291: Expose exact stats for screen readers while keeping the “+” UI

Animated, rounded numbers plus “+” can be unclear to assistive tech. Add an accessible label and hide the animated number from AT.

Apply:

-            <div key={stat.label}>
-              <SecondaryCard className="text-center">
-                <div className="mb-2 text-3xl font-bold text-blue-400">
+            <div key={stat.label}>
+              <SecondaryCard className="text-center" aria-label={`${stat.label}: ${stat.value}`}>
+                <div className="mb-2 text-3xl font-bold text-blue-400" aria-hidden="true">
                   <AnimatedCounter end={Math.floor(stat.value / 10) * 10} duration={2} />+
                 </div>
+                <span className="sr-only">{`${stat.value} ${stat.label}`}</span>
                 <div className="text-gray-600 dark:text-gray-300">{stat.label}</div>
               </SecondaryCard>
             </div>
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f6de0a and 7fa1906.

📒 Files selected for processing (1)
  • frontend/src/app/about/page.tsx (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
  • missionContent (9-14)
  • keyFeatures (16-36)
  • getInvolvedContent (38-50)
  • projectHistory (52-81)
frontend/src/components/MarkdownWrapper.tsx (1)
  • Markdown (4-17)
🔇 Additional comments (1)
frontend/src/app/about/page.tsx (1)

26-32: Imports align with the new data-driven model — good move

The page now sources copy and lists from utils/aboutData; keeps UI and content decoupled.

kasya
kasya previously approved these changes Sep 16, 2025
Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @anurag2787 !
This looks good to me 👌🏼

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7fa1906 and 656856b.

📒 Files selected for processing (3)
  • frontend/src/app/about/page.tsx (5 hunks)
  • frontend/src/types/about.ts (1 hunks)
  • frontend/src/utils/aboutData.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/types/about.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/app/about/page.tsx (2)
frontend/src/utils/aboutData.ts (4)
  • missionContent (9-14)
  • keyFeatures (16-47)
  • getInvolvedContent (49-61)
  • projectTimeline (63-118)
frontend/src/components/MarkdownWrapper.tsx (1)
  • Markdown (4-17)
frontend/src/utils/aboutData.ts (1)
frontend/src/types/about.ts (4)
  • MissionContent (12-15)
  • KeyFeature (7-10)
  • GetInvolved (1-5)
  • ProjectTimeline (17-21)

@sonarqubecloud
Copy link

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arkid15r arkid15r added this pull request to the merge queue Sep 28, 2025
Merged via the queue into OWASP:main with commit 5d882ec Sep 28, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve /about page

3 participants