web-next: Profile hover cards for avatars, names, and mentions#276
Conversation
|
@codex review |
📝 WalkthroughWalkthroughAdds actor hover cards and mention-hover popovers: UI primitives, preloaders, preview card + skeleton, mention-hover hook/layer, GraphQL actor-by-URL lookup and tests, and integrates hover/mention behavior across many components and routes. ChangesActor and Mention Hover Cards
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Timeline/Content
participant HoverCard as ActorHoverCard
participant Loader as ActorHoverCardLoader
participant API as Relay/GraphQL
participant Preview as ActorPreviewCard
User->>UI: hover actor avatar/link
UI->>HoverCard: trigger open
HoverCard->>Loader: mount with handle/url
Loader->>API: loadQuery(actorByHandle/actorByUrl)
API-->>Loader: returns data
Loader->>Preview: render skeleton then preview
Preview-->>User: display hover card
User->>UI: pointer out
HoverCard->>Loader: unmount (close)
sequenceDiagram
participant User as User
participant Prose as Article/Note Prose
participant Hook as useMentionHoverCards
participant Layer as MentionHoverCardLayer
participant Loader as ActorHoverCardLoader
User->>Prose: hover mention link
Prose->>Hook: pointerover -> resolve anchor and lookup
Hook->>Hook: start open delay (400ms)
Hook->>Layer: set anchor/lookup and open=true
Layer->>Loader: mount loader for handle or url
Loader-->>User: show skeleton then preview
User->>Prose: move away
Hook->>Hook: start close delay (200ms)
Hook->>Layer: open=false -> unmount loader
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Wraps Kobalte's HoverCard for the new web-next stack, mirroring the existing Tooltip primitive in components/ui/. The default content surface follows DESIGN.md (rounded-lg, bg-popover, single elevation shadow-md) and uses the motion-safe: prefix on the open/close animation classes so reduced-motion users get a static fade. This is the first step toward profile hover cards (issue hackers-pub#90). hackers-pub#90 Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
Three new pieces, not yet wired into any surface:
- ActorPreviewCard: the body shown inside a hover card. New
ActorPreviewCard_actor fragment selects name, handle, avatar, bio,
follow counts, plus the FollowButton fragment. Layout follows
DESIGN.md (achromatic surface, single elevation, hairline borders)
and matches the X reference in spirit: avatar + name + follow on
top, clamped bio, counts row.
- ActorPreviewSkeleton: matched-proportions placeholder shown while
the actor query is in flight.
- ActorHoverCardLoader: lazy actor fetch via the established
loadQuery + createPreloadedQuery pattern, scoped behind an
ErrorBoundary so a failing GraphQL/network call collapses to the
same "Could not load profile." fallback as a null actorByHandle.
- ActorHoverCard: thin glue wrapping a child in a HoverCardTrigger
with role="presentation" + tabIndex={-1} (drops Kobalte Link.Root's
phantom tab stop) and class="inline-flex" so Popper has a real
anchor rect. Defers loader mount until open.
hackers-pub#90
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Wraps avatar and display-name links across the timeline, profile cards, notifications, and shares so hovering them shows the actor preview (avatar, name, handle, bio, follow counts, follow button). The trigger wrapper is a non-focusable inline-flex span; ActorHoverCard now accepts an optional `class` prop so call sites that wrap a fixed- size avatar in a flex container pass `shrink-0` for the wrapper itself. NotificationMessage's actor stack moves its z-index/transition utility classes from each `<Avatar>` to the `ActorHoverCard` wrapper so the parent's `-space-x-*` and the per-item `hover:z-10` keep working with the wrapper as the direct flex child. Surfaces wired: PostAvatar, NoteHeader display-name, ArticleCard avatar + name, QuotedNoteCard avatar + name, SmallProfileCard avatar + name, NotificationActor link, NotificationMessage avatar stack, PostSharer "shared by" link, QuestionCard shared-actor link, and the inline display-name link inside QuestionCardContent. hackers-pub#90 Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
Mention layer:
- New `web-next/src/lib/mentionHoverCards.tsx` exports
`useMentionHoverCards(ref)` + `<MentionHoverCardLayer>`. The hook
takes a reactive ref accessor, attaches delegated `pointerover` /
`pointerout` listeners via `createEffect` (so listeners track the
current container element across re-renders), and debounces a
400ms open / 200ms close. Reads the canonical fediverse handle
from the mention's `data-username` + `data-host` attributes,
constructs `@user@host`, and feeds it to `ActorHoverCardLoader`
inside a Kobalte `Popover` with `modal={false}` and an external
`anchorRef`. Effect cleanup also drops pending timers and resets
state if the current anchor was inside the detached element, so
no stale popovers can fire against a removed node.
- Applied the hook to every prose surface that may contain
`@mentions`: NoteCardInternal, QuotedNoteCard, ProfileCard bio,
QuestionCardContent, and ArticleCard (one ref + layer per
`<article>`, since prose appears in four mutually-exclusive
summary/content branches).
Avatar trigger fix:
- The `ActorHoverCard` trigger was an `inline-flex` span. In flex
parents with default `align-items: stretch`, the wrapper was
stretching to the full cross-axis height, so Kobalte's anchor
rect grew to the entire note's height and the hover card landed
far below the avatar instead of right under it. Adding
`self-start` keeps the wrapper hugging the avatar's natural
height while remaining a no-op for non-flex parents.
hackers-pub#90
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The new "Could not load profile." string is shown by ActorHoverCardLoader when the lazy actorByHandle query returns null (suspended/unknown actor) or fails. Filled in for ja-JP, ko-KR, zh-CN, zh-TW per each locale's glossary and tone conventions; the en-US catalog only picks up the line-number reshuffle from the new extraction. Assisted-by: Claude Code:claude-opus-4-7
Three follow-ups from a Codex review of the full branch diff:
- The full article detail page
(`web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx`)
now wraps the author avatar and display-name link with
`ActorHoverCard`, and attaches `useMentionHoverCards` +
`MentionHoverCardLayer` to the article body's prose `<div>`. Hover
cards previously worked on previews but not on the article itself.
- `SmallProfileCard` (used in followers / following lists) now wires
the same mention hover-card layer onto its bio prose, so mentions
inside short bios behave consistently with the rest of the app.
- Popover-surface styling was duplicated between `HoverCardContent`
and `MentionHoverCardLayer`; extracted to a single
`ACTOR_HOVER_SURFACE_CLASS` exported from
`web-next/src/components/ui/hover-card.tsx`. Each surface only adds
its primitive-specific `--kb-*-content-transform-origin` variable.
hackers-pub#90
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Inbound ActivityPub content arrives with Mastodon-style h-card markup (<span class="h-card"><a class="u-url mention" href="…">@user</a></span>) that lacks our local renderer's `data-username` / `data-host` attrs, so the hook silently bailed on those mentions and the hover card never opened. The mention detector now falls back to deriving the host from `URL(href).host` and the username from the link's text content (leading `@` stripped, second `@` split off to handle `@user@host` text rendering). Locally-rendered mentions still take the fast `data-*` path unchanged. hackers-pub#90 Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
dahlia
force-pushed
the
web-next/hover-cards
branch
from
ab0212b to
533feac
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements actor hover cards across the web-next application, allowing users to preview profile information by hovering over avatars, names, or mentions. It introduces several new components, including ActorHoverCard, ActorPreviewCard, and a specialized hook for handling mentions in rendered content. The feedback focuses on ensuring that the username is correctly displayed as a fallback when an actor's display name is an empty string, suggesting the use of the logical OR operator instead of nullish coalescing in the preview components.
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-next/src/components/NoteHeader.tsx (1)
45-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the hover card on the handle fallback too.
When
n().actor.nameis empty, the visible identity is just the handle, but it sits outside theActorHoverCardtrigger. That means handle-only profiles lose the new preview entirely on note headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-next/src/components/NoteHeader.tsx` around lines 45 - 63, The ActorHoverCard trigger currently wraps only the name (InternalLink with innerHTML using n().actor.name) so when n().actor.name is empty the visible handle (span showing n().actor.handle) is outside the hover card and loses the preview; update the JSX so ActorHoverCard (component ActorHoverCard) wraps both the name/InternalLink fallback and the handle span (or move the InternalLink to render the handle when name is empty) so that n().actor.handle is included as the hover trigger in all cases, preserving the existing href/internalHref behavior (n().actor.url ?? n().actor.iri and the internalHref logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-next/src/components/ActorPreviewCard.tsx`:
- Around line 48-66: The anchor tags that use profileHref()/profileTarget() (the
avatar link and the name link in ActorPreviewCard) open remote profiles in a new
tab but lack rel attributes; add a rel="noreferrer noopener" for non-local links
to prevent tabnabbing. Implement this by either introducing a small helper
(e.g., profileRel() that returns "noreferrer noopener" when profileTarget()
returns "_blank" or undefined otherwise) or inline the conditional rel prop on
both <a href={profileHref()} target={profileTarget()} ...> occurrences so remote
links include rel="noreferrer noopener" while local links keep rel undefined.
---
Outside diff comments:
In `@web-next/src/components/NoteHeader.tsx`:
- Around line 45-63: The ActorHoverCard trigger currently wraps only the name
(InternalLink with innerHTML using n().actor.name) so when n().actor.name is
empty the visible handle (span showing n().actor.handle) is outside the hover
card and loses the preview; update the JSX so ActorHoverCard (component
ActorHoverCard) wraps both the name/InternalLink fallback and the handle span
(or move the InternalLink to render the handle when name is empty) so that
n().actor.handle is included as the hover trigger in all cases, preserving the
existing href/internalHref behavior (n().actor.url ?? n().actor.iri and the
internalHref logic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5ced6f0-7d4a-4e78-ad12-86d83f91fd37
📒 Files selected for processing (23)
web-next/src/components/ActorHoverCard.tsxweb-next/src/components/ActorHoverCardLoader.tsxweb-next/src/components/ActorPreviewCard.tsxweb-next/src/components/ActorPreviewSkeleton.tsxweb-next/src/components/ArticleCard.tsxweb-next/src/components/NoteCardInternal.tsxweb-next/src/components/NoteHeader.tsxweb-next/src/components/NotificationActor.tsxweb-next/src/components/PostAvatar.tsxweb-next/src/components/PostSharer.tsxweb-next/src/components/ProfileCard.tsxweb-next/src/components/QuestionCard.tsxweb-next/src/components/QuotedNoteCard.tsxweb-next/src/components/SmallProfileCard.tsxweb-next/src/components/notification/NotificationMessage.tsxweb-next/src/components/ui/hover-card.tsxweb-next/src/lib/mentionHoverCards.tsxweb-next/src/locales/en-US/messages.poweb-next/src/locales/ja-JP/messages.poweb-next/src/locales/ko-KR/messages.poweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.poweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx
The hover-card and small-profile-card name links used `name ?? username` to choose what to render. Nullish coalescing only falls back when the left side is null/undefined, so an actor whose display name is set to an empty string still rendered a blank link. Switching to `||` covers the empty-string case the same way an unset name is covered. hackers-pub#276 (comment) hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
The avatar and name links in ActorPreviewCard open remote actors in a new tab via target="_blank". Without rel="noopener", the new tab can reach back to the opener (window.opener) and trigger a tabnabbing redirect. Modern browsers add the noopener default for plain target="_blank", but the project's other target="_blank" call sites (LinkPreview, AppSidebar, ArticleComposerForm) set the rel explicitly, so match that. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a profile hover card system for the new Solid-based stack in web-next. It includes a generic ActorHoverCard component and a specialized MentionHoverCardLayer that uses a custom hook to attach hover behavior to mentions within rendered HTML content. The system is integrated across multiple components, including articles, notes, and notifications. Feedback highlights an opportunity to refactor duplicated follow-count logic in ActorPreviewCard and suggests improving the robustness of the username parsing logic for federated content in mentionHoverCards.tsx.
|
@codex review |
The header wrapped only the display-name link with ActorHoverCard, so when an actor had no name the entire Show fell through and the only remaining identifier (the handle span) had no hover preview at all. Now the handle span has its own ActorHoverCard, so the hover card is reachable from the handle whether or not the name is set. The wrapper takes over the handle span's `min-w-0 grow` to keep the flex grow/truncation behavior; `truncate` stays on the inner span. Assisted-by: Claude Code:claude-opus-4-7
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Same fix as 10d647e for the rest of the surfaces that have a name link gated on a non-empty display name plus a sibling handle span: QuotedNoteCard, ArticleCard, and QuestionCard. The handle span now has its own ActorHoverCard wrapper, so the hover preview is reachable from the handle whether or not the actor has a display name. Layout-affecting utilities (`min-w-0`, `min-w-0 grow`) move from the inner span to the wrapper class where present, since the wrapper is now the flex item; `truncate` and `break-all` stay on the inner span. Assisted-by: Claude Code:claude-opus-4-7
The local and remote branches of the follow-counts row each repeated the plural strings and the i18n call. Use the same single-anchor shape ProfileCard already uses: one <a> with a conditional href that is set for local actors (the only case where the followers / following routes exist) and undefined for remote actors. Same rendered text, half the JSX. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
Splitting the link text at the first `@` works for the common Mastodon-style `@user` rendering but is fragile when the link contains decorative children, custom emoji, or a multi-`@` text shape. The mention's `href` is more structured: across the common ActivityPub profile URL shapes (`/@user`, `/users/user`, `/profile/user`) the username is the last non-empty path segment, optionally with a leading `@`. Pull the username from the URL pathname first, and only fall back to the previous text-content extraction when the URL path doesn't produce a non-empty segment. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request implements actor hover cards across the web-next application, enabling profile previews when hovering over user handles, names, or mentions. It introduces several new components, including ActorHoverCard, ActorPreviewCard, and a MentionHoverCardLayer that uses a custom hook to detect and resolve mentions within rendered content. Feedback primarily addresses the ActorPreviewCard, suggesting the use of InternalLink to preserve SPA navigation for profile and follower links, and recommending a safer rendering pattern for actor names to avoid potential issues with innerHTML.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a4619135
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Mirrors the existing lookupPostByUrl helper for actors. The new lookupActorByUrl(ctx, parsed) probes the local actor table by `iri` first (the canonical, unique ActivityPub identifier) and then falls back to the human-facing `url` (nullable, non-unique) so a colliding `url` value never wins over an exact IRI match. If neither matches, it asks the federation context to lookupObject + persistActor. Exposed as the actorByUrl(url: URL!): Actor query. The resolver is a drizzleField so Pothos can drive selection-based relation loads on the returned actor; the helper returns a raw row and the resolver re-fetches it through the Pothos query builder before returning. Tests cover four cases: local actor by IRI, remote actor by IRI, remote actor by `url`, and the IRI-vs-url precedence regression. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
graphql/lookup.ts (1)
68-100: Unauthenticated federation lookup triggers apersistActorDB write
lookupActorByUrlis exposed as a public GraphQL resolver (no authentication required) and matches the existingactorByHandlepattern, but hover cards significantly lower the trigger threshold: any page visitor (including unauthenticated guests) hovering a remote mention in a public post will cause an outbound HTTP request to the remote instance and, if the actor is not yet local, apersistActorwrite to the database.Key risk surface:
- A popular post with many remote
@mentionscould generate a burst of federation requests from readers, each potentially persisting new actor rows.- A malicious post author could include mentions of attacker-controlled URLs and use readers' browsers to perform server-side request forgery-style probing — every hover sends a
lookupObjectrequest from the server.Note: CVE-2026-34148 (redirect loop DoS in Fedify) is patched in v2.1.5 (in use), so that specific concern does not apply.
Consider rate-limiting the
actorByUrlresolver per IP/session, or restricting the federation fallback to authenticated requests only (consistent with how sensitive federation operations are typically guarded).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/lookup.ts` around lines 68 - 100, lookupActorByUrl currently performs network fetch + persistActor even for unauthenticated requests; to prevent unauth guests from triggering federation writes, change lookupActorByUrl so that after the local DB checks (byIri, byUrl) it returns null for unauthenticated callers instead of calling ctx.fedCtx.lookupObject/persistActor. Concretely: in lookupActorByUrl, check ctx.account == null and if so skip the documentLoader/lookupObject/persistActor path (return null), leaving authenticated flows unchanged so only authenticated users can trigger network lookups and DB writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-next/src/components/ActorHoverCardLoader.tsx`:
- Around line 51-70: The createPreloadedQuery factory in ActorHoverCardLoader
reads reactive props.handle/props.url and may call loadQuery multiple times;
capture and dispose the previous PreloadedQuery ref before creating a new one
(or refactor to use useQueryLoader which auto-disposes). Specifically, inside
ActorHoverCardLoader ensure the factory stores the last returned ref (from
loadQuery) in a stable variable (e.g., a ref) and calls previousRef.dispose()
before assigning a new ref, and also dispose on unmount; target the
createPreloadedQuery factory, the loadQuery call, and MentionHoverCardLayer
usage to prevent leaking Relay subscriptions.
---
Nitpick comments:
In `@graphql/lookup.ts`:
- Around line 68-100: lookupActorByUrl currently performs network fetch +
persistActor even for unauthenticated requests; to prevent unauth guests from
triggering federation writes, change lookupActorByUrl so that after the local DB
checks (byIri, byUrl) it returns null for unauthenticated callers instead of
calling ctx.fedCtx.lookupObject/persistActor. Concretely: in lookupActorByUrl,
check ctx.account == null and if so skip the
documentLoader/lookupObject/persistActor path (return null), leaving
authenticated flows unchanged so only authenticated users can trigger network
lookups and DB writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44fe593d-1414-4783-a3fc-4fb3214c60da
📒 Files selected for processing (27)
graphql/actor.more.test.tsgraphql/actor.tsgraphql/lookup.tsgraphql/schema.graphqlweb-next/src/components/ActorHoverCard.tsxweb-next/src/components/ActorHoverCardLoader.tsxweb-next/src/components/ActorPreviewCard.tsxweb-next/src/components/ActorPreviewSkeleton.tsxweb-next/src/components/ArticleCard.tsxweb-next/src/components/NoteCardInternal.tsxweb-next/src/components/NoteHeader.tsxweb-next/src/components/NotificationActor.tsxweb-next/src/components/PostAvatar.tsxweb-next/src/components/PostSharer.tsxweb-next/src/components/ProfileCard.tsxweb-next/src/components/QuestionCard.tsxweb-next/src/components/QuotedNoteCard.tsxweb-next/src/components/SmallProfileCard.tsxweb-next/src/components/notification/NotificationMessage.tsxweb-next/src/components/ui/hover-card.tsxweb-next/src/lib/mentionHoverCards.tsxweb-next/src/locales/en-US/messages.poweb-next/src/locales/ja-JP/messages.poweb-next/src/locales/ko-KR/messages.poweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.poweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx
✅ Files skipped from review due to trivial changes (7)
- web-next/src/components/PostAvatar.tsx
- web-next/src/components/ActorHoverCard.tsx
- web-next/src/components/ActorPreviewSkeleton.tsx
- web-next/src/components/ProfileCard.tsx
- web-next/src/components/ui/hover-card.tsx
- web-next/src/components/QuestionCard.tsx
- web-next/src/locales/ko-KR/messages.po
🚧 Files skipped from review as they are similar to previous changes (8)
- web-next/src/components/PostSharer.tsx
- web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx
- web-next/src/components/ArticleCard.tsx
- web-next/src/locales/ja-JP/messages.po
- web-next/src/locales/zh-CN/messages.po
- web-next/src/components/QuotedNoteCard.tsx
- web-next/src/components/notification/NotificationMessage.tsx
- web-next/src/locales/en-US/messages.po
There was a problem hiding this comment.
Code Review
This pull request introduces actor hover cards across the web-next stack, allowing users to preview profile information by hovering over avatars, names, handles, or mentions in posts. It includes a new actorByUrl GraphQL query to resolve actors via ActivityPub IRIs or human-facing URLs, along with the necessary backend lookup logic and frontend components. Feedback focuses on improving the user experience in ArticleCard by consolidating hover card instances to prevent flickering and addressing a potential XSS vulnerability in SmallProfileCard by avoiding innerHTML for username fallbacks.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Same fix pattern as the recent ActorPreviewCard change. The name link rendered `name || username` through innerHTML, so a user-typed username (no HTML sanitization) would have been interpreted as markup. Wrap the link in a Show whose body uses innerHTML for the sanitized name and whose fallback renders the username as plain text children. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the actorByUrl GraphQL query to resolve actors by their IRI or human-facing URL, with support for federation lookups. It also introduces the ActorHoverCard component and MentionHoverCardLayer to display profile previews when hovering over actor handles or mentions. Feedback was provided regarding the use of window.setTimeout in the hover card logic, which may cause issues in server-side rendering environments like SolidStart.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Every other setTimeout call site in web-next/src/ uses the bare global (e.g., InternalLink, MentionAutocomplete, QuestionCard, useUnsavedGuard, sign/index.tsx). The `window.` prefix here was the only outlier, and reaching into `window` in module-level code is unnecessary in SSR-aware contexts since the global is available everywhere setTimeout is. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements profile hover cards across the web-next application, allowing users to preview actor information by hovering over names, handles, avatars, or mentions. It introduces a new actorByUrl GraphQL query to resolve actors from ActivityPub IRIs or human-facing URLs, supported by backend lookup logic and comprehensive tests. Feedback includes a recommendation to wrap adjacent actor identifiers in a single hover card component to prevent UI flickering and a suggestion to provide a base URL to the URL constructor in the mention lookup logic to safely handle relative links.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Each post-card header had two adjacent ActorHoverCard wrappers for the same actor (one around the name link, one around the handle span). Moving the cursor between them closed one card and re-opened another after each card's open/close delay, even though the content was identical. Wrap both children in a single ActorHoverCard whose class makes the wrapper itself the flex row that previously lived on the parent div, so hovering anywhere across name+handle keeps the card open. Touched: NoteHeader, ArticleCard, QuotedNoteCard, QuestionCard. The avatar wrapper in each card stays separate (different layout slot, still intentionally its own trigger). hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements actor hover cards and profile previews for mentions in the web-next application, introducing an actorByUrl GraphQL query and a backend lookup utility for local and federated actor resolution. Frontend changes include the ActorHoverCard component and a useMentionHoverCards hook to handle hover interactions on mention links. Feedback focuses on optimizing reactivity by batching signal updates in the hover card logic and improving the type safety of timer variables for cross-platform compatibility.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7648b979a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`setTimeout` returns `number` in browser libs and `Timeout` in Node libs. The hook had its timer handles typed as `number`, which works fine in deno check today but locks us out of contexts where Node typings dominate. `ReturnType<typeof setTimeout>` resolves to the right shape in either environment. hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
The open path sets anchor, lookup, and open in three sequential signal writes; the cleanup path unwinds them the same way. Each write currently triggers an independent reactive cycle, so downstream Popover/Loader components can briefly see a transitional combination (e.g., a new anchor paired with stale lookup data) before the next write lands. Wrap both transitions in `batch()` so all three signals settle in a single update cycle. hackers-pub#276 (comment) hackers-pub#276 (comment) hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7
Federated HTML can carry protocol-relative anchor hrefs (`<a href="//host/@user">`). The `models/html.ts` link extractor already handles them by treating the leading `//` as `https://` before parsing; do the same in the mention hover-card resolver so those mentions don't silently fall through to "no hover card". hackers-pub#276 (comment) Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements actor hover cards across the web-next interface, enabling profile previews when hovering over avatars, handles, or mentions. Key additions include a new actorByUrl GraphQL query, a backend lookup utility that resolves actors via local database or federation, and several frontend components such as ActorHoverCard and ActorPreviewCard. The PR also introduces useMentionHoverCards to dynamically attach hover behavior to mentions within rendered content. I have no feedback to provide.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-next/src/components/ActorPreviewCard.tsx (1)
52-69: ⚡ Quick winSimplify pluralized strings to idiomatic Lingui v5
ttag.The
i18n._(msg\${plural(...)}`)pattern is a pre-v5 workaround. In Lingui v5 with theuseLingui()macro,t(already destructured on line 19) is the idiomatic, context-bound equivalent. Usingplural()` standalone inside a component also compiles to a call on the global i18n instance rather than the one from context, which can cause problems in SSR environments.♻️ Proposed refactor
- const followingText = () => - i18n._( - msg`${ - plural(a().followeesCount.totalCount, { - one: "# following", - other: "# following", - }) - }`, - ); - const followersText = () => - i18n._( - msg`${ - plural(a().followersCount.totalCount, { - one: "# follower", - other: "# followers", - }) - }`, - ); + const followingText = () => + t`${plural(a().followeesCount.totalCount, { + one: "# following", + other: "# following", + })}`; + const followersText = () => + t`${plural(a().followersCount.totalCount, { + one: "# follower", + other: "# followers", + })}`;Based on Lingui v5 docs,
useLingui()macro from@lingui/react/macroexposes attag that compiles toi18n._()bound to the context instance, makingi18n._(msg\...`)redundant. Usingplural()standalone while relying onuseLingui()` can cause the plural to resolve against the global i18n instance rather than the context-bound one, which may produce incorrect output in server-side rendering scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-next/src/components/ActorPreviewCard.tsx` around lines 52 - 69, The pluralized text helpers followingText and followersText use i18n._(msg`...`) with plural(...) which is a pre-v5 pattern; replace them to use the context-bound t tag (destructured from useLingui) and inline pluralization so the plural resolves against the context i18n instance—i.e., convert i18n._(msg`...${plural(...)}`) to t`...${plural(...)}` within followingText and followersText so SSR and context-bound localization work correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-next/src/components/ActorPreviewCard.tsx`:
- Around line 97-102: The ActorPreviewCard uses innerHTML={a().name ?? ""} which
is an XSS sink; locate the GraphQL resolver that populates the Actor.name field
and either ensure server-side sanitization (strip/escape HTML or run a robust
sanitizer like DOMPurify/xss) before returning name, or change the component to
render the name as plain text instead of innerHTML (update InternalLink usage
around a().name), keeping bio as the only explicitly-HTML field; reference the
InternalLink component, the a() accessor, and the Actor.name resolver when
making the fix and add tests verifying HTML tags/scripts in remote names are
neutralized.
---
Nitpick comments:
In `@web-next/src/components/ActorPreviewCard.tsx`:
- Around line 52-69: The pluralized text helpers followingText and followersText
use i18n._(msg`...`) with plural(...) which is a pre-v5 pattern; replace them to
use the context-bound t tag (destructured from useLingui) and inline
pluralization so the plural resolves against the context i18n instance—i.e.,
convert i18n._(msg`...${plural(...)}`) to t`...${plural(...)}` within
followingText and followersText so SSR and context-bound localization work
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8668a6b0-ed10-4492-b9ca-b8e34e0d67c5
📒 Files selected for processing (7)
web-next/src/components/ActorPreviewCard.tsxweb-next/src/components/ArticleCard.tsxweb-next/src/components/NoteHeader.tsxweb-next/src/components/QuestionCard.tsxweb-next/src/components/QuotedNoteCard.tsxweb-next/src/components/SmallProfileCard.tsxweb-next/src/lib/mentionHoverCards.tsx
✅ Files skipped from review due to trivial changes (2)
- web-next/src/components/QuestionCard.tsx
- web-next/src/lib/mentionHoverCards.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web-next/src/components/SmallProfileCard.tsx
- web-next/src/components/QuotedNoteCard.tsx
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Hovering an avatar, display name, or
@mentionnow opens a profile preview with avatar, name, handle, follow button, bio, and follow counts. This lets readers check a profile from the timeline without navigating away.Closes #90.
Implementation notes
This uses two primitives because Kobalte's
HoverCardroot does not exposeanchorRef, which mention links rendered throughinnerHTMLneed:@kobalte/core/hover-card.PopoverwithanchorRefdriven by delegatedpointerover/pointerouton the prose container. The hook lives at web-next/src/lib/mentionHoverCards.tsx.Both paths share the card body (web-next/src/components/ActorPreviewCard.tsx) and lazy actor loader (web-next/src/components/ActorHoverCardLoader.tsx), which uses the existing
loadQuery+createPreloadedQuerypattern. Loading shows a skeleton. Failed queries and unknown actors fall back to "Could not load profile."The mention hook handles both kinds of mention markup we render:
<a class="mention" data-username data-host …>(see models/markup.ts).<span class="h-card"><a class="u-url mention" href="…">@user</a></span>. These links have no data attrs, so the hook derives host fromURL(href).hostand username from the link text.Surfaces wired up
Avatars and display names:
PostAvatar,NoteHeader,ArticleCard(card and detail-page header at web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx),QuotedNoteCard,SmallProfileCard,NotificationActor,NotificationMessage(avatar stack),PostSharer,QuestionCard.Mentions inside prose:
NoteCardInternal,QuotedNoteCard,ProfileCardbio,QuestionCardContent,ArticleCard, article detail body, andSmallProfileCardbio.Internationalization
Added one string, “Could not load profile”, with
ja-JP,ko-KR,zh-CN, andzh-TWtranslations following each locale's glossary.Known limitations
pointerType !== "touch"; Kobalte'sHoverCardalready does the same internally.ActorHoverCardwrapper is non-focusable so it doesn't add a phantom tab stop, and the mention layer has noPopover.Trigger. Worth revisiting if focus-driven previews become important.Test plan
@mentioninside a note's prose → same card.FollowButtonupdates from the shared Relay store, no flicker.isViewerbehavior).?lang=koand verify the Korean translation appears.