✨ Added Explore by topic to the Network tab #25500
Conversation
WalkthroughThis PR bumps activitypub to 1.1.0, removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/activitypub/src/views/Explore/Explore.tsx (4)
43-83: Check click bubbling from FollowButton and skeleton rowsBecause the whole row is clickable and navigates to
/profile/${profile.handle}, there are two small UX edge cases to verify:
- Clicking the
FollowButtonmay also trigger the rowonClickand navigate to the profile unlessFollowButton(or its internal button) stops propagation.- In the loading state, skeleton rows still have the same
onClick, which will currently navigate to/profile/(empty handle).If this is not desired, consider guarding the row click, e.g.:
- <div - className='flex w-full cursor-pointer items-start gap-3 pt-4 [&:last-of-type>:nth-child(2)]:border-none' - onClick={() => { - navigate(`/profile/${profile.handle}`); - }} - > + <div + className='flex w-full cursor-pointer items-start gap-3 pt-4 [&:last-of-type>:nth-child(2)]:border-none' + onClick={() => { + if (!isLoading && profile.handle) { + navigate(`/profile/${profile.handle}`); + } + }} + >or ensure
FollowButtoninternally callsevent.stopPropagation()on its clicks.
101-107: Normalize/validate the topic route param
const topic: Topic = (params.topic as Topic) || 'top';trusts whatever is in the URL and casts it toTopic. For unexpected URLs (e.g. manual edits or old links), you can end up with an invalid topic string flowing intouseExploreProfilesForUserByTopicandTopicFilter.If you want a stricter behavior, consider validating against the known topics and falling back to
'top', for example (assuming you expose a topics array fromTopicFilter):const rawTopic = params.topic ?? 'top'; const topic: Topic = TOPICS.includes(rawTopic as Topic) ? (rawTopic as Topic) : 'top';Not critical, but it would make the route handling more robust to bad URLs.
123-140: IntersectionObserver: optional guard for unsupported environmentsThe infinite‑scroll
IntersectionObserversetup is clean and the cleanup is correct. If you ever run this component in environments withoutIntersectionObserver(older browsers, some test runners), a small defensive check could avoid runtime errors:- const observer = new IntersectionObserver( + if (typeof IntersectionObserver === 'undefined') { + return; + } + const observer = new IntersectionObserver(Not required if your supported environments/polyfills already guarantee
IntersectionObserver, but worth considering.
167-201: Loading / empty states for topicsThe skeleton state and subsequent infinite‑scroll list look good. One small UX enhancement to consider: when
!isLoadingExploreProfilesandprofiles.length === 0, users currently see an empty area with just the invisible.load-more-trigger.You might add an explicit “No accounts to show for this topic yet” style message in that case to make it clear that loading has finished. This can be layered on top of the existing logic without affecting the paging behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/activitypub/package.json(1 hunks)apps/activitypub/src/components/layout/Layout.tsx(1 hunks)apps/activitypub/src/lib/feature-flags.tsx(1 hunks)apps/activitypub/src/views/Explore/Explore.tsx(1 hunks)apps/activitypub/src/views/Explore/ExploreByFollowing.tsx(0 hunks)apps/activitypub/src/views/Explore/ExploreByTopic.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- apps/activitypub/src/views/Explore/ExploreByFollowing.tsx
- apps/activitypub/src/views/Explore/ExploreByTopic.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-10T11:07:10.800Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Applied to files:
apps/activitypub/src/lib/feature-flags.tsx
📚 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
🧬 Code graph analysis (1)
apps/activitypub/src/views/Explore/Explore.tsx (5)
apps/activitypub/src/api/activitypub.ts (2)
ExploreAccount(37-40)Account(9-30)apps/activitypub/src/hooks/use-activity-pub-queries.ts (2)
useAccountForUser(1616-1628)useExploreProfilesForUserByTopic(2615-2673)apps/activitypub/src/hooks/use-navigate-with-base-path.ts (1)
useNavigateWithBasePath(15-37)apps/activitypub/src/utils/content-formatters.ts (2)
openLinksInNewTab(94-114)stripHtml(1-53)apps/activitypub/src/components/TopicFilter.tsx (1)
Topic(4-4)
🪛 ast-grep (0.40.0)
apps/activitypub/src/views/Explore/Explore.tsx
[warning] 90-90: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/activitypub/src/views/Explore/Explore.tsx
[error] 91-91: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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: Setup
- GitHub Check: Build & Push
🔇 Additional comments (4)
apps/activitypub/package.json (1)
3-3: Version bump aligns with feature scopeBumping
@tryghost/activitypubto1.1.0for the Explore consolidation looks appropriate; nothing else in this manifest raises concerns.apps/activitypub/src/components/layout/Layout.tsx (1)
49-51: Header border condition simplification looks goodRemoving the dependency on
explore-topicand hiding the border on allexploreandreaderpages is consistent with the unified Explore flow; no behavioral regressions apparent from this file.apps/activitypub/src/lib/feature-flags.tsx (1)
5-5: Clearing built‑in feature flags matches flag removalSetting
FEATURE_FLAGSto an empty array cleanly removesexplore-topicwhile keeping the feature‑flag plumbing intact (localStorage +isEnabled). This is a reasonable baseline until new flags are introduced.apps/activitypub/src/views/Explore/Explore.tsx (1)
21-39: ExploreProfile optimistic follow/unfollow wiring looks solidUsing the
update(id, partialAccount)callback to optimistically togglefollowedByMekeeps the Explore list in sync with mutations without extra refetches; theisCurrentUserguard prevents self‑follow. This structure looks good.
| {isLoading ? | ||
| <Skeleton className='w-full max-w-96' /> | ||
| : | ||
| profile.bio && | ||
| <div | ||
| dangerouslySetInnerHTML={{__html: openLinksInNewTab(stripHtml(profile.bio, ['a', 'br']))}} | ||
| className='ap-profile-content pointer-events-none mt-0 line-clamp-2 max-w-[460px] break-anywhere' | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
Re‑evaluate XSS posture around dangerouslySetInnerHTML
You’re sensibly running profile.bio through stripHtml(..., ['a', 'br']) and openLinksInNewTab, which:
- Strips all tags except
<a>/<br>. - Normalizes link targets and blocks obviously dangerous protocols.
However, this pipeline does not remove all potentially unsafe attributes on <a> (for example, event handlers like onclick, onfocus, etc.), and the result is injected via dangerouslySetInnerHTML. That leaves you reliant on upstream sanitization for bios coming from ActivityPub.
Depending on your threat model, consider one of:
- Passing the bio through a dedicated HTML sanitizer (e.g., DOMPurify) on the frontend before
dangerouslySetInnerHTML, or - Ensuring/confirming that the backend already strips unsafe attributes from bios and documenting that assumption here (and potentially relaxing the linter rule only for this well‑audited case).
Right now, this is more of a security‑posture gap than a clear exploit, but it’s worth double‑checking.
I'll verify the XSS posture by examining the sanitization functions and their implementations.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for stripHtml function definition
rg -n "stripHtml\s*=" --type ts --type tsx --type js
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for openLinksInNewTab function definition
rg -n "openLinksInNewTab\s*=" --type ts --type tsx --type jsLength of output: 27
🏁 Script executed:
#!/bin/bash
# Check package.json for sanitization libraries
cat package.json | grep -A 5 -B 5 -E "dompurify|sanitize|xss" || echo "No sanitizer found in main package.json"Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find all package.json files
fd package.jsonLength of output: 1165
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for stripHtml function definition without type restriction
rg -n "stripHtml\s*=" --context 3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for openLinksInNewTab function definition
rg -n "openLinksInNewTab\s*=" --context 3Length of output: 573
🏁 Script executed:
#!/bin/bash
# Check imports in Explore.tsx file
cat apps/activitypub/src/views/Explore/Explore.tsx | head -50Length of output: 2092
🏁 Script executed:
#!/bin/bash
# Check activitypub package.json for sanitization dependencies
cat apps/activitypub/package.json | grep -E "dompurify|sanitize|xss|dependencies|devDependencies" -A 20Length of output: 733
🏁 Script executed:
#!/bin/bash
# Search where these functions are imported from in all activitypub files
rg "stripHtml|openLinksInNewTab" apps/activitypub/src --context 2Length of output: 12888
🏁 Script executed:
#!/bin/bash
# Search for backend ActivityPub profile or bio handling
rg -i "profile.*bio|bio.*sanitiz" --max-count 20Length of output: 159155
---
🏁 Script executed:
```shell
#!/bin/bash
# View the complete content-formatters.ts file
cat apps/activitypub/src/utils/content-formatters.ts
Length of output: 4662
🏁 Script executed:
#!/bin/bash
# Search for any ActivityPub bio sanitization backend code
rg -i "bio" apps/activitypub/src --type ts -C 2 | head -100Length of output: 8062
🏁 Script executed:
#!/bin/bash
# Check if there's any backend sanitization in ghost/core for ActivityPub
rg -i "activitypub.*bio\|profile.*sanitize" ghost/core --type ts --type js | head -20Length of output: 0
Confirm: unsafe event handler attributes on whitelisted tags are not stripped
The review's concern is valid. The stripHtml function preserves excluded tags like <a> with all their attributes intact, and openLinksInNewTab only modifies the href, target, and rel attributes—it does not remove event handler attributes like onclick, onfocus, onerror, or onload. An attacker-controlled ActivityPub bio containing <a onclick="malicious()">text</a> would pass through the pipeline unchanged and execute when rendered. The pointer-events-none CSS class provides no XSS mitigation—it only prevents pointer events and can be overridden.
The pipeline is reliant on upstream ActivityPub sanitization, which should be documented and audited.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 90-90: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 91-91: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
ref https://linear.app/ghost/project/activitypub-45ebefb7b78b/overview#milestone-ad7b6573-7d09-4c69-930f-f8e50677f0ad