Skip to content

Conversation

@minimaluminium
Copy link
Member

ref https://linear.app/ghost/issue/BER-3119/add-keyboard-navigation-for-search-results-list

Users can now navigate search results using arrow keys and Enter:

  • ArrowUp/ArrowDown to move between results with visual highlight
  • Enter to navigate to selected profile
  • Selection wraps around from last to first result and vice versa

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 9, 2025

Warning

Rate limit exceeded

@minimaluminium has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 5 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 706409a and 575792b.

📒 Files selected for processing (3)
  • apps/activitypub/package.json (1 hunks)
  • apps/activitypub/src/components/activities/ActivityItem.tsx (1 hunks)
  • apps/activitypub/src/components/modals/Search.tsx (6 hunks)

Walkthrough

ActivityItem.tsx now accepts an optional prop isSelected?: boolean (default false) and applies a conditional classname to indicate selection. Search.tsx was refactored to merge topic and account results into a unified SearchResult union, remove the prior per-item components, introduce STICKY_HEADER_HEIGHT, and implement keyboard navigation (ArrowUp/ArrowDown to move, Enter to activate), selection state with per-item refs and scroll-into-view logic, focus-on-mount for the input, combined loading/empty-state handling, and unified inline rendering for topic and account rows (including current-user checks and follow/update flows). apps/activitypub/package.json version bumped 3.0.3 → 3.0.4.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review ActivityItem.tsx: prop typing/default and conditional classname logic.
  • Review Search.tsx: keyboard handlers, selection lifecycle and boundary behavior, per-item refs and scroll-into-view math (relative to STICKY_HEADER_HEIGHT), focus-on-mount, unified SearchResult usage, and Enter navigation.
  • Additional attention:
    • Correct handling when results update (reset/retain selection).
    • isCurrentUser rendering paths and FollowButton/updateResult flows.
    • Removal of prior per-item components and any residual references.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding keyboard navigation to the ActivityPub search component, which is the primary focus of the changeset.
Description check ✅ Passed The description is directly related to the changeset, detailing the keyboard navigation features (ArrowUp/Down, Enter, wrapping selection) that were implemented in the Search.tsx file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
apps/activitypub/src/components/modals/Search.tsx (5)

29-60: AccountSearchResultItem: selection + ref wiring looks solid; consider ARIA state

The added isSelected and onRefSet props and the wrapper <div ref={onRefSet}> cleanly separate visual selection from data and make it easy to track DOM nodes per result. Passing isSelected through to ActivityItem is also a good reuse of the shared component.

If ActivityItem does not already expose aria-selected (or similar) when isSelected is true, consider adding that at some point so assistive tech can perceive the selected state as well, not just the visual highlight.


95-121: SearchResults: itemRefs mapping is correct but could benefit from light defensive cleanup

The selectedIndex/itemRefs additions here are consistent and the callback ref pattern:

onRefSet={(ref) => {
    itemRefs.current[index] = ref;
}}

is appropriate for tracking multiple items.

Given that the list can change length, this will naturally null-out entries on unmount (React calls the ref with null), so you won’t hit stale nodes for indices you actually use. If you ever start reordering or doing virtualized rendering, it might be worth explicitly clearing itemRefs.current.length = results.length before mapping, but it’s not required with the current straightforward mapping.


133-153: Search: selection state management on results changes is sensible

Introducing itemRefs and selectedIndex plus resetting selectedIndex to 0 whenever new results arrive or when no results are found keeps the state predictable and avoids out-of-bounds indexes.

One small consideration: when shouldSearch flips from true to false (query length drops below 2), you keep displayResults intact and rely on showSearchResults to hide them. That matches the previous behavior, so it’s fine, but if product ever wants the list cleared when the query becomes “too short”, you can just add a branch in this useEffect to setDisplayResults([]) for !shouldSearch. Based on learnings, this keeps the debounced server search behavior intact while keeping local state predictable.


167-195: Global keydown handler: behavior is correct; consider scoping if conflicts arise

The window.addEventListener('keydown', ...) approach correctly:

  • Short-circuits when showSearchResults is false or there are no results.
  • Uses modulo arithmetic to wrap selectedIndex in both directions.
  • Triggers navigation on Enter only when a valid result is selected, closing the modal afterwards.

This should work well for the modal use case.

If you ever see conflicts with other shortcuts or key handling in the admin UI, a future refinement could be to:

  • Attach the handler to a container element (or the input) via onKeyDown, or
  • Ignore events where e.defaultPrevented is already true or where e.target is outside the search modal,

to reduce global coupling. Not necessary now, but worth keeping in mind.


196-239: Scroll-into-view logic works; consider a more explicit scroll container hook

The scroll-into-view effect correctly:

  • Uses itemRefs.current[selectedIndex] and bails out if there’s no element.
  • Guards against missing/non-scrollable containers.
  • Handles “go to first result” by scrolling to the top.
  • Adjusts for the sticky header with configurable spacing when scrolling up or down to reveal the selected row.

The one slightly brittle part is:

const scrollContainer = selectedElement.closest('[class*="overflow"]') ||
    selectedElement.parentElement?.parentElement?.parentElement;

This couples the behavior to class naming and DOM depth. If this component’s structure or surrounding layout changes, the selector may start matching the wrong container or none at all.

A more robust option down the line would be to:

  • Pass a dedicated scroll container ref into Search from the modal shell, or
  • Mark the intended container with a data attribute (e.g. data-ap-search-scroll) and select against that instead of a fuzzy class match.

Functionally this is fine as-is; the suggestion is mainly for future-proofing.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9d06ea and 87b5646.

📒 Files selected for processing (2)
  • apps/activitypub/src/components/activities/ActivityItem.tsx (1 hunks)
  • apps/activitypub/src/components/modals/Search.tsx (6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.
📚 Learning: 2025-07-14T12:20:35.268Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24346
File: apps/admin-x-settings/src/components/settings/growth/Network.tsx:8-12
Timestamp: 2025-07-14T12:20:35.268Z
Learning: The Network component toggle in `apps/admin-x-settings/src/components/settings/growth/Network.tsx` is intentionally implemented as static UI with a no-op onChange handler, as part of a UI-first development approach before connecting actual ActivityPub functionality.

Applied to files:

  • apps/activitypub/src/components/activities/ActivityItem.tsx
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
📚 Learning: 2025-12-09T08:28:22.100Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
🧬 Code graph analysis (1)
apps/activitypub/src/components/modals/Search.tsx (3)
apps/activitypub/src/api/activitypub.ts (2)
  • SearchResults (43-45)
  • AccountSearchResult (33-36)
apps/activitypub/src/hooks/use-navigate-with-base-path.ts (1)
  • useNavigateWithBasePath (15-37)
apps/activitypub/src/hooks/use-activity-pub-queries.ts (1)
  • useSearchForUser (1362-1399)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: ActivityPub tests
  • GitHub Check: Unit tests (Node 22.18.0)
  • GitHub Check: Build & Push Docker Image
🔇 Additional comments (3)
apps/activitypub/src/components/activities/ActivityItem.tsx (2)

8-8: LGTM!

The isSelected prop addition is clean and appropriately typed for keyboard navigation support.


11-11: LGTM!

Good practice providing a sensible default value for the optional isSelected prop.

apps/activitypub/src/components/modals/Search.tsx (1)

268-275: SearchResults wiring: props propagation matches new selection API

Passing itemRefs and selectedIndex through to SearchResults ties the new selection/scroll behavior together cleanly, with onOpenChange/onUpdate remaining unchanged. This keeps the public surface area of Search simple while enabling richer interaction inside the results list.


const Item = (
<div className='relative flex w-full max-w-[620px] cursor-pointer flex-col before:absolute before:inset-x-[-16px] before:inset-y-[-1px] before:rounded-md before:bg-gray-50 before:opacity-0 before:transition-opacity hover:z-10 hover:cursor-pointer hover:border-b-transparent hover:before:opacity-100 dark:before:bg-gray-950' data-testid={testId} onClick={() => {
<div className={`relative flex w-full max-w-[620px] cursor-pointer flex-col before:absolute before:inset-x-[-16px] before:inset-y-[-1px] before:rounded-md before:bg-gray-50 before:transition-opacity hover:z-10 hover:cursor-pointer hover:border-b-transparent hover:before:opacity-100 dark:before:bg-gray-950 ${isSelected ? 'z-10 before:opacity-100' : 'before:opacity-0'}`} data-testid={testId} onClick={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find and inspect ActivityItem.tsx
cd apps/activitypub/src/components/activities
head -100 ActivityItem.tsx

Repository: TryGhost/Ghost

Length of output: 1560


🏁 Script executed:

#!/bin/bash
# Check the full context around line 15 and see the component structure
cd apps/activitypub/src/components/activities
cat -n ActivityItem.tsx | head -50

Repository: TryGhost/Ghost

Length of output: 1868


🏁 Script executed:

#!/bin/bash
# Find the parent Search component
find apps/activitypub/src/components/modals -name "Search.tsx" -o -name "Search.ts"

Repository: TryGhost/Ghost

Length of output: 107


🏁 Script executed:

#!/bin/bash
# Inspect the parent Search.tsx component
cat -n apps/activitypub/src/components/modals/Search.tsx | head -150

Repository: TryGhost/Ghost

Length of output: 7015


🏁 Script executed:

#!/bin/bash
# Search for where ActivityItem is used in Search.tsx
rg -n "ActivityItem\|role=\|aria-" apps/activitypub/src/components/modals/Search.tsx

Repository: TryGhost/Ghost

Length of output: 40


🏁 Script executed:

#!/bin/bash
# Search for keyboard event handling in Search.tsx
rg -n "onKeyDown\|onKeyUp\|useEffect\|ArrowUp\|ArrowDown\|Enter" apps/activitypub/src/components/modals/Search.tsx

Repository: TryGhost/Ghost

Length of output: 40


🏁 Script executed:

#!/bin/bash
# Get the rest of Search.tsx to see keyboard handling
tail -100 apps/activitypub/src/components/modals/Search.tsx

Repository: TryGhost/Ghost

Length of output: 4477


Add ARIA attributes to support keyboard navigation accessibility for screen readers.

The keyboard navigation implementation works visually, but the parent Search component lacks ARIA roles needed for screen reader announcements. To properly support keyboard navigation, add:

  1. aria-selected={isSelected} to ActivityItem
  2. role="listbox" to the SearchResults container
  3. role="option" to each ActivityItem
  4. aria-owns or aria-controls on the Input to associate it with the results container
  5. aria-activedescendant on the Input to announce the currently selected item

The parent Search.tsx manages selectedIndex and keyboard navigation via handleKeyDown, but these ARIA attributes are required for screen readers to announce selection changes and navigation state to users.

🤖 Prompt for AI Agents
In apps/activitypub/src/components/activities/ActivityItem.tsx around line 15,
the ActivityItem JSX needs ARIA attributes for screen-reader keyboard
navigation: add role="option" and aria-selected={isSelected} to the ActivityItem
element, ensure the parent SearchResults container (in Search.tsx) has
role="listbox" and a stable id (use the existing testId or generate an id),
update the Input element in Search.tsx to include aria-owns or aria-controls
pointing to the results container id and aria-activedescendant set to the id of
the currently selected ActivityItem (derive each ActivityItem id from its index
or stable key), and ensure ids are stable and unique so screen readers can
announce the active option correctly.

@mike182uk mike182uk force-pushed the ap-search-keyboard-navigation-BER-3119 branch from 87b5646 to 7619ed8 Compare December 9, 2025 16:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
apps/activitypub/src/components/modals/Search.tsx (3)

38-38: Consider deriving STICKY_HEADER_HEIGHT dynamically or documenting the calculation.

The hardcoded value of 80 could become stale if the header's height or padding changes (currently h-[72px] + padding on line 180).

Consider either:

  • Documenting the calculation in a comment (e.g., // 72px height + 8px bottom padding)
  • Computing it dynamically using getBoundingClientRect() on the header element

151-156: Consider reducing or removing the 50ms throttle.

While the throttle prevents excessive updates, 50ms might make keyboard navigation feel slightly sluggish. React efficiently batches rapid state updates, so you could reduce this to 20-30ms or remove it entirely for a more responsive feel.

If you decide to reduce the throttle:

-            if (now - lastKeyPressRef.current < 50) {
+            if (now - lastKeyPressRef.current < 20) {
                 return;
             }

182-192: Consider adding ARIA attributes for better accessibility.

The keyboard navigation implementation is solid, but adding ARIA attributes would improve screen reader support.

Consider adding:

  • role="combobox" to the input
  • aria-expanded={showSearchResults} to indicate results visibility
  • aria-activedescendant to reference the selected item's ID
  • aria-controls to reference the results container ID

These attributes help screen readers understand the relationship between the input and the results list.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87b5646 and 7619ed8.

📒 Files selected for processing (2)
  • apps/activitypub/src/components/activities/ActivityItem.tsx (1 hunks)
  • apps/activitypub/src/components/modals/Search.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/activitypub/src/components/activities/ActivityItem.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.
📚 Learning: 2025-12-09T08:28:22.100Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
🔇 Additional comments (8)
apps/activitypub/src/components/modals/Search.tsx (8)

42-62: LGTM!

The ref declarations and hook usage are clean and appropriate for keyboard navigation state management.


65-101: LGTM!

The unified searchResults list with discriminated union pattern cleanly enables keyboard navigation across both topics and accounts. The selection reset logic correctly prevents out-of-bounds indices.


104-106: LGTM!

The display conditions correctly reference the unified searchResults array, maintaining proper state management for empty, loading, and populated result states.


109-111: LGTM!

Auto-focusing the search input on mount is good UX practice for modal dialogs.


114-141: LGTM!

The scroll-into-view logic correctly handles both Radix scroll containers and standard overflow containers, with appropriate fallback and sticky header compensation.


163-175: LGTM!

The Enter key handler correctly leverages the discriminated union pattern to navigate to the appropriate route based on result type.


208-280: LGTM!

The inline rendering logic correctly handles both result types with proper selection highlighting, ref assignment, and navigation. The conditional rendering for blocked users and the current user is well-structured.


236-245: Avoid double-cast type assertions; verify or refactor the type relationship.

The cast account as unknown as ActorProperties bypasses TypeScript's type checking, which could hide incompatibilities between the account type and ActorProperties. Either verify that the types are compatible and use a direct cast, or create a proper type adapter function instead of chaining casts through unknown.

@github-actions
Copy link
Contributor

Ember E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 20071345300 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@minimaluminium minimaluminium force-pushed the ap-search-keyboard-navigation-BER-3119 branch from 1e50b9a to f1416b5 Compare December 10, 2025 09:49
@minimaluminium minimaluminium force-pushed the ap-search-keyboard-navigation-BER-3119 branch from 706409a to dfea6ba Compare December 10, 2025 09:53
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/activitypub/src/components/modals/Search.tsx (2)

38-38: Consider avoiding magic number for sticky header height

STICKY_HEADER_HEIGHT = 80 is only used in the scroll-into-view logic but the sticky header itself is styled via Tailwind classes (h-[72px], -top-6, etc.). If the header layout changes, the constant can easily get out of sync and cause the selected item to hide under the header.

Consider either:

  • Deriving the effective offset from the DOM (e.g., measuring the sticky header element), or
  • Co-locating this constant with the header styles / documenting the relationship so future layout tweaks don’t silently break the scroll behavior.

Also applies to: 126-155


156-189: Keyboard handler behavior is solid; minor robustness tweaks optional

The ArrowUp/ArrowDown + Enter handling and throttling look fine and match the spec. A couple of small, optional robustness tweaks you might consider:

  • Early-return if searchResults.length === 0 (even though showSearchResults should prevent it today, it hardens against future changes).
  • If you find the 50ms throttle feels sluggish in practice, you could relax or remove it and rely on React’s batching; but that’s subjective and can be tuned later.

These are non-blocking and can be revisited based on UX feedback.

Also applies to: 203-205

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e50b9a and 706409a.

📒 Files selected for processing (3)
  • apps/activitypub/package.json (1 hunks)
  • apps/activitypub/src/components/activities/ActivityItem.tsx (1 hunks)
  • apps/activitypub/src/components/modals/Search.tsx (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/activitypub/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/activitypub/src/components/activities/ActivityItem.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.
📚 Learning: 2025-12-09T08:28:22.100Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 25651
File: apps/activitypub/src/components/modals/Search.tsx:183-199
Timestamp: 2025-12-09T08:28:22.100Z
Learning: In apps/activitypub/src/components/modals/Search.tsx, the topic filtering intentionally uses the immediate `query` value while account search uses `debouncedQuery`. This is by design: topic filtering is a local operation that should update instantly, while account search requires server calls and needs debouncing to avoid excessive API requests.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.

Applied to files:

  • apps/activitypub/src/components/modals/Search.tsx
🔇 Additional comments (4)
apps/activitypub/src/components/modals/Search.tsx (4)

23-30: Good use of discriminated union for unified search results

The TopicSearchResult interface and SearchResult discriminated union are a clean way to unify topics and accounts, and they simplify the rendering/keyboard logic against a single searchResults array. This also makes future extension (more result types) straightforward.

Also applies to: 88-92


68-86: Topic vs account search behavior preserved correctly

The client-side topic filtering still uses the immediate query value, while the account search is driven by debouncedQuery (useSearchForUser('index', shouldSearch ? debouncedQuery : '')). This preserves the intentional behavior where topics update instantly but account lookups are debounced to protect the API.

Based on learnings, this matches the intended design for this component.

Also applies to: 88-92


121-124: Input focus-on-mount behavior looks good

Auto‑focusing the query input on mount via queryInputRef.current?.focus() is appropriate for this modal and doesn’t introduce obvious side effects, since it only runs client-side in useEffect.


221-293: Unified rendering with selection highlighting is well-structured

The unified searchResults.map rendering for topics and accounts, with:

  • isSelected passed into ActivityItem,
  • ref={isSelected ? selectedItemRef : undefined} on the per-row wrapper,
  • Correct keys (topic.slug / account.id),
  • isCurrentUser detection via currentUser?.handle,
  • Follow/blocked state wired through updateResult,

is clean and matches the new keyboard navigation model. This should give a clear visual highlight and consistent activation behavior for both result types.

@github-actions
Copy link
Contributor

Ember E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 20071345300 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

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.

3 participants