[Feature] : Community Overview page best for User experience #3919
Closed
Isha-upadhyay wants to merge 7 commits intoOWASP:mainfrom
Closed
[Feature] : Community Overview page best for User experience #3919Isha-upadhyay wants to merge 7 commits intoOWASP:mainfrom
Isha-upadhyay wants to merge 7 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Community overview: backend OrganizationQuery.recent_organizations with limit normalization; new frontend Community layout, page component, GraphQL query, ChapterCard component, breadcrumb logic to insert a "Community" segment; plus unit, e2e, and a11y tests and mock fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 21 files
Confidence score: 4/5
- This PR looks safe to merge; the only noted issue is a low-severity mock/schema mismatch that mainly affects test fidelity rather than runtime behavior.
- The mock
topContributorsincludes abiofield not returned by theGetCommunityPageDataquery, which could mislead future tests about the real response shape. - Pay close attention to
frontend/__tests__/mockData/mockCommunityData.ts- remove the extrabiofield to keep mocks aligned with the GraphQL schema.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/__tests__/mockData/mockCommunityData.ts">
<violation number="1" location="frontend/__tests__/mockData/mockCommunityData.ts:56">
P2: Mock `topContributors` includes `bio` field which is not present in the `GetCommunityPageData` GraphQL query. Remove it so the mock accurately reflects the real response shape and doesn't mislead future tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/app/community/page.tsx`:
- Around line 159-168: The stats grid <div> is mis-indented relative to its
surrounding container; locate the block that renders statsData (the <div
className="grid gap-6 sm:grid-cols-2 lg:grid-cols-4"> mapping over statsData and
rendering SecondaryCard) and fix its indentation to match the surrounding JSX
nesting (align it under the parent container where max-w-6xl is used) so the JSX
tree remains visually consistent and properly nested.
In `@frontend/src/hooks/useBreadcrumbs.ts`:
- Around line 26-34: The breadcrumb logic currently skips adding a "Community"
breadcrumb for the `/community` page because the injection guard uses
!pathname.startsWith('/community') and the segment 'community' is listed in
HIDDEN_SEGMENTS; update the logic in useBreadcrumbs to ensure `/community`
yields a Community item by either (A) special-casing pathname === '/community'
to call items.push({ title: 'Community', path: '/community' }) before the
segment iteration, or (B) remove 'community' from HIDDEN_SEGMENTS and rely on
the existing COMMUNITY_RELATED_PATHS check (COMMUNITY_RELATED_PATHS,
HIDDEN_SEGMENTS, pathname, items.push are the relevant symbols) so the injection
branch adds the Community breadcrumb correctly.
🧹 Nitpick comments (7)
backend/tests/apps/github/api/internal/queries/organization_test.py (1)
52-82: Good coverage of the core paths.The positive, zero, and negative limit cases are well-structured. One optional gap: there's no test verifying that a limit exceeding
MAX_LIMIT(e.g.,limit=200) is capped to100. This would confirm thenormalize_limitintegration end-to-end within this query.frontend/__tests__/e2e/pages/Chapters.spec.ts (1)
59-60: Minor formatting inconsistency: missing space before}.Line 59 has
({ page})while other test callbacks in this file (lines 27, 34, 45, 52) use({ page })with a space. This is cosmetic but inconsistent within the same file.🔧 Proposed fix
- test('breadcrumb renders correct segments on /chapters', async ({ page}) => { + test('breadcrumb renders correct segments on /chapters', async ({ page }) => {frontend/__tests__/e2e/pages/Community.spec.ts (1)
7-17: Blanket GraphQL route mock may interfere with subcomponent queries.All
**/graphql/requests are intercepted with the same combined payload. IfMultiSearchBaror other subcomponents issue their own GraphQL queries, they'll receive community/home data instead of their expected response. This is unlikely to cause test failures for visibility assertions but could become fragile if components start making additional GraphQL calls.Consider matching on the GraphQL operation name in the request body to return the correct mock for each query, if this becomes an issue.
frontend/src/app/community/page.tsx (4)
55-55: Redundant variable alias.
const data = graphQLDataadds no value — just usegraphQLDatadirectly (or rename it todatain the destructuring on line 23).♻️ Suggested fix
const { - data: graphQLData, + data, error: graphQLRequestError, loading: isLoading, } = useQuery(GetCommunityPageDataDocument)Then remove line 55 and replace remaining
graphQLDatareferences accordingly.
91-157: Extract navigation cards into a data-driven loop to eliminate duplication.Four nearly identical card blocks (~16 lines each) differ only in
href, icon, title, description, and CTA text. This is a prime DRY candidate.♻️ Suggested approach
const navItems = [ { href: '/chapters', icon: FaLocationDot, title: 'Chapters', description: 'Find OWASP chapters in your area and join local meetups', cta: 'Explore' }, { href: '/members', icon: FaPeopleGroup, title: 'Members', description: 'Connect with security professionals and contributors', cta: 'Browse' }, { href: '/organizations', icon: FaBuilding, title: 'Organizations', description: 'Discover organizations supporting OWASP initiatives', cta: 'View All' }, { href: '/community/snapshots', icon: FaCamera, title: 'Snapshots', description: 'View community activity highlights and milestones', cta: 'See Updates' }, ] // Then in JSX: <div className="mb-16 grid gap-6 sm:grid-cols-2 lg:grid-cols-4"> {navItems.map(({ href, icon: Icon, title, description, cta }) => ( <Link key={href} href={href} className="group relative overflow-hidden rounded-xl bg-white p-6 shadow-lg transition-all hover:-translate-y-1 hover:shadow-xl dark:bg-gray-800"> <div className="mb-4 inline-flex rounded-lg bg-blue-100 p-3 text-blue-600 dark:bg-blue-900/30 dark:text-blue-400"> <Icon className="h-6 w-6" /> </div> <h3 className="mb-2 text-lg font-semibold text-gray-900 dark:text-white">{title}</h3> <p className="mb-4 text-sm text-gray-600 dark:text-gray-400">{description}</p> <span className="inline-flex items-center gap-1 text-sm font-medium text-blue-600 group-hover:gap-2 dark:text-blue-400"> {cta} <FaArrowRight className="h-3 w-3 transition-transform" /> </span> </Link> ))} </div>
209-216: Duplicated avatar URL construction — extract a helper.The pattern
`${url}${url.includes('?') ? '&' : '?'}s=${size}`appears for both organizations (line 211) and contributors (line 280). Extract a small utility:function avatarUrl(baseUrl: string, size: number): string { const sep = baseUrl.includes('?') ? '&' : '?' return `${baseUrl}${sep}s=${size}` }This also makes it easier to adjust sizing or add other params later.
Also applies to: 275-283
178-189: No empty-state handling for data sections.If
recentChapters,recentOrganizations,snapshots, ortopContributorsresolve to empty arrays, the section headers still render with an empty body. Consider either hiding the section or showing an "No data available" placeholder for a polished UX.Also applies to: 202-227, 239-256, 268-295
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/hooks/useBreadcrumbs.ts`:
- Around line 8-9: Breadcrumbs for nested community routes are missing because
the code only checks pathname === '/community' and skips the injection when
pathname.startsWith('/community'); update useBreadcrumbs to detect nested
community paths by using pathname.startsWith('/community') instead of strict
equality and ensure the injection block that inserts the 'community' segment
runs for nested routes; modify the injection logic to check the path segments
(from the split used to build breadcrumbs) for any of COMMUNITY_RELATED_PATHS
and, if found, insert a breadcrumb for 'community' before those segments (or add
'community' to COMMUNITY_RELATED_PATHS handling), and keep HIDDEN_SEGMENTS logic
but ensure 'community' is not removed after insertion so nested routes like
/community/snapshots/[id] produce [Home, Community, Snapshots, id].
🧹 Nitpick comments (1)
frontend/src/hooks/useBreadcrumbs.ts (1)
16-16: Defensive|| []is unnecessary given the type signature.
registeredItemsis typed asBreadcrumbItem[], so it should never be nullish. IfuseBreadcrumb()can returnundefined, the type at the call site (Line 67) should be fixed instead.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/hooks/useBreadcrumbs.ts">
<violation number="1" location="frontend/src/hooks/useBreadcrumbs.ts:8">
P1: Bug: Removing `'community'` from `HIDDEN_SEGMENTS` causes a duplicate "Community" breadcrumb for any sub-path under `/community`. The explicit push above already adds the Community breadcrumb, and then the segment loop adds it again since `'community'` is no longer skipped. Add `'community'` back to `HIDDEN_SEGMENTS`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/hooks/useBreadcrumbs.ts`:
- Line 8: The duplicate "Community" breadcrumb is caused by removing 'community'
from HIDDEN_SEGMENTS while an explicit injection block (in useBreadcrumbs)
already inserts a Community breadcrumb for paths starting with /community;
restore 'community' in the HIDDEN_SEGMENTS set so the injection block remains
the sole source of that breadcrumb and the subsequent segment loop in
useBreadcrumbs does not push a second identical entry (also update any other
similar injection vs. segment-loop logic references to avoid duplicates).
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/hooks/useBreadcrumbs.ts">
<violation number="1" location="frontend/src/hooks/useBreadcrumbs.ts:26">
P1: Bug: Community breadcrumb is missing for sub-pages under `/community/`. Routes like `/community/snapshots` will show `Home > Snapshots` instead of `Home > Community > Snapshots` because `isCommunityLogicalParent` is `false` when pathname starts with `/community`, and the `'community'` segment is now hidden. The condition should also match sub-paths of `/community/`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/hooks/useBreadcrumbs.test.tsx (1)
97-107:⚠️ Potential issue | 🟡 MinorTest name contradicts the expected output.
The test is named
does not show "community" in breadcrumbs when present in path, yet the expected result at line 104 now includes{ title: 'Community', path: '/community' }. The name should be updated to reflect the current behavior — e.g., "injects Community breadcrumb and hides the community URL segment".
🧹 Nitpick comments (2)
frontend/src/hooks/useBreadcrumbs.ts (2)
8-9: Consider whetherCOMMUNITY_RELATED_PATHSshould use exact-segment matching.
pathname.startsWith('/members')would also match a hypothetical/members-archiveroute. Using exact prefix + slash (pathname === path || pathname.startsWith(path + '/')) would be more precise, though unlikely to cause issues today.♻️ Tighter prefix matching
- COMMUNITY_RELATED_PATHS.some((path) => pathname.startsWith(path)) + COMMUNITY_RELATED_PATHS.some((p) => pathname === p || pathname.startsWith(p + '/'))
16-16: Unnecessary null guard on a non-nullable parameter.
registeredItemsis typedBreadcrumbItem[], so|| []is dead code. It's harmless but could mislead readers into thinking the parameter is nullable.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/hooks/useBreadcrumbs.test.tsx">
<violation number="1" location="frontend/__tests__/unit/hooks/useBreadcrumbs.test.tsx:104">
P2: Test name says "does not show 'community' in breadcrumbs" but the assertion now expects Community to be present. The test description must be updated to reflect the new behavior (e.g., `shows Community breadcrumb for /community/* paths despite community being a hidden segment`), otherwise the test is misleading and will confuse future maintainers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai Please review it again, and can you tell me CI/CD fails because pillow is not upgrade what to do now ? |
Contributor
@Isha-upadhyay I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
4 issues found across 21 files
Confidence score: 4/5
- Mostly safe to merge, but there’s a concrete user-facing risk:
frontend/src/app/page.tsxmixesFaCalendarimports fromfavsfa6, which can cause a visual regression on the page. frontend/__tests__/mockData/mockCommunityData.tsuses string timestamps whileChapterCardexpects numeric Unix timestamps, which can mask real behavior in tests and mocks.- Pay close attention to
frontend/src/app/page.tsx,frontend/__tests__/mockData/mockCommunityData.ts,frontend/src/components/ChapterCard.tsx,frontend/src/app/community/page.tsx- icon mismatch, mock data shape, and string coercion/duplication patterns.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/community/page.tsx">
<violation number="1" location="frontend/src/app/community/page.tsx:211">
P2: Duplicate avatar URL construction logic. This inline URL-sizing pattern is repeated twice in this file and already exists as a helper in `ModuleCard.tsx` (with safer URL parsing via `try/catch`). Extract a shared utility (e.g., `getAvatarUrlWithSize(url, size)`) to avoid duplication and benefit from the safer implementation.
(Based on your team's feedback about using existing shared components and utilities.) [FEEDBACK_USED]</violation>
</file>
<file name="frontend/src/components/ChapterCard.tsx">
<violation number="1" location="frontend/src/components/ChapterCard.tsx:45">
P2: `String(leaders)` relies on implicit array-to-string coercion. Use `leaders.join(',')` for explicit, readable conversion. `String([...])` happens to join with commas, but this is a subtle JS behavior that's easy to misunderstand, and it obscures intent.</violation>
</file>
<file name="frontend/__tests__/mockData/mockCommunityData.ts">
<violation number="1" location="frontend/__tests__/mockData/mockCommunityData.ts:5">
P2: Mock `recentChapters[].createdAt` is an ISO date string, but the GraphQL schema and `ChapterCard` component expect a `number` (Unix timestamp). The mock should use a numeric timestamp to match the actual response shape. Consider also adding a type annotation (e.g., `GetCommunityPageDataQuery`) to prevent future mismatches.</violation>
</file>
<file name="frontend/src/app/page.tsx">
<violation number="1" location="frontend/src/app/page.tsx:36">
P2: Visual regression: `ChapterCard` imports `FaCalendar` from `react-icons/fa` (Font Awesome 5), but the rest of the page uses `FaCalendar` from `react-icons/fa6` (Font Awesome 6). The original inline code used the `fa6` version. Update the import in `ChapterCard.tsx` to use `react-icons/fa6` for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <div className="flex items-center gap-3"> | ||
| {org.avatarUrl && ( | ||
| <Image | ||
| src={`${org.avatarUrl}${org.avatarUrl.includes('?') ? '&' : '?'}s=80`} |
Contributor
There was a problem hiding this comment.
P2: Duplicate avatar URL construction logic. This inline URL-sizing pattern is repeated twice in this file and already exists as a helper in ModuleCard.tsx (with safer URL parsing via try/catch). Extract a shared utility (e.g., getAvatarUrlWithSize(url, size)) to avoid duplication and benefit from the safer implementation.
(Based on your team's feedback about using existing shared components and utilities.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/community/page.tsx, line 211:
<comment>Duplicate avatar URL construction logic. This inline URL-sizing pattern is repeated twice in this file and already exists as a helper in `ModuleCard.tsx` (with safer URL parsing via `try/catch`). Extract a shared utility (e.g., `getAvatarUrlWithSize(url, size)`) to avoid duplication and benefit from the safer implementation.
(Based on your team's feedback about using existing shared components and utilities.) </comment>
<file context>
@@ -0,0 +1,300 @@
+ <div className="flex items-center gap-3">
+ {org.avatarUrl && (
+ <Image
+ src={`${org.avatarUrl}${org.avatarUrl.includes('?') ? '&' : '?'}s=80`}
+ alt={org.name || org.login}
+ width={40}
</file context>
| {leaders.length > 0 && ( | ||
| <div className="mt-1 flex items-center gap-x-2 text-sm text-gray-600 dark:text-gray-400"> | ||
| <HiUserGroup className="h-4 w-4 shrink-0" /> | ||
| <LeadersList entityKey={`${chapterKey}-leaders`} leaders={String(leaders)} /> |
Contributor
There was a problem hiding this comment.
P2: String(leaders) relies on implicit array-to-string coercion. Use leaders.join(',') for explicit, readable conversion. String([...]) happens to join with commas, but this is a subtle JS behavior that's easy to misunderstand, and it obscures intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/ChapterCard.tsx, line 45:
<comment>`String(leaders)` relies on implicit array-to-string coercion. Use `leaders.join(',')` for explicit, readable conversion. `String([...])` happens to join with commas, but this is a subtle JS behavior that's easy to misunderstand, and it obscures intent.</comment>
<file context>
@@ -0,0 +1,51 @@
+ {leaders.length > 0 && (
+ <div className="mt-1 flex items-center gap-x-2 text-sm text-gray-600 dark:text-gray-400">
+ <HiUserGroup className="h-4 w-4 shrink-0" />
+ <LeadersList entityKey={`${chapterKey}-leaders`} leaders={String(leaders)} />
+ </div>
+ )}
</file context>
| recentChapters: [ | ||
| { | ||
| id: '1', | ||
| createdAt: '2025-01-01T10:00:00Z', |
Contributor
There was a problem hiding this comment.
P2: Mock recentChapters[].createdAt is an ISO date string, but the GraphQL schema and ChapterCard component expect a number (Unix timestamp). The mock should use a numeric timestamp to match the actual response shape. Consider also adding a type annotation (e.g., GetCommunityPageDataQuery) to prevent future mismatches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/__tests__/mockData/mockCommunityData.ts, line 5:
<comment>Mock `recentChapters[].createdAt` is an ISO date string, but the GraphQL schema and `ChapterCard` component expect a `number` (Unix timestamp). The mock should use a numeric timestamp to match the actual response shape. Consider also adding a type annotation (e.g., `GetCommunityPageDataQuery`) to prevent future mismatches.</comment>
<file context>
@@ -0,0 +1,70 @@
+ recentChapters: [
+ {
+ id: '1',
+ createdAt: '2025-01-01T10:00:00Z',
+ key: 'chapter-1',
+ leaders: ['Leader 1', 'Leader 2'],
</file context>
| import { getMemberUrl } from 'utils/urlFormatter' | ||
| import AnchorTitle from 'components/AnchorTitle' | ||
| import CalendarButton from 'components/CalendarButton' | ||
| import ChapterCard from 'components/ChapterCard' |
Contributor
There was a problem hiding this comment.
P2: Visual regression: ChapterCard imports FaCalendar from react-icons/fa (Font Awesome 5), but the rest of the page uses FaCalendar from react-icons/fa6 (Font Awesome 6). The original inline code used the fa6 version. Update the import in ChapterCard.tsx to use react-icons/fa6 for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/page.tsx, line 36:
<comment>Visual regression: `ChapterCard` imports `FaCalendar` from `react-icons/fa` (Font Awesome 5), but the rest of the page uses `FaCalendar` from `react-icons/fa6` (Font Awesome 6). The original inline code used the `fa6` version. Update the import in `ChapterCard.tsx` to use `react-icons/fa6` for consistency.</comment>
<file context>
@@ -33,6 +33,7 @@ import { formatDate, formatDateRange } from 'utils/dateFormatter'
import { getMemberUrl } from 'utils/urlFormatter'
import AnchorTitle from 'components/AnchorTitle'
import CalendarButton from 'components/CalendarButton'
+import ChapterCard from 'components/ChapterCard'
import ChapterMapWrapper from 'components/ChapterMapWrapper'
import ContributorsList from 'components/ContributorsList'
</file context>
Isha-upadhyay
force-pushed
the
feature/community-page
branch
from
68f1904 to
b4479f3
Compare
|
Collaborator
|
Closing in favor of #3912 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
**Resolves #3862
Adds a Community Overview to give users quick context about the OWASP Nest community, including a short description and high level stats.
Community.page.mp4
Checklist
make check-testlocally: all warnings addressed, tests passed