refactor: resolve implicit any errors and improve fe type safety#3289
refactor: resolve implicit any errors and improve fe type safety#3289hussainjamal760 wants to merge 21 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughWidespread TypeScript typing additions and refactors across the frontend: added ambient module declarations, tightened component and GraphQL typings, refactored MultiSearch item rendering and ItemCardList shapes, strengthened Apollo client typings, introduced metrics filter helpers, and updated associated tests and small utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/utils/helpers/githubHeatmap.ts (1)
16-18: Fix the HeatmapData.contributions type.The
contributionsproperty is typed as an empty array[]which doesn't accurately represent its structure. This should be a proper array type matching the actual data shape.🔧 Proposed fix
export interface HeatmapData { years: DataStructYear[] - contributions: [] + contributions: Array<{ date: string; count: number }> }
🧹 Nitpick comments (5)
frontend/src/components/MultiSearch.tsx (1)
266-272: Consider using nullish coalescing (??) for more precise type-safe fallbacks.The current implementation uses
||which treats empty strings,0, andfalseas falsy. For type-safe identifier and display name resolution, the nullish coalescing operator (??) would only fallback onnullorundefined, preserving intentional empty strings.♻️ Proposed refinement using nullish coalescing
{suggestion.hits.map((hit, subIndex) => { const keyId = - ('key' in hit && hit.key) || - ('login' in hit && hit.login) || - ('url' in hit && hit.url) || + ('key' in hit && hit.key) ?? + ('login' in hit && hit.login) ?? + ('url' in hit && hit.url) ?? subIndex - const displayName = hit.name || ('login' in hit ? hit.login : '') + const displayName = hit.name ?? ('login' in hit ? hit.login : '')frontend/src/utils/helpers/githubHeatmap.ts (1)
54-69: Consider refactoring repeated type assertions.The same type assertion
{ date: string; count: number }[]is repeated multiple times on lines 54-55, 57, 67-68. This approach works but creates maintenance overhead and verbosity.♻️ Proposed refactor
Consider extracting and typing the contributions array once to improve readability:
if (!heatmapData.contributions) { return { years: [], contributions: [], } } - heatmapData.contributions = ( - heatmapData.contributions as { date: string; count: number }[] - ).filter( - (item: { date: string; count: number }) => - new Date(item.date) <= endDate && new Date(item.date) >= startDate - ) + + const typedContributions = heatmapData.contributions as { date: string; count: number }[] + heatmapData.contributions = typedContributions.filter( + (item) => new Date(item.date) <= endDate && new Date(item.date) >= startDate + ) const allDates: string[] = [] for (let d = new Date(startDate); d <= endDate; d.setDate(d.getDate() + 1)) { allDates.push(format(d, 'yyyy-MM-dd')) } const transformedContributions: HeatmapContribution[] = allDates.map((date) => { - const contribution = (heatmapData.contributions as { date: string; count: number }[]).find( - (c: { date: string; count: number }) => c.date === date - ) + const contribution = typedContributions.find((c) => c.date === date) return contribution ? { date: contribution.date,frontend/src/utils/metaconfig.ts (2)
77-77: Consider removing redundantString()conversion.Since
pageKeyis typed asMetadataPageKey(a union of string literals fromMETADATA_CONFIG), it's already a string. TheString()wrapper is unnecessary.♻️ Simplified version
- canonicalPath: canonicalPath || `/${String(pageKey).toLowerCase()}`, + canonicalPath: canonicalPath || `/${pageKey}`,
81-81: Type cast is safe but consider improving config typing.The cast is currently valid since all
METADATA_CONFIGentries havetype: 'website'. However, this creates a maintenance risk if the config is later modified.Consider defining the config type structure explicitly to avoid the need for type assertions:
type MetadataConfigEntry = { description: string keywords: string[] pageTitle: string type: 'website' | 'article' | 'profile' }Then use this type when defining
METADATA_CONFIGinutils/metadata.ts.frontend/src/components/AutoScrollToTop.tsx (1)
4-16: Type annotation added, though plain functions are preferred in React 19.The conversion to
FCadds explicit typing, which aligns with the PR's type safety goals. However, React 19 documentation recommends plain function declarations overFCfor simplicity and better inference.If you prefer following React 19 best practices, the plain function approach works equally well:
export default function AutoScrollToTop() { const pathname = usePathname() // ... rest of implementation }Both approaches are valid; this is a style preference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/src/app/about/page.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/AutoScrollToTop.tsxfrontend/src/components/BarChart.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ContributionHeatmap.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/components/ProgramCard.tsxfrontend/src/utils/helpers/githubHeatmap.tsfrontend/src/utils/metaconfig.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ProgramCard.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/AutoScrollToTop.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/components/AutoScrollToTop.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/src/components/BarChart.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/BarChart.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/MultiSearch.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/projects/dashboard/metrics/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/projects/dashboard/metrics/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🧬 Code graph analysis (2)
frontend/src/utils/metaconfig.ts (1)
frontend/src/utils/metadata.ts (1)
METADATA_CONFIG(1-57)
frontend/src/app/about/page.tsx (1)
backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)
🔇 Additional comments (10)
frontend/src/components/MultiSearch.tsx (1)
274-295: LGTM! Excellent semantic HTML restructuring.The refactored rendering structure correctly places the button inside the list item rather than wrapping the list item with a button, which improves semantic HTML and accessibility. The type-safe property access using type guards (
'key' in hit,'login' in hit) properly handles the polymorphic union types while maintaining all interactive behaviors and styling.frontend/src/app/about/page.tsx (1)
324-328: LGTM! Good type-safety improvement.The introduction of the
LeaderUsernametype alias and the safe access pattern with a fallback ensures type-safe lookups in the leaders dictionary while gracefully handling cases where a username might not be present.frontend/src/components/ContributionHeatmap.tsx (1)
183-193: LGTM! Excellent type safety enhancement.The explicit typing of the tooltip's custom function parameters and the typed access to series data significantly improves type safety while maintaining the existing functionality. The optional chaining on line 193 adds appropriate defensive programming.
frontend/src/components/ProgramCard.tsx (1)
38-42: LGTM! Clean type annotation.The explicit
Record<string, string>type annotation improves type safety for the role-to-CSS-class mapping, making the code more maintainable and self-documenting.frontend/src/components/BarChart.tsx (1)
82-98: LGTM! Improved type safety for color callback.The explicit parameter types for the colors callback function enhance type safety and code clarity. The destructured parameters with explicit types make the function's contract clear while maintaining the original coloring logic.
frontend/src/utils/metaconfig.ts (1)
68-70: LGTM! Excellent type safety improvement.The
MetadataPageKeytype alias ensures compile-time validation of valid page keys, preventing runtime errors from invalid keys.frontend/src/app/projects/dashboard/metrics/page.tsx (2)
165-189: Excellent type-safe filter validation pattern.The typed filter keys combined with runtime validation using the
inoperator ensure both compile-time and runtime safety. The pattern correctly:
- Derives types from the mapping objects
- Validates raw query parameters before type assertion
- Safely accesses mappings only with validated keys
This eliminates implicit any errors while preventing invalid property access.
292-299: Consistent type-safe handling in action handler.The action handler correctly applies the same validation pattern used during initialization. The
inoperator checks combined with type assertions ensure safe mapping access.frontend/src/components/CardDetailsPage.tsx (2)
106-106: LGTM! Appropriate use ofPartial<Record<...>>.The
Partial<Record<CardType, string>>type correctly represents a mapping where only someCardTypevalues have entries. The partial nature allows the fallback on line 111 to work as intended.
3-3: Good explicit typing for SocialLinks component.The
SocialLinksPropsinterface andReact.FCtyping improve type safety for this exported component. The import on line 3 supports this typing.Similar to AutoScrollToTop, while
React.FCis acceptable, React 19 documentation recommends plain function declarations for better type inference and simplicity.Also applies to: 367-371
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/MultiSearch.tsx (1)
53-53: Use generic type parameter instead of type assertion for type-safe Algolia results.The type assertion on line 53 is needed because
fetchAlgoliaDatais called without a type parameter, leavingdata.hitsasunknown[]. Instead of asserting the type, provide the proper generic type based on the index name:// Current pattern (type-unsafe): const data = await fetchAlgoliaData(index, query, pageCount, suggestionCount) hits: data.hits as Chapter[] | Event[] | Organization[] | Project[] | User[], // Better pattern (see useSearchPage.ts for reference): const typeMap = { 'chapters': /* Chapter[] */, 'organizations': /* Organization[] */, 'projects': /* Project[] */, 'users': /* User[] */, } const data = await fetchAlgoliaData<TypeForIndex>(index, query, pageCount, suggestionCount)This makes the type flow explicit and eliminates the need for assertions.
🧹 Nitpick comments (1)
frontend/src/components/MultiSearch.tsx (1)
266-272: Consider using nullish coalescing for more precise fallback logic.The current implementation uses logical OR (
||) which will fallback to the next option for any falsy value (empty string,0,false, etc.). While unlikely in practice, ifhit.key,hit.login, orhit.namecould legitimately be empty strings, this logic would incorrectly skip them.♻️ Use nullish coalescing (??) for more precise fallback
- const keyId = - ('key' in hit && hit.key) || - ('login' in hit && hit.login) || - ('url' in hit && hit.url) || - subIndex + const keyId = + ('key' in hit && hit.key) ?? + ('login' in hit && hit.login) ?? + ('url' in hit && hit.url) ?? + subIndex - const displayName = hit.name || ('login' in hit ? hit.login : '') + const displayName = hit.name ?? ('login' in hit ? hit.login : '')This ensures the fallback only occurs when the value is
nullorundefined, not when it's an empty string or other falsy value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/CardDetailsPage.tsxfrontend/src/components/MultiSearch.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
Learnt from: kasya
Repo: OWASP/Nest PR: 1680
File: frontend/src/components/SponsorCard.tsx:3-3
Timestamp: 2025-06-30T00:55:03.133Z
Learning: In the OWASP/Nest project, interfaces or types are not created for component props that are used only once. Inline prop type definitions are preferred for single-use cases.
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/MultiSearch.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (5)
frontend/src/components/CardDetailsPage.tsx (4)
3-3: LGTM! React import enables explicit FC typing.The React import is necessary for the
React.FC<SocialLinksProps>typing used in the SocialLinks component (line 371). While React 19's JSX transform doesn't require importing React for JSX syntax, the import is still needed for TypeScript type references.
106-111: Excellent type safety improvement!The explicit
Partial<Record<CardType, string>>type annotation eliminates implicit any while correctly expressing that only a subset of CardType values need mappings. The fallback pattern on line 111 handles unmapped types safely.
367-369: Well-defined props interface for exported component.The explicit
SocialLinksPropsinterface appropriately types the component props withurls: string[], eliminating implicit any. SinceSocialLinksis exported, defining a reusable interface is appropriate.
371-394: Strongly typed component with safe implementation.The explicit
React.FC<SocialLinksProps>typing eliminates implicit any and provides clear type contracts. The implementation safely guards against empty arrays (line 372) and correctly maps social URLs to icons using thegetSocialIconutility.frontend/src/components/MultiSearch.tsx (1)
274-295: LGTM! Well-structured accessibility improvements.The refactored rendering correctly places the icon and text inside a single button element, improving both semantic correctness and keyboard navigation. The implementation includes:
- Proper type guards for safe property access
- Comprehensive keyboard handling (
Enter,Space)- Appropriate focus styles with outline
- Full-width clickable area for better UX
- Text truncation to prevent overflow
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/src/components/MultiSearch.tsx:
- Around line 267-271: The fallback chain computing keyId is broken because
expressions like ('key' in hit && hit.key) yield false when the property is
absent, and false is not nullish so the ?? chain never advances; replace each
('prop' in hit && hit.prop) piece with a null-returning conditional such as
('prop' in hit ? hit.prop : null) so that keyId = ( 'key' in hit ? hit.key :
null ) ?? ( 'login' in hit ? hit.login : null ) ?? ( 'url' in hit ? hit.url :
null ) ?? subIndex, ensuring the chain properly falls back; update the
expression around keyId, hit, and subIndex accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/MultiSearch.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
🧬 Code graph analysis (1)
frontend/src/components/MultiSearch.tsx (6)
frontend/src/types/chapter.ts (1)
Chapter(4-29)frontend/src/types/event.ts (1)
Event(1-10)frontend/src/types/organization.ts (1)
Organization(1-17)frontend/src/types/project.ts (1)
Project(20-57)frontend/src/types/user.ts (1)
User(11-35)frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(6-59)
🔇 Additional comments (4)
frontend/src/components/MultiSearch.tsx (4)
16-16: LGTM! Clean type alias for Algolia hits.The
AlgoliaHitunion type clearly captures all possible search result types and improves type safety throughout the component.
51-51: LGTM! Explicit generic parameter improves type safety.Adding the
<AlgoliaHit>type parameter eliminates implicitanyand ensures type-safe access to the returned hits.
52-56: LGTM! Nullish coalescing is more semantically correct.The change from
|| 0to?? 0on line 55 is a better choice for numeric defaults, as it only treatsnullandundefinedas triggers rather than all falsy values.
266-296: LGTM! Rendering refactor improves accessibility and type safety.The structural changes are excellent:
- The
displayNamecomputation correctly handles bothnameandloginproperties- Wrapping content in a
<button>inside<li>improves accessibility and keyboard navigation- Moving the icon inside the button makes the entire area properly clickable
- Focus styling and keyboard handlers are appropriate
Once the
keyIdcomputation bug is fixed (see previous comment), this rendering logic will be solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/MultiSearch.tsx (1)
267-271: Type guard logic is sound but consider simplification.The keyId derivation correctly uses type guards with nullish coalescing to safely access properties across the union type. The
??operator properly handlesundefinedvalues from optional properties.However, note that when all property checks fail and
keyIdbecomessubIndex, the React key on Line 277 becomes${subIndex}-${subIndex}, which is redundant but harmless. Consider simplifying the key to justString(keyId)since the keyId fallback already includes subIndex when needed.♻️ Optional: simplify React key generation
Since
keyIdalready falls back tosubIndex, the key can be simplified:- key={`${keyId}-${subIndex}`} + key={String(keyId)}This avoids the redundant
${subIndex}-${subIndex}pattern when keyId defaults to subIndex.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/about/page.tsxfrontend/src/components/MultiSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/about/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
🧬 Code graph analysis (1)
frontend/src/components/MultiSearch.tsx (6)
frontend/src/types/chapter.ts (1)
Chapter(4-29)frontend/src/types/event.ts (1)
Event(1-10)frontend/src/types/organization.ts (1)
Organization(1-17)frontend/src/types/project.ts (1)
Project(20-57)frontend/src/types/user.ts (1)
User(11-35)frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(6-59)
🔇 Additional comments (5)
frontend/src/components/MultiSearch.tsx (5)
16-16: LGTM! Type alias improves clarity.The
AlgoliaHittype alias clearly defines all possible search result types and eliminates implicit any issues when handling search results.
51-51: LGTM! Generic type parameter enhances type safety.Adding the generic type parameter ensures that
data.hitsis correctly typed asAlgoliaHit[], eliminating implicit any and improving IntelliSense support.
52-56: Excellent fix:??is more appropriate than||for numeric defaults.The change from
|| 0to?? 0on Line 55 correctly handles the case wheretotalPagesis legitimately0, since??only treatsnullandundefinedas nullish, whereas||treats all falsy values (including0) as requiring the default.
273-273: Display name logic is sound with expected behavior.The displayName derivation correctly falls back from
nametologinfor Organization and User types. Note that the??operator means an empty string''fornamewill be used rather than falling back tologin. This is likely the intended behavior, but verify that empty names are acceptable in your UI.
275-296: Excellent accessibility improvements with button structure.The refactored rendering structure wraps each suggestion in a proper
<button>element with:
- Correct semantic HTML (
<li>containing<button>)- Proper keyboard event handling for Enter and Space keys
- Focus management with visible outline styles
- Full-width clickable area for better UX
This significantly improves accessibility for keyboard and screen reader users.
|
Hi @rudransh-shrivastava , |
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Nice work, this resolves many implicit any conversions. However, a few still exist.
If you enable noImplicitAny in tsconfig.json, you'll be able to identify other instances of implicit any conversions. I'm not suggesting that we should enable it, but just for identification. I think ignoring the ones in test files should be okay for this PR.
If possible, we might be able to enable it just for the src/ directory using the extends property: https://www.typescriptlang.org/tsconfig/extends.html
Also, separate out type/const definitions as suggested by Ark. I have unresolved that comment.
|
@rudransh-shrivastava Yes, I usually work in the same way. I make all the changes using noimplictany, but I remove it before pushing the code. I will implement the changes you suggested and will verify them using make check-test. I will send it back to you for review. Thank you very much for your valuable feedback. |
hussainjamal760
force-pushed
the
refactor/fe-type-safety
branch
from
77bb549 to
eeb7a9f
Compare
|
@rudransh-shrivastava @arkid15r I have made the requested changes. Please review them. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
164-184:leadersValueis computed but never used; JSX.Element stringification bug persists.The
leadersValuevariable (lines 167-172) correctly handles the case wheredetail.valuemight be aJSX.Element, but line 177 still usesString(detail.value)which will render as[object Object]for JSX elements.🐛 Proposed fix: Use the computed leadersValue
return isLeaders ? ( <div key={detail.label} className="flex flex-row gap-1 pb-1"> <strong>{detail.label}:</strong>{' '} - <LeadersList leaders={detail?.value == null ? 'Unknown' : String(detail.value)} /> + <LeadersList leaders={leadersValue} /> </div> ) : (
🤖 Fix all issues with AI agents
In `@frontend/src/components/CardDetailsPage.tsx`:
- Line 116: The declared variable userLogin (const userLogin = (data as
ExtendedSession)?.user?.login) is referencing undefined data and is unused;
either remove that declaration from CardDetailsPage.tsx, or change it to use the
existing session variable (const userLogin = (session as
ExtendedSession)?.user?.login) and then replace occurrences of
session?.user?.login with userLogin; update or remove the unused ExtendedSession
cast accordingly so there are no reference errors or unused variables.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/app/about/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ProgramCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/ProgramCard.tsx
- frontend/src/app/about/page.tsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-08-10T11:08:47.258Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/api/auth/[...nextauth]/route.ts:13-25
Timestamp: 2025-08-10T11:08:47.258Z
Learning: In the OWASP Nest codebase (frontend/src/app/api/auth/[...nextauth]/route.ts), input validation and string trimming for authentication-related queries like `isProjectLeader` and `isMentor` are handled in the backend rather than the frontend. The backend is responsible for sanitizing and validating input parameters.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🧬 Code graph analysis (1)
frontend/src/components/CardDetailsPage.tsx (1)
frontend/src/types/auth.ts (1)
ExtendedSession(8-20)
🔇 Additional comments (2)
frontend/src/components/CardDetailsPage.tsx (2)
105-109: LGTM!The explicit
Partial<Record<CardType, string>>type annotation correctly represents the map structure and improves type safety.
381-381: LGTM!Adding explicit prop types to
SocialLinksimproves type safety and aligns with the PR's objective of eliminating implicit any types.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@rudransh-shrivastava Kindly review this and let me know if there are any issues, so I can fix them. |
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Hey, I updated apolloClient.ts to return a typed data object. Please update any instances of data as <Query> type assertions that may not be needed anymore. I updated frontend/src/app/api/auth/[...nextauth]/route.ts for reference.
Note: I have not reviewed the following files yet. I will when I get a chance:
frontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/ItemCardList.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/server/apolloClient.ts
Also, please run make check-test locally and make sure everything passes:
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/committees/[committeeKey]/layout.tsx (1)
38-45: Inconsistent metadata fallback: returnsnullinstead of{}.The projects layout (line 41) returns
{}when no project is found, but this committee layout returnsnull. For consistency and type correctness withPromise<Metadata>, consider returning an empty object:Suggested fix for consistency
return committee ? generateSeoMetadata({ canonicalPath: `/committees/${committeeKey}`, description: committee.summary ?? `${committee.name} details`, keywords: ['owasp', 'security', 'committee', committeeKey, committee.name], title: committee.name, }) - : null + : {}
🤖 Fix all issues with AI agents
In `@frontend/src/components/ItemCardList.tsx`:
- Around line 45-50: The prop typing for the ItemCardList component is too
narrow: change the `data` prop declaration on the ItemCardList props to allow
null/undefined (e.g. make it `data?: (Issue | Milestone | PullRequest |
Release)[] | null | undefined`) so it matches guards (`data && data.length > 0`)
and callsites that pass `data?.recentIssues`; update the prop signature where
`data: (Issue | Milestone | PullRequest | Release)[]` is declared (alongside
`icon`, `showAvatar`, `showSingleColumn`, and `renderDetails`) to the widened
nullable/optional form. Ensure no other code relies on non-null `data` without
checking first.
♻️ Duplicate comments (3)
frontend/src/app/chapters/[chapterKey]/layout.tsx (1)
11-27: Consider whether explicit typing is necessary here.The
GetChapterMetadataDocumentis already typed asDocumentNode<GetChapterMetadataQuery, GetChapterMetadataQueryVariables>(per the generated types), which allows Apollo to infer the query result type automatically. The explicit generic parameter and return type annotation are technically redundant.A previous reviewer noted these changes may not be required. If the team prefers relying on TypeScript's inference from the typed
DocumentNode, you could simplify to:const getChapterMetadata = cache(async (chapterKey: string) => { try { const { data } = await apolloClient.query({ query: GetChapterMetadataDocument, variables: { key: chapterKey }, }) return data?.chapter ? data : null } catch { return null } })That said, explicit annotations can aid readability and provide clearer error messages when types drift. Either approach is valid—defer to team preference.
frontend/src/types/declarations.d.ts (1)
1-38: Avoid shadowing react-leaflet’s bundled typings.If the installed react-leaflet version already ships official TypeScript definitions, this ambient module can mask/duplicate them and narrows the prop surface, which runs counter to the type-safety goals of this PR. Prefer removing this block and rely on the package-provided types; if you must add missing pieces, use augmentation that only adds new types rather than redefining exports.
♻️ Proposed removal (use bundled react-leaflet types)
-declare module 'react-leaflet' { - import * as L from 'leaflet' - import * as React from 'react' - - interface MapContainerProps { - center?: L.LatLngExpression - zoom?: number - scrollWheelZoom?: boolean - style?: React.CSSProperties - zoomControl?: boolean - maxBounds?: L.LatLngBoundsExpression - maxBoundsViscosity?: number - children?: React.ReactNode - } - - interface TileLayerProps { - attribution?: string - url: string - className?: string - } - - interface MarkerProps { - position: L.LatLngExpression - icon?: L.Icon | L.DivIcon - title?: string - children?: React.ReactNode - } - - interface PopupProps { - children?: React.ReactNode - } - - export const MapContainer: React.FC<MapContainerProps> - export const TileLayer: React.FC<TileLayerProps> - export const Marker: React.FC<MarkerProps> - export const Popup: React.FC<PopupProps> - export function useMap(): L.Map -}Does the current react-leaflet version used in OWASP/Nest ship built-in TypeScript definitions, and if so should local ambient module declarations be removed?frontend/src/components/ItemCardList.tsx (1)
86-88: Addrel="noopener noreferrer"to the external link withtarget="_blank".Opening external URLs in a new tab without
rel="noopener noreferrer"exposes the page to security and privacy risks: the opened page can accesswindow.openerto manipulate the original tab, and the referrer is leaked. This attribute should be added to the Link component.🔧 Suggested fix
- <Link href={cardItem.url} target="_blank"> + <Link href={cardItem.url} target="_blank" rel="noopener noreferrer">
🧹 Nitpick comments (3)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx (1)
36-44: Consider aligning return value with organization layout for consistency.This returns
nullwhen no repository is found, while the organization layout (frontend/src/app/organizations/[organizationKey]/layout.tsxline 35) returns{}in the same scenario. Both are valid for Next.jsMetadata, but using a consistent approach across layouts improves maintainability.♻️ Suggested change for consistency
return repository ? generateSeoMetadata({ canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`, description: repository.description ?? `${repository.name} repository details`, keywords: ['owasp', 'repository', repositoryKey, repository.name], title: repository.name, }) - : null + : {}frontend/src/app/organizations/[organizationKey]/layout.tsx (1)
105-110: Type annotation is correct; consider caching for deduplication.The generic type is properly applied. Note that this query duplicates the fetch in
generateMetadata(line 20). While outside the scope of this type-safety PR, wrapping in acachefunction (like the repository layout does) would deduplicate these calls during a single render.frontend/src/server/apolloClient.ts (1)
37-38: Unsafe type assertion:null as Tis a type lie.The no-op client returns
{ data: null as T }but the return type claimsPromise<{ data: T }>. This is unsafe because callers expectdatato be of typeT, notnull. While this is intended for E2E tests, consuming code may encounter runtime errors when accessing properties ondata.Consider making the return type honest:
Suggested fix
const noopApolloClient = { - mutate: async <T>(_options?: unknown): Promise<{ data: T }> => ({ data: null as T }), - query: async <T>(_options?: unknown): Promise<{ data: T }> => ({ data: null as T }), + mutate: async <T>(_options?: unknown): Promise<{ data: T | null }> => ({ data: null }), + query: async <T>(_options?: unknown): Promise<{ data: T | null }> => ({ data: null }), }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/ItemCardList.tsx`:
- Around line 26-31: Update the RenderItem type so its date fields match the
source models instead of forcing strings: change createdAt to a union using
Issue['createdAt'] | Milestone['createdAt'] | PullRequest['createdAt'], change
publishedAt to Release['publishedAt'], and add openIssuesCount and
closedIssuesCount as optional numbers; remove the runtime/type cast that masks
these mismatches (the cast currently used where RenderItem instances are
produced) so TypeScript will enforce the correct union types from the source
models.
🧹 Nitpick comments (2)
frontend/src/components/ItemCardList.tsx (2)
56-57: Avoid the broadas RenderItem & BaseItemassertion.After aligning
RenderItemwith the source types, you can drop this cast so TypeScript enforces the shape instead of bypassing it.♻️ Suggested tweak
- const cardItem = item as RenderItem & BaseItem + const cardItem: RenderItem = item
68-83: Default avatar fallback is currently unreachable.Rendering is gated on
author?.avatarUrl, so'/images/default-avatar.png'is never used. If you want a placeholder, gate onauthorinstead; otherwise drop the fallback.🧹 Optional cleanup
- {showAvatar && cardItem?.author?.avatarUrl && ( + {showAvatar && cardItem?.author && ( ... - src={cardItem?.author?.avatarUrl || '/images/default-avatar.png'} + src={cardItem.author.avatarUrl || '/images/default-avatar.png'}
fix(types): add minZoom, maxZoom, worldCopyJump to MapContainerProps
|
@hussainjamal760 could you resolve conflicts on this one? |
@kasya resolved |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3289 +/- ##
==========================================
- Coverage 85.60% 85.60% -0.01%
==========================================
Files 461 461
Lines 14222 14173 -49
Branches 1894 1879 -15
==========================================
- Hits 12175 12133 -42
+ Misses 1678 1674 -4
+ Partials 369 366 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Hi, @rudransh-shrivastava I’ve noticed that the CI/CD pipeline has failed. I’d appreciate your help in analyzing the issue so we can fix it and close this matter. |
|
@hussainjamal760 do you plan to address those or I can close the PR? |
|
Closing as abandoned. |
4 participants
Proposed change
Resolves #3285
This PR focuses on enhancing the frontend code quality by resolving multiple noImplicitAny errors across core components and utility functions. The goal is to move the project towards a stricter and more maintainable TypeScript configuration.
Key Improvements:
Type Refactoring: Converted implicit any types to explicit interfaces in major components.
Polymorphic Safety: Implemented Type Guards ('prop' in object) and keyof typeof patterns to safely handle complex Union types (Issues, Milestones, Releases, etc.).
Build Integrity: Verified that all refactored files pass strict type checks, reducing the risk of runtime property access errors.
Checklist
[x] Required: I read and followed the contributing guidelines
[x] Required: I ran make check-test locally and all tests passed
