-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
Improved search UX by eliminating content flashing in ActivityPub #25666
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.
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
Conversation
WalkthroughThe PR updates apps/activitypub/package.json version from "3.0.2" to "3.0.3". In apps/activitypub/src/components/modals/Search.tsx a new state lastResultState (null | 'results' | 'none') was added. The effect that updates displayResults now uses data?.accounts, isFetched, and shouldSearch; when shouldSearch is false it clears displayResults and resets lastResultState, when data arrives it sets displayResults and lastResultState to 'results', and when no results it clears displayResults and sets lastResultState to 'none'. Visibility logic for suggestions, no-results, and search-results was reworked to depend on lastResultState; loading is still derived from isFetching and shouldSearch. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (2)
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. Comment |
There was a problem hiding this 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 (1)
apps/activitypub/src/components/modals/Search.tsx (1)
141-147: Search state flags look correct; consider minor boolean coercion for clarityThe interplay between:
showLoading = isFetching && shouldSearchhasResults = data?.accounts && data.accounts.length > 0showNoResults = !isFetching && isFetched && !hasResults && shouldSearch && debouncedQuery === queryshowSearchResults = shouldSearch && (hasResults || (isFetching && displayResults.length > 0))showSuggested = query.length < 2 || (shouldSearch && !showSearchResults && !showNoResults)gives sensible states in all the key flows:
- Initial load / typing to length ≥ 2: suggestions stay visible until either results arrive or a definitive empty response is fetched.
- While fetching with existing results:
showSearchResultsstays true via the(isFetching && displayResults.length > 0)branch, so old results remain visible.- When a search completes with zero results:
showNoResultsbecomes true (withdebouncedQuery === query), and suggestions/results are both hidden, avoiding the “no results” flicker for transient queries.One small optional tidy‑up:
hasResultscan be explicitly boolean to make the intent clearer and avoid theboolean | undefinedtype:- const hasResults = data?.accounts && data.accounts.length > 0; + const hasResults = !!(data?.accounts && data.accounts.length > 0);This doesn’t change behavior but can simplify reasoning for future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/activitypub/package.json(1 hunks)apps/activitypub/src/components/modals/Search.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
apps/activitypub/package.json
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
apps/activitypub/package.json
📚 Learning: 2025-03-13T09:02:50.102Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx:29-34
Timestamp: 2025-03-13T09:02:50.102Z
Learning: In the Ghost ActivityPub module, error handling for mutations is handled at the hook level (in use-activity-pub-queries.ts) rather than in individual components. This allows for centralized error handling across the application.
Applied to files:
apps/activitypub/package.json
📚 Learning: 2025-08-07T12:14:51.248Z
Learnt from: vershwal
Repo: TryGhost/Ghost PR: 24621
File: apps/admin-x-activitypub/src/utils/posts.ts:145-145
Timestamp: 2025-08-07T12:14:51.248Z
Learning: In Ghost's ActivityPub implementation, the `object.published` field from ActivityPub activities is expected to always contain a valid date format, so defensive date validation is not required when parsing it in `mapActivityToPost()`.
Applied to files:
apps/activitypub/package.json
🧬 Code graph analysis (1)
apps/activitypub/src/components/modals/Search.tsx (1)
apps/sodo-search/src/components/popup-modal.js (1)
hasResults(495-495)
⏰ 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: Setup
- GitHub Check: Build & Push
- GitHub Check: Setup
🔇 Additional comments (2)
apps/activitypub/package.json (1)
3-3: Version bump looks consistent with ActivityPub changesThe version increment to
2.0.6cleanly reflects the new ActivityPub search UX behavior; no issues from this file alone.apps/activitypub/src/components/modals/Search.tsx (1)
133-139: Effect logic now matches “keep results while typing” requirementThis effect now:
- Updates
displayResultswhen new accounts arrive.- Clears results only when
!shouldSearch(query too short).That should prevent results from being cleared on every keystroke while a search is in flight, and only reset when the user stops “searching” entirely. Behavior looks aligned with the PR goal and doesn’t introduce obvious edge‑case gaps.
apps/activitypub/package.json
Outdated
| { | ||
| "name": "@tryghost/activitypub", | ||
| "version": "2.0.5", | ||
| "version": "2.0.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump me :)
There was a problem hiding this 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/activitypub/src/components/modals/Search.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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
🔇 Additional comments (2)
apps/activitypub/src/components/modals/Search.tsx (2)
132-132: Good addition to track result state independently.The
lastResultStatevariable effectively distinguishes between "no results yet" (null) and "no results found" ('none'), which is key to preventing content flashing.
156-158: Verify: Are suggestions intentionally shown during initial search?The condition
!lastResultState && shouldSearchon line 156 means suggested profiles display during the first search while results are being fetched. This provides content instead of a blank screen, but could be considered a "flash" of suggestions before results appear.The PR description mentions eliminating flashes where "suggested accounts" briefly appear, so this behavior might warrant review.
If suggestions shouldn't show during the initial fetch, consider:
-const showSuggested = query.length < 2 || (!lastResultState && shouldSearch); +const showSuggested = query.length < 2 || (!lastResultState && shouldSearch && !isFetching);This would prevent suggestions from appearing while the first search is loading. However, this might result in a blank screen with only a loading indicator, which could be acceptable or undesirable depending on UX preferences.
Fixed flashing between no result and suggested accounts
69e8cbd to
07e5510
Compare
ref https://linear.app/ghost/issue/BER-3081/slice-1-the-search-result-shouldnt-be-cleared-on-keystroke
Previously, when typing search queries, there were jarring flashes where content would briefly disappear or show incorrect states (suggested accounts, "no results found") before displaying the actual results. This happened because display logic relied on delayed state updates rather than the actual query data.
The fix ensures smooth transitions by:
data?.accounts) for display conditions instead of delayed display state