Website Rehaul

[stevenha75]

Summary by CodeRabbit

• New Features

  • Redesigned homepage with carousel, testimonials, sponsors, newsletter, interactive map placeholder, and CTAs.
  • New About experience and member directory; new Apply flows with hero, intro, timeline, FAQ, testimonials, and CTA.
  • New project library for browsing past projects; enhanced project pages and hero sections.

• Improvements

  • Unified design system with color/typography tokens and dark-mode support.
  • New reusable UI primitives (buttons, cards, inputs, accordion) and improved responsiveness & accessibility.

• Chores

  • Removed legacy UI and redundant styles in favor of the new system.

[coderabbitai]

Actionable comments posted: 10

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

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

⚠️ Outside diff range comments (3)

frontend/src/components/our_work/PastProjects.tsx (1)

41-82: ⚠️ Potential issue | 🟡 Minor

Filter callback should return a value on all code paths.

The static analyzer correctly identifies that this filter callback doesn't always return a value. When none of the conditions match and member_match.length > 0 is false but member_match.length == 0 is also somehow false (which can't happen with numbers, but TypeScript doesn't know that), the function returns undefined.

More importantly, the logic can be simplified - returning the truthy project object works the same as returning true for filter purposes, but returning null instead of false is unconventional.

Proposed simplification

-  const filteredData = past_projects.filter((project: any) => {
+  const filteredData = past_projects.filter((project: any): boolean => {
     // check if query is similar to a team member's name
     const team_lowercase: string[] = [];
     (project['members']['data'] as Array<any>).forEach((member) => {
       team_lowercase.push(
         ((member['attributes']['firstName'] + ' ' + member['attributes']['lastName']) as string).toLowerCase(),
       );
     });
     const member_match: string[] = team_lowercase.filter((elem) => {
       if (elem.includes(modifiedInput)) {
         return true;
       }
+      return false;
     });
 
     // used to check if the input contains the season and/or year
     const season_and_year: string = (
       getSeason((project['startDate'] as string).substring(5, 7) as unknown as number) +
       ' ' +
       (project['startDate'] as string).substring(0, 4)
     ).toLowerCase();
 
-    //if query is empty, return all projects
-    if (modifiedInput === '') {
-      return project;
-    } else if (project['title'] && (project['title'] as string).toLowerCase().includes(modifiedInput)) {
-      return project;
-    } else if (project['startDate'] && (project['startDate'] as string).toLowerCase().includes(modifiedInput)) {
-      return project;
-    } else if (project['link'] && (project['link'] as string).toLowerCase().includes(modifiedInput)) {
-      return project;
-    } else if (
-      (getSeason((project['startDate'] as string).substring(5, 7) as unknown as number) ||
-        (project['startDate'] as string).substring(0, 4)) &&
-      season_and_year.includes(modifiedInput)
-    ) {
-      return project;
-    } else if (member_match.length > 0) {
-      return project;
-    } else if (member_match.length == 0) {
-      return null;
-    }
+    if (modifiedInput === '') return true;
+    if (project['title']?.toLowerCase().includes(modifiedInput)) return true;
+    if (project['startDate']?.toLowerCase().includes(modifiedInput)) return true;
+    if (project['link']?.toLowerCase().includes(modifiedInput)) return true;
+    if (season_and_year.includes(modifiedInput)) return true;
+    if (member_match.length > 0) return true;
+    return false;
   });

frontend/src/components/HelperFunctions.tsx (2)

10-14: ⚠️ Potential issue | 🟠 Major

AbortController is never reset after cancellation.

The controllerRef is initialized once on component mount. If cancel() is called, the signal becomes aborted permanently. Any subsequent axios request using this signal will fail immediately because the controller isn't recreated.

🔧 Proposed fix: Reset controller on each request

 const controllerRef = useRef(new AbortController());

 const cancel = () => {
   controllerRef.current.abort();
 };

 useEffect(() => {
+  controllerRef.current = new AbortController();
   (async () => {
     try {
       const response = await axios.request({
         data: payload,
         signal: controllerRef.current.signal,
         method,
         url,
       });

---

26-28: ⚠️ Potential issue | 🟠 Major

Errors are silently swallowed.

The commented-out setError(error.message) means all request failures are ignored, making debugging very difficult. If error handling isn't needed, at minimum log the error during development.

🔧 Proposed fix

       } catch (error) {
-        // setError(error.message);
+        if (error instanceof Error && error.name !== 'CanceledError') {
+          setError(error.message);
+        }
       } finally {

🤖 Fix all issues with AI agents

In `@frontend/package.json`:
- Line 5: The project uses ESM ("type":"module"), so replace the CommonJS
require in frontend/tailwind.config.js: add a top-level ESM import like import
tailwindcssAnimate from "tailwindcss-animate"; and then replace plugins:
[require("tailwindcss-animate")] with plugins: [tailwindcssAnimate]; ensure the
import is placed before the exported config object and remove any leftover
require usage.

In `@frontend/src/components/about/PersonCard.tsx`:
- Around line 11-25: The linkedinUrl passed into PersonCard must be sanitized
before rendering: in the component (PersonCard) validate linkedinUrl using a
safe check (e.g., construct a URL and ensure its protocol is "http:" or "https:"
or explicitly allow only strings starting with "http://" or "https://") and if
it fails validation treat it as absent (don't render the anchor or use a safe
fallback). Locate the place where linkedinUrl is used (the anchor href around
the profile/link icons, lines ~51-60) and add this validation step; keep
resolveImageSrc unchanged but ensure the same validation pattern is applied to
any other external hrefs. Ensure invalid values are not injected into href to
prevent javascript: or other unsafe schemes.

In `@frontend/src/components/buttons/StandardButton.tsx`:
- Around line 19-37: The current implementation nests a <button> inside
anchor/Link which is invalid; update the render to use the Button component's
asChild pattern: remove the intermediate buttonComponent variable and instead
render Button with asChild when externalLink or internal Link is used — e.g.,
render <Button asChild ...><a href=.../></Button> for externalLink and <Button
asChild ...><Link to=.../></Button> for internal routing; keep props ariaLabel,
className (including colorClass), type, and text, and ensure target/rel are
applied on the anchor element when externalLink is true.

In `@frontend/src/components/home/HeroCarousel.tsx`:
- Around line 8-17: The heroSlides array in HeroCarousel.tsx uses source paths
that won't resolve after Vite bundling; update the component to use static
assets correctly by either moving the image files into the public/ folder and
updating the image values to their public URLs (e.g., '/images/...'), or import
them as modules at the top of the file (e.g., import h4iPhoto from
'@/components/assets/h4igroup_photo.jpg' and aboutHeader from
'@/components/assets/aboutus_header.png') and replace the image fields in
heroSlides with those imported identifiers; ensure the import-based approach
references the imported names in heroSlides so Vite includes them in the bundle.
- Around line 51-65: The plugin registers mouseover/mouseout handlers on
slider.container and starts an autoplay timeout via nextTimeout/clearNextTimeout
inside the slider.on('created') callback but never cleans them up on unmount;
update the Keen Slider plugin to return a cleanup/destroy function that removes
the added event listeners (removeEventListener for the handlers added in the
'created' callback), clears the autoplay timeout by calling clearNextTimeout,
and unregisters any slider event handlers (remove or call slider.off for
'created', 'dragStarted', 'animationEnded', 'updated') so all resources set up
in the plugin (mouseover/mouseout, timeouts, slider.on listeners) are properly
removed when the component unmounts.

In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 49-83: The Apply dropdown currently toggles only on hover
(handlers in the <li> using onMouseEnter/onMouseLeave and state setIsApplyOpen)
which prevents keyboard users from opening it; add keyboard and ARIA support by
wiring onFocus and onBlur on the same <li> (or the top Link) to call
setIsApplyOpen(true/false), add aria-haspopup="true" and
aria-expanded={isApplyOpen} to the trigger Link, mark the dropdown container
with role="menu" and each Link inside with role="menuitem", and add a basic
onKeyDown on the trigger to handle Escape to close and Arrow keys if desired so
keyboard users can open/close and navigate the dropdown (update references:
setIsApplyOpen, handleNavClick, the trigger Link and the dropdown div).

In `@frontend/src/components/projects/Projects.tsx`:
- Around line 14-24: The Projects component currently conditionally calls the
useAxios hook via a ternary (res = isFeatured == true ? useAxios(...) :
useAxios(...)), violating React's rules of hooks; fix by computing the request
URL (or params) first based on isFeatured (e.g., derive a variable like
requestUrl or queryString using isFeatured) and then call useAxios
unconditionally at the top level (const res = useAxios(requestUrl, 'GET', {}));
update references to res accordingly so the hook call order is stable across
renders.

In `@frontend/src/components/ui/button.tsx`:
- Around line 42-50: Button currently omits the HTML type attribute causing
default submit behavior; update the React.forwardRef component (Button) to
extract a type prop from props (e.g., const { type = "button", className,
variant, size, asChild = false, ...props } = props) and when asChild is false
ensure the rendered component (Comp === "button") receives that type prop, while
when asChild is true simply pass through the original type value without forcing
a default; keep ref, className, and the rest of props (and use buttonVariants/cn
as before).

In `@frontend/src/pages/NonprofitApply.tsx`:
- Around line 12-31: The timelineSteps array contains hard-coded past date
ranges (titles/subtitles) that are outdated; update the subtitle values in
timelineSteps to reflect the current application cycle or replace them with "TBD
— 2026" (or just "TBD") as appropriate so dates are not misleading; locate the
timelineSteps constant in NonprofitApply.tsx and change the three subtitle
fields ("Jan 15 – Feb 1", "Feb 2 – Feb 15", "Late Feb – Early Mar") to the
chosen current-cycle ranges or "TBD — 2026".
- Around line 75-77: Update the outdated banner text in the NonprofitApply
component: locate the div with className "bg-accent px-6 py-3 text-center
text-sm font-semibold text-primary" that currently reads "Currently taking Fall
2025 Applications. Apply Now" and change the copy to the correct season/year
(e.g., "Currently taking Fall 2026 Applications. Apply Now") or remove the
entire banner if you prefer to hide season-specific dates until they’re
confirmed.

🟡 Minor comments (14)

frontend/.env-1-7 (1)

1-7: ⚠️ Potential issue | 🟡 Minor

Align .env formatting to avoid parser/lint mismatches (quotes/order/EOF newline).
Unquoted values and a stable key order reduce tooling noise and avoid false diffs. Add a final blank line.

🔧 Proposed tidy-up

-VITE_ROOT_URL="https://chapter-website-backend.herokuapp.com"
-VITE_API_URL="http://localhost:8000"
+VITE_API_URL=http://localhost:8000
+VITE_ROOT_URL=https://chapter-website-backend.herokuapp.com
 
 # EmailJS Configuration (for contact form)
-VITE_EMAILJS_SERVICE_ID="service_rux8luc"
-VITE_EMAILJS_TEMPLATE_ID="template_fgd74qw"
-VITE_EMAILJS_PUBLIC_KEY="oqfPTswPuNLGMxG8o"
+VITE_EMAILJS_PUBLIC_KEY=oqfPTswPuNLGMxG8o
+VITE_EMAILJS_SERVICE_ID=service_rux8luc
+VITE_EMAILJS_TEMPLATE_ID=template_fgd74qw
+

frontend/src/components/layout/Footer.tsx-30-35 (1)

30-35: ⚠️ Potential issue | 🟡 Minor

Add TODO comment indicating newsletter form is a placeholder awaiting backend integration.

The Footer newsletter form updates only local state without sending data anywhere. While NewsletterSection.tsx acknowledges this with a // Placeholder - would connect to EmailJS or similar comment, Footer.tsx is missing this clarification. Add a similar TODO comment to signal that this requires actual email service integration (e.g., EmailJS, Mailchimp, or similar).

Additionally, note that the success status never resets to 'idle', causing the success message to persist. Consider implementing a reset mechanism (e.g., auto-dismiss after a few seconds) to improve UX.

frontend/src/components/home/NonprofitMapSection.tsx-18-35 (1)

18-35: ⚠️ Potential issue | 🟡 Minor

Mark the decorative SVG as hidden from assistive tech.
It’s purely visual, so hiding it avoids extra “graphic” announcements.

🧩 Suggested fix

-            <svg
+            <svg
               className="w-full h-full"
               viewBox="0 0 800 500"
               fill="none"
               xmlns="http://www.w3.org/2000/svg"
+              aria-hidden="true"
+              focusable="false"
             >

frontend/src/components/buttons/ScrollToTopButton.tsx-10-17 (1)

10-17: ⚠️ Potential issue | 🟡 Minor

Initialize visibility on mount.

If the user reloads the page while scrolled past the threshold, the button remains hidden until the next scroll event. Add an initial call to toggleVisible() within the effect to check the current scroll position on mount.

✅ Suggested fix

  useEffect(() => {
    const toggleVisible = () => {
      setVisible(document.documentElement.scrollTop > SCROLL_THRESHOLD);
    };

+   toggleVisible();
    window.addEventListener('scroll', toggleVisible);
    return () => window.removeEventListener('scroll', toggleVisible);
  }, []);

frontend/src/components/home/NewsletterSection.tsx-1-27 (1)

1-27: ⚠️ Potential issue | 🟡 Minor

Add timeout cleanup on component unmount to prevent setState warnings.

The 1-second timeout can fire after the component unmounts, causing React to warn about setting state on an unmounted component. Store the timeout ID in a ref and clear it in a cleanup function.

Suggested cleanup

-import { useState } from 'react';
+import { useEffect, useRef, useState } from 'react';

 export default function NewsletterSection() {
   const [email, setEmail] = useState('');
   const [status, setStatus] = useState<'idle' | 'loading' | 'success' | 'error'>('idle');
+  const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+
+  useEffect(() => {
+    return () => {
+      if (timeoutRef.current) clearTimeout(timeoutRef.current);
+    };
+  }, []);

   const handleSubmit = (e: React.FormEvent) => {
     e.preventDefault();
     if (!email) return;

     setStatus('loading');
     // Placeholder - would connect to EmailJS or similar
-    setTimeout(() => {
+    timeoutRef.current = setTimeout(() => {
       setStatus('success');
       setEmail('');
     }, 1000);
   };

frontend/src/pages/StudentApply.tsx-118-121 (1)

118-121: ⚠️ Potential issue | 🟡 Minor

Student intro copy reads nonprofit-focused.

Lines 119–121 reference “nonprofit organizations” and “your organization’s needs,” which sounds like the nonprofit page. Consider student-centric wording here.

frontend/src/pages/StudentApply.tsx-56-75 (1)

56-75: ⚠️ Potential issue | 🟡 Minor

Replace placeholder “Dates” subtitles to avoid ambiguity.

Line 59/65/71 still say “Dates”. Consider real ranges or “TBD” so applicants aren’t confused.

frontend/src/pages/NonprofitApply.tsx-91-102 (1)

91-102: ⚠️ Potential issue | 🟡 Minor

Remove duplicate paragraph in Criteria/Qualifications.

Lines 92–101 repeat the same paragraph twice, which reads like a copy/paste error.

docs/plans/HOMEPAGE_NAVBAR_REDESIGN.md-3-7 (1)

3-7: ⚠️ Potential issue | 🟡 Minor

Status marked COMPLETE but open tasks remain.

Line 3 says “STATUS: COMPLETE” while later sections still show unchecked tasks (e.g., accessibility, cleanup, QA). Consider updating the status or moving remaining items to a “Future Work” section for clarity.

frontend/src/components/ui/card.tsx-32-44 (1)

32-44: ⚠️ Potential issue | 🟡 Minor

Fix CardTitle ref type mismatch.

CardTitle renders an <h3> element but the ref is typed as HTMLParagraphElement. Change the first generic type parameter to HTMLHeadingElement to match the rendered element type.

Suggested fix

-const CardTitle = React.forwardRef<
-  HTMLParagraphElement,
-  React.HTMLAttributes<HTMLHeadingElement>
->(({ className, ...props }, ref) => (
+const CardTitle = React.forwardRef<
+  HTMLHeadingElement,
+  React.HTMLAttributes<HTMLHeadingElement>
+>(({ className, ...props }, ref) => (

frontend/src/pages/ProjectPage.tsx-19-22 (1)

19-22: ⚠️ Potential issue | 🟡 Minor

Add a safeguard for the VITE_ROOT_URL environment variable to prevent broken API calls if the env var is missing.

If VITE_ROOT_URL isn't defined in .env, the URL will resolve to undefined/api/.... Add a fallback to ensure the API URL is valid:

-  const res = useAxios(
-    import.meta.env.VITE_ROOT_URL +
+  const rootUrl = import.meta.env.VITE_ROOT_URL ?? '';
+  const res = useAxios(
+    rootUrl +
       '/api/projects?fields[0]=title&fields[1]=startDate&fields[2]=blurb&fields[3]=repoURL&fields[4]=hostedProjectURL&populate[image][fields][0]=url&populate[members][fields][0]=firstName&populate[members][fields][1]=lastName&populate[members][fields][2]=pronouns&populate[members][populate][componentRolesArr][fields][0]=title&populate[members][populate][componentRolesArr][fields][1]=isDisplayRole&populate[members][populate][componentRolesArr][fields][2]=team&populate[members][populate][avatar][fields][0]=url&filters[path][$eq]=' +
       params.projectpath,
     'GET',
     {},
   );

Note: This pattern is used in other files (Projects.tsx, PastProjects.tsx) without safeguards. Consider applying the same fix consistently across the codebase.

frontend/src/App.tsx-61-61 (1)

61-61: ⚠️ Potential issue | 🟡 Minor

Add leading slash to route path for consistency.

The route path "ourwork/:projectpath" is missing the leading slash, unlike all other route paths. While React Router can handle relative paths, this inconsistency could cause confusion or unexpected behavior in nested routing scenarios.

🔧 Proposed fix

-              <Route path="ourwork/:projectpath" element={<ProjectPage />} />
+              <Route path="/ourwork/:projectpath" element={<ProjectPage />} />

frontend/src/pages/ContactPage.tsx-84-92 (1)

84-92: ⚠️ Potential issue | 🟡 Minor

Add value props to form inputs for controlled components.

The inputs use onChange to update state but lack value props, making them uncontrolled. This can cause state/UI desynchronization, especially if the form needs to be reset or pre-populated.

🔧 Proposed fix for one input (apply pattern to all)

             <input
               type="text"
               id="firstName"
               name="firstName"
+              value={contactInfo.firstName}
               onChange={handleChange}
               autoComplete="given-name"
               required
               aria-required="true"
             />

frontend/src/components/about/MembersSection.tsx-59-65 (1)

59-65: ⚠️ Potential issue | 🟡 Minor

Ensure non-listed roles don’t sort ahead of exec order.

indexOf returns -1 for roles not in EXEC_ORDER, which currently sorts them to the top. Consider pushing unknown roles to the end.

🔧 Suggested fix

-          return EXEC_ORDER.indexOf(aRole) - EXEC_ORDER.indexOf(bRole);
+          const aIndex = EXEC_ORDER.indexOf(aRole);
+          const bIndex = EXEC_ORDER.indexOf(bRole);
+          const safeA = aIndex === -1 ? Number.MAX_SAFE_INTEGER : aIndex;
+          const safeB = bIndex === -1 ? Number.MAX_SAFE_INTEGER : bIndex;
+          return safeA - safeB;

🧹 Nitpick comments (16)

frontend/src/components/our_work/PastProjects.tsx (1)

79-79: Use strict equality (===) instead of loose equality (==).

member_match.length == 0 should be member_match.length === 0 for consistency and to avoid type coercion surprises.

frontend/src/components/ui/input.tsx (1)

5-6: Consider adding component-specific props or a clarifying comment.

The empty interface extension works but adding a comment explaining it's intentionally empty (for future extensibility) would improve clarity:

/** Extends native input attributes; add custom props here as needed. */
export interface InputProps
  extends React.InputHTMLAttributes<HTMLInputElement> {}

This is a minor nitpick—the current implementation is valid.

frontend/src/components/HelperFunctions.tsx (1)

32-32: Empty dependency array prevents refetching on prop changes.

The useEffect runs only on mount. If url, method, or payload change, the hook won't make a new request. If this is intentional, consider adding a comment. Otherwise, add the dependencies.

-  }, []);
+  }, [url, method, payload]);

frontend/src/components/layout/Footer.tsx (1)

69-72: Success message persists indefinitely.

Once status is set to 'success', it never resets to 'idle'. The success message will remain visible until the component unmounts. Consider resetting after a timeout or when the user starts typing again.

💡 Optional: Reset status after timeout

   const handleSubmit = (e: React.FormEvent) => {
     e.preventDefault();
     if (!email) return;
     setStatus('success');
     setEmail('');
+    setTimeout(() => setStatus('idle'), 3000);
   };

frontend/src/components/home/NewsletterSection.tsx (1)

48-53: Reset status when the user edits the email again.
This avoids showing the success message while a new address is being typed.

♻️ Suggested tweak

-  onChange={(e) => setEmail(e.target.value)}
+  onChange={(e) => {
+    setEmail(e.target.value);
+    if (status === 'success') setStatus('idle');
+  }}

frontend/src/components/about/CurrentProjectsSection.tsx (1)

23-23: Consider avoiding !important modifiers in className props.

The !mx-0 !max-w-none overrides suggest the Projects component's internal styles conflict with the desired layout here. This works but creates tight coupling. If feasible, consider exposing a variant prop or adjusting the Projects component's default container styles to avoid needing important overrides.

frontend/src/components/apply/ApplyIntro.tsx (2)

24-26: Hardcoded hover color breaks design system consistency.

The hover:bg-[#004785] bypasses the design token system used elsewhere (e.g., bg-primary). Consider using a design token like hover:bg-primary/90 or defining a --primary-hover CSS variable if a specific darker shade is needed.

---

29-34: Consider adding explicit dimensions to prevent layout shift.

The image lacks width and height attributes, which can cause Cumulative Layout Shift (CLS) during loading. Adding intrinsic dimensions or using aspect-ratio helps the browser reserve space.

♻️ Suggested improvement

           <img
             src={imageSrc}
             alt={imageAlt}
-            className="w-full max-w-[520px] rounded-2xl object-cover shadow-lg"
+            className="w-full max-w-[520px] aspect-[4/3] rounded-2xl object-cover shadow-lg"
             loading="lazy"
           />

frontend/src/components/apply/ApplyCTA.tsx (1)

20-22: Duplicated hardcoded hover color.

Same hardcoded hover:bg-[#004785] appears here and in ApplyIntro.tsx. Consider extracting this to a shared constant, a Tailwind theme extension, or a reusable button variant to maintain consistency and ease future updates.

frontend/src/components/apply/ApplyFAQ.tsx (1)

21-29: Consider key strategy consistency.

Using item.question as the React key (line 22) assumes questions are unique, while the Accordion value uses the index (faq-${index}). If duplicate questions are possible, this could cause issues. Consider using a consistent approach—either both use index, or add an id field to FaqItem.

frontend/src/components/apply/ApplyTestimonials.tsx (1)

17-27: Key may not be unique if same person has multiple testimonials.

Using testimonial.name as the key could cause React reconciliation issues if the same person provides multiple testimonials. Consider adding an id field to the Testimonial type or using a combination like ${name}-${index}.

docs/plans/ABOUT_PAGE_REDESIGN.md (1)

42-50: Minor markdown formatting improvements.

A few minor markdown linting issues could be addressed for consistency:

• Lines 42 and 237: Fenced code blocks should have a language identifier (e.g., ```text or ```plaintext for file structures).
• Lines 143 and 333: Tables should be surrounded by blank lines for proper rendering.

frontend/src/index.css (1)

5-52: Consider merging the two @layer base blocks.

There are two separate @layer base blocks (lines 5-52 and 54-86). While functionally equivalent, merging them improves readability and maintains a single source for base layer styles.

♻️ Suggested structure

 `@layer` base {
   :root {
     /* CSS variables */
   }

   .dark {
     /* Dark mode variables */
   }
+
+  * {
+    `@apply` border-border;
+  }
+
+  body {
+    `@apply` m-0 font-body antialiased bg-background text-foreground;
+  }
+  /* ... rest of element styles */
 }
-
-@layer base {
-  * {
-    `@apply` border-border;
-  }
-  /* ... */
-}

frontend/src/App.tsx (1)

21-21: Consider removing unused import or using a feature flag.

RecruitmentBanner is imported but only referenced in comments. If it's intentionally kept for future toggling, consider using an environment variable or feature flag instead of commented code.

♻️ Alternative approaches

Option 1: Remove until needed

-import RecruitmentBanner from './components/banner/RecruitmentBanner';

Option 2: Use feature flag

{import.meta.env.VITE_SHOW_RECRUITMENT_BANNER === 'true' && <RecruitmentBanner />}

frontend/src/pages/ContactPage.tsx (2)

129-140: Consider making phone number optional.

Phone number is marked as required, but many users prefer not to share it for initial contact. Consider making it optional unless there's a specific business need.

♻️ Suggested changes

           <div>
             <label htmlFor="phoneNumber">Phone Number</label>
             <input
               type="tel"
               id="phoneNumber"
               name="phoneNumber"
               onChange={handleChange}
               autoComplete="tel"
-              required
-              aria-required="true"
             />
           </div>

Also update validation to exclude phone from required check:

const requiredFields = ['firstName', 'lastName', 'subject', 'email', 'message'];
const hasEmptyFields = requiredFields.some((field) => !contactInfo[field].trim());

---

63-66: Rename catch parameter to avoid shadowing state variable.

The catch parameter error shadows the error state variable, which can cause confusion when reading the code.

♻️ Proposed fix

-    } catch (error) {
-      console.error('Failed to send message:', error);
+    } catch (err) {
+      console.error('Failed to send message:', err);
       setError(ERROR_MESSAGES.SEND_FAILED);
       setIsSubmitting(false);
     }

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)

frontend/src/components/projects/Projects.tsx (1)

49-53: ⚠️ Potential issue | 🟡 Minor

Potential null dereference if image attribute is missing.

item['attributes']['image']['data'] will throw if image is null or undefined. Use optional chaining to guard against this.

Proposed fix

                  image={
-                    item['attributes']['image']['data']
-                      ? item['attributes']['image']['data'][0]['attributes']['url']
+                    item['attributes']['image']?.['data']
+                      ? item['attributes']['image']['data'][0]?.['attributes']?.['url']
                      : 'https://plugins.jetbrains.com/files/16260/113019/icon/pluginIcon.png'
                  }

🤖 Fix all issues with AI agents

In `@frontend/src/App.tsx`:
- Line 59: The Route declaration for ProjectPage uses an inconsistent path
string; update the Route element with path="/ourwork/:projectpath" (add the
leading slash) so it matches the other top-level routes and avoids potential
nesting issues—locate the Route element rendering <ProjectPage /> in App.tsx and
change its path attribute accordingly.

In `@frontend/src/components/layout/Footer.tsx`:
- Around line 29-34: handleSubmit currently fakes a success by calling
setStatus('success') and clearing the email without sending data; update
handleSubmit in Footer.tsx (the handleSubmit function that uses setStatus and
setEmail) to either (A) add a clear TODO comment stating it must call the real
subscription API/service (matching NewsletterSection's intended
EmailJS/integration) and set a loading state before calling setStatus, or (B)
actually perform the subscription call: set a loading state, POST the email to
the same subscription endpoint or EmailJS method used by NewsletterSection,
await the response, then call setStatus('success') or setStatus('error') based
on the result and only clear the email on success; also include error handling
and user-facing messages.

In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 106-160: Navbar currently allows body scrolling while the mobile
menu (isMenuOpen) is open; add a useEffect inside the Navbar component that
imports useEffect from React and toggles document.body.style.overflow = 'hidden'
when isMenuOpen is true and restores it (empty string) when false, also ensuring
the cleanup/reset occurs on unmount to avoid leaving overflow locked; reference
the isMenuOpen state and ensure this effect depends on [isMenuOpen].

🧹 Nitpick comments (5)

frontend/src/components/projects/Projects.tsx (1)

35-35: Consider replacing any with a proper type for project items.

item: any bypasses type safety. A Project interface (even a minimal one) would catch typos in attribute access paths (e.g., item['attributes']['image']) at compile time rather than runtime.

frontend/src/components/layout/Footer.tsx (1)

25-71: Duplicate newsletter subscription logic — consider extracting a shared hook or component.

Both Footer and NewsletterSection independently manage email/status state and define their own handleSubmit. When the real subscription service is wired up, you'll need to update two places. Consider extracting a useNewsletterSubscription hook or a shared <NewsletterForm> component to keep the logic in one place.

frontend/tailwind.config.js (1)

70-75: Minor redundancy in font family definitions.

heading/karla and body/rubik are duplicates pointing to the same font stacks. This is fine if intentional (semantic vs. explicit naming), but if only the semantic names are used in practice, consider dropping the karla/rubik aliases to reduce surface area.

frontend/src/components/layout/Navbar.tsx (1)

25-32: Magic number 1000 duplicates the Tailwind nav breakpoint.

handleNavClick hard-codes 1000 to decide whether to close the mobile menu. If the nav breakpoint in tailwind.config.js ever changes, this will silently drift. Consider extracting the breakpoint value to a shared constant.

Example

// e.g., in a shared constants file or at the top of Navbar.tsx
const NAV_BREAKPOINT = 1000;

Then use it in both tailwind.config.js (screens: { nav: ${NAV_BREAKPOINT}px }) and handleNavClick.

frontend/src/App.tsx (1)

30-41: Consider moving Homepage to its own page file.

Homepage is the only page component defined inline in App.tsx — all others (AboutUs, OurWork, etc.) are imported from ./pages/. Moving it to frontend/src/pages/HomePage.tsx would keep the routing file focused on route wiring and match the existing convention.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 33-40: Replace the hardcoded literal in handleNavClick with a
shared constant so the breakpoint cannot drift from Tailwind: create and export
e.g. NAV_BREAKPOINT = 1000 from a small shared module (or read it from tailwind
config if you prefer), import NAV_BREAKPOINT into Navbar.tsx and use if
(window.innerWidth <= NAV_BREAKPOINT) { setIsMenuOpen(false); }, and update any
other places (e.g., other components using the same breakpoint) to import that
same constant so CSS and JS remain in sync.

In `@frontend/src/components/our_work/OurWorkProjectLibrary.tsx`:
- Around line 110-132: The component currently treats a failed API call as an
empty list; update OurWorkProjectLibrary to detect and surface API errors by
checking projectsRes.error (and fall back to projectsRes.data === null) before
rendering the empty-state. Specifically, inside the loading/loaded logic after
obtaining projectsRes (from useAxios) and before mapping with mapProject, if
projectsRes.error is truthy render a distinct error state (different message/UI)
instead of the "No past projects" empty message; ensure you still cast
projectsRes.data to ProjectsApiResponse | null and keep the existing
mapping/filtering (mapProject and ProjectItem) for the success path.

🧹 Nitpick comments (4)

frontend/src/components/layout/Footer.tsx (1)

34-41: Placeholder promise never rejects — error path is dead code.

The await new Promise((resolve) => setTimeout(resolve, 800)) always resolves, so the catch block on Line 39 is unreachable. When the real API call replaces this stub the error handling will kick in, but until then the "Something went wrong" message can never be shown, making it untestable.

Consider having the placeholder randomly reject so both UI states can be exercised during development:

Suggested tweak

-      await new Promise((resolve) => setTimeout(resolve, 800));
+      // TODO: Replace with the real newsletter subscription API (EmailJS or backend endpoint).
+      await new Promise((resolve, reject) =>
+        setTimeout(Math.random() > 0.8 ? reject : resolve, 800)
+      );

frontend/src/styles/our_work/OurWork.module.css (1)

1-5: min-height: 100vh stacks on top of the flex layout's own stretching.

The parent div.flex-1 in App.tsx already stretches to fill the remaining viewport height. Adding min-height: 100vh here means the .page will always be at least the full viewport height regardless of the flex container, which pushes the footer further down than necessary on short-content pages. Consider using min-height: calc(100vh - <navbar+footer height>) or removing it in favor of the parent flex stretch.

That said, this is a cosmetic issue with a safe fallback (extra whitespace at the bottom).

frontend/src/components/our_work/OurWorkProjectLibrary.tsx (1)

103-108: Consider extracting the long API URL into a constant or helper.

The 200+ character query-string URL is hard to read and maintain inline. Extracting it (or at least the query params) into a named constant or builder function at the top of the file would improve readability and make future field changes easier to spot in diffs.

frontend/src/styles/our_work/OurWorkHeader.module.css (1)

7-11: Add display: block to the hero image to eliminate the inline baseline gap.

<img> elements are inline by default, which can introduce a small whitespace gap below the image due to the font baseline. While overflow: hidden on the parent masks this in most cases, adding display: block is the standard fix and avoids subtle layout surprises.

Proposed fix

 .heroImage {
+  display: block;
   width: 100%;
   height: 100%;
   object-fit: cover;
 }

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find Tailwind config files
fd tailwind.config -e js -e ts -e mjs -e cjs --type f

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

cat frontend/tailwind.config.js

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 3903

---

🏁 Script executed:

rg "nav:" frontend/src --type tsx --type ts -A 2

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

rg "nav:" frontend/src -A 2

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 1223

---

Extract the nav breakpoint into a shared constant to prevent desync.

The hardcoded window.innerWidth <= 1000 in handleNavClick currently matches the nav: '1000px' breakpoint in tailwind.config.js, but if the Tailwind config is updated without updating this JavaScript value, they will fall out of sync. This could cause the mobile menu to stay visible even after navigation or not close when it should.

Create a shared constant (e.g., const NAV_BREAKPOINT = 1000) or read the breakpoint from the Tailwind config dynamically so both CSS and JavaScript stay synchronized.

🤖 Prompt for AI Agents

In `@frontend/src/components/layout/Navbar.tsx` around lines 33 - 40, Replace the
hardcoded literal in handleNavClick with a shared constant so the breakpoint
cannot drift from Tailwind: create and export e.g. NAV_BREAKPOINT = 1000 from a
small shared module (or read it from tailwind config if you prefer), import
NAV_BREAKPOINT into Navbar.tsx and use if (window.innerWidth <= NAV_BREAKPOINT)
{ setIsMenuOpen(false); }, and update any other places (e.g., other components
using the same breakpoint) to import that same constant so CSS and JS remain in
sync.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

API errors are silently swallowed — users see "no projects" instead of an error message.

When the API call fails, useAxios sets loaded = true with data = null (and error is effectively unused since setError is commented out in the hook). This component then falls through to the empty-state branch on Line 126, displaying "No past projects are available right now" — which is misleading when the real problem is a network/server error.

At minimum, check projectsRes.error (even if it's currently always '') so the component is ready when the hook is fixed, and surface a distinct error message:

Suggested approach

   if (!projectsRes.loaded) {
     return (
       <section className={styles.sectionShell}>
         <LoadingSpinner text="Loading projects..." />
       </section>
     );
   }
 
+  if (projectsRes.error || !projectsRes.data) {
+    return (
+      <section className={styles.sectionShell}>
+        <p className={styles.emptyState}>
+          Unable to load projects right now. Please try again later.
+        </p>
+      </section>
+    );
+  }
+
   const apiResponse = projectsRes.data as ProjectsApiResponse | null;

🤖 Prompt for AI Agents

In `@frontend/src/components/our_work/OurWorkProjectLibrary.tsx` around lines 110
- 132, The component currently treats a failed API call as an empty list; update
OurWorkProjectLibrary to detect and surface API errors by checking
projectsRes.error (and fall back to projectsRes.data === null) before rendering
the empty-state. Specifically, inside the loading/loaded logic after obtaining
projectsRes (from useAxios) and before mapping with mapProject, if
projectsRes.error is truthy render a distinct error state (different message/UI)
instead of the "No past projects" empty message; ensure you still cast
projectsRes.data to ProjectsApiResponse | null and keep the existing
mapping/filtering (mapProject and ProjectItem) for the success path.

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (4)

frontend/src/components/layout/Navbar.tsx (1)

114-168: Consider adding a focus trap and Escape-key dismissal to the mobile menu.

When the mobile menu is open, keyboard users can Tab into page content behind the overlay. Additionally, there's no onKeyDown handler on the overlay to close on Escape. The APG modal pattern recommends trapping focus within the active menu and restoring it to the trigger on close.

A lightweight option is the focus-trap-react package; alternatively a small custom hook using onKeyDown on the menu container will cover the common cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/layout/Navbar.tsx` around lines 114 - 168, The mobile
menu overlay (controlled by isMenuOpen / setIsMenuOpen and the menu container
div) currently allows keyboard focus to escape and lacks Escape-key dismissal;
wrap the menu content in a focus trap (e.g., focus-trap-react) or implement a
small custom hook that: traps Tab/Shift+Tab within the menu container, listens
for Escape on the menu container or overlay to call setIsMenuOpen(false), and on
close restores focus to the menu trigger element (store the trigger ref before
opening). Add appropriate aria attributes (aria-hidden on background,
aria-modal/role="dialog" on the menu container) and ensure the overlay div is
keyboard-focusable (tabIndex={-1}) so it can receive key events; update the menu
render where navLinks are mapped and where setIsApplyOpen is used to include
these handlers.

frontend/src/components/project_page/projectPageMapper.ts (1)

66-80: resolveAssetUrl duplicates resolveImageSrc in PersonCard.tsx.

Both functions share identical logic for handling absolute URLs, root-relative paths, and VITE_ROOT_URL prefixing. Consider extracting to a shared utility (e.g., src/lib/resolveAssetUrl.ts) and importing it in both files to avoid drift (e.g., the blob: handling present in resolveImageSrc but absent here).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/project_page/projectPageMapper.ts` around lines 66 -
80, The logic in resolveAssetUrl duplicates resolveImageSrc; extract the shared
URL resolution into a single utility (e.g., a new resolveAssetUrl function in a
shared module) and replace both implementations to import and use that utility;
ensure the consolidated function handles all current cases from both versions
(http(s)://, data:, blob:, root-relative paths starting with '/', and prefixing
with import.meta.env.VITE_ROOT_URL for other relative paths) so neither
PersonCard.tsx nor projectPageMapper.ts reintroduce divergent logic.

frontend/src/components/project_page/projectPageContent.ts (1)

1-5: Duplicate import of the same asset.

defaultFeatureImage (line 1) and defaultTeamPhoto (line 5) both resolve to h4igroup_photo.jpg. Consolidate to a single import and alias at the usage sites.

♻️ Proposed refactor

-import defaultFeatureImage from '@/components/assets/h4igroup_photo.jpg';
 import featureOneImage from '@/components/assets/mott_haven_image.jpg';
 import featureTwoImage from '@/components/assets/yknot_image.jpg';
 import defaultSolutionImage from '@/components/assets/projectBG.png';
-import defaultTeamPhoto from '@/components/assets/h4igroup_photo.jpg';
+import h4iGroupPhoto from '@/components/assets/h4igroup_photo.jpg';
+const defaultFeatureImage = h4iGroupPhoto;
+const defaultTeamPhoto = h4iGroupPhoto;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/project_page/projectPageContent.ts` around lines 1 -
5, The file imports the same asset twice as defaultFeatureImage and
defaultTeamPhoto; remove the redundant import and keep a single import (e.g.,
defaultFeatureImage) and update all usages of defaultTeamPhoto to use that
single identifier (or vice versa), ensuring references in projectPageContent
(where defaultFeatureImage and defaultTeamPhoto are used) are updated
accordingly and no other imports are changed.

frontend/src/pages/ProjectPage.tsx (1)

245-366: Text content used as React keys across multiple list renders.

key={paragraph}, key={highlight}, key={testimonial.quote} etc. bind rendering identity to content. Duplicate strings (two identical paragraph bodies, two highlights with the same text) will produce silent key collisions causing missed updates. The DEFAULT_STATIC_CONTENT values are currently unique, but this is fragile when the data is API-driven.

Prefer index-based keys or, ideally, unique IDs when the data shape permits:

♻️ Representative fix (apply the same pattern to all map calls)

-            {viewModel.aboutParagraphs.map((paragraph) => (
-              <p key={paragraph} ...>
+            {viewModel.aboutParagraphs.map((paragraph, i) => (
+              <p key={i} ...>

-                  {feature.highlights.map((highlight) => (
-                    <li key={highlight} ...>
+                  {feature.highlights.map((highlight, i) => (
+                    <li key={i} ...>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/pages/ProjectPage.tsx` around lines 245 - 366, The code uses raw
text values as React keys (e.g., in viewModel.aboutParagraphs.map,
feature.paragraphs.map, feature.highlights.map, viewModel.testimonials.map where
you have key={paragraph}, key={highlight}, key={testimonial.quote}), which can
collide when strings repeat; update each map to use a stable unique key source
instead — prefer a unique id on the data object (e.g., paragraph.id,
highlight.id, testimonial.id) and if the data lacks IDs, fall back to an
index-based key (e.g., map((item, idx) => <... key={idx} />) or a composite key
like `${feature.title}-para-${idx}`) so replace all instances of content-as-key
with either the item.id or a deterministic index-based key.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/src/components/about/PersonCard.tsx`:
- Around line 67-69: The primary decorative LinkedIn placeholder in PersonCard
(the JSX branch using showPrimaryPlaceholder and the Linkedin component) lacks
accessibility markup; update that LinkedIn element to match the secondary
placeholder by adding aria-hidden="true" (or role="presentation") so screen
readers ignore the non-interactive icon, ensuring the showPrimaryPlaceholder
branch uses the same accessibility attributes as the secondary placeholder.

In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 21-22: The shared isApplyOpen state leaks between desktop and
mobile; replace it with two separate states (e.g., isDesktopApplyOpen /
setIsDesktopApplyOpen and isMobileApplyOpen / setIsMobileApplyOpen) while
keeping isMenuOpen as-is; update all desktop dropdown usages (the desktop <li>
event handlers and dropdown visibility checks that currently read/write
isApplyOpen) to use isDesktopApplyOpen / setIsDesktopApplyOpen, and update the
mobile accordion button toggle and sub-list render (where isApplyOpen is read or
toggled) to use isMobileApplyOpen / setIsMobileApplyOpen so the two UI modes no
longer share state.
- Around line 125-136: The mobile Apply accordion button lacks accessible state
and control linkage—update the button in Navbar.tsx (the element using
setIsApplyOpen and isApplyOpen and rendering ChevronDown) to include
aria-expanded={isApplyOpen} and aria-controls with a stable id (e.g.
aria-controls={`apply-submenu`} or using the link id like `${link.id}-submenu`),
and ensure the accordion panel element (the submenu rendered when isApplyOpen is
true) has the matching id (e.g. id="apply-submenu") so screen readers can
associate the button with the controlled region; keep the existing toggle logic
with setIsApplyOpen unchanged.

In `@frontend/src/components/project_page/projectPageMapper.ts`:
- Around line 109-116: normalizeRichText currently strips HTML tags but leaves
HTML entities (e.g., &amp;, &lt;, &nbsp;) intact; update normalizeRichText to
decode HTML entities after tag removal (and normalize non-breaking spaces to
regular spaces) before collapsing whitespace so the UI shows proper characters.
Locate the normalizeRichText function and, after computing withoutTags, run the
string through a browser-safe HTML entity decoder (e.g., using a DOM-based
decoder like creating a temporary element or DOMParser to set innerHTML and read
textContent) then replace \u00A0 with a normal space and finally collapse
whitespace and trim as currently done.

In `@frontend/src/pages/ProjectPage.tsx`:
- Around line 105-115: The current fall-through appends any unrecognized role to
engineering; change the logic in ProjectPage.tsx so that after checking
PRODUCT_DESIGN_ROLES and ENGINEERING_ROLES you explicitly handle unknown roles:
create an otherRoles (or unknownRoles) bucket and push teamMember there (or at
minimum call console.warn/processLogger with the role and teamMember id in
development), using the existing role variable and teamMember object and
preserving productDesign and engineering behavior; update any rendering to show
the new bucket or ensure logs surface unrecognized roles for future CMS changes.
- Around line 209-236: The rendered external links (ApplyLink used in
ProjectPage for viewModel.repoURL and viewModel.hostedProjectURL, and
PersonCard.linkedinUrl) are passed straight to anchor hrefs allowing
javascript:, data:, etc.; add URL validation to prevent XSS by creating a helper
(e.g., isSafeUrl or sanitizeHref) that parses the string with the URL
constructor (or a robust parsing library) and only allows safe protocols (http,
https, mailto if desired), returning null/undefined or a safe fallback for
anything else, then use that helper inside ApplyLink (and update calls from
ProjectPage and PersonCard to pass the sanitized href or avoid rendering the
anchor when unsafe) so no untrusted protocol is written into the DOM.

---

Duplicate comments:
In `@frontend/src/components/about/PersonCard.tsx`:
- Around line 57-66: The anchor using linkedinUrl in the PersonCard (see
hasPrimaryLink, linkedinUrl, and the <a> rendering) is vulnerable to javascript:
XSS; validate and sanitize linkedinUrl before rendering by parsing it (e.g., new
URL or equivalent) and only allow safe schemes (http or https) or normalize by
prepending https:// for scheme-less values, and reject or replace any value with
an unsafe scheme (javascript:, data:, etc.) so the href passed to the <a> is
guaranteed to be a safe HTTP(S) URL.

In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 56-90: The dropdown is not keyboard accessible; update the li/Link
and dropdown markup that uses setIsApplyOpen, isApplyOpen, handleNavClick, Link
and the dropdown div to add keyboard and ARIA support: add onFocus/onBlur (or
onFocusWithin/onBlurWithin) to toggle setIsApplyOpen, add onKeyDown on the
trigger to open/close on Enter/Space and close on Escape, add
aria-haspopup="true" and aria-expanded={isApplyOpen} to the trigger element, set
role="menu" on the dropdown container and role="menuitem" on each dropdown Link,
ensure each menu item is focusable (tabIndex= -1 or use natural links) and
manage focus so opening moves focus into the first menu item and closing returns
focus to the trigger.
- Around line 33-40: The click handler uses a hardcoded breakpoint
(window.innerWidth <= 1000) that is out of sync with Tailwind; instead export a
single NAV_BREAKPOINT constant from a shared config (or re-export the Tailwind
nav value) and use that in handleNavClick to decide when to call
setIsMenuOpen(false) (and/or use window.matchMedia with that value); update the
reference in the Navbar component (handleNavClick) to compare against the shared
NAV_BREAKPOINT (or feed it into matchMedia) so the breakpoint is maintained in
one place and stays consistent with Tailwind's nav: '1000px' value.

---

Nitpick comments:
In `@frontend/src/components/layout/Navbar.tsx`:
- Around line 114-168: The mobile menu overlay (controlled by isMenuOpen /
setIsMenuOpen and the menu container div) currently allows keyboard focus to
escape and lacks Escape-key dismissal; wrap the menu content in a focus trap
(e.g., focus-trap-react) or implement a small custom hook that: traps
Tab/Shift+Tab within the menu container, listens for Escape on the menu
container or overlay to call setIsMenuOpen(false), and on close restores focus
to the menu trigger element (store the trigger ref before opening). Add
appropriate aria attributes (aria-hidden on background, aria-modal/role="dialog"
on the menu container) and ensure the overlay div is keyboard-focusable
(tabIndex={-1}) so it can receive key events; update the menu render where
navLinks are mapped and where setIsApplyOpen is used to include these handlers.

In `@frontend/src/components/project_page/projectPageContent.ts`:
- Around line 1-5: The file imports the same asset twice as defaultFeatureImage
and defaultTeamPhoto; remove the redundant import and keep a single import
(e.g., defaultFeatureImage) and update all usages of defaultTeamPhoto to use
that single identifier (or vice versa), ensuring references in
projectPageContent (where defaultFeatureImage and defaultTeamPhoto are used) are
updated accordingly and no other imports are changed.

In `@frontend/src/components/project_page/projectPageMapper.ts`:
- Around line 66-80: The logic in resolveAssetUrl duplicates resolveImageSrc;
extract the shared URL resolution into a single utility (e.g., a new
resolveAssetUrl function in a shared module) and replace both implementations to
import and use that utility; ensure the consolidated function handles all
current cases from both versions (http(s)://, data:, blob:, root-relative paths
starting with '/', and prefixing with import.meta.env.VITE_ROOT_URL for other
relative paths) so neither PersonCard.tsx nor projectPageMapper.ts reintroduce
divergent logic.

In `@frontend/src/pages/ProjectPage.tsx`:
- Around line 245-366: The code uses raw text values as React keys (e.g., in
viewModel.aboutParagraphs.map, feature.paragraphs.map, feature.highlights.map,
viewModel.testimonials.map where you have key={paragraph}, key={highlight},
key={testimonial.quote}), which can collide when strings repeat; update each map
to use a stable unique key source instead — prefer a unique id on the data
object (e.g., paragraph.id, highlight.id, testimonial.id) and if the data lacks
IDs, fall back to an index-based key (e.g., map((item, idx) => <... key={idx}
/>) or a composite key like `${feature.title}-para-${idx}`) so replace all
instances of content-as-key with either the item.id or a deterministic
index-based key.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Primary placeholder icon missing aria-hidden.

The decorative placeholder rendered when there is no LinkedIn link has no aria-hidden attribute, unlike the secondary placeholder at lines 71–74. Screen readers will announce this non-interactive icon without context.

♿ Proposed fix

-          <Linkedin className="w-5 h-5 text-muted-foreground/40" />
+          <Linkedin className="w-5 h-5 text-muted-foreground/40" aria-hidden="true" />

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/about/PersonCard.tsx` around lines 67 - 69, The
primary decorative LinkedIn placeholder in PersonCard (the JSX branch using
showPrimaryPlaceholder and the Linkedin component) lacks accessibility markup;
update that LinkedIn element to match the secondary placeholder by adding
aria-hidden="true" (or role="presentation") so screen readers ignore the
non-interactive icon, ensuring the showPrimaryPlaceholder branch uses the same
accessibility attributes as the secondary placeholder.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

isApplyOpen is shared between the desktop dropdown and the mobile accordion.

Although CSS hides them mutually, the state leaks across breakpoints on resize — e.g., hovering the desktop dropdown then switching to mobile viewport leaves the accordion pre-expanded, and vice versa. Split into two distinct state variables.

🔧 Proposed fix

-  const [isApplyOpen, setIsApplyOpen] = useState(false);
+  const [isDesktopApplyOpen, setIsDesktopApplyOpen] = useState(false);
+  const [isMobileApplyOpen, setIsMobileApplyOpen] = useState(false);

Then replace each usage:

• Desktop <li> handlers → setIsDesktopApplyOpen
• Desktop dropdown visibility → isDesktopApplyOpen
• Mobile <button> toggle / sub-list render → setIsMobileApplyOpen / isMobileApplyOpen

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/layout/Navbar.tsx` around lines 21 - 22, The shared
isApplyOpen state leaks between desktop and mobile; replace it with two separate
states (e.g., isDesktopApplyOpen / setIsDesktopApplyOpen and isMobileApplyOpen /
setIsMobileApplyOpen) while keeping isMenuOpen as-is; update all desktop
dropdown usages (the desktop <li> event handlers and dropdown visibility checks
that currently read/write isApplyOpen) to use isDesktopApplyOpen /
setIsDesktopApplyOpen, and update the mobile accordion button toggle and
sub-list render (where isApplyOpen is read or toggled) to use isMobileApplyOpen
/ setIsMobileApplyOpen so the two UI modes no longer share state.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Mobile Apply accordion button is missing aria-expanded and aria-controls.

Screen reader users have no way to know whether the accordion sub-menu is open or closed. This violates WCAG 2.1 SC 4.1.2 (Name, Role, Value).

♿ Proposed fix

                    <button
                      onClick={() => setIsApplyOpen(!isApplyOpen)}
                      className="flex w-full items-center justify-between px-6 py-4 text-lg font-heading font-bold text-foreground"
+                     aria-expanded={isApplyOpen}
+                     aria-controls="mobile-apply-menu"
                    >
                      {link.label}

                    {isApplyOpen && (
-                     <ul className="bg-muted">
+                     <ul id="mobile-apply-menu" className="bg-muted">

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/layout/Navbar.tsx` around lines 125 - 136, The mobile
Apply accordion button lacks accessible state and control linkage—update the
button in Navbar.tsx (the element using setIsApplyOpen and isApplyOpen and
rendering ChevronDown) to include aria-expanded={isApplyOpen} and aria-controls
with a stable id (e.g. aria-controls={`apply-submenu`} or using the link id like
`${link.id}-submenu`), and ensure the accordion panel element (the submenu
rendered when isApplyOpen is true) has the matching id (e.g. id="apply-submenu")
so screen readers can associate the button with the controlled region; keep the
existing toggle logic with setIsApplyOpen unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

normalizeRichText does not decode HTML entities.

After stripping tags, sequences like &, <, and   remain as literal characters in the string. Since React renders text nodes verbatim (no HTML entity decoding), these will appear as-is in the UI if the CMS stores HTML-encoded content.

🛠️ Proposed fix

 function normalizeRichText(input?: string): string {
   if (!input) {
     return '';
   }
 
   const withoutTags = input.replace(/<[^>]+>/g, ' ');
-  return withoutTags.replace(/\s+/g, ' ').trim();
+  const collapsed = withoutTags.replace(/\s+/g, ' ').trim();
+  // Decode common HTML entities
+  return collapsed
+    .replace(/&amp;/g, '&')
+    .replace(/&lt;/g, '<')
+    .replace(/&gt;/g, '>')
+    .replace(/&quot;/g, '"')
+    .replace(/&#39;/g, "'")
+    .replace(/&nbsp;/g, ' ');
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/project_page/projectPageMapper.ts` around lines 109 -
116, normalizeRichText currently strips HTML tags but leaves HTML entities
(e.g., &, <,  ) intact; update normalizeRichText to decode HTML
entities after tag removal (and normalize non-breaking spaces to regular spaces)
before collapsing whitespace so the UI shows proper characters. Locate the
normalizeRichText function and, after computing withoutTags, run the string
through a browser-safe HTML entity decoder (e.g., using a DOM-based decoder like
creating a temporary element or DOMParser to set innerHTML and read textContent)
then replace \u00A0 with a normal space and finally collapse whitespace and trim
as currently done.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Catch-all silently classifies any unrecognized role as Engineering.

Members whose role string is not in PRODUCT_DESIGN_ROLES or ENGINEERING_ROLES fall through to line 115 and are appended to engineering. If the CMS ever introduces roles like "Advisor" or "Executive Director", they will appear under the Engineering section without warning.

Consider an explicit catch-all bucket or logging an unrecognized role in development:

🛠️ Proposed fix

-    engineering.push(teamMember);
+    // Unrecognized roles are intentionally surfaced under Engineering as a fallback.
+    // Add any new roles to PRODUCT_DESIGN_ROLES or ENGINEERING_ROLES as needed.
+    if (import.meta.env.DEV) {
+      console.warn(`[groupTeamMembers] Unrecognized role "${role}" for member "${teamMember.name}" — falling back to Engineering.`);
+    }
+    engineering.push(teamMember);

📝 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.

Suggested change

	if (PRODUCT_DESIGN_ROLES.includes(role as (typeof PRODUCT_DESIGN_ROLES)[number])) {
	productDesign.push(teamMember);
	return;
	}
	if (ENGINEERING_ROLES.includes(role as (typeof ENGINEERING_ROLES)[number])) {
	engineering.push(teamMember);
	return;
	}
	engineering.push(teamMember);
	if (PRODUCT_DESIGN_ROLES.includes(role as (typeof PRODUCT_DESIGN_ROLES)[number])) {
	productDesign.push(teamMember);
	return;
	}
	if (ENGINEERING_ROLES.includes(role as (typeof ENGINEERING_ROLES)[number])) {
	engineering.push(teamMember);
	return;
	}
	// Unrecognized roles are intentionally surfaced under Engineering as a fallback.
	// Add any new roles to PRODUCT_DESIGN_ROLES or ENGINEERING_ROLES as needed.
	if (import.meta.env.DEV) {
	console.warn(`[groupTeamMembers] Unrecognized role "${role}" for member "${teamMember.name}" — falling back to Engineering.`);
	}
	engineering.push(teamMember);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/pages/ProjectPage.tsx` around lines 105 - 115, The current
fall-through appends any unrecognized role to engineering; change the logic in
ProjectPage.tsx so that after checking PRODUCT_DESIGN_ROLES and
ENGINEERING_ROLES you explicitly handle unknown roles: create an otherRoles (or
unknownRoles) bucket and push teamMember there (or at minimum call
console.warn/processLogger with the role and teamMember id in development),
using the existing role variable and teamMember object and preserving
productDesign and engineering behavior; update any rendering to show the new
bucket or ensure logs surface unrecognized roles for future CMS changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find ApplyLink component
fd -t f -i "applylink" . --exec cat {}

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 672

---

🏁 Script executed:

# Alternative search for ApplyLink
rg -t tsx -t ts "ApplyLink" --files | head -20

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

# Search for ApplyLink definition/export
rg "export.*ApplyLink|const ApplyLink|function ApplyLink" -t tsx -t ts -A 10

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

# Find PersonCard to verify the linkedinUrl issue mentioned in the review
rg -l "PersonCard" --type js --type jsx --type ts --type tsx

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

# Search for PersonCard and linkedinUrl handling
rg "linkedinUrl" -A 5 -B 5 --type js --type jsx --type ts --type tsx

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 107

---

🏁 Script executed:

# Search for PersonCard component
rg "PersonCard" -l | head -10

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 281

---

🏁 Script executed:

# Search for linkedinUrl without file type restrictions
rg "linkedinUrl" -A 5 -B 5

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 2906

---

🏁 Script executed:

# Check how repoURL and hostedProjectURL are fetched from API
rg "repoURL|hostedProjectURL" -B 5 -A 5 frontend/src/pages/ProjectPage.tsx

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 2326

---

🏁 Script executed:

# Check ProjectPage.tsx to see the full context of data flow
cat frontend/src/pages/ProjectPage.tsx | head -100

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 2758

---

🏁 Script executed:

# Search for any URL sanitization utilities in the codebase
rg "sanitize|validate.*url|URL.*validate" -i

Repository: Hack4Impact-UMD/umd-website-redesign

Length of output: 1023

---

ApplyLink renders API-sourced URLs without sanitization, enabling XSS via CMS.

ApplyLink passes href directly to an <a> tag for external URLs (those starting with http) without any protocol validation. If repoURL or hostedProjectURL contains a malicious URL like http://javascript:alert(1), it would render unsanitized in the DOM. The same vulnerability exists in PersonCard with linkedinUrl. Add URL validation to reject dangerous protocols (javascript:, data:, etc.) before rendering, or use a URL parsing utility to safely construct href values.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/src/pages/ProjectPage.tsx` around lines 209 - 236, The rendered
external links (ApplyLink used in ProjectPage for viewModel.repoURL and
viewModel.hostedProjectURL, and PersonCard.linkedinUrl) are passed straight to
anchor hrefs allowing javascript:, data:, etc.; add URL validation to prevent
XSS by creating a helper (e.g., isSafeUrl or sanitizeHref) that parses the
string with the URL constructor (or a robust parsing library) and only allows
safe protocols (http, https, mailto if desired), returning null/undefined or a
safe fallback for anything else, then use that helper inside ApplyLink (and
update calls from ProjectPage and PersonCard to pass the sanitized href or avoid
rendering the anchor when unsafe) so no untrusted protocol is written into the
DOM.