Updated Icons in About Page to Use SVGs Instead of Icon Classes #1282
Conversation
Summary by CodeRabbit
WalkthroughThe changes update the icon handling in the About page. In tests and data files, icon string identifiers for various tools have been replaced with paths to SVG files. Additionally, the About page component now uses Next.js’ Image component for icon rendering instead of legacy Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/__tests__/unit/pages/About.test.tsx (1)
257-259: SVG element verification needs improvement.The test checks for mock icons but doesn't properly verify the actual SVG content, which is a missed opportunity for more comprehensive testing.
Consider enhancing the test to verify the actual SVG content:
-const svgElements = screen.getAllByTestId('mock-icon') -expect(svgElements.length).toBeGreaterThan(0) +// Check that at least one SVG element is rendered for each tool +const pythonContainer = screen.getByText('Python').closest('li') +expect(pythonContainer).toContainHTML('svg') + +const reactContainer = screen.getByText('React').closest('li') +expect(reactContainer).toContainHTML('svg')frontend/src/utils/aboutData.ts (5)
57-61: PostgreSQL SVG icon is comprehensive but could be optimized.The PostgreSQL icon SVG is very detailed and includes many path elements. While it renders correctly, it contains some unnecessary elements that could be optimized.
Consider optimizing the PostgreSQL SVG to reduce complexity and file size. You can use tools like SVGO to remove unnecessary paths and attributes:
#!/bin/bash # Install SVGO and optimize SVGs npm install -g svgo # Example usage (not to be run directly) # echo '<svg>...</svg>' | svgo --input - --output -
70-74: TypeScript SVG icon has unnecessary elements.The TypeScript SVG includes a white fill path that's not needed and could cause rendering issues on dark backgrounds.
Remove the unnecessary white fill path:
-<svg viewBox="0 0 128 128" width="24" height="24"> -<path fill="white" d="M22.67 47h99.67v73.67H22.67z"></path><path data-name="original" fill="#9ca3af" d="M1.5 63.91v62.5h125v-125H1.5zm100.73-5a15.56 15.56 0 017.82 4.5 20.58 20.58 0 013 4c0 .16-5.4 3.81-8.69 5.85-.12.08-.6-.44-1.13-1.23a7.09 7.09 0 00-5.87-3.53c-3.79-.26-6.23 1.73-6.21 5a4.58 4.58 0 00.54 2.34c.83 1.73 2.38 2.76 7.24 4.86 8.95 3.85 12.78 6.39 15.16 10 2.66 4 3.25 10.46 1.45 15.24-2 5.2-6.9 8.73-13.83 9.9a38.32 38.32 0 01-9.52-.1 23 23 0 01-12.72-6.63c-1.15-1.27-3.39-4.58-3.25-4.82a9.34 9.34 0 011.15-.73L82 101l3.59-2.08.75 1.11a16.78 16.78 0 004.74 4.54c4 2.1 9.46 1.81 12.16-.62a5.43 5.43 0 00.69-6.92c-1-1.39-3-2.56-8.59-5-6.45-2.78-9.23-4.5-11.77-7.24a16.48 16.48 0 01-3.43-6.25 25 25 0 01-.22-8c1.33-6.23 6-10.58 12.82-11.87a31.66 31.66 0 019.49.26zm-29.34 5.24v5.12H56.66v46.23H45.15V69.26H28.88v-5a49.19 49.19 0 01.12-5.17C29.08 59 39 59 51 59h21.83z"></path> +<svg viewBox="0 0 128 128" width="24" height="24"> +<path data-name="original" fill="#9ca3af" d="M1.5 63.91v62.5h125v-125H1.5zm100.73-5a15.56 15.56 0 017.82 4.5 20.58 20.58 0 013 4c0 .16-5.4 3.81-8.69 5.85-.12.08-.6-.44-1.13-1.23a7.09 7.09 0 00-5.87-3.53c-3.79-.26-6.23 1.73-6.21 5a4.58 4.58 0 00.54 2.34c.83 1.73 2.38 2.76 7.24 4.86 8.95 3.85 12.78 6.39 15.16 10 2.66 4 3.25 10.46 1.45 15.24-2 5.2-6.9 8.73-13.83 9.9a38.32 38.32 0 01-9.52-.1 23 23 0 01-12.72-6.63c-1.15-1.27-3.39-4.58-3.25-4.82a9.34 9.34 0 011.15-.73L82 101l3.59-2.08.75 1.11a16.78 16.78 0 004.74 4.54c4 2.1 9.46 1.81 12.16-.62a5.43 5.43 0 00.69-6.92c-1-1.39-3-2.56-8.59-5-6.45-2.78-9.23-4.5-11.77-7.24a16.48 16.48 0 01-3.43-6.25 25 25 0 01-.22-8c1.33-6.23 6-10.58 12.82-11.87a31.66 31.66 0 019.49.26zm-29.34 5.24v5.12H56.66v46.23H45.15V69.26H28.88v-5a49.19 49.19 0 01.12-5.17C29.08 59 39 59 51 59h21.83z"></path>
86-90: Tailwind CSS SVG icon uses a different structure.Unlike other icons, the Tailwind CSS SVG uses an explicit xmlns attribute and slightly different formatting.
For consistency, update the Tailwind SVG to match the pattern of other icons:
-<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 128 128" width="24" height="24"> +<svg viewBox="0 0 128 128" width="24" height="24">
107-111: Playwright SVG icon is very detailed and could be optimized.The Playwright SVG contains numerous path elements and could benefit from optimization.
Consider simplifying the Playwright SVG to improve performance. You can use tools like SVGO to remove unnecessary precision, paths, and attributes.
36-153: Consider extracting SVG icons to a dedicated utility file.The entire
technologiesarray has grown significantly in size due to the embedded SVG content. This makes the file harder to maintain and review.Consider extracting all SVG icons to a dedicated utility file, making the aboutData.ts file more maintainable:
// New file: src/utils/iconSvgs.ts export const iconSvgs = { python: `<svg viewBox="0 0 128 128" width="24" height="24">...</svg>`, react: `<svg viewBox="0 0 128 128" width="24" height="24">...</svg>`, // ... other icons } // In aboutData.ts import { iconSvgs } from './iconSvgs'; export const technologies = [ { section: 'Backend', tools: { Python: { icon: iconSvgs.python, url: 'https://www.python.org/', }, // ... } } // ... ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/unit/pages/About.test.tsx(5 hunks)frontend/index.html(0 hunks)frontend/src/pages/About.tsx(1 hunks)frontend/src/utils/aboutData.ts(3 hunks)proxy/security.conf(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/index.html
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/pages/About.tsx
[error] 97-97: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (18)
frontend/__tests__/unit/pages/About.test.tsx (9)
31-35: Icon mock implementation is well structured.The SVG implementation for the Python icon is comprehensive and properly formatted. Good job ensuring that the SVG maintains its original structure while using the gray color scheme from the application's design system.
44-48: React icon implementation follows consistent pattern.The SVG for React is properly implemented and follows the same pattern as the other icons. The consistent use of the
#9ca3afcolor ensures that all icons visually match the application's design system.
57-61: Jest icon correctly implemented.The SVG implementation for the Jest icon follows the established pattern and maintains visual consistency with the other icons in the application.
65-69: Pytest icon correctly implemented.The SVG for Pytest is properly formatted and uses the consistent color scheme. Good job maintaining visual consistency across all tool icons.
78-82: Ansible icon correctly implemented.The SVG for Ansible follows the established pattern with proper formatting and consistent coloring.
85-92: Docker icon implementation is comprehensive and well-structured.The Docker SVG is more complex but correctly implemented, maintaining visual consistency with the other icons while preserving the distinctive Docker logo characteristics.
242-246: Updated test assertions properly check for the new tools.The test now correctly verifies the presence of Python, React, Jest, Docker, and Ansible tools in the About page.
248-250: URL verification for Python tool is properly updated.The test correctly verifies that the Python link points to the right URL.
254-256: Jest URL verification is correctly implemented.The test properly checks that the Jest link points to the correct URL, which is important for maintaining accurate resource references.
frontend/src/utils/aboutData.ts (8)
41-45: Python SVG icon is well-implemented.The Python icon's SVG implementation is clean, properly sized, and maintains visual consistency with a uniform color scheme. The SVG viewBox is correctly set and the path definitions are complete.
49-54: Django SVG icon is properly formatted.The Django SVG icon is well-structured and follows the same consistent pattern as the other icons.
78-82: React SVG icon is well-implemented.The React SVG icon is clean, properly sized, and follows the established pattern.
99-103: Jest SVG icon is well-implemented.The Jest SVG icon follows the same pattern and maintains visual consistency.
115-119: Pytest SVG icon is well-formatted.The Pytest SVG follows the established pattern and maintains visual consistency.
128-132: Ansible SVG icon is well-implemented.The Ansible SVG icon follows the established pattern with appropriate sizing and colors.
136-140: GitHub SVG icon is properly implemented.The GitHub SVG icon is correctly formatted with consistent styling.
144-148: Docker SVG icon is very detailed.The Docker SVG contains numerous path elements that define the logo, but it renders well and maintains the visual style consistency.
proxy/security.conf (1)
1-1:❓ Verification inconclusive
Security policy updated to align with SVG icon changes.
The Content-Security-Policy has been modified to replace a specific CDN reference with a more generic
https:directive for style sources. This change aligns with the PR objective of removing dependencies on external icon stylesheets by using inline SVGs instead.While this correctly supports the architectural change, using the generic
https:directive instyle-srcallows styles from any secure origin rather than a restricted set of domains, which broadens the potential attack surface.Consider evaluating whether a more restrictive approach would be sufficient:
-style-src 'self' 'unsafe-inline' https: https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com; +style-src 'self' 'unsafe-inline' https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com;Validate that removing the generic
https:policy doesn't break any required functionality after the SVG implementation.
🏁 Script executed:
#!/bin/bash # Check if there are any other stylesheet references in the codebase that might require the generic https: policy echo "Searching for external stylesheet references in HTML files..." grep -r "<link.*stylesheet.*https://" --include="*.html" . echo "Searching for style imports in CSS/SCSS files..." grep -r "@import.*https://" --include="*.css" --include="*.scss" . echo "Searching for dynamically loaded stylesheets in JS/TS files..." grep -r "\.href.*https://.*\.css" --include="*.js" --include="*.ts" --include="*.tsx" .Length of output: 594
Action Required: Verify Restrictive
style-srcDirectiveThe updated Content-Security-Policy in
proxy/security.confreplaces a specific CDN restriction with the generichttps:directive for style sources. While this aligns with the architectural changes for inline SVGs, it now permits styles from any secure origin. Our automated grep checks did not produce any evidence of external stylesheet references, but the inference may be low-confidence.Please manually verify that:
- Removing the generic
https:fromstyle-src(i.e., limiting allowed style sources only tohttps://owasp-nest.s3.amazonaws.comandhttps://owasp-nest-production.s3.amazonaws.com) does not break any functionality.- No dynamically loaded styles or less obvious stylesheet references exist that might rely on the broader
https:allowance.Proposed diff snippet for testing:
-style-src 'self' 'unsafe-inline' https: https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com; +style-src 'self' 'unsafe-inline' https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com;Please review and verify in your staging or test environment before finalizing this change.
|
@coderabbitai what if i use dangerouslySetInnerHTML after properly sanitizing the svg content |
|
Tip For best results, initiate chat on the files or code changes. @M-ayank2005 Yes, using For proper SVG sanitization:
Your current implementation seems low-risk since you're using pre-defined SVGs directly in your code rather than accepting user input. For static SVGs, this approach offers better performance than repeatedly sanitizing at runtime. Alternatives to consider:
Would you like me to show an example of how to implement any of these alternative approaches? (_____/) Code Analysis ToolsTool Analysis ResultsCodeRabbit ran static analysis tools on your files and found the following: 🪛 Biome (1.9.4)frontend/src/pages/About.tsx[error] 97-97: Avoid passing content using the dangerouslySetInnerHTML prop. Setting content using code can expose users to cross-site scripting (XSS) attacks (lint/security/noDangerouslySetInnerHtml) Files with Issuesfrontend/src/pages/About.tsx🪛 Biome (1.9.4)[error] 97-97: Avoid passing content using the dangerouslySetInnerHTML prop. Setting content using code can expose users to cross-site scripting (XSS) attacks (lint/security/noDangerouslySetInnerHtml) Next StepsBased on these results, you can:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/skeletons/svgRenderer.tsx (2)
14-19: Reconsider using dangerouslySetInnerHTML.As flagged by Biome, using
dangerouslySetInnerHTMLcan expose users to XSS attacks, even with sanitization. Consider these alternatives:
- Use React SVG components directly (for static known SVGs)
- Use SVGR to convert SVGs to React components at build time
- Use a dedicated icon library like react-icons
If you must use this approach:
- Memoize the sanitized output to improve performance
- Add error handling for invalid SVG content
- return ( - <span - className="inline-block" - dangerouslySetInnerHTML={{ __html: sanitizeSVG(svgContent) }} - /> - ); + import { useMemo } from 'react'; + + // Rest of component code... + + const sanitizedSVG = useMemo(() => { + try { + return sanitizeSVG(svgContent); + } catch (error) { + console.error('Failed to sanitize SVG:', error); + return ''; + } + }, [svgContent]); + + return ( + <span + className="inline-block" + dangerouslySetInnerHTML={{ __html: sanitizedSVG }} + /> + );Would you like me to demonstrate how to implement any of these alternative approaches?
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
1-22: Consider implementing a React Error Boundary.Since this component renders potentially unsafe content, it would be good practice to wrap it with an Error Boundary to prevent the entire application from crashing if an SVG rendering fails.
// In a separate file (e.g., ErrorBoundary.tsx) import React, { Component, ErrorInfo, ReactNode } from 'react'; interface Props { children: ReactNode; fallback?: ReactNode; } interface State { hasError: boolean; } class ErrorBoundary extends Component<Props, State> { public state: State = { hasError: false }; public static getDerivedStateFromError(): State { return { hasError: true }; } public componentDidCatch(error: Error, errorInfo: ErrorInfo) { console.error("SVG Renderer error:", error, errorInfo); } public render() { if (this.state.hasError) { return this.props.fallback || <span>❌</span>; } return this.props.children; } } export default ErrorBoundary; // Then in your SVGRenderer component: import ErrorBoundary from './ErrorBoundary'; // Wrap the return statement with: return ( <ErrorBoundary> <span className="inline-block" dangerouslySetInnerHTML={{ __html: sanitizeSVG(svgContent) }} /> </ErrorBoundary> );🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/skeletons/svgRenderer.tsx(1 hunks)frontend/src/pages/About.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/About.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/skeletons/svgRenderer.tsx
[error] 17-17: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
frontend/src/components/skeletons/svgRenderer.tsx (1)
1-5: Well-structured component interface.The interface definition is clear and follows React best practices with TypeScript. The component accepts a single string prop which makes the API simple to understand and use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/components/skeletons/svgRenderer.tsx (4)
8-15: Optimize the sanitizeSVG function by moving it outside the component.The sanitization function is currently defined inside the component, causing it to be recreated on every render. Since it doesn't depend on any component state or props, it should be defined outside for better performance.
import React from 'react'; import DOMPurify from 'dompurify'; interface SVGRendererProps { svgContent: string; } +const sanitizeSVG = (svg: string): string => { + return DOMPurify.sanitize(svg, { + USE_PROFILES: { svg: true, svgFilters: true }, + FORBID_TAGS: ['script'], + FORBID_ATTR: ['onerror', 'onload', 'onclick'] + }); +}; + const SVGRenderer: React.FC<SVGRendererProps> = ({ svgContent }) => { - const sanitizeSVG = (svg: string): string => { - return DOMPurify.sanitize(svg, { - USE_PROFILES: { svg: true, svgFilters: true }, - FORBID_TAGS: ['script'], - FORBID_ATTR: ['onerror', 'onload', 'onclick'] - }); - }; return ( <span className="inline-block" dangerouslySetInnerHTML={{ __html: sanitizeSVG(svgContent) }} /> ); };
13-13: Enhance SVG security by expanding the list of forbidden attributes.The current list of forbidden attributes is good but could be enhanced to include additional potentially dangerous attributes like
hrefwith javascript protocol.return DOMPurify.sanitize(svg, { USE_PROFILES: { svg: true, svgFilters: true }, FORBID_TAGS: ['script'], - FORBID_ATTR: ['onerror', 'onload', 'onclick'] + FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover', 'onmouseout', 'onmousemove', 'formaction'] });
18-21: Improve accessibility for SVG rendering.The current implementation uses a
spantag without accessibility attributes. Even with sanitized SVGs, it's good practice to add proper role and ARIA attributes for screen readers.return ( <span className="inline-block" + role="img" + aria-hidden="true" dangerouslySetInnerHTML={{ __html: sanitizeSVG(svgContent) }} /> );Setting
aria-hidden="true"is appropriate for decorative icons, but if these SVGs convey meaning, consider adding descriptivearia-labelattributes instead.🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
20-20: Address dangerouslySetInnerHTML security concerns with appropriate precautions.The static analysis tool flagged the use of
dangerouslySetInnerHTMLas a potential security issue. While you're already correctly using DOMPurify to sanitize the SVG content, there are a few additional considerations:
- Make sure all SVG content is from trusted sources
- Consider adding a comment explaining why this usage is safe to inform other developers
- Consider adding error handling in case sanitization fails
return ( <span className="inline-block" + /* + * Using dangerouslySetInnerHTML is safe here because: + * 1. All SVG content is sanitized with DOMPurify + * 2. SVG content comes from our controlled sources, not user input + */ dangerouslySetInnerHTML={{ __html: sanitizeSVG(svgContent) }} /> );🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/skeletons/svgRenderer.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/skeletons/svgRenderer.tsx
[error] 20-20: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
|
@arkid15r I’ve made the suggested changes to properly handle SVG icons by sanitizing them to mitigate risks. I created a separate SvgRenderer component as recommended by coderabbitai, and ensured proper sanitization to remove scripts, onClick, and other potentially unsafe attributes. Please review and let me know if it’s good to go. Thanks. |
There was a problem hiding this comment.
@M-ayank2005 thanks for working on this. Could you resolve conflicts before we review this PR?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
13-15: Consider updating FontAwesome mock for consistency with SVG changes.While the SVG icons have been implemented, the FontAwesome mock still returns a generic mock icon. For better test accuracy, you might want to update this mock to reflect how FontAwesome icons are used in the actual component after these changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/About.test.tsx(5 hunks)
🔇 Additional comments (8)
frontend/__tests__/unit/pages/About.test.tsx (8)
32-36: SVG implementation looks good for Python icon.The SVG markup for the Python icon is properly formatted and maintains consistency with a gray (#9ca3af) color scheme. The implementation follows the PR's objective of replacing icon classes with SVG elements.
45-49: SVG implementation looks good for React icon.The React icon SVG is well-structured and maintains the same styling pattern as the other icons with the gray color scheme. Good consistency across icons.
58-62: SVG implementation looks good for Jest icon.The Jest icon SVG implementation follows the same pattern and styling as the other icons, maintaining consistency throughout the test file.
66-70: SVG implementation looks good for Pytest icon.The Pytest icon SVG is properly formatted and follows the consistent styling approach established for all icons.
79-83: SVG implementation looks good for Ansible icon.The Ansible icon SVG maintains the same styling pattern with the consistent gray color scheme, ensuring visual unity across all technology icons.
86-93: SVG implementation looks good for Docker icon.The Docker icon SVG is well-structured and maintains consistent styling with the other icons. This is a good addition that replaces the previous GitHub tool, as indicated in the AI summary.
326-343: Tests correctly updated to verify SVG presence instead of icon classes.The test assertions have been appropriately updated to check for the new SVG elements and their associated links, ensuring the test suite properly validates the updated About page implementation. Lines 341-342 are particularly important as they verify that the SVG elements render correctly.
27-40:✅ Verification successful
Verify SVG sanitization in the actual implementation.
Based on the PR comments, there was a discussion about using
dangerouslySetInnerHTMLwith proper sanitization for SVGs. Ensure that the actualSVGRenderercomponent mentioned in the AI summary properly sanitizes these SVG strings to prevent XSS vulnerabilities.
🏁 Script executed:
#!/bin/bash # Search for SVGRenderer implementation to verify sanitization # The PR comments mentioned concerns about dangerouslySetInnerHTML # Look for SVGRenderer implementation echo "Searching for SVGRenderer component implementation..." rg -A 10 "SVGRenderer" --type tsxLength of output: 178
Below is a new shell script to better locate the
SVGRenderercomponent (and related instances ofdangerouslySetInnerHTML) by forcing the search into TSX files using glob patterns. This will help us verify whether the SVG sanitization is implemented as expected.Please run this script (or verify the output manually) to ensure that the
SVGRenderercomponent exists and properly sanitizes SVG strings when usingdangerouslySetInnerHTML.
🏁 Script executed:
#!/bin/bash echo "Searching for SVGRenderer component implementation in TSX files (using glob filter)..." rg --glob "*.tsx" -A 10 "SVGRenderer" echo "Searching for usage of dangerouslySetInnerHTML in TSX files to verify sanitization..." rg --glob "*.tsx" -A 10 "dangerouslySetInnerHTML"Length of output: 4136
SVG sanitization verified — no further action required.
The implementation in
frontend/src/components/skeletons/svgRenderer.tsxproperly sanitizes SVG content before rendering usingdangerouslySetInnerHTML. Specifically, the component utilizesDOMPurify.sanitizewith appropriate options (enabling SVG profiles, forbidding<script>tags, and disallowing attributes likeonerror,onload, andonclick). This confirms that the XSS mitigations discussed in the PR have been correctly implemented.
- Location verified:
frontend/src/components/skeletons/svgRenderer.tsx(sanitization logic and usage ofdangerouslySetInnerHTML)frontend/src/pages/About.tsxcorrectly imports and usesSVGRenderer
@kasya I’ve resolved the conflicts. Please have a look when you get a chance. Thanks |
github-actions
bot
added
backend
docker
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (31)
trivyignore.yaml (1)
1-7: Review Absolute Path in Secrets ConfigurationThe secrets block ignores the PyJWT metadata using an absolute file path:
/home/owasp/.venv/lib/python3.13/site-packages/PyJWT-2.10.1.dist-info/METADATAConsider whether this absolute path is appropriate for all environments. To improve portability, you might explore using a relative path or a pattern match if supported by your tooling.
.github/workflows/run-ci-cd.yaml (2)
244-251: Environment Variables Update for Staging BuildThe updated script block properly transitions the frontend environment variables from a Vite-specific naming (i.e.
VITE_) to the Next.js compatibleNEXT_PUBLIC_prefix. In addition, using${{ github.event.release.tag_name }}as the release version is a good improvement over the previous date/commit hash based versioning. Please double-check that the associated secrets (still named with theVITE_prefix) are documented appropriately and remain consistent with your deployment configuration.For improved maintainability, consider extracting this repeated setup into a reusable script or action, for example:
+#!/bin/bash +touch frontend/.env +cat <<'EOF' >> frontend/.env +NEXT_PUBLIC_API_URL=${{ secrets.VITE_API_URL }} +NEXT_PUBLIC_CSRF_URL=${{ secrets.VITE_CSRF_URL }} +NEXT_PUBLIC_ENVIRONMENT=${{ secrets.VITE_ENVIRONMENT }} +NEXT_PUBLIC_GRAPHQL_URL=${{ secrets.VITE_GRAPHQL_URL }} +NEXT_PUBLIC_IDX_URL=${{ secrets.VITE_IDX_URL }} +NEXT_PUBLIC_RELEASE_VERSION=${{ github.event.release.tag_name }} +NEXT_PUBLIC_SENTRY_DSN=${{ secrets.VITE_SENTRY_DSN }} +EOF
408-414: Consistent Environment Variables Update for Production BuildThis block in the production image build mirrors the changes made for staging, ensuring that the
NEXT_PUBLIC_prefixed variables are correctly set from the GitHub secrets and release tag. Consistency between staging and production is key, and this update meets that requirement.As with the staging block, consider refactoring this duplicate environment setup into a common script or reusable action to reduce code repetition and simplify future updates.
frontend/src/app/committees/page.tsx (1)
54-67: Add optional chaining for committees arrayConsider using optional chaining when mapping over the committees array to handle potential null/undefined values more safely.
- {committees && committees.map(renderCommitteeCard)} + {committees?.map(renderCommitteeCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 65-65: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/components/LoadingSpinner.tsx (1)
21-34: Well-implemented Next.js Image components with proper attributesBoth Image components are correctly implemented with the required width and height attributes. This change will improve loading performance through Next.js image optimization features like lazy loading and proper sizing.
You might consider adding the
priorityattribute to these images if they appear above the fold, to ensure they're preloaded for an even better user experience.frontend/src/app/layout.tsx (2)
21-29: Consider enhancing metadata for better SEOThe metadata is correctly implemented, but you might want to add more SEO-friendly properties like
openGraph,robotsconfigurations to improve the application's visibility and appearance when shared on social media.export const metadata: Metadata = { title: 'OWASP Nest', description: 'OWASP Nest', icons: { icon: 'https://owasp.org/www--site-theme/favicon.ico', shortcut: 'https://owasp.org/www--site-theme/favicon.ico', apple: 'https://owasp.org/www--site-theme/favicon.ico', }, + openGraph: { + title: 'OWASP Nest', + description: 'OWASP Nest - Open Web Application Security Project', + url: 'https://nest.owasp.org', + siteName: 'OWASP Nest', + images: [ + { + url: 'https://owasp.org/path-to-opengraph-image.jpg', + width: 1200, + height: 630, + alt: 'OWASP Nest', + }, + ], + locale: 'en_US', + type: 'website', + }, + twitter: { + card: 'summary_large_image', + title: 'OWASP Nest', + description: 'OWASP Nest - Open Web Application Security Project', + images: ['https://owasp.org/path-to-twitter-image.jpg'], + }, }
36-51: Well-structured layout following Next.js conventionsThe layout structure follows Next.js best practices with a properly configured html element (including the suppressHydrationWarning attribute for dark mode compatibility), application of font variables to the body, and wrapping everything in the Providers component.
Consider adding
viewportmetadata to ensure proper responsive behavior across devices:export const metadata: Metadata = { title: 'OWASP Nest', description: 'OWASP Nest', + viewport: { + width: 'device-width', + initialScale: 1, + maximumScale: 5, + }, icons: { // ...existing icons }, }frontend/src/app/community/users/page.tsx (1)
63-63: Consider using optional chaining when mapping users array.Since the static analysis tool has suggested it, consider updating the users mapping to include optional chaining to prevent potential errors if
usersis null or undefined.-{users && users.map((user) => <div key={user.key}>{renderUserCard(user)}</div>)} +{users?.map((user) => <div key={user.key}>{renderUserCard(user)}</div>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/chapters/page.tsx (1)
103-103: Consider using optional chaining.Replace the conditional rendering pattern with optional chaining for better readability and conciseness.
- {chapters && chapters.filter((chapter) => chapter.is_active).map(renderChapterCard)} + {chapters?.filter((chapter) => chapter.is_active).map(renderChapterCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/projects/contribute/page.tsx (1)
84-84: Consider using optional chaining.Replace the conditional rendering pattern with optional chaining for better readability and conciseness.
- {issues && issues.map(renderContributeCard)} + {issues?.map(renderContributeCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/community/snapshots/[id]/page.tsx (3)
153-153: Extract filtered data to improve readability.Consider extracting the filtered active chapters to a variable before mapping for better readability and to avoid re-filtering if the component re-renders.
+ const activeChapters = snapshot?.newChapters.filter((chapter) => chapter.isActive); - {snapshot?.newChapters.filter((chapter) => chapter.isActive).map(renderChapterCard)} + {activeChapters.map(renderChapterCard)}
164-164: Extract filtered data to improve readability.Similar to the chapters section, consider extracting the filtered active projects to a variable for better readability and performance.
+ const activeProjects = snapshot?.newProjects.filter((project) => project.isActive); - {snapshot?.newProjects.filter((project) => project.isActive).map(renderProjectCard)} + {activeProjects.map(renderProjectCard)}
127-133: Add null check for formatDate calls.The formatDate function is called with potentially null or undefined values. Consider adding a default empty string or handling this case more explicitly.
- {formatDate(snapshot?.startAt ?? '')} - {formatDate(snapshot?.endAt ?? '')} + {formatDate(snapshot?.startAt || '')} - {formatDate(snapshot?.endAt || '')}frontend/src/app/community/snapshots/page.tsx (1)
68-74: Simplify conditional rendering.The conditional rendering could be simplified for better readability while maintaining the same functionality.
- {!snapshots?.length ? ( - <div className="col-span-full py-8 text-center">No Snapshots found</div> - ) : ( - snapshots.map((snapshot: Snapshots) => ( - <div key={snapshot.key}>{renderSnapshotCard(snapshot)}</div> - )) - )} + {!snapshots?.length && <div className="col-span-full py-8 text-center">No Snapshots found</div>} + {snapshots?.map((snapshot: Snapshots) => ( + <div key={snapshot.key}>{renderSnapshotCard(snapshot)}</div> + ))}frontend/src/app/projects/page.tsx (1)
60-82: Use optional chaining for safer array operationsThe current implementation has a null check, but optional chaining would be more concise and equally safe.
- {projects && projects.filter((project) => project.is_active).map(renderProjectCard)} + {projects?.filter((project) => project.is_active).map(renderProjectCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/__tests__/unit/pages/Home.test.tsx (1)
37-44: Consider removing dangerouslySetInnerHTML from test mocksWhile this is only test code, the use of
dangerouslySetInnerHTMLhas been flagged by static analysis. Consider using a safer alternative in your mock implementation.- return ({ content, className }: { content: string; className?: string }) => ( - <div - className={`md-wrapper ${className || ''}`} - dangerouslySetInnerHTML={{ __html: content }} - /> - ) + return ({ content, className }: { content: string; className?: string }) => ( + <div className={`md-wrapper ${className || ''}`}> + {content} + </div> + )🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/components/Modal.test.tsx (2)
8-16: Consider removing dangerouslySetInnerHTML from MarkdownWrapper mockWhile this is only test code, the use of
dangerouslySetInnerHTMLin the MarkdownWrapper mock has been flagged by static analysis as a potential security risk. Consider using a safer alternative.- return ({ content, className }: { content: string; className?: string }) => ( - <div - data-testid="markdown" - className={`md-wrapper ${className || ''}`} - dangerouslySetInnerHTML={{ __html: content }} - /> - ) + return ({ content, className }: { content: string; className?: string }) => ( + <div + data-testid="markdown" + className={`md-wrapper ${className || ''}`} + > + {content} + </div> + )🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
43-49: Test setup/teardown has redundant clearMocks callsThere's duplication in the test setup/teardown, with
jest.clearAllMocks()being called in bothbeforeEachandafterEach. Consider removing one of these calls to avoid redundancy.beforeEach(() => { jest.clearAllMocks() }) afterEach(() => { - jest.clearAllMocks() jest.restoreAllMocks() })frontend/src/app/chapters/[chapterKey]/page.tsx (1)
16-16: Consider initializing with null instead of empty object.Initializing with an empty object cast as ChapterTypeGraphQL might lead to unexpected behavior when accessing properties before data is loaded. Consider using null or providing sensible defaults.
- const [chapter, setChapter] = useState<ChapterTypeGraphQL>({} as ChapterTypeGraphQL) + const [chapter, setChapter] = useState<ChapterTypeGraphQL | null>(null)frontend/tsconfig.json (1)
35-35: Consider enabling strict type checking.Setting
strict: falsereduces type safety. While this might ease migration to Next.js, consider enabling it in the future to catch type-related issues early.- "strict": false + "strict": truefrontend/src/app/community/users/[userKey]/page.tsx (1)
31-31: Consider initializing with null instead of empty object.Initializing with an empty object cast as HeatmapData might lead to unexpected behavior. Consider using null or providing sensible defaults.
- const [data, setData] = useState<HeatmapData>({} as HeatmapData) + const [data, setData] = useState<HeatmapData | null>(null)frontend/__tests__/unit/pages/Chapters.test.tsx (1)
41-42: Redundant type declarationThere's a redundant type declaration for mockRouter on line 41 as it's also initialized in the beforeEach function on line 47.
- let mockRouter: { push: jest.Mock }frontend/src/components/Modal.tsx (2)
10-10: Renamed component from Modal to DialogComp.Renaming the component to DialogComp better reflects its purpose, but could cause confusion with the imported Modal component. Consider a more distinct name.
-const DialogComp: React.FC<ModalProps> = ({ +const EnhancedModal: React.FC<ModalProps> = ({and
-export default DialogComp +export default EnhancedModalAlso applies to: 67-67
46-46: Replaced Separator component with custom divider.The custom divider implementation works, but consider using a dedicated Divider component from @heroui if available for consistency.
-<div className="inset-0 -m-7 my-[.3rem] h-[.5px] border-gray-200 bg-gray-300 dark:bg-gray-700" /> +<Divider className="my-[.3rem]" />frontend/src/components/MultiSearch.tsx (1)
263-265: Conditional rendering of suggestion label
You're checking'name' in hit ? hit.name : 'login' in hit ? hit.login : ''. Consider adding a more explicit fallback if neither property exists, e.g.'Unknown'.- {'name' in hit ? hit.name : 'login' in hit ? hit.login : ''} + {'name' in hit ? hit.name + : 'login' in hit ? hit.login + : 'Unknown'}frontend/src/components/Card.tsx (1)
97-99: Added fallback for projectLink to prevent navigation errors.The Link now includes an empty string fallback when projectLink is undefined, preventing potential errors. Consider taking this further with conditional rendering to avoid creating unnecessary link elements when no URL is provided.
-<Link href={projectLink || ''} rel="noopener noreferrer" className="mt-2 font-medium"> - {projectName} -</Link> +{projectLink ? ( + <Link href={projectLink} rel="noopener noreferrer" className="mt-2 font-medium"> + {projectName} + </Link> +) : ( + <span className="mt-2 font-medium">{projectName}</span> +)}frontend/src/wrappers/ErrorWrapper.tsx (2)
71-73: Consider reintroducing or replacing Sentry for production error tracking
Commenting out Sentry may reduce visibility into production errors. If you’ve intentionally removed error logging, confirm that this meets your application's observability requirements. Otherwise, consider readding Sentry or using another error tracking solution.
87-96: Removal of Error Boundary
Removing the SentryErrorBoundarymeans runtime errors may go unhandled at the component boundary level. If you still want UI fallback for broken components, consider using a lightweight custom Error Boundary.Would you like assistance creating a small
ErrorBoundaryreplacement if Sentry is no longer desired?frontend/src/components/Header.tsx (3)
29-42: Global click listener for menu closure
Attaching a window-level click event to close the menu is workable but can be fragile. Consider using React refs or a more controlled approach to avoid potential collisions with other click handling.
56-77: Logo rendering with dark & light variants
Multiple<Image>tags effectively handle dark mode. If used across multiple components, consider extracting to a sharedLogocomponent.
160-248: Mobile menu structure
Moving from aDrawerto a fixed sidebar with transform animations is straightforward. Confirm you have tested keyboard/focus accessibility.I can help outline focus-trap or keyboard navigation logic if you want to ensure full accessibility compliance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
backend/poetry.lockis excluded by!**/*.lockcspell/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (149)
.dockerignore(2 hunks).github/workflows/run-ci-cd.yaml(2 hunks).trivyignore(0 hunks)cspell/custom-dict.txt(2 hunks)docker/docker-compose-local.yaml(4 hunks)frontend/.env.example(1 hunks)frontend/.gitignore(1 hunks)frontend/Makefile(1 hunks)frontend/__tests__/e2e/data/mockHomeData.ts(1 hunks)frontend/__tests__/e2e/pages/ChapterDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Chapters.spec.tsx(1 hunks)frontend/__tests__/e2e/pages/Committees.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(2 hunks)frontend/__tests__/e2e/pages/ProjectDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Projects.spec.ts(1 hunks)frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts(0 hunks)frontend/__tests__/e2e/pages/Users.spec.tsx(1 hunks)frontend/__tests__/unit/App.test.tsx(0 hunks)frontend/__tests__/unit/components/MarkdownWrapper.test.tsx(0 hunks)frontend/__tests__/unit/components/Modal.test.tsx(1 hunks)frontend/__tests__/unit/data/mockAboutData.ts(1 hunks)frontend/__tests__/unit/data/mockChapterDetailsData.ts(1 hunks)frontend/__tests__/unit/data/mockHomeData.ts(4 hunks)frontend/__tests__/unit/data/mockProjectData.ts(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(3 hunks)frontend/__tests__/unit/data/mockRepositoryData.ts(3 hunks)frontend/__tests__/unit/data/mockUserData.ts(1 hunks)frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/__tests__/unit/data/mockUserDetailsData.ts(0 hunks)frontend/__tests__/unit/pages/About.test.tsx(9 hunks)frontend/__tests__/unit/pages/ChapterDetails.test.tsx(3 hunks)frontend/__tests__/unit/pages/Chapters.test.tsx(3 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/Committees.test.tsx(4 hunks)frontend/__tests__/unit/pages/Contribute.test.tsx(3 hunks)frontend/__tests__/unit/pages/Home.test.tsx(3 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(3 hunks)frontend/__tests__/unit/pages/Projects.test.tsx(3 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(3 hunks)frontend/__tests__/unit/pages/SnapshotDetails.test.tsx(4 hunks)frontend/__tests__/unit/pages/Snapshots.test.tsx(2 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(5 hunks)frontend/__tests__/unit/pages/Users.test.tsx(4 hunks)frontend/__tests__/unit/utils/utility.test.ts(1 hunks)frontend/components.json(0 hunks)frontend/docker/Dockerfile(1 hunks)frontend/docker/Dockerfile.e2e.test(2 hunks)frontend/docker/Dockerfile.local(1 hunks)frontend/eslint.config.mjs(6 hunks)frontend/global.d.ts(0 hunks)frontend/jest.config.ts(3 hunks)frontend/next.config.ts(1 hunks)frontend/nginx.conf(1 hunks)frontend/package.json(4 hunks)frontend/playwright.config.ts(2 hunks)frontend/postcss.config.js(1 hunks)frontend/scripts/open-coverage.sh(0 hunks)frontend/src/App.tsx(0 hunks)frontend/src/app/about/page.tsx(5 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(3 hunks)frontend/src/app/chapters/page.tsx(3 hunks)frontend/src/app/committees/[committeeKey]/page.tsx(5 hunks)frontend/src/app/committees/page.tsx(3 hunks)frontend/src/app/community/snapshots/[id]/page.tsx(1 hunks)frontend/src/app/community/snapshots/page.tsx(3 hunks)frontend/src/app/community/users/[userKey]/page.tsx(9 hunks)frontend/src/app/community/users/page.tsx(3 hunks)frontend/src/app/globals.css(2 hunks)frontend/src/app/layout.tsx(1 hunks)frontend/src/app/page.tsx(11 hunks)frontend/src/app/projects/[projectKey]/page.tsx(4 hunks)frontend/src/app/projects/contribute/page.tsx(3 hunks)frontend/src/app/projects/page.tsx(3 hunks)frontend/src/app/repositories/[repositoryKey]/page.tsx(5 hunks)frontend/src/components/ActionButton.tsx(3 hunks)frontend/src/components/Card.tsx(4 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/ChapterMap.tsx(3 hunks)frontend/src/components/ChapterMapWrapper.tsx(1 hunks)frontend/src/components/ContributorAvatar.tsx(2 hunks)frontend/src/components/DisplayIcon.tsx(2 hunks)frontend/src/components/Footer.tsx(3 hunks)frontend/src/components/Header.tsx(3 hunks)frontend/src/components/InfoBlock.tsx(1 hunks)frontend/src/components/ItemCardList.tsx(3 hunks)frontend/src/components/LeadersList.tsx(2 hunks)frontend/src/components/LoadingSpinner.tsx(2 hunks)frontend/src/components/LogoCarousel.tsx(3 hunks)frontend/src/components/MarkdownWrapper.tsx(1 hunks)frontend/src/components/MetadataManager.tsx(0 hunks)frontend/src/components/Modal.tsx(1 hunks)frontend/src/components/ModeToggle.tsx(1 hunks)frontend/src/components/MultiSearch.tsx(5 hunks)frontend/src/components/NavButton.tsx(1 hunks)frontend/src/components/Pagination.tsx(4 hunks)frontend/src/components/RecentReleases.tsx(2 hunks)frontend/src/components/ReleaseCard.tsx(0 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/components/ScrollToTop.tsx(2 hunks)frontend/src/components/Search.tsx(2 hunks)frontend/src/components/SearchPageLayout.tsx(2 hunks)frontend/src/components/SecondaryCard.tsx(1 hunks)frontend/src/components/SkeletonsBase.tsx(3 hunks)frontend/src/components/SnapshotCard.tsx(1 hunks)frontend/src/components/SortBy.tsx(3 hunks)frontend/src/components/ToggleableList.tsx(3 hunks)frontend/src/components/TopContributors.tsx(4 hunks)frontend/src/components/UserCard.tsx(2 hunks)frontend/src/components/skeletons/Card.tsx(2 hunks)frontend/src/components/skeletons/UserCard.tsx(1 hunks)frontend/src/components/ui/CloseButton.tsx(0 hunks)frontend/src/components/ui/Dialog.tsx(0 hunks)frontend/src/components/ui/Select.tsx(0 hunks)frontend/src/components/ui/Skeleton.tsx(0 hunks)frontend/src/components/ui/button.tsx(0 hunks)frontend/src/components/ui/toaster.tsx(0 hunks)frontend/src/components/ui/tooltip.tsx(0 hunks)frontend/src/hooks/useModal.ts(1 hunks)frontend/src/hooks/useSearchPage.ts(6 hunks)frontend/src/main.tsx(0 hunks)frontend/src/pages/SnapshotDetails.tsx(0 hunks)frontend/src/pages/index.ts(0 hunks)frontend/src/sentry.config.ts(1 hunks)frontend/src/server/fetchAlgoliaData.ts(3 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/chapter.ts(1 hunks)frontend/src/types/event.ts(1 hunks)frontend/src/types/project.ts(1 hunks)frontend/src/types/seo.ts(0 hunks)frontend/src/types/sortBy.ts(1 hunks)frontend/src/types/user.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)frontend/src/utils/helpers/githubHeatmap.ts(3 hunks)frontend/src/utils/logger.ts(1 hunks)frontend/src/utils/metadata.ts(0 hunks)frontend/src/utils/sortingOptions.ts(1 hunks)frontend/src/utils/theme.ts(0 hunks)frontend/src/utils/urlIconMappings.ts(1 hunks)frontend/src/utils/utility.ts(1 hunks)frontend/src/vite-env.d.ts(0 hunks)frontend/src/wrappers/ErrorWrapper.tsx(4 hunks)frontend/src/wrappers/provider.tsx(1 hunks)frontend/src/wrappers/testUtil.tsx(1 hunks)frontend/tailwind.config.js(2 hunks)frontend/tsconfig.json(1 hunks)frontend/vite.config.ts(0 hunks)trivy.yaml(1 hunks)trivyignore.yaml(1 hunks)
💤 Files with no reviewable changes (26)
- frontend/src/vite-env.d.ts
- .trivyignore
- frontend/tests/e2e/pages/RepositoryDetails.spec.ts
- frontend/src/components/ui/CloseButton.tsx
- frontend/components.json
- frontend/src/utils/metadata.ts
- frontend/src/pages/SnapshotDetails.tsx
- frontend/src/components/ui/button.tsx
- frontend/scripts/open-coverage.sh
- frontend/src/components/ui/toaster.tsx
- frontend/global.d.ts
- frontend/src/main.tsx
- frontend/src/components/MetadataManager.tsx
- frontend/src/components/ReleaseCard.tsx
- frontend/tests/unit/components/MarkdownWrapper.test.tsx
- frontend/src/components/ui/tooltip.tsx
- frontend/tests/unit/data/mockUserDetailsData.ts
- frontend/src/App.tsx
- frontend/src/utils/theme.ts
- frontend/tests/unit/App.test.tsx
- frontend/vite.config.ts
- frontend/src/components/ui/Skeleton.tsx
- frontend/src/components/ui/Dialog.tsx
- frontend/src/types/seo.ts
- frontend/src/pages/index.ts
- frontend/src/components/ui/Select.tsx
✅ Files skipped from review due to trivial changes (21)
- frontend/src/components/MarkdownWrapper.tsx
- cspell/custom-dict.txt
- frontend/postcss.config.js
- frontend/src/types/chapter.ts
- frontend/src/components/ScrollToTop.tsx
- frontend/src/hooks/useModal.ts
- frontend/tests/unit/data/mockUserData.ts
- frontend/tests/unit/data/mockRepositoryData.ts
- frontend/tests/unit/data/mockUserDetails.ts
- frontend/tests/unit/data/mockChapterDetailsData.ts
- frontend/tests/unit/utils/utility.test.ts
- frontend/src/sentry.config.ts
- frontend/src/app/globals.css
- frontend/src/types/event.ts
- frontend/src/utils/logger.ts
- frontend/src/utils/utility.ts
- frontend/tests/unit/data/mockAboutData.ts
- frontend/.gitignore
- frontend/tests/unit/data/mockProjectDetailsData.ts
- frontend/src/utils/credentials.ts
- frontend/.env.example
🧰 Additional context used
🧬 Code Definitions (29)
frontend/src/types/card.ts (1)
frontend/src/types/chapter.ts (1)
GeoLocDataGraphQL(51-55)
frontend/src/components/skeletons/UserCard.tsx (1)
frontend/src/types/skeleton.ts (1)
UserCardSkeletonProps(13-17)
frontend/src/components/RecentReleases.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/LogoCarousel.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/InfoBlock.tsx (1)
frontend/src/utils/pluralize.ts (1)
pluralize(1-4)
frontend/src/components/ContributorAvatar.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/__tests__/unit/pages/Contribute.test.tsx (2)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(7-61)frontend/__tests__/unit/data/mockContributeData.ts (1)
mockContributeData(6-23)
frontend/src/app/projects/page.tsx (3)
frontend/src/types/project.ts (1)
ProjectTypeAlgolia(25-46)frontend/src/utils/utility.ts (1)
getFilteredIcons(22-35)frontend/src/utils/sortingOptions.ts (1)
sortOptionsProject(1-8)
frontend/src/app/community/snapshots/page.tsx (1)
frontend/src/types/snapshot.ts (1)
Snapshots(21-26)
frontend/src/app/chapters/page.tsx (2)
frontend/src/types/chapter.ts (1)
ChapterTypeAlgolia(9-24)frontend/src/utils/utility.ts (2)
getFilteredIcons(22-35)handleSocialUrls(55-63)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/hooks/useSearchPage.ts (1)
frontend/src/wrappers/ErrorWrapper.tsx (1)
handleAppError(64-82)
frontend/src/components/ChapterMapWrapper.tsx (1)
frontend/src/types/chapter.ts (2)
GeoLocDataGraphQL(51-55)GeoLocDataAlgolia(45-49)
frontend/__tests__/unit/pages/Users.test.tsx (2)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(7-61)frontend/__tests__/unit/data/mockUserData.ts (1)
mockUserData(1-18)
frontend/src/app/projects/[projectKey]/page.tsx (2)
frontend/src/types/project.ts (1)
ProjectTypeGraphql(48-69)frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/app/repositories/[repositoryKey]/page.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/__tests__/unit/pages/Home.test.tsx (2)
frontend/__tests__/unit/data/mockHomeData.ts (1)
mockAlgoliaData(143-165)frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(7-61)
frontend/src/app/community/users/page.tsx (1)
frontend/src/types/user.ts (1)
user(3-17)
frontend/src/components/MultiSearch.tsx (1)
frontend/src/types/event.ts (1)
EventType(1-10)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-52)frontend/src/wrappers/testUtil.tsx (1)
render(14-14)
frontend/src/components/UserCard.tsx (1)
frontend/src/types/card.ts (1)
UserCardProps(54-62)
frontend/__tests__/unit/components/Modal.test.tsx (1)
frontend/src/wrappers/testUtil.tsx (1)
render(14-14)
frontend/__tests__/unit/pages/Projects.test.tsx (2)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(7-61)frontend/__tests__/unit/data/mockProjectData.ts (1)
mockProjectData(1-29)
frontend/src/components/Header.tsx (3)
frontend/src/utils/constants.ts (2)
desktopViewMinWidth(113-113)headerLinks(5-26)frontend/src/types/link.ts (1)
Link(1-6)frontend/src/utils/utility.ts (1)
cn(16-18)
frontend/__tests__/unit/pages/Chapters.test.tsx (2)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(7-61)frontend/__tests__/unit/data/mockChapterData.ts (1)
mockChapterData(1-33)
frontend/src/components/RepositoriesCard.tsx (1)
frontend/src/types/project.ts (1)
RepositoryCardProps(75-84)
frontend/src/components/Card.tsx (2)
frontend/src/types/link.ts (1)
Link(1-6)frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-54)
frontend/src/app/about/page.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/Footer.tsx (2)
frontend/src/utils/constants.ts (1)
footerSections(46-98)frontend/src/types/link.ts (1)
Link(1-6)
🪛 Biome (1.9.4)
frontend/src/app/committees/page.tsx
[error] 65-65: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/__tests__/unit/pages/Contribute.test.tsx
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/src/app/projects/page.tsx
[error] 81-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/chapters/page.tsx
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/projects/contribute/page.tsx
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/__tests__/unit/pages/Home.test.tsx
[error] 41-41: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/src/app/community/users/page.tsx
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/__tests__/unit/pages/Committees.test.tsx
[error] 36-36: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/components/Modal.test.tsx
[error] 13-13: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/pages/SnapshotDetails.test.tsx
[error] 33-33: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/pages/Projects.test.tsx
[error] 39-39: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/pages/Chapters.test.tsx
[error] 35-35: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/src/app/about/page.tsx
[error] 102-107: Expected corresponding JSX closing tag for 'a'.
Opening tag
closing tag
(parse)
🔇 Additional comments (353)
frontend/src/components/InfoBlock.tsx (1)
23-25: Good defensive programming to handle undefined unit parameter.This change improves the robustness of the code by providing an empty string fallback when the
unitparameter is undefined. Looking at thepluralizefunction implementation, this prevents potential issues where an undefined value would result in "undefineds" when concatenated with 's'. The multi-line formatting also enhances readability.trivy.yaml (1)
10-10: Confirm Updated Ignore File ReferenceThe
ignorefileproperty is now set totrivyignore.yamlinstead of.trivyignore. Please verify that the new file exists in the expected location and that any CI/CD pipelines or scripts which previously referenced.trivyignorehave been updated accordingly.trivyignore.yaml (1)
8-16: Vulnerability List ValidationThe vulnerabilities section lists several CVE identifiers with descriptive comments. This format is clear and informative. Just ensure that this list is periodically reviewed and maintained as new advisories emerge or dependencies change.
docker/docker-compose-local.yaml (7)
22-23: Backend Service: New Network Configuration
The addition of thenest-networkunder the backend service ensures that it joins the unified network topology. This configuration aligns with the overall objective of improving inter-service connectivity.
25-28: Backend Service: Updated Port Binding and Volume Naming
The port mapping has been simplified to8000:8000by removing the explicit host binding, and the volume is now renamed tobackend-venvfor clearer representation of its purpose. Ensure that any scripts or configurations referencing the old volume name are updated accordingly.
42-43: DB Service: Unified Network Integration
Adding thenest-networkto the DB service standardizes the networking setup across all services. This change facilitates smoother inter-service communication.
57-64: Docs Service: Consistent Networking and Volume Naming
The docs service now leveragesnest-network, uses the port mapping8001:8001, and adopts the new volume namedocs-venv. This update improves consistency with the other services and clarifies the volume’s usage.
75-87: Frontend Service: Enhanced Dependency and Environment Configuration
The frontend service now:
- Specifies a dependency on the backend via the
depends_ondirective.- Includes new environment variables (
NEXT_TELEMETRY_DISABLED=1andNODE_ENV=development) to support the Next.js configuration.- Joins the
nest-networkand updates its volume names tofrontend-next(for build artifacts) andfrontend-node-modules(for node modules).These improvements contribute to a more organized and reliable service orchestration. Please verify these changes with your Next.js build and deployment processes.
89-91: Global Network Declaration:nest-network
The global declaration ofnest-networksolidifies the network structure for the application. This centralized network configuration is a good move for streamlined service communication.
93-98: Global Volume Naming Conventions Updated
The updated volume names—backend-venv,docs-venv,frontend-next, andfrontend-node-modules—reflect a clear and consistent naming convention that enhances clarity and maintainability. Ensure that all references to these volumes are updated in related documentation and scripts.frontend/next.config.ts (1)
1-27: Configuration appears appropriate for Next.js migrationThe Next.js configuration properly defines remote image patterns for external sources including GitHub avatars, OWASP resources, and OpenStreetMap tiles, which aligns well with the PR's objective of enhancing image handling in the About page.
.dockerignore (1)
5-5: Docker build optimization looks goodThe added entries to .dockerignore appropriately exclude Next.js build artifacts (.next), Docker-related files, and logs, which will help optimize Docker build context size and build performance.
Also applies to: 11-11, 23-24, 26-26
frontend/src/components/LeadersList.tsx (1)
1-1: Next.js routing migration implemented correctlyThe component has been properly updated to use Next.js routing by:
- Replacing
react-router-domimport withnext/link- Changing the
toprop tohrefwith the correct path formatThis change successfully aligns with the project's migration to Next.js.
Also applies to: 25-25
frontend/src/components/ChapterMap.tsx (3)
1-2: Client-side component directive and imports properly updatedThe 'use client' directive correctly marks this component for client-side rendering, and the imports have been updated to accommodate Next.js requirements.
Also applies to: 4-5
21-21: Type reference updated correctlyThe markerClusterRef type has been properly updated to use the imported MarkerClusterGroup type.
33-33: Server-side rendering safeguard addedThe early return when
windowis undefined prevents the map initialization code from running during server-side rendering, which is crucial for Next.js compatibility since Leaflet requires browser APIs.frontend/src/utils/helpers/githubHeatmap.ts (8)
21-21: LGTM - Improved type safety for getIntensity functionAdding the explicit number type annotation to the count parameter enhances type safety and code clarity.
29-38: LGTM - Well-structured interfaces for heatmap dataThe new interfaces
HeatmapContributionandHeatmapResponseprovide clear type definitions for the data structures, making the code more maintainable and self-documenting.
40-40: LGTM - Improved function signature with return typeThe updated function signature with proper parameter and return type annotations enhances type safety and improves API documentation.
43-48: LGTM - Enhanced type safety and better return structureThe typed variable declaration and structured return object when contributions are missing provide better type checking and consistent data structure.
54-54: LGTM - Added type annotation for clarityAdding the explicit string[] type annotation to allDates improves code readability and type checking.
59-59: LGTM - Properly typed transformedContributionsThe explicit HeatmapContribution[] type declaration ensures proper type checking for the array elements.
83-83: LGTM - Improved error handlingThe enhanced error handling provides more descriptive error messages based on the error type, improving debugging experience.
225-226: LGTM - Type-safe theme property accessUsing keyof operator to access theme properties ensures type safety and prevents potential runtime errors.
frontend/package.json (6)
3-3: LGTM - Appropriate version bumpIncrementing the version from 0.0.1 to 0.1.0 is appropriate given the significant architectural changes from Vite to Next.js.
6-8: LGTM - Updated scripts for Next.jsThe dev, build, and start scripts have been properly updated to use Next.js commands instead of Vite.
Also applies to: 12-12
10-11: LGTM - Updated ESLint configurationThe lint scripts have been updated to use the .mjs extension for the ESLint configuration file, which is appropriate for ESM modules.
23-32: LGTM - Added HeroUI component libraryThe addition of HeroUI components aligns with the PR objective of updating the UI components, potentially including the SVG icons.
52-53: LGTM - Added Next.js dependenciesAdding Next.js and next-themes as dependencies is essential for the migration from Vite to Next.js.
59-59: LGTM - Updated dependency versionsThe updates to dependency versions are appropriate and help maintain compatibility with the new Next.js-based architecture.
Also applies to: 67-76, 85-85, 105-105
frontend/src/types/project.ts (1)
78-78: LGTM - Added optional key propertyAdding an optional key property to the RepositoryCardProps interface is a good practice for React components, allowing these props to be used directly in lists that require unique key identifiers.
frontend/docker/Dockerfile.local (1)
21-21: LGTM - Created directory for Next.js build artifactsCreating the
/home/owasp/.nextdirectory is necessary for Next.js to store its build artifacts. This change properly aligns the Dockerfile with the migration from Vite to Next.js.frontend/src/utils/urlIconMappings.ts (1)
24-24: Good addition of type definition for improved type safety!Adding the explicit type annotation
: stringto theurlparameter improves type safety and makes the function's expected input clearer.frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
37-40: Great test coverage enhancement!These additional assertions verify that important map control buttons are visible to users, which is crucial for ensuring proper interaction with the map feature. This helps prevent regression issues where these controls might accidentally become hidden.
frontend/src/components/SecondaryCard.tsx (1)
19-19: Good defensive programming with conditional rendering!Adding the conditional check
{icon && ...}prevents rendering the FontAwesomeIcon when the icon prop is undefined or null, making the component more robust against potential errors.frontend/src/types/user.ts (1)
50-50: Good type extension with optional properties!Adding the optional
issuesandreleasesarrays to the User type appropriately extends the data structure to accommodate additional user-related information. The optional marker (?) correctly indicates that these properties might not always be present.Also applies to: 57-57
frontend/src/types/sortBy.ts (1)
2-2: Type definition improved for clarityChanged from
ListCollectionto a more explicit array type with well-defined object shape. This improves type safety and code readability by making the expected structure immediately clear to developers.frontend/src/types/card.ts (1)
35-35: Type enhancement to handle null values explicitlyAdding
| nullto thegeolocationDatatype improves type safety by making it clear that null is an acceptable value, rather than just undefined. This allows for more precise handling of cases where geolocation data is intentionally absent.frontend/src/components/DisplayIcon.tsx (2)
1-1: Updated Tooltip import sourceMigrated from a custom tooltip implementation to the
@heroui/tooltippackage. This change is part of the broader library standardization effort across the codebase.
39-43: Updated Tooltip properties to match new APIThe component props have been updated to match the
@heroui/tooltipAPI:
openDelaychanged todelaypositioningchanged toplacementThis ensures compatibility with the new tooltip library while maintaining the same behavior.
frontend/src/components/NavButton.tsx (1)
2-2: Migrated to Next.js Link componentReplaced Chakra UI's Link component with Next.js's native Link component. This change aligns with the codebase's migration to Next.js routing system and will provide better integration with Next.js features like prefetching and client-side navigation.
frontend/src/components/CardDetailsPage.tsx (1)
13-13: Good implementation of dynamic component loading!The change from directly using
ChapterMapto usingChapterMapWrappersuggests a refactoring to support dynamic imports, which is a recommended pattern in Next.js for optimizing load time and managing client-side-only components. This aligns with modern React best practices for code splitting.Also applies to: 94-94
frontend/Makefile (1)
6-6: Good enhancement to the clean-frontend targetAdding the removal of the
.nextdirectory to the clean-frontend target is necessary for a complete cleanup when working with Next.js applications. This ensures that all build artifacts are properly removed before rebuilding the application.frontend/src/components/SnapshotCard.tsx (1)
3-3: Properly migrated UI component importThe change from Chakra UI to HeroUI for the Button component is consistent with the UI library migration occurring across the application. The implementation remains functionally the same while upgrading to the new component library.
frontend/__tests__/e2e/pages/Contribute.spec.ts (2)
47-47: Good enhancement to test reliabilityAdding a wait for the button to be visible before clicking it is a best practice in E2E testing. This ensures that the element is ready for interaction, reducing flaky tests.
49-49: Improved assertion styleThe change to use
await expect(page).toHaveURL(/page=2/)is a more modern and idiomatic approach in Playwright testing. This pattern is more readable and provides better error messages compared to the previous string-based assertion.frontend/src/components/ChapterMapWrapper.tsx (1)
1-15: Clean implementation of client-side map renderingThis wrapper component correctly implements the pattern for loading map components in Next.js by using dynamic import with
ssr: false. This ensures that the map component only loads on the client side, which is necessary for components that use browser-specific APIs.The component is well-typed with appropriate props that match the expected input for the ChapterMap component.
frontend/src/wrappers/testUtil.tsx (1)
1-8:❓ Verification inconclusive
Verify if tests requiring router context still function correctly
The test utility has been updated to use
HeroUIProviderbut now omits theBrowserRouterthat was previously wrapping the components. This might affect tests that rely on routing functionality.
🏁 Script executed:
#!/bin/bash # Find test files that might be using routing functionality # Check for tests that import useNavigate, useLocation, useParams or other router hooks echo "Tests that might rely on router context:" rg -l "useNavigate|useLocation|useParams|useHistory" --type ts --type tsx "__tests__"Length of output: 204
Action Required: Verify Router-Dependent Tests
The test utility was updated to wrap UI elements with
HeroUIProvider, and the previousBrowserRouterwrapper was removed. The initial check for test files using router hooks (e.g.,useNavigate,useLocation) resulted in an error due to an unrecognized file type (tsx). This makes it unclear whether any tests actually depend on router context.• Next Step:
Please re-run the search using a glob pattern that correctly matches both.tsand.tsxfiles. For example, try:rg -l "useNavigate|useLocation|useParams|useHistory" --glob "*.{ts,tsx}" __tests__• Action:
Verify manually (or with the adjusted command) that no tests require a router context. If tests do rely on routing functionality, consider reintroducing the router context wrapper or updating the tests accordingly.frontend/src/wrappers/provider.tsx (1)
1-20: Well-structured providers implementationThis new Providers component effectively consolidates all the application's context providers in one place, which is good for maintainability. The component correctly:
- Uses the 'use client' directive for Next.js client components
- Implements Suspense for async rendering
- Includes UI, theming, toast notifications, and Apollo client providers
The default theme is hardcoded to "dark", which is fine if that's the application's preference.
frontend/src/app/committees/page.tsx (2)
1-4: Proper migration to Next.js routingThe component has been correctly updated to use Next.js routing with
useRouterfrom 'next/navigation' instead of React Router'suseNavigate.
23-30: Correctly updated navigation logicThe navigation logic has been properly updated to use Next.js routing with
router.push()instead of React Router'snavigate().frontend/src/components/Search.tsx (2)
3-3: UI library migration from Chakra UI to HeroUIThe import has been updated to use the Skeleton component from HeroUI instead of Chakra UI, which aligns with the PR's objective of modernizing the frontend components.
92-92: Simplified styling approach using Tailwind CSSThe Skeleton component styling has been updated to use Tailwind CSS classes instead of individual props, which simplifies the code and is consistent with the project's styling approach.
frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
105-105: Improved URL assertion using Playwright's native methodThis change enhances the test by:
- Using Playwright's dedicated URL assertion method
- Adding proper async/await handling
- Improving readability
The test is now more robust and follows Playwright's best practices.
frontend/src/components/ItemCardList.tsx (3)
2-3: Added Next.js optimized component importsThe imports for Next.js's
ImageandLinkcomponents have been added, which will enable better image optimization and client-side navigation.
41-53: Enhanced avatar display with Next.js componentsReplaced HTML
<a>and<img>tags with Next.js'sLinkandImagecomponents. TheImagecomponent now includes explicit height and width properties, which:
- Eliminates layout shifts during image loading (improves CLS metrics)
- Enables automatic image optimization
- Ensures better accessibility with the alt text fallback
This change aligns with the PR objective of modernizing components and improving performance.
55-62: Improved link handling with Next.js Link componentReplaced the standard
<a>tag with Next.js'sLinkcomponent for the title, which enables client-side navigation and improves the user experience by avoiding full page reloads for internal navigation.frontend/src/components/LogoCarousel.tsx (3)
1-2: Added Next.js optimized component importsThe imports for Next.js's
ImageandLinkcomponents enable better image optimization and client-side navigation capabilities.
33-46: Enhanced sponsor logo display with Next.js componentsReplaced HTML
<a>tag with Next.js'sLinkcomponent and<img>with theImagecomponent using thefillproperty. This change:
- Enables automatic image optimization
- Ensures images properly fill their containers while maintaining aspect ratio
- Improves performance through lazy loading and modern image formats
This implementation aligns with the PR objective of modernizing the frontend components.
55-62: Improved text links with Next.js Link componentReplaced standard
<a>tags with Next.js'sLinkcomponents for the supporting text links, ensuring consistent navigation behavior throughout the application and improving user experience.Also applies to: 67-74
frontend/src/components/RecentReleases.tsx (3)
3-4: Great job migrating to Next.js components!Adding the Next.js
ImageandLinkcomponents is a good practice that will improve performance and user experience.
32-43: Good implementation of Next.js Image with proper dimensionsUsing Next.js
Imagecomponent with explicit dimensions (height/width) helps prevent layout shifts and improves performance through automatic image optimization. The fallback alt text is also a good accessibility practice.
46-49: Well-handled fallback for potentially undefined URLsGood defensive programming by providing a fallback URL (
'/') whenitem?.urlmight be undefined. This ensures users won't encounter broken links.frontend/src/components/LoadingSpinner.tsx (1)
1-1: Good addition of the Next.js Image importAdding the Next.js
Imagecomponent import is the correct first step for the migration.frontend/playwright.config.ts (3)
10-15: Consider keeping mobile device testingYou've commented out the mobile Safari device testing. While this may simplify your immediate test runs, consider if mobile compatibility testing should remain part of your continuous integration to ensure responsive design issues don't creep in.
Is this a temporary measure, or do you plan to test mobile compatibility through other means?
20-20: Increased test timeout is reasonable for complex applicationsSetting a longer timeout (2 minutes) for tests is reasonable for complex applications, especially when using server-side rendering or when tests involve complex data loading.
27-28: Good switch to production build for testingChanging from
npm run devtopnpm run build && pnpm run startis a good practice as it tests against a production build rather than a development server, providing more accurate results. The added timeout for the web server startup is also appropriate for complex builds.frontend/src/app/layout.tsx (2)
1-19: Well-implemented font configuration using Next.js font utilitiesThe Google fonts (Geist and Geist_Mono) are correctly imported and configured with CSS variables, following Next.js best practices. This approach ensures optimal font loading and application.
31-35: Correctly implemented RootLayout function with proper TypeScript typingThe RootLayout function is properly defined with the correct React.ReactNode typing for children and Readonly for immutability, following recommended Next.js patterns.
frontend/src/components/skeletons/UserCard.tsx (6)
1-1: Library migration looks good.The import has been changed from Chakra UI to use the
@heroui/skeletonpackage, which is consistent with the project's goal of modernizing components.
11-11: Well-structured container replacement.Good job replacing the Chakra UI Box component with a standard HTML div element while preserving the component's functionality and appearance.
Also applies to: 32-32
15-15: Clean flex container implementation.Successfully replaced Chakra UI Flex component with a standard div using appropriate Tailwind CSS flex classes, maintaining the original layout.
Also applies to: 25-25
17-19: Effective avatar skeleton implementation.Good implementation of the avatar skeleton using a standard div with proper Tailwind classes and the new Skeleton component with the rounded-full class to achieve the circle effect.
22-22: Name container structure properly maintained.Successfully refactored the name container while maintaining the same layout and spacing.
Also applies to: 24-24
28-28: View profile section properly transitioned.The view profile section has been correctly transitioned from Chakra UI components to standard HTML with appropriate Tailwind classes.
Also applies to: 30-30
frontend/__tests__/unit/data/mockProjectData.ts (2)
6-6: URL updated for consistency.Updated URL from 'https://example.com/project1' to 'https://avatars.githubusercontent.com/project1', which aligns with using GitHub as the source for images.
10-19: Enhanced mock data structure.Great improvement to the
top_contributorsfield, which now provides a more realistic data structure with detailed contributor information (avatarUrl, login, name, etc.) instead of just a string. This will make tests more accurate and representative of real API responses.frontend/src/server/fetchAlgoliaData.ts (4)
5-5: Good import specificity.Explicitly importing
IndexedObjecttype andremoveIdxPrefixfunction makes the dependencies clearer.
12-12: Improved type safety.Adding explicit type annotation
string[]to the facetFilters parameter enhances type safety and code documentation.
19-22: Good defensive programming.Adding a check for IDX_URL being defined before using it prevents potential runtime errors and improves code robustness.
46-46: Enhanced type safety in map function.Adding type annotation
IndexedObjectto the hit parameter in the map function ensures type safety and consistency.frontend/src/components/SearchPageLayout.tsx (2)
2-2: Improved import paths.Changed from relative imports that reference a higher directory to local imports, which is cleaner and suggests proper file organization.
Also applies to: 4-4
58-58: Enhanced dark mode support.Updated class name from "text bg:text-white m-4 text-xl" to "text m-4 text-xl dark:text-white", which properly implements Tailwind's dark mode convention for better theme support.
frontend/src/utils/sortingOptions.ts (1)
1-8:❓ Verification inconclusive
Clean structure upgrade that improves maintainability.
The refactoring from a Chakra UI
createListCollectionto a simple array of objects simplifies the code structure while maintaining the same functionality. The property name change fromvaluetokeyaligns with standard naming conventions.
🏁 Script executed:
#!/bin/bash # Verify that all references to sortOptionsProject have been updated to use 'key' instead of 'value' rg "sortOptionsProject.*value" -g "!*.test.*" -g "!*.spec.*"Length of output: 62
Refactoring Improvement – Manual Verification Requested
The change to a simplified array of objects for
sortOptionsProject, with the property renamed fromvaluetokey, clearly improves maintainability and aligns with standard naming conventions. The automated search didn't reveal any outdated references tovalue, but since the output was inconclusive, please perform a manual verification to ensure all parts of the application are updated accordingly.
- File:
frontend/src/utils/sortingOptions.ts- Verify that all non-test/spec files correctly use the new
keyproperty instead ofvalue.frontend/src/components/Pagination.tsx (2)
3-3: Import update for UI library migration.Switching from Chakra UI to @heroui/button is consistent with the project's library migration mentioned in the PR objectives.
65-65: Event handler updates to match new component API.Changed from
onClicktoonPressto align with @heroui/button component's API. This maintains consistent behavior while adapting to the new library's conventions.Also applies to: 83-83, 92-92
frontend/src/components/UserCard.tsx (4)
3-4: Library migration imports for improved UI components.The switch from Chakra UI to @heroui and addition of Next.js Image component follows the project's migration strategy and introduces performance improvements.
10-10: Updated event handler to match new component API.Changed from
onClicktoonPressto align with @heroui/button component's API, maintaining consistent behavior.
16-16: Performance improvement with Next.js Image component.Replacing the HTML img tag with Next.js's Image component provides automatic image optimization, lazy loading, and prevents layout shifts. The
fillproperty withobjectFit="cover"ensures proper image rendering, and the improved alt attribute fallback enhances accessibility.
28-28: Style update for heading element.Removed the
text-blue-400class from the heading, changing text color from blue to the default gray. This appears to be an intentional design update that aligns with the new styling approach.frontend/__tests__/e2e/pages/Projects.spec.ts (2)
45-45: Enhanced test reliability with visibility checks.Adding explicit wait conditions for button visibility before clicking improves test stability and reduces flakiness by ensuring elements are fully loaded and interactive before attempting to click them.
Also applies to: 52-52
47-47: Improved URL assertion syntax following Playwright best practices.The updated URL assertions use Playwright's recommended
await expect(page).toHaveURL()syntax, which is more concise and expressive than the previous approach. Line 47 uses a regex pattern for partial matching, while line 54 uses an exact string match, appropriate for their respective use cases.Also applies to: 54-54
frontend/nginx.conf (3)
2-2: Port change to standard HTTP portChanging from port 3000 to port 80 is appropriate for standard HTTP traffic, which makes the application accessible without requiring a port specification in the URL.
5-9: Good Next.js static file configurationThe dedicated location block for Next.js static files with a 1-year cache expiration time is a best practice for optimizing performance. Disabling access logging for static assets will also help reduce log volume.
12-17: Proper proxy configuration for Next.jsThe proxy configuration correctly forwards requests to the Next.js server running on port 3000, with all the necessary headers for WebSocket support. This setup is ideal for a production environment where Nginx serves as a reverse proxy in front of Next.js.
frontend/__tests__/e2e/pages/Committees.spec.ts (3)
46-46: Improved test reliability with visibility checkAdding visibility checks before interactions is a best practice that prevents flaky tests by ensuring elements are ready for interaction.
48-48: Better URL assertion using Playwright's idiomatic approachUsing
await expect(page).toHaveURL(/page=2/)is more idiomatic in Playwright than the previous approach. It provides clearer error messages and makes the test more readable.
53-55: Consistent test improvementsAdding visibility checks and using the improved URL assertion pattern consistently throughout the test file enhances reliability and maintainability.
frontend/src/components/ContributorAvatar.tsx (3)
1-3: Updated component libraries to use Next.js and @herouiThe import changes reflect a migration from Chakra UI to Next.js components and @heroui library, which aligns with modern React patterns and the project's overall transition to Next.js.
46-49: Updated tooltip API propertiesThe properties have been updated to match the @heroui/tooltip API:
openDelay→delaypositioning→placementThis ensures compatibility with the new component library.
52-58: Replaced img with Next.js Image componentSwitching to Next.js
Imagecomponent with explicit dimensions is a best practice that improves performance through automatic:
- Image optimization
- Layout shift prevention
- Responsive loading
While maintaining the existing styling classes.
frontend/jest.config.ts (3)
1-3: Enhanced type safety with explicit Config typeAdding explicit type information for the Jest configuration improves type safety and developer experience with better IDE support.
8-16: Refined code coverage exclusionsThe additional exclusions for API, app layout, server code, and types provide more granular control over coverage reporting, focusing it on application logic rather than infrastructure code.
45-46: Improved export styleChanging from direct object export to a named constant export improves code clarity and maintainability, making it easier to add comments or modify the configuration in the future.
frontend/__tests__/e2e/pages/Users.spec.tsx (2)
44-44: Great improvement for test stability!Adding explicit wait for visibility before interacting with elements is a best practice in e2e testing. This helps reduce flaky tests by ensuring elements are actually visible and ready for interaction before clicking.
Also applies to: 53-53
46-46: Good transition to native Playwright assertionsSwitching to Playwright's built-in
expect(page).toHaveURL()assertions from string comparison is an excellent improvement that provides:
- More readable test code
- Better error messages when assertions fail
- Built-in waiting behavior (assertions retry until they pass or timeout)
Also applies to: 55-55
frontend/docker/Dockerfile (2)
9-9: Properly updated copy commands for Next.js configurationThe inclusion of Next.js configuration files aligns with the move to Next.js framework.
24-24: Updated command matches Next.js execution patternUsing
pnpm run startis appropriate for running a Next.js application, which provides both server-side rendering and static generation capabilities.frontend/src/components/SkeletonsBase.tsx (3)
1-1: Good migration to dedicated UI libraryUsing a dedicated UI component library (
@heroui/skeleton) instead of local components helps standardize the UI and improves maintainability.
17-23: Great TypeScript type improvementsAdding explicit TypeScript types for component props is a best practice that:
- Improves code readability
- Enables better IDE autocompletion
- Prevents potential runtime errors by catching type mismatches at compile time
58-58: Good transition to Tailwind classes for stylingSwitching from prop-based height (
h={400}) to utility class-based styling (h-96) provides more consistent styling throughout the application if you're using Tailwind CSS for other components.frontend/src/components/ActionButton.tsx (3)
1-3: Consistent migration to Next.js and @heroui libraryThe imports have been properly updated to use the new UI component library and Next.js routing, aligning with the modernization efforts described in the PR.
46-46: Simplified tooltip implementationThe Tooltip component has been simplified while maintaining its core functionality. Make sure to verify that the new implementation still provides the same accessibility features as the previous version.
34-34:✅ Verification successful
Verify event handler behavior with new component library
The change from
onClicktoonPressreflects the different API of the new component library but should be verified to ensure it behaves the same way in all browser/device contexts.Consider confirming that
onPresshas the same behavior asonClickin various contexts, including keyboard navigation and screen readers. You can check the @heroui documentation to confirm this implementation follows accessibility best practices.
🌐 Web query:
Does @heroui/button onPress prop support keyboard navigation and screen readers?💡 Result:
The
@heroui/buttoncomponent'sonPressprop fully supports keyboard navigation and screen reader accessibility through built-in compliance with web accessibility standards. Key features include:Keyboard Navigation
- Enter/Space activation: Triggers
onPressvia both keys, following WCAG guidelines[8][9].- Focus management: Uses
data-focusanddata-focus-visibleattributes for visible keyboard focus indicators[10].- Event handling: Supports
onKeyDownandonKeyUpprops for custom keyboard interactions[10].Screen Reader Support
- ARIA roles: Implements
role="button"and semantic HTML attributes[8][10].- State announcements: Uses
data-pressedanddata-disabledattributes to communicate button states[10][5].- Accessible labeling: Works with screen readers through proper DOM structure and ARIA attribute handling[5][8].
The component follows React Aria's accessibility patterns[1][9], ensuring consistent behavior across mouse, touch, and keyboard inputs. For disabled states, it uses
aria-disabledinstead of the nativedisabledattribute to maintain keyboard focusability while preventing interactions[5][10].Citations:
- 1: https://react-spectrum.adobe.com/react-aria/Button.html
- 2: https://docs.flutter.dev/ui/accessibility-and-internationalization/accessibility
- 3: https://dev.to/yetillc/accessibility-first-in-react-native-o02
- 4: [BUG] - Button: onPress prop not being invoked on Mouse Events when Select Popover is open heroui-inc/heroui#1680
- 5: https://css-tricks.com/making-disabled-buttons-more-inclusive/
- 6: https://scarcoco.github.io/react-native/docs/0.5/accessibility
- 7: https://www.youtube.com/watch?v=Zah51QMaYLw
- 8: https://www.telerik.com/design-system/docs/components/button/accessibility/
- 9: https://react-spectrum.adobe.com/react-spectrum/Button.html
- 10: https://www.heroui.com/docs/components/button
Verified Accessibility for
onPressEvent HandlerThe new
onPressprop on the<Button>component from@heroui/buttonhas been confirmed to fully support keyboard navigation and screen reader accessibility. According to the documentation, it correctly handles activation via Enter/Space, manages focus via appropriate attributes, and implements standard ARIA roles and state announcements. This behavior aligns with accessibility best practices, ensuring consistent interaction across different devices and browsers.
- Location:
frontend/src/components/ActionButton.tsx(Line 34)- Details: The change from
onClicktoonPressis valid as the new component library guarantees equivalent functionality in terms of accessibility.frontend/__tests__/e2e/pages/Chapters.spec.tsx (2)
44-49: Enhanced test reliability for page navigation.The added wait for button visibility before clicking and the improved URL check with regex pattern make this test more robust against timing issues and false positives.
51-56: Improved test stability for View Details interaction.Adding visibility check and waiting for network idle state before asserting URL changes will prevent flaky test failures due to timing issues.
frontend/src/hooks/useSearchPage.ts (5)
1-3: LGTM: Proper Next.js client directive and imports.The 'use client' directive is correctly added for client-side functionality, and the imports are properly updated to use Next.js navigation hooks.
37-39: Correctly migrated from React Router to Next.js routing.The hook now properly uses Next.js router patterns instead of React Router.
78-79: Proper implementation of Next.js navigation.The URL update logic correctly uses
router.pushfrom Next.js instead of React Router's navigation.
95-95: Added explicit type casting for better type safety.The explicit cast to number helps ensure type consistency.
111-111: Consider behavior change in scroll behavior.The scroll behavior was changed from 'smooth' to 'auto'. Make sure this is intentional and consistent with the desired user experience.
frontend/__tests__/unit/pages/ChapterDetails.test.tsx (3)
5-5: Updated import path for Next.js app directory structure.The import path correctly reflects the new Next.js file-system based routing structure.
23-31: Added proper Next.js navigation mocks.These mocks are necessary for testing components that use Next.js routing hooks.
73-73: Updated mock data structure for error cases.The test now properly returns
{ chapter: null }instead ofnullto match the expected structure in the component.frontend/src/app/about/page.tsx (5)
1-8: Properly migrated imports for Next.js.The 'use client' directive is correctly added, and imports are updated for Next.js compatibility including the toast library and Link component.
18-18: Added SVGRenderer for rendering SVG icons.This change aligns with the PR objective to use SVGs instead of icon classes.
37-44: Enhanced toast configuration.The toast notification now includes additional parameters like timeout, progress indication, and styling, providing a better user experience.
99-101: Implemented SVG rendering for tool icons.Properly replaced icon classes with SVG content using the SVGRenderer component.
124-131: Correctly migrated anchor tag to Next.js Link.Properly replaced the
<a>tag with Next.js<Link>component for better client-side navigation.frontend/src/app/committees/[committeeKey]/page.tsx (5)
1-1: Next.js client component directive properly added.The
'use client'directive correctly marks this component as a client component, which is required for components using client-side features like hooks, event handlers, and browser APIs.
10-14: Imports properly updated for Next.js compatibility.The changes correctly:
- Add
addToastfrom the new@heroui/toastlibrary- Import
Linkfromnext/linkinstead of using HTML anchor tags- Update routing from
react-router-domto Next.js'snext/navigation- Update the GraphQL query import path from
api/queriestoserver/queriesThese changes align well with the project's migration to Next.js.
39-46: Toast notification updated to use the new API.The error notification has been properly updated to use
addToastwith the correct parameters including timeout, progress indication, color, and variant options.
70-72: Anchor tag replaced with Next.js Link component.This change follows Next.js best practices by replacing the HTML
<a>tag with the<Link>component, maintaining proper styling with theclassNameproperty.
91-99: Component structure correctly simplified.The component structure has been simplified by removing the
MetadataManagercomponent while maintaining all the necessary props for theDetailsCardcomponent.frontend/src/app/community/users/page.tsx (5)
1-1: Next.js client component directive properly added.The
'use client'directive correctly marks this component as a client component, which is required for components using hooks and browser APIs.
3-3: Import correctly updated for Next.js routing.The import statement has been properly updated to use
useRouterfromnext/navigationinstead ofuseNavigatefromreact-router-dom.
24-24: Router hook properly initialized.The
useRouterhook is correctly initialized to replace the previoususeNavigatehook.
27-27: Navigation method properly updated.The navigation method has been correctly updated from
navigatetorouter.push, following Next.js's routing conventions.
41-43: User properties now handle undefined values safely.The code has been improved to handle potential undefined values in user properties by adding fallbacks to empty strings.
frontend/__tests__/unit/pages/Snapshots.test.tsx (6)
2-2: Import correctly updated for new toast library.The import statement has been properly updated to use
addToastfrom@heroui/toast.
6-6: Import path updated to match new Next.js file structure.The import path for
SnapshotsPagehas been correctly updated to reflect the new file structure used in Next.js.
8-15: Mock setup properly updated for Next.js routing.The mock setup has been correctly updated to:
- Create a
mockRouterobject with apushmethod- Mock the
next/navigationmodule and itsuseRouterhook to return the mock routerThis approach properly tests the routing functionality in the Next.js context.
22-24: Mock added for new toast library.The mock for
@heroui/toastand itsaddToastfunction has been properly added to support testing the error handling functionality.
98-105: Test expectations updated for new toast implementation.The test expectations have been correctly updated to check for the
addToastcall with the appropriate parameters, including timeout, progress indication, color, and variant options.
120-120: Navigation test properly updated.The navigation test has been correctly updated to verify that
mockRouter.pushis called with the expected URL, replacing the previous navigation method.frontend/eslint.config.mjs (9)
1-4: Imports properly updated for ESLint configuration.The imports have been correctly updated to:
- Import
dirnamespecifically frompath- Add
FlatCompatfrom@eslint/eslintrcfor compatibility with Next.js configuration- Import
pathas a separate moduleThese changes support the transition to the new ESLint flat config format and Next.js integration.
16-16: Next.js ESLint plugin correctly imported.The
@next/eslint-plugin-nextimport has been added to support Next.js specific linting rules.
20-24: Directory configuration and FlatCompat setup properly added.The
__dirnameis now correctly defined, andFlatCompatis properly initialized with the base directory to enable compatibility with Next.js ESLint configurations.
26-27: ESLint configuration structured for Next.js compatibility.The ESLint configuration is now properly structured as an array that extends the Next.js TypeScript configuration, which is necessary for proper linting in a Next.js project.
30-30: Ignored directories updated to include Next.js output directory.The
.nextdirectory has been correctly added to the list of ignored directories, which prevents unnecessary linting of generated files.
54-54: Next.js ESLint plugin properly added to plugins list.The
@next/nextplugin has been correctly added to the list of plugins, enabling Next.js specific linting rules.
73-73: Next.js recommended rules correctly incorporated.The Next.js recommended rules have been properly included in the ESLint configuration, which will help enforce Next.js best practices.
92-92: Path pattern updated to reflect Next.js directory structure.The path group pattern has been correctly updated from
pages/**toapp/**to reflect the migration to Next.js App Router directory structure.
118-118: Export statement correctly updated.The export statement has been properly updated to export the
eslintConfigvariable instead of an inline array, improving the structure and readability of the configuration.frontend/src/app/chapters/page.tsx (3)
1-5: Good update to Next.js routing and imports.The changes to use Next.js routing (
useRouterfromnext/navigation) instead of React Router'suseNavigateare appropriate for a Next.js application. The explicit 'use client' directive and updated import path forfetchAlgoliaDataalign with Next.js best practices.
48-56: Navigation correctly updated for Next.js.The navigation logic has been properly updated to use Next.js routing with
router.push()instead of React Router'snavigate(). This change is consistent with the migration to Next.js architecture.
90-102: ChapterMapWrapper implementation looks good.The addition of the ChapterMapWrapper component with appropriate props (geoLocData, showLocal, and styling) provides a clear visual representation of chapter locations. The styling provides good visual separation with border radius and box shadow.
frontend/src/app/projects/contribute/page.tsx (3)
1-12: Good update to component imports.The changes to use
DialogCompfrom 'components/Modal' and the addition of the 'use client' directive are appropriate for the Next.js framework.
58-67: Modal correctly replaced with DialogComp.The component migration from Modal to DialogComp maintains the same functionality while adhering to the new UI component structure in the application.
73-85: SearchPageLayout implementation is cleaner.The removal of the MetadataManager component simplifies the component structure while maintaining all necessary functionality in the SearchPageLayout component.
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/community/snapshots/[id]/page.tsx (2)
27-47: Well-implemented data fetching and error handling.The GraphQL query setup with useQuery and the error handling with useEffect is well implemented. The error notification using addToast provides a good user experience with appropriate timeout and styling.
106-118: Good loading and error states.The component handles loading and error states well, showing a LoadingSpinner during data fetching and an ErrorDisplay when snapshot data is not found.
frontend/src/app/community/snapshots/page.tsx (4)
1-7: Good update to imports for Next.js and toast notifications.The changes to use Next.js routing and @heroui/toast are consistent with the modernization efforts across the codebase. The updated import path for GraphQL queries is also appropriate.
13-13: Good type definition with explicit null.Explicitly allowing null in the type definition for snapshots state is a good practice for clarity and type safety.
24-31: Well-configured toast notification.The toast notification is well-configured with appropriate timeout, progress indicator, color, and variant properties for a better user experience.
36-40: Navigation correctly updated for Next.js.The navigation logic has been properly updated to use Next.js routing with
router.push()instead of React Router'snavigate(). This is consistent with the migration to Next.js architecture.frontend/src/app/projects/page.tsx (2)
1-3: Implementation of Next.js routing properly handles client-side componentThe change from React Router to Next.js routing is well-implemented. The 'use client' directive at the top correctly marks this as a client component, which is necessary since it uses hooks like
useRouter.
32-38: Navigation implementation updated correctlyThe navigation has been properly updated from React Router's
useNavigateto Next.js'suseRouter. Therouter.pushmethod is correctly used for navigation to the project detail page.frontend/src/components/RepositoriesCard.tsx (2)
10-13: Properly migrated to Next.js routingThe import statements have been correctly updated to use Next.js's router instead of React Router.
51-55: Improved type safety for the RepositoryItem componentThe explicit typing of the
detailsprop using theRepositoryCardPropsinterface enhances type safety and code maintainability.frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (5)
1-7: Updated import paths and toast libraryThe notification system has been properly updated to use
@heroui/toastinstead of the custom toaster, and the import path forSnapshotDetailsPagenow follows the Next.js file structure conventions.
13-15: Toast mock implementation updatedThe mock implementation has been correctly updated to reflect the new toast library's API.
17-25: Router mock implementation for Next.jsThe test now correctly mocks Next.js's router instead of React Router, which is aligned with the codebase migration to Next.js.
92-99: Toast parameters updated for the new libraryThe toast parameters have been correctly updated to match the new library's API, including additional properties like
timeout,shouldShowTimeoutProgress,color, andvariant.
31-35:✅ Verification successful
Security concern with dangerouslySetInnerHTML in MarkdownWrapper mock
While this is in a test context, the use of
dangerouslySetInnerHTMLwithout sanitization could reflect a pattern used in the actual components.
🏁 Script executed:
#!/bin/bash # Verify if actual MarkdownWrapper component uses sanitization with dangerouslySetInnerHTML rg -A 5 -B 5 "dangerouslySetInnerHTML" "frontend/src/components"Length of output: 1446
Clarification on Security Concerns in MarkdownWrapper Test Mock
After verifying the production implementation in
frontend/src/components/MarkdownWrapper.tsx, we found that the component correctly sanitizes rendered HTML usingDOMPurify.sanitizebefore assigning it todangerouslySetInnerHTML. Given that the test mock infrontend/__tests__/unit/pages/SnapshotDetails.test.tsxis solely intended to simulate the component’s behavior without replicating external library behavior, the lack of sanitization in the mock is acceptable in this context. No changes to the test implementation are required.🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/__tests__/unit/pages/Contribute.test.tsx (6)
3-6: Updated import paths for Next.js compatibilityThe import paths have been updated to match the new file structure, with
fetchAlgoliaDatanow imported fromserver/andContributePagefrom the appropriate Next.js path.
12-15: Added mock for Next.js Link componentA simple mock for the Next.js Link component has been added to render its children directly, which is appropriate for tests.
35-43: Proper Next.js router and search params mockingThe Next.js router and search params have been correctly mocked for testing purposes, replacing the previous React Router mocks.
46-51: Updated beforeEach setup for Next.js routerThe test setup now correctly initializes and mocks the Next.js router instead of React Router's navigate function.
154-154: Updated expected page parameter in fetchAlgoliaData callThe test now expects
fetchAlgoliaDatato be called with page 1 instead of 2, reflecting a change in the component implementation.
26-33:✅ Verification successful
Security concern with dangerouslySetInnerHTML in MarkdownWrapper mock
Similar to the other test file, the mock uses
dangerouslySetInnerHTMLwithout sanitization. This is less concerning in a test context but should be verified in the actual component.
🏁 Script executed:
#!/bin/bash # Check if the actual MarkdownWrapper component sanitizes content before using dangerouslySetInnerHTML fd "MarkdownWrapper.tsx" frontend/src --exec cat {}Length of output: 486
Security safety verified: MarkdownWrapper production code properly sanitizes content
The actual MarkdownWrapper component (located in
frontend/src/MarkdownWrapper.tsx) usesDOMPurify.sanitizeon the rendered markdown, ensuring that any usage ofdangerouslySetInnerHTMLis secure. While the test mock infrontend/__tests__/unit/pages/Contribute.test.tsxdirectly appliesdangerouslySetInnerHTMLwithout sanitization, this is acceptable in the isolated test environment.
- Production Component Verification: The file at
frontend/src/MarkdownWrapper.tsxcorrectly applies sanitization using DOMPurify after converting markdown, mitigating security risks.- Test Context: The simplified mock usage in tests does not compromise security, as tests do not expose unsanitized content externally.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/src/components/SortBy.tsx (5)
1-6: Import updates for HeroUI migrationThe component has been updated to import components from the HeroUI library (
@heroui/selectand@heroui/tooltip) rather than the previous UI library.
14-14: Simplified condition checkGood simplification of the condition to check if sortOptions is empty.
18-48: Well-structured HeroUI Select implementationThe implementation has been successfully migrated to use the HeroUI Select component with appropriate styling and behavior.
However, you might consider a small optimization:
- className=""The empty className prop on line 20 could be removed as it doesn't add any value.
52-58: Updated tooltip props to match HeroUI APIThe tooltip props have been correctly updated from
positioningtoplacementandopenDelaytodelayto match the HeroUI API.
59-74: Standard button element replaces previous Button componentThe button implementation has been updated to use a standard HTML button element instead of a component from the previous UI library.
frontend/tailwind.config.js (1)
1-7:❓ Verification inconclusive
Mixed module systems may cause issues
The file mixes CommonJS (
module.exports) with ES module imports (import { heroui }), which could lead to configuration issues.Consider using either all CommonJS or all ES module syntax:
- import { heroui } from '@heroui/react' - module.exports = { + const { heroui } = require('@heroui/react'); + module.exports = {or:
- module.exports = { + export default {Run this script to check if the current configuration works as expected:
🏁 Script executed:
#!/bin/bash # Check if the tailwind configuration is valid cd frontend npx tailwindcss --config tailwind.config.js --help # Look for any errors in the build logs related to tailwind grep -r "tailwind" .next/build-manifest.jsonLength of output: 264
Mixed Module Syntax Detected – Please Verify Manually
The file
frontend/tailwind.config.jscurrently mixes CommonJS (module.exports) with ES module import (import { heroui } from '@heroui/react'), which might lead to configuration issues. We recommend choosing one consistent approach. For example:Option 1 (CommonJS):
- import { heroui } from '@heroui/react' - module.exports = { + const { heroui } = require('@heroui/react'); + module.exports = {Option 2 (ES Modules):
- module.exports = { + export default {The initially provided shell script did not yield conclusive results (errors such as
npx: command not foundand missing build logs). Please perform a manual check in your environment to ensure the Tailwind configuration functions correctly.frontend/src/app/repositories/[repositoryKey]/page.tsx (5)
1-2: Correct use of 'use client' directiveThe
'use client'directive is properly placed at the top of the file, which is required for Next.js client components.
11-15: Updated imports for Next.js and HeroUIThe imports have been correctly updated to use Next.js routing (
next/navigation) and HeroUI toast notifications (@heroui/toast).
34-41: Enhanced toast notification with better UX optionsThe toast notification implementation has been improved with additional options for timeout, progress indicator, color, and variant.
76-78: Next.js Link component replaces anchor tagThe anchor tag has been correctly replaced with Next.js
Linkcomponent, which provides better client-side navigation.
112-123: Simplified component structureThe return statement has been simplified to only include the
DetailsCardcomponent, removing the previously includedMetadataManagercomponent. This makes the code cleaner and more focused.frontend/src/components/ToggleableList.tsx (3)
3-4: Updated imports for Next.js routing and HeroUIThe imports have been correctly updated to use Next.js routing (
useRouterfromnext/navigation) and HeroUI button component (Buttonfrom@heroui/button).
16-21: Migration from React Router to Next.js routingThe navigation logic has been correctly updated to use Next.js
router.push()instead of React Router'snavigate().The explicit typing for the
handleButtonClickparameter is a good practice for type safety.
37-41: Updated Button props for HeroUIThe Button component props have been updated to match the HeroUI API (
onPressinstead ofonClick, addeddisableAnimation).The addition of
bg-transparentto the className ensures the button has a transparent background, which maintains the visual design.frontend/docker/Dockerfile.e2e.test (2)
13-14: Updated configuration files and added public directory for Next.js compatibilityThe COPY commands have been updated to include Next.js-specific configuration files (
next.config.ts,postcss.config.js) and thepublicdirectory, while removing Vite-specific files. This aligns with the project's migration from Vite to Next.js.
1-1:❓ Verification inconclusive
Docker image update from 'noble' to 'jammy'
The base image has been changed from
playwright:v1.51.1-nobletoplaywright:v1.51.1-jammy. This reflects a change in the Ubuntu LTS version being used (noble is Ubuntu 24.04, while jammy is Ubuntu 22.04).
🏁 Script executed:
#!/bin/bash # Check for any specific dependencies or features that might differ between the two Ubuntu versions docker run --rm mcr.microsoft.com/playwright:v1.51.1-jammy cat /etc/os-release | grep VERSIONLength of output: 192
Action Required: Manual Verification of Docker Base Image
The Dockerfile at
frontend/docker/Dockerfile.e2e.testnow uses the base image:FROM mcr.microsoft.com/playwright:v1.51.1-jammyThis change updates the underlying Ubuntu LTS from what was previously
noble(Ubuntu 24.04) tojammy(Ubuntu 22.04). Note that the verification script intended to check the/etc/os-releaseoutput could not run in the current environment because thedockercommand was unavailable.
- Please verify manually (e.g., by running the docker command locally) that the new image correctly reflects the expected Ubuntu version changes.
- Confirm that any dependencies or features impacted by the change in base image remain fully compatible.
frontend/__tests__/unit/pages/Users.test.tsx (5)
3-6: Updated routing imports and paths for Next.js compatibilityThe import statements have been updated to use Next.js routing (
useRouterfromnext/navigation) instead of React Router (useNavigatefromreact-router-dom). Additionally, the API import path has been updated fromapi/fetchAlgoliaDatatoserver/fetchAlgoliaData, and the component import path frompages/Userstoapp/community/users/page, aligning with Next.js file structure conventions.
8-17: Updated mock implementation for Next.js routerThe mock implementation has been updated to support Next.js routing by creating a
mockRouterobject with apushmethod and updating the mock fornext/navigation. This properly mocks the router functionality needed for testing navigation in the component.
19-21: Updated mock path for fetchAlgoliaDataThe mock path for
fetchAlgoliaDatahas been updated to match the new import path in the application code.
37-46: Updated mock router setup in beforeEachThe
beforeEachsetup has been updated to initialize and configure the mock router for each test, ensuring clean test isolation.
124-134: Updated navigation test for Next.js routingThe test for navigation has been updated to use the Next.js router's
pushmethod and verify the correct URL is used when clicking the "View Profile" button (previously "View Details"). This ensures the test properly verifies the component's navigation behavior with the new routing system.frontend/src/app/projects/[projectKey]/page.tsx (6)
1-1: Added 'use client' directiveThe 'use client' directive is required for components that use client-side features like state or event handlers in Next.js. This is necessary because Next.js components are server components by default.
10-12: Updated imports for notifications and Next.js routingThe imports have been updated to use the
addToastfunction from@heroui/toastinstead of a previous toast implementation, and to use Next.jsLinkanduseParamsinstead of their React Router equivalents.
24-24: Explicitly typed state to include nullThe state type for
projecthas been updated to explicitly includenullas a possible value, improving type safety.
36-43: Updated error notification implementationThe error notification has been updated to use
addToastinstead oftoaster.create, with additional parameters for timeout, progress display, and styling. This aligns with the new notification system used throughout the application.
71-71: Updated Link component usageThe
Linkcomponent usage has been updated to use Next.js Link with className for styling instead of a Chakra UI Link component.
98-111: Simplified component renderingThe component rendering has been simplified by directly passing props to the
DetailsCardcomponent instead of using aMetadataManagerwrapper. This improves code readability and reduces unnecessary component nesting.frontend/src/components/Footer.tsx (8)
1-1: Added 'use client' directiveThe 'use client' directive is required for Next.js components that use client-side features like state management.
4-5: Updated component importsReplaced Chakra UI component imports with HeroUI and Next.js equivalents, which aligns with the project's UI library migration.
22-24: Replaced Box components with semantic HTML elementsReplaced Chakra UI Box components with semantic HTML elements (
footeranddiv) using Tailwind CSS classes for styling. This improves accessibility and semantic structure of the page.
28-32: Updated Button component propsThe Button component has been updated to use
onPressinstead ofonClickand added thedisableAnimationprop. The class names have been updated for Tailwind CSS styling, making the button transparent and removing left padding.
35-42: Updated heading and icon container elementsThe heading element has been changed from a Chakra UI Heading component to a standard HTML
h3tag, and the icon container has been updated from a Box to adivwith appropriate transition styling.
44-66: Restructured footer section contentThe footer section content has been restructured to use standard HTML elements with Tailwind CSS classes instead of Chakra UI components. The conditional rendering for links has been maintained, with a default href of '/' if not provided.
70-84: Updated social media icons sectionThe social media icons section has been updated to use a flex container with appropriate spacing, maintaining the same functionality while using standard HTML and Tailwind CSS.
86-92: Simplified copyright sectionThe copyright section has been simplified to use standard HTML elements with Tailwind CSS classes, improving the semantic structure and maintainability of the code.
frontend/src/components/skeletons/Card.tsx (2)
1-1: LGTM - Successful Migration to @heroui/skeletonThe import statement correctly reflects the migration from Chakra UI components to the @heroui library's Skeleton component.
20-81: Complete UI library migration successfully implementedThe component has been successfully refactored from Chakra UI to use Tailwind CSS classes with the @heroui/skeleton library. The migration preserves all functionality while:
- Replacing Box/Flex components with semantic div elements
- Converting SkeletonCircle and SkeletonText to unified Skeleton components with appropriate Tailwind classes
- Maintaining all conditional rendering logic
- Preserving responsive behavior
The code structure is clean, well-organized, and follows modern React patterns.
frontend/__tests__/unit/pages/Home.test.tsx (4)
2-2: Import paths correctly updated for Next.js migrationThe imports have been properly updated to reflect the project's migration to Next.js architecture:
addToastfrom@heroui/toastreplaces the previous toaster componentuseRouterfrom Next.js navigation is correctly importedfetchAlgoliaDatapath has been updated to the new server directory- Home component is now imported from the app directory following Next.js conventions
Also applies to: 5-6, 8-8
15-17: Mocks appropriately updated for Next.js and @heroui componentsThe test mocks have been properly updated to support the new architecture:
- Updated path for
fetchAlgoliaDatamock- Added FontAwesomeIconWrapper mock
- Added mock for @heroui/toast
- Added Next.js router mock
- Added MarkdownWrapper mock
- Added Next.js Link mock
These changes ensure all dependencies are properly mocked for isolated component testing.
Also applies to: 19-26, 28-35, 37-44, 64-66
69-69: Router mock properly initializedThe mock router object is correctly declared and initialized in the beforeEach block, ensuring each test has a fresh router instance.
Also applies to: 78-79
120-127: Toast notification parameters updated to match @heroui/toast APIThe test expectations for error handling have been correctly updated to match the new toast notification API, including additional parameters like
timeout,shouldShowTimeoutProgress,color, andvariant.frontend/__tests__/unit/components/Modal.test.tsx (4)
1-7: Imports updated to support new component structureThe imports have been properly updated to reflect the project's migration to new UI components and testing utilities.
18-22: ActionButton mock simplifiedThe ActionButton mock has been simplified and correctly implements the required functionality for testing.
24-26: Test setup properly updated for new DialogComp componentThe test suite has been correctly renamed to DialogComp and the mock functions and default props have been updated to align with the new component API.
Also applies to: 29-41
51-95: Tests simplified and focused on core functionalityThe test cases have been properly streamlined to focus on essential functionality of the DialogComp component, making the test suite more maintainable while still providing good coverage.
frontend/__tests__/unit/pages/CommitteeDetails.test.tsx (2)
5-5: Import path updated for Next.js pages conventionThe import path for CommitteeDetailsPage has been correctly updated to match Next.js routing conventions.
16-24: Navigation mocks updated from react-router-dom to Next.jsThe navigation mocks have been properly updated to use Next.js router hooks instead of react-router-dom, correctly implementing:
- A mockRouter object with a push method
- useRouter mock returning the mockRouter
- useParams mock returning the expected params
This correctly supports the migration from React Router to Next.js routing.
frontend/src/app/chapters/[chapterKey]/page.tsx (5)
1-1: Appropriate use of 'use client' directive.The 'use client' directive is correctly placed at the top of the file, indicating that this component uses client-side features and should be rendered on the client.
3-5: Good update to Next.js navigation and toast libraries.The changes to import statements properly integrate the Next.js navigation system and the @heroui/toast library for notifications.
7-7: Appropriate path update for GraphQL queries.Updated the import path from 'api/queries' to 'server/queries' which aligns with the project's new structure.
29-36: Improved toast notification configuration.The toast notification system has been enhanced with better configuration options, including timeout, progress indicator, and color settings.
68-77: Enhanced DetailsCard implementation with proper props.The DetailsCard component now receives all required props in a cleaner, more structured format.
frontend/tsconfig.json (5)
5-7: Updated TypeScript configuration for Next.js compatibility.The lib array has been correctly updated to lowercase versions and allowJs has been added to support JavaScript files in the project.
14-15: JSX handling configuration updated for Next.js.Changed jsx mode to "preserve" and added incremental compilation support which are appropriate for a Next.js project.
19-23: Added Next.js plugin configuration.Correctly added the Next.js plugin to the TypeScript configuration to enable Next.js-specific features.
27-34: Updated path mappings for better module resolution.The path mappings have been updated to include specific paths for various modules, enhancing code organization and import clarity.
37-38: Updated includes and excludes for Next.js.Properly configured TypeScript to include Next.js specific files and exclude node_modules.
frontend/src/app/community/users/[userKey]/page.tsx (8)
1-1: Appropriate use of 'use client' directive.The 'use client' directive is correctly placed at the top of the file, indicating that this component uses client-side features.
10-15: Updated imports for Next.js compatibility.The imports have been correctly updated to use Next.js components and navigation hooks.
53-60: Improved toast notification configuration.The toast notification has been enhanced with better configuration options including timeout, progress indicator, and color settings.
67-74: Added proper null check and type safety.Good addition of userKey validation and result type checking to prevent errors during data fetching.
91-95: Improved regex handling for GitHub usernames.The regex matching for GitHub usernames has been enhanced to better capture usernames with punctuation.
214-217: Upgraded to Next.js Image component with proper dimensions.Correctly replaced standard img tags with Next.js Image components and specified width and height for better performance and layout stability.
Also applies to: 224-227
241-247: Upgraded to Next.js Image component for user avatar.Properly updated the user avatar to use Next.js Image component with explicit dimensions.
258-270: Enhanced DetailsCard implementation with proper props.The DetailsCard component now receives all required props in a cleaner, more structured format.
frontend/__tests__/e2e/pages/Home.spec.ts (7)
35-38: Updated test assertions to match new data format.The test assertions have been updated to match the new data format, including leader name, date, and URL format changes from hyphen to underscore.
43-46: Updated project test assertions.Similar to the chapters test, the projects test has been updated to use the new data format with updated leader name, date, and URL structure.
49-54: Simplified test description and updated date.The test description has been simplified from "should have recent posts" to "should have posts" and the date assertion has been updated.
61-61: Updated project name in top contributors test.Changed from "Project 21" to "OWASP Juice Shop" to match the updated mock data.
67-67: Updated date assertion in recent issues test.The date assertion has been updated to match the new mock data.
74-75: Updated date and repository name in releases test.Both the date and repository name assertions have been updated to match the new mock data.
91-91: Updated date in upcoming events test.The date assertion has been updated to match the new mock data.
frontend/src/components/ModeToggle.tsx (5)
3-9: Great job updating to modern theme handling!The use of
useThemefrom next-themes and the implementation of modern UI components from @heroui is a nice improvement over the previous manual theme management approach.
10-12: Improved hydration support with mounted stateUsing a mounted state that only renders after the component is mounted on the client side is a good practice to avoid hydration mismatches with server-side rendering.
14-16: Simplified theme toggling logicThe darkModeHandler has been simplified to leverage the
setThemefunction provided by the useTheme hook, making the code more concise and maintainable.
18-19: Good hydration protectionReturning null when not mounted prevents flash of unstyled content and hydration mismatches. This is a best practice when using themes with SSR.
21-37: Well-implemented accessibility and user experienceThe implementation provides good accessibility through proper aria-labels and tooltips that dynamically update based on the current theme. The use of an icon-only button with proper labeling follows best practices.
frontend/src/components/TopContributors.tsx (5)
4-6: Good migration to Next.js components and routingThe update from Chakra UI to @heroui components and from react-router-dom to Next.js routing shows a consistent approach to modernizing the codebase.
25-26: Updated navigation implementationUsing
useRouterfrom Next.js navigation instead ofuseNavigatefrom react-router-dom aligns with the project's migration to Next.js.
43-44: Updated router usageThe navigation has been properly updated to use Next.js routing with
router.push.
48-54: Improved image handling with Next.js Image componentReplacing the standard HTML
<img>tag with Next.js<Image>component and providing explicit width and height properties is a good practice to prevent layout shifts and improve performance.
73-76: Updated button implementationThe Button component has been updated to use the @heroui API with
onPressinstead ofonClickand added thedisableAnimationprop. The className has also been updated to includebg-transparent.frontend/__tests__/unit/pages/UserDetails.test.tsx (4)
2-8: Updated imports for Next.js and toast handlingThe imports have been appropriately updated to use Next.js routing and the new toast notification system from @heroui/toast.
21-33: Updated routing mock for Next.jsThe mocking strategy has been updated to use Next.js routing hooks instead of react-router-dom, which aligns with the project's migration to Next.js.
41-43: Updated toast mockingThe toast mocking has been updated to use the new @heroui/toast library, ensuring that the tests properly verify the new toast notification system.
245-252: Updated toast parametersThe toast parameters have been updated to use the new @heroui/toast API with additional parameters like timeout, shouldShowTimeoutProgress, color, and variant.
frontend/__tests__/unit/pages/Chapters.test.tsx (4)
4-7: Updated imports for Next.jsThe imports have been appropriately updated to use Next.js routing and updated API paths, which aligns with the project's migration to Next.js.
9-11: Updated mock for server data fetchingThe mock for
fetchAlgoliaDatahas been updated to use the new server path, which is consistent with the project's structure changes.
17-21: Updated routing mock for Next.jsThe mocking strategy has been updated to use Next.js routing hooks and parameters, which allows proper testing of navigation in the Next.js environment.
129-129: Updated navigation testingThe test has been properly updated to verify Next.js navigation using router.push instead of the previous navigation method.
frontend/__tests__/unit/data/mockHomeData.ts (4)
9-9: Consistent URL additions improve test data structure.Adding URL properties to all project entries enhances the mock data's realism and better simulates the actual API responses, which is good for testing navigation functionality.
Also applies to: 17-17, 25-25, 33-33, 41-41
56-56: Updated sponsor imageUrl to use GitHub avatar URL format.The change from a generic URL to a specific GitHub avatar URL format is more realistic and consistent with actual data patterns.
66-66: Added URL properties to improve test data completeness.Adding URL properties to chapters, issues, and releases enhances the test data structure and makes it more suitable for testing navigation and linking functionality.
Also applies to: 88-88, 101-101
93-96: Enhanced author representation with proper object structure.Replacing null value with a proper author object containing avatarUrl and login properties makes the mock data more realistic and aligned with the actual API responses.
frontend/__tests__/unit/pages/Committees.test.tsx (3)
3-6: Updated imports to use Next.js navigation.The migration from React Router's useNavigate to Next.js's useRouter reflects the architectural changes in the application's routing system.
8-20: Mock setup updated for Next.js navigation.The mocking strategy has been updated to properly test the Next.js routing functionality, which is necessary after migrating from React Router.
133-134: Navigation test updated for Next.js routing.The test has been properly updated to verify navigation using the Next.js router's push method instead of React Router's navigate.
Also applies to: 144-144
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (5)
2-2: Updated imports for new toast library and Next.js page structure.The changes reflect the migration to @heroui/toast for notifications and the new Next.js file-based routing structure.
Also applies to: 6-6
17-19: Added mock for new toast library.Properly mocking the new toast library is essential for testing error handling without triggering actual toast notifications.
21-29: Mock setup for Next.js router.The router mock is properly implemented to test navigation functionality in the Next.js context, including the necessary parameters for the repository details page.
91-98: Updated error handling test for new toast API.The test now correctly checks that addToast is called with the appropriate parameters, including the new timeout and variant options available in the @heroui/toast library.
135-135: Navigation assertion updated for Next.js routing.The test now correctly verifies that navigation to a user's page works with the Next.js router implementation.
frontend/src/components/Modal.tsx (3)
1-5: Updated imports to use @heroui component library.The migration from custom Dialog components to @heroui's Modal components aligns with the overall UI library standardization in the project.
21-30: Refactored dialog structure to use @heroui Modal.The structure has been updated to use the @heroui Modal component, which provides better accessibility and user experience features.
55-55: Updated event handler from onClick to onPress.Changed to use the correct event handler for @heroui Button component. This is a necessary update for the component to work correctly.
frontend/src/components/MultiSearch.tsx (7)
11-11: Use ofuseRouterlooks solid.
Switching to Next.js routing is consistent with the rest of the codebase and aligns well with Next.js best practices.
14-14: Updated import path forfetchAlgoliaData.
This change toserver/fetchAlgoliaDataappears correct. Ensure all references align with the new location.
35-35:routerinitialization
Usingconst router = useRouter()is appropriate for Next.js. The local usage seems consistent.
55-55: Fallback assignment fortotalPages
Providing a default of0helps prevent errors whendata.totalPagesis undefined. Good defensive coding.
87-87: Fallback assignment forsuggestion.name
Usingsuggestion.name ?? ''ensures the input remains a string. This is a safe approach.
92-92: Navigation logic for different indexes
Each case appropriately callsrouter.pushwith a distinct path. Ensuresuggestion.keyis always defined to prevent runtime errors.Also applies to: 98-98, 101-101
105-105: Dependency array foruseCallback
Listing[router]is sufficient. No issues spotted.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (7)
2-2: Introduction ofaddToastimport
AdoptingaddToastfor notifications is consistent with your move to@heroui/toast.
6-6: Revised import path forProjectDetailsPage
Importing from'app/projects/[projectKey]/page'aligns with Next.js routing conventions.
13-15: Mocking@heroui/toast
StubbingaddToastis straightforward and allows for clear testing of toast notifications.
21-23:mockRouterdefinition
Defining a simplepushmock is appropriate for verifying navigation in tests.
25-27: Mockingnext/navigation
This approach ensures your test can intercept and validate calls torouter.push.
89-95: Toast message verification
These lines confirm proper error handling withaddToast. The structure looks consistent with your error UI logic.
133-133: Correct push call assertion
TestingmockRouter.push('/community/users/contributor1')ensures the intended route is triggered.frontend/__tests__/unit/pages/About.test.tsx (27)
2-2: New import foraddToast
Integration with@heroui/toastmatches the broader adoption seen elsewhere in the codebase.
5-5: Migration touseRouter
Usingnext/navigationis a step toward full Next.js routing usage. Good job keeping the code standard.
7-7: ImportingAboutfromapp/about/page
The updated import path correctly references the new page location under Next.js app routing.
14-20: Defining and usingmockRouter
OverridinguseRouterfor test mocks is consistent and ensures calls torouter.pushcan be inspected.
26-28: Mocking@heroui/toast
Stubbing outaddToastallows controlling and verifying toast calls. Implementation looks good.
45-49: Python icon SVG
Storing the raw SVG string is valid for inline usage. Make sure the final rendering includes safe sanitization if it’s not strictly developer-controlled.
58-62: React icon SVG
Inline SVG strings are consistent. Keep sanitization in mind if any external input is ever introduced.
71-75: Jest icon SVG
Inlined icon content is valid. Continue verifying these are developer-controlled and safe.
79-83: Pytest icon SVG
All statements seem correct. This is in line with the approach used for other tool icons.
92-95: Ansible icon SVG
No issues noted. The code matches the pattern used in other icons.
99-104: Docker icon SVG
Properly included. The chunk of SVG is consistent with the other tools.
254-254: Assertion forPython
Simple check ensures the icon text is rendered. Looks good.
256-258: Assertion forJestandDocker,Ansible
Verifying presence of these tool names is consistent with the rest of the tests.
260-261: Python link verification
Ensures correct external link usage.
263-263: React link verification
Checks that the React entry has its correct url.
266-267: Jest link verification
Asserting the link is accurate for the Jest official site.
269-270: Ensuring SVG icons are rendered
getAllByTestId('mock-icon')is an effective way to confirm the icon placeholders.
301-305: GraphQL error handling for project data
The test thoroughly checks fallback UI when data is not found and triggers a toast. This flow appears correct.Also applies to: 310-310, 313-320
323-324: Leader data loading state
You handle the asynchronous load scenario gracefully, verifying both the spinner and eventual user data.Also applies to: 328-335, 340-341, 343-349
353-364: Handling null user data
Your test ensures fallback messages appear when specific leaders’ info is missing. Good coverage.Also applies to: 369-370, 373-375, 377-379
381-399: Null project data
You confirm the UI properly renders error messaging, addressing a critical edge case. Nicely done.
401-420: Undefined user data
Verifies fallback usage for partially available data. This thorough check ensures robust error handling.
422-454: Partial user data scenario
Demonstrates the component’s resilience if user objects lack thenamefield. This coverage is comprehensive.
457-475: Fallback user data
Ensuring a graceful fallback if the user’s data is missing. Test logic is consistent.
478-495: Loading spinner for project data
Validates that a spinner is displayed while project data loads. No issues found.
497-515: ErrorDisplay with null project
The test properly checks the “Data not found” scenario and a 404-like message.
517-538: Toaster error for GraphQL failure
Test coverage looks solid, confirming the error toast is fired with the correct parameters.frontend/__tests__/unit/pages/Projects.test.tsx (4)
3-4: Correct migration from React Router to Next.js navigation.The import changes from
react-router-domtonext/navigationcorrectly align with the transition to Next.js routing system, and the fetchAlgoliaData import path has been properly updated to match the new server directory structure.
8-17: Good implementation of the Next.js router mock.The mock router implementation correctly simulates the Next.js router behavior, particularly the
pushmethod that will be tested. The mock setup is comprehensive, including additional router functionality likeuseParamsanduseSearchParams.
44-53: Good test setup for the router mock.The test setup correctly initializes and resets the mockRouter between tests, ensuring test isolation and preventing test pollution.
133-139: Updated test assertion for Next.js navigation.The test now correctly uses
screen.getByRolewith a name attribute to find the button, which is more accessible and robust than searching by text. The assertion has been updated to verify the router's push method is called with the correct path.frontend/__tests__/e2e/data/mockHomeData.ts (4)
5-13: Updated mock data with consistent key format and additional fields.The mock data has been updated with:
- Future-dated timestamps (2025)
- Standardized key format using underscores
- New fields: openIssuesCount and repositoriesCount
These changes align with the data structure expected by the application.
39-59: Enhanced post mock data with realistic URLs and image paths.The mock data now includes more realistic URLs and image paths that better represent actual blog posts, which improves the fidelity of the test data.
229-266: Enhanced event mock data with descriptive summaries.The event mock data has been significantly improved with detailed, realistic summaries that better simulate actual event content. This provides better test data for components that display this information.
155-161:❓ Verification inconclusive
Verify title vs authorName mismatch in post data.
There appears to be a potential inconsistency in the third post object where the title field (line 56) contains "Author 3" which matches the authorName field (line 53) rather than being a proper post title.
🏁 Script executed:
#!/bin/bash # Check for any references to "Author 3" as a title in actual components # Search in component files rg "post\.title" -A 2 -B 2 --glob "frontend/src/components/*.tsx" # Also check if this is used in any tests rg "title: 'Author 3'" --glob "frontend/src/**/*.tsx"Length of output: 129
Next Steps: Investigate In-Test Data Inconsistency
It appears the initial search in component files yielded no results because the potential mismatch is isolated to the e2e test data file. To gather more context on the issue in the test data (specifically within
frontend/__tests__/e2e/data/mockHomeData.ts), please run the following shell script to inspect occurrences of'Author 3'in the title field:#!/bin/bash # Check for 'Author 3' occurrences along with surrounding context in the test data file. rg -n -C 3 "title: 'Author 3'" frontend/__tests__/e2e/data/mockHomeData.tsThis will help verify whether the title field in the third post object is indeed set to
"Author 3"(matching the authorName) instead of a proper post title. Once you review the output from this script, we can proceed with any necessary fixes.frontend/src/app/page.tsx (6)
17-22: Updated imports for Next.js and HeroUI integration.The import updates align with the migration to Next.js (using Link from next/link) and the transition to the HeroUI component library (using addToast from @heroui/toast). The data fetching imports now correctly reference the server directory.
59-67: Enhanced toast implementation with improved configuration.The toast notification has been updated to use the new HeroUI toast API with additional configuration options for timeout, visual progress indicator, and styling variants, providing a better user experience.
112-125: Added optional chaining for safer data access.Optional chaining has been added when accessing data properties, preventing potential runtime errors if data is undefined. This is a good defensive programming practice.
151-198: Transition to Next.js Link component for chapters.The anchor tags have been correctly replaced with Next.js Link components, maintaining the same functionality while benefiting from Next.js's optimized client-side navigation.
224-229: Simplified Link implementation for projects.The Link component implementation for projects has been streamlined, wrapping only the essential elements while maintaining the same functionality.
331-339: Updated OWASP membership link to use Next.js Link.The "Join OWASP" button has been correctly migrated to use Next.js Link, ensuring consistent navigation behavior throughout the application.
frontend/src/components/Card.tsx (3)
1-15: Updated import statements for Next.js and HeroUI.The component now correctly uses Next.js Link and HeroUI Tooltip components, aligning with the project's migration to these libraries. The relative component imports have also been simplified.
50-57: Updated Tooltip API properties.The Tooltip component properties have been updated to match the HeroUI API, replacing
openDelaywithdelayandpositioningwithplacement. This ensures compatibility with the new component library.
143-156: Replaced HStack with standard flex div for social links.The component has been updated to use a standard div with flexbox styling instead of the Chakra UI HStack component, making it framework-agnostic and easier to maintain as you transition away from Chakra UI.
frontend/src/wrappers/ErrorWrapper.tsx (4)
1-5: Consider consolidating imports and verifying client-side usage
All imports for client-side functionalities (e.g.,@heroui/button,@heroui/toast,next/navigation) look correct. Verify that'use client'is necessary for this component since it referenceswindowand DOM APIs later in the file.Would you like to confirm whether the entirety of this wrapper will be used only on the client side by running a quick code search or investigating usage in server components?
27-27: Ensure client-side router usage aligns with Next.js recommended patterns
Usingconst router = useRouter()is valid for client components. Just ensure that any server component tries are avoided.
40-40: Good approach for home redirection
UsingonPress={() => router.push('/')}is clean. This effectively replaces traditionalonClick. No issues found here.
76-80: Toast notification usage
TheaddToastapproach for error messaging is clear. Ensure you handle potential edge cases (e.g., repeated errors, large volumes of errors).frontend/src/components/Header.tsx (5)
1-2: Use of'use client'and additional icon imports
Marking this component as client-only is expected because it references window and manages state. The FontAwesome imports appear correct.
19-19: Global path tracking
Storingconst pathname = usePathname()is a clean solution for highlighting active links or toggling states based on the route.
80-120: Desktop nav links
Mapping overheaderLinksand rendering submenus is good.pathnamecomparisons are correct. Ensure each link is tested for accessibility, especially nested submenus.
147-148: UsingonPressfor the mobile toggle
This is consistent with the@heroui/buttonAPI. No issues found.
8-11:❓ Verification inconclusive
Transitioning to Next.js-based navigation
Switching from older libraries toButtonfrom@heroui/button,Image,Link, andusePathnameis appropriate for Next.js. Confirm that your project’s bundling excludes unused dependencies from removed libraries.
🏁 Script executed:
#!/bin/bash # Searching for old references to ensure removal rg "@chakra-ui/react|react-router-dom"Length of output: 925
Action Required: Verify Production Bundle Excludes Legacy Dependencies
The updated
Header.tsxnow correctly uses Next.js navigation with@heroui/button,next/image,next/link, andusePathname. However, our search indicates that legacy libraries—specificallyreact-router-domand@chakra-ui/react(the latter seen in lock files)—still appear in the repository (e.g., infrontend/__tests__/unit/pages/ChapterDetails.test.tsxand inpackage.json).Please verify that these dependencies are not being bundled into production. If they are required only for testing or development purposes, consider moving them to devDependencies (or otherwise isolating them from the production build) to ensure they’re excluded from the final bundle.
| @@ -52,6 +56,6 @@ export const fetchAlgoliaData = async <T>( | |||
| if (error instanceof AppError) { | |||
| throw error | |||
| } | |||
| throw new AppError(500, 'Search service error') | |||
| //logger.error('Error fetching Algolia data:', error) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider commented out error logging.
The error logging has been commented out, which might make debugging more difficult if errors occur in production.
Consider using a proper logging mechanism instead of commenting out the error logging. For example:
-//logger.error('Error fetching Algolia data:', error)
+console.error('Error fetching Algolia data:', error)Or better yet, integrate with your application's logging system:
-//logger.error('Error fetching Algolia data:', error)
+import { logger } from 'utils/logger'
+logger.error('Error fetching Algolia data:', error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //logger.error('Error fetching Algolia data:', error) | |
| console.error('Error fetching Algolia data:', error) |
frontend/src/app/about/page.tsx
Outdated
| <a | ||
| href={details.url} | ||
| className="text-gray-600 hover:underline dark:text-gray-300" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| {name} | ||
| </a> | ||
| </Link> |
There was a problem hiding this comment.
Fix mismatched JSX tags.
There's a critical issue with tag mismatch - opening tag is <a> but closing tag is </Link>.
Fix this by either:
- <a
- href={details.url}
- className="text-gray-600 hover:underline dark:text-gray-300"
- target="_blank"
- rel="noopener noreferrer"
- >
- {name}
- </Link>
+ <Link
+ href={details.url}
+ className="text-gray-600 hover:underline dark:text-gray-300"
+ target="_blank"
+ rel="noopener noreferrer"
+ >
+ {name}
+ </Link>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a | |
| href={details.url} | |
| className="text-gray-600 hover:underline dark:text-gray-300" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| {name} | |
| </a> | |
| </Link> | |
| <Link | |
| href={details.url} | |
| className="text-gray-600 hover:underline dark:text-gray-300" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| {name} | |
| </Link> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-107: Expected corresponding JSX closing tag for 'a'.
Opening tag
closing tag
(parse)
frontend/tailwind.config.js
Outdated
| darkMode: 'class', | ||
| plugins: [heroui(), import('tailwindcss-animate')], |
There was a problem hiding this comment.
Potential issue with dynamic import in plugins array
Using import('tailwindcss-animate') (which returns a Promise) in the plugins array might not work as expected since Tailwind expects synchronous plugin objects.
Replace with:
- plugins: [heroui(), import('tailwindcss-animate')],
+ plugins: [heroui(), require('tailwindcss-animate')],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| darkMode: 'class', | |
| plugins: [heroui(), import('tailwindcss-animate')], | |
| darkMode: 'class', | |
| plugins: [heroui(), require('tailwindcss-animate')], |
| data: { | ||
| ...mockUserDetailsData, | ||
| user: { ...mockUserDetailsData.user, recentIssues: {} }, | ||
| }, |
There was a problem hiding this comment.
Inconsistent data structure in test
The test is setting recentIssues to an empty object instead of an array which could lead to inconsistent behavior depending on how the component expects this data.
- user: { ...mockUserDetailsData.user, recentIssues: {} },
+ user: { ...mockUserDetailsData.user, recentIssues: [] },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data: { | |
| ...mockUserDetailsData, | |
| user: { ...mockUserDetailsData.user, recentIssues: {} }, | |
| }, | |
| data: { | |
| ...mockUserDetailsData, | |
| user: { ...mockUserDetailsData.user, recentIssues: [] }, | |
| }, |
| await waitFor(() => { | ||
| expect(screen.getByText('Recent Issues')).toBeInTheDocument() | ||
| expect(screen.queryByText('Test Issue')).not.toBeInTheDocument() | ||
| expect(screen.getByText('No recent releases.')).toBeInTheDocument() |
There was a problem hiding this comment.
Incorrect expectation text
The test is checking for "No recent releases" in the context of recent issues, which seems inconsistent. This might be a bug in either the test or the implementation.
- expect(screen.getByText('No recent releases.')).toBeInTheDocument()
+ expect(screen.getByText('No recent issues.')).toBeInTheDocument()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(screen.getByText('No recent releases.')).toBeInTheDocument() | |
| expect(screen.getByText('No recent issues.')).toBeInTheDocument() |
| const noIssuesData = { | ||
| ...mockUserDetailsData, | ||
| recentIssues: [], | ||
| user: { ...mockUserDetailsData.user, recentIssues: {} }, | ||
| } |
There was a problem hiding this comment.
Inconsistent data structure in another test
Similar to the issue above, this test also uses an empty object for recentIssues instead of an empty array.
- user: { ...mockUserDetailsData.user, recentIssues: {} },
+ user: { ...mockUserDetailsData.user, recentIssues: [] },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const noIssuesData = { | |
| ...mockUserDetailsData, | |
| recentIssues: [], | |
| user: { ...mockUserDetailsData.user, recentIssues: {} }, | |
| } | |
| const noIssuesData = { | |
| user: { ...mockUserDetailsData.user, recentIssues: [] }, | |
| } |
| jest.mock('@/components/MarkdownWrapper', () => { | ||
| return ({ content, className }: { content: string; className?: string }) => ( | ||
| <div | ||
| className={`md-wrapper ${className || ''}`} | ||
| dangerouslySetInnerHTML={{ __html: content }} | ||
| /> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Security concern with dangerouslySetInnerHTML
The mock for MarkdownWrapper uses dangerouslySetInnerHTML which could potentially be a security risk, especially when rendering user-generated content.
Consider creating a safer mock that does not directly use dangerouslySetInnerHTML or ensure that proper sanitization is in place:
jest.mock('@/components/MarkdownWrapper', () => {
- return ({ content, className }: { content: string; className?: string }) => (
- <div
- className={`md-wrapper ${className || ''}`}
- dangerouslySetInnerHTML={{ __html: content }}
- />
- )
+ return ({ content, className }: { content: string; className?: string }) => (
+ <div className={`md-wrapper ${className || ''}`}>
+ {/* This is a test mock that simulates rendered markdown */}
+ {content}
+ </div>
+ )
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div | |
| className={`md-wrapper ${className || ''}`} | |
| dangerouslySetInnerHTML={{ __html: content }} | |
| /> | |
| ) | |
| }) | |
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div className={`md-wrapper ${className || ''}`}> | |
| {/* This is a test mock that simulates rendered markdown */} | |
| {content} | |
| </div> | |
| ) | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| jest.mock('@/components/MarkdownWrapper', () => { | ||
| return ({ content, className }: { content: string; className?: string }) => ( | ||
| <div | ||
| className={`md-wrapper ${className || ''}`} | ||
| dangerouslySetInnerHTML={{ __html: content }} | ||
| /> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Security concern with dangerouslySetInnerHTML in MarkdownWrapper mock.
Using dangerouslySetInnerHTML in the mock implementation could expose vulnerabilities if real content is used in tests.
jest.mock('@/components/MarkdownWrapper', () => {
- return ({ content, className }: { content: string; className?: string }) => (
- <div
- className={`md-wrapper ${className || ''}`}
- dangerouslySetInnerHTML={{ __html: content }}
- />
- )
+ return ({ content, className }: { content: string; className?: string }) => (
+ <div className={`md-wrapper ${className || ''}`}>
+ {content}
+ </div>
+ )
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div | |
| className={`md-wrapper ${className || ''}`} | |
| dangerouslySetInnerHTML={{ __html: content }} | |
| /> | |
| ) | |
| }) | |
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div className={`md-wrapper ${className || ''}`}> | |
| {content} | |
| </div> | |
| ) | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| jest.mock('wrappers/FontAwesomeIconWrapper', () => ({ | ||
| __esModule: true, | ||
| default: () => <span data-testid="mock-icon" />, | ||
| })) | ||
|
|
||
| jest.mock('@/components/MarkdownWrapper', () => { | ||
| return ({ content, className }: { content: string; className?: string }) => ( | ||
| <div | ||
| className={`md-wrapper ${className || ''}`} | ||
| dangerouslySetInnerHTML={{ __html: content }} | ||
| /> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern in mock implementation.
The mock for MarkdownWrapper uses dangerouslySetInnerHTML which could potentially introduce XSS vulnerabilities. While this is just a test mock, it's still a good practice to avoid this approach when possible.
Consider using a safer alternative that doesn't use dangerouslySetInnerHTML:
-jest.mock('@/components/MarkdownWrapper', () => {
- return ({ content, className }: { content: string; className?: string }) => (
- <div
- className={`md-wrapper ${className || ''}`}
- dangerouslySetInnerHTML={{ __html: content }}
- />
- )
-})
+jest.mock('@/components/MarkdownWrapper', () => {
+ return ({ content, className }: { content: string; className?: string }) => (
+ <div className={`md-wrapper ${className || ''}`}>
+ {content}
+ </div>
+ )
+})I'll run a shell script to check for all occurrences of dangerouslySetInnerHTML in the codebase to validate whether this pattern is used only in test mocks or if similar implementations exist elsewhere.
🏁 Script executed:
#!/bin/bash
rg "dangerouslySetInnerHTML" -n .Length of output: 1082
Security Update Required: Refactor MarkdownWrapper Test Mock
After reviewing the codebase, we noted that while production code uses dangerouslySetInnerHTML with proper sanitization (e.g., via DOMPurify in frontend/src/components/MarkdownWrapper.tsx), several test files—including Projects.test.tsx—use it without any sanitization. Even though these are test mocks, embedding dangerouslySetInnerHTML can set a bad precedent and may hide potential XSS risks if similar patterns evolve into production code.
To align with best practices and mitigate any security concerns—even in test environments—please update the MarkdownWrapper mock in Projects.test.tsx as suggested:
-jest.mock('@/components/MarkdownWrapper', () => {
- return ({ content, className }: { content: string; className?: string }) => (
- <div
- className={`md-wrapper ${className || ''}`}
- dangerouslySetInnerHTML={{ __html: content }}
- />
- )
-})
+jest.mock('@/components/MarkdownWrapper', () => {
+ return ({ content, className }: { content: string; className?: string }) => (
+ <div className={`md-wrapper ${className || ''}`}>
+ {content}
+ </div>
+ )
+})- Location:
frontend/__tests__/unit/pages/Projects.test.tsx - Change: Replace the
dangerouslySetInnerHTMLimplementation with direct content rendering.
This change will help avoid potential XSS pitfalls in test mocks while maintaining readability and consistency across the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock('wrappers/FontAwesomeIconWrapper', () => ({ | |
| __esModule: true, | |
| default: () => <span data-testid="mock-icon" />, | |
| })) | |
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div | |
| className={`md-wrapper ${className || ''}`} | |
| dangerouslySetInnerHTML={{ __html: content }} | |
| /> | |
| ) | |
| }) | |
| jest.mock('wrappers/FontAwesomeIconWrapper', () => ({ | |
| __esModule: true, | |
| default: () => <span data-testid="mock-icon" />, | |
| })) | |
| jest.mock('@/components/MarkdownWrapper', () => { | |
| return ({ content, className }: { content: string; className?: string }) => ( | |
| <div className={`md-wrapper ${className || ''}`}> | |
| {content} | |
| </div> | |
| ) | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
|
I'm running into some merge conflicts due to recent commits in the main branch. I attempted to resolve them, but it's showing a large number of unexpected file changes in the PR, and I'm not sure if I’ve handled it correctly. Could you please advise on the best next step? It be better for me if you help resolve the conflicts directly on your end? |
github-actions
bot
removed
backend
docker
Got it — @arkid15r would you like me to reimplement the changes using static SVG files instead, now that we've moved to Next.js? |
Yes, the task is still the same -- we just want a bit different implementation. Thank you! |
|
Thanks for the clarification. I’ll go ahead and reimplement it using the new approach with static SVGs |
|
Hi @arkid15r , I've used local svg icons . When you get a chance, could you please review it? Thanks. |
|
Description
This PR updates the icons used on the About page of the frontend application by replacing old icon classes with SVG elements. Additionally, it updates corresponding tests and removes the external stylesheet for icons, ensuring a more streamlined and self-contained design.
Changes Made
Icon Updates
Linked Issue
Resolves #1275