Polls in messages and server discovery/customization updates#184
Merged
acarlson33 merged 1 commit intoMay 5, 2026
Merged
Conversation
FirepitProject ID: Tip Function builds can take up to 45 minutes before timing out |
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (53)
📝 WalkthroughWalkthroughThis PR introduces a message polling feature with voting and closure workflows, and simultaneously adds server-level customization controls: visibility flags, descriptions, branding assets, and a ChangesMessage Polls
Server Privacy, Customization & Admin Management
Dependency Updates
Documentation
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Chat Page
participant Hook as useMessages Hook
participant API as POST /api/messages/[id]/poll-votes
participant Server as Backend
participant DB as Appwrite DB
User->>Frontend: Click poll option
Frontend->>Hook: votePoll(messageId, optionId)
Hook->>Hook: Apply optimistic vote state
Frontend->>Frontend: Render updated poll UI
Hook->>API: POST {optionId}
activate API
API->>Server: Authenticate session
Server->>DB: List existing vote for user
DB-->>Server: Vote document (or null)
alt Existing Vote
Server->>DB: Update vote optionId
else New Vote
Server->>DB: Create vote document
end
DB-->>Server: Updated/created vote
Server->>DB: Fetch poll & votes
DB-->>Server: Poll state
Server->>Server: Build MessagePoll with counts
API-->>Hook: {poll: updated poll}
deactivate API
Hook->>Hook: Replace poll state with server response
Frontend->>Frontend: Render server poll state
User->>Frontend: Network error occurs
Hook->>Hook: Revert to previous poll state
Frontend->>Frontend: Show error toast
Frontend->>Frontend: Restore original UI
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR spans two substantial, independent feature domains: polls (new API routes, DB schema, component hierarchy, state management) and server privacy/customization (new/updated endpoints, admin controls, UI panels, permissions). The changes involve dense logic across authentication, authorization, data normalization, optimistic updates, and multi-tier component wiring. While changes within each domain follow consistent patterns, the breadth across many files (80+ modified/new files) and diverse logic densities (from simple field additions to intricate optimistic sync and permission enforcement) requires careful, separate reasoning for each area. Homogeneous test additions and dependency bumps reduce total load modestly. Possibly Related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
Owner
Author
This was referenced Apr 19, 2026
Merged
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/messages/route.ts (1)
245-283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRollback the message if poll creation fails.
After the message insert succeeds, any failure while creating the poll leaves the plain question message stored but still returns 500. That creates orphaned poll messages and makes retries duplicate the content. This second write needs compensating cleanup or another atomic pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/messages/route.ts` around lines 245 - 283, Wrap the poll-creation block (the databases.createDocument call that creates pollDocument and the subsequent buildMessagePoll logic) in a try/catch so that if any error occurs you perform a compensating delete of the already-created message (use databases.deleteDocument with env.databaseId, env.collections.messages and String(res.$id) and the same permissions) before rethrowing or returning the error; ensure you await the delete, handle failures of the delete minimally (log or ignore) and keep references to parsedPoll, res.$id, databases.createDocument, and buildMessagePoll to locate the code to modify.src/components/chat-thread-content.tsx (1)
17-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve moderator close rights in the thread pane.
canCloseonly checksmessage.poll.createdBy === currentUserIdhere, so users who can close a poll fromsrc/components/chat-surface-message-item.tsxviacanManageMessageslose that action after opening the thread. This needs the same permission prop/predicate to keep poll-closing behavior consistent across views.Also applies to: 76-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat-thread-content.tsx` around lines 17 - 37, The thread pane currently derives canClose solely from message.poll.createdBy === currentUserId which drops moderator/manager rights; add a permission prop (e.g., canManageMessages?: (message: ChatSurfaceMessage) => boolean) to ChatThreadContentProps and use it when computing canClose (use message.poll.createdBy === currentUserId || canManageMessages?.(message)). Update any other canClose checks in this component (the other instances referenced around the 76-98 block) to use the same combined predicate so poll-closing behavior matches chat-surface-message-item's canManageMessages 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 `@docs/openapi-doc.yml`:
- Around line 879-882: The PATCH schema is missing admin-only documentation for
the defaultOnSignup field; update the PATCH operation's schema where the sibling
isPublic field and the defaultOnSignup field are defined to add a clear
description on defaultOnSignup stating it is "admin-only" (e.g., "Only instance
admins may modify this field") and also update the PATCH 403 response
description to mirror the GET's "(requires instance admin)" note so clients
understand why a PATCH updating defaultOnSignup would be rejected; locate the
defaultOnSignup field in the PATCH request body schema and the PATCH operation's
403 response to apply these changes.
- Around line 828-842: The response currently defines an inline schema for the
"server" property exposing only $id and name; replace that inline object schema
under the "server" property with a $ref to the shared schema (e.g.
"#/components/schemas/Server" or "#/components/schemas/ServerPreview") so the
endpoint returns the full server record and stays in sync; update the "server"
property's schema to use the $ref and remove the inline properties ($id, name)
to avoid duplication.
In `@src/__tests__/api-routes/messages.test.ts`:
- Around line 289-339: The test suite lacks coverage for the partial-failure
case where the first createDocument succeeds (message created) but the second
(poll creation) rejects; add a new test that uses mockGetServerSession and
mockCreateDocument.mockResolvedValueOnce(...) followed by
mockCreateDocument.mockRejectedValueOnce(new Error("Database write failed")),
then call POST(request) and assert the route's intended behavior: either it
returns 500 and triggers a rollback by calling the message deletion helper
(e.g., expect(mockDeleteDocument).toHaveBeenCalledWith("test-db",
"messages-collection", "msg-poll", expect.any(Array))) or, if the design returns
a partial success, assert the exact status and response shape; reference the
POST handler and the mockCreateDocument/mockDeleteDocument mocks to implement
the test and assertions.
In `@src/__tests__/chat-hooks-useMessages.test.ts`:
- Around line 720-889: Add tests that cover optimistic updates and error
rollbacks for useMessages.votePoll and useMessages.closePoll: simulate fetch
rejecting or returning ok: false (stub global fetch via vi.stubGlobal) and
assert that after initiating votePoll/closePoll the messages show the optimistic
intermediate state, then after the failed response the poll state
(options.voterIds and poll.status) reverts to the original initialPoll values;
include separate tests for votePoll rollback and closePoll rollback and reuse
the existing mock initialPoll, renderHook result, and fetchMock pattern to
verify both intermediate optimistic state and final reverted state.
In `@src/app/admin/server-actions.ts`:
- Around line 234-243: The loop over defaultsResponse.documents contains a
redundant boolean check for server.defaultOnSignup === true because the earlier
query already used Query.equal("defaultOnSignup", true); remove the unnecessary
if statement and directly call databases.updateDocument(...) for each server in
defaultsResponse.documents (referencing defaultsResponse.documents,
server.defaultOnSignup, and
databases.updateDocument/SERVERS_COLLECTION_ID/DATABASE_ID to locate the code).
In `@src/app/api/messages/`[messageId]/poll-votes/route.ts:
- Around line 86-125: The current read-then-create/update flow is racy; replace
it with a true upsert using a deterministic document id (e.g. const voteId =
`${poll.$id}:${user.$id}` or a stable hash) and perform conflict-aware writes
against env.collections.pollVotes: attempt databases.createDocument with
ID.unique replaced by the deterministic voteId and the vote payload (pollId,
userId, optionId, votedAt); if createDocument throws a "document already exists"
conflict, catch that error and call databases.updateDocument(env.databaseId,
env.collections.pollVotes, voteId, { optionId, votedAt }) to converge to a
single vote per (pollId, userId). Remove the initial databases.listDocuments
read and ensure you use the same voteId for both create and update paths.
In `@src/app/api/messages/`[messageId]/poll/close/route.ts:
- Around line 35-39: Wrap the call to databases.getDocument(messageId) in a
try/catch and explicitly handle a missing/invalid message: if getDocument
returns no result or throws a "not found" error, return a 404 response (with a
short message) instead of letting the exception bubble; for other unexpected
errors rethrow or return a 500. Concretely, update the code around
databases.getDocument(...) in the route handler so it checks for a
null/undefined message and catches provider errors indicating "not found" (map
those to 404), while preserving other errors for normal error handling.
In `@src/app/api/messages/route.ts`:
- Around line 131-137: The poll validation only rejects imageFileId and
attachments but not imageUrl, letting callers bypass the restriction; update the
conditional that checks creatingPoll to also include imageUrl (e.g., change the
guard that currently checks creatingPoll && (imageFileId || (attachments?.length
?? 0) > 0) to also check imageUrl) and return the same NextResponse.json error;
apply the same addition to the other analogous validation block that handles
attachments/image checks (the second creatingPoll check) so both places reject
imageUrl for poll messages.
In `@src/app/api/servers/`[serverId]/route.ts:
- Around line 177-199: The Query.limit(100) in the default-clear path can miss
records; when payload.defaultOnSignup === true use databases.listDocuments (the
call using Query.equal("defaultOnSignup", true)) in a paginated loop (or remove
the artificial limit) to fetch all matching documents rather than only 100,
iterating over existingDefaultServers.documents and calling
databases.updateDocument for each non-matching defaultServer.$id (skip when
defaultServer.$id === serverId) until no more pages remain; ensure you reference
payload.defaultOnSignup, databases.listDocuments, Query.limit,
existingDefaultServers.documents, databases.updateDocument, and serverId when
implementing pagination.
- Around line 187-198: The loop that clears other default servers is performing
sequential awaits on databases.updateDocument; change the implementation that
iterates existingDefaultServers.documents (where you check if defaultServer.$id
=== serverId) to collect each databases.updateDocument(...) call into an array
of promises and then await Promise.all(promises) so updates run in parallel;
keep the skip for the current serverId and preserve the same arguments
(env.databaseId, env.collections.servers, defaultServer.$id, { defaultOnSignup:
false }) when creating the promises.
In `@src/app/api/servers/default-signup/route.ts`:
- Around line 9-47: Wrap the entire GET handler body in a try/catch to ensure
any exceptions from getServerSession, getUserRoles, or databases.listDocuments
are caught and returned as structured JSON; inside catch return
NextResponse.json({ error: (error as Error).message || "Internal Server Error"
}, { status: 500 }); keep existing authorization checks and response shape
intact and reference the GET function, getServerSession, getUserRoles, and
databases.listDocuments when locating the code; also add the provided test to
servers-default-signup.test.ts that mocks
getServerSession/getUserRoles/listDocuments to throw and asserts a 500 response
with the error message.
In `@src/app/api/servers/route.ts`:
- Around line 37-41: The code calls databases.listDocuments with
env.collections.memberships without guarding for a missing membership
collection, which will throw; add an explicit check for the membership
collection ID (env.collections.memberships) in the route handler before calling
databases.listDocuments (where membershipsResponse is computed) and return a 400
error with the same message used in join/route.ts ("Memberships are not enabled
on this instance") when it's falsy; ensure the check is placed near the start of
the handler and references env.collections.memberships and the listDocuments
call so the path never reaches databases.listDocuments if memberships are
unconfigured.
In `@src/app/chat/components/ServerBrowser.tsx`:
- Around line 322-332: The inline backgroundImage uses server.bannerUrl directly
in ServerBrowser.tsx (the style block that sets backgroundImage from
server.bannerUrl), which risks CSS/URL injection; fix this by
validating/sanitizing banner URLs on the server API that returns
server.bannerUrl (e.g., enforce allowed schemes (https), allowed host/paths or a
storage ID, normalize/encode the URL, and reject or replace unsafe values), and
optionally update the client to only accept a known-safe token/ID or a
pre-signed URL field (instead of raw user input) before using it in the
backgroundImage style.
- Around line 130-134: The effect calls loadServers but doesn't include it in
the dependency array, which risks stale closures; refactor loadServers into a
stable memoized callback (wrap the loadServers function in useCallback and list
any state/props it reads as its dependencies) and then include that memoized
loadServers in the useEffect dependency array alongside canLoadServers, userId,
and joinedServerIdsKey (alternatively, if you prefer the ref pattern, store the
latest loadServers in a ref and call ref.current() from the effect). Ensure the
symbol names match: useCallback for loadServers and the useEffect that currently
depends on canLoadServers, userId, joinedServerIdsKey.
In `@src/app/chat/hooks/useMessages.ts`:
- Around line 954-1063: votePoll and closePoll only update the top-level
messages via replaceMessagePoll/setMessages, so polls in threads
(activeThreadParent and threadMessages) stay stale or cause early returns;
modify the poll update path to patch both places: after computing optimisticPoll
or receiving payload.poll, call replaceMessagePoll(messageId, poll) (or
setMessages mapping) and also update thread state by replacing the poll on
activeThreadParent if its $id matches messageId and mapping threadMessages
similarly (use setActiveThreadParent and setThreadMessages or the existing
thread-state setters in this hook); likewise on error restore both places to
previousPoll. Ensure all three callbacks reference the same messageId/poll logic
so thread parents and replies stay in sync with main messages.
In `@src/app/chat/page.tsx`:
- Around line 1276-1306: buildPollCommand currently only normalizes quotes but
leaves pipe characters which break the `/poll` parser; update the sanitization
to escape any '|' characters (e.g. replaceAll("|", "\\|") or use .replace(/\|/g,
'\\\\|')) for both pollQuestion and each entry in pollOptions before trimming
and validating, then use those escaped values when building the command (refer
to buildPollCommand, pollQuestion, pollOptions, sanitizedQuestion,
sanitizedOptions).
In `@src/components/chat-pinned-messages-content.tsx`:
- Around line 130-137: The pinned-polls UI always passes currentUserId={null}
into MessagePollBlock because ChatPinnedMessagesContentProps lacks a
currentUserId prop; add an optional currentUserId?: string to the
ChatPinnedMessagesContentProps type/interface and thread it through the
ChatPinnedMessagesContent component, then replace currentUserId={null} with
currentUserId={currentUserId ?? null} when rendering MessagePollBlock so the
viewer's identity is used to show their vote.
In `@src/components/create-server-dialog.tsx`:
- Around line 147-160: The Switch component controlling "Public discovery" is
missing an accessible label; add an id to the label text (the <p> containing
"Public discovery") and associate it with the Switch by passing aria-labelledby
(or aria-label) to the Switch component so screen readers hear the label; update
the JSX around the Switch (the Switch element using props checked={isPublic},
onCheckedChange={setIsPublic}, disabled={isCreating}) to include
aria-labelledby="..." that references the new id (or provide a descriptive
aria-label) and ensure the id is unique and matches the visible label.
In `@src/components/message-poll.tsx`:
- Around line 56-62: Currently vote() only disables the clicked option by
calling setSubmittingVote(optionId), allowing other options to be clicked while
the request is in flight; change the logic to block the whole poll during
submission by introducing or using a boolean submitting state (e.g.,
isSubmittingVote) or a sentinel value (e.g., setSubmittingVote('ALL')) so all
option buttons consult that state and disable when submitting is true/'ALL';
update vote(), the option button disabled checks, and the similar logic
referenced around the code at the second occurrence (lines ~149-150) to set
submitting on try, clear it in finally, and prevent any other vote calls until
cleared.
- Around line 30-40: The effect that calls setLocalPollState(null) on every poll
prop change causes successful local votes/closes to be wiped when the parent
re-renders; remove that effect and instead synchronize only when the poll
identity changes (e.g., useEffect that sets localPollState = poll only when
poll?.id differs or when localPollState is null), so localPollState persists
across parent rerenders until a truly new poll arrives; update the useEffect
referencing poll (or remove it) and keep pollState = localPollState ?? poll
as-is.
In `@src/lib/polls-server.ts`:
- Around line 15-66: The two duplicated normalization functions
normalizePollDocument and normalizePollVoteDocument should be extracted into a
shared module (e.g., create src/lib/polls.ts) and exported so both
src/lib/polls-server.ts and src/lib/appwrite-polls.ts can import them; move the
implementations verbatim into that file, export the functions, remove the local
copies from polls-server.ts and appwrite-polls.ts, and add imports for
normalizePollDocument and normalizePollVoteDocument where needed to ensure a
single source of truth.
---
Outside diff comments:
In `@src/app/api/messages/route.ts`:
- Around line 245-283: Wrap the poll-creation block (the
databases.createDocument call that creates pollDocument and the subsequent
buildMessagePoll logic) in a try/catch so that if any error occurs you perform a
compensating delete of the already-created message (use databases.deleteDocument
with env.databaseId, env.collections.messages and String(res.$id) and the same
permissions) before rethrowing or returning the error; ensure you await the
delete, handle failures of the delete minimally (log or ignore) and keep
references to parsedPoll, res.$id, databases.createDocument, and
buildMessagePoll to locate the code to modify.
In `@src/components/chat-thread-content.tsx`:
- Around line 17-37: The thread pane currently derives canClose solely from
message.poll.createdBy === currentUserId which drops moderator/manager rights;
add a permission prop (e.g., canManageMessages?: (message: ChatSurfaceMessage)
=> boolean) to ChatThreadContentProps and use it when computing canClose (use
message.poll.createdBy === currentUserId || canManageMessages?.(message)).
Update any other canClose checks in this component (the other instances
referenced around the 76-98 block) to use the same combined predicate so
poll-closing behavior matches chat-surface-message-item's canManageMessages
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c70c0a96-4a84-471c-bb8f-c5d13337660c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lockand included bybun.lock
📒 Files selected for processing (53)
docs/SERVER_ADMINISTRATION.mddocs/openapi-doc.ymlpackage.jsonscripts/setup-appwrite.tssrc/__tests__/admin-server-management.test.tsxsrc/__tests__/api-routes/message-poll-close.test.tssrc/__tests__/api-routes/message-poll-votes.test.tssrc/__tests__/api-routes/messages.test.tssrc/__tests__/api-routes/server-route-settings.test.tssrc/__tests__/api-routes/servers-create-route.test.tssrc/__tests__/api-routes/servers-default-signup.test.tssrc/__tests__/api-routes/servers-join.test.tssrc/__tests__/api-routes/servers-public.test.tssrc/__tests__/api-routes/servers.test.tssrc/__tests__/appwrite-messages-enriched.test.tssrc/__tests__/chat-hooks-useMessages.test.tssrc/__tests__/chat-page.test.tsxsrc/__tests__/components/message-poll.test.tsxsrc/__tests__/login-security.test.tssrc/__tests__/polls.test.tssrc/app/(auth)/login/actions.tssrc/app/admin/server-actions.tssrc/app/admin/server-management.tsxsrc/app/api/messages/[messageId]/poll-votes/route.tssrc/app/api/messages/[messageId]/poll/close/route.tssrc/app/api/messages/route.tssrc/app/api/servers/[serverId]/route.tssrc/app/api/servers/create/route.tssrc/app/api/servers/default-signup/route.tssrc/app/api/servers/join/route.tssrc/app/api/servers/public/route.tssrc/app/api/servers/route.tssrc/app/chat/components/ServerBrowser.tsxsrc/app/chat/hooks/useChatSurfaceController.tssrc/app/chat/hooks/useMessages.tssrc/app/chat/page.tsxsrc/components/chat-pinned-messages-content.tsxsrc/components/chat-surface-message-item.tsxsrc/components/chat-surface-panel.tsxsrc/components/chat-thread-content.tsxsrc/components/create-server-dialog.tsxsrc/components/message-poll.tsxsrc/components/server-admin-panel.tsxsrc/components/virtualized-message-list.tsxsrc/lib/appwrite-core.tssrc/lib/appwrite-messages-enriched.tssrc/lib/appwrite-polls.tssrc/lib/appwrite-servers.tssrc/lib/chat-surface.tssrc/lib/polls-server.tssrc/lib/polls.tssrc/lib/server-metadata.tssrc/lib/types.ts
Owner
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved and changes approved. |
Owner
Author
Merge activity
|
acarlson33
changed the base branch from
04-12-basic_markdown_support
to
graphite-base/184
acarlson33
force-pushed
the
04-15-polls_in_messages_and_server_discovery_customization_updates
branch
from
f0efe51 to
93bcfef
Compare
acarlson33
deleted the
04-15-polls_in_messages_and_server_discovery_customization_updates
branch
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.
This pull request introduces several new features and improvements around server management, privacy controls, and poll infrastructure, as well as some dependency updates and expanded API documentation. The most significant changes include new server customization options, expanded API endpoints and documentation, poll support in the database setup, and improved test coverage for server admin controls.
Server management and privacy enhancements:
PATCH /api/servers/{serverId}endpoint to support updating server name, description, icon, banner, and visibility; only owners and members withmanageServerpermission can update these fields. [1] [2]iconFileId,bannerFileId,isPublic, anddefaultOnSignup, with corresponding schema and API documentation updates. [1] [2] [3]API and documentation improvements:
/api/servers/default-signup,/api/servers/{serverId}PATCH), new properties, and improved response schemas. [1] [2] [3]Database and setup scripts:
pollsandpoll_votescollections, their fields, and indexes. [1] [2]serverscollection to include new fields and indexes forisPublicanddefaultOnSignup.Testing and admin tooling:
Dependency updates:
appwrite,nanoid,newrelic,posthog-js,react-virtuoso,@typescript-eslint,baseline-browser-mapping,happy-dom, andprettier. [1] [2] [3]