feat: add user blocking system, username editing, and mention display#24
Merged
BuckyMcYolo merged 5 commits intomainfrom Mar 22, 2026
Merged
feat: add user blocking system, username editing, and mention display#24BuckyMcYolo merged 5 commits intomainfrom
BuckyMcYolo merged 5 commits intomainfrom
Conversation
fix - User blocking: schema, API (block/unblock/list), realtime DM enforcement, blocked tab on allies page, block/unblock in profile popover, message collapse with click-to-reveal, typing indicator filtering, DM input disabled when blocked, blocked users hidden from new DM dialog - Username editing in account settings with availability check - Fix mention suggestion list to show displayUsername instead of username
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds user blocking and privacy settings across DB, API, realtime, and frontend: new schemas, routes/handlers, privacy endpoints, allies/DM gating, realtime enforcement for messages/typing/presence, frontend hooks/UI for blocking, and username-editing UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant WebApp as Web App
participant API as API Server
participant DB as Database
participant Realtime as Realtime Server
User->>WebApp: Click "Block" on profile
WebApp->>API: POST /v1/blocks { userId }
API->>DB: INSERT INTO user_block (blocker_id, blocked_id) (ignore conflict)
DB-->>API: Insert result
API->>DB: DELETE FROM ally_request WHERE (sender/receiver in pair)
DB-->>API: Delete result
API-->>WebApp: 200 { success: true }
WebApp->>WebApp: Invalidate queries (blocked-users, allies, dms, user-profile)
WebApp-->>User: Update UI (blocked)
sequenceDiagram
actor Sender
participant Realtime as Realtime Server
participant BlocksSvc as Blocks Service
participant DB as Database
Sender->>Realtime: Emit "message:send" (channelId, payload)
Realtime->>BlocksSvc: isDMBlockedForUser(channelId, senderId)
BlocksSvc->>DB: SELECT other member FROM channel_member WHERE channelId = ? AND userId != ?
DB-->>BlocksSvc: otherUserId
BlocksSvc->>DB: SELECT 1 FROM user_block WHERE (blocker,blocked) IN ((sender,other),(other,sender))
DB-->>BlocksSvc: row exists / none
BlocksSvc-->>Realtime: true / false
alt blocked
Realtime-->>Sender: ack { ok: false, error: "Cannot send messages in this conversation" }
else allowed
Realtime->>Realtime: proceed to persist/broadcast message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/use-typing-indicator.ts (1)
39-67:⚠️ Potential issue | 🟡 MinorMissing
blockedUserIdsin effect dependencies causes stale closure.The
onTypingUpdatecallback referencesblockedUserIdsbut it's not included in the dependency array on line 67. When a user is blocked or unblocked, the effect won't re-run, and the callback will use a stale reference. This means typing indicators from newly blocked users will still appear until the channel changes.🔧 Proposed fix
return () => { socket.off("typing:update", onTypingUpdate) } - }, [socket, channelId, currentUserId]) + }, [socket, channelId, currentUserId, blockedUserIds])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-typing-indicator.ts` around lines 39 - 67, The onTypingUpdate handler inside the useEffect references blockedUserIds but the effect dependency array only lists [socket, channelId, currentUserId], causing a stale closure; update the dependency array to include blockedUserIds (or a stable representation of it) so the effect re-registers the socket listener when blocked users change. If blockedUserIds is a Set that changes identity frequently, wrap it with useMemo (e.g., const blockedIdsArray = useMemo(() => Array.from(blockedUserIds), [blockedUserIds]) or store a stable ref) and use that memoized value in the dependency array so that the useEffect (and the onTypingUpdate registration/deregistration) runs only when the actual blocked set changes.
🤖 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/blocks/handlers.ts`:
- Around line 112-124: The query building the blocked-user list (the const
blocks = await db.select(...) from schema.userBlock innerJoin schema.user where
eq(schema.userBlock.blockerId, currentUser.id)) lacks an ORDER BY, causing
nondeterministic response order; add an explicit orderBy on a stable column such
as schema.userBlock.createdAt (or the aliased blockedAt) to sort
deterministically (e.g., newest first) so the blocks listing is stable across
requests and avoids UI/cache jitter.
- Around line 37-80: The non-atomic pre-check using existingBlock plus the later
tx.insert(schema.userBlock) allows a race that causes a 500 on
unique-constraint; wrap the insert in the transaction with duplicate-handling:
either use the DB library's upsert/ON CONFLICT DO NOTHING equivalent or catch
the unique-violation error from tx.insert(schema.userBlock) (check the DB
error/code, e.g., Postgres 23505) and return c.json({ success: false, message:
"User is already blocked" }, HttpStatusCodes.BAD_REQUEST) instead of surfacing a
500; ensure this handling occurs inside db.transaction and still runs the
ally-request delete when a concurrent insert happened.
In `@apps/web/src/components/chat/message-item.tsx`:
- Line 144: The local state showBlockedContent in MessageItem can persist across
reused instances; modify the component to reset showBlockedContent whenever the
message changes by adding an effect that watches message.id (or the message
prop) and calls setShowBlockedContent(false), and apply the same pattern for any
other per-message state referenced around the showBlockedContent usage (e.g.,
the logic touched in the block near lines 197-209) so each new message starts
with the correct default state.
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 252-257: The availability logic currently treats
usernameAvailability === "idle" as valid which lets a changed username be saved
without a completed availability check; update the condition for isUsernameValid
so that "idle" is only considered valid when usernameChanged is false (i.e., an
unchanged username), otherwise require usernameAvailability === "available" (or
explicitly treat "idle" and "error" as invalid). Adjust the isValid computation
accordingly and ensure any save/submit UI (e.g., the save button) is disabled
when isUsernameValid is false so users cannot save an unvalidated username.
- Around line 75-113: The debounce timer stored in usernameCheckTimer (used
inside handleUsernameChange) isn't cleared on component unmount; add a cleanup
effect that clears usernameCheckTimer.current (clearTimeout) when the component
unmounts and optionally set usernameCheckTimer.current = undefined/null to avoid
stale references, ensuring no async callback tries to set state after unmount.
Implement this with a useEffect that returns a cleanup function and reference
the existing usernameCheckTimer variable.
In `@apps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsx`:
- Around line 48-52: filteredAllies currently filters only by users I blocked
(blockedUserIds.has) so it still shows users who have blocked me and will cause
createDM to 403; update the filtering in the new-dm-dialog (the filteredAllies
computation that uses allies?.allies and blockedUserIds.has) to exclude any ally
with a non-"none" block status or to also check a "blockedByUserIds" /
reciprocal set returned from the allies payload; if backend can include
blockStatus on each ally, filter out allies where ally.blockStatus !== "none"
(or ally.blockStatus is present and not "none") so bidirectional blocks are
removed before selection.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx:
- Around line 160-165: The current UI only checks whether the current user has
blocked the other DM participant (variables isDirect, otherMemberId,
isOtherBlocked, blockedUserIds) but does not detect if the other participant has
blocked the current user, which leads to a confusing server-side rejection;
update the DM input visibility logic to also consult the other participant's
blockStatus from the user profile API (use the existing profile/blockStatus
field for the otherMemberId when isDirect) and combine that with the existing
isOtherBlocked check so the input is hidden or shows an appropriate message when
either side has blocked the other.
In `@packages/db/src/schemas/user-blocks.ts`:
- Around line 12-31: The user_block table currently allows rows where blockerId
=== blockedId; add a DB-level CHECK constraint in the userBlock pgTable
definition to enforce blocker_id <> blocked_id so self-blocks cannot be inserted
from any path. Update the userBlock table (the pgTable call that defines id,
createdAt, blockerId, blockedId and indexes) to include a CHECK constraint
(e.g., "blocker_id <> blocked_id") so the database rejects self-block attempts;
keep the constraint name clear (e.g., user_block_no_self_block) for easy
debugging.
In `@ROADMAP.md`:
- Around line 42-44: The roadmap still lists "User blocking" as incomplete;
update the ROADMAP.md task list to reflect the work merged by either marking
"User blocking" as complete (change "[ ] User blocking" to "[x] User blocking")
or, if there are remaining discrete items, split that line into explicit
sub-tasks (e.g., "User blocking: schema", "User blocking: API", "User blocking:
UI") and mark the finished sub-items complete while leaving any remaining ones
unchecked so the board is accurate.
---
Outside diff comments:
In `@apps/web/src/hooks/use-typing-indicator.ts`:
- Around line 39-67: The onTypingUpdate handler inside the useEffect references
blockedUserIds but the effect dependency array only lists [socket, channelId,
currentUserId], causing a stale closure; update the dependency array to include
blockedUserIds (or a stable representation of it) so the effect re-registers the
socket listener when blocked users change. If blockedUserIds is a Set that
changes identity frequently, wrap it with useMemo (e.g., const blockedIdsArray =
useMemo(() => Array.from(blockedUserIds), [blockedUserIds]) or store a stable
ref) and use that memoized value in the dependency array so that the useEffect
(and the onTypingUpdate registration/deregistration) runs only when the actual
blocked set changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: befa491b-2cad-4450-a921-ba1ba32d2437
📒 Files selected for processing (27)
ROADMAP.mdapps/api/src/app.tsapps/api/src/routes/v1/allies/handlers.tsapps/api/src/routes/v1/blocks/handlers.tsapps/api/src/routes/v1/blocks/index.tsapps/api/src/routes/v1/blocks/routes.tsapps/api/src/routes/v1/blocks/schema.tsapps/api/src/routes/v1/dms/handlers.tsapps/api/src/routes/v1/users/handlers.tsapps/api/src/routes/v1/users/schema.tsapps/realtime/src/index.tsapps/realtime/src/services/blocks.tsapps/web/src/components/allies/allies-page.tsxapps/web/src/components/chat/composer/mention-suggestion-list.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/components/chat/message-list.tsxapps/web/src/components/settings/my-account-settings.tsxapps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsxapps/web/src/components/ui/user-profile-card.tsxapps/web/src/hooks/use-blocked-users.tsapps/web/src/hooks/use-typing-indicator.tsapps/web/src/lib/api-types.tsapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxpackages/db/src/schemas/index.tspackages/db/src/schemas/user-blocks.tspackages/db/src/schemas/users.ts
Comment on lines
+48
to
52
| const filteredAllies = (allies?.allies ?? []).filter( | ||
| (ally) => | ||
| ally.name.toLowerCase().includes(search.toLowerCase()) && | ||
| !blockedUserIds.has(ally.id) | ||
| ) |
There was a problem hiding this comment.
One-way block filtering still allows invalid DM targets.
This filter only removes users blocked by me. Users who blocked me are still listed/selectable, but createDM is rejected server-side (403) by the bidirectional block check in apps/api/src/routes/v1/dms/handlers.ts (Lines 75-98). Please filter using bidirectional block data (or include blockStatus in the allies payload and exclude all non-none states).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/sidebar/dm-panel/new-dm-dialog.tsx` around lines 48 -
52, filteredAllies currently filters only by users I blocked
(blockedUserIds.has) so it still shows users who have blocked me and will cause
createDM to 403; update the filtering in the new-dm-dialog (the filteredAllies
computation that uses allies?.allies and blockedUserIds.has) to exclude any ally
with a non-"none" block status or to also check a "blockedByUserIds" /
reciprocal set returned from the allies payload; if backend can include
blockStatus on each ally, filter out allies where ally.blockStatus !== "none"
(or ally.blockStatus is present and not "none") so bidirectional blocks are
removed before selection.
Comment on lines
+160
to
+165
| // For 1:1 DMs, check if the other user is blocked | ||
| const isDirect = dm.type === "dm" | ||
| const otherMemberId = isDirect ? dm.members[0]?.id : undefined | ||
| const isOtherBlocked = otherMemberId | ||
| ? blockedUserIds.has(otherMemberId) | ||
| : false |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider if blocked-by-other-user case needs client-side handling.
The current implementation only checks if the current user blocked the other member. If the other user blocked the current user, the input will still be shown, and the server will reject the message (per apps/realtime/src/index.ts enforcement). This is likely intentional to avoid revealing that someone blocked you, but results in a confusing "Cannot send messages" error at send time.
If this UX is acceptable, this is fine as-is. Otherwise, the user profile API already exposes blockStatus which could inform the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx around lines 160 - 165, The
current UI only checks whether the current user has blocked the other DM
participant (variables isDirect, otherMemberId, isOtherBlocked, blockedUserIds)
but does not detect if the other participant has blocked the current user, which
leads to a confusing server-side rejection; update the DM input visibility logic
to also consult the other participant's blockStatus from the user profile API
(use the existing profile/blockStatus field for the otherMemberId when isDirect)
and combine that with the existing isOtherBlocked check so the input is hidden
or shows an appropriate message when either side has blocked the other.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/use-typing-indicator.ts (1)
42-67:⚠️ Potential issue | 🟡 MinorRemove already-visible typers when the block list updates.
The new guard only affects future
typing:updateevents. If a blocked user is already present intypingUserswhenblockedUserIdsloads or changes, they keep showing until the 5s expiry timer runs. Prune the current list on blocklist changes so the indicator disappears immediately.💡 Proposed fix
useEffect(() => { if (!socket) return @@ return () => { socket.off("typing:update", onTypingUpdate) } }, [socket, channelId, currentUserId, blockedUserIds]) + + useEffect(() => { + if (!blockedUserIds) return + setTypingUsers((prev) => + prev.filter((user) => !blockedUserIds.has(user.userId)) + ) + }, [blockedUserIds])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-typing-indicator.ts` around lines 42 - 67, When blockedUserIds updates we also need to remove any already-visible typers; inside the same effect that registers onTypingUpdate (use-typing-indicator.ts) call setTypingUsers and filter out users whose userId is in blockedUserIds (guarding for blockedUserIds being undefined) so existing entries are pruned immediately; update logic near the onTypingUpdate/setTypingUsers code (referencing the onTypingUpdate handler and setTypingUsers) to run this prune whenever blockedUserIds changes.
🤖 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/blocks/handlers.ts`:
- Around line 4-8: The import of route types (BlockUserRoute,
ListBlockedUsersRoute, UnblockUserRoute) uses a sibling relative path
("./routes") which violates the apps/api import convention; change that import
to use the project alias (@"./routes" -> "@/routes") so the types are imported
via the "@/..." alias consistent with tsc + tsc-alias rewrites and the rest of
the codebase; update the import statement in handlers.ts to reference "@/routes"
for these named type imports.
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 101-110: The availability callback can be stale: before awaiting
authClient.isUsernameAvailable capture the candidate string (e.g., const checked
= trimmed) and after the await compare it to the current input (e.g., const
current = username.trim()) and bail out if they differ so old responses don't
overwrite state; keep using setUsernameAvailability and
usernameCheckTimer.current as before but only call setUsernameAvailability when
checked === current.
---
Outside diff comments:
In `@apps/web/src/hooks/use-typing-indicator.ts`:
- Around line 42-67: When blockedUserIds updates we also need to remove any
already-visible typers; inside the same effect that registers onTypingUpdate
(use-typing-indicator.ts) call setTypingUsers and filter out users whose userId
is in blockedUserIds (guarding for blockedUserIds being undefined) so existing
entries are pruned immediately; update logic near the
onTypingUpdate/setTypingUsers code (referencing the onTypingUpdate handler and
setTypingUsers) to run this prune whenever blockedUserIds changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20d6f184-2e0a-44d3-97ce-d9079697fea1
📒 Files selected for processing (6)
ROADMAP.mdapps/api/src/routes/v1/blocks/handlers.tsapps/web/src/components/chat/message-item.tsxapps/web/src/components/settings/my-account-settings.tsxapps/web/src/hooks/use-typing-indicator.tspackages/db/src/schemas/user-blocks.ts
Owner
Author
|
Tip For best results, initiate chat on the files or code changes.
What would you like me to do? 😊 |
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/realtime/src/index.ts`:
- Around line 236-257: The current broadcast uses onlinePrivacy from privacyRow
and only suppresses "no_one", which still emits presence:user:update to every
guildRoom in guildIds even when onlinePrivacy === "allies_only"; change this by
doing per-recipient filtering before emitting: when onlinePrivacy ===
"allies_only" query the set of allowed recipient userIds (e.g., allies or
mutual-contacts) for each guild (or fetch a map of guildId -> allowedUserIds)
and only call io.to(guildRoom(guildId)).emit for rooms where the room contains
at least one allowed recipient, or iterate allowed recipient sockets to emit
directly; update the loop around guildIds and use socket.data.user.id and
presence:user:update as identifiers to ensure only allies receive the event.
- Around line 373-399: The current allyRows query pulls any accepted ally pairs
where both senderId and receiverId are in [requestingUserId, ...alliesOnlyIds],
which returns relationships between two alliesOnlyIds and wastes DB work; change
the WHERE to require status = "accepted" AND ((senderId = requestingUserId AND
receiverId IN alliesOnlyIds) OR (receiverId = requestingUserId AND senderId IN
alliesOnlyIds)). Update the query that builds allyRows (the
db.select/...from(schema.allyRequest) call) to use those OR conditions with
eq(schema.allyRequest.senderId, requestingUserId),
eq(schema.allyRequest.receiverId, requestingUserId) and inArray against
alliesOnlyIds so only rows involving the requester are fetched.
In `@apps/web/src/components/settings/privacy-safety-settings.tsx`:
- Around line 39-48: The handleChange currently calls updateSettings({ [key]:
value }) and only shows a toast on error, so the Select's visible value remains
changed even if the server rejects it; modify the mutation to perform an
optimistic update with rollback by using updateSettings' onMutate to store the
previous PrivacySettings value (lookup by key) and immediately update local
state/UI, and then implement onError to restore that previous value and show the
toast; alternatively, if you don't use mutation-level optimistic updates, make
handleChange set the Select back to the previous value inside the onError
callback (using the stored previous value) so the Select component and
PrivacySettings remain consistent when updateSettings fails.
In `@packages/db/src/schemas/user-privacy-settings.ts`:
- Around line 43-76: The code is using the deprecated drizzle-zod package;
update the import of createSelectSchema and createInsertSchema to come from
"drizzle-orm/zod" instead of "drizzle-zod" and remove drizzle-zod from package
dependencies (update packages/db/package.json) so the project uses drizzle-orm's
built-in Zod support; adjust the import statement where createSelectSchema and
createInsertSchema are used (referenced in selectUserPrivacySettingsSchema and
insertUserPrivacySettingsSchema) to point to "drizzle-orm/zod".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 075ad6fa-9b37-49d5-b8d4-83da00a39a93
📒 Files selected for processing (16)
apps/api/src/app.tsapps/api/src/routes/v1/allies/handlers.tsapps/api/src/routes/v1/allies/routes.tsapps/api/src/routes/v1/dms/handlers.tsapps/api/src/routes/v1/privacy-settings/handlers.tsapps/api/src/routes/v1/privacy-settings/index.tsapps/api/src/routes/v1/privacy-settings/routes.tsapps/api/src/routes/v1/privacy-settings/schema.tsapps/realtime/src/index.tsapps/web/src/components/settings/privacy-safety-settings.tsxapps/web/src/components/settings/settings-dialog.tsxapps/web/src/components/ui/user-profile-card.tsxapps/web/src/hooks/use-privacy-settings.tspackages/db/src/schemas/index.tspackages/db/src/schemas/user-privacy-settings.tspackages/db/src/schemas/users.ts
Comment on lines
+236
to
257
| // Check user's online status privacy before broadcasting | ||
| const privacyRow = await db | ||
| .select({ | ||
| onlineStatusPrivacy: schema.userPrivacySettings.onlineStatusPrivacy, | ||
| }) | ||
| .from(schema.userPrivacySettings) | ||
| .where(eq(schema.userPrivacySettings.userId, socket.data.user.id)) | ||
| .limit(1) | ||
| .then((rows) => rows[0]) | ||
|
|
||
| const onlinePrivacy = privacyRow?.onlineStatusPrivacy ?? "everyone" | ||
|
|
||
| if (onlinePrivacy !== "no_one") { | ||
| for (const guildId of guildIds) { | ||
| io.to(guildRoom(guildId)).emit("presence:user:update", { | ||
| guildId, | ||
| userId: socket.data.user.id, | ||
| status: "online", | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify "allies_only" presence broadcast behavior.
The online presence broadcast suppresses only "no_one" but still broadcasts to all guild rooms for "allies_only" users. This means the filtering for "allies_only" happens on the receiving end in presence:subscribe (lines 337-410), not at broadcast time.
This is a valid design choice (broadcast-then-filter), but it means online status events are still emitted to guild rooms even for users who don't want non-allies to see them—the filtering is deferred to snapshot requests. If you want to prevent even the event emission for "allies_only", you'd need per-recipient filtering here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/realtime/src/index.ts` around lines 236 - 257, The current broadcast
uses onlinePrivacy from privacyRow and only suppresses "no_one", which still
emits presence:user:update to every guildRoom in guildIds even when
onlinePrivacy === "allies_only"; change this by doing per-recipient filtering
before emitting: when onlinePrivacy === "allies_only" query the set of allowed
recipient userIds (e.g., allies or mutual-contacts) for each guild (or fetch a
map of guildId -> allowedUserIds) and only call io.to(guildRoom(guildId)).emit
for rooms where the room contains at least one allowed recipient, or iterate
allowed recipient sockets to emit directly; update the loop around guildIds and
use socket.data.user.id and presence:user:update as identifiers to ensure only
allies receive the event.
Comment on lines
+39
to
+48
| const handleChange = (key: keyof PrivacySettings, value: string) => { | ||
| updateSettings( | ||
| { [key]: value }, | ||
| { | ||
| onError: () => { | ||
| toast.error("Failed to update privacy setting") | ||
| }, | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reverting UI state on mutation failure.
When updateSettings fails, the Select component will display the newly selected value even though the server rejected the update. The user sees a toast error but the UI shows the failed value.
🛠️ Suggested improvement using optimistic update rollback
const handleChange = (key: keyof PrivacySettings, value: string) => {
+ const previousValue = settings?.[key]
updateSettings(
{ [key]: value },
{
onError: () => {
toast.error("Failed to update privacy setting")
+ // React Query will automatically rollback if using optimistic updates,
+ // or you can manually invalidate to refetch:
+ // queryClient.invalidateQueries({ queryKey: ["privacy-settings"] })
},
}
)
}Alternatively, configure the mutation hook with onMutate/onError for proper optimistic updates with rollback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/settings/privacy-safety-settings.tsx` around lines 39
- 48, The handleChange currently calls updateSettings({ [key]: value }) and only
shows a toast on error, so the Select's visible value remains changed even if
the server rejects it; modify the mutation to perform an optimistic update with
rollback by using updateSettings' onMutate to store the previous PrivacySettings
value (lookup by key) and immediately update local state/UI, and then implement
onError to restore that previous value and show the toast; alternatively, if you
don't use mutation-level optimistic updates, make handleChange set the Select
back to the previous value inside the onError callback (using the stored
previous value) so the Select component and PrivacySettings remain consistent
when updateSettings fails.
Comment on lines
+43
to
+76
| export const selectUserPrivacySettingsSchema = | ||
| createSelectSchema(userPrivacySettings) | ||
| export const insertUserPrivacySettingsSchema = createInsertSchema( | ||
| userPrivacySettings | ||
| ).omit({ | ||
| id: true, | ||
| createdAt: true, | ||
| updatedAt: true, | ||
| }) | ||
|
|
||
| export const privacySettingsResponseSchema = | ||
| selectUserPrivacySettingsSchema.pick({ | ||
| dmPrivacy: true, | ||
| allyRequestPrivacy: true, | ||
| onlineStatusPrivacy: true, | ||
| }) | ||
|
|
||
| export const updatePrivacySettingsSchema = insertUserPrivacySettingsSchema | ||
| .pick({ | ||
| dmPrivacy: true, | ||
| allyRequestPrivacy: true, | ||
| onlineStatusPrivacy: true, | ||
| }) | ||
| .partial() | ||
|
|
||
| export const userPrivacySettingsRelations = relations( | ||
| userPrivacySettings, | ||
| ({ one }) => ({ | ||
| user: one(user, { | ||
| fields: [userPrivacySettings.userId], | ||
| references: [user.id], | ||
| }), | ||
| }) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
drizzle-zod compatibility with zod 4
💡 Result:
drizzle-zod has partial compatibility with Zod 4. It supports Zod v4 features via the 'zod/v4' namespace in Zod ^3.25+, as implemented in PR #4478 (merged May 2025) and released in drizzle-zod@0.8.1+. Use import { z } from 'zod/v4'; for Zod v4 schemas. However, it is not fully compatible with the standalone Zod ^4.0.0 package due to peerDependency on Zod ^3.25.1, causing installation conflicts. Issue #4746 (open as of late 2025) requests full Zod v4 support. Workarounds include --legacy-peer-deps, pnpm overrides, or pinning to older versions like ^0.6.0. Note: drizzle-zod is deprecated since drizzle-orm ^0.XX (exact version unspecified). Official recommendation: Use built-in Zod support in Drizzle ORM core via drizzle-orm/zod, which explicitly supports Zod v4 (import { z } from 'zod/v4';) and provides equivalent functionality like createSelectSchema.
Citations:
- 1: Support Zod v4 in drizzle-zod drizzle-team/drizzle-orm#4478
- 2: [FEATURE]: drizzle-zod support for zod v4 drizzle-team/drizzle-orm#4625
- 3: [FEATURE]: drizzle-zod adopt zod v4 drizzle-team/drizzle-orm#4406
- 4: https://orm.drizzle.team/docs/zod
- 5: [BUG]: Bump
drizzle-zodtozodv4 drizzle-team/drizzle-orm#4746 - 6: https://github.com/drizzle-team/drizzle-orm/tree/main/drizzle-zod
🏁 Script executed:
# Check the imports in the user-privacy-settings.ts file
head -n 50 packages/db/src/schemas/user-privacy-settings.tsRepository: BuckyMcYolo/townhall
Length of output: 1525
🏁 Script executed:
# Check package.json for dependency versions
cat package.json | grep -A 5 -B 5 "drizzle\|zod"Repository: BuckyMcYolo/townhall
Length of output: 46
🏁 Script executed:
# Find and display package.json
find . -name "package.json" -type f | head -5Repository: BuckyMcYolo/townhall
Length of output: 191
🏁 Script executed:
# Check the root package.json for dependencies
cd packages/db && cat package.json 2>/dev/null || cat ../../package.json 2>/dev/null | grep -A 20 '"dependencies"'Repository: BuckyMcYolo/townhall
Length of output: 714
Replace drizzle-zod with drizzle-orm/zod — the library is deprecated and incompatible with Zod v4.
The Zod schemas themselves are correct, but drizzle-zod 0.8.3 is deprecated and has a peerDependency conflict with Zod ^4.3.6. The official recommendation from Drizzle is to migrate to drizzle-orm/zod, which provides the same createSelectSchema and createInsertSchema functions with explicit Zod v4 support.
Update the import on line 3:
import { createInsertSchema, createSelectSchema } from "drizzle-orm/zod"
Also update packages/db/package.json to remove drizzle-zod and use the built-in Zod support from drizzle-orm (already at ^0.45.1).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/schemas/user-privacy-settings.ts` around lines 43 - 76, The
code is using the deprecated drizzle-zod package; update the import of
createSelectSchema and createInsertSchema to come from "drizzle-orm/zod" instead
of "drizzle-zod" and remove drizzle-zod from package dependencies (update
packages/db/package.json) so the project uses drizzle-orm's built-in Zod
support; adjust the import statement where createSelectSchema and
createInsertSchema are used (referenced in selectUserPrivacySettingsSchema and
insertUserPrivacySettingsSchema) to point to "drizzle-orm/zod".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/components/settings/my-account-settings.tsx (1)
101-119:⚠️ Potential issue | 🟠 MajorTrack username checks by request identity, not by
"checking"state.Line 109 only proves that some availability lookup is still
"checking", not that this callback belongs to the latest input. If request A resolves after request B has already put the field back into"checking", A can overwrite B's result, and B's later response will then be ignored because the state is no longer"checking".💡 Suggested fix
const usernameCheckTimer = useRef<ReturnType<typeof setTimeout> | null>(null) +const usernameRequestSeq = useRef(0) const handleUsernameChange = useCallback( (value: string) => { + const requestSeq = ++usernameRequestSeq.current setDisplayUsername(value) if (usernameCheckTimer.current) clearTimeout(usernameCheckTimer.current) @@ const checked = trimmed usernameCheckTimer.current = setTimeout(async () => { try { const { data } = await authClient.isUsernameAvailable({ username: checked, }) - // Only update if input hasn't changed since we started checking - setUsernameAvailability((prev) => - prev === "checking" - ? data?.available - ? "available" - : "taken" - : prev - ) + if (requestSeq !== usernameRequestSeq.current) return + setUsernameAvailability(data?.available ? "available" : "taken") } catch { - setUsernameAvailability((prev) => - prev === "checking" ? "idle" : prev - ) + if (requestSeq !== usernameRequestSeq.current) return + setUsernameAvailability("idle") } }, 500) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/my-account-settings.tsx` around lines 101 - 119, The availability check currently gates updates by comparing state to "checking", which can let out-of-order responses overwrite newer results; fix this by adding a request identity (e.g., a ref like usernameCheckIdRef) that you increment before scheduling the timeout, capture the current id in the setTimeout closure (where you call authClient.isUsernameAvailable with the checked variable), and on success/failure only call setUsernameAvailability if the captured id matches the latest id in the ref; also ensure any existing usernameCheckTimer is cleared when scheduling a new check (usernameCheckTimer) so timers and ids stay in sync.
🤖 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/web/src/hooks/use-typing-indicator.ts`:
- Around line 62-68: The pruning of blocked typers currently runs inside the
listener setup effect; extract that logic into its own useEffect that depends
only on blockedUserIds (and setTypingUsers) so it only runs when the block list
changes. Locate the existing filtered logic that uses setTypingUsers and
blockedUserIds in use-typing-indicator.ts (the block that filters prev.filter(u
=> !blockedUserIds.has(u.userId))) and move it into a new effect which
early-returns when blockedUserIds is empty and calls setTypingUsers with the
same filtering and no-ops when nothing changed.
---
Duplicate comments:
In `@apps/web/src/components/settings/my-account-settings.tsx`:
- Around line 101-119: The availability check currently gates updates by
comparing state to "checking", which can let out-of-order responses overwrite
newer results; fix this by adding a request identity (e.g., a ref like
usernameCheckIdRef) that you increment before scheduling the timeout, capture
the current id in the setTimeout closure (where you call
authClient.isUsernameAvailable with the checked variable), and on
success/failure only call setUsernameAvailability if the captured id matches the
latest id in the ref; also ensure any existing usernameCheckTimer is cleared
when scheduling a new check (usernameCheckTimer) so timers and ids stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54495170-35f1-4638-975e-e462b360cf90
📒 Files selected for processing (2)
apps/web/src/components/settings/my-account-settings.tsxapps/web/src/hooks/use-typing-indicator.ts
Comment on lines
+62
to
+68
| // Prune any currently visible typers who are now blocked | ||
| if (blockedUserIds && blockedUserIds.size > 0) { | ||
| setTypingUsers((prev) => { | ||
| const filtered = prev.filter((u) => !blockedUserIds.has(u.userId)) | ||
| return filtered.length === prev.length ? prev : filtered | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the pruning logic into a separate effect.
The pruning logic runs inside the listener setup effect, so it executes on every dependency change (socket, channelId, currentUserId, blockedUserIds), not just when blockedUserIds changes. While functionally correct, a dedicated effect would be more targeted:
♻️ Suggested refactor to isolate pruning
+ // Prune typing users when blocklist changes
+ useEffect(() => {
+ if (blockedUserIds && blockedUserIds.size > 0) {
+ setTypingUsers((prev) => {
+ const filtered = prev.filter((u) => !blockedUserIds.has(u.userId))
+ return filtered.length === prev.length ? prev : filtered
+ })
+ }
+ }, [blockedUserIds])
+
// Listen for typing events from others
useEffect(() => {
if (!socket) return
const onTypingUpdate = (payload: TypingIndicatorEvent) => {
if (payload.channelId !== channelId) return
if (payload.userId === currentUserId) return
if (blockedUserIds?.has(payload.userId)) return
// ... rest of handler
}
- // Prune any currently visible typers who are now blocked
- if (blockedUserIds && blockedUserIds.size > 0) {
- setTypingUsers((prev) => {
- const filtered = prev.filter((u) => !blockedUserIds.has(u.userId))
- return filtered.length === prev.length ? prev : filtered
- })
- }
-
socket.on("typing:update", onTypingUpdate)
return () => {
socket.off("typing:update", onTypingUpdate)
}
}, [socket, channelId, currentUserId, blockedUserIds])This separates concerns and avoids redundant pruning passes when only socket or channelId changes. That said, the current implementation is functionally correct and the overhead is minimal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Prune any currently visible typers who are now blocked | |
| if (blockedUserIds && blockedUserIds.size > 0) { | |
| setTypingUsers((prev) => { | |
| const filtered = prev.filter((u) => !blockedUserIds.has(u.userId)) | |
| return filtered.length === prev.length ? prev : filtered | |
| }) | |
| } | |
| // Prune typing users when blocklist changes | |
| useEffect(() => { | |
| if (blockedUserIds && blockedUserIds.size > 0) { | |
| setTypingUsers((prev) => { | |
| const filtered = prev.filter((u) => !blockedUserIds.has(u.userId)) | |
| return filtered.length === prev.length ? prev : filtered | |
| }) | |
| } | |
| }, [blockedUserIds]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/use-typing-indicator.ts` around lines 62 - 68, The pruning
of blocked typers currently runs inside the listener setup effect; extract that
logic into its own useEffect that depends only on blockedUserIds (and
setTypingUsers) so it only runs when the block list changes. Locate the existing
filtered logic that uses setTypingUsers and blockedUserIds in
use-typing-indicator.ts (the block that filters prev.filter(u =>
!blockedUserIds.has(u.userId))) and move it into a new effect which
early-returns when blockedUserIds is empty and calls setTypingUsers with the
same filtering and no-ops when nothing changed.
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.
fix
Pull Request Summary: User Blocking System, Username Editing, and Mention Display
Overview
Adds a complete user blocking system (DB schema, API, realtime enforcement, and UI), privacy settings for ally requests/DMs/online presence, username/displayUsername editing with availability checks, and fixes mention suggestion display to prefer displayUsername.
Key Features Implemented
Database
API
Business logic & integrations
Realtime
Frontend / UX
Types
Notable Implementation Details
Deployment & Operational Notes
Tests & Coverage
Areas for Review / Concerns
Confidence Score: 3/5
Rationale: Comprehensive and consistent across DB, API, realtime, and UI with types and OpenAPI schemas. Primary issues are operational (ensure migrations are applied) and absence of automated tests for critical behaviors; a few edge-case and consistency reviews are advisable prior to merging.