Skip to content

Perf - Fixed the React "Hydration failed because the initial UI..." error on homepage #2568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: testing
Choose a base branch
from
Open

Perf - Fixed the React "Hydration failed because the initial UI..." error on homepage #2568

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 102 additions & 12 deletions src/components/HomePage/Card/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import React from 'react';
import React, { useCallback, useRef } from 'react';

import classNames from 'classnames';
import { useRouter } from 'next/router';

import styles from './Card.module.scss';

import Link from '@/dls/Link/Link';

interface CardProps {
children: React.ReactNode;
link?: string;
Expand All @@ -23,15 +22,106 @@ const Card: React.FC<CardProps> = ({
linkClassName,
onClick,
}) => {
if (link) {
return (
<Link href={link} isNewTab={isNewTab} className={linkClassName} onClick={onClick}>
<div className={classNames(className, styles.card, styles.cardWithLink)}>{children}</div>
</Link>
);
}

return <div className={classNames(className, styles.card)}>{children}</div>;
const router = useRouter();
const cardRef = useRef<HTMLDivElement>(null);

/**
* Determine if an event target is a nested interactive element that should keep control.
*/
const shouldIgnoreEvent = useCallback(
(target: EventTarget | null) => {
if (!link) return false;
if (!(target instanceof HTMLElement)) return false;
if (!cardRef.current) return false;
const interactiveElement = target.closest(
'a, button, input, textarea, select, [role="button"], [role="link"]',
);
return Boolean(interactiveElement && interactiveElement !== cardRef.current);
},
[link],
);
Comment on lines +31 to +42
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The shouldIgnoreEvent function checks if the clicked element is a nested interactive element, but the CardProps interface allows nested Links and Buttons (as seen in ChapterCard usage). This creates an accessibility issue where:

  1. The outer card has role="link" and tabIndex={0}
  2. Nested Links/Buttons inside also have their own roles and tab indices

This violates WCAG guidelines against nested interactive elements. Nested interactive elements cause:

  • Screen readers to announce multiple roles confusingly
  • Tab navigation issues (which element receives focus?)
  • Keyboard activation ambiguity

Consider one of these approaches:

  1. Make the card purely presentational (remove role/tabIndex) and require consumers to use proper Link/Button children only
  2. Document that Cards with link prop should NOT contain nested interactive elements
  3. Show a dev-mode warning when nested interactive elements are detected

Copilot uses AI. Check for mistakes.

/**
* Trigger navigation using Next.js for internal routes or the browser for external URLs.
*/
const navigate = useCallback(() => {
if (!link) return;

if (isNewTab) {
if (typeof window !== 'undefined') {
window.open(link, '_blank', 'noopener,noreferrer');
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The window.open call includes security parameters 'noopener,noreferrer', which is good practice. However, these should be verified to work consistently across browsers.

Consider using the more explicit object syntax for better browser compatibility:

window.open(link, '_blank', 'noopener=yes,noreferrer=yes');

Or even better, use the modern approach:

const newWindow = window.open(link, '_blank');
if (newWindow) {
  newWindow.opener = null;
}

This ensures the security is applied even if the feature string parsing fails in older browsers.

Suggested change
window.open(link, '_blank', 'noopener,noreferrer');
const newWindow = window.open(link, '_blank');
if (newWindow) {
newWindow.opener = null;
}

Copilot uses AI. Check for mistakes.
}
return;
}

const isInternal = link.startsWith('/') || link.startsWith('#');

if (isInternal) {
router.push(link);
return;
}

if (typeof window !== 'undefined') {
window.location.href = link;
}
Comment on lines +47 to +66
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The typeof window !== 'undefined' checks on lines 51 and 64 are unnecessary in this context. These handlers (handleCardClick, handleCardKeyDown) are only called in response to user interactions in the browser, which means window is always defined.

These checks are only needed in code that runs during SSR (Server-Side Rendering), such as:

  • Top-level module code
  • React component render (not in event handlers)
  • useEffect hooks
  • getServerSideProps/getStaticProps

Since these are event handlers that only execute in the browser, you can safely remove the checks:

if (isNewTab) {
  window.open(link, '_blank', 'noopener,noreferrer');
  return;
}

// ... later ...
window.location.href = link;

This simplifies the code without any functional impact.

Copilot uses AI. Check for mistakes.
}, [isNewTab, link, router]);

/**
* Handle mouse clicks on the card while respecting nested controls.
*/
const handleCardClick = useCallback(
(event: React.MouseEvent<HTMLDivElement>) => {
if (!link) {
onClick?.();
return;
}

onClick?.();

if (event.defaultPrevented || shouldIgnoreEvent(event.target)) return;

event.preventDefault();

navigate();
},
[link, navigate, onClick, shouldIgnoreEvent],
);
Comment on lines +72 to +88
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The onClick callback is called twice in the click handler - once at line 79 when there's no link, and again at line 79 when there is a link. This is inconsistent behavior:

  • Without link: onClick is called (line 75)
  • With link: onClick is called (line 79), then navigation happens

However, looking more carefully, when there's a link, onClick is called BEFORE checking if the event should be ignored (line 81). This means onClick fires even when clicking nested interactive elements, which may not be the intended behavior.

Consider restructuring the logic:

const handleCardClick = useCallback(
  (event: React.MouseEvent<HTMLDivElement>) => {
    if (!link) {
      onClick?.();
      return;
    }

    if (event.defaultPrevented || shouldIgnoreEvent(event.target)) return;

    event.preventDefault();
    onClick?.(); // Only call onClick when actually navigating
    navigate();
  },
  [link, navigate, onClick, shouldIgnoreEvent],
);

This ensures onClick only fires when the card navigation actually occurs.

Copilot uses AI. Check for mistakes.

/**
* Provide keyboard accessibility for the card when it acts like a link.
*/
const handleCardKeyDown = useCallback(
(event: React.KeyboardEvent<HTMLDivElement>) => {
if (!link) return;

if (event.key === 'Enter' || event.key === ' ') {
event.preventDefault();
onClick?.();
navigate();
}
},
[link, navigate, onClick],
Comment on lines +99 to +103
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The Space key handler on line 97 doesn't prevent default scroll behavior before checking if it should navigate. When a user presses Space on a scrollable page, the browser scrolls down by default. The event.preventDefault() should be called earlier to prevent this.

Move event.preventDefault() before the nested interactive check:

const handleCardKeyDown = useCallback(
  (event: React.KeyboardEvent<HTMLDivElement>) => {
    if (!link) return;

    if (event.key === 'Enter' || event.key === ' ') {
      event.preventDefault(); // Move this here
      if (shouldIgnoreEvent(event.target)) return;
      onClick?.();
      navigate();
    }
  },
  [link, navigate, onClick, shouldIgnoreEvent],
);

This ensures Space doesn't scroll even when clicking nested interactive elements.

Suggested change
onClick?.();
navigate();
}
},
[link, navigate, onClick],
if (shouldIgnoreEvent(event.target)) return;
onClick?.();
navigate();
}
},
[link, navigate, onClick, shouldIgnoreEvent],

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +103
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The keyboard handler doesn't respect nested interactive elements, unlike the click handler which uses shouldIgnoreEvent. When a user focuses a nested button/link and presses Enter/Space, both the nested element AND the card navigation will trigger.

This creates a double-navigation bug. Add the same check as in handleCardClick:

const handleCardKeyDown = useCallback(
  (event: React.KeyboardEvent<HTMLDivElement>) => {
    if (!link) return;

    if (event.key === 'Enter' || event.key === ' ') {
      if (shouldIgnoreEvent(event.target)) return; // Add this check
      event.preventDefault();
      onClick?.();
      navigate();
    }
  },
  [link, navigate, onClick, shouldIgnoreEvent],
);

Also update the dependency array to include shouldIgnoreEvent.

Suggested change
event.preventDefault();
onClick?.();
navigate();
}
},
[link, navigate, onClick],
if (shouldIgnoreEvent(event.target)) return;
event.preventDefault();
onClick?.();
navigate();
}
},
[link, navigate, onClick, shouldIgnoreEvent],

Copilot uses AI. Check for mistakes.
);

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
ref={cardRef}
role={link ? 'link' : undefined}
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={link ? 0 : undefined}
Comment on lines +107 to +112
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The eslint disable comments on lines 107 and 111 are suppressing legitimate accessibility warnings. These rules exist to prevent exactly the pattern being implemented here:

  • jsx-a11y/no-static-element-interactions: Warns against click handlers on non-interactive elements
  • jsx-a11y/no-noninteractive-tabindex: Warns against tabIndex on non-interactive elements

While the implementation attempts to make the div interactive with role="link", this is an anti-pattern. The proper solution is either:

  1. Use an actual <a> element and handle nested interactivity properly
  2. Use a <button> element styled as a card

The current approach circumvents accessibility linting without actually solving the underlying accessibility issues. Consider refactoring to use semantic HTML instead of suppressing these warnings.

Copilot uses AI. Check for mistakes.
className={classNames(
styles.card,
className,
{ [styles.cardWithLink]: Boolean(link) },
link ? linkClassName : undefined,
)}
onClick={link ? handleCardClick : onClick}
onKeyDown={link ? handleCardKeyDown : undefined}
Comment on lines +106 to +120
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Nov 2, 2025

Choose a reason for hiding this comment

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

P1 Badge Replacing anchor with div removes browser link affordances

The new card renders a div with role="link" and drives navigation via router.push/window.location instead of the previous <Link> wrapper. While this prevents the hydration warning, it removes native link semantics: users can no longer open the card in a new tab with middle‑click or Cmd/Ctrl‑click, there is no context‑menu “open link” option, and Next.js’ Link prefetching is lost. These regressions affect accessibility and performance for every card rendered with a link prop.

Useful? React with 👍 / 👎.

>
{children}
</div>
);
Comment on lines +25 to +124
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The component now implements complex navigation and event handling logic but lacks test coverage. Given that:

  1. The repository uses comprehensive automated testing (vitest)
  2. This change fixes a critical hydration bug
  3. The component handles multiple edge cases (nested interactivity, keyboard events, internal/external URLs, new tab handling)

Add test coverage for:

  • Basic rendering with and without link prop
  • Navigation for internal vs external URLs
  • New tab behavior (window.open with correct params)
  • Keyboard accessibility (Enter and Space keys)
  • Nested interactive element detection and event blocking
  • Click handler respecting event.defaultPrevented

Example test structure:

// src/components/HomePage/Card/index.test.tsx
describe('Card', () => {
  it('navigates on click when link is provided', () => { ... });
  it('ignores clicks on nested interactive elements', () => { ... });
  it('handles keyboard navigation with Enter and Space', () => { ... });
  it('opens external links in new tab when isNewTab is true', () => { ... });
});

Copilot uses AI. Check for mistakes.
};

export default Card;