minor: add Mastodon-compatible direct messages#718
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive direct messaging and conversation system, including new database tables for tracking threads and participant memberships, as well as a dedicated Messages UI. It adds several Mastodon-compatible API endpoints for fetching, reading, and hiding conversations. Feedback focuses on optimizing database performance by addressing N+1 query patterns during conversation syncing and root status resolution. Additionally, there are recommendations to ensure consistent status ordering in thread views, fix a logic error in the account lookup fallback, and implement pagination in the messaging interface.
There was a problem hiding this comment.
Pull request overview
Adds Mastodon-compatible direct messages: new conversation tables and backfill migration, sync helpers and DirectConversationDatabase operations, REST endpoints under /api/v1/conversations, a /messages UI, and extended OAuth child scopes (read:statuses, write:statuses, write:conversations) backed by an updated guard that honors parent-scope hierarchy. Direct statuses are routed to a new direct timeline, kept out of home/noannounce/main timelines, and federated only to explicit recipients via a new shared getFederatedStatusDeliveryInboxes helper.
Changes:
- New
direct_conversations*tables, backfill migration, andDirectConversationSQLDatabaseMixinfor syncing/reading conversations and memberships. - Mastodon-compatible REST endpoints, client helpers, and
/messagespage with compose/reply/read/hide flows; newMastodon.Conversationtype. - Federation/visibility updates: direct messages require explicit recipients, direct deletes target original recipients only, and a shared inbox-resolution helper consolidates
sendNote/sendUpdateNote/deleteStatusdelivery.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/20260517002000_add_direct_conversations.js | Adds tables and backfills conversations/participants/memberships from existing direct statuses. |
| lib/types/mastodon/conversation.ts, lib/types/mastodon/index.ts | New Mastodon Conversation schema export. |
| lib/types/database/operations.ts | Adds DirectConversation* types, ops interface, and new OAuth child scopes. |
| lib/database/types.ts, lib/database/sql/index.ts | Wires DirectConversationDatabase into the SQL composite database. |
| lib/database/sql/conversation.ts (+test) | Implements sync/read/hide/mark-read logic with cursor pagination. |
| lib/database/sql/timeline.ts | Handles DIRECT in actor-scoped timeline queries. |
| lib/services/timelines/{types,index}.ts (+test) | Adds DIRECT timeline and routes direct statuses there, excluding main/noannounce. |
| lib/services/mastodon/getMastodonConversation.ts | Serializes a DirectConversation into Mastodon format. |
| lib/services/federation/statusDelivery.ts | Shared inbox resolver for status delivery (public/followers + explicit recipients). |
| lib/jobs/sendNoteJob.ts, lib/jobs/sendUpdateNoteJob.ts | Switch to shared inbox resolver. |
| lib/actions/createNote.ts, lib/actions/createPoll.ts, lib/actions/visibility.integration.test.ts | Reject direct without explicit recipient; updated tests. |
| lib/actions/deleteStatus.ts, lib/activities/index.ts, lib/activities/deleteStatus.ts | Pass to/cc for direct deletes through the AP Delete activity. |
| lib/services/guards/OAuthGuard.ts (+test) | Centralizes scope checking with parent (read/write) hierarchy. |
| app/api/v1/timelines/[timeline]/route.ts, app/api/v1/statuses/route.ts | Switch to OAuthGuardAnyScope with new child scopes. |
| app/api/v1/conversations/* | New GET list, GET statuses, DELETE hide, POST read endpoints. |
| app/api/v1/accounts/{search,lookup}/route.ts | Webfinger-resolve unknown handles when resolve=true. |
| app/(timeline)/messages/{page,MessagesPage}.tsx | New server page and client UI for messages. |
| lib/client.ts | New client helpers for conversations, account search, and direct message sending. |
| lib/components/layout/nav-items.* (+mobile-nav.test) | Add Messages nav entry. |
llun
left a comment
llun
left a comment
There was a problem hiding this comment.
Code Review – PR #718: Mastodon-compatible direct messages
Overall this is a well-scoped feature. The database schema, migration, federation delivery refactor, and OAuth scope extensions all look intentional. The issues below need to be addressed before merge.
Must-Fix
1. Group DM replies silently drop participants not in the status text
In lib/actions/createNote.ts and lib/actions/createPoll.ts, when visibility=direct and in_reply_to_id is set, statusRecipientsTo builds the to array from mentions.map(href) + replyStatus.actorId. Any third-party Mastodon client that sends a reply without typing @mention for every group participant will only deliver to the original author — the other group members are silently dropped.
The built-in UI works because createDirectMessage in lib/client.ts always prefixes @mention tags for all composerRecipients, but this is an API-level contract violation.
Fix: When the status is a direct reply, also fold in the full to list from the parent status so all existing participants are preserved regardless of mention text.
2. accounts/lookup splits on @ without validating segment count
app/api/v1/accounts/lookup/route.ts:
const [username, domain] = normalizedAcct.split('@')'user@host@domain'.split('@') yields three segments; destructuring silently takes domain = 'host', causing a WebFinger lookup against the wrong host. The accounts/search route already uses parseAccountHandle which rejects inputs with more than two segments — use the same helper here.
3. Migration down does not restore removed timeline rows — silent data loss on rollback
migrations/20260517002000_add_direct_conversations.js: the up function deletes all pre-existing direct-message entries from main/home/noannounce timelines. The down function drops the new tables but never re-inserts those rows. After a rollback, pre-existing DMs are permanently gone from the old timelines, and orphaned direct timeline rows accumulate.
Either make the migration explicitly irreversible (add a comment saying so and remove the down function body, or throw), or restore the deleted rows in down.
4. getMastodonConversation returns Promise<Conversation | null> but z.parse() throws instead of returning null
lib/services/mastodon/getMastodonConversation.ts:
return Mastodon.Conversation.parse({ ... }) // throws ZodError on bad shapeA malformed last_status will cause an unhandled exception propagating to the API as a 500.
Fix: Use .safeParse() and return null on failure, or wrap in try/catch. The same issue applies to the conversation list in app/api/v1/conversations/route.ts where the filter(conversation !== null) guard won't catch a thrown error.
5. Missing read:conversations / write:conversations OAuth scopes
app/api/v1/conversations/route.ts and app/api/v1/conversations/[id]/statuses/route.ts accept [Scope.enum.read, Scope.enum['read:statuses']]. The Mastodon spec defines read:conversations for conversations endpoints. Third-party clients that request only read:conversations will receive 401s.
Fix: Add read:conversations and write:conversations to the Scope enum in lib/types/database/operations.ts, include them in UsableScopes, and add them to the allowed-scope lists on the conversations routes.
6. Double loadThread call on first message to a new recipient
app/(timeline)/messages/MessagesPage.tsx: after sendMessage resolves, the code does:
setSelectedConversationId(nextConversationId) // triggers useEffect → loadThread
// ...
await loadThread(nextConversationId) // explicit second callWhen creating a brand-new conversation, both the useEffect (triggered by the state change) and the explicit call fire concurrently, causing two overlapping API requests and two setThreadStatuses updates.
Fix: Only call loadThread explicitly in sendMessage when nextConversationId === selectedConversationId (i.e. the conversation ID didn't change, so the useEffect won't fire). Otherwise rely solely on the useEffect.
7. No tests for the new OAuth scope hierarchy
The PR introduces hasGrantedScope — a token with read now satisfies read:statuses, read:conversations, etc. This is a meaningful change to auth behaviour with zero test coverage.
Fix: Add unit tests in lib/services/guards/OAuthGuard.test.ts covering at minimum:
- parent scope (
read) satisfies child scope (read:statuses) ✓ - sibling does NOT satisfy sibling (
read:statusesdoes not satisfywrite:statuses) ✓ - child does NOT satisfy parent ✓
Should-Fix
A. Migration backfill loads all Note/Poll rows into memory at once
getDirectStatusRows fetches every row from statuses with no pagination. On a large instance this will cause an OOM during migration. Batch by ID range.
B. N+1 queries in app/(timeline)/messages/page.tsx
Lines 37–45 call getMastodonActorFromId once per participant per conversation. With 20 conversations × 3 participants = 60 individual queries. Add a bulk getMastodonActorsFromIds lookup.
C. Layer coupling: isDirectStatus imported from SQL layer into services layer
lib/services/timelines/index.ts imports isDirectStatus from lib/database/sql/conversation. Move the helper to a shared utility (e.g. lib/utils/status.ts) that both layers import from.
D. Timeline.DIRECT is now a live endpoint at /api/v1/timelines/direct
This is redundant with /api/v1/conversations and leaks an internal implementation detail. Consider adding Timeline.DIRECT to the UNSUPPORTED_TIMELINE blocklist in the timelines route, or explicitly document the intentional overlap in a comment.
E. Magic string 'activities_next' in conversations route
app/api/v1/conversations/route.ts:
url.searchParams.get('format') === 'activities_next'All other routes in the diff use TimelineFormat.enum.activities_next. Use the enum constant here too.
F. refreshConversations not wrapped in useCallback
app/(timeline)/messages/MessagesPage.tsx: refreshConversations is a plain async () => recreated on every render, inconsistent with loadThread which uses useCallback. Wrap it for consistency.
Generated by Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa66097162
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review pass addressed in 10863f3. Fixed: group DM reply recipients for notes/polls, strict account lookup parsing, irreversible migration rollback handling, safe Mastodon conversation parsing, read:conversations OAuth support plus scope hierarchy tests, duplicate Messages thread load after send, migration backfill batching, direct-status helper placement, TimelineFormat enum use, Messages pagination/read refresh handling, direct delete cc preservation, visibility-filtered conversation status hydration, stale lastStatus fallback, and malformed conversation id/cursor validation. Not changed: the Mastodon Conversation id remains the per-actor membership id by design from the feature plan, with the shared conversation id kept internal as activities_next conversationId. I also left the /messages server-page participant account lookups as-is because that page is bounded to 20 conversations and a true bulk Mastodon-account lookup would require a broader database interface change outside this review loop. /api/v1/timelines/direct remains intentionally supported for Mastodon compatibility and now has an inline note. |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive direct messaging system, featuring a new Messages UI, API endpoints for conversation management, and database support for threaded direct messages. Key updates include remote account resolution via Webfinger, refactored status delivery for direct visibility, and a migration to backfill existing direct statuses into the new conversation structure. Review feedback highlights the need to address a potential race condition in the message thread loading logic and suggests optimizing unread status updates to reduce unnecessary network traffic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10863f3f77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
llun
left a comment
llun
left a comment
There was a problem hiding this comment.
Second-Round Review – PR #718
Great update. All 7 must-fix issues are resolved, and 5 of 6 should-fix items are addressed. The update does introduce 4 new issues that need attention.
Previous Issues — Status
| # | Issue | Status |
|---|---|---|
| 1 | Group DM replies drop participants | ✅ FIXED — statusRecipientsTo now spreads replyStatus.to |
| 2 | accounts/lookup unsafe @-split |
✅ FIXED — local parseAccountHandle rejects >2 segments, returns 400 |
| 3 | Migration down loses data silently |
✅ FIXED — down now throws an explicit Error (irreversible, documented) |
| 4 | getMastodonConversation throws instead of returning null |
✅ FIXED — uses .safeParse() |
| 5 | Missing read:conversations / write:conversations scopes |
✅ FIXED — all four scopes added to enum, UsableScopes, and auth.ts |
| 6 | Double loadThread on new conversation |
✅ FIXED — explicit call only fires when nextConversationId === selectedConversationId |
| 7 | No tests for hasGrantedScope scope hierarchy |
✅ FIXED — four new tests cover parent→child, sibling rejection, child→parent rejection |
| A | Migration backfill OOM risk | ✅ FIXED — cursor-based batching with DIRECT_STATUS_BATCH_SIZE = 500 |
| B | N+1 queries in messages/page.tsx |
❌ NOT FIXED — getMastodonActorFromId still called once per participant per conversation |
| C | isDirectStatus layer coupling |
✅ FIXED — moved to lib/utils/directStatus.ts |
| D | Timeline.DIRECT live endpoint |
✅ ACCEPTED — intentionally retained with a comment explaining Mastodon client compatibility |
| E | Magic string 'activities_next' |
✅ FIXED — uses TimelineFormat.enum.activities_next consistently |
| F | refreshConversations not in useCallback |
✅ FIXED |
New Issues Introduced
NEW-1 (medium): hasGrantedScope passes vacuously when scopes: [] in JWT path
OAuthGuard.ts now calls verifyAccessToken with scopes: [] on the JWT path, then delegates all scope checking to hasRequiredScopes. When the guard is invoked with matchMode: 'all' and an empty required-scopes list, [].every(...) is vacuously true — any valid JWT passes regardless of what scopes are in its scope claim. The original code enforced scope checking inside verifyAccessToken itself. This is a defence-in-depth regression: routes that accidentally pass an empty scope list will now accept tokens with no relevant grants.
Fix: Either keep scope validation inside verifyAccessToken for the JWT path, or add an early guard in hasRequiredScopes / hasGrantedScope that returns false when the required-scopes list is empty.
NEW-2 (low-medium): createDirectMessage silently ignores recipients when replying
lib/client.ts:
const mentionPrefix = replyStatus
? ''
: recipients.map(accountMention).join(' ')When replying, no @mention tags are injected into the status body, so getMentions extracts zero mentions from the text. The server-side null guard (mentions.length === 0 && !isReplyingToDirectThread) avoids a crash, and replyStatus.to is used to populate recipients — which is correct for propagating existing thread members. However, any recipients passed by the caller for a reply (e.g. to add a new participant mid-thread) are silently dropped. If the UI never passes extra recipients for replies this is fine, but it is an invisible contract that could break a future feature.
Fix: If adding participants to a reply is intentional in future, document the limitation with a comment. If it should be supported now, merge recipients into the mention prefix even when replying.
NEW-3 (low): URL↔ID cursor round-trip in getConversationStatuses
lib/client.ts:
if (maxStatusId) url.searchParams.set('max_id', urlToId(maxStatusId))nextMaxStatusId values in state are already full URL strings (the Status.id field). The client converts them to short IDs via urlToId, then the route calls idToUrl to reconstruct the URL. For local statuses this round-trips correctly. For federated statuses whose IDs are external URLs that urlToId cannot reconstruct losslessly, the cursor will be wrong and pagination will silently skip or repeat items.
Fix: Pass the status URL directly in max_id (no urlToId conversion) and adjust the route to accept raw status URLs as cursor values, consistent with how max_id is handled in the home/public timeline routes.
NEW-4 (minor): Several event handlers in MessagesPage.tsx still missing useCallback
refreshConversations was fixed, but addRecipient, removeRecipient, startNewConversation, hideSelectedConversation, and sendMessage are still plain async () => arrow functions recreated on every render. Since loadThread and refreshConversations explicitly use useCallback, these should be consistent — and stale-closure bugs become more likely as dependencies are added.
Remaining from Previous Round
B (not fixed): N+1 queries in app/(timeline)/messages/page.tsx and conversations/route.ts
conversation.participantActorIds
.filter((actorId) => actorId !== actor.id)
.map((actorId) => database.getMastodonActorFromId({ id: actorId }))This still fires one query per participant per conversation in both locations. Worth a follow-up if conversation list performance becomes a concern at scale.
Generated by Claude Code
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive direct messaging and threaded conversation system, including new database tables, SQL logic for conversation management, and a dedicated "Messages" UI. It also updates API endpoints to support Mastodon-compatible conversation entities and implements a scope hierarchy for OAuth permissions. Review feedback focuses on performance optimizations, specifically identifying N+1 query patterns in the database implementation and migration scripts. Additionally, there is a recommendation to avoid performing database updates during hydration steps to prevent unexpected side effects during read operations.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a threaded direct messaging system, including a new Messages UI, backend API routes for conversation management, and a database schema for tracking direct message threads. It also refactors status delivery logic and implements granular OAuth scopes. The review feedback identifies several performance issues related to N+1 query patterns during data hydration in the API routes and suggests adding error handling for asynchronous UI actions to prevent state inconsistency.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2c15a3f4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
/gemini review |
- Block bypass (P1, Codex): syncDirectConversationForStatus now skips membership creation for local recipients that block the sender, so blocked actors no longer see direct conversation entries. - Bearer account lookup (P2, Codex): accounts lookup and search routes now accept the granular read:accounts scope alongside the broader read scope. OptionalOAuthGuard gains a matchMode option so the bearer branch can require any of the listed scopes. - Mark-as-read retry (P2, Codex / Gemini): replace the permanent failed-read set with a cooldown timestamp and a per-conversation retry nonce. Re-selecting a conversation clears the cooldown and triggers a fresh mark attempt; in-flight responses from older requests no longer touch state. - Recipient search (Gemini): replace the auto-pick of the first search result with a results list users select from. Errors from searchAccounts now surface an inline alert instead of bubbling out of the void handler.
- Add 'read:accounts' to the better-auth oauthProvider scope allow-list so least-privilege clients can request it. - Switch status read/edit/delete routes (GET/PUT/DELETE [id], context, source, history) to OAuthGuardAnyScope so tokens scoped to read:statuses / write:statuses are accepted alongside the parent read/write scopes. - Match reply participants in the direct-message client helper through normalizeActorId + idToUrl so encoded MastodonAccount.id values resolve against the reply status to/cc/actorId set. - Use the returned status URI to find the freshly created conversation after sending a new message instead of assuming it is first in the refreshed list. - Add load-more pagination for the messages conversation sidebar. - Export isBearerAuthorizationHeader from OAuthGuard and reuse it in the account lookup route. - Move readRetryNonce alongside the other useState declarations.
- Request limit+1 in refreshConversations and loadMoreConversations so the Load more button accurately reflects server state when the count is exactly the limit. - Use normalizeActorId when mapping conversation participants to accounts and drop the fragile index-based fallback that relied on DB row ordering. - Cap the visible-conversation and conversation-status scan loops with a max-batch counter to guarantee bounded work in pathological cases. - Set the limit URL param when 0 is passed explicitly to getConversations. - Add tests for the sidebar Load more flow (success, has-more, failure). https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
- loadThread and loadMoreStatuses now catch request failures and show an inline error instead of silently leaving the thread blank. - markDirectConversationRead reads and updates the membership row inside a transaction with a row lock so a concurrent status sync cannot have its unread flag overwritten by an interleaved mark-as-read. https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
Replace per-status getMastodonStatus calls with the getMastodonStatuses bulk helper so the timeline and status context endpoints load actor accounts in a single query instead of an N+1 pattern. https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
loadMoreStatuses surfaces an inline error on failure but the Load more button has no error reset, so a successful retry left the previous "Could not load more messages" alert visible. Clear the error when the action starts, matching sendMessage and searchForRecipients. https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
…uble thread fetch - The direct-conversations migration no longer classifies recipient-less Note/Poll rows as direct. Statuses created before the recipients table existed (and before the now-removed visibility column) have no recipient rows regardless of their original visibility, so treating them as direct could pull legacy public posts out of normal timelines. Added a regression test covering a recipient-less status. - sendMessage no longer issues a redundant loadThread call when sending a message that opens a different conversation; selectConversation already triggers the thread-loading effect, so the explicit reload now runs only for same-conversation replies. https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
createNoteFromUserInput returns null only for unprocessable input (a direct post with no explicit recipients, or a fitness-file mismatch), never for a missing resource. The outbox note branch mapped that null to 404, misclassifying client validation errors as not-found. It now returns 422, matching the poll branch. Added a route test. https://claude.ai/code/session_019fqaoJhN53z7Qnqr714yZV
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive messaging system, including a new Messages UI, conversation-threaded API endpoints, and a database schema for tracking direct message threads. It refactors status delivery logic and updates OAuth guards to support granular scopes like read:statuses and read:conversations. The review feedback points out opportunities to optimize the database layer by removing redundant deduplication logic where data uniqueness is already guaranteed by the query structure or primary key constraints.
| const unresolvedFallbackConversationIds = new Set([ | ||
| ...new Set(rowsMissingLastStatus.map((row) => row.conversationId)) | ||
| ]) |
There was a problem hiding this comment.
The creation of unresolvedFallbackConversationIds involves redundant deduplication. Since rowsMissingLastStatus is derived from a query for a single actor and the (actorId, conversationId) pair is unique, the mapped conversation IDs are already unique. Avoid redundant deduplication when uniqueness is guaranteed by the generation logic.
const unresolvedFallbackConversationIds = rowsMissingLastStatus.map((row) => row.conversationId)References
- Avoid redundant deduplication if the data is already guaranteed to be unique by the generation logic, such as when using a Map with a keying function.
| ...new Set( | ||
| await getLocalParticipantActorIds(trx, participantActorIds) | ||
| ) | ||
| ].filter((actorId) => !excludedActorIdSet.has(actorId)) |
There was a problem hiding this comment.
The use of [...new Set(...)] here is redundant. getLocalParticipantActorIds performs a query that returns unique IDs based on the primary key constraint, and the input is already deduplicated. Avoid redundant deduplication when the data is already guaranteed to be unique.
const localParticipantActorIds = (
await getLocalParticipantActorIds(trx, participantActorIds)
).filter((actorId) => !excludedActorIdSet.has(actorId))References
- Avoid redundant deduplication if the data is already guaranteed to be unique by the generation logic, such as when using a Map with a keying function.
Summary
Adds Mastodon-compatible direct messages backed by direct-visibility statuses and per-actor conversation memberships.
/messagesUI with conversation list, thread view, group-capable compose, read marking, and hide/delete actions.read/writecompatibility.Validation
node -v->v24.13.0yarn run prettier --write .yarn lint(0 errors; 11 pre-existing warnings)yarn buildyarn test(309 suites, 2492 tests)/messageswith a temporary local SQLite database and authenticated throwaway user.