Dev#6
Merged
BuckyMcYolo merged 6 commits intomainfrom Feb 18, 2026
Merged
Conversation
channels and API for repositioning the channels
- add clickable DM list with active state and /dms/$dmId route stub - add clickable channel list with active state and /$guildSlug/$channelId route stub - render stacked avatar cluster for group DMs using AvatarGroup - seed script: generate 4 group DM conversations with members and messages - remove grip handle icons; make full channel/category row draggable - reduce channel panel left padding (px-3 → px-2)
📝 WalkthroughWalkthroughAdds a paginated DM listing API and seed script, session-only auth middleware, guild-scoped channel reorder API, frontend DM panel and DnD channel reordering UI, resizable sidebar layout, new UI primitives, and related config/dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Web as Client (Web)
participant API as API Server
participant DB as Database
User->>Web: Open DMs view
Web->>API: GET /v1/dms?page=1&perPage=20
API->>DB: Query DM channels (filter by member) + count
DB-->>API: channels list + total
API->>DB: Query latest message per channel (by channelIds)
DB-->>API: messages + authors
API->>DB: Query channel members (exclude current user)
DB-->>API: members per channel
API->>Web: 200 OK { paginated channels with members + lastMessage }
Web->>User: Render DM list (avatars, previews)
sequenceDiagram
actor User as User
participant Web as Client (Web)
participant API as API Server
participant DB as Database
User->>Web: Drag channel to new position
Web->>Web: Apply optimistic reorder locally
Web->>API: PATCH /v1/guilds/{guildSlug}/channels/reorder {channels: [...]}
API->>DB: BEGIN TRANSACTION
loop each channel
API->>DB: UPDATE channels SET position,parentId WHERE id=...
end
DB-->>API: COMMIT
API->>Web: 200 OK { success: true }
Web->>User: Keep reordered view (confirm)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 11
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/routes.ts (1)
19-38:⚠️ Potential issue | 🟡 MinorMissing 404 response in OpenAPI definitions.
The
guildAuthMiddlewarecan return a404 Not Foundwhen the guild slug doesn't match any guild, but none of the route definitions (listChannels,createChannel,reorderChannels) document a 404 response. This means the generated OpenAPI spec won't reflect this possible response, which could confuse API consumers.Consider adding a
[HttpStatusCodes.NOT_FOUND]: notFoundSchemaentry to each route'sresponses.Also applies to: 40-63, 65-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/channels/routes.ts` around lines 19 - 38, The route OpenAPI responses are missing a 404 for cases where guildAuthMiddleware returns Not Found; update the responses object for listChannels (and likewise for createChannel and reorderChannels) to include [HttpStatusCodes.NOT_FOUND]: notFoundSchema so the generated spec documents the middleware’s possible 404 outcome; locate the responses block in each createRoute call (symbols: listChannels, createChannel, reorderChannels) and add that entry alongside the existing OK/UNAUTHORIZED/FORBIDDEN/INTERNAL_SERVER_ERROR entries.
🧹 Nitpick comments (16)
apps/api/src/middleware/session-auth.ts (1)
16-32: Wrapauth.api.getSession()in a try/catch.If the underlying auth call throws (e.g. database unavailable, cookie parsing error), the exception propagates uncaught and Hono's generic 500 handler fires. A structured catch gives you a consistent error response and a log trace.
🛡️ Proposed fix
export const sessionAuthMiddleware = async ( c: Context<AppBindings>, next: Next ) => { - const session = await auth.api.getSession({ headers: c.req.raw.headers }) + let session: Awaited<ReturnType<typeof auth.api.getSession>> + try { + session = await auth.api.getSession({ headers: c.req.raw.headers }) + } catch { + return c.json( + { success: false, message: "Unauthorized" }, + HttpStatusCodes.UNAUTHORIZED + ) + } if (!session) { return c.json( { success: false, message: "Unauthorized" }, HttpStatusCodes.UNAUTHORIZED ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/middleware/session-auth.ts` around lines 16 - 32, Wrap the call to auth.api.getSession inside sessionAuthMiddleware in a try/catch so any thrown errors are handled; specifically, call auth.api.getSession({ headers: c.req.raw.headers }) inside a try block, and in the catch log the error (using your app logger or console.error) and return a consistent c.json response with HttpStatusCodes.INTERNAL_SERVER_ERROR and a short message (e.g. "Authentication error"); preserve the existing behavior for when session is null (return UNAUTHORIZED) and continue to set c.set("user", session.user) and c.set("session", session.session) when successful.apps/web/src/routes/signup.tsx (1)
23-33: Consider extracting the session-redirect guard into a shared hook.The session-check
useEffectpattern —authClient.useSession()→ redirect to/when non-pending and truthy — is identical acrosssignup.tsxandlogin.tsx. A small custom hook likeuseRedirectIfAuthenticated()would eliminate the duplication.♻️ Proposed extraction
// e.g., apps/web/src/hooks/use-redirect-if-authenticated.ts +import { authClient } from "@repo/auth/client" +import { useNavigate } from "@tanstack/react-router" +import { useEffect } from "react" + +export function useRedirectIfAuthenticated(to = "/") { + const navigate = useNavigate() + const { data: session, isPending: sessionPending } = authClient.useSession() + useEffect(() => { + if (!sessionPending && session) { + navigate({ to }) + } + }, [sessionPending, session, navigate, to]) +}Then in both
LoginPageandSignUpPage:- const navigate = useNavigate() - const { data: session, isPending: sessionPending } = authClient.useSession() - useEffect(() => { - if (!sessionPending && session) { - navigate({ to: "/" }) - } - }, [sessionPending, session, navigate]) + useRedirectIfAuthenticated()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/signup.tsx` around lines 23 - 33, Extract the duplicate session-redirect logic into a new hook (e.g., useRedirectIfAuthenticated) that calls authClient.useSession(), checks sessionPending and session, and calls navigate({ to: "/" }) when session is non-pending and truthy; then replace the useEffect block in SignupPage (signup.tsx) and LoginPage (login.tsx) with a call to useRedirectIfAuthenticated so both files use the shared hook. Ensure the hook exports a default or named function and imports navigate from the same router used in the existing files so behavior remains identical.apps/api/src/lib/helpers/openapi/schemas.ts (1)
2-2:ZodTypeis valid in Zod 4 but may cause module-instance drift with@hono/zod-openapi.In Zod 4, all schemas extend the
z.ZodTypebase class, which in turn extendsz.$ZodTypefromzod/v4/core, so the import is correct. However,zis sourced from@hono/zod-openapiwhileZodTypeis pulled directly from"zod". If the two packages resolve to separate zod instances (possible in certain monorepo hoisting scenarios), TypeScript may report the constraint as unsatisfied. For robustness, deriveZodTypefrom the samezexport used everywhere in the file:♻️ Proposed fix
import { z } from "@hono/zod-openapi" -import type { ZodType } from "zod" import jsonContent from "./json-content" -export const paginatedResponseSchema = <T extends ZodType>(itemSchema: T) => +export const paginatedResponseSchema = <T extends z.ZodType>(itemSchema: T) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/helpers/openapi/schemas.ts` at line 2, The file imports ZodType directly from "zod", which can cause module-instance drift versus the `z` export used from `@hono/zod-openapi`; change the code to derive ZodType from the same `z` export used elsewhere in the file (e.g. import the type from the same module that provides `z` such as `@hono/zod-openapi`, or reference it as `z.ZodType`) so that all schema types share the same Zod instance; update the import line and any type references to use that unified `ZodType` symbol (e.g., replace `import type { ZodType } from "zod"` with an import from the module that exports `z` or use `z.ZodType`).apps/web/src/components/sidebar/dm-panel/dm-list.tsx (2)
65-74:getInitialsduplicates logic fromUserAvatar.The initials computation here (split → first char → join → slice(0,2) → uppercase) is identical to the one in
apps/web/src/components/ui/user-avatar.tsx. Consider extracting it into a shared utility to keep things DRY.🤖 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/dm-list.tsx` around lines 65 - 74, The initials logic in getInitials duplicates the same computation used by UserAvatar; extract that logic into a shared utility function (e.g., export function computeInitials(name: string | null): string) and replace the local getInitials in dm-list.tsx and the inline logic in apps/web/src/components/ui/user-avatar.tsx to import and call computeInitials instead, keeping its behavior (split, take first char of each word, join, slice(0,2), uppercase, default "?") and update imports/exports accordingly.
28-34: Loading state is indistinguishable from empty state.When
useQueryis still fetching,dataisundefined, so this renders "No conversations yet." Consider checkingisLoadingfromuseQueryto show a skeleton or spinner during the initial fetch.Proposed approach
- const { data } = useQuery({ + const { data, isLoading } = useQuery({ queryKey: ["dms"], queryFn: async () => { const res = await apiClient.v1.dms.$get({ query: {} }) if (!res.ok) throw new Error("Failed to fetch DMs") return res.json() }, }) + if (isLoading) { + return <div className="px-2 py-4 text-center text-sm text-muted-foreground">Loading…</div> + } + if (!data || data.data.length === 0) {🤖 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/dm-list.tsx` around lines 28 - 34, The current early return treats undefined `data` (while useQuery is loading) the same as an empty result; update the rendering logic in the DM list component that calls useQuery so it reads the hook's isLoading (and/or isFetching) flag and, when true, renders a loading UI (spinner or skeleton) instead of the "No conversations yet." message, and only show the empty-state message when isLoading is false and data?.data?.length === 0; reference the useQuery call and its isLoading return value to implement this conditional.apps/api/src/routes/v1/dms/schema.ts (1)
5-26:lastMessageAuthorSchemaanddmMemberSchemahave identical shapes.Both schemas define
{ id, name, username, displayUsername, image }. If they're expected to stay in sync, consider extracting a shared base schema to reduce duplication. If they're intentionally separate to allow independent evolution, this is fine as-is.Example shared schema approach
+const dmUserSchema = z.object({ + id: z.string().uuid(), + name: z.string(), + username: z.string().nullable(), + displayUsername: z.string().nullable(), + image: z.string().nullable(), +}) + -export const lastMessageAuthorSchema = z.object({ - id: z.string().uuid(), - name: z.string(), - username: z.string().nullable(), - displayUsername: z.string().nullable(), - image: z.string().nullable(), -}) +export const lastMessageAuthorSchema = dmUserSchema -export const dmMemberSchema = z.object({ - id: z.string().uuid(), - name: z.string(), - username: z.string().nullable(), - displayUsername: z.string().nullable(), - image: z.string().nullable(), -}) +export const dmMemberSchema = dmUserSchema🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/dms/schema.ts` around lines 5 - 26, lastMessageAuthorSchema and dmMemberSchema duplicate the same shape; extract a shared base schema (e.g., userBaseSchema or memberBaseSchema) that defines id, name, username, displayUsername, image and then reuse it to build lastMessageAuthorSchema and dmMemberSchema via zod's .extend() or by assigning them to the shared schema to remove duplication; alternatively, if you intend them to diverge later, add a brief comment above both schemas explaining the intentional duplication so future maintainers know it's deliberate.apps/api/scripts/seed-dms.ts (2)
84-91: Consider validating that the CLI argument is a valid UUID.If a non-UUID string is passed, the DB query will either error or return no rows. A quick UUID format check would give a friendlier error message.
Example
const userId = process.argv[2] if (!userId) { console.error( "Usage: pnpm --filter `@repo/api` exec tsx scripts/seed-dms.ts <user-id>" ) process.exit(1) } + +const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i +if (!uuidRegex.test(userId)) { + console.error(`Invalid UUID: ${userId}`) + process.exit(1) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/scripts/seed-dms.ts` around lines 84 - 91, The seed function currently queries the DB with userId without validating it; add a UUID format check for the CLI argument (userId) before calling db.select() in seed() so invalid strings produce a friendly error instead of a DB failure. Implement the check right before the existing select (or at the start of seed()), using a UUID validation helper (e.g., a regex or a library function) to verify userId, and if invalid call console.error with a clear message and process.exit(1); keep the rest of the logic (the db.select().from(user).where(eq(user.id, userId)) and the existing targetUser handling) unchanged.
112-120: N+1 query pattern for orphan detection.Each DM channel is individually queried for members. For a seed script this is acceptable, but it could be a single query with a
LEFT JOIN/ subquery to find channels with zero members.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/scripts/seed-dms.ts` around lines 112 - 120, The current loop over orphaned channels issues an N+1 query by calling db.select per channel; replace this with a single query that deletes all channels with zero members in one operation: use the channel and channelMember symbols and build a single DELETE ... WHERE channel.id IN (SELECT channel.id FROM channel LEFT JOIN channelMember ON channel.id = channelMember.channelId WHERE channel.id IN (orphaned ids) GROUP BY channel.id HAVING COUNT(channelMember.id) = 0) or an equivalent NOT EXISTS subquery; update the code that currently iterates `for (const ch of orphaned)` to call this single delete via the db instance to remove all empty channels at once.apps/api/src/app.ts (1)
39-53: Remove stale commented-out route-mounting code.The block at Lines 39–53 is a leftover from a previous implementation and is now completely superseded by the chained
.route()approach above it. It also references onlywaitlistRouterandchannelsRouter—leaving outdmsRouter—so re-enabling it as-is would silently drop the DMs API surface.✂️ Proposed cleanup
-// // Internal routes (not versioned) -// const internalRoutes = [waitlistRouter] as const -// for (const route of internalRoutes) { -// app.route("/", route) -// } -// -// // Versioned public API routes -// const v1Routes = [channelsRouter] as const -// for (const route of v1Routes) { -// app.route("/v1", route) -// } -// -// const allRoutes = [...internalRoutes, ...v1Routes] as const -// -// export type AppType = (typeof allRoutes)[number]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app.ts` around lines 39 - 53, Remove the stale commented-out route-mounting block that defines internalRoutes, v1Routes, allRoutes, and the AppType alias (the block that references waitlistRouter and channelsRouter) since the code now uses the chained .route() approach and the commented code would omit dmsRouter; delete this entire commented section to avoid confusion and duplication, ensuring routing is only configured via the existing chained app.route(...) calls and no dead/commented route declarations remain.apps/web/src/components/sidebar/channel-panel/channel-panel.tsx (1)
12-12: Nit:px-3 pl-1is misleading — prefer explicitpr-3 pl-1.
px-3sets both sides to0.75rem, thenpl-1immediately overrides the left to0.25rem. The net result is correct (pr-3 pl-1), but the intent is clearer without the conflicting shorthand.✨ Proposed cleanup
- <ScrollArea className="flex-1 px-3 pl-1 pt-3"> + <ScrollArea className="flex-1 pr-3 pl-1 pt-3">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sidebar/channel-panel/channel-panel.tsx` at line 12, The ScrollArea component's className uses the conflicting shorthand "px-3 pl-1" which sets both padding sides then immediately overrides the left; update the className on the ScrollArea in channel-panel.tsx to use explicit "pr-3 pl-1" (keep pl-1) so intent is clear and no shorthand is overridden, e.g., change the className value on the ScrollArea element to replace "px-3" with "pr-3".apps/api/src/routes/v1/channels/handlers.ts (1)
101-108: Consider parallelizing updates inside the transaction or using a single batch statement.The current
for...of+awaitpattern issues N sequential round-trips to the database. For typical guild channel counts this is fine, but a singlePromise.allover the update promises (or a bulkCASE WHENupdate) would be more efficient.♻️ Proposed refactor (parallel updates)
await db.transaction(async (tx) => { - for (const update of updates) { - await tx - .update(channel) - .set({ position: update.position, parentId: update.parentId }) - .where(and(eq(channel.id, update.id), eq(channel.guildId, guild.id))) - } + await Promise.all( + updates.map((update) => + tx + .update(channel) + .set({ position: update.position, parentId: update.parentId }) + .where(and(eq(channel.id, update.id), eq(channel.guildId, guild.id))) + ) + ) })Note: if the ORM serializes statements within a single connection anyway, the gain may be negligible, but the intent is clearer.
🤖 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 101 - 108, The transaction loop issues updates sequentially; inside the db.transaction callback, collect each tx.update(channel).set(...).where(...) call into an array of promises and run them in parallel with Promise.all (i.e., map over updates to create promises and await Promise.all(promises)) or replace with a single bulk update (CASE WHEN) if supported; target the db.transaction closure, the tx.update(channel).set(...) calls, the updates array and the guild.id/channel.id predicates when implementing the change.apps/api/src/routes/v1/dms/handlers.ts (2)
109-144: Consider filtering out the current user at the DB level.The members query (line 121) fetches all members for each DM channel, then the loop on line 134 skips the current user in JS. Adding
ne(channelMember.userId, currentUser.id)to theWHEREclause would reduce rows transferred from the database.♻️ Proposed change
.from(channelMember) .innerJoin(user, eq(channelMember.userId, user.id)) - .where(inArray(channelMember.channelId, dmChannelIds)) + .where( + and( + inArray(channelMember.channelId, dmChannelIds), + ne(channelMember.userId, currentUser.id) + ) + )This would also let you drop the
if (m.id === currentUser.id) continueguard on line 134. You'd need to importnefromdrizzle-orm.🤖 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 109 - 144, The members query currently fetches all channel members then filters out currentUser in JS; change the DB query in the members selection (the query building around members, channelMember, and user) to add ne(channelMember.userId, currentUser.id) to the .where(...) (alongside the existing inArray(channelMember.channelId, dmChannelIds)), import ne from drizzle-orm, and then remove the runtime guard if (m.id === currentUser.id) in the loop that builds membersByChannel to avoid transferring and processing the current user's row.
61-89: Fetching all messages for dedup is a scalability concern.This query retrieves every message across all page channels, then deduplicates client-side to keep only the latest per channel. For active DM channels with thousands of messages, this will transfer a large number of rows from the database only to discard all but one per channel.
The comment on line 62 says "Uses DISTINCT ON" but the actual query does not — it performs client-side filtering via a
Set.Consider pushing the dedup to the database. For example, a subquery or Drizzle's
sqlhelper with a window function /DISTINCT ON(if using PostgreSQL):♻️ Sketch: DB-side latest-message-per-channel
SELECT DISTINCT ON (channel_id) channel_id, id, content, created_at, author_id FROM message WHERE channel_id = ANY($1) ORDER BY channel_id, created_at DESCIn Drizzle you can use
db.execute(sql\...`)or build a subquery. Alternatively, add.limit(dmChannelIds.length)as a rough cap won't work here, but a raw SQLDISTINCT ON` or a lateral join will.🤖 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 61 - 89, The current implementation builds dmChannelIds from dmChannels then queries message joined to user returning all rows and uses a client-side Set in the latestMessages post-processing to keep only the most recent message per channel; replace this client-side dedup with a DB-side "latest-per-channel" query (use PostgreSQL DISTINCT ON or a window function) so the database returns one row per channel directly (e.g. use db.execute(sql`...`) or a Drizzle subquery/lateral join) and remove the rows.filter(...) dedup; update usages of dmChannelIds, latestMessages, message and user selections accordingly to still include author fields but ensure the ORDER BY/DISTINCT ON or ROW_NUMBER() OVER (PARTITION BY message.channelId ORDER BY message.createdAt DESC) is applied in the SQL so only the newest message per channel is retrieved.apps/web/src/components/sidebar/channel-panel/channel-list.tsx (3)
183-195: VariableactiveItemshadows the component-level state.The local
const activeItemon line 189 shadows theactiveItemstate declared on line 147. While functionally harmless (the callback uses the freshly-found value), this can confuse readers and is flagged by many linters.Consider renaming to something like
foundActiveordraggedItem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx` around lines 183 - 195, The local const activeItem in the handleDragOver callback shadows the component-level activeItem state; rename the local variable returned by findChannel(active.id as string) (and similarly overItem if desired) to something like draggedItem (and targetItem) and update all references inside handleDragOver to use those new names so linters and readers won't confuse the local lookup with the component state; locate this in the handleDragOver function where findChannel is called and replace references to activeItem/overItem inside that callback.
128-145: NoonSuccesshandler — optimistic state may drift from server.The
reorderMutationonly invalidates on error. If the server silently rejects or adjusts positions (e.g., due to a concurrent edit), the client's optimistic state will diverge. Consider invalidating ononSettledto always resync after the mutation completes, or at minimum verifying the server response on success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx` around lines 128 - 145, The mutation reorderMutation currently only invalidates via onError which can leave client state out-of-sync; add an onSettled (or onSuccess) handler on the useMutation config to always call queryClient.invalidateQueries({ queryKey: ["channels", guildSlug] }) after the request completes (or additionally verify and reconcile the returned payload on success), so that the channels list in the cache is refreshed regardless of silent server-side adjustments; locate the useMutation call for reorderMutation and update its config to include onSettled (or onSuccess) that invalidates the ["channels", guildSlug] query key.
434-511: Category collapse toggle also triggers drag — potential UX conflict.The category header
<button>has both anonClickhandler (toggle collapse) and{...listeners}(drag listeners) on the same element. A click intended to collapse/expand may inadvertently start a drag, or vice versa. ThePointerSensoractivation constraint ofdistance: 5(line 154) provides a small buffer, but quick clicks on touch devices or shaky hands could still trigger unintended drags.Consider separating the drag handle from the collapse toggle (e.g., a dedicated grip icon for dragging) if this becomes a UX problem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx` around lines 434 - 511, The category header button in SortableCategorySection currently mixes collapse toggle (onClick={() => setCollapsed(!collapsed)}) with drag listeners ({...attributes} {...listeners}), causing clicks to sometimes start a drag; separate concerns by moving {...attributes} and {...listeners} off the main collapse toggle and onto a dedicated drag-handle element (e.g., a small grip/icon next to the Chevron or replace Chevron with two elements: one for collapse with onClick only, and one for dragging with the listeners), retain the PointerSensor activation constraint but ensure the collapse button has only the toggle behavior and proper aria-labels for accessibility; update SortableCategorySection to reference the new drag-handle element for the sortable set-up.
🤖 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/scripts/seed-dms.ts`:
- Around line 108-121: The cleanup only deletes channels with channel.type ===
"dm", leaving seeded group DMs behind; update the orphan query to also select
group_dm channels that belong to the previous seed by adding an OR condition:
include channels where channel.type === "group_dm" AND (channel.name matches the
seed names list OR channel.ownerId equals the seed user id), then run the same
member-count check (using channelMember where eq(channelMember.channelId,
ch.id)) and delete if members.length === 0 (or delete directly for known
seed-owned group_dm). Refer to the channel, channel.type, channel.name,
channel.ownerId, channel.id, and channelMember/channelMember.channelId symbols
when implementing the additional filter.
In `@apps/api/src/routes/v1/channels/handlers.ts`:
- Around line 86-99: The rejection happens because channelIds is built from
updates.map(u => u.id) and can contain duplicates while the inArray SQL check
(inArray(channel.id, channelIds)) deduplicates, so existing.length can be
smaller than channelIds.length causing a false 403; fix by deduplicating the IDs
before comparing (e.g., compute a set/uniqueIds from channelIds and compare
existing.length to uniqueIds.size) or enforce uniqueness earlier by adding a Zod
refinement to reorderChannelsRequestSchema to reject duplicate ids; update
references to channelIds, existing and the inArray check accordingly.
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx`:
- Around line 275-324: The setQueryData updater currently performs a side-effect
by calling reorderMutation.mutate(payload) inside the callback; move the API
call out of the pure updater: compute newData inside the
queryClient.setQueryData callback (as currently done), return newData, then
immediately after the setQueryData call build the payload with
buildReorderPayload(newData) and call reorderMutation.mutate(payload). Ensure
you reference the same variables (newData, payload) and functions
(queryClient.setQueryData, buildReorderPayload, reorderMutation.mutate) so the
mutation is invoked exactly once outside the updater.
In `@apps/web/src/components/sidebar/channel-panel/guild-header.tsx`:
- Around line 10-11: Destructure isPending (or isLoading) from the useQuery call
that fetches guilds and stop using guildName ?? "Loading..."; instead, when
rendering the guild title (where guildName and guildSlug are used) show
"Loading…" only while isPending is true, and after data resolves if guildName is
undefined render a clear fallback like "Guild not found" (apply the same change
for the other occurrences around the guild header/title rendering and the
secondary usage). Ensure you reference the useQuery call that returns { data:
guilds } and the variable guildName (derived from guilds.find by guildSlug) when
implementing the conditional.
- Around line 12-14: The queryFn in guild-header.tsx incorrectly treats the
authClient.organization.list() response as success even when res.error is set;
update queryFn to check the returned object from authClient.organization.list(),
and if res.error exists throw that error (or throw new Error(res.error.message
|| String(res.error))) so React Query receives a rejected promise and moves to
the error state; keep returning res.data when there is no error.
In `@apps/web/src/components/sidebar/channel-panel/user-bar.tsx`:
- Around line 30-32: The handleLogout function currently calls
authClient.signOut() without handling rejections; update handleLogout to catch
errors from authClient.signOut(), log the error (use existing logger or
console.error) and surface user feedback (e.g., show a toast/error banner) so
the user knows sign-out failed; also consider setting a local loading state
(e.g., isSigningOut) to disable UI while the call is in-flight and only close
the dropdown or update session state on success. Ensure you modify the
handleLogout function to await authClient.signOut() inside a try/catch and
handle both success and failure paths.
In `@apps/web/src/components/sidebar/dm-panel/dm-list.tsx`:
- Line 143: The close icon (<X />) in dm-list.tsx is rendered with no click
handler, so add an interactive handler: wire an onClick on the <X /> element to
call the DM removal function (e.g., handleRemoveDM or removeConversation)
passing the conversation id, or call a provided prop like
onRemoveDM(conversationId); ensure the handler stops event propagation and
triggers the conversation deletion/UI update (or, if removal logic isn't
available yet, remove the <X /> from the JSX to avoid a non-functional control).
In `@apps/web/src/components/sidebar/dm-panel/dm-panel.tsx`:
- Around line 14-19: The Plus button in dm-panel.tsx is missing accessibility
and interaction: add an aria-label (e.g., aria-label="Create direct message" or
similar) to the button element and wire a click handler (either a real handler
or a temporary no-op like onClick={() => {}}) so it is keyboard/assistive-tech
accessible and not inert; update the button JSX where the <Plus .../> icon is
rendered to include these props and, if applicable, call or forward to an
existing handler function (e.g., openNewDm, handleCreateDm) in the component.
In `@packages/auth/src/lib/auth.ts`:
- Around line 22-27: The cookieCache configuration under session currently sets
maxAge to 5*60 which allows revoked sessions to remain valid for up to 5
minutes; change this by either lowering session.cookieCache.maxAge to a safer
value (e.g., 60) or set session.cookieCache.disableCookieCache: true for
sensitive admin routes; alternatively, keep a short global maxAge and explicitly
set disableCookieCache: true in request options for functions handling bans/role
revocations so session revocation is effective immediately (refer to the
session, cookieCache, maxAge and disableCookieCache symbols).
In `@packages/ui/src/components/skeleton.tsx`:
- Around line 1-3: The type reference React.ComponentProps<"div"> in the
Skeleton component causes a TS error because React is not imported; either add
an import for React (e.g., import React from "react" or import type React from
"react") so React is in scope for type references, or replace the type with a
React-free alternative such as JSX.IntrinsicElements["div"] or
React.ComponentPropsWithoutRef<"div"> with an explicit import of just the type
if needed; update the function signature for Skeleton accordingly (reference
symbols: Skeleton, React.ComponentProps<"div">).
In `@packages/ui/src/components/toggle-group.tsx`:
- Around line 36-40: The Tailwind arbitrary value for gap is invalid: in the
ToggleGroup component, update the className string where it currently contains
"gap-[--spacing(var(--gap))]" to use the correct custom property syntax
"gap-(--gap)" so Tailwind outputs gap: var(--gap); (alternatively compute the
resolved pixel value in the style prop before passing into the inline style).
Ensure you change the className token in the toggle-group component so it
matches the CSS custom property used in the style prop (the spacing variable set
via style={{ "--gap": spacing } as React.CSSProperties}).
---
Outside diff comments:
In `@apps/api/src/routes/v1/channels/routes.ts`:
- Around line 19-38: The route OpenAPI responses are missing a 404 for cases
where guildAuthMiddleware returns Not Found; update the responses object for
listChannels (and likewise for createChannel and reorderChannels) to include
[HttpStatusCodes.NOT_FOUND]: notFoundSchema so the generated spec documents the
middleware’s possible 404 outcome; locate the responses block in each
createRoute call (symbols: listChannels, createChannel, reorderChannels) and add
that entry alongside the existing
OK/UNAUTHORIZED/FORBIDDEN/INTERNAL_SERVER_ERROR entries.
---
Duplicate comments:
In `@apps/web/src/routes/login.tsx`:
- Around line 23-31: The session-redirect useEffect in login.tsx duplicates
logic from signup.tsx; extract that logic into a reusable hook (e.g.,
useRedirectIfAuthenticated) that internally calls authClient.useSession() and
perform navigate({ to: "/" }) when session exists and sessionPending is false,
then replace the local useEffect and direct useSession usage in login.tsx with a
call to useRedirectIfAuthenticated(); ensure the hook exports a clear name and
is imported/used in both login.tsx and signup.tsx so the duplicated useEffect is
removed.
---
Nitpick comments:
In `@apps/api/scripts/seed-dms.ts`:
- Around line 84-91: The seed function currently queries the DB with userId
without validating it; add a UUID format check for the CLI argument (userId)
before calling db.select() in seed() so invalid strings produce a friendly error
instead of a DB failure. Implement the check right before the existing select
(or at the start of seed()), using a UUID validation helper (e.g., a regex or a
library function) to verify userId, and if invalid call console.error with a
clear message and process.exit(1); keep the rest of the logic (the
db.select().from(user).where(eq(user.id, userId)) and the existing targetUser
handling) unchanged.
- Around line 112-120: The current loop over orphaned channels issues an N+1
query by calling db.select per channel; replace this with a single query that
deletes all channels with zero members in one operation: use the channel and
channelMember symbols and build a single DELETE ... WHERE channel.id IN (SELECT
channel.id FROM channel LEFT JOIN channelMember ON channel.id =
channelMember.channelId WHERE channel.id IN (orphaned ids) GROUP BY channel.id
HAVING COUNT(channelMember.id) = 0) or an equivalent NOT EXISTS subquery; update
the code that currently iterates `for (const ch of orphaned)` to call this
single delete via the db instance to remove all empty channels at once.
In `@apps/api/src/app.ts`:
- Around line 39-53: Remove the stale commented-out route-mounting block that
defines internalRoutes, v1Routes, allRoutes, and the AppType alias (the block
that references waitlistRouter and channelsRouter) since the code now uses the
chained .route() approach and the commented code would omit dmsRouter; delete
this entire commented section to avoid confusion and duplication, ensuring
routing is only configured via the existing chained app.route(...) calls and no
dead/commented route declarations remain.
In `@apps/api/src/lib/helpers/openapi/schemas.ts`:
- Line 2: The file imports ZodType directly from "zod", which can cause
module-instance drift versus the `z` export used from `@hono/zod-openapi`;
change the code to derive ZodType from the same `z` export used elsewhere in the
file (e.g. import the type from the same module that provides `z` such as
`@hono/zod-openapi`, or reference it as `z.ZodType`) so that all schema types
share the same Zod instance; update the import line and any type references to
use that unified `ZodType` symbol (e.g., replace `import type { ZodType } from
"zod"` with an import from the module that exports `z` or use `z.ZodType`).
In `@apps/api/src/middleware/session-auth.ts`:
- Around line 16-32: Wrap the call to auth.api.getSession inside
sessionAuthMiddleware in a try/catch so any thrown errors are handled;
specifically, call auth.api.getSession({ headers: c.req.raw.headers }) inside a
try block, and in the catch log the error (using your app logger or
console.error) and return a consistent c.json response with
HttpStatusCodes.INTERNAL_SERVER_ERROR and a short message (e.g. "Authentication
error"); preserve the existing behavior for when session is null (return
UNAUTHORIZED) and continue to set c.set("user", session.user) and
c.set("session", session.session) when successful.
In `@apps/api/src/routes/v1/channels/handlers.ts`:
- Around line 101-108: The transaction loop issues updates sequentially; inside
the db.transaction callback, collect each tx.update(channel).set(...).where(...)
call into an array of promises and run them in parallel with Promise.all (i.e.,
map over updates to create promises and await Promise.all(promises)) or replace
with a single bulk update (CASE WHEN) if supported; target the db.transaction
closure, the tx.update(channel).set(...) calls, the updates array and the
guild.id/channel.id predicates when implementing the change.
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 109-144: The members query currently fetches all channel members
then filters out currentUser in JS; change the DB query in the members selection
(the query building around members, channelMember, and user) to add
ne(channelMember.userId, currentUser.id) to the .where(...) (alongside the
existing inArray(channelMember.channelId, dmChannelIds)), import ne from
drizzle-orm, and then remove the runtime guard if (m.id === currentUser.id) in
the loop that builds membersByChannel to avoid transferring and processing the
current user's row.
- Around line 61-89: The current implementation builds dmChannelIds from
dmChannels then queries message joined to user returning all rows and uses a
client-side Set in the latestMessages post-processing to keep only the most
recent message per channel; replace this client-side dedup with a DB-side
"latest-per-channel" query (use PostgreSQL DISTINCT ON or a window function) so
the database returns one row per channel directly (e.g. use db.execute(sql`...`)
or a Drizzle subquery/lateral join) and remove the rows.filter(...) dedup;
update usages of dmChannelIds, latestMessages, message and user selections
accordingly to still include author fields but ensure the ORDER BY/DISTINCT ON
or ROW_NUMBER() OVER (PARTITION BY message.channelId ORDER BY message.createdAt
DESC) is applied in the SQL so only the newest message per channel is retrieved.
In `@apps/api/src/routes/v1/dms/schema.ts`:
- Around line 5-26: lastMessageAuthorSchema and dmMemberSchema duplicate the
same shape; extract a shared base schema (e.g., userBaseSchema or
memberBaseSchema) that defines id, name, username, displayUsername, image and
then reuse it to build lastMessageAuthorSchema and dmMemberSchema via zod's
.extend() or by assigning them to the shared schema to remove duplication;
alternatively, if you intend them to diverge later, add a brief comment above
both schemas explaining the intentional duplication so future maintainers know
it's deliberate.
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx`:
- Around line 183-195: The local const activeItem in the handleDragOver callback
shadows the component-level activeItem state; rename the local variable returned
by findChannel(active.id as string) (and similarly overItem if desired) to
something like draggedItem (and targetItem) and update all references inside
handleDragOver to use those new names so linters and readers won't confuse the
local lookup with the component state; locate this in the handleDragOver
function where findChannel is called and replace references to
activeItem/overItem inside that callback.
- Around line 128-145: The mutation reorderMutation currently only invalidates
via onError which can leave client state out-of-sync; add an onSettled (or
onSuccess) handler on the useMutation config to always call
queryClient.invalidateQueries({ queryKey: ["channels", guildSlug] }) after the
request completes (or additionally verify and reconcile the returned payload on
success), so that the channels list in the cache is refreshed regardless of
silent server-side adjustments; locate the useMutation call for reorderMutation
and update its config to include onSettled (or onSuccess) that invalidates the
["channels", guildSlug] query key.
- Around line 434-511: The category header button in SortableCategorySection
currently mixes collapse toggle (onClick={() => setCollapsed(!collapsed)}) with
drag listeners ({...attributes} {...listeners}), causing clicks to sometimes
start a drag; separate concerns by moving {...attributes} and {...listeners} off
the main collapse toggle and onto a dedicated drag-handle element (e.g., a small
grip/icon next to the Chevron or replace Chevron with two elements: one for
collapse with onClick only, and one for dragging with the listeners), retain the
PointerSensor activation constraint but ensure the collapse button has only the
toggle behavior and proper aria-labels for accessibility; update
SortableCategorySection to reference the new drag-handle element for the
sortable set-up.
In `@apps/web/src/components/sidebar/channel-panel/channel-panel.tsx`:
- Line 12: The ScrollArea component's className uses the conflicting shorthand
"px-3 pl-1" which sets both padding sides then immediately overrides the left;
update the className on the ScrollArea in channel-panel.tsx to use explicit
"pr-3 pl-1" (keep pl-1) so intent is clear and no shorthand is overridden, e.g.,
change the className value on the ScrollArea element to replace "px-3" with
"pr-3".
In `@apps/web/src/components/sidebar/dm-panel/dm-list.tsx`:
- Around line 65-74: The initials logic in getInitials duplicates the same
computation used by UserAvatar; extract that logic into a shared utility
function (e.g., export function computeInitials(name: string | null): string)
and replace the local getInitials in dm-list.tsx and the inline logic in
apps/web/src/components/ui/user-avatar.tsx to import and call computeInitials
instead, keeping its behavior (split, take first char of each word, join,
slice(0,2), uppercase, default "?") and update imports/exports accordingly.
- Around line 28-34: The current early return treats undefined `data` (while
useQuery is loading) the same as an empty result; update the rendering logic in
the DM list component that calls useQuery so it reads the hook's isLoading
(and/or isFetching) flag and, when true, renders a loading UI (spinner or
skeleton) instead of the "No conversations yet." message, and only show the
empty-state message when isLoading is false and data?.data?.length === 0;
reference the useQuery call and its isLoading return value to implement this
conditional.
In `@apps/web/src/routes/signup.tsx`:
- Around line 23-33: Extract the duplicate session-redirect logic into a new
hook (e.g., useRedirectIfAuthenticated) that calls authClient.useSession(),
checks sessionPending and session, and calls navigate({ to: "/" }) when session
is non-pending and truthy; then replace the useEffect block in SignupPage
(signup.tsx) and LoginPage (login.tsx) with a call to useRedirectIfAuthenticated
so both files use the shared hook. Ensure the hook exports a default or named
function and imports navigate from the same router used in the existing files so
behavior remains identical.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/web/src/components/sidebar/dm-panel/dm-list.tsx (2)
64-73:getInitialsduplicates the initials logic already insideUserAvatar
apps/web/src/components/ui/user-avatar.tsxcomputes initials with identicalsplit / map / join / slice / toUpperCaselogic. SinceGroupDMAvatarsuses raw<Avatar>elements instead of<UserAvatar>, the logic is duplicated. Consider exporting a sharedgetInitialsutility fromuser-avatar.tsx(or a shared utils file) and importing it here.🤖 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/dm-list.tsx` around lines 64 - 73, The initials logic in getInitials is duplicated from UserAvatar; to fix, move/export the initials helper from the UserAvatar implementation (e.g., export function getInitials from user-avatar.tsx or a small shared utils module) and replace the local getInitials in dm-list.tsx by importing that exported getInitials; update GroupDMAvatars (where raw <Avatar> elements are used) to call the shared getInitials, remove the local duplicate, and ensure the exported function signature accepts name: string | null and returns the same two-letter uppercase fallback "?" behavior.
12-12: Use the@/path alias for theUserAvatarimportThe rest of the file uses
@/for local app imports (e.g.,@/lib/api-clienton line 11). The relative../../ui/user-avataris inconsistent and fragile under any future directory restructuring.♻️ Proposed fix
-import { UserAvatar } from "../../ui/user-avatar" +import { UserAvatar } from "@/components/ui/user-avatar"🤖 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/dm-list.tsx` at line 12, Replace the relative import of UserAvatar with the project alias; in dm-list.tsx change the import "import { UserAvatar } from \"../../ui/user-avatar\"" to use the "@/..." alias (e.g., import { UserAvatar } from "@/components/ui/user-avatar") so it matches other imports like "@/lib/api-client" and is resilient to future refactors.apps/api/src/routes/v1/channels/handlers.ts (1)
104-110: Deduplication is not applied to the actual update loop — duplicates still cause redundant writes.
uniqueChannelIdsis used for the guild-membership check but the transaction still iterates over the originalupdatesarray. A payload with duplicate IDs (e.g., the same channel appearing twice with different positions) passes verification and then executes multipleUPDATEstatements for the same row. The last write silently wins, which is non-deterministic from the caller's perspective and wastes DB round trips.The cleanest fix is to enforce uniqueness at the schema level so duplicates are rejected before reaching this handler:
🐛 Option A — Zod refinement on the schema (preferred)
Add a refinement to
reorderChannelsRequestSchemainschema.ts:export const reorderChannelsRequestSchema = z.object({ channels: z.array( z.object({ id: z.string().uuid(), position: z.number().int().nonnegative(), parentId: z.string().uuid().nullable().optional(), }) - ), + ).refine( + (items) => new Set(items.map((i) => i.id)).size === items.length, + { message: "Duplicate channel IDs are not allowed" } + ), })🐛 Option B — Deduplicate inside the handler
const channelIds = updates.map((u) => u.id) const uniqueChannelIds = [...new Set(channelIds)] + // Deduplicate updates so each channel is written at most once + const uniqueUpdates = [ + ...new Map(updates.map((u) => [u.id, u])).values(), + ] + // Verify all channels belong to this guild const existing = await db .select({ id: channel.id }) .from(channel) .where( and(eq(channel.guildId, guild.id), inArray(channel.id, uniqueChannelIds)) ) if (existing.length !== uniqueChannelIds.length) { … } await db.transaction(async (tx) => { - for (const update of updates) { + for (const update of uniqueUpdates) { await tx .update(channel) .set({ position: update.position, parentId: update.parentId }) .where(and(eq(channel.id, update.id), eq(channel.guildId, guild.id))) } })🤖 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 104 - 110, The handler currently validates membership using uniqueChannelIds but still iterates over the original updates array, causing duplicate UPDATEs; fix by enforcing uniqueness at parse time (preferred) or deduplicating before the transaction: add a Zod refinement to reorderChannelsRequestSchema (in schema.ts) that checks all channel ids are unique and returns a validation error if duplicates exist, or alternatively in the handler create a dedupedUpdates array/Map keyed by update.id (and a deduped uniqueChannelIds set) and use that deduped collection in the db.transaction loop where tx.update(channel).set(...).where(...) is called so each channel id is updated exactly once.apps/web/src/components/sidebar/channel-panel/channel-list.tsx (1)
113-145:guildSlug as stringsilently passesundefinedto the API when the param is absent.
useParams({ strict: false })returnsguildSlugasstring | undefined. Casting withas string(lines 117, 135) bypasses TypeScript's protection — ifguildSlugis undefined, the API call targets the literal string"undefined". ForuseQuerytheenabled: !!guildSlugguard is in place, butreorderMutation.mutationFnhas no equivalent guard; it relies onhandleDragEnd's!dataearly return being a reliable proxy.A more explicit alternative:
♻️ Proposed refactor
queryFn: async () => { + if (!guildSlug) throw new Error("guildSlug is required") const res = await apiClient.v1.guilds[":guildSlug"].channels.$get({ - param: { guildSlug: guildSlug as string }, + param: { guildSlug }, })mutationFn: async (channels: ...) => { + if (!guildSlug) throw new Error("guildSlug is required") const res = await apiClient.v1.guilds[":guildSlug"].channels.reorder.$patch({ - param: { guildSlug: guildSlug as string }, + param: { guildSlug }, json: { channels }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx` around lines 113 - 145, The mutation and query currently cast guildSlug with "as string" which can silently pass undefined to the API; update both the useQuery queryFn and reorderMutation.mutationFn to explicitly check for guildSlug before calling apiClient (e.g. if (!guildSlug) throw or return early), remove the unsafe "as string" casts, and ensure reorderMutation rejects/throws when guildSlug is missing so the API never receives the literal "undefined" (refer to useQuery queryFn, reorderMutation.mutationFn, guildSlug, and apiClient.v1.guilds[":guildSlug"].channels.reorder).
🤖 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/channels/handlers.ts`:
- Around line 104-111: The global error handler in
apps/api/src/middleware/on-error.ts currently returns { message, stack? } which
violates the route schema expected by reorderChannels; update the handler to
always include success: false in the JSON response (e.g., return { success:
false, message: err.message, stack?: err.stack }), preserving the optional stack
field and existing status code logic so any DB errors thrown in reorderChannels
still produce a response that matches the declared INTERNAL_SERVER_ERROR schema.
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx`:
- Around line 265-269: Add an onDragCancel handler and guard the !over path so
optimistic updates from handleDragOver get reverted: implement onDragCancel on
the DndContext to call the same revert logic you use after successful reorder
(use queryClient.invalidateQueries or reset the cached data set by
queryClient.setQueryData) and clear activeItem, and update handleDragEnd to,
when !over, invalidate/reset the query before returning (so
reorderMutation.mutate is not skipped leaving stale optimistic state); refer to
handleDragEnd, handleDragOver, reorderMutation.mutate, queryClient.setQueryData
and ensure the cancel handler mirrors the cleanup performed after successful
mutations.
In `@apps/web/src/components/sidebar/dm-panel/dm-list.tsx`:
- Around line 18-33: The empty-state "No conversations yet." shows during
initial fetch because the component only checks !data; update the useQuery call
in dm-list.tsx to destructure the loading flag (isLoading or isFetching)
alongside data from useQuery, and change the rendering logic so that while
isLoading is true you render a spinner/skeleton placeholder, and only render the
"No conversations yet." message when isLoading is false and data?.data.length
=== 0; reference the existing useQuery invocation and the data variable to
locate where to add the isLoading/isFetching check and replace the early-return
empty-state.
---
Duplicate comments:
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx`:
- Around line 275-326: The updater passed to queryClient.setQueryData is impure
because it assigns to the outer newData variable; make it pure by removing the
newData assignment inside the setQueryData callback and instead call
queryClient.getQueryData(["channels", guildSlug]) immediately after setQueryData
to read the updated ChannelData, then call
reorderMutation.mutate(buildReorderPayload(updatedData)). Remove the newData
variable entirely and ensure all mutation logic uses the value returned by
getQueryData so setQueryData's updater has no side effects.
---
Nitpick comments:
In `@apps/api/src/routes/v1/channels/handlers.ts`:
- Around line 104-110: The handler currently validates membership using
uniqueChannelIds but still iterates over the original updates array, causing
duplicate UPDATEs; fix by enforcing uniqueness at parse time (preferred) or
deduplicating before the transaction: add a Zod refinement to
reorderChannelsRequestSchema (in schema.ts) that checks all channel ids are
unique and returns a validation error if duplicates exist, or alternatively in
the handler create a dedupedUpdates array/Map keyed by update.id (and a deduped
uniqueChannelIds set) and use that deduped collection in the db.transaction loop
where tx.update(channel).set(...).where(...) is called so each channel id is
updated exactly once.
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx`:
- Around line 113-145: The mutation and query currently cast guildSlug with "as
string" which can silently pass undefined to the API; update both the useQuery
queryFn and reorderMutation.mutationFn to explicitly check for guildSlug before
calling apiClient (e.g. if (!guildSlug) throw or return early), remove the
unsafe "as string" casts, and ensure reorderMutation rejects/throws when
guildSlug is missing so the API never receives the literal "undefined" (refer to
useQuery queryFn, reorderMutation.mutationFn, guildSlug, and
apiClient.v1.guilds[":guildSlug"].channels.reorder).
In `@apps/web/src/components/sidebar/dm-panel/dm-list.tsx`:
- Around line 64-73: The initials logic in getInitials is duplicated from
UserAvatar; to fix, move/export the initials helper from the UserAvatar
implementation (e.g., export function getInitials from user-avatar.tsx or a
small shared utils module) and replace the local getInitials in dm-list.tsx by
importing that exported getInitials; update GroupDMAvatars (where raw <Avatar>
elements are used) to call the shared getInitials, remove the local duplicate,
and ensure the exported function signature accepts name: string | null and
returns the same two-letter uppercase fallback "?" behavior.
- Line 12: Replace the relative import of UserAvatar with the project alias; in
dm-list.tsx change the import "import { UserAvatar } from
\"../../ui/user-avatar\"" to use the "@/..." alias (e.g., import { UserAvatar }
from "@/components/ui/user-avatar") so it matches other imports like
"@/lib/api-client" and is resilient to future refactors.
Comment on lines
+104
to
+111
| await db.transaction(async (tx) => { | ||
| for (const update of updates) { | ||
| await tx | ||
| .update(channel) | ||
| .set({ position: update.position, parentId: update.parentId }) | ||
| .where(and(eq(channel.id, update.id), eq(channel.guildId, guild.id))) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for global error handler in the API
find apps/api -name "*.ts" -o -name "*.tsx" | head -20Repository: BuckyMcYolo/townhall
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Look for error handler patterns - onError, middleware, etc.
rg -n "onError|app\.use.*error|errorHandler|catch.*Error" apps/api/src --type ts -B 2 -A 5 | head -100Repository: BuckyMcYolo/townhall
Length of output: 2110
🏁 Script executed:
#!/bin/bash
# Check the main app setup/entry point
find apps/api/src -name "index.ts" -o -name "main.ts" -o -name "app.ts" -o -name "server.ts"Repository: BuckyMcYolo/townhall
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Look at the route file to see the schema definition
cat -n apps/api/src/routes/v1/channels/handlers.ts | head -150Repository: BuckyMcYolo/townhall
Length of output: 3793
🏁 Script executed:
cat -n apps/api/src/middleware/on-error.tsRepository: BuckyMcYolo/townhall
Length of output: 757
🏁 Script executed:
find apps/api/src/routes/v1/channels -name "*.ts" | xargs cat -nRepository: BuckyMcYolo/townhall
Length of output: 9578
🏁 Script executed:
cat -n apps/api/src/lib/helpers/openapi/schemas.tsRepository: BuckyMcYolo/townhall
Length of output: 1990
Global error handler violates the declared schema response contract.
The reorderChannels transaction (lines 104–111) propagates unhandled errors to the global Hono error handler at apps/api/src/middleware/on-error.ts. However, this handler returns { message, stack? } while the route's INTERNAL_SERVER_ERROR response schema requires { success: boolean, message: string }. Any DB error thrown during the transaction will produce a malformed response that fails OpenAPI schema validation. The error handler must be updated to include the success field in its response body to match the declared schema.
🤖 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 104 - 111, The
global error handler in apps/api/src/middleware/on-error.ts currently returns {
message, stack? } which violates the route schema expected by reorderChannels;
update the handler to always include success: false in the JSON response (e.g.,
return { success: false, message: err.message, stack?: err.stack }), preserving
the optional stack field and existing status code logic so any DB errors thrown
in reorderChannels still produce a response that matches the declared
INTERNAL_SERVER_ERROR schema.
Comment on lines
+265
to
+269
| const handleDragEnd = useCallback( | ||
| (event: DragEndEvent) => { | ||
| setActiveItem(null) | ||
| const { active, over } = event | ||
| if (!over || !data || active.id === over.id) return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/components/sidebar/channel-panel/channel-list.tsx | head -100Repository: BuckyMcYolo/townhall
Length of output: 3473
🏁 Script executed:
cat -n apps/web/src/components/sidebar/channel-panel/channel-list.tsx | sed -n '260,280p'Repository: BuckyMcYolo/townhall
Length of output: 856
🏁 Script executed:
cat -n apps/web/src/components/sidebar/channel-panel/channel-list.tsx | sed -n '340,370p'Repository: BuckyMcYolo/townhall
Length of output: 1180
🏁 Script executed:
cat -n apps/web/src/components/sidebar/channel-panel/channel-list.tsx | sed -n '200,260p'Repository: BuckyMcYolo/townhall
Length of output: 2510
🏁 Script executed:
cat -n apps/web/src/components/sidebar/channel-panel/channel-list.tsx | sed -n '265-330p'Repository: BuckyMcYolo/townhall
Length of output: 115
🌐 Web query:
DnD Kit onDragCancel event handler documentation
💡 Result:
In dnd-kit (React) legacy @dnd-kit/core, onDragCancel is documented as a prop on <DndContext>:
- Signature (legacy docs):
onDragCancel?(): void(docs.dndkit.com) - When it fires: when the current drag operation is canceled (example given: user presses Escape while dragging). (docs.dndkit.com)
If you’re looking at the new “next” dnd-kit React API, note that onDragCancel is effectively replaced by a canceled boolean on dragEnd (so you handle cancellation inside onDragEnd). (next.dndkit.com)
Citations:
- 1: https://docs.dndkit.com/api-documentation/context-provider
- 2: https://docs.dndkit.com/api-documentation/context-provider
- 3: https://next.dndkit.com/react/hooks/use-drag-drop-monitor?utm_source=openai
🏁 Script executed:
sed -n '265,330p' apps/web/src/components/sidebar/channel-panel/channel-list.tsxRepository: BuckyMcYolo/townhall
Length of output: 2533
🏁 Script executed:
sed -n '350,360p' apps/web/src/components/sidebar/channel-panel/channel-list.tsxRepository: BuckyMcYolo/townhall
Length of output: 337
Add onDragCancel handler to prevent persistent cache corruption when drag operations are cancelled or dropped outside valid targets.
When dragging channels between containers, handleDragOver applies optimistic cache updates via queryClient.setQueryData(). However, two scenarios leave the cache in an inconsistent state:
- Drag cancelled:
DndContextfiresonDragCancelseparately fromonDragEnd, but no handler exists to revert the optimistic update. - Dropped outside target (
!over):handleDragEndreturns early at line 269, skippingreorderMutation.mutate(), but the optimistic cache mutation fromhandleDragOverpersists indefinitely.
Add onDragCancel handler and guard the !over case to invalidate the query:
Proposed fix
+ const handleDragCancel = useCallback(() => {
+ setActiveItem(null)
+ queryClient.invalidateQueries({ queryKey: ["channels", guildSlug] })
+ }, [guildSlug, queryClient])
const handleDragEnd = useCallback(
(event: DragEndEvent) => {
setActiveItem(null)
const { active, over } = event
- if (!over || !data || active.id === over.id) return
+ if (!data) return
+ if (!over || active.id === over.id) {
+ queryClient.invalidateQueries({ queryKey: ["channels", guildSlug] })
+ return
+ } <DndContext
sensors={sensors}
collisionDetection={closestCenter}
onDragStart={handleDragStart}
onDragOver={handleDragOver}
onDragEnd={handleDragEnd}
+ onDragCancel={handleDragCancel}
>Also applies to: 352–358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/sidebar/channel-panel/channel-list.tsx` around lines
265 - 269, Add an onDragCancel handler and guard the !over path so optimistic
updates from handleDragOver get reverted: implement onDragCancel on the
DndContext to call the same revert logic you use after successful reorder (use
queryClient.invalidateQueries or reset the cached data set by
queryClient.setQueryData) and clear activeItem, and update handleDragEnd to,
when !over, invalidate/reset the query before returning (so
reorderMutation.mutate is not skipped leaving stale optimistic state); refer to
handleDragEnd, handleDragOver, reorderMutation.mutate, queryClient.setQueryData
and ensure the cancel handler mirrors the cleanup performed after successful
mutations.
Comment on lines
+18
to
+33
| const { data } = useQuery({ | ||
| queryKey: ["dms"], | ||
| queryFn: async () => { | ||
| const res = await apiClient.v1.dms.$get({ query: {} }) | ||
| if (!res.ok) throw new Error("Failed to fetch DMs") | ||
| return res.json() | ||
| }, | ||
| }) | ||
|
|
||
| if (!data || data.data.length === 0) { | ||
| return ( | ||
| <div className="px-2 py-4 text-center text-sm text-muted-foreground"> | ||
| <p>No conversations yet.</p> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
"No conversations yet." flashes during initial load
!data is true while the query is still fetching, so the empty-state message is shown briefly on every mount before the DMs arrive. Destructure isPending (or isLoading) and render a skeleton/spinner instead.
💡 Proposed fix
- const { data } = useQuery({
+ const { data, isPending } = useQuery({
queryKey: ["dms"],
queryFn: async () => {
const res = await apiClient.v1.dms.$get({ query: {} })
if (!res.ok) throw new Error("Failed to fetch DMs")
return res.json()
},
})
+ if (isPending) {
+ return (
+ <div className="space-y-0.5 px-2 py-1">
+ {Array.from({ length: 3 }).map((_, i) => (
+ <div key={i} className="h-10 animate-pulse rounded-lg bg-foreground/[0.06]" />
+ ))}
+ </div>
+ )
+ }
+
if (!data || data.data.length === 0) {📝 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
| const { data } = useQuery({ | |
| queryKey: ["dms"], | |
| queryFn: async () => { | |
| const res = await apiClient.v1.dms.$get({ query: {} }) | |
| if (!res.ok) throw new Error("Failed to fetch DMs") | |
| return res.json() | |
| }, | |
| }) | |
| if (!data || data.data.length === 0) { | |
| return ( | |
| <div className="px-2 py-4 text-center text-sm text-muted-foreground"> | |
| <p>No conversations yet.</p> | |
| </div> | |
| ) | |
| } | |
| const { data, isPending } = useQuery({ | |
| queryKey: ["dms"], | |
| queryFn: async () => { | |
| const res = await apiClient.v1.dms.$get({ query: {} }) | |
| if (!res.ok) throw new Error("Failed to fetch DMs") | |
| return res.json() | |
| }, | |
| }) | |
| if (isPending) { | |
| return ( | |
| <div className="space-y-0.5 px-2 py-1"> | |
| {Array.from({ length: 3 }).map((_, i) => ( | |
| <div key={i} className="h-10 animate-pulse rounded-lg bg-foreground/[0.06]" /> | |
| ))} | |
| </div> | |
| ) | |
| } | |
| if (!data || data.data.length === 0) { | |
| return ( | |
| <div className="px-2 py-4 text-center text-sm text-muted-foreground"> | |
| <p>No conversations yet.</p> | |
| </div> | |
| ) | |
| } |
🤖 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/dm-list.tsx` around lines 18 - 33,
The empty-state "No conversations yet." shows during initial fetch because the
component only checks !data; update the useQuery call in dm-list.tsx to
destructure the loading flag (isLoading or isFetching) alongside data from
useQuery, and change the rendering logic so that while isLoading is true you
render a spinner/skeleton placeholder, and only render the "No conversations
yet." message when isLoading is false and data?.data.length === 0; reference the
existing useQuery invocation and the data variable to locate where to add the
isLoading/isFetching check and replace the early-return empty-state.
This was referenced Feb 20, 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.
Summary by CodeRabbit
New Features
UI/UX Improvements