Skip to content

fixes#5505

Merged
fshowalter merged 2 commits intomainfrom
draft
Mar 14, 2026
Merged

fixes#5505
fshowalter merged 2 commits intomainfrom
draft

Conversation

@fshowalter
Copy link
Owner

No description provided.

@fshowalter fshowalter enabled auto-merge (squash) March 14, 2026 01:30
@claude
Copy link

claude bot commented Mar 14, 2026

Claude finished @fshowalter's task in 1m 18s —— View job


PR Review

  • Gather context and read CLAUDE.md
  • Examine PR diff
  • Review changed files
  • Provide feedback

This PR has two distinct concerns: a comment cleanup and a meaningful functional change in FilterAndSortContainer.

Comment Cleanup (15 of 19 files)

The bulk of the PR removes AIDEV-NOTE: prefixes from inline comments, converting them to plain comments. The comments themselves are preserved — only the prefix is stripped. This is clean and consistent with the previous commit (67b084d).

One cosmetic nit in CoverList.astro:26:

-    /* AIDEV-NOTE: The 250px values below cannot be extracted...
+    /*    The 250px values below cannot be extracted...

The replacement has extra leading spaces (/* The). Not a bug, just slightly untidy.

Functional Change: Remove jsdom Keyboard Bridge (FilterAndSortContainer.tsx)

The more substantive change is the removal of this useEffect:

// Bridge for jsdom: native browsers fire cancel automatically; jsdom does not.
useEffect(() => {
  const handleKeyDown = (e: KeyboardEvent): void => {
    if (e.key === "Escape" && filterDrawerVisible) {
      dialogRef.current?.dispatchEvent(new Event("cancel"));
      dialogRef.current?.close();
    }
  };
  document.addEventListener("keydown", handleKeyDown);
  return (): void => document.removeEventListener("keydown", handleKeyDown);
}, [filterDrawerVisible]);

Assessment: correct. This was a workaround for jsdom's lack of native <dialog> Escape handling, not required in real browsers. Real browsers fire cancel and close the dialog natively. Removing it keeps production code clean of test-environment shims.

The corresponding test update correctly simulates the native browser sequence:

act(() => {
  dialog?.dispatchEvent(new Event("cancel"));
  dialog?.close();
});

This directly mirrors what browsers do, without needing the bridge in production code.

The removal of the scrollIntoView mock from the spec is also correct — it was only needed for behavior that no longer exists.

PagefindSearch.astro Comment Style

HTML `` comments were converted to JSX {/* */} comments. Both are valid in `.astro` files, but the JSX style is slightly more consistent with the surrounding component expressions. The two free-standing block comments (for `` tags) now use wrapping `{ }` blocks:

{
  /* <template> content is inert until cloned by search-box.ts. ... */
}

This is valid Astro syntax and renders no output, though it adds some visual noise. No functional concern.

Summary

The PR is clean and correct. The functional change (removing the jsdom bridge) is the right call — production code shouldn't carry test-environment workarounds. No bugs or regressions identified.

@github-actions
Copy link

github-actions bot commented Mar 14, 2026

@github-actions github-actions bot temporarily deployed to pull request March 14, 2026 01:31 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2026 01:38 Inactive
@fshowalter fshowalter merged commit d9b151b into main Mar 14, 2026
10 checks passed
@fshowalter fshowalter deleted the draft branch March 14, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant