Dev#15
Merged
BuckyMcYolo merged 5 commits intomainfrom Mar 9, 2026
Merged
Conversation
highlighting when clicking on a replied to message
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds reply and attachment support end-to-end: new schemas/types (referencedMessage, attachments), DB/realtime wiring for referencedMessageId, S3 presign upload API and client, client file-upload hook and UI, composer reply UI and optimistic send flow, and integration across routes and adapters. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MessageList
participant MessageItem
participant useReplyState
participant MessageInput
participant useFileUpload
participant useMessageSending
participant RealtimeService
participant API
participant S3
User->>MessageItem: click "Reply"
MessageItem->>MessageList: onReply(message)
MessageList->>useReplyState: setReplyingTo(message)
useReplyState->>MessageInput: replyingTo prop set
MessageInput->>User: show reply banner
User->>MessageInput: add text (+files via useFileUpload) -> Send
MessageInput->>useFileUpload: request presign for files
useFileUpload->>API: POST /v1/uploads/presign
API->>useFileUpload: returns uploadUrl & fileUrl
useFileUpload->>S3: upload file to uploadUrl
useFileUpload->>MessageInput: returns attachment metadata
MessageInput->>useMessageSending: handleSend(content, {referencedMessage, attachments})
useMessageSending->>MessageInput: create optimistic message
useMessageSending->>RealtimeService: emit message:send (referencedMessageId + attachments)
RealtimeService->>API: persist message
API->>RealtimeService: stored message with referencedMessage populated
RealtimeService->>useMessageSending: broadcast stored message
useMessageSending->>MessageList: replace optimistic with real message
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/chat/date-divider.tsx (1)
9-14: 🧹 Nitpick | 🔵 TrivialConsider caching the formatted date.
formatDateDivider(date)is called twice—once for thedata-date-dividerattribute and once for the label text. While the overhead is likely minimal, you could assign it to a variable for clarity.♻️ Optional refactor
export function DateDivider({ date }: DateDividerProps) { + const formattedDate = formatDateDivider(date) return ( - <div className="py-3" data-date-divider={formatDateDivider(date)}> + <div className="py-3" data-date-divider={formattedDate}> <div className="relative flex items-center"> <div className="h-px w-full bg-border/70" /> <span className="absolute left-1/2 -translate-x-1/2 rounded-full border border-border/70 bg-background px-2.5 py-0.5 text-xs font-medium text-muted-foreground"> - {formatDateDivider(date)} + {formattedDate} </span> </div> </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/date-divider.tsx` around lines 9 - 14, The component calls formatDateDivider(date) twice; cache the formatted value in a local variable (e.g., const formatted = formatDateDivider(date)) inside the date-divider component and replace both uses (data-date-divider and the span content) with that variable to avoid duplicate calls and improve clarity (refer to formatDateDivider and the JSX surrounding data-date-divider and the span).apps/web/src/components/chat/message-item.tsx (1)
130-149:⚠️ Potential issue | 🟠 MajorDisable reply actions for optimistic rows.
handleReplycan capture a message whoseidis still just the client nonce.createOptimisticMessageuses that nonce as a temporary id until the ACK arrives inapps/web/src/lib/realtime-adapter.ts, Lines 136-160, so a quick reply ends up sending areferencedMessageIdthe server can't resolve. Even if the row is later replaced in the list, the stored draft still carries the old nonce. GateonReplybehind a persisted/pending check before wiring it intoMessageActionBar.🤖 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 130 - 149, The reply action must be disabled for optimistic (non-persisted) messages that still carry the client nonce id: add a persisted/pending guard (matching the logic from createOptimisticMessage in realtime-adapter.ts) before wiring onReply into MessageActionBar and also short-circuit handleReply; e.g., compute a boolean like canReply = message.persisted || !message.pending (or the equivalent persisted check used elsewhere), pass onReply={canReply ? handleReply : undefined} into <MessageActionBar>, and make handleReply return early if the message is optimistic so no draft is created referencing the temporary nonce.
🤖 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/chat/composer/message-input.tsx`:
- Around line 100-106: The onSend prop should signal actual send success so
reply mode is only cleared after the message is persisted: change the onSend
signature used in message-input.tsx to return a Promise<boolean> (or some
success indicator) instead of being fire-and-forget, update callers (notably
useMessageSending) to resolve true only on ACK/persist and false on early-return
or failure, and then in the component call onCancelReply (or clear replyingTo)
only when the returned promise indicates success; reference the onSend prop in
message-input.tsx, the useMessageSending hook (the send logic/ACK handling), and
the replyingTo/onCancelReply flow so the optimistic row rollback path does not
clear reply mode prematurely.
In `@apps/web/src/lib/realtime-adapter.ts`:
- Around line 16-17: Add a new field referencedMessageId: string | null to the
RealtimeMessage type and ensure the object emitted in the "message:created"
event populates that field from the source message's referenced ID (use the
stored referencedMessageId or rm.referencedMessage?.id as fallback), so reply
identity is preserved even when referencedMessage is deleted; update the
RealtimeMessage type definition and the place constructing the emitted message
(the message creation / emission code in realtime-adapter.ts) to include
referencedMessageId.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx:
- Around line 74-75: The reply state (replyingTo) is not cleared when the route
param dmId changes, so a selected reply can leak into a different DM; add an
effect that watches dmId and calls clearReply (or setReplyingTo(null)) whenever
dmId changes (and optionally on unmount) to reset the local reply state; locate
the useReplyState destructure (replyingTo, setReplyingTo, clearReply) in the
component and add a useEffect dependent on dmId that calls clearReply to ensure
referencedMessageId is never emitted for the wrong conversation.
In `@packages/realtime-types/src/events.ts`:
- Around line 80-90: The author object is duplicated across
RealtimeReferencedMessage, RealtimeMessage.author, and RealtimeMessageMention;
extract a shared type (e.g., RealtimeAuthor) that defines { id, name, username,
displayUsername, image } and replace the inline author shapes with this new
RealtimeAuthor type in RealtimeReferencedMessage, RealtimeMessage (author
property), and RealtimeMessageMention to remove duplication and keep types
consistent.
---
Outside diff comments:
In `@apps/web/src/components/chat/date-divider.tsx`:
- Around line 9-14: The component calls formatDateDivider(date) twice; cache the
formatted value in a local variable (e.g., const formatted =
formatDateDivider(date)) inside the date-divider component and replace both uses
(data-date-divider and the span content) with that variable to avoid duplicate
calls and improve clarity (refer to formatDateDivider and the JSX surrounding
data-date-divider and the span).
In `@apps/web/src/components/chat/message-item.tsx`:
- Around line 130-149: The reply action must be disabled for optimistic
(non-persisted) messages that still carry the client nonce id: add a
persisted/pending guard (matching the logic from createOptimisticMessage in
realtime-adapter.ts) before wiring onReply into MessageActionBar and also
short-circuit handleReply; e.g., compute a boolean like canReply =
message.persisted || !message.pending (or the equivalent persisted check used
elsewhere), pass onReply={canReply ? handleReply : undefined} into
<MessageActionBar>, and make handleReply return early if the message is
optimistic so no draft is created referencing the temporary nonce.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b6f18ad-2ade-47af-8330-3d3a6df66e2f
📒 Files selected for processing (14)
apps/api/src/lib/helpers/openapi/message-schemas.tsapps/api/src/lib/queries/messages.tsapps/realtime/src/services/messages.tsapps/web/src/components/chat/composer/message-input.tsxapps/web/src/components/chat/date-divider.tsxapps/web/src/components/chat/message-action-bar.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/components/chat/message-list.tsxapps/web/src/hooks/use-message-sending.tsapps/web/src/hooks/use-reply-state.tsapps/web/src/lib/realtime-adapter.tsapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxpackages/realtime-types/src/events.ts
Comment on lines
+100
to
+106
| onSend: ( | ||
| content: string, | ||
| options?: { | ||
| mentions: Message["mentions"] | ||
| referencedMessage?: Message["referencedMessage"] | ||
| } | ||
| ) => void |
There was a problem hiding this comment.
Only clear reply mode after a successful send.
onSend is still fire-and-forget, but useMessageSending can return early when the socket or user is unavailable and can roll the optimistic row back on ACK failure in apps/web/src/hooks/use-message-sending.ts, Lines 114-150. Line 580 clears replyingTo anyway, so a failed send silently exits reply mode and the retry becomes a plain message. Have onSend resolve a success signal and only call onCancelReply once the send actually sticks.
🛠️ Proposed fix
interface MessageInputProps {
context: ChatContext
onSend: (
content: string,
options?: {
mentions: Message["mentions"]
referencedMessage?: Message["referencedMessage"]
}
- ) => void
+ ) => Promise<boolean> | boolean
isSending?: boolean
currentUserId?: string
mentionCandidates?: MentionCandidate[]
replyingTo?: Message | null
onCancelReply?: () => void
}
@@
- const handleSend = useCallback(() => {
+ const handleSend = useCallback(async () => {
if (!editor) return
@@
- onSend(trimmed, {
+ const didSend = await onSend(trimmed, {
mentions,
referencedMessage: replyingTo
? {
id: replyingTo.id,
content: replyingTo.content,
author: replyingTo.author,
}
: undefined,
})
+ if (!didSend) return
editor.commands.clearContent(true)
editor.commands.focus("end")
setPlainText("")
onCancelReply?.()
}, [Also applies to: 567-580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/chat/composer/message-input.tsx` around lines 100 -
106, The onSend prop should signal actual send success so reply mode is only
cleared after the message is persisted: change the onSend signature used in
message-input.tsx to return a Promise<boolean> (or some success indicator)
instead of being fire-and-forget, update callers (notably useMessageSending) to
resolve true only on ACK/persist and false on early-return or failure, and then
in the component call onCancelReply (or clear replyingTo) only when the returned
promise indicates success; reference the onSend prop in message-input.tsx, the
useMessageSending hook (the send logic/ACK handling), and the
replyingTo/onCancelReply flow so the optimistic row rollback path does not clear
reply mode prematurely.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/main.tsx (1)
3-3:⚠️ Potential issue | 🟡 MinorRemove unused import.
The
ReactQueryDevtoolsimport is no longer used since the component is commented out on line 44. This creates dead code and may trigger linting warnings.🧹 Proposed fix
-import { ReactQueryDevtools } from "@tanstack/react-query-devtools"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/main.tsx` at line 3, Remove the unused ReactQueryDevtools import: delete the import declaration for ReactQueryDevtools and ensure no remaining references to that symbol (the devtools component is currently commented out in main.tsx), so the code no longer contains the unused import that triggers lint warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 10-15: Reorder the S3-related environment variable entries in
.env.example into alphabetical order by variable name (referencing
S3_ACCESS_KEY_ID, S3_BUCKET_NAME, S3_ENDPOINT, S3_PUBLIC_URL, S3_REGION,
S3_SECRET_ACCESS_KEY) so they appear sorted and easier to locate; update the
block to list those variables in ascending alphabetical order while preserving
values/placeholders and any comments.
In `@apps/api/src/routes/v1/uploads/handlers.ts`:
- Around line 19-24: Replace the incorrect 403 response for oversized uploads
with the proper 413 status: locate the branch that checks "if (size >
env.MAX_FILE_UPLOAD_SIZE)" and change the c.json response that currently uses
HttpStatusCodes.FORBIDDEN to use HttpStatusCodes.PAYLOAD_TOO_LARGE (and adjust
the message to indicate the payload is too large) so clients receive the correct
HTTP 413 semantics for payload size violations.
- Around line 41-83: The current logic can skip both checks when ch.guildId is
falsy and ch.type is not in DM_CHANNEL_TYPES, allowing authorization bypass;
change the branch so the DM membership check is run only for non-guild channels
and unknown channel types are rejected: replace the second independent if
(DM_CHANNEL_TYPES.includes(...)) block with an else if (ch.guildId == null &&
DM_CHANNEL_TYPES.includes(ch.type as ...)) that queries channelMember, and add a
final else that returns the same forbidden response (using the existing json +
HttpStatusCodes.FORBIDDEN). Keep the same db.select(...) calls and the
guildMember/channelMember identifiers but ensure the fallback else handles
unknown/non-guild non-DM channel types.
In `@apps/api/src/routes/v1/uploads/schema.ts`:
- Around line 3-18: Extract the ALLOWED_MIME_TYPES array into a shared package
(e.g., export const ALLOWED_MIME_TYPES from `@repo/shared`) and replace the
duplicated local definitions in both locations: remove the local
ALLOWED_MIME_TYPES in the uploads schema and import the shared constant instead,
keeping it as a readonly array (as const) so consumers can adapt it; update the
client hook (use-file-upload) to construct a Set from the shared array (new
Set(ALLOWED_MIME_TYPES)) where it previously used its own Set, and ensure all
imports and type usages reference the shared symbol ALLOWED_MIME_TYPES.
- Line 31: The size field in the upload schema (size: z.number().int().min(1))
should also enforce the maximum file size used by the handler: add
.max(MAX_FILE_UPLOAD_SIZE) to the zod chain so the schema validates the same
limit as the handler (reference MAX_FILE_UPLOAD_SIZE used in the upload
handler). If importing env/MAX_FILE_UPLOAD_SIZE at schema-definition time causes
module init issues, document that and keep the handler-level check as the source
of truth; otherwise reference the env constant when building the schema so
OpenAPI and early validation reflect the actual max.
In `@apps/realtime/src/services/messages.ts`:
- Around line 39-50: The code currently uses input.payload.referencedMessageId
(hasReply) and writes the row as a reply before verifying the referenced message
belongs to the same channel; fix this by checking inside the db.transaction
(using tx and schema.message) that a message with id =
input.payload.referencedMessageId AND channelId = input.payload.channelId
exists, and only set type = "reply" and referencedMessageId when that validation
succeeds; apply the same validation and conditional-persist logic to the second
code path referenced (the block around the other insert at lines ~92-107) so no
cross-channel referencedMessage leaks are written or emitted.
In `@apps/web/src/components/chat/composer/attachment-preview.tsx`:
- Around line 29-35: The remove button in attachment-preview.tsx is an icon-only
control so screen readers can't identify which attachment it targets; update the
button in the component that calls onRemove(attachment.id) to provide an
accessible name (e.g., add aria-label={`Remove attachment ${attachment.name ||
attachment.id}`} or include visually-hidden text describing the attachment) so
each X button is uniquely announced and remains clickable while preserving the
existing onClick/onRemove behavior.
In `@apps/web/src/components/chat/message-attachment.tsx`:
- Around line 92-102: The current useEffect only registers global keys when
hasMultiple is true so Escape won't reliably close single-image lightboxes;
modify the effect (useEffect) and its local handler (handleKeyDown) to always
attach when open is true, have handleKeyDown call goNext() / goPrev() only for
ArrowRight/Left when hasMultiple is true, and call the dialog close callback
(onClose) when e.key === "Escape"; ensure you still remove the same handler in
the cleanup and keep the dependency list ([open, hasMultiple, goNext, goPrev,
onClose]).
In `@apps/web/src/components/chat/message-item.tsx`:
- Around line 112-113: The current isReply boolean should not depend on
referencedMessageId or showHeader; change isReply to be message.type === "reply"
only, then in the reply render path use message.referencedMessageId to decide
whether to render the referenced message content or the "Original message was
deleted" fallback (render the fallback when referencedMessageId is null), and
remove the extra showHeader guard that prevents rendering reply context for
grouped replies—adjust the rendering logic around the reply block (the isReply
usage and the render at the earlier reply block and the follow-up render at the
block currently gated by showHeader) so reply context always displays for
message.type === "reply" while still showing the deletion fallback when
referencedMessageId is null.
In `@apps/web/src/hooks/use-file-upload.ts`:
- Around line 85-118: The current addFiles function slices the incoming files
before validation and uses render-time attachments.length, causing invalid files
to consume slots and race conditions when multiple addFiles run; instead, first
validate/filter the incoming files by size and MIME (using ALLOWED_MIME_TYPES
and env.NEXT_PUBLIC_MAX_FILE_UPLOAD_SIZE) to produce validatedCandidates, then
inside setAttachments(prev => { const remaining = MAX_ATTACHMENTS - prev.length;
const accepted = validatedCandidates.slice(0, remaining); return [...prev,
...acceptedMappedToPendingAttachments] }) compute the remaining slots from prev,
map only the accepted candidates to PendingAttachment objects (using
getImageDimensions, URL.createObjectURL, crypto.randomUUID, etc.), and append
them, ensuring the cap is enforced atomically and avoids burning slots for
rejected files.
In `@apps/web/src/main.tsx`:
- Line 44: The commented-out ReactQueryDevtools line in main.tsx should be
either removed or made environment-aware; either delete the commented line
entirely, or restore it and wrap the ReactQueryDevtools component (the
ReactQueryDevtools symbol) with a runtime condition that only renders when
process.env.NODE_ENV === 'development' (or your app's equivalent env flag) so
devtools appear in dev builds and are excluded in production. Ensure the
conditional is applied where the app tree is rendered in main.tsx so the
devtools mount is automatic for development and omitted for prod.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 104-105: The component's reply state from useReplyState
(replyingTo, setReplyingTo, clearReply) is not cleared when channelId changes,
causing stale referencedMessageId to leak across channels; add an effect that
watches channelId and calls clearReply (or setReplyingTo(null)) whenever
channelId changes to reset the local reply state and prevent cross-channel
leakage.
In `@packages/realtime-types/src/events.ts`:
- Around line 14-30: The attachmentPayloadSchema used by
sendMessagePayloadSchema is too permissive (accepts any valid URL and
contentType) allowing callers to bypass the uploads presign checks; update
validation to enforce the same allowlist/host and path constraints used by the
upload API (or invoke the shared upload URL validator) for
attachmentPayloadSchema (and/or revalidate attachments inside createMessage) so
only approved hosts/paths (e.g., your /uploads/presign origin and allowed
hostnames) or signed URLs are accepted before persisting.
In `@ROADMAP.md`:
- Line 24: Update the ROADMAP entry for "File/image attachment uploads (R2)" so
the roadmap isn't stale after this PR: either move that checklist item from
Phase 1 pending to the Completed/Done section (mark it shipped) or replace the
text with a new, specific follow-up task describing the remaining work that
truly belongs to R2 (e.g., "Finalize R2: server-side thumbnailing" or similar).
Edit the "File/image attachment uploads (R2)" line in ROADMAP.md accordingly and
ensure the checklist/section formatting remains consistent with the surrounding
items.
---
Outside diff comments:
In `@apps/web/src/main.tsx`:
- Line 3: Remove the unused ReactQueryDevtools import: delete the import
declaration for ReactQueryDevtools and ensure no remaining references to that
symbol (the devtools component is currently commented out in main.tsx), so the
code no longer contains the unused import that triggers lint warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2d83d3c-42c2-4ae0-b2df-34a3261acc8e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.env.exampleROADMAP.mdapps/api/package.jsonapps/api/src/app.tsapps/api/src/lib/s3.tsapps/api/src/routes/v1/uploads/handlers.tsapps/api/src/routes/v1/uploads/index.tsapps/api/src/routes/v1/uploads/routes.tsapps/api/src/routes/v1/uploads/schema.tsapps/realtime/src/index.tsapps/realtime/src/services/messages.tsapps/web/src/components/chat/composer/attachment-preview.tsxapps/web/src/components/chat/composer/message-input.tsxapps/web/src/components/chat/message-attachment.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/hooks/use-file-upload.tsapps/web/src/hooks/use-message-sending.tsapps/web/src/lib/realtime-adapter.tsapps/web/src/main.tsxapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxpackages/env/src/server.tspackages/realtime-types/src/events.ts
Comment on lines
+19
to
+24
| if (size > env.MAX_FILE_UPLOAD_SIZE) { | ||
| return c.json( | ||
| { success: false, message: "File too large" }, | ||
| HttpStatusCodes.FORBIDDEN | ||
| ) | ||
| } |
There was a problem hiding this comment.
Consider using HTTP 413 for payload size violations.
Returning FORBIDDEN (403) for oversized files conflates authorization failures with payload issues. HTTP 413 PAYLOAD_TOO_LARGE is semantically correct here and helps clients distinguish between "you're not allowed" vs "your file is too big."
Suggested fix
if (size > env.MAX_FILE_UPLOAD_SIZE) {
return c.json(
{ success: false, message: "File too large" },
- HttpStatusCodes.FORBIDDEN
+ HttpStatusCodes.CONTENT_TOO_LARGE
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/handlers.ts` around lines 19 - 24, Replace the
incorrect 403 response for oversized uploads with the proper 413 status: locate
the branch that checks "if (size > env.MAX_FILE_UPLOAD_SIZE)" and change the
c.json response that currently uses HttpStatusCodes.FORBIDDEN to use
HttpStatusCodes.PAYLOAD_TOO_LARGE (and adjust the message to indicate the
payload is too large) so clients receive the correct HTTP 413 semantics for
payload size violations.
| .refine((ct) => (ALLOWED_MIME_TYPES as readonly string[]).includes(ct), { | ||
| message: "Unsupported file type", | ||
| }), | ||
| size: z.number().int().min(1), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a maximum size constraint to the schema.
The schema only validates min(1) for size, while the handler separately enforces MAX_FILE_UPLOAD_SIZE. Adding a max constraint to the schema would provide earlier validation and better OpenAPI documentation.
♻️ Suggested change to include max size validation
+import { env } from "@repo/env/server"
+
export const presignRequestSchema = z.object({
channelId: z.string().uuid(),
filename: z.string().min(1).max(256),
contentType: z
.string()
.refine((ct) => (ALLOWED_MIME_TYPES as readonly string[]).includes(ct), {
message: "Unsupported file type",
}),
- size: z.number().int().min(1),
+ size: z.number().int().min(1).max(env.MAX_FILE_UPLOAD_SIZE),
})Note: If env cannot be imported at schema definition time due to module initialization order, the current handler-level enforcement is acceptable.
📝 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
| size: z.number().int().min(1), | |
| import { env } from "@repo/env/server" | |
| export const presignRequestSchema = z.object({ | |
| channelId: z.string().uuid(), | |
| filename: z.string().min(1).max(256), | |
| contentType: z | |
| .string() | |
| .refine((ct) => (ALLOWED_MIME_TYPES as readonly string[]).includes(ct), { | |
| message: "Unsupported file type", | |
| }), | |
| size: z.number().int().min(1).max(env.MAX_FILE_UPLOAD_SIZE), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/schema.ts` at line 31, The size field in the
upload schema (size: z.number().int().min(1)) should also enforce the maximum
file size used by the handler: add .max(MAX_FILE_UPLOAD_SIZE) to the zod chain
so the schema validates the same limit as the handler (reference
MAX_FILE_UPLOAD_SIZE used in the upload handler). If importing
env/MAX_FILE_UPLOAD_SIZE at schema-definition time causes module init issues,
document that and keep the handler-level check as the source of truth; otherwise
reference the env constant when building the schema so OpenAPI and early
validation reflect the actual max.
| </TooltipProvider> | ||
| </ThemeProvider> | ||
| <ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" /> | ||
| {/*<ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" />*/} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider environment-based conditional rendering or complete removal.
Commented-out code can clutter the codebase. Consider either:
- Removing the devtools entirely if no longer needed, or
- Conditionally rendering based on environment for automatic dev/prod handling
♻️ Alternative approach: Environment-based conditional rendering
- {/*<ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" />*/}
+ {import.meta.env.DEV && (
+ <ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" />
+ )}This approach automatically enables devtools in development and disables them in production builds.
📝 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
| {/*<ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" />*/} | |
| {import.meta.env.DEV && ( | |
| <ReactQueryDevtools initialIsOpen={false} buttonPosition="top-right" /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/main.tsx` at line 44, The commented-out ReactQueryDevtools line
in main.tsx should be either removed or made environment-aware; either delete
the commented line entirely, or restore it and wrap the ReactQueryDevtools
component (the ReactQueryDevtools symbol) with a runtime condition that only
renders when process.env.NODE_ENV === 'development' (or your app's equivalent
env flag) so devtools appear in dev builds and are excluded in production.
Ensure the conditional is applied where the app tree is rendered in main.tsx so
the devtools mount is automatic for development and omitted for prod.
Comment on lines
+14
to
+30
| export const attachmentPayloadSchema = z.object({ | ||
| url: z.string().url(), | ||
| filename: z.string().min(1).max(256), | ||
| size: z.number().int().min(1), | ||
| contentType: z.string().min(1), | ||
| width: z.number().int().positive().optional(), | ||
| height: z.number().int().positive().optional(), | ||
| }) | ||
|
|
||
| export const sendMessagePayloadSchema = z | ||
| .object({ | ||
| channelId: z.string().uuid(), | ||
| content: z.string().trim().max(2000).optional(), | ||
| nonce: z.string().max(100).optional(), | ||
| referencedMessageId: z.string().uuid().optional(), | ||
| attachments: z.array(attachmentPayloadSchema).max(10).optional(), | ||
| }) |
There was a problem hiding this comment.
The attachment payload is too permissive for a server-trusted path.
This only requires a syntactically valid URL and a non-empty contentType. A direct message:send caller can bypass the /uploads/presign checks and persist arbitrary external attachments, and createMessage stores them verbatim. Reuse the same allowlist/host validation as the upload API, or validate attachments again on the server before persisting them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/realtime-types/src/events.ts` around lines 14 - 30, The
attachmentPayloadSchema used by sendMessagePayloadSchema is too permissive
(accepts any valid URL and contentType) allowing callers to bypass the uploads
presign checks; update validation to enforce the same allowlist/host and path
constraints used by the upload API (or invoke the shared upload URL validator)
for attachmentPayloadSchema (and/or revalidate attachments inside createMessage)
so only approved hosts/paths (e.g., your /uploads/presign origin and allowed
hostnames) or signed URLs are accepted before persisting.
|
|
||
| ## Phase 1 — Core UX Gaps | ||
|
|
||
| - [ ] File/image attachment uploads (R2) |
There was a problem hiding this comment.
Mark the attachment upload item as shipped or split out the remaining follow-up.
This PR adds attachment support, but Line 24 still tracks File/image attachment uploads (R2) as pending in Phase 1. That leaves the roadmap stale immediately after merge. Either move this item to Completed or rename it to the specific remaining follow-up if “R2” means a narrower next step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ROADMAP.md` at line 24, Update the ROADMAP entry for "File/image attachment
uploads (R2)" so the roadmap isn't stale after this PR: either move that
checklist item from Phase 1 pending to the Completed/Done section (mark it
shipped) or replace the text with a new, specific follow-up task describing the
remaining work that truly belongs to R2 (e.g., "Finalize R2: server-side
thumbnailing" or similar). Edit the "File/image attachment uploads (R2)" line in
ROADMAP.md accordingly and ensure the checklist/section formatting remains
consistent with the surrounding items.
- Add Sonner toasts for oversized/unsupported file upload errors - Fix upload handler auth bypass for non-guild non-DM channels - Use 413 instead of 403 for oversized uploads - Validate referenced message belongs to same channel - Fix race condition in useFileUpload slot checking - Fix lightbox Escape key and reply display for grouped messages - Clear reply state on channel change
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
apps/realtime/src/services/messages.ts (1)
113-144:⚠️ Potential issue | 🟡 MinorHydrate the reply preview inside the transaction.
Lines 115-129 re-read the referenced row after commit with a separate
dbquery. That means the reply can be persisted successfully, but the emitted realtime payload can still go out withreferencedMessage: nullor different data if the target changes between the validation/insert and this fetch. Reuse the row you already validated inside the transaction and buildreferencedMessagefrom that result instead.🤖 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 113 - 144, The code re-queries the referenced message after commit causing potential stale/missing reply previews; instead, when hasReply and input.payload.referencedMessageId are validated inside the transaction, reuse the previously fetched row (the refMsg/validated result obtained in-transaction) to populate referencedMessage rather than issuing a new db.select after commit; construct referencedMessage from that refMsg (assign id, content and author fields) within the transaction scope so the emitted realtime payload uses the same validated data.apps/web/src/routes/_authenticated/dms/$dmId.tsx (1)
77-80:⚠️ Potential issue | 🟠 MajorReset reply state when
dmIdchanges.This still carries
replyingToacross DM navigation, so a reply selected in one conversation can be sent in another. Mirror theclearReply()effect fromapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx around lines 77 - 80, The reply selection persists across DM navigation because we never clear reply state when dmId changes; add an effect that calls clearReply whenever dmId changes. Specifically, in the component that uses useReplyState (replyingTo, setReplyingTo, clearReply) add a useEffect watching dmId (and clearReply) and invoke clearReply() inside it—mirror the clearReply effect used in the $guildSlug/$channelId.tsx file so replies from one conversation are reset when switching DMs.apps/web/src/components/chat/composer/message-input.tsx (1)
95-102:⚠️ Potential issue | 🟠 MajorOnly clear the composer after the send actually succeeds.
useMessageSendingcan return early when the socket or user is unavailable and can roll the optimistic row back on ACK failure, but this component clears the editor, reply target, and attachments immediately after callingonSend. MakeonSendresolve a success signal and gate the cleanup on that result.Also applies to: 581-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/composer/message-input.tsx` around lines 95 - 102, The component currently calls the onSend callback and immediately clears the editor, reply target, and attachments; change it to await a success signal from onSend and only perform cleanup when it resolves successfully. Update the onSend prop usage in message-input.tsx to expect a Promise<boolean> (or similar success result) from onSend, call await onSend(content, options), and only if the result indicates success then clear the editor state, reply target, and attachments (the existing clearEditor/clearAttachments/setReplyTargetCleared code paths); if onSend rejects or returns failure, keep the composer state intact so optimistic rollbacks or retries work correctly. Ensure callers of useMessageSending/onSend are updated to return the success boolean/promise so the component can gate cleanup.
🤖 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/chat/composer/attachment-preview.tsx`:
- Around line 25-27: The div rendering each attachment (the element with
key={attachment.id} in the attachment preview JSX) uses the deprecated Tailwind
v3 utility "flex-shrink-0"; update the className to use the Tailwind v4
equivalent "shrink-0" (replace "flex-shrink-0" with "shrink-0" in the className
string) so the preview cards no longer shrink.
In `@apps/web/src/components/chat/composer/message-input.tsx`:
- Around line 553-558: The send button currently bases canSend on
pendingAttachments.length while handleSend uses getUploadedAttachments(), so
failed uploads leave the button enabled but clicking does nothing; change
canSend (and the analogous check near the other block mentioned) to derive from
getUploadedAttachments().length and/or uploadedAttachments (the result of
getUploadedAttachments()) and also ensure isSending/content length checks
remain; update any early-return logic in handleSend/send-related handlers to use
uploadedAttachments (not pendingAttachments) so only actually uploaded
attachments enable sending.
In `@apps/web/src/components/chat/message-attachment.tsx`:
- Around line 17-26: The <track kind="captions" /> element is an empty
placeholder and provides no accessibility value; remove this empty <track> node
from the video element (the JSX that uses attachment.url) so you don't expose
meaningless caption tracks, or if you intentionally left it as a placeholder add
a clear inline comment explaining that captions are unavailable and why the
empty track remains; update the video JSX accordingly (the element that renders
the video with src={attachment.url}) to reflect either removal or the
explanatory comment.
In `@apps/web/src/hooks/use-file-upload.ts`:
- Around line 127-140: The current pattern declares and assigns the local
variable `accepted` inside the `setAttachments` updater which is unconventional
and confusing; move the calculation of accepted files out of the updater
(compute `accepted = prepared.slice(0, MAX_ATTACHMENTS - prev.length)` or
compute `remaining` and `accepted` before calling `setAttachments`) so you call
`setAttachments(prev => accepted.length === 0 ? prev : [...prev, ...accepted])`
with `accepted` already determined, and keep the existing `toast.error` logic
when `prepared.length > remaining`; alternatively rename to `acceptedRef` or add
a clear comment if you must capture it for use after the setter — update
references to `accepted`, `setAttachments`, `MAX_ATTACHMENTS`, `prepared`, and
the `toast.error` branch accordingly.
In `@ROADMAP.md`:
- Around line 22-25: Update the Phase 1 — Core UX Gaps list in ROADMAP.md to
reflect that message replies were shipped: either add a checked item like "- [x]
Message replies" under the same section or merge it into the existing "- [x]
File/image attachment uploads (R2)" entry so the roadmap matches the delivered
scope; ensure the checkbox is checked and the wording matches other items for
consistency (e.g., "Message replies" or "Replies and attachments").
---
Duplicate comments:
In `@apps/realtime/src/services/messages.ts`:
- Around line 113-144: The code re-queries the referenced message after commit
causing potential stale/missing reply previews; instead, when hasReply and
input.payload.referencedMessageId are validated inside the transaction, reuse
the previously fetched row (the refMsg/validated result obtained in-transaction)
to populate referencedMessage rather than issuing a new db.select after commit;
construct referencedMessage from that refMsg (assign id, content and author
fields) within the transaction scope so the emitted realtime payload uses the
same validated data.
In `@apps/web/src/components/chat/composer/message-input.tsx`:
- Around line 95-102: The component currently calls the onSend callback and
immediately clears the editor, reply target, and attachments; change it to await
a success signal from onSend and only perform cleanup when it resolves
successfully. Update the onSend prop usage in message-input.tsx to expect a
Promise<boolean> (or similar success result) from onSend, call await
onSend(content, options), and only if the result indicates success then clear
the editor state, reply target, and attachments (the existing
clearEditor/clearAttachments/setReplyTargetCleared code paths); if onSend
rejects or returns failure, keep the composer state intact so optimistic
rollbacks or retries work correctly. Ensure callers of useMessageSending/onSend
are updated to return the success boolean/promise so the component can gate
cleanup.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx:
- Around line 77-80: The reply selection persists across DM navigation because
we never clear reply state when dmId changes; add an effect that calls
clearReply whenever dmId changes. Specifically, in the component that uses
useReplyState (replyingTo, setReplyingTo, clearReply) add a useEffect watching
dmId (and clearReply) and invoke clearReply() inside it—mirror the clearReply
effect used in the $guildSlug/$channelId.tsx file so replies from one
conversation are reset when switching DMs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b593806-15b3-4964-bf04-b159651c7215
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
ROADMAP.mdapps/api/src/lib/helpers/openapi/schemas.tsapps/api/src/routes/v1/uploads/handlers.tsapps/api/src/routes/v1/uploads/routes.tsapps/api/src/routes/v1/uploads/schema.tsapps/realtime/src/services/messages.tsapps/web/package.jsonapps/web/src/components/chat/composer/attachment-preview.tsxapps/web/src/components/chat/composer/message-input.tsxapps/web/src/components/chat/drop-zone-overlay.tsxapps/web/src/components/chat/message-attachment.tsxapps/web/src/components/chat/message-item.tsxapps/web/src/hooks/use-file-upload.tsapps/web/src/main.tsxapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxpackages/ui/package.jsonpackages/ui/src/components/sonner.tsx
Comment on lines
+25
to
+27
| <div | ||
| key={attachment.id} | ||
| className="relative flex-shrink-0 rounded-md border border-border bg-background" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check Tailwind version in package.json
find . -name "package.json" -type f | head -20 | xargs grep -l "tailwind" 2>/dev/nullRepository: BuckyMcYolo/townhall
Length of output: 138
🏁 Script executed:
# Look for tailwind config
find . -name "tailwind.config.*" -type fRepository: BuckyMcYolo/townhall
Length of output: 46
🏁 Script executed:
# Check the actual file content at the specified location
cat -n apps/web/src/components/chat/composer/attachment-preview.tsx | sed -n '20,35p'Repository: BuckyMcYolo/townhall
Length of output: 846
🌐 Web query:
Tailwind CSS v4 flex-shrink utility naming changes
💡 Result:
In Tailwind CSS v4, the old flex-shrink-* utilities were removed (they were already deprecated in v3) and replaced with the shorter shrink-* naming scheme. [1]
Mapping:
flex-shrink→shrink[2]flex-shrink-0→shrink-0[2]flex-shrink-{n}→shrink-{n}(e.g.flex-shrink-2→shrink-2) [1]
Tailwind v4 also supports:
shrink-[<value>](arbitrary values)shrink-(<custom-property>)(CSS variable shorthand) [2]
Sources:
[1] Tailwind CSS Upgrade Guide (v3 → v4) — deprecated utilities removed/replaced
[2] Tailwind CSS docs: flex-shrink (v4) — shows shrink, shrink-0, and related syntaxes
🏁 Script executed:
# Check Tailwind version in the web app package.json
cat apps/web/package.json | jq '.devDependencies.tailwindcss, .dependencies.tailwindcss'Repository: BuckyMcYolo/townhall
Length of output: 78
Replace deprecated Tailwind v3 utility with v4 equivalent.
The project uses Tailwind v4 (^4.1.18), which removed flex-shrink-0 and replaced it with shrink-0. Using the old class name will not prevent the preview cards from shrinking. Update to the new utility:
Suggested fix
- className="relative flex-shrink-0 rounded-md border border-border bg-background"
+ className="relative shrink-0 rounded-md border border-border bg-background"📝 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
| <div | |
| key={attachment.id} | |
| className="relative flex-shrink-0 rounded-md border border-border bg-background" | |
| <div | |
| key={attachment.id} | |
| className="relative shrink-0 rounded-md border border-border bg-background" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/chat/composer/attachment-preview.tsx` around lines 25
- 27, The div rendering each attachment (the element with key={attachment.id} in
the attachment preview JSX) uses the deprecated Tailwind v3 utility
"flex-shrink-0"; update the className to use the Tailwind v4 equivalent
"shrink-0" (replace "flex-shrink-0" with "shrink-0" in the className string) so
the preview cards no longer shrink.
Comment on lines
+553
to
+558
| const uploadedAttachments = getUploadedAttachments() | ||
| const hasAttachments = uploadedAttachments.length > 0 | ||
| const hasContent = trimmed.length > 0 | ||
|
|
||
| if ((!hasContent && !hasAttachments) || isSending) return | ||
| if (hasContent && trimmed.length > MAX_MESSAGE_LENGTH) return |
There was a problem hiding this comment.
Don't enable send for attachments that aren't actually sendable.
canSend uses pendingAttachments.length, but handleSend only submits getUploadedAttachments(). After an upload error, the send button stays enabled even though clicking it becomes a no-op. Derive canSend from successfully uploaded attachments instead.
Also applies to: 645-649
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/chat/composer/message-input.tsx` around lines 553 -
558, The send button currently bases canSend on pendingAttachments.length while
handleSend uses getUploadedAttachments(), so failed uploads leave the button
enabled but clicking does nothing; change canSend (and the analogous check near
the other block mentioned) to derive from getUploadedAttachments().length and/or
uploadedAttachments (the result of getUploadedAttachments()) and also ensure
isSending/content length checks remain; update any early-return logic in
handleSend/send-related handlers to use uploadedAttachments (not
pendingAttachments) so only actually uploaded attachments enable sending.
Comment on lines
+17
to
+26
| <video | ||
| src={attachment.url} | ||
| controls | ||
| preload="metadata" | ||
| className="max-h-[300px] max-w-[400px] rounded-md" | ||
| > | ||
| <track kind="captions" /> | ||
| </video> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty caption tracks provide no accessibility value.
The <track kind="captions" /> elements are placeholders that satisfy linting but don't provide actual captions. For proper accessibility, consider either removing them (if captions aren't available) or adding a comment explaining the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/chat/message-attachment.tsx` around lines 17 - 26,
The <track kind="captions" /> element is an empty placeholder and provides no
accessibility value; remove this empty <track> node from the video element (the
JSX that uses attachment.url) so you don't expose meaningless caption tracks, or
if you intentionally left it as a placeholder add a clear inline comment
explaining that captions are unavailable and why the empty track remains; update
the video JSX accordingly (the element that renders the video with
src={attachment.url}) to reflect either removal or the explanatory comment.
Comment on lines
+127
to
+140
| let accepted: PendingAttachment[] = [] | ||
| setAttachments((prev) => { | ||
| const remaining = MAX_ATTACHMENTS - prev.length | ||
| accepted = prepared.slice(0, remaining) | ||
| if (prepared.length > remaining) { | ||
| toast.error( | ||
| `You can only attach up to ${MAX_ATTACHMENTS} files per message` | ||
| ) | ||
| } | ||
| if (accepted.length === 0) return prev | ||
| return [...prev, ...accepted] | ||
| }) | ||
|
|
||
| if (accepted.length === 0) return |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting accepted files calculation for clarity.
The pattern of declaring accepted outside setAttachments and assigning it inside works correctly because React's state setter executes synchronously. However, this pattern is unconventional and could confuse future maintainers.
♻️ Alternative approach for clarity
- // Atomically check remaining slots and append
- let accepted: PendingAttachment[] = []
- setAttachments((prev) => {
- const remaining = MAX_ATTACHMENTS - prev.length
- accepted = prepared.slice(0, remaining)
- if (prepared.length > remaining) {
- toast.error(
- `You can only attach up to ${MAX_ATTACHMENTS} files per message`
- )
- }
- if (accepted.length === 0) return prev
- return [...prev, ...accepted]
- })
-
- if (accepted.length === 0) return
+ // Atomically check remaining slots and append
+ let acceptedRef: PendingAttachment[] = []
+ setAttachments((prev) => {
+ const remaining = MAX_ATTACHMENTS - prev.length
+ const toAccept = prepared.slice(0, remaining)
+ if (prepared.length > remaining) {
+ toast.error(
+ `You can only attach up to ${MAX_ATTACHMENTS} files per message`
+ )
+ }
+ if (toAccept.length === 0) return prev
+ acceptedRef = toAccept // Capture for upload phase
+ return [...prev, ...toAccept]
+ })
+
+ if (acceptedRef.length === 0) returnAdding a comment or renaming to acceptedRef makes the intent clearer that this variable captures state for use after the setter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/use-file-upload.ts` around lines 127 - 140, The current
pattern declares and assigns the local variable `accepted` inside the
`setAttachments` updater which is unconventional and confusing; move the
calculation of accepted files out of the updater (compute `accepted =
prepared.slice(0, MAX_ATTACHMENTS - prev.length)` or compute `remaining` and
`accepted` before calling `setAttachments`) so you call `setAttachments(prev =>
accepted.length === 0 ? prev : [...prev, ...accepted])` with `accepted` already
determined, and keep the existing `toast.error` logic when `prepared.length >
remaining`; alternatively rename to `acceptedRef` or add a clear comment if you
must capture it for use after the setter — update references to `accepted`,
`setAttachments`, `MAX_ATTACHMENTS`, `prepared`, and the `toast.error` branch
accordingly.
Comment on lines
+22
to
+25
| ## Phase 1 — Core UX Gaps | ||
|
|
||
| - [x] File/image attachment uploads (R2) | ||
| - [ ] Message deletion |
There was a problem hiding this comment.
Add reply support to the shipped roadmap items.
This PR ships message replies as well as attachments, but the roadmap update only reflects the attachment half. That leaves ROADMAP.md stale immediately after merge. Please add a completed item for replies here or fold it into the completed entry so the roadmap matches the delivered scope.
📝 Suggested update
-## Phase 1 — Core UX Gaps
-
-- [x] File/image attachment uploads (R2)
+## Phase 1 — Core UX Gaps
+
+- [x] Message replies
+- [x] File/image attachment uploads📝 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
| ## Phase 1 — Core UX Gaps | |
| - [x] File/image attachment uploads (R2) | |
| - [ ] Message deletion | |
| ## Phase 1 — Core UX Gaps | |
| - [x] Message replies | |
| - [x] File/image attachment uploads | |
| - [ ] Message deletion |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ROADMAP.md` around lines 22 - 25, Update the Phase 1 — Core UX Gaps list in
ROADMAP.md to reflect that message replies were shipped: either add a checked
item like "- [x] Message replies" under the same section or merge it into the
existing "- [x] File/image attachment uploads (R2)" entry so the roadmap matches
the delivered scope; ensure the checkbox is checked and the wording matches
other items for consistency (e.g., "Message replies" or "Replies and
attachments").
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements message replies and file attachments end-to-end (types, API, realtime, frontend, and UI), adds presigned S3 uploads, and updates related tooling/config.
Key Changes
Cross-layer referenced-message support:
API & storage:
Realtime service:
Queries / performance:
Frontend (web):
Packages & config:
Quality Observations
Strengths
Risks / Issues
Recommended follow-ups
Confidence Score: 4/5
Well-structured and comprehensive cross-layer changes with attention to types and performance. Main concerns are missing tests and verifying deployment S3 configuration; otherwise ready with minor follow-ups.