feat: add ally system, user profile popover, and reaction tooltips#22
Merged
BuckyMcYolo merged 6 commits intomainfrom Mar 22, 2026
Merged
feat: add ally system, user profile popover, and reaction tooltips#22BuckyMcYolo merged 6 commits intomainfrom
BuckyMcYolo merged 6 commits intomainfrom
Conversation
- Ally (friend) system: schema, API endpoints (send/accept/decline requests, list allies, remove), and allies page UI with tabs - User profile popover on avatar/name click in messages and member list showing bio, status, presence, and ally actions - Reaction tooltips showing who reacted (with "You" for current user) including real-time reactor list updates
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an allies/friend system (DB schema, API routes, handlers, OpenAPI schemas), user profile endpoint with presence and ally status, DM creation with ally checks and reuse, reaction reactor tracking end-to-end (types → queries → realtime → web), UI for allies/DM creation and user-profile popovers, and mounts new v1 routers into the app. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (web)
participant Realtime as Realtime Service
participant API as API Server
participant DB as Database
participant Redis as Redis
Client->>Realtime: emit "message:reaction:toggle" (messageId, emoji, userId, userName)
Realtime->>API: call toggleMessageReaction(input including userName)
API->>DB: query/update reactions, join user for reactor names
DB-->>API: reaction rows (including reactor user names)
API->>Realtime: update payload (emoji, count, actorUserId, actorName, reactors)
Realtime-->>Client: broadcast "message:reaction:updated" (payload)
Client->>Client: applyReactionUpdateToMessage (updates reactors array and UI)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/v1/channels/handlers.ts (1)
421-441: 🧹 Nitpick | 🔵 TrivialConsider extracting shared reaction aggregation logic.
The reaction aggregation logic (type definition, Map initialization, and population) is duplicated between
listPinnedMessagesandfetchMessagePageinapps/api/src/lib/queries/messages.ts. Consider extracting a shared helper to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/channels/handlers.ts` around lines 421 - 441, The reaction aggregation logic (building reactionsByMessageId Map from reactionRows) is duplicated in listPinnedMessages and fetchMessagePage; extract this into a shared helper function (e.g., buildReactionsByMessageId or aggregateReactions) that accepts reactionRows and returns Map<string, Map<string, {emoji:string,count:number,reactedByCurrentUser:boolean,reactors:Array<{id:string,name:string}>}>>; replace the duplicated blocks in listPinnedMessages and fetchMessagePage to call this helper and ensure the helper handles Map initialization, incrementing count, and pushing reactors for each row (using the same row fields: messageId, emoji, userId, userName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app.ts`:
- Around line 40-46: The API has new route mounts in app.ts (.route("/v1",
alliesRouter), channelsRouter, guildsRouter, invitesRouter, dmsRouter,
uploadsRouter, usersRouter) but the autogenerated frontend API client was not
rebuilt; regenerate the API client artifacts so the frontend/type definitions
reflect these new routes. Run the project’s API client generation/build script
(e.g., the repo’s api-client build or generate command) after pulling the
changes so the updated OpenAPI/TS types are emitted, then commit the regenerated
client artifacts.
In `@apps/api/src/routes/v1/allies/handlers.ts`:
- Around line 252-257: When receiverMap.get(row.receiverId) returns undefined
you currently silently continue; update the handler to log a warning before the
continue so missing receivers are visible in logs: emit a warn including
row.receiverId (and row.id or other identifying fields) plus currentUser.id to
provide context. Use the module's existing logger (e.g., logger or
processLogger) and keep the current flow (log then continue); touch the receiver
lookup block referencing receiverMap, row.receiverId, currentUser and
currentUserInfo.
- Around line 133-137: The delete-then-insert sequence for replacing a declined
request (using db.delete(schema.allyRequest).where(eq(schema.allyRequest.id,
existing.id)) followed by the subsequent db.insert(schema.allyRequest) call) is
vulnerable to a race that can produce duplicate/unique-constraint errors; make
the operation atomic by wrapping the delete+insert in a single transaction or by
switching to an upsert pattern (e.g., transactional db.transaction(...) that
performs the delete and insert within the same TX or using
db.insert(...).onConflict(...).merge()/upsert equivalent) so the replacement
cannot be interleaved by concurrent requests.
In `@apps/api/src/routes/v1/allies/routes.ts`:
- Around line 24-49: The route sendAllyRequest currently declares a successful
response under HttpStatusCodes.OK; change the route's responses entry to use
HttpStatusCodes.CREATED for the success response (keep using
sendAllyRequestResponseSchema and the existing description), and update the
route handler that sends the response to return HttpStatusCodes.CREATED instead
of HttpStatusCodes.OK so the endpoint correctly signals resource creation.
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 349-356: The current code uses removeAlly.isPending which disables
every AllyRow; instead track the specific ally being removed by adding a local
piece of state (e.g., removingAllyId) and set it when initiating
removeAlly.mutate and clear it in the mutation's onSuccess/onError handlers.
Update the AllyRow prop to isRemoving={removingAllyId === ally.id} and change
the onRemove call to first set removingAllyId = ally.id before calling
removeAlly.mutate, then clear removingAllyId in the mutation callbacks (or use
the mutation's lifecycle hooks) so only the targeted row's button is disabled.
- Around line 180-183: The invalidate() helper only clears ["allies"] and
["ally-requests"], leaving cached entries used by UserProfileCard (keyed as
["user-profile", userId]) stale; update invalidate() to also invalidate
user-profile entries (either by invalidating the root ["user-profile"] namespace
or by calling queryClient.invalidateQueries({ queryKey: ["user-profile", userId]
}) when you know the affected userId) and update the mutation callbacks in
allies-page (accept/decline/remove handlers) to pass the relevant userId into
invalidate so the profile popover reflects the new allyStatus immediately.
In `@apps/web/src/components/chat/message-item.tsx`:
- Around line 291-298: In MessageItem
(apps/web/src/components/chat/message-item.tsx) avoid calling .map on a possibly
undefined reaction.reactors by using a defensive fallback (e.g., const reactors
= reaction.reactors ?? []), then derive names from reactors and build
tooltipText from that safe array; update any uses of reaction.reactors.map(...)
to use the fallback variable so hydrated REST messages won't throw at runtime.
In `@apps/web/src/hooks/use-message-reactions.ts`:
- Around line 49-52: The currentUser construction uses truthy checks so an empty
string name yields undefined; change the condition in use-message-reactions.ts
where currentUser is built to use nullish checks (explicit null/undefined) for
currentUserId and currentUserName (e.g., ensure both are not null/undefined via
!= null or !== undefined checks) and only then set currentUser = { id:
currentUserId, name: currentUserName }; otherwise keep it undefined so
blank-but-valid names still preserve optimistic reaction identity.
In `@apps/web/src/lib/realtime-adapter.ts`:
- Around line 148-150: The current spread of currentReaction.reactors can throw
if reactors is undefined; update the reactors assignment in realtime-adapter.ts
so you spread a safe default empty array when reading currentReaction.reactors
(e.g., use a nullish-coalescing or logical-or fallback to an empty array) and
keep the conditional that appends currentUser only when present; target the
expression building the reactors array around currentReaction and currentUser to
implement this safe fallback.
- Around line 138-140: currentReaction.reactors may be undefined when messages
are hydrated from the REST API, causing a TypeError when calling .filter();
update the expression in realtime-adapter.ts to defensively handle undefined by
using a fallback (e.g. (currentReaction.reactors ?? []) ) before filtering so
when currentUser is present use (currentReaction.reactors ?? []).filter(r =>
r.id !== currentUser.id) and when not present use currentReaction.reactors ??
[]; keep references to reactors, currentReaction, and currentUser when making
the change.
---
Outside diff comments:
In `@apps/api/src/routes/v1/channels/handlers.ts`:
- Around line 421-441: The reaction aggregation logic (building
reactionsByMessageId Map from reactionRows) is duplicated in listPinnedMessages
and fetchMessagePage; extract this into a shared helper function (e.g.,
buildReactionsByMessageId or aggregateReactions) that accepts reactionRows and
returns Map<string, Map<string,
{emoji:string,count:number,reactedByCurrentUser:boolean,reactors:Array<{id:string,name:string}>}>>;
replace the duplicated blocks in listPinnedMessages and fetchMessagePage to call
this helper and ensure the helper handles Map initialization, incrementing
count, and pushing reactors for each row (using the same row fields: messageId,
emoji, userId, userName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b5481fe-e7c8-42c7-a131-b036bbd59fe5
📒 Files selected for processing (28)
apps/api/src/app.tsapps/api/src/lib/helpers/openapi/message-schemas.tsapps/api/src/lib/helpers/openapi/schemas.tsapps/api/src/lib/queries/messages.tsapps/api/src/routes/v1/allies/handlers.tsapps/api/src/routes/v1/allies/index.tsapps/api/src/routes/v1/allies/routes.tsapps/api/src/routes/v1/allies/schema.tsapps/api/src/routes/v1/channels/handlers.tsapps/api/src/routes/v1/users/handlers.tsapps/api/src/routes/v1/users/index.tsapps/api/src/routes/v1/users/routes.tsapps/api/src/routes/v1/users/schema.tsapps/realtime/src/index.tsapps/realtime/src/services/messages.tsapps/web/src/components/allies/allies-page.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/components/sidebar/right-panel/guild-members-panel.tsxapps/web/src/components/ui/user-profile-card.tsxapps/web/src/hooks/use-message-reactions.tsapps/web/src/lib/api-types.tsapps/web/src/lib/realtime-adapter.tsapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxapps/web/src/routes/_authenticated/dms/index.tsxpackages/db/src/schemas/ally-requests.tspackages/db/src/schemas/index.tspackages/realtime-types/src/events.ts
Owner
Author
|
@CodeRabbit we do not commit migration files currently, do not raise an issue for this, we just use db:push locally now |
|
Tip For best results, initiate chat on the files or code changes.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/web/src/components/allies/allies-page.tsx (1)
182-190:⚠️ Potential issue | 🟡 MinorProfile caches still stay stale after non-remove mutations.
apps/web/src/components/ui/user-profile-card.tsx:81-98caches profiles under["user-profile", userId]. This helper can invalidate one of those keys, but only the remove path uses it. Sending, accepting, or declining a request can still leave an already-cached popover on the oldallyStatus.♻️ Minimal fix
- const invalidate = (affectedUserId?: string) => { + const invalidate = () => { void queryClient.invalidateQueries({ queryKey: ["allies"] }) void queryClient.invalidateQueries({ queryKey: ["ally-requests"] }) - if (affectedUserId) { - void queryClient.invalidateQueries({ - queryKey: ["user-profile", affectedUserId], - }) - } + void queryClient.invalidateQueries({ queryKey: ["user-profile"] }) } @@ - onSuccess: (userId) => { + onSuccess: () => { setRemovingAllyId(null) - invalidate(userId) + invalidate() toast.success("Ally removed") },Also applies to: 205-245, 261-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/allies/allies-page.tsx` around lines 182 - 190, The invalidate helper in allies-page.tsx only clears the ["user-profile", userId] cache when called, but several non-remove mutations (send/accept/decline ally request) do not call it so cached user-profile popovers show stale allyStatus; update the onSuccess handlers of the mutation functions (the send/accept/decline mutations in allies-page.tsx) to call invalidate with the affected userId (so it triggers queryClient.invalidateQueries({ queryKey: ["user-profile", affectedUserId] })) after each successful mutation, ensuring the cached key used in user-profile-card.tsx (["user-profile", userId]) is refreshed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/allies/handlers.ts`:
- Around line 346-357: The UPDATE currently only filters by id after a separate
pending check, allowing races; change the db.update call that sets
status="accepted" (the block that calls
db.update(schema.allyRequest).set({...}).where(eq(schema.allyRequest.id,
requestId)).returning()) to include both eq(schema.allyRequest.receiverId,
currentUser.id) and eq(schema.allyRequest.status, "pending") in the WHERE
predicate so the transition is conditional, then check the returned updated
count (or array length) and treat 0 updated rows as a stale/forbidden result
(return a 403 or appropriate error). Apply the identical predicate+0-updates
check to the decline handler that updates status to "declined" so both paths are
safe from races.
- Around line 97-118: The check-then-insert is racy because uniqueness is only
on (senderId, receiverId); change to canonicalize the unordered pair (e.g.,
compute userAId = min(currentUser.id, targetUserId), userBId = max(...)) and add
a DB unique constraint on (userAId, userBId) in the allyRequest table, then
replace the select-then-insert flow with a conflict-safe insert: insert the
request using db.insert(schema.allyRequest).values(...) with the canonical pair
and use ON CONFLICT (userAId, userBId) DO NOTHING (or DO UPDATE to set status)
and RETURNING to get the existing/created row so you don't need the separate
`existing` select; apply the same canonicalization + conflict-safe insert fix to
the other send/accept paths referenced (the other allyRequest insert sites
around the noted ranges).
- Around line 4-11: The import of route types currently uses a relative path
("./routes"); update it to use the project path alias used in apps/api so
tsc-alias can rewrite during build (replace the "./routes" import with the alias
import path for the same module, e.g., import the types from
"@/routes/v1/allies/routes"). Ensure the same exported type names
(AcceptAllyRequestRoute, DeclineAllyRequestRoute, ListAlliesRoute,
ListAllyRequestsRoute, RemoveAllyRoute, SendAllyRequestRoute) are preserved and
that the change is made in handlers.ts where those types are currently imported.
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 62-79: The remove control is hidden by default using "opacity-0
group-hover:opacity-100", which hides it from touch and keyboard users; update
the action container in allies-page.tsx (the div wrapping the Buttons) to also
reveal on keyboard focus and non-hover layouts by adding focus/focus-within and
non-hover breakpoint classes (e.g., include group-focus-within:opacity-100 and a
responsive override like sm:opacity-100 or focus-visible variants) so the Remove
button (onClick => onRemove(ally.id)) becomes visible to keyboard and touch
users while preserving hover behavior.
- Around line 162-178: The useQuery calls for allies and ally-requests currently
only destructure data and isPending, so fetch failures fall back to empty arrays
and show empty-state UI; update both useQuery hooks (the ones assigning
allies/alliesLoading and requests/requestsLoading) to also destructure isError
and/or error, then add early error UI branches that render when isError or error
is present (check alliesLoading/requestsLoading first), placing those error
branches before the existing empty-state checks so failed fetches show an error
message instead of the "no data" state.
In `@apps/web/src/hooks/use-message-reactions.ts`:
- Around line 54-61: The currentUser object is recreated each render causing
toggleReactionLocal (defined with useCallback) to be recreated; memoize
currentUser with useMemo so its reference only changes when currentUserId or
currentUserName change, then keep toggleReactionLocal’s dependency array as
[updateMessageInCache, currentUser]; update the code where currentUser is
constructed (the object created on lines around its declaration) to use useMemo
and reference currentUserId/currentUserName so
toggleReactionOptimistically(message, emoji, currentUser) receives a stable
object and toggleReactionLocal stops being unnecessarily recreated.
---
Duplicate comments:
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 182-190: The invalidate helper in allies-page.tsx only clears the
["user-profile", userId] cache when called, but several non-remove mutations
(send/accept/decline ally request) do not call it so cached user-profile
popovers show stale allyStatus; update the onSuccess handlers of the mutation
functions (the send/accept/decline mutations in allies-page.tsx) to call
invalidate with the affected userId (so it triggers
queryClient.invalidateQueries({ queryKey: ["user-profile", affectedUserId] }))
after each successful mutation, ensuring the cached key used in
user-profile-card.tsx (["user-profile", userId]) is refreshed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff8e099f-9138-4db7-b2bc-84537c37bcf4
📒 Files selected for processing (6)
ROADMAP.mdapps/api/src/routes/v1/allies/handlers.tsapps/web/src/components/allies/allies-page.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/hooks/use-message-reactions.tsapps/web/src/lib/realtime-adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/web/src/components/allies/allies-page.tsx (1)
65-85:⚠️ Potential issue | 🟠 MajorDon't keep the ally actions hover-only.
This still hides the only DM/remove controls from touch users, and keyboard users can tab into an invisible action group. Make the buttons visible on non-hover layouts and when the row receives focus.
♿ Suggested fix
- <div className="flex items-center gap-1 opacity-0 group-hover:opacity-100"> + <div className="flex items-center gap-1 opacity-100 md:opacity-0 md:group-hover:opacity-100 md:group-focus-within:opacity-100">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/allies/allies-page.tsx` around lines 65 - 85, The action buttons are hidden by default using "opacity-0 group-hover:opacity-100", which makes them inaccessible to touch and keyboard users; update the wrapper div's classes so the controls are visible when the row receives focus or for non-hover/touch layouts (e.g., replace "opacity-0 group-hover:opacity-100" with classes that include focus/focus-within and non-hover visibility such as "opacity-100 group-hover:opacity-100 group-focus-within:opacity-100 focus-within:opacity-100" or otherwise remove the initial opacity-0), keeping the onClick handlers (onMessage/onRemove), the disabled prop (isRemoving) and icons (MessageCircle/UserMinus) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 114-189: Wrap the whole 1:1 DM lookup+create in a single database
transaction and use a DB-backed uniqueness key for direct-DMs to avoid race
conditions: generate a canonical identifier for the direct pair (e.g., sorted
user IDs joined, stored as dm_key on channel) and add a UNIQUE constraint on
(type='dm', dm_key) in the schema; inside a transaction (using db.transaction or
your DB client), first try to select an existing DM by dm_key, if none attempt
to insert the channel with dm_key and then insert channelMember rows in the same
transaction; on unique-constraint violation (insert conflict) catch the error,
re-select the existing channel within the transaction and return it, and ensure
any failure rolls back so no orphan channel remains. Use the existing symbols
channel, channelMember, allMemberIds, isDirect, db.insert/db.select, and
returnDMResponse when implementing this.
- Around line 13-18: The import statement at the top of handlers.ts uses a
relative path to "./routes" which violates the apps/api guideline to use the `@/`
path alias; replace the current import with an alias import that points to the
same module (e.g., import the types CreateDMRoute, GetDMRoute,
ListDMMessagesRoute, ListDMsRoute from "@/routes") so tsc-alias can rewrite
paths during build and keep apps/api compatible with the build pipeline.
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 192-195: The single-ID state removingAllyId is race-prone; change
it to track multiple in-flight removals (e.g., useState<Set<string>> or
useRef<Set<string>>) and update all places that read/write removingAllyId to add
the target ally id before calling the delete, and remove it after the request
settles (success or error). Concretely, replace removingAllyId /
setRemovingAllyId usage with removingAllyIds and
addRemovingAllyId/removeRemovingAllyId helpers, update any button
disabled/loader checks from removingAllyId === id to removingAllyIds.has(id),
and apply the same pattern in the other remove handlers referenced (the blocks
using removingAllyId at lines noted) so concurrent deletes are tracked per-id
rather than clobbered by a single scalar. Ensure cleanup in finally so state is
cleared for that id regardless of outcome.
In `@apps/web/src/components/sidebar/dm-panel/dm-panel.tsx`:
- Around line 50-56: The icon-only button that opens the new DM modal lacks an
accessible name; update the button (the element using onClick={() =>
setNewDMOpen(true)} and rendering the Plus icon) to provide an accessible label
(for example add an aria-label like "New direct message" or include visually
hidden text inside the button) so screen readers can announce its purpose while
preserving the current visual appearance and behavior.
In `@apps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsx`:
- Around line 32-40: The query result is collapsing fetch failures into the
empty state; update the useQuery usage for allies (queryKey ["allies"], queryFn
using apiClient.v1.allies.$get) to surface errors by reading the query's
error/isError (or status) and render an explicit error branch when the request
fails instead of showing the "You don't have any allies yet." empty message;
ensure the component checks isPending, then isError/error and displays a failure
message (or error details) before falling back to the empty allies rendering.
---
Duplicate comments:
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 65-85: The action buttons are hidden by default using "opacity-0
group-hover:opacity-100", which makes them inaccessible to touch and keyboard
users; update the wrapper div's classes so the controls are visible when the row
receives focus or for non-hover/touch layouts (e.g., replace "opacity-0
group-hover:opacity-100" with classes that include focus/focus-within and
non-hover visibility such as "opacity-100 group-hover:opacity-100
group-focus-within:opacity-100 focus-within:opacity-100" or otherwise remove the
initial opacity-0), keeping the onClick handlers (onMessage/onRemove), the
disabled prop (isRemoving) and icons (MessageCircle/UserMinus) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54106d8d-6210-4c0c-9ab2-75c8966bbca1
📒 Files selected for processing (10)
apps/api/src/routes/v1/allies/handlers.tsapps/api/src/routes/v1/dms/handlers.tsapps/api/src/routes/v1/dms/index.tsapps/api/src/routes/v1/dms/routes.tsapps/api/src/routes/v1/dms/schema.tsapps/web/src/components/allies/allies-page.tsxapps/web/src/components/sidebar/dm-panel/dm-panel.tsxapps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsxapps/web/src/hooks/use-create-dm.tsapps/web/src/hooks/use-message-reactions.ts
Comment on lines
+13
to
+18
| import type { | ||
| CreateDMRoute, | ||
| GetDMRoute, | ||
| ListDMMessagesRoute, | ||
| ListDMsRoute, | ||
| } from "./routes" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the API path alias for these route types.
Keep apps/api imports on @/... so this package continues to build through tsc && tsc-alias.
♻️ Suggested fix
-} from "./routes"
+} from "@/routes/v1/dms/routes"As per coding guidelines, "Use @/* path alias in apps/api for imports (e.g., import foo from "@/lib/foo"). tsc-alias will rewrite these to relative .js paths in the compiled output."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/dms/handlers.ts` around lines 13 - 18, The import
statement at the top of handlers.ts uses a relative path to "./routes" which
violates the apps/api guideline to use the `@/` path alias; replace the current
import with an alias import that points to the same module (e.g., import the
types CreateDMRoute, GetDMRoute, ListDMMessagesRoute, ListDMsRoute from
"@/routes") so tsc-alias can rewrite paths during build and keep apps/api
compatible with the build pipeline.
Comment on lines
+192
to
+195
| const [removingAllyId, setRemovingAllyId] = useState<string | null>(null) | ||
|
|
||
| const invalidate = (affectedUserId?: string) => { | ||
| void queryClient.invalidateQueries({ queryKey: ["allies"] }) |
There was a problem hiding this comment.
removingAllyId is still race-prone when deletes overlap.
Because other rows stay clickable, a second remove can start before the first one settles. With only one tracked ID, the later click overwrites the earlier loading state, and whichever request finishes first clears the indicator for any remaining delete. Track a set of in-flight ally IDs, or intentionally serialize removals.
🛠️ Suggested fix
- const [removingAllyId, setRemovingAllyId] = useState<string | null>(null)
+ const [removingAllyIds, setRemovingAllyIds] = useState<Set<string>>(
+ new Set()
+ )
...
const removeAlly = useMutation({
mutationFn: async (userId: string) => {
- setRemovingAllyId(userId)
const res = await apiClient.v1.allies[":userId"].$delete({
param: { userId },
})
if (!res.ok) throw new Error("Failed to remove ally")
return userId
},
+ onMutate: (userId) => {
+ setRemovingAllyIds((prev) => {
+ const next = new Set(prev)
+ next.add(userId)
+ return next
+ })
+ },
onSuccess: (userId) => {
- setRemovingAllyId(null)
invalidate(userId)
toast.success("Ally removed")
},
onError: () => {
- setRemovingAllyId(null)
toast.error("Failed to remove ally")
},
+ onSettled: (_data, _error, userId) => {
+ if (!userId) return
+ setRemovingAllyIds((prev) => {
+ const next = new Set(prev)
+ next.delete(userId)
+ return next
+ })
+ },
})
...
- isRemoving={removingAllyId === ally.id}
+ isRemoving={removingAllyIds.has(ally.id)}Also applies to: 277-295, 389-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/allies/allies-page.tsx` around lines 192 - 195, The
single-ID state removingAllyId is race-prone; change it to track multiple
in-flight removals (e.g., useState<Set<string>> or useRef<Set<string>>) and
update all places that read/write removingAllyId to add the target ally id
before calling the delete, and remove it after the request settles (success or
error). Concretely, replace removingAllyId / setRemovingAllyId usage with
removingAllyIds and addRemovingAllyId/removeRemovingAllyId helpers, update any
button disabled/loader checks from removingAllyId === id to
removingAllyIds.has(id), and apply the same pattern in the other remove handlers
referenced (the blocks using removingAllyId at lines noted) so concurrent
deletes are tracked per-id rather than clobbered by a single scalar. Ensure
cleanup in finally so state is cleared for that id regardless of outcome.
channel memory - Wrap 1:1 DM lookup+create in a transaction to prevent duplicate DMs - Add confirmation dialog before removing an ally - Remember last visited channel per guild in localStorage and auto-navigate - Remove unused Message Requests button from DM sidebar - Add error states and accessibility improvements
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 142-170: The DM creation transaction (db.transaction around
newChannelId with isDirect check and the candidate query using
channelMember/channel) can race under READ COMMITTED; update the candidate
lookup to acquire row-level locks (use FOR UPDATE / forUpdate on the tx query
that selects candidate.channelId) so concurrent transactions serialize, add a
canonical DM uniqueness constraint in your channels schema (e.g., a dm_key
column derived from sorted member IDs) and update the insert path to catch and
handle unique-violation errors (return the existing channelId when constraint
violated), and/or run this transaction at SERIALIZABLE isolation as a fallback;
apply the locking change to the tx.select(...).innerJoin(...) query and add
handling around the insert that creates the channel inside the same
db.transaction where newChannelId is produced.
In `@apps/web/src/components/allies/allies-page.tsx`:
- Around line 475-477: The current buttons use a shared pending flag
(isPending={acceptRequest.isPending || declineRequest.isPending}) which disables
all incoming request controls; change this to a per-request pending state by
tracking the active request id (e.g., local state like activeRequestId or a map)
and set isPending on each Accept/Decline button based on whether that button's
request id matches the pending id or has pending[id] true; update the
acceptRequest/declineRequest invocation handlers (the functions referenced as
acceptRequest and declineRequest) to set and clear the per-request pending state
around the async call so only the specific request's buttons are disabled.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 42-44: The current useEffect writes last-channel immediately and
unguarded, which can persist invalid channel IDs and throw in restricted
environments; update the useEffect so it only runs after the channel is
resolved/validated (e.g., wait for a truthy channel object or isChannelLoaded
flag) and ensure guildSlug and channelId are non-empty, then wrap the storage
call in a try/catch and verify localStorage availability before calling
localStorage.setItem (use symbols: useEffect, guildSlug, channelId,
localStorage.setItem).
In `@apps/web/src/routes/_authenticated/`$guildSlug/index.tsx:
- Around line 12-21: The effect in useEffect reads localStorage.getItem directly
which can throw in restricted environments; wrap the storage access for the key
`last-channel:${guildSlug}` in a safe guard (e.g., check typeof window !==
"undefined" and try/catch around localStorage.getItem) and fall back to null if
unavailable, then only call navigate when lastChannelId is non-null; update the
code around useEffect / localStorage.getItem to perform this guarded read so
navigate is never invoked due to an exception from storage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 390990a9-f587-47c6-babf-9b45a73c6c36
📒 Files selected for processing (6)
apps/api/src/routes/v1/dms/handlers.tsapps/web/src/components/allies/allies-page.tsxapps/web/src/components/sidebar/dm-panel/dm-panel.tsxapps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsxapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/$guildSlug/index.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/src/routes/_authenticated/$guildSlug/$channelId.tsx (1)
42-51:⚠️ Potential issue | 🟡 MinorPersisting before channel validation still not addressed
The try/catch and window guard are good, but the effect still writes to localStorage before verifying the channel exists (via the
dataquery result). If a user navigates to an invalid channel URL, the invalid ID gets cached, causing redirect loops from the guild home page.Adding a
datadependency and early return would prevent caching invalid channel IDs.Proposed fix
useEffect(() => { - if (!guildSlug || !channelId) return + if (!guildSlug || !channelId || !data) return try { if (typeof window !== "undefined") { localStorage.setItem(`last-channel:${guildSlug}`, channelId) } } catch { // localStorage may be unavailable in restricted environments } - }, [guildSlug, channelId]) + }, [guildSlug, channelId, data])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx around lines 42 - 51, The effect is persisting channelId to localStorage before the query result is validated; update the useEffect that references guildSlug and channelId so it also depends on the query result (data) and returns early unless the channel is confirmed valid (e.g., data shows the channel exists for the guild). Specifically, in the useEffect containing localStorage.setItem(`last-channel:${guildSlug}`, channelId) add data to the dependency array and add an early guard like if (!data || !channelExistsIn(data, channelId)) return so you only cache known-good channel IDs and avoid redirect loops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 42-51: The effect is persisting channelId to localStorage before
the query result is validated; update the useEffect that references guildSlug
and channelId so it also depends on the query result (data) and returns early
unless the channel is confirmed valid (e.g., data shows the channel exists for
the guild). Specifically, in the useEffect containing
localStorage.setItem(`last-channel:${guildSlug}`, channelId) add data to the
dependency array and add an early guard like if (!data || !channelExistsIn(data,
channelId)) return so you only cache known-good channel IDs and avoid redirect
loops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cd4747b-55d4-4fbf-a405-e9e99a989463
📒 Files selected for processing (2)
apps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/$guildSlug/index.tsx
This was referenced Mar 22, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Summary: Ally System, User Profile Popover, Reaction Tooltips, and DM improvements
Overview
This PR implements an ally (friend) system (DB schema + API + UI), a user profile popover with ally actions, reaction tooltips that show who reacted, and DM creation UI/APIs. Changes span database schemas, API routes, realtime events, frontend components, hooks, and types.
Backend Changes
Database Schema
ally_requestDrizzle schema (packages/db/src/schemas/ally-requests.ts):ally_request_status(pending, accepted, declined)ally_request(id, createdAt, updatedAt, senderId → user.id, receiverId → user.id, status)userNote about migrations: generated Drizzle migration artifacts are not present in the repo. The PR author explicitly requested that migration files not be committed (the project uses db:push locally / CI), and an automated bot in the thread acknowledged that policy.
Allies API Routes (
/v1/allies)New route tree providing:
All routes use sessionAuthMiddleware and Zod/OpenAPI schemas with appropriate response mappings.
Users API
none,pending_incoming,pending_outgoing,allies), and allyRequestId when applicable.DMs API
Reaction & Realtime
reactorsper emojiFrontend Changes
UI Components
Hooks & Client Logic
reactorsarrays; toggleReactionOptimistically updated to accept optional currentUserNotable Files Touched
Issues & Recommendations
Migrations / Deployment
Tests / Coverage
Concurrency & Edge Cases
UX Change Confirmation
Positive Notes
Confidence Score: 4/5
Rationale: Implementation is well-structured and consistent across stacks, with clear schemas and OpenAPI contracts. The only significant operational caveat is the absence of committed migration artifacts — which appears to be an intentional repo policy (db:push) per author/bot discussion — plus a lack of tests. If CI/CD applies schema changes and tests are added, this PR is production-ready with minor nits only.