feat: added simple SocketIO server in apps/realtime#8
Merged
BuckyMcYolo merged 8 commits intomainfrom Feb 20, 2026
Merged
Conversation
API:
- Add GET /v1/guilds/:guildSlug/channels/:channelId route
- Add GET /v1/guilds/:guildSlug/channels/:channelId/messages route
- Add GET /v1/dms/:dmId route
- Add GET /v1/dms/:dmId/messages route
- Extract shared messageWithAuthorSchema to
lib/helpers/openapi/message-schemas.ts
- Extract shared fetchMessagePage query helper to
lib/queries/messages.ts
- Remove { success, data } wrapper from channels API responses
Web:
- Add shared ChatView component for channel and DM conversations
- Add api-types.ts with InferResponseType-derived frontend types
- Wire $channelId route to use GET /channels/:channelId directly
- Wire $dmId route to use GET /dms/:dmId directly
- Drop DMMessage type alias in favour of shared Message type
Packages:
- Add @repo/utils with sluggify, Luxon-based date utils, and phone
formatting
- Add selectMessageSchema / insertMessageSchema to @repo/db messages
schema
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Socket.IO realtime gateway and supporting services (access, messages, notifications, read-states), paginated message endpoints and OpenAPI/Zod schemas, DB tables for read states/mentions/notification events, new web chat UI/routes/types, a shared utils package, and an auth hook to seed default guild channels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant Realtime as Realtime Server
participant API as REST API
participant DB as Database
Client->>Realtime: message:send { channelId, content, nonce }
Realtime->>Realtime: validate payload & auth (session)
Realtime->>API: assertUserCanAccessChannel(userId, channelId)
API->>DB: check channel & membership
DB-->>API: membership result
API-->>Realtime: allowed / forbidden
alt forbidden
Realtime-->>Client: ack { ok: false, message: "Forbidden" }
else allowed
Realtime->>DB: insert message & update channel (transaction)
DB-->>Realtime: new message + author
Realtime->>Realtime: buildMessageFanout (mentions/unread)
Realtime->>DB: persist mentions & notification events
Realtime-->>Client: ack { ok: true, message: RealtimeMessage }
Realtime-->>Client: broadcast message:created to channel room
Realtime-->>Client: emit notification:unread / notification:mention
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate 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: 14
🧹 Nitpick comments (16)
packages/auth/src/lib/auth.ts (1)
32-39: Batch same-type inserts into a single statement.The uncategorized channels (2 rows) and the child channels within each category (2 rows each) are inserted row-by-row in
for...ofloops, producing multiple round-trips. Drizzle's.values([...])accepts an array, which collapses these into a singleINSERTstatement per group.♻️ Proposed refactor — batch uncategorized and child-channel inserts
async function seedDefaultGuildChannels(guildId: string) { await db.transaction(async (tx) => { - for (const [index, ch] of defaultGuildChannels.uncategorized.entries()) { - await tx.insert(schema.channel).values({ + let topLevelPosition = 0 + await tx.insert(schema.channel).values( + defaultGuildChannels.uncategorized.map((ch) => ({ name: ch.name, type: ch.type, guildId, - position: index, - }) - } + position: topLevelPosition++, + })) + ) for (const [ categoryIndex, categoryConfig, ] of defaultGuildChannels.categories.entries()) { const insertedCategories = await tx .insert(schema.channel) .values({ name: categoryConfig.name, type: "category", guildId, - position: categoryIndex, + position: topLevelPosition++, }) .returning({ id: schema.channel.id }) const createdCategory = insertedCategories[0] if (!createdCategory) continue - for (const [channelIndex, ch] of categoryConfig.channels.entries()) { - await tx.insert(schema.channel).values({ + await tx.insert(schema.channel).values( + categoryConfig.channels.map((ch, channelIndex) => ({ name: ch.name, type: ch.type, guildId, parentId: createdCategory.id, position: channelIndex, - }) - } + })) + ) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/lib/auth.ts` around lines 32 - 39, The current code inserts uncategorized channels one-by-one in the loop using tx.insert(schema.channel).values({...}) per iteration; collect the uncategorized rows into an array first (mapping defaultGuildChannels.uncategorized to objects with name, type, guildId, position) and call tx.insert(schema.channel).values(array) once to perform a single batched insert. Do the same for child channels inside each category: for each category (from defaultGuildChannels.categories) build an array of child-channel objects (including the category's id as parentId and proper positions) and call tx.insert(schema.channel).values(childArray) once per category to replace the per-iteration inserts..env.example (1)
1-6: Sort environment variable keys alphabetically.Static analysis (dotenv-linter) flags that
BETTER_AUTH_SECRETshould precedeDATABASE_URLandNODE_ENVshould precedePORT.Proposed ordering
-DATABASE_URL=postgresql://user:password@localhost:5432/townhall -PORT=8080 -NEXT_PUBLIC_API_URL=http://localhost:8080 -BETTER_AUTH_SECRET=replace-with-a-long-random-secret -SELF_HOSTED=true -NODE_ENV=development +BETTER_AUTH_SECRET=replace-with-a-long-random-secret +DATABASE_URL=postgresql://user:password@localhost:5432/townhall +NEXT_PUBLIC_API_URL=http://localhost:8080 +NODE_ENV=development +PORT=8080 +SELF_HOSTED=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 1 - 6, The .env example keys are out of alphabetical order; reorder the environment variable names so they are sorted lexicographically (BETTER_AUTH_SECRET, DATABASE_URL, NEXT_PUBLIC_API_URL, NODE_ENV, PORT, SELF_HOSTED). Edit the .env.example file and move the lines for BETTER_AUTH_SECRET, DATABASE_URL, NEXT_PUBLIC_API_URL, NODE_ENV, PORT, and SELF_HOSTED into that alphabetical sequence so dotenv-linter no longer complains.packages/db/src/index.ts (1)
10-10: Consider narrowing the drizzle-orm re-export.The wildcard
export * from "drizzle-orm"exposes the entire drizzle-orm public API through@repo/db. Across the codebase, onlyeqandandoperators are actually imported from@repo/db(inapps/realtimeandpackages/auth). A named re-export would be more explicit and maintainable:Example narrowed re-export
-export * from "drizzle-orm" +export { eq, and, or, not, sql, asc, desc } from "drizzle-orm"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/index.ts` at line 10, The current wildcard re-export exposes the entire drizzle-orm API; narrow it to only the operators actually used by the repo by replacing the export * from "drizzle-orm" with a named re-export that exports eq and and (and any additional operators you later need) from "drizzle-orm" so consumers import only those symbols (eq, and) via this package.apps/web/package.json (1)
20-20:workspace:^is inconsistent with the other workspace deps that useworkspace:*.All other
@repo/*entries in this file useworkspace:*. Since this package isprivate: true, there's no functional difference, but consistency would preferworkspace:*.♻️ Suggested fix
- "@repo/utils": "workspace:^", + "@repo/utils": "workspace:*",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/package.json` at line 20, The dependency entry for "@repo/utils" in apps/web's package.json uses "workspace:^" which is inconsistent with other workspace deps; update the value for "@repo/utils" to "workspace:*" to match the rest of the `@repo/`* entries and maintain consistency across the file.apps/realtime/package.json (1)
8-8:tsc --declaration --emitDeclarationOnlyis unnecessary for a private app.This package is
private: trueand never consumed as a library, so emitting.d.tsfiles adds build time with no benefit. Thetsupstep already produces the runnable output.♻️ Suggested simplification
- "build": "tsup && tsc --declaration --emitDeclarationOnly --outDir dist", + "build": "tsup",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/package.json` at line 8, The build script is emitting unnecessary TypeScript declarations for a private package; update the "build" npm script to remove the tsc declaration-only step (remove or replace `tsc --declaration --emitDeclarationOnly --outDir dist`) and keep just the `tsup` invocation so the "build" script only runs `tsup` (or an equivalent single step) instead of `tsup && tsc --declaration --emitDeclarationOnly --outDir dist`.apps/web/src/components/chat/message-item.tsx (2)
28-33: Consider using the sharedformatTimeutility from@repo/utils/date.This PR introduces a
formatTimehelper inpackages/utils/src/date.ts. Using it here instead of inlinenew Date(...).toLocaleTimeString(...)would keep time formatting consistent across the app. The current inline version is slightly different (explicithour: "2-digit"/minute: "2-digit"), so the output may vary — worth aligning if a single format is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/message-item.tsx` around lines 28 - 33, Replace the inline time formatting in MessageItem with the shared formatTime utility: import formatTime from "@repo/utils/date" (or the named export) and use formatTime(message.createdAt) instead of new Date(message.createdAt).toLocaleTimeString(...); update the JSX span in the MessageItem component to call formatTime so all timestamps use the same app-wide format.
14-18: Missing fallback when avatar image isnull.When
author.imageisnull, the<img>getssrc={undefined}, which produces no visible content — just abg-mutedcircle with a broken-image icon in some browsers. Consider a placeholder (initials or a default avatar SVG).Example fallback approach
{showHeader ? ( - <img - src={author.image ?? undefined} - alt={author.name} - className="mt-0.5 size-10 shrink-0 rounded-full bg-muted object-cover" - /> + author.image ? ( + <img + src={author.image} + alt={author.name} + className="mt-0.5 size-10 shrink-0 rounded-full bg-muted object-cover" + /> + ) : ( + <div className="mt-0.5 flex size-10 shrink-0 items-center justify-center rounded-full bg-muted text-xs font-medium text-muted-foreground"> + {author.name.charAt(0).toUpperCase()} + </div> + ) ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/message-item.tsx` around lines 14 - 18, The avatar <img> currently uses src={author.image ?? undefined} which yields a broken/empty image when author.image is null; update the MessageItem avatar logic to provide a real fallback: either set src to a default avatar asset (e.g., defaultAvatarUrl) and add an onError handler to swap to the default, or render a conditional fallback element (a rounded div using the same classes that shows author initials derived from author.name) when author.image is falsy. Reference the JSX around author.image and author.name to locate where to replace the <img> handling.apps/web/src/components/onboarding/onboarding-dialog.tsx (2)
53-88:loadingstate is never reset on the success path.After a successful creation (Lines 71-83),
setLoading(false)is never called. This works in practice because navigation unmounts the dialog, but if navigation were ever deferred or the dialog stayed mounted for any reason, the UI would be stuck in a loading state. Afinallyblock would be more resilient.Suggested fix
try { // ... existing logic ... - } catch { + } catch { setError("Something went wrong. Please try again.") + } finally { setLoading(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/onboarding/onboarding-dialog.tsx` around lines 53 - 88, The handleCreate function never resets loading on the success path; wrap the try/catch in a try/catch/finally or add a finally clause that calls setLoading(false) so loading is always cleared regardless of outcome. Specifically, update handleCreate (the async handler that calls authClient.organization.create, queryClient.invalidateQueries, getFirstChannelId, and navigate) to ensure setLoading(false) is executed in a finally block after the create/invalidate/navigation logic, while preserving early returns after navigate so the finally still runs to clear loading.
40-51: Return type isstring | null | undefined— consider normalizing.
getFirstChannelIdreturnsnullon Line 45 butundefinedon Line 48-50 when both arrays are empty (sinceundefined ?? undefinedyieldsundefined). The caller's truthy check on Line 75 handles both, so this isn't a bug, but explicitly returning?? nullat the end would make the contract clearer.Suggested fix
const channels = await channelsRes.json() return ( - channels.uncategorized[0]?.id ?? channels.categories[0]?.channels[0]?.id + channels.uncategorized[0]?.id ?? channels.categories[0]?.channels[0]?.id ?? null )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/onboarding/onboarding-dialog.tsx` around lines 40 - 51, getFirstChannelId currently can return string, null, or undefined because the fallback expression (channels.uncategorized[0]?.id ?? channels.categories[0]?.channels[0]?.id) can produce undefined; normalize the return to always be string | null by ensuring the final returned value uses a null coalescing fallback (e.g., append ?? null) or explicitly return null when no id is found; locate getFirstChannelId and the api call apiClient.v1.guilds[":guildSlug"].channels.$get to apply this change so callers receive a consistent string | null type.packages/utils/src/date.ts (1)
6-54: Extract repeatedDate | string → DateTimeconversion into a helper.The same 4-line conversion block appears in all 5 functions. A small private helper would eliminate the duplication.
Suggested helper
+function toDateTime(date: Date | string): DateTime { + return typeof date === "string" + ? DateTime.fromISO(date) + : DateTime.fromJSDate(date) +} + export function timeAgo(date: Date | string): string { - const dt = - typeof date === "string" - ? DateTime.fromISO(date) - : DateTime.fromJSDate(date) + const dt = toDateTime(date) return dt.toRelative() ?? formatDate(date) }Apply the same pattern to
formatDate,formatDateTime,formatTime, andgetDayLabel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/date.ts` around lines 6 - 54, Several functions (timeAgo, formatDate, formatDateTime, formatTime, getDayLabel) duplicate the same conversion from Date | string to a luxon DateTime; add a small private helper (e.g., toDateTime(value: Date | string): DateTime) that does the typeof check and returns DateTime.fromISO(...) or DateTime.fromJSDate(...), then replace the repeated 4-line blocks in each of those functions with a single call to that helper to eliminate duplication while keeping existing return behavior; keep using DateTime symbols (toRelative, toLocaleString, DATE_MED, DATETIME_MED, TIME_SIMPLE) as before.apps/api/src/routes/v1/channels/handlers.ts (1)
137-163:listChannelMessagesmakes two sequential DB round-trips for the same channel lookupThe handler first queries the
channeltable to verify guild membership (lines 145–150), thenfetchMessagePagequeriesmessagejoined againstuser(lines 159–162). The validation query is a second round-trip whose result is discarded once the check passes.This is a minor performance concern — acceptable for now — but worth tracking. One option: fold the membership check directly into
fetchMessagePage(e.g., add an inner join to the channel table with aguildIdfilter), or keep it as is given it's a simple O(1) lookup with an indexed PK.🤖 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 137 - 163, The handler listChannelMessages does an extra DB round-trip to validate channel guild membership before calling fetchMessagePage; to fix, fold the membership check into fetchMessagePage (or into the message query) by adding a join/where against the channel table filtered by guild.id so a single query verifies channel belongs to the guild and fetches the messages, updating fetchMessagePage (or its underlying query) to accept guildId and include an inner join on channel with eq(channel.id, channelId) and eq(channel.guildId, guildId); alternatively, if you prefer to keep validation separate, document/annotate why the extra lookup is acceptable.apps/web/src/components/chat/message-list.tsx (1)
14-31:EmptyStatealways renders aHashchannel icon, even for DM/group DM contextsThe heading correctly branches on
context.type, but the icon is always<Hash />. Fordmandgroup_dmtypes a user/people icon would be more appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/message-list.tsx` around lines 14 - 31, EmptyState currently always renders the Hash icon; update EmptyState so the icon is chosen by context.type: keep <Hash /> for context.type === "channel", render a user/person icon (e.g., <User /> or <Person />) for "dm", and a group/people icon (e.g., <Users /> or <People />) for "group_dm"; replace the single Hash element in the icon container with a small conditional that returns the correct icon component based on context.type so the visuals match the heading logic.apps/api/src/routes/v1/channels/schema.ts (1)
22-34:channelParamsSchemaduplicates theguildSlugfield fromguildSlugParamsSchema.The
guildSlugdefinition (string + OpenAPI metadata) is copy-pasted verbatim. If theguildSlugparam metadata ever changes, both schemas need updating. Consider composing from the existing schema:♻️ Suggested refactor
- export const channelParamsSchema = z.object({ - guildSlug: z.string().openapi({ - param: { name: "guildSlug", in: "path", required: true }, - example: "my-guild", - }), - channelId: z - .string() - .uuid() - .openapi({ - param: { name: "channelId", in: "path", required: true }, - example: "00000000-0000-0000-0000-000000000000", - }), - }) + export const channelParamsSchema = guildSlugParamsSchema.extend({ + channelId: z + .string() + .uuid() + .openapi({ + param: { name: "channelId", in: "path", required: true }, + example: "00000000-0000-0000-0000-000000000000", + }), + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/channels/schema.ts` around lines 22 - 34, channelParamsSchema duplicates the guildSlug definition instead of reusing the existing guildSlugParamsSchema; import and compose with guildSlugParamsSchema (e.g., merge or extend) so guildSlug metadata is maintained in one place. Replace the inline guildSlug field in channelParamsSchema with a composition from guildSlugParamsSchema and then add channelId (keep its .uuid() and OpenAPI metadata) to the composed schema, referencing channelParamsSchema and guildSlugParamsSchema to locate the change.apps/realtime/src/lib/events.ts (1)
21-35:RealtimeMessage.contentisstring | nullwhilemessage:sendalways produces non-null content.
sendMessagePayloadSchemaenforcescontent.min(1), so any message created viamessage:sendwill have a non-nullcontent. However,RealtimeMessage.contenttyped asstring | nullmeans consumers ofSendMessageAckmust null-checkmessage.contentunnecessarily.This is likely intentional for DB shape compatibility (system messages etc. may have null content), but if
message:sendis the only source ofRealtimeMessageevents for now, a tighterstringtype (with a separate DB-facing type keepingnull) would give clients better guarantees. At minimum this is worth a comment in the type definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/lib/events.ts` around lines 21 - 35, RealtimeMessage currently has content: string | null which forces unnecessary null-checks for consumers of message:send; update the types so runtime guarantees from sendMessagePayloadSchema are reflected: either (a) change RealtimeMessage.content to string and create a separate DB-facing type (e.g., RealtimeDbMessage) that keeps content: string | null, making SendMessageAck and any send-path use RealtimeMessage, or (b) if you prefer minimal change, add a clear comment above the RealtimeMessage type explaining that message:send always provides non-null content and that null is only for DB/system messages; reference the RealtimeMessage type, SendMessageAck usage, and sendMessagePayloadSchema when making the change so consumers get the correct contract.apps/realtime/src/services/messages.ts (1)
13-18: RedundantsendMessagePayloadSchema.parse— the caller already validates the payload.
index.ts(line 112) callssendMessagePayloadSchema.parse(payload)before invokingcreateMessage, so by the time execution reaches lines 14–18 the payload is already validated. The re-parse is defensive but wasteful; if you want the service to remain independently callable, keep it — but consider documenting the intent or accepting the pre-parsedSendMessagePayloadtype directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/services/messages.ts` around lines 13 - 18, The createMessage function currently reparses the payload with sendMessagePayloadSchema.parse even though the caller (index.ts) already validates it; remove the redundant parse call inside createMessage (the sendMessagePayloadSchema.parse(...) lines) and use the incoming input directly, or alternatively change createMessage's signature to accept the pre-parsed SendMessagePayload type and update callers accordingly; ensure references to sendMessagePayloadSchema.parse, createMessage, and the SendMessagePayload type are updated and, if you keep validation at call sites only, add a brief comment in createMessage noting it expects a validated payload.apps/api/src/routes/v1/dms/handlers.ts (1)
172-184: Consider selecting only needed columns in the existence/membership check query.
db.select()without arguments fetches all columns from bothchannelandchannelMember. ThechannelMemberside (id,channelId,userId,createdAt) is immediately discarded by.then((rows) => rows[0]?.channel). Specifying columns keeps the query surface minimal.♻️ Suggested tightening (optional)
- const ch = await db - .select() - .from(channel) + const ch = await db + .select({ + id: channel.id, + createdAt: channel.createdAt, + updatedAt: channel.updatedAt, + name: channel.name, + topic: channel.topic, + type: channel.type, + guildId: channel.guildId, + parentId: channel.parentId, + position: channel.position, + ownerId: channel.ownerId, + rateLimitPerUser: channel.rateLimitPerUser, + }) + .from(channel)With an explicit selection the
.then((rows) => rows[0])also simplifies (no.channelaccessor needed).🤖 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 172 - 184, The query currently uses db.select() which pulls all columns from both channel and channelMember; change it to explicitly select only the channel columns (e.g., use db.select(channel) or an explicit channel projection) in the query that builds off channel and channelMember, and update the result handling to use rows[0] (or the selected alias) instead of rows[0]?.channel so we don't fetch/discard channelMember fields unnecessarily; adjust the call site that expects the channel result accordingly (references: db.select, channel, channelMember, currentUser.id, dmId).
🤖 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 120-121: The broadcast currently uses io.to(parsed.channelId).emit
which delivers message:created to everyone in the room including the sender,
causing duplicates because the sender also receives the ack; change the
broadcast to socket.to(parsed.channelId).emit("message:created", createdMessage)
so the sender is excluded and continues to receive the createdMessage via
ack?.({ ok: true, message: createdMessage }); update the line using io.to(...)
to socket.to(...) and ensure the surrounding handler references the active
socket variable used for the connection.
- Around line 23-26: The current port parsing lets an empty REALTIME_PORT become
0 because Number("") === 0; update the logic around realtimePort to treat an
empty string as "not provided", parse the selected value with Number or
parseInt, and validate it is a positive integer (and optionally within 1–65535).
Specifically, change how REALTIME_PORT is read so process.env.REALTIME_PORT is
checked for null/undefined/empty (trim() === ''), then coerce the chosen string
to a number and ensure Number.isFinite(value) && Number.isInteger(value) &&
value > 0 (and <= 65535 if desired); if validation fails, throw the existing
Error("Invalid realtime port"). Apply this to the realtimePort variable used in
the snippet.
- Around line 70-71: The cast to HeadersInit is unsafe because
socket.handshake.headers can contain string[] (e.g., set-cookie); replace the
direct cast/new Headers(...) with explicit construction: create a Headers
instance and iterate over Object.entries(socket.handshake.headers) (from the
socket.handshake object), for each header key if value is undefined skip, if
it's an array call headers.append for each item, otherwise call headers.set with
the string value; then pass that Headers instance where new Headers(...) was
used to ensure multi-value headers are preserved and types are correct.
In `@apps/realtime/src/services/messages.ts`:
- Around line 22-65: The insert/update/select sequence that creates
insertedMessage, updates schema.channel, and then fetches messageWithAuthor must
be executed inside a single Drizzle transaction so the insert isn't committed if
the final re-fetch fails; refactor the block that calls
db.insert(...).returning(...).then(...),
db.update(schema.channel).set(...).where(...), and the subsequent
db.select(...).from(...).innerJoin(...).where(...).limit(1).then(...) to run
inside db.transaction (or the project's Drizzle transaction helper), performing
the insert, channel update, and select within the same transactional callback
and returning the composed messageWithAuthor (or throwing to abort) so any
failure rolls back the insert and prevents phantom messages.
In `@apps/web/src/components/chat/header.tsx`:
- Line 5: The DM variant's avatarUrl property on the ChatHeader props is unused;
either render it or remove it. If you want avatars in DM headers, update the
ChatHeader component (the render path for props.type === "dm") to conditionally
render an <img> or Avatar component using the avatarUrl prop (fall back to
initials or a placeholder when undefined) and ensure any CSS/container (header
layout) accounts for the avatar. If avatars are not desired, remove avatarUrl
from the union type { type: "dm"; name: string; avatarUrl?: string } and any
references to it to keep the prop definition and ChatHeader implementation
consistent.
In `@apps/web/src/components/chat/message-list.tsx`:
- Around line 49-61: The "Load more" button is being rendered as the first child
while the container uses flex-col-reverse, so it appears at the bottom; move the
hasMore block so it is rendered after the message list (i.e., as the last child
in the JSX) so that with the existing flex-col-reverse it visually appears at
the top; update the JSX in the MessageList component (look for hasMore and
onLoadMore in message-list.tsx) to render the button after the messages or,
alternatively, remove flex-col-reverse and handle visual ordering
explicitly—prefer moving the hasMore block below the messages to minimally fix
the issue.
- Around line 62-68: The header visibility logic in the MessageList mapping is
inverted because messages are in desc(createdAt) order and rendered with
flex-col-reverse; change the grouping check to compare the current message
against the next-older element instead of the previous-newer one: replace the
use of prev = messages[i - 1] and showHeader = !prev || prev.authorId !==
msg.authorId with a next = messages[i + 1] check and set showHeader = !next ||
next.authorId !== msg.authorId so MessageItem (component MessageItem) shows the
header on the oldest message of each author group.
- Around line 41-43: The isLoading branch in the MessageList component currently
returns an empty div which provides no visual feedback; update the isLoading
check (in the render/return where isLoading is evaluated) to render a visible
loading indicator (e.g., an existing Spinner or a simple fallback like a
centered "Loading messages..." text or skeleton message cards) instead of the
empty div, and include accessibility attributes (role="status" and
aria-live="polite") so screen readers announce the state; ensure you use the
same container classes ("flex flex-1 items-center justify-center") so layout
stays consistent and reference the isLoading conditional in message-list.tsx
when making the change.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 36-42: The component currently treats any undefined data as
"Channel not found" even when the fetch errored; update the useQuery call to
also destructure isError and error (e.g., const { data, isPending, isError,
error } = useQuery(...)) and change the render order to: if (isPending) show
loading, else if (isError) render an error state that surfaces error.message,
else if (!data) render "Channel not found". Ensure the conditional checks
reference the existing useQuery result and the same JSX return path so
network/fetch errors are clearly distinguished from a true missing channel.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx:
- Line 24: The DM route currently returns null when isPending, causing a blank
screen; change the early return in the component that reads "if (isPending)
return null" to render the same loading UI used in the channel route
($channelId.tsx) (e.g., the "Loading..." text or the shared Loading/Spinner
component) so the UX is consistent; locate the isPending check in the default
export component of $dmId.tsx and replace the null return with the same loading
markup/component used by $channelId.tsx.
- Around line 36-47: The group DM name construction in the context object can
include literal "null" because dm.members.map((m) => m.name).join(", ") doesn't
handle null/undefined names; update the group branch of the context (the block
that sets type: "group_dm", name: dm.name ?? dm.members.map((m) =>
m.name).join(", "), memberCount: dm.members.length) to build the name by
filtering out null/undefined names (e.g., map to a safe string and/or filter
falsy names) and provide a sensible fallback when all names are missing (e.g.,
"Unknown" or a generated placeholder) so the resulting string never contains the
literal "null".
In `@packages/auth/src/lib/auth.ts`:
- Around line 155-176: afterCreateOrganization currently catches
seedDefaultGuildChannels failures independently, so onboardingCompleted is set
even if seeding fails; change the flow so the user is only marked
onboardingCompleted when seeding succeeds: either wrap both calls in a single
try and only run db.update(schema.user).set({ onboardingCompleted: true }) after
await seedDefaultGuildChannels(organization.id) completes without error, or run
both operations inside a single DB transaction tied to organization creation;
locate the afterCreateOrganization handler and modify the logic around
seedDefaultGuildChannels and the db.update(schema.user) call to gate the
onboarding flag on successful seeding (or rollback on failure).
- Around line 32-53: Replace the absolute import of the DB schema in this file
with a relative import (do not import from "@repo/db") and fix top-level channel
position collisions by using a single global counter when inserting
uncategorized channels and categories: instead of using the per-loop `index` and
`categoryIndex` for the `position` field in the
`tx.insert(schema.channel).values({...})` calls, maintain a `topLevelPosition`
counter and increment it for every top-level insert (both in the loop over
`defaultGuildChannels.uncategorized` and when inserting category rows via
`defaultGuildChannels.categories`/the `insertedCategories` logic) so each
top-level channel/category gets a unique, monotonically increasing `position`.
In `@packages/utils/src/slug.ts`:
- Around line 6-14: In sluggify(value: string, maxLength = 50) the final
.slice(0, maxLength) runs after the edge-hyphen trim (.replace(/^-+|-+$/g, "")),
which can reintroduce a trailing hyphen when the slice cuts mid-hyphen; move the
.slice(0, maxLength) to run before the edge-trim (i.e., perform .slice after
collapsing/replacing hyphens but before .replace(/^-+|-+$/g, "")), and keep the
final replace(/^-+|-+$/g, "") to ensure no leading/trailing hyphens remain after
the cut.
---
Nitpick comments:
In @.env.example:
- Around line 1-6: The .env example keys are out of alphabetical order; reorder
the environment variable names so they are sorted lexicographically
(BETTER_AUTH_SECRET, DATABASE_URL, NEXT_PUBLIC_API_URL, NODE_ENV, PORT,
SELF_HOSTED). Edit the .env.example file and move the lines for
BETTER_AUTH_SECRET, DATABASE_URL, NEXT_PUBLIC_API_URL, NODE_ENV, PORT, and
SELF_HOSTED into that alphabetical sequence so dotenv-linter no longer
complains.
In `@apps/api/src/routes/v1/channels/handlers.ts`:
- Around line 137-163: The handler listChannelMessages does an extra DB
round-trip to validate channel guild membership before calling fetchMessagePage;
to fix, fold the membership check into fetchMessagePage (or into the message
query) by adding a join/where against the channel table filtered by guild.id so
a single query verifies channel belongs to the guild and fetches the messages,
updating fetchMessagePage (or its underlying query) to accept guildId and
include an inner join on channel with eq(channel.id, channelId) and
eq(channel.guildId, guildId); alternatively, if you prefer to keep validation
separate, document/annotate why the extra lookup is acceptable.
In `@apps/api/src/routes/v1/channels/schema.ts`:
- Around line 22-34: channelParamsSchema duplicates the guildSlug definition
instead of reusing the existing guildSlugParamsSchema; import and compose with
guildSlugParamsSchema (e.g., merge or extend) so guildSlug metadata is
maintained in one place. Replace the inline guildSlug field in
channelParamsSchema with a composition from guildSlugParamsSchema and then add
channelId (keep its .uuid() and OpenAPI metadata) to the composed schema,
referencing channelParamsSchema and guildSlugParamsSchema to locate the change.
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 172-184: The query currently uses db.select() which pulls all
columns from both channel and channelMember; change it to explicitly select only
the channel columns (e.g., use db.select(channel) or an explicit channel
projection) in the query that builds off channel and channelMember, and update
the result handling to use rows[0] (or the selected alias) instead of
rows[0]?.channel so we don't fetch/discard channelMember fields unnecessarily;
adjust the call site that expects the channel result accordingly (references:
db.select, channel, channelMember, currentUser.id, dmId).
In `@apps/realtime/package.json`:
- Line 8: The build script is emitting unnecessary TypeScript declarations for a
private package; update the "build" npm script to remove the tsc
declaration-only step (remove or replace `tsc --declaration
--emitDeclarationOnly --outDir dist`) and keep just the `tsup` invocation so the
"build" script only runs `tsup` (or an equivalent single step) instead of `tsup
&& tsc --declaration --emitDeclarationOnly --outDir dist`.
In `@apps/realtime/src/lib/events.ts`:
- Around line 21-35: RealtimeMessage currently has content: string | null which
forces unnecessary null-checks for consumers of message:send; update the types
so runtime guarantees from sendMessagePayloadSchema are reflected: either (a)
change RealtimeMessage.content to string and create a separate DB-facing type
(e.g., RealtimeDbMessage) that keeps content: string | null, making
SendMessageAck and any send-path use RealtimeMessage, or (b) if you prefer
minimal change, add a clear comment above the RealtimeMessage type explaining
that message:send always provides non-null content and that null is only for
DB/system messages; reference the RealtimeMessage type, SendMessageAck usage,
and sendMessagePayloadSchema when making the change so consumers get the correct
contract.
In `@apps/realtime/src/services/messages.ts`:
- Around line 13-18: The createMessage function currently reparses the payload
with sendMessagePayloadSchema.parse even though the caller (index.ts) already
validates it; remove the redundant parse call inside createMessage (the
sendMessagePayloadSchema.parse(...) lines) and use the incoming input directly,
or alternatively change createMessage's signature to accept the pre-parsed
SendMessagePayload type and update callers accordingly; ensure references to
sendMessagePayloadSchema.parse, createMessage, and the SendMessagePayload type
are updated and, if you keep validation at call sites only, add a brief comment
in createMessage noting it expects a validated payload.
In `@apps/web/package.json`:
- Line 20: The dependency entry for "@repo/utils" in apps/web's package.json
uses "workspace:^" which is inconsistent with other workspace deps; update the
value for "@repo/utils" to "workspace:*" to match the rest of the `@repo/`*
entries and maintain consistency across the file.
In `@apps/web/src/components/chat/message-item.tsx`:
- Around line 28-33: Replace the inline time formatting in MessageItem with the
shared formatTime utility: import formatTime from "@repo/utils/date" (or the
named export) and use formatTime(message.createdAt) instead of new
Date(message.createdAt).toLocaleTimeString(...); update the JSX span in the
MessageItem component to call formatTime so all timestamps use the same app-wide
format.
- Around line 14-18: The avatar <img> currently uses src={author.image ??
undefined} which yields a broken/empty image when author.image is null; update
the MessageItem avatar logic to provide a real fallback: either set src to a
default avatar asset (e.g., defaultAvatarUrl) and add an onError handler to swap
to the default, or render a conditional fallback element (a rounded div using
the same classes that shows author initials derived from author.name) when
author.image is falsy. Reference the JSX around author.image and author.name to
locate where to replace the <img> handling.
In `@apps/web/src/components/chat/message-list.tsx`:
- Around line 14-31: EmptyState currently always renders the Hash icon; update
EmptyState so the icon is chosen by context.type: keep <Hash /> for context.type
=== "channel", render a user/person icon (e.g., <User /> or <Person />) for
"dm", and a group/people icon (e.g., <Users /> or <People />) for "group_dm";
replace the single Hash element in the icon container with a small conditional
that returns the correct icon component based on context.type so the visuals
match the heading logic.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx`:
- Around line 53-88: The handleCreate function never resets loading on the
success path; wrap the try/catch in a try/catch/finally or add a finally clause
that calls setLoading(false) so loading is always cleared regardless of outcome.
Specifically, update handleCreate (the async handler that calls
authClient.organization.create, queryClient.invalidateQueries,
getFirstChannelId, and navigate) to ensure setLoading(false) is executed in a
finally block after the create/invalidate/navigation logic, while preserving
early returns after navigate so the finally still runs to clear loading.
- Around line 40-51: getFirstChannelId currently can return string, null, or
undefined because the fallback expression (channels.uncategorized[0]?.id ??
channels.categories[0]?.channels[0]?.id) can produce undefined; normalize the
return to always be string | null by ensuring the final returned value uses a
null coalescing fallback (e.g., append ?? null) or explicitly return null when
no id is found; locate getFirstChannelId and the api call
apiClient.v1.guilds[":guildSlug"].channels.$get to apply this change so callers
receive a consistent string | null type.
In `@packages/auth/src/lib/auth.ts`:
- Around line 32-39: The current code inserts uncategorized channels one-by-one
in the loop using tx.insert(schema.channel).values({...}) per iteration; collect
the uncategorized rows into an array first (mapping
defaultGuildChannels.uncategorized to objects with name, type, guildId,
position) and call tx.insert(schema.channel).values(array) once to perform a
single batched insert. Do the same for child channels inside each category: for
each category (from defaultGuildChannels.categories) build an array of
child-channel objects (including the category's id as parentId and proper
positions) and call tx.insert(schema.channel).values(childArray) once per
category to replace the per-iteration inserts.
In `@packages/db/src/index.ts`:
- Line 10: The current wildcard re-export exposes the entire drizzle-orm API;
narrow it to only the operators actually used by the repo by replacing the
export * from "drizzle-orm" with a named re-export that exports eq and and (and
any additional operators you later need) from "drizzle-orm" so consumers import
only those symbols (eq, and) via this package.
In `@packages/utils/src/date.ts`:
- Around line 6-54: Several functions (timeAgo, formatDate, formatDateTime,
formatTime, getDayLabel) duplicate the same conversion from Date | string to a
luxon DateTime; add a small private helper (e.g., toDateTime(value: Date |
string): DateTime) that does the typeof check and returns DateTime.fromISO(...)
or DateTime.fromJSDate(...), then replace the repeated 4-line blocks in each of
those functions with a single call to that helper to eliminate duplication while
keeping existing return behavior; keep using DateTime symbols (toRelative,
toLocaleString, DATE_MED, DATETIME_MED, TIME_SIMPLE) as before.
chat/onboarding flows - add realtime room model (`user:`, `guild:`, `channel:`), unread/mention emits, and `channel:mark-read` - add DB schemas for read state, message mentions, and notification events - harden realtime auth/header handling, remove sender echo on message broadcast, and transaction-wrap message creation - move realtime port to typed env (`REALTIME_PORT`, default `8000`) and simplify server startup - improve web chat UX: shared Avatar component usage, loading states, message grouping, load-more placement, and empty-state icons - improve channel/DM route error/loading handling and safer group DM naming - gate onboarding completion on successful default channel seeding; batch seed inserts and fix top-level position ordering - tidy API schema/query shape and small utils fixes (`slug`, `date`)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/auth/src/lib/auth.ts (1)
30-72:topLevelPosition++as a side effect insidemap— minor style concern.
mapcallbacks are conventionally pure; mutating an outer variable as a positional counter insidemapcan be surprising. SincedefaultGuildChannels.uncategorizedis a static constant, the element index is readily available:🪄 Optional refactor
- const uncategorizedRows = defaultGuildChannels.uncategorized.map((ch) => ({ - name: ch.name, - type: ch.type, - guildId, - position: topLevelPosition++, - })) + const uncategorizedRows = defaultGuildChannels.uncategorized.map((ch, i) => ({ + name: ch.name, + type: ch.type, + guildId, + position: i, + })) + topLevelPosition += uncategorizedRows.lengthThe rest of the seeding logic (global
topLevelPositioncounter across uncategorized + categories, guarded.returning()result, per-categorychannelIndexfor children) looks correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/lib/auth.ts` around lines 30 - 72, The map for uncategorizedRows mutates topLevelPosition as a side-effect (using topLevelPosition++), so change the map in seedDefaultGuildChannels to use the map index instead and then advance topLevelPosition afterwards; e.g., build uncategorizedRows with .map((ch, idx) => ({ name: ch.name, type: ch.type, guildId, position: topLevelPosition + idx })) and after inserting (or before processing categories) do topLevelPosition += uncategorizedRows.length to preserve the global ordering without side effects inside the map.apps/api/src/routes/v1/dms/handlers.ts (3)
253-268: Filter the current user out at the DB level instead of in JavaScript.The
.filter((m) => m.id !== currentUser.id)on Line 257 fetches the current user's row from the DB and then discards it. Adding anepredicate to the WHERE clause avoids the wasted row entirely (same opportunity exists inlistDMsline 122 andgetDMline 217 for consistency).♻️ Suggested fix
+import { and, count, desc, eq, inArray, ne, or } from "drizzle-orm".from(channelMember) .innerJoin(user, eq(channelMember.userId, user.id)) - .where(eq(channelMember.channelId, ch.id)), + .where( + and( + eq(channelMember.channelId, ch.id), + ne(channelMember.userId, currentUser.id) + ) + ),Then remove the
.filterat the response site:members: members - .filter((m) => m.id !== currentUser.id) .map((m) => ({🤖 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 253 - 268, The handler is fetching the current user's row from the DB and filtering it out in JS (members.filter((m) => m.id !== currentUser.id)); update the DB query that populates members to add a "not equal" predicate for currentUser.id so the DB excludes that row (apply the same change to the analogous queries used by listDMs and getDM), then remove the client-side .filter in the response construction in handlers.ts so members is returned already excluding the current user.
172-196: Duplicated DM membership-verification query — extract a shared helper.Both
getDM(Lines 172–196) andlistDMMessages(Lines 279–291) reproduce the same pattern: querychannel⟶innerJoin(channelMember)⟶ filter bydmId,userId, and DM type ⟶.limit(1)⟶ return 404 when absent. Any future change (e.g., adding soft-delete filtering or tenancy checks) must be applied in two places.Consider extracting a small helper, e.g.:
// apps/api/src/lib/queries/dms.ts export async function verifyDMMembership( dmId: string, userId: string ): Promise<{ id: string } | undefined> { return db .select({ id: channel.id }) .from(channel) .innerJoin(channelMember, eq(channelMember.channelId, channel.id)) .where( and( eq(channel.id, dmId), eq(channelMember.userId, userId), or(eq(channel.type, "dm"), eq(channel.type, "group_dm")) ) ) .limit(1) .then((rows) => rows[0]) }
getDMcan call this and then issue the richer channel select only after membership is confirmed.Also applies to: 279-298
🤖 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 172 - 196, getDM and listDMMessages duplicate the same membership-verification query; extract that logic into a shared async helper (e.g., verifyDMMembership(dmId: string, userId: string)) that runs the db.select(...).from(channel).innerJoin(channelMember, eq(channelMember.channelId, channel.id)).where(and(eq(channel.id, dmId), eq(channelMember.userId, userId), or(eq(channel.type, "dm"), eq(channel.type, "group_dm")))).limit(1).then(rows => rows[0]) and return the minimal identifying result (e.g., { id }). Replace the inline queries in getDM and listDMMessages with calls to verifyDMMembership(dmId, currentUser.id) and keep the richer channel select in getDM only after the helper confirms membership; this centralizes future filters like soft-delete or tenancy checks.
205-217:channelIdis selected in thememberssub-query but never used ingetDM.Unlike
listDMs, wherechannelIdis needed to key aMapacross multiple channels,getDMoperates on a single known channel (ch.id). The field is projected from the DB but silently dropped before the response is returned.♻️ Suggested fix
const [members, latestMessages] = await Promise.all([ db .select({ - channelId: channelMember.channelId, id: user.id, name: user.name, username: user.username, displayUsername: user.displayUsername, image: user.image, })🤖 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 205 - 217, The members sub-query is projecting channelId even though getDM only works on a single channel (ch.id) and never uses that field; update the db.select inside the Promise.all (the members query that selects from channelMember and joins user) to remove channelId from the projection (or alternatively use ch.id directly if you intended to include it), ensuring the select object only returns id, name, username, displayUsername, and image so the unused channelId is not projected.packages/env/src/server.ts (2)
29-30:PORTlacks the same range/integer constraints asREALTIME_PORT.
PORTaccepts floats, zero, negatives, and values > 65535; consider aligning it withREALTIME_PORT's validation now that the better pattern is established here.♻️ Align PORT validation with REALTIME_PORT
- PORT: z.coerce.number().default(8080), + PORT: z.coerce.number().int().min(1).max(65535).default(8080), REALTIME_PORT: z.coerce.number().int().min(1).max(65535).default(8000),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/env/src/server.ts` around lines 29 - 30, PORT validation currently uses z.coerce.number().default(8080) and therefore allows floats, zero, negatives and >65535; change PORT to use the same constraints as REALTIME_PORT (z.coerce.number().int().min(1).max(65535).default(8080)) so it only accepts integers in the valid port range—update the PORT schema entry in server.ts accordingly, referencing the existing REALTIME_PORT pattern.
30-30:REALTIME_PORTin the shared server schema is architecturally unnecessary for most consumers.While
REALTIME_PORTis currently defined in the sharedpackages/env/src/server.tsschema, onlyapps/realtimeactually uses it (line 49). Consumers likepackages/db,packages/auth, andapps/apiimport the server schema but never reference this field. Because the field has a default value, this doesn't cause functional issues—but it does create unnecessary conceptual coupling in a shared package.Consider isolating realtime-specific env fields in a dedicated module (e.g.,
packages/env/src/realtime.ts) or extending the base schema only within the realtime app itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/env/src/server.ts` at line 30, Remove REALTIME_PORT from the shared server schema (the zod key REALTIME_PORT) and move it into a dedicated realtime schema module (create a new realtime schema exporting REALTIME_PORT with z.coerce.number().int().min(1).max(65535).default(8000)); then update the realtime app to import/extend that realtime schema instead of the shared server schema so other consumers (packages/db, packages/auth, apps/api) no longer receive this realtime-specific field. Ensure exports/types remain compatible where used and run tests/build to verify no breakage.apps/realtime/src/services/read-states.ts (1)
55-73: Upsert allows the read pointer to move backward.The
onConflictDoUpdateunconditionally overwriteslastReadAtandlastReadMessageId. If a client sends an olderlastReadMessageId(e.g., due to out-of-order requests or stale state), the read pointer will regress, causing previously-read messages to reappear as unread.If that's not desired, add a guard in the
setclause, e.g., using a SQLGREATESTor a conditional that only advanceslastReadAt:Suggested approach
.onConflictDoUpdate({ target: [ schema.channelReadState.channelId, schema.channelReadState.userId, ], set: { - lastReadMessageId, - lastReadAt, + lastReadMessageId: sql`CASE WHEN ${schema.channelReadState.lastReadAt} < ${lastReadAt} THEN ${lastReadMessageId} ELSE ${schema.channelReadState.lastReadMessageId} END`, + lastReadAt: sql`GREATEST(${schema.channelReadState.lastReadAt}, ${lastReadAt})`, updatedAt: new Date(), }, })If backward movement is intentional, please disregard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/services/read-states.ts` around lines 55 - 73, The upsert currently in the db.insert(schema.channelReadState).onConflictDoUpdate call unconditionally sets lastReadMessageId and lastReadAt, allowing the read pointer to move backward; change the set clause used in onConflictDoUpdate (for schema.channelReadState) to only advance these fields—e.g., use a SQL conditional or GREATEST to set lastReadAt = GREATEST(existing.lastReadAt, newLastReadAt) and only update lastReadMessageId when the new message id is greater than the stored one (or corresponds to the later timestamp); update the set mapping for lastReadAt/lastReadMessageId in the onConflictDoUpdate block to use these guarded expressions so stale/out-of-order requests cannot regress the pointer.apps/realtime/src/lib/events.ts (1)
29-45:type: stringinRealtimeMessageis wider than necessary.
MentionNotification(line 72) correctly uses a discriminated string literal union"direct_mention" | "everyone_mention", butRealtimeMessage.typestays asstring. Narrowing it to a union (e.g.,"text" | "system") would catch mismatches at compile time between the realtime service and the client consumer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/lib/events.ts` around lines 29 - 45, RealtimeMessage.type is too broad (string) and should be narrowed to a discriminated union to catch mismatches; change the type declaration in the RealtimeMessage type to a string literal union that covers all runtime message kinds your service emits (e.g., "text" | "system" | "attachment" or the actual kind names used by the realtime service) and ensure any code constructing or switching on RealtimeMessage.type (and related types like MentionNotification) uses those exact literals so TypeScript can enforce correctness.
🤖 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 208-209: The emit in the channel:mark-read handler uses
io.to(userRoom(socket.data.user.id)) which also sends the channel:read-state
event to the emitting socket (causing duplicate delivery); change the broadcast
to exclude the current socket by using socket.to(userRoom(socket.data.user.id))
so other sessions receive the channel:read-state while the current session still
gets the state via the ack (ensure ack?.({ ok: true, state }) remains
unchanged).
- Around line 109-214: The connection handler registers event listeners
(channel:join, channel:leave, message:send, channel:mark-read) only after
awaiting DB/socket.join operations in the io.on("connection", async (socket) =>
{ ... }) flow, which can lead to dropped packets; move the socket.on(...)
registrations (for channel:join, channel:leave, message:send, channel:mark-read)
to run synchronously at the top of the connection callback (before any awaits)
and have the async setup (the userRoom join, guild membership DB query, joining
guild rooms, and emitting presence:ready) run in the background (e.g., call an
async init function or kick off a promise without awaiting) so handlers like
assertUserCanAccessChannel, createMessage, buildMessageFanout, markChannelRead
and payload schemas (channelRoomPayloadSchema, sendMessagePayloadSchema,
markChannelReadPayloadSchema) remain usable but are registered immediately.
In `@apps/realtime/src/services/notifications.ts`:
- Around line 158-170: onConflictDoNothing().returning() only yields
actually-inserted rows so retried fanouts will drop already-existing
notifications; update buildMessageFanout so after the insert into
schema.notificationEvent (the insertedNotifications result) you detect when some
rows were suppressed (insertedNotifications.length < notificationRows.length)
and then query schema.notificationEvent for the missing notification keys (e.g.,
by messageId + userId or channelId + userId) to fetch the existing rows, merge
them with insertedNotifications, and use that combined set when emitting
mentionNotifications and further processing.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx`:
- Around line 215-218: The onChange handler currently calls sluggify on every
keystroke which strips trailing hyphens and prevents typing them; change the
logic so onChange only does minimal normalization (e.g., lowercase and remove
illegal chars but preserve leading/trailing hyphens) by replacing the
setSlug(sluggify(...)) call in the current onChange with a lightweight transform
(keep setSlugEdited(true) and setSlug(lightNormalize(e.target.value))); then add
an onBlur handler that runs the full sluggify and sets the final slug (use
setSlug(sluggify(slug)) inside the new onBlur). Update references to sluggify,
setSlug, setSlugEdited, and the input's value={slug} accordingly so the input
remains controlled but full trimming happens only on blur/submit.
In `@packages/utils/src/date.ts`:
- Around line 44-51: getDayLabel compares the input DateTime (from toDateTime)
to DateTime.now() using hasSame but fails to normalize zones, causing UTC vs
local-day mismatches; update getDayLabel to normalize the parsed date to the
local zone before comparing (e.g., call toDateTime(date).toLocal() or setZone to
the local zone) and then use hasSame against DateTime.now() (and now.minus({
days: 1 })) so "Today"/"Yesterday" are computed using the same timezone; keep
the final fallback to formatDate(date).
- Around line 3-7: toDateTime currently returns an invalid Luxon DateTime
silently (from DateTime.fromISO/fromJSDate) which causes downstream functions
(formatDate, formatDateTime, formatTime, getDayLabel) to render "Invalid
DateTime"; update toDateTime to validate the result (const dt = typeof date ===
"string" ? DateTime.fromISO(date) : DateTime.fromJSDate(date)) and if
!dt.isValid throw a descriptive Error (include the original input) so callers
get a clear failure mode (or return null and update
formatDate/formatDateTime/formatTime/getDayLabel to handle null) — change only
toDateTime and adjust the listed callers if you choose the null-return approach.
---
Duplicate comments:
In `@packages/auth/src/lib/auth.ts`:
- Line 1: The import line uses path aliases (e.g., "@repo/db" and similarly
"@repo/env") which violates the relative-import guideline for uncompiled
packages; update the import statements in the auth module (the top-level import
that currently references "@repo/db" and any "@repo/env" usages) to use relative
paths into the local packages (e.g., adjust imports in the file that contains
the db/schema/eq references and any env references) so they point to the sibling
package source via relative paths instead of the "@repo/..." alias.
---
Nitpick comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 253-268: The handler is fetching the current user's row from the
DB and filtering it out in JS (members.filter((m) => m.id !== currentUser.id));
update the DB query that populates members to add a "not equal" predicate for
currentUser.id so the DB excludes that row (apply the same change to the
analogous queries used by listDMs and getDM), then remove the client-side
.filter in the response construction in handlers.ts so members is returned
already excluding the current user.
- Around line 172-196: getDM and listDMMessages duplicate the same
membership-verification query; extract that logic into a shared async helper
(e.g., verifyDMMembership(dmId: string, userId: string)) that runs the
db.select(...).from(channel).innerJoin(channelMember,
eq(channelMember.channelId, channel.id)).where(and(eq(channel.id, dmId),
eq(channelMember.userId, userId), or(eq(channel.type, "dm"), eq(channel.type,
"group_dm")))).limit(1).then(rows => rows[0]) and return the minimal identifying
result (e.g., { id }). Replace the inline queries in getDM and listDMMessages
with calls to verifyDMMembership(dmId, currentUser.id) and keep the richer
channel select in getDM only after the helper confirms membership; this
centralizes future filters like soft-delete or tenancy checks.
- Around line 205-217: The members sub-query is projecting channelId even though
getDM only works on a single channel (ch.id) and never uses that field; update
the db.select inside the Promise.all (the members query that selects from
channelMember and joins user) to remove channelId from the projection (or
alternatively use ch.id directly if you intended to include it), ensuring the
select object only returns id, name, username, displayUsername, and image so the
unused channelId is not projected.
In `@apps/realtime/src/lib/events.ts`:
- Around line 29-45: RealtimeMessage.type is too broad (string) and should be
narrowed to a discriminated union to catch mismatches; change the type
declaration in the RealtimeMessage type to a string literal union that covers
all runtime message kinds your service emits (e.g., "text" | "system" |
"attachment" or the actual kind names used by the realtime service) and ensure
any code constructing or switching on RealtimeMessage.type (and related types
like MentionNotification) uses those exact literals so TypeScript can enforce
correctness.
In `@apps/realtime/src/services/read-states.ts`:
- Around line 55-73: The upsert currently in the
db.insert(schema.channelReadState).onConflictDoUpdate call unconditionally sets
lastReadMessageId and lastReadAt, allowing the read pointer to move backward;
change the set clause used in onConflictDoUpdate (for schema.channelReadState)
to only advance these fields—e.g., use a SQL conditional or GREATEST to set
lastReadAt = GREATEST(existing.lastReadAt, newLastReadAt) and only update
lastReadMessageId when the new message id is greater than the stored one (or
corresponds to the later timestamp); update the set mapping for
lastReadAt/lastReadMessageId in the onConflictDoUpdate block to use these
guarded expressions so stale/out-of-order requests cannot regress the pointer.
In `@packages/auth/src/lib/auth.ts`:
- Around line 30-72: The map for uncategorizedRows mutates topLevelPosition as a
side-effect (using topLevelPosition++), so change the map in
seedDefaultGuildChannels to use the map index instead and then advance
topLevelPosition afterwards; e.g., build uncategorizedRows with .map((ch, idx)
=> ({ name: ch.name, type: ch.type, guildId, position: topLevelPosition + idx
})) and after inserting (or before processing categories) do topLevelPosition +=
uncategorizedRows.length to preserve the global ordering without side effects
inside the map.
In `@packages/env/src/server.ts`:
- Around line 29-30: PORT validation currently uses
z.coerce.number().default(8080) and therefore allows floats, zero, negatives and
>65535; change PORT to use the same constraints as REALTIME_PORT
(z.coerce.number().int().min(1).max(65535).default(8080)) so it only accepts
integers in the valid port range—update the PORT schema entry in server.ts
accordingly, referencing the existing REALTIME_PORT pattern.
- Line 30: Remove REALTIME_PORT from the shared server schema (the zod key
REALTIME_PORT) and move it into a dedicated realtime schema module (create a new
realtime schema exporting REALTIME_PORT with
z.coerce.number().int().min(1).max(65535).default(8000)); then update the
realtime app to import/extend that realtime schema instead of the shared server
schema so other consumers (packages/db, packages/auth, apps/api) no longer
receive this realtime-specific field. Ensure exports/types remain compatible
where used and run tests/build to verify no breakage.
onboarding/date/env handling - realtime: register socket handlers immediately on connect and run room/presence init asynchronously - realtime: prevent duplicate read-state delivery to emitting socket (`socket.to(userRoom)` in `channel:mark-read`) - realtime: make mention fanout retry-safe by merging existing notification rows when `onConflictDoNothing().returning()` omits conflicts - realtime: narrow `RealtimeMessage.type` to explicit message-type union - realtime: prevent read cursor regression in `channel_read_state` upsert and compute counts from persisted state - onboarding: preserve user slug typing behavior on change, apply full `sluggify` on blur and submit - utils/date: validate invalid DateTime inputs in `toDateTime` and normalize `getDayLabel` comparisons to local timezone - api dms: exclude current user at DB layer for member queries and remove redundant JS-side filtering - auth seeding: remove side-effectful top-level position mutation inside map while preserving ordering - env: constrain `PORT` to integer range 1..65535
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/src/routes/v1/dms/handlers.ts (2)
264-270: Redundant.map()— themembersselect already projects the exact same shape.The query at lines 210–225 selects
{ id, name, username, displayUsername, image }, which is identical to what the.map()reconstructs. You can drop the mapping entirely.♻️ Proposed simplification
return c.json( { ...ch, - members: members.map((m) => ({ - id: m.id, - name: m.name, - username: m.username, - displayUsername: m.displayUsername, - image: m.image, - })), + members, lastMessage, }, HttpStatusCodes.OK )🤖 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 264 - 270, The members array is being re-mapped into the exact same shape already returned by the DB select; remove the redundant members.map(...) and use the selected members directly (where the code reconstructs members with id, name, username, displayUsername, image). Update the handler that builds the response (the block using members: members.map((m) => {...})) to assign members without mapping, keeping the original selected fields intact.
176-199: Extract the repeated membership-check query into a shared helper.The access-gate pattern —
innerJoin(channelMember)+where(eq(channel.id, dmId) && eq(channelMember.userId, currentUser.id) && or(dm/group_dm type))+.limit(1).then(rows => rows[0])— is copy-pasted verbatim betweengetDMandlistDMMessages. A single helper avoids drift if the access semantics ever change (e.g. adding soft-delete, blocked-user checks).As a bonus, harmonise the channel-type predicate with the rest of the file:
listDMsusesinArray(channel.type, ["dm", "group_dm"])(line 24) while both new handlers useor(eq(...), eq(...)).inArrayis shorter and already established.♻️ Suggested refactor
+/** Returns the DM channel row if it exists and the user is a member; null otherwise. */ +async function resolveDMChannel(dmId: string, userId: string) { + return db + .select({ id: channel.id }) + .from(channel) + .innerJoin(channelMember, eq(channelMember.channelId, channel.id)) + .where( + and( + eq(channel.id, dmId), + eq(channelMember.userId, userId), + inArray(channel.type, ["dm", "group_dm"]) + ) + ) + .limit(1) + .then((rows) => rows[0] ?? null) +}
getDMthen fetches full channel metadata only after the membership gate passes:- const ch = await db - .select({ - id: channel.id, - createdAt: channel.createdAt, - // ... - }) - .from(channel) - .innerJoin(channelMember, eq(channelMember.channelId, channel.id)) - .where( - and( - eq(channel.id, dmId), - eq(channelMember.userId, currentUser.id), - or(eq(channel.type, "dm"), eq(channel.type, "group_dm")) - ) - ) - .limit(1) - .then((rows) => rows[0]) + const access = await resolveDMChannel(dmId, currentUser.id) if (!ch) { ... } + const ch = await db.select({ id: channel.id, createdAt: ..., ... }).from(channel).where(eq(channel.id, access.id)).limit(1).then(r => r[0])
listDMMessages:- const ch = await db - .select({ id: channel.id }) - ... + const ch = await resolveDMChannel(dmId, currentUser.id) if (!ch) { ... }Also applies to: 285-297
🤖 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 176 - 199, Extract the repeated membership-gate query used in getDM and listDMMessages into a shared helper (e.g., ensureChannelMembership or fetchChannelMembershipRow) that performs the innerJoin on channelMember, applies the where predicate eq(channel.id, dmId), eq(channelMember.userId, currentUser.id) and the channel type check using inArray(channel.type, ["dm","group_dm"]), then returns the single row (limit(1) -> rows[0] or null). Replace the duplicated blocks in getDM and listDMMessages with calls to this helper (passing db, dmId and currentUser.id) so both handlers reuse the same access logic and avoid divergence; keep references to channel, channelMember, db, inArray, dmId and currentUser.id to locate and wire the helper.apps/realtime/src/services/notifications.ts (1)
225-236: Theas MentionNotification["type"]cast bypasses type narrowing.
notification.typefrom the DB is typed as the fullnotificationEvent.typeenum (which may include values beyond"direct_mention" | "everyone_mention"). In this flow the rows are scoped to those two types, so the cast is safe at runtime, but a runtime guard or a.where(inArray(..., ['direct_mention', 'everyone_mention']))in the fallback query (line 196) would make it provably safe and eliminate the cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/services/notifications.ts` around lines 225 - 236, The code casts notification.type to MentionNotification["type"] when building mentionNotifications which circumvents type narrowing; update the fallback query that loads notifications (the query referenced near the earlier fallback) to explicitly filter types to ['direct_mention','everyone_mention'] (e.g. add a .where(inArray(...)) or equivalent) so the DB rows are provably scoped, then remove the unsafe cast and rely on the narrowed type; alternatively, add a runtime guard that asserts notification.type is one of the two values before mapping (so MentionNotification and mentionNotifications no longer need the cast).apps/realtime/src/index.ts (1)
58-64: CORS origins fall back to rawprocess.envwhile port uses validatedenv.
REALTIME_CORS_ORIGINis read fromprocess.env(line 60) while other config uses the validatedenvobject. This isn't a bug, but if the env schema is the canonical source of truth, consider addingREALTIME_CORS_ORIGINthere for consistent validation and to surface misconfigurations at startup rather than at runtime.🤖 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 58 - 64, The CORS setup reads REALTIME_CORS_ORIGIN directly from process.env instead of the validated env schema; update the runtime to use the validated env value (e.g., replace process.env.REALTIME_CORS_ORIGIN usage with env.REALTIME_CORS_ORIGIN) and add REALTIME_CORS_ORIGIN to the env schema (with a sensible default like defaultOrigins.join(",") or an optional string) so validation surfaces misconfiguration at startup; adjust the corsOrigins calculation (the variable named corsOrigins and constant defaultOrigins) to fall back to the validated env default and keep the same split/map/filter logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx`:
- Around line 83-90: The call to getFirstChannelId can throw and currently
bubbles to the outer catch, blocking successful guild creation; wrap the await
getFirstChannelId(createdGuildSlug) in a local try-catch inside the onboarding
flow (around the code that calls navigate) so any error from getFirstChannelId
or channelsRes.json() is caught, log or report the error, and on failure fall
back to navigate to the guild root route (use navigate with to: "/$guildSlug" or
params: { guildSlug: createdGuildSlug }) instead of rethrowing; ensure you still
return after a successful first-channel navigation to keep existing control
flow.
---
Duplicate comments:
In `@apps/realtime/src/index.ts`:
- Around line 116-145: The review contains a duplicate approval/comment covering
the same change; remove the duplicated comment block so there's a single
approval and ensure the feedback about synchronous handler registration and
async presence setup (referencing initializeConnection and the connection
callback that registers socket.on handlers) is applied to both affected ranges
if intended; update the PR comment or review metadata to keep only one reference
to this approval to avoid confusion.
- Around line 207-221: The handler for "channel:mark-read" is correct: use
socket.to(userRoom(socket.data.user.id)) to broadcast the "channel:read-state"
update to other sockets and use ack to return the state to the sender; no code
changes needed—verify the payload is validated via
markChannelReadPayloadSchema.parse, the update uses markChannelRead({ userId:
socket.data.user.id, channelId: parsed.channelId, lastReadMessageId:
parsed.lastReadMessageId }), and the broadcast uses
userRoom(socket.data.user.id) so the current socket is excluded while ack
returns the state to the originator.
---
Nitpick comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 264-270: The members array is being re-mapped into the exact same
shape already returned by the DB select; remove the redundant members.map(...)
and use the selected members directly (where the code reconstructs members with
id, name, username, displayUsername, image). Update the handler that builds the
response (the block using members: members.map((m) => {...})) to assign members
without mapping, keeping the original selected fields intact.
- Around line 176-199: Extract the repeated membership-gate query used in getDM
and listDMMessages into a shared helper (e.g., ensureChannelMembership or
fetchChannelMembershipRow) that performs the innerJoin on channelMember, applies
the where predicate eq(channel.id, dmId), eq(channelMember.userId,
currentUser.id) and the channel type check using inArray(channel.type,
["dm","group_dm"]), then returns the single row (limit(1) -> rows[0] or null).
Replace the duplicated blocks in getDM and listDMMessages with calls to this
helper (passing db, dmId and currentUser.id) so both handlers reuse the same
access logic and avoid divergence; keep references to channel, channelMember,
db, inArray, dmId and currentUser.id to locate and wire the helper.
In `@apps/realtime/src/index.ts`:
- Around line 58-64: The CORS setup reads REALTIME_CORS_ORIGIN directly from
process.env instead of the validated env schema; update the runtime to use the
validated env value (e.g., replace process.env.REALTIME_CORS_ORIGIN usage with
env.REALTIME_CORS_ORIGIN) and add REALTIME_CORS_ORIGIN to the env schema (with a
sensible default like defaultOrigins.join(",") or an optional string) so
validation surfaces misconfiguration at startup; adjust the corsOrigins
calculation (the variable named corsOrigins and constant defaultOrigins) to fall
back to the validated env default and keep the same split/map/filter logic.
In `@apps/realtime/src/services/notifications.ts`:
- Around line 225-236: The code casts notification.type to
MentionNotification["type"] when building mentionNotifications which circumvents
type narrowing; update the fallback query that loads notifications (the query
referenced near the earlier fallback) to explicitly filter types to
['direct_mention','everyone_mention'] (e.g. add a .where(inArray(...)) or
equivalent) so the DB rows are provably scoped, then remove the unsafe cast and
rely on the narrowed type; alternatively, add a runtime guard that asserts
notification.type is one of the two values before mapping (so
MentionNotification and mentionNotifications no longer need the cast).
Comment on lines
83
to
90
| const firstChannelId = await getFirstChannelId(createdGuildSlug) | ||
| if (firstChannelId) { | ||
| navigate({ | ||
| to: "/$guildSlug/$channelId", | ||
| params: { guildSlug: createdGuildSlug, channelId: firstChannelId }, | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
getFirstChannelId exceptions surface as "Something went wrong" after the guild is already created.
If channelsRes.json() throws or the fetch itself rejects (transient network error, malformed response), the unhandled rejection falls through to the outer catch, which displays "Something went wrong. Please try again." — but the guild was successfully created at that point. The user then retries and hits a duplicate-slug error from the org-creation API, leaving them stuck with no visible way forward.
Wrap getFirstChannelId in its own try-catch and fall back to guild-root navigation on failure:
🛠️ Proposed fix
- const firstChannelId = await getFirstChannelId(createdGuildSlug)
- if (firstChannelId) {
- navigate({
- to: "/$guildSlug/$channelId",
- params: { guildSlug: createdGuildSlug, channelId: firstChannelId },
- })
- return
- }
-
- navigate({ to: "/$guildSlug", params: { guildSlug: createdGuildSlug } })
+ try {
+ const firstChannelId = await getFirstChannelId(createdGuildSlug)
+ if (firstChannelId) {
+ navigate({
+ to: "/$guildSlug/$channelId",
+ params: { guildSlug: createdGuildSlug, channelId: firstChannelId },
+ })
+ return
+ }
+ } catch {
+ // channel fetch failed — fall through to guild root
+ }
+
+ navigate({ to: "/$guildSlug", params: { guildSlug: createdGuildSlug } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx` around lines 83 -
90, The call to getFirstChannelId can throw and currently bubbles to the outer
catch, blocking successful guild creation; wrap the await
getFirstChannelId(createdGuildSlug) in a local try-catch inside the onboarding
flow (around the code that calls navigate) so any error from getFirstChannelId
or channelsRes.json() is caught, log or report the error, and on failure fall
back to navigate to the guild root route (use navigate with to: "/$guildSlug" or
params: { guildSlug: createdGuildSlug }) instead of rethrowing; ensure you still
return after a successful first-channel navigation to keep existing control
flow.
DM membership checks
There was a problem hiding this comment.
Actionable comments posted: 6
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/dms/handlers.ts (1)
52-55: 🛠️ Refactor suggestion | 🟠 MajorUse
DM_CHANNEL_TYPESinstead of the inline literal array.
DM_CHANNEL_TYPESwas introduced at line 17 to be the single source of truth for valid DM channel types, butlistDMsstill embeds["dm", "group_dm"]inline. Any future addition toDM_CHANNEL_TYPES(e.g., a voice DM variant) will silently miss thelistDMscount/channel queries.♻️ Proposed fix
const dmFilter = and( eq(channelMember.userId, currentUser.id), - inArray(channel.type, ["dm", "group_dm"]) + inArray(channel.type, DM_CHANNEL_TYPES) )🤖 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 52 - 55, The dmFilter uses an inline array ["dm", "group_dm"] instead of the canonical DM_CHANNEL_TYPES constant, which risks divergence; update dmFilter (the inArray(...) call that checks channel.type in the list) to reference DM_CHANNEL_TYPES instead of the literal array, ensuring the listDMs query uses the single source of truth DM_CHANNEL_TYPES (no other logic changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 232-265: The duplicated logic that builds the last-message SQL
projection and the JS DTO is present in getDM and listDMs (see
latestMessages/lastMessageRow → lastMessage); extract a single helper (e.g.,
formatLastMessageRow or buildLastMessageProjection) at the top of the file or
shared lib that (a) returns the SQL projection used in the db.select and (b)
converts a DB row into the JS object with createdAt.toISOString() and the nested
author shape; replace the inline projection/row-to-object mapping in both
listDMs and getDM to call that helper so future author fields (like
avatarDecorationData) are added in one place.
In `@apps/realtime/src/index.ts`:
- Around line 140-142: The catch block in initializeConnection is swallowing
exceptions from the DB query (schema.guildMember) or socket.join and
disconnecting the socket silently; change the catch to capture the thrown error
(e.g., catch (err)), log a clear error including context (socket.id,
guildId/userId if available, and the error object) using the existing logger in
this file, then call socket.disconnect(true); ensure the logged message
references initializeConnection, schema.guildMember, and socket.join so failures
are visible in production logs.
In `@apps/realtime/src/services/notifications.ts`:
- Around line 46-63: The `@everyone` fanout currently done by listRecipientUserIds
(when channel.guildId is set) performs an unbounded full-guild-membership query
and then inserts and emits per-member inside the message:send handler (writing
to notificationEvent and calling io.to(userRoom(...)).emit) which will not
scale; change this by preventing synchronous full-guild fanout: implement
application-level rate-limiting/authorization for `@everyone`, and move the DB
fanout and per-user notificationEvent inserts into a background job/queue worker
(enqueue a single guild-fanout task from the message:send handler instead of
looping there), and in the worker batch DB writes and emissions and only emit to
currently-connected rooms (inspect io.sockets.adapter.rooms or equivalent to
filter recipient userIds) and emit in chunks to avoid exhausting the event loop
and DB pool.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx`:
- Around line 86-90: The catch block currently uses the parameter name "error"
which shadows the component's "error" state variable; rename the catch parameter
to something like "fetchError" (or "err") and update the console.error call to
use that new name, and if you intend to reflect the failure in the UI also call
the component's state setter (e.g. setError(fetchError.message ||
String(fetchError))) so the "error" state and the caught exception are distinct;
the change should be applied around the block that references createdGuildSlug
and the catch in the fetch-first-channel logic.
- Around line 23-28: normalizeSlugInput currently permits consecutive hyphens
while typing (e.g., "my--guild"); update normalizeSlugInput to collapse runs of
hyphens by applying a hyphen-collapse replace (e.g., replace(/-+/g, "-")) after
converting spaces to "-" and removing invalid chars so intermediate input is
normalized; keep sluggify-on-blur behavior intact but ensure normalizeSlugInput
performs the hyphen collapse to provide a smoother typing experience.
In `@packages/env/src/server.ts`:
- Around line 23-24: DEFAULT_REALTIME_CORS_ORIGIN currently defaults to
localhost which will silently block non-local clients in production; update
startup checks in packages/env/src/server.ts to detect when process.env.NODE_ENV
=== "production" and the resolved REALTIME_CORS_ORIGIN (the zod-parsed env or
env var) equals DEFAULT_REALTIME_CORS_ORIGIN, then either throw an explicit
error or log a loud warning and exit so the misconfiguration is surfaced; apply
the same check for the analogous default constant at the other occurrence (e.g.,
the constant referenced around line 33) so both REALTIME_CORS_ORIGIN-like
defaults are validated at startup.
---
Outside diff comments:
In `@apps/api/src/routes/v1/dms/handlers.ts`:
- Around line 52-55: The dmFilter uses an inline array ["dm", "group_dm"]
instead of the canonical DM_CHANNEL_TYPES constant, which risks divergence;
update dmFilter (the inArray(...) call that checks channel.type in the list) to
reference DM_CHANNEL_TYPES instead of the literal array, ensuring the listDMs
query uses the single source of truth DM_CHANNEL_TYPES (no other logic changes
required).
---
Duplicate comments:
In `@apps/realtime/src/index.ts`:
- Around line 35-54: The toHeaders function already correctly handles
multi-value headers for IncomingHttpHeaders by iterating over
Object.entries(handshakeHeaders), appending each array item with headers.append
and using headers.set for single string values; no change needed—leave the
toHeaders function as implemented (function name: toHeaders).
- Around line 145-222: The review contains a duplicate review note/marker
([duplicate_comment]) rather than a code issue; remove the duplicate
comment/marker so the PR shows a single approval—keep the approval text about
the io.on("connection", (socket) => { ... }) handlers and the call to
initializeConnection(socket) intact but delete the redundant [duplicate_comment]
tag/line from the review comment.
- Line 56: The previous fragile Number(...) parsing is replaced by relying on
the validated env.REALTIME_PORT; ensure the code uses the already-coerced
numeric constant realtimePort (from env.REALTIME_PORT) everywhere expecting a
port (e.g., server.listen calls) and remove any leftover manual parsing or
fallback logic for port 0; verify realtimePort is not re-cast to string and that
functions using realtimePort accept a number type (search for realtimePort
references and update usages if they still expect string).
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx`:
- Around line 83-91: The try-catch around getFirstChannelId correctly isolates
errors and falls back to guild-root navigation; no code change required—just
ensure the error handling remains (the local variable firstChannelId, the await
getFirstChannelId(createdGuildSlug) call, and the fallback navigation to the
guild root are preserved) and keep the console.error message for debugging.
- Around line 232-238: Keep the current input handling: onChange should call
setSlugEdited(true) and setSlug(normalizeSlugInput(e.target.value)) to lightly
normalize while preserving trailing hyphens during typing, and onBlur should
finalize the value via setSlug(current => sluggify(current)); ensure
normalizeSlugInput and sluggify remain distinct functions and that setSlugEdited
is still invoked on change.
Comment on lines
+232
to
+265
| db | ||
| .select({ | ||
| channelId: message.channelId, | ||
| id: message.id, | ||
| content: message.content, | ||
| createdAt: message.createdAt, | ||
| authorId: user.id, | ||
| authorName: user.name, | ||
| authorUsername: user.username, | ||
| authorDisplayUsername: user.displayUsername, | ||
| authorImage: user.image, | ||
| }) | ||
| .from(message) | ||
| .innerJoin(user, eq(message.authorId, user.id)) | ||
| .where(eq(message.channelId, ch.id)) | ||
| .orderBy(desc(message.createdAt)) | ||
| .limit(1), | ||
| ]) | ||
|
|
||
| const lastMessageRow = latestMessages[0] | ||
| const lastMessage = lastMessageRow | ||
| ? { | ||
| id: lastMessageRow.id, | ||
| content: lastMessageRow.content, | ||
| createdAt: lastMessageRow.createdAt.toISOString(), | ||
| author: { | ||
| id: lastMessageRow.authorId, | ||
| name: lastMessageRow.authorName, | ||
| username: lastMessageRow.authorUsername, | ||
| displayUsername: lastMessageRow.authorDisplayUsername, | ||
| image: lastMessageRow.authorImage, | ||
| }, | ||
| } | ||
| : null |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the repeated last-message formatting into a shared helper.
The latest-message SQL projection and the resulting object shape { id, content, createdAt: .toISOString(), author: {...} } are duplicated verbatim between listDMs (lines 96–138) and getDM (lines 232–265). A future change to the author shape (e.g. adding avatarDecorationData) must be applied in two places.
♻️ Proposed refactor
Extract a formatter at the top of the file (or in a shared lib):
+type LastMessageRow = {
+ id: string
+ content: string
+ createdAt: Date
+ authorId: string
+ authorName: string
+ authorUsername: string | null
+ authorDisplayUsername: string | null
+ authorImage: string | null
+}
+
+function formatLastMessage(row: LastMessageRow) {
+ return {
+ id: row.id,
+ content: row.content,
+ createdAt: row.createdAt.toISOString(),
+ author: {
+ id: row.authorId,
+ name: row.authorName,
+ username: row.authorUsername,
+ displayUsername: row.authorDisplayUsername,
+ image: row.authorImage,
+ },
+ }
+}Then in listDMs replace lines 122–138:
- const lastMessageByChannel = new Map(
- latestMessages.map((msg) => [
- msg.channelId,
- {
- id: msg.id,
- content: msg.content,
- createdAt: msg.createdAt.toISOString(),
- author: {
- id: msg.authorId,
- name: msg.authorName,
- username: msg.authorUsername,
- displayUsername: msg.authorDisplayUsername,
- image: msg.authorImage,
- },
- },
- ])
- )
+ const lastMessageByChannel = new Map(
+ latestMessages.map((msg) => [msg.channelId, formatLastMessage(msg)])
+ )And in getDM replace lines 252–265:
- const lastMessage = lastMessageRow
- ? {
- id: lastMessageRow.id,
- content: lastMessageRow.content,
- createdAt: lastMessageRow.createdAt.toISOString(),
- author: {
- id: lastMessageRow.authorId,
- name: lastMessageRow.authorName,
- username: lastMessageRow.authorUsername,
- displayUsername: lastMessageRow.authorDisplayUsername,
- image: lastMessageRow.authorImage,
- },
- }
- : null
+ const lastMessage = lastMessageRow ? formatLastMessage(lastMessageRow) : null🤖 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 232 - 265, The
duplicated logic that builds the last-message SQL projection and the JS DTO is
present in getDM and listDMs (see latestMessages/lastMessageRow → lastMessage);
extract a single helper (e.g., formatLastMessageRow or
buildLastMessageProjection) at the top of the file or shared lib that (a)
returns the SQL projection used in the db.select and (b) converts a DB row into
the JS object with createdAt.toISOString() and the nested author shape; replace
the inline projection/row-to-object mapping in both listDMs and getDM to call
that helper so future author fields (like avatarDecorationData) are added in one
place.
Comment on lines
+46
to
+63
| if (channel.guildId) { | ||
| return db | ||
| .select({ | ||
| userId: schema.guildMember.userId, | ||
| }) | ||
| .from(schema.guildMember) | ||
| .where(eq(schema.guildMember.guildId, channel.guildId)) | ||
| .then((rows) => rows.map((row) => row.userId)) | ||
| } | ||
|
|
||
| return db | ||
| .select({ | ||
| userId: schema.channelMember.userId, | ||
| }) | ||
| .from(schema.channelMember) | ||
| .where(eq(schema.channelMember.channelId, channel.id)) | ||
| .then((rows) => rows.map((row) => row.userId)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
@everyone fanout performs an unbounded full-guild-membership query and emits one Socket.IO event per member.
listRecipientUserIds with a guildId fetches every guild member in a single query, which for large guilds could return thousands of rows. An @everyone mention then inserts one row per member into notificationEvent and fires one io.to(userRoom(...)).emit(...) per member, all within a single message:send handler. This will not scale and can exhaust DB connection pool time and event-loop time simultaneously.
Consider: rate-limiting @everyone mentions at the application level, using a background job/queue for the DB fanout, and batching or limiting the real-time emissions (e.g., only emit to connected user rooms, querying io.sockets.adapter.rooms).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/realtime/src/services/notifications.ts` around lines 46 - 63, The
`@everyone` fanout currently done by listRecipientUserIds (when channel.guildId is
set) performs an unbounded full-guild-membership query and then inserts and
emits per-member inside the message:send handler (writing to notificationEvent
and calling io.to(userRoom(...)).emit) which will not scale; change this by
preventing synchronous full-guild fanout: implement application-level
rate-limiting/authorization for `@everyone`, and move the DB fanout and per-user
notificationEvent inserts into a background job/queue worker (enqueue a single
guild-fanout task from the message:send handler instead of looping there), and
in the worker batch DB writes and emissions and only emit to currently-connected
rooms (inspect io.sockets.adapter.rooms or equivalent to filter recipient
userIds) and emit in chunks to avoid exhausting the event loop and DB pool.
Comment on lines
+23
to
28
| function normalizeSlugInput(value: string) { | ||
| return value | ||
| .toLowerCase() | ||
| .trim() | ||
| .replace(/[^a-z0-9\s-]/g, "") | ||
| .replace(/\s+/g, "-") | ||
| .replace(/-+/g, "-") | ||
| .replace(/^-+|-+$/g, "") | ||
| .slice(0, 50) | ||
| .replace(/[^a-z0-9-]/g, "") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consecutive hyphens are not collapsed during typing.
normalizeSlugInput allows sequences like "my--guild" to appear in the input. While sluggify on blur will clean this up, collapsing runs of hyphens during typing provides a smoother experience.
Proposed fix
function normalizeSlugInput(value: string) {
return value
.toLowerCase()
.replace(/\s+/g, "-")
.replace(/[^a-z0-9-]/g, "")
+ .replace(/-+/g, "-")
}📝 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
| function normalizeSlugInput(value: string) { | |
| return value | |
| .toLowerCase() | |
| .trim() | |
| .replace(/[^a-z0-9\s-]/g, "") | |
| .replace(/\s+/g, "-") | |
| .replace(/-+/g, "-") | |
| .replace(/^-+|-+$/g, "") | |
| .slice(0, 50) | |
| .replace(/[^a-z0-9-]/g, "") | |
| } | |
| function normalizeSlugInput(value: string) { | |
| return value | |
| .toLowerCase() | |
| .replace(/\s+/g, "-") | |
| .replace(/[^a-z0-9-]/g, "") | |
| .replace(/-+/g, "-") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx` around lines 23 -
28, normalizeSlugInput currently permits consecutive hyphens while typing (e.g.,
"my--guild"); update normalizeSlugInput to collapse runs of hyphens by applying
a hyphen-collapse replace (e.g., replace(/-+/g, "-")) after converting spaces to
"-" and removing invalid chars so intermediate input is normalized; keep
sluggify-on-blur behavior intact but ensure normalizeSlugInput performs the
hyphen collapse to provide a smoother typing experience.
Comment on lines
+86
to
+90
| } catch (error) { | ||
| console.error( | ||
| `Failed to fetch first channel for guild ${createdGuildSlug}:`, | ||
| error | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
error in catch block shadows the component's error state variable (line 36).
This makes the caught exception and the UI error state indistinguishable within the block, which is confusing and fragile for future edits.
Proposed fix
- } catch (error) {
+ } catch (err) {
console.error(
`Failed to fetch first channel for guild ${createdGuildSlug}:`,
- error
+ err
)
}📝 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
| } catch (error) { | |
| console.error( | |
| `Failed to fetch first channel for guild ${createdGuildSlug}:`, | |
| error | |
| ) | |
| } catch (err) { | |
| console.error( | |
| `Failed to fetch first channel for guild ${createdGuildSlug}:`, | |
| err | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/onboarding/onboarding-dialog.tsx` around lines 86 -
90, The catch block currently uses the parameter name "error" which shadows the
component's "error" state variable; rename the catch parameter to something like
"fetchError" (or "err") and update the console.error call to use that new name,
and if you intend to reflect the failure in the UI also call the component's
state setter (e.g. setError(fetchError.message || String(fetchError))) so the
"error" state and the caught exception are distinct; the change should be
applied around the block that references createdGuildSlug and the catch in the
fetch-first-channel logic.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 175-211: The handler for "message:send" currently treats
buildMessageFanout failures as overall failures even after createMessage and the
channel broadcast succeed; change the flow so that after createMessage(...) and
the socket.to(...).emit("message:created", ...) you immediately send ack?.({ ok:
true, message: createdMessage.message }) and then perform
buildMessageFanout(...) in a separate try/catch (or fire-and-forget Promise) so
any errors from buildMessageFanout or the subsequent io.to(...).emit calls are
caught/logged but do not flip the success ack; keep references to the same
symbols (createMessage, createdMessage, channelRoom, ack, buildMessageFanout,
fanout.unreadNotifications, fanout.mentionNotifications) to locate and update
the code.
- Around line 95-112: The auth middleware's catch block is swallowing the thrown
error; modify the io.use middleware (around auth.api.getSession and
socket.handshake.headers) to capture the caught exception (e.g., catch (err))
and log it with the project's structured logger (e.g., processLogger.error or
logger.error) including context such as the socket id and that it occurred
during auth.getSession, then still call next(new Error("Unauthorized")) so the
client behavior is unchanged; ensure you reference io.use, auth.api.getSession,
socket.handshake.headers, and socket.data in your change.
In `@ROADMAP.md`:
- Line 22: The TODO in ROADMAP.md about adding explicit error logging in
initializeConnection is stale — the initializeConnection implementation (see
apps/realtime/src/index.ts) already logs socket.id, userId and the error before
disconnecting; update ROADMAP.md by moving that checklist item to the Completed
section (or remove it) and adjust the checklist text to reflect that
initializeConnection already performs socketId/userId/error logging prior to
socket.disconnect().
---
Duplicate comments:
In `@apps/realtime/src/index.ts`:
- Line 56: The code now reads env.REALTIME_PORT directly but the env Zod schema
must guarantee it parses to a numeric TCP port; update the env validation (the
Zod schema that builds the env object used by env.REALTIME_PORT) to coerce to a
number, require it is an integer > 0 and <= 65535 (e.g.,
z.coerce.number().int().positive().max(65535)) and ensure empty strings are
rejected/coerced so REALTIME_PORT cannot become 0; then use the validated
env.REALTIME_PORT value in index.ts (keep the env.REALTIME_PORT reference but
rely on the schema change).
This was referenced Feb 24, 2026
Merged
This was referenced Mar 4, 2026
Merged
Merged
Merged
This was referenced Mar 12, 2026
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 (updated)
New Features
Documentation
Chores / Infra
Notable public API & schema changes
Risks / Review notes (high level)
Confidence Score: 3/5
Intent and implementation are clear and align with existing patterns, but the PR introduces schema-level DB changes and a sizable realtime runtime component that require careful migration, integration testing, and some operational hardening before production deployment.