[GSoC-25]; Implemented required changes for subscription at scale #3671
Conversation
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
Signed-off-by: imanishx <[email protected]>
|
To generate Unit Tests for this PR, please click here. |
WalkthroughThis PR adds chat typing (direct vs group) to the system: DB schema adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL as GraphQL API
participant Validator as Input Validation
participant DB as Database
participant PubSub
Client->>GraphQL: mutation createChat(type, participants?, avatar?)
GraphQL->>Validator: validate input (type, participants)
alt type == "direct"
Validator->>GraphQL: ensure exactly 2 distinct participant IDs
GraphQL->>DB: compute sorted hash & query by (org, hash, type)
alt existing direct chat found
DB-->>GraphQL: return existing chat
GraphQL->>DB: ensure memberships exist (create if missing)
else no existing chat
GraphQL->>DB: create chat (type='direct', hash), create 2 memberships (transaction)
end
else type == "group"
GraphQL->>DB: create chat (type='group')
alt avatar provided
GraphQL->>DB: upload avatar and attach
end
GraphQL->>DB: create admin membership for creator
alt participants provided
GraphQL->>DB: create regular memberships for participants
end
end
GraphQL->>PubSub: publish chat event (created)
DB-->>GraphQL: commit transaction
GraphQL-->>Client: return Chat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/graphql/types/Chat/members.test.ts (1)
181-182: Major: Test assertions checking for potentially undefined values are less meaningful.The assertions
expect(ids).toContain(memberA.user?.id)will pass ifmemberA.user?.idis undefined and the array doesn't contain undefined, or if both contain undefined. This reduces the effectiveness of the test validation.After adding null checks for user creation, use direct access in assertions:
-expect(ids).toContain(memberA.user?.id); -expect(ids).toContain(memberB.user?.id); +expect(ids).toContain(memberA.user.id); +expect(ids).toContain(memberB.user.id);-for (const id of [memberA.user?.id, memberB.user?.id]) +for (const id of [memberA.user.id, memberB.user.id]) expect(combined).toContain(id);Also applies to: 209-210
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (31)
drizzle_migrations/20251015160719_perpetual_wasp.sql(1 hunks)drizzle_migrations/meta/_journal.json(1 hunks)schema.graphql(6 hunks)src/drizzle/enums/chatType.ts(1 hunks)src/drizzle/tables/chats.ts(1 hunks)src/graphql/enums/ChatType.ts(1 hunks)src/graphql/enums/index.ts(1 hunks)src/graphql/inputs/MutationCreateChatInput.ts(3 hunks)src/graphql/inputs/SubscriptionChatMessageDeleteInput.ts(1 hunks)src/graphql/inputs/SubscriptionChatMessageUpdateInput.ts(1 hunks)src/graphql/pubsub.ts(1 hunks)src/graphql/types/Chat/Chat.ts(1 hunks)src/graphql/types/Mutation/createChat.ts(2 hunks)src/graphql/types/Mutation/createChatMembership.ts(1 hunks)src/graphql/types/Mutation/deleteChatMessage.ts(1 hunks)src/graphql/types/Mutation/updateChatMembership.ts(1 hunks)src/graphql/types/Mutation/updateChatMessage.ts(1 hunks)src/graphql/types/Subscription/chatMessageCreate.ts(0 hunks)src/graphql/types/Subscription/chatMessageDelete.ts(1 hunks)src/graphql/types/Subscription/chatMessageUpdate.ts(1 hunks)src/graphql/types/Subscription/index.ts(1 hunks)test/graphql/types/Chat/Chat.test.ts(6 hunks)test/graphql/types/Chat/creator.test.ts(0 hunks)test/graphql/types/Chat/members.test.ts(14 hunks)test/graphql/types/Mutation/MarkChatAsRead.test.ts(14 hunks)test/graphql/types/Mutation/createChat.test.ts(11 hunks)test/graphql/types/Mutation/createChatMembership.test.ts(2 hunks)test/graphql/types/Mutation/updateChatMembership.test.ts(16 hunks)test/graphql/types/Query/UnreadChat.test.ts(0 hunks)test/graphql/types/Query/UnreadChats.test.ts(12 hunks)test/graphql/types/gql.tada.d.ts(2 hunks)
💤 Files with no reviewable changes (3)
- src/graphql/types/Subscription/chatMessageCreate.ts
- test/graphql/types/Chat/creator.test.ts
- test/graphql/types/Query/UnreadChat.test.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-02-10T18:57:27.311Z
Learnt from: shivasankaran18
PR: PalisadoesFoundation/talawa-api#3175
File: test/graphql/types/Mutation/updateAgendaItem.test.ts:33-49
Timestamp: 2025-02-10T18:57:27.311Z
Learning: In test/graphql/types/Mutation/updateAgendaItem.test.ts, while a global mock setup is used for the drizzle client, an alternative approach is to use specialized mocks initialized in beforeEach for each test scenario to reduce global state and ensure test isolation. However, this requires careful type handling to maintain type safety.
Applied to files:
test/graphql/types/Query/UnreadChats.test.tstest/graphql/types/Mutation/updateChatMembership.test.tstest/graphql/types/Mutation/createChat.test.tstest/graphql/types/Mutation/createChatMembership.test.ts
📚 Learning: 2025-03-14T03:48:58.166Z
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/graphql/types/Advertisement/updatedAt.test.ts:172-172
Timestamp: 2025-03-14T03:48:58.166Z
Learning: In test/graphql/types/Advertisement/updatedAt.test.ts, variables initialized in beforeEach blocks are recreated before each test, making it safe to modify their values directly within test cases without causing side effects in subsequent tests.
Applied to files:
test/graphql/types/Query/UnreadChats.test.ts
📚 Learning: 2025-02-18T15:03:52.111Z
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3218
File: test/routes/graphql/Query/event.test.ts:107-140
Timestamp: 2025-02-18T15:03:52.111Z
Learning: In test/routes/graphql/Query/event.test.ts, the createAndDeleteTestUser function intentionally combines user creation and deletion in a single function to ensure proper cleanup after test execution, as every user creation needs to be cleaned up.
Applied to files:
test/graphql/types/Query/UnreadChats.test.tstest/graphql/types/Chat/Chat.test.tstest/graphql/types/Chat/members.test.tstest/graphql/types/Mutation/updateChatMembership.test.tstest/graphql/types/Mutation/createChat.test.tstest/graphql/types/Mutation/MarkChatAsRead.test.ts
📚 Learning: 2024-09-22T09:37:15.069Z
Learnt from: disha1202
PR: PalisadoesFoundation/talawa-api#2466
File: src/types/generatedGraphQLTypes.ts:3096-3101
Timestamp: 2024-09-22T09:37:15.069Z
Learning: Direct chats do not need to be associated with any organization, so the `organizationId` field in `ChatInput` remains optional.
Applied to files:
test/graphql/types/Mutation/createChat.test.tsschema.graphql
📚 Learning: 2024-10-12T09:54:09.642Z
Learnt from: syedali237
PR: PalisadoesFoundation/talawa-api#2582
File: src/resolvers/Mutation/sendMembershipRequest.ts:156-170
Timestamp: 2024-10-12T09:54:09.642Z
Learning: In the `sendMembershipRequest` mutation within `src/resolvers/Mutation/sendMembershipRequest.ts`, the team prefers not to introduce transactions for simple operations when existing error handling is sufficient, to avoid unnecessary overhead.
Applied to files:
src/graphql/types/Mutation/createChatMembership.tssrc/graphql/types/Mutation/updateChatMembership.tstest/graphql/types/Mutation/createChatMembership.test.ts
📚 Learning: 2025-01-29T17:47:13.159Z
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#3094
File: src/types/generatedGraphQLTypes.ts:5118-5122
Timestamp: 2025-01-29T17:47:13.159Z
Learning: The file src/types/generatedGraphQLTypes.ts is auto-generated and should not be modified directly. Any changes needed should be made to the GraphQL schema instead.
Applied to files:
src/graphql/enums/index.ts
📚 Learning: 2025-09-29T02:49:37.032Z
Learnt from: BharathC0
PR: PalisadoesFoundation/talawa-api#0
File: :0-0
Timestamp: 2025-09-29T02:49:37.032Z
Learning: For comprehensive test coverage in talawa-api mutation tests, it's essential to test all error scenarios (unauthenticated, invalid arguments, resource not found, business logic violations) and success scenarios using the mercuriusClient pattern with proper GraphQL document nodes and suite/test structure from Vitest.
Applied to files:
test/graphql/types/Mutation/createChatMembership.test.ts
🧬 Code graph analysis (13)
test/graphql/types/Query/UnreadChats.test.ts (2)
test/graphql/types/documentNodes.ts (1)
Query_signIn(191-218)test/routes/graphql/documentNodes.ts (1)
Query_signIn(191-218)
test/graphql/types/Chat/members.test.ts (1)
test/graphql/types/documentNodes.ts (1)
Query_signIn(191-218)
test/graphql/types/Mutation/updateChatMembership.test.ts (1)
test/graphql/types/documentNodes.ts (1)
Mutation_deleteUser(49-74)
src/graphql/types/Subscription/chatMessageUpdate.ts (2)
src/graphql/inputs/SubscriptionChatMessageUpdateInput.ts (2)
subscriptionChatMessageUpdateInputSchema(4-6)SubscriptionChatMessageUpdateInput(8-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
test/graphql/types/Mutation/createChat.test.ts (2)
test/graphql/types/documentNodes.ts (1)
Mutation_createChat(835-842)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLFormattedError(282-284)
src/graphql/enums/ChatType.ts (1)
src/drizzle/enums/chatType.ts (1)
chatTypeEnum(6-6)
src/graphql/types/Subscription/chatMessageDelete.ts (2)
src/graphql/inputs/SubscriptionChatMessageDeleteInput.ts (2)
subscriptionChatMessageDeleteInputSchema(4-6)SubscriptionChatMessageDeleteInput(8-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
src/graphql/types/Mutation/createChatMembership.ts (3)
src/graphql/types/Chat/Chat.ts (2)
Chat(25-25)Chat(27-27)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/drizzle/tables/chatMemberships.ts (1)
chatMembershipsTable(18-99)
src/graphql/types/Mutation/createChat.ts (2)
src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/drizzle/tables/chats.ts (1)
chatsTable(14-101)
src/graphql/types/Mutation/updateChatMembership.ts (1)
src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
test/graphql/types/Mutation/MarkChatAsRead.test.ts (1)
test/graphql/types/documentNodes.ts (3)
Mutation_createOrganizationMembership(469-474)Mutation_deleteChatMembership(913-919)Mutation_createChatMembership(899-911)
src/graphql/inputs/MutationCreateChatInput.ts (2)
src/drizzle/tables/chats.ts (1)
chatsTableInsertSchema(142-146)src/graphql/enums/ChatType.ts (1)
ChatType(4-8)
test/graphql/types/Mutation/createChatMembership.test.ts (1)
test/graphql/types/documentNodes.ts (9)
Query_signIn(191-218)Mutation_createUser(12-40)Mutation_deleteUser(49-74)Mutation_createOrganization(415-423)Mutation_deleteOrganization(503-510)Mutation_createOrganizationMembership(469-474)Mutation_createChat(835-842)Mutation_deleteChat(1336-1343)Mutation_createChatMembership(899-911)
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Query/UnreadChats.test.ts
[error] 187-187: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
[error] 207-207: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
[error] 337-337: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
test/graphql/types/Mutation/createChat.test.ts
[error] 693-696: Formatter would have printed the following content: extra whitespace/newline difference near end of test block (formatting issue detected by biome format:check)
src/graphql/enums/ChatType.ts
[error] 5-6: Formatter would have printed the following content: description: "Possible variants of the type of a chat." (formatting issue detected by biome format:check)
src/graphql/types/Mutation/createChat.ts
[error] 341-341: Formatter would have printed the following content: unintended formatting change detected (formatting issue detected by biome format:check)
src/graphql/inputs/MutationCreateChatInput.ts
[error] 29-33: Formatter would have printed the following content: incorrect description formatting for participants field (formatting issue detected by biome format:check)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (18)
src/drizzle/enums/chatType.ts (1)
1-6: LGTM!Clean runtime-validated enum definition using Zod. This will provide type safety across the database and GraphQL layers.
src/graphql/inputs/SubscriptionChatMessageDeleteInput.ts (1)
1-20: LGTM!Clean implementation of the subscription input with proper UUID validation and clear documentation.
drizzle_migrations/20251015160719_perpetual_wasp.sql (1)
1-2: Migration is correct, but consider unique constraint in follow-up.The migration successfully adds the required columns. However, as noted in the review of
src/drizzle/tables/chats.ts,direct_participants_hashshould have a unique constraint to prevent duplicate direct chats between the same pair of users. Consider creating a follow-up migration to add this constraint.src/graphql/types/Subscription/index.ts (1)
3-4: LGTM! New subscription imports added.The new subscription imports follow the existing pattern and properly register the chat message update and delete subscriptions.
src/graphql/types/Mutation/deleteChatMessage.ts (1)
156-156: Critical fix: Corrected PubSub topic for delete events.The delete mutation now publishes to the correct
::deletetopic instead of incorrectly publishing to::create. This ensures subscribers receive delete events on the appropriate channel.src/graphql/enums/index.ts (1)
4-4: LGTM! ChatType enum properly registered.The import follows the established pattern and maintains alphabetical ordering.
src/graphql/types/Mutation/updateChatMessage.ts (1)
161-161: Critical fix: Corrected PubSub topic for update events.The update mutation now publishes to the correct
::updatetopic instead of incorrectly publishing to::create. This ensures subscribers receive update events on the appropriate channel.test/graphql/types/gql.tada.d.ts (1)
25-25: Auto-generated introspection updated correctly.The introspection types reflect the new schema changes (Chat.type field and MutationCreateChatInput.participants field). As this is an auto-generated file, these changes are expected and correct.
Based on learnings
Also applies to: 105-105
src/graphql/types/Mutation/updateChatMembership.ts (1)
140-152: Guard correctly enforces direct chat membership immutability.The guard properly prevents membership modifications for direct (1:1) chats. The placement in the control flow is correct, and the error code is appropriate.
Consider whether the default error message for
unauthorized_action_on_arguments_associated_resourcesclearly communicates to API consumers that direct chat memberships cannot be modified. If not, you might want to provide a custom message parameter to make the intent more explicit.src/graphql/inputs/SubscriptionChatMessageUpdateInput.ts (1)
1-20: LGTM! Subscription input follows established patterns.The input type is well-structured with proper validation (UUID) and clear descriptions. It mirrors the pattern used in SubscriptionChatMessageDeleteInput.
src/graphql/types/Subscription/chatMessageUpdate.ts (1)
9-9: The import path is correct. Ignore this review comment.The file actually exists as
graphqLimits.ts, notgraphqlLimits.ts. The import statementimport envConfig from "~/src/utilities/graphqLimits"matches the actual filename and is correct. The suggested change would break the build.Likely an incorrect or invalid review comment.
test/graphql/types/Mutation/updateChatMembership.test.ts (1)
58-63: Fail fast on missing IDs; reduce boilerplate with a helper.
- Add explicit assertions for
*.user?.id(and tokens where used) right after creation; avoid passing undefined to DB filters/cleanup.- Consider a
createAndDeleteTestUser-style helper to encapsulate create/sign-in/cleanup to cut repetition and ensure cleanup parity. Based on learnings.Patch pattern:
@@ assertToBeNonNullish(creatorRes.data?.createUser); const creator = creatorRes.data.createUser; +assertToBeNonNullish(creator.user?.id); @@ assertToBeNonNullish(actorRes.data?.createUser); const actor = actorRes.data.createUser; +assertToBeNonNullish(actor.user?.id); @@ assertToBeNonNullish(targetRes.data?.createUser); const target = targetRes.data.createUser; +assertToBeNonNullish(target.user?.id);Additionally, in beforeAll (Lines 731-734), check password too:
- if (!server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS) { + if ( + !server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS || + !server.envConfig.API_ADMINISTRATOR_USER_PASSWORD + ) {Also applies to: 83-84, 104-105, 177-178, 202-202, 259-260, 281-281, 407-407, 428-428, 657-657, 780-780, 829-829, 851-851, 987-987, 1009-1009, 1140-1140
⛔ Skipped due to learnings
Learnt from: meetulr PR: PalisadoesFoundation/talawa-api#2635 File: tests/resolvers/UserTag/usersAssignedTo.spec.ts:81-124 Timestamp: 2024-11-02T04:54:41.376Z Learning: In test files such as `tests/resolvers/UserTag/usersAssignedTo.spec.ts` in the Talawa API project, the team prefers to prioritize code clarity over database query optimization. Separate database queries are acceptable for the sake of readability in tests.src/graphql/pubsub.ts (1)
33-38: Clarify or remove the unused::systemvariant keys before refactoring.The
::systemkeys (lines 34, 36, 38) are defined in the type but never published or subscribed to. Current code only emits and listens to base topics:
- Publishers:
chats.${chatId}:chat_messages::create|update|delete- Subscribers:
chats.${id}:chat_messages::create|update|deleteThe mapped type refactor is syntactically valid and would reduce duplication, but it would preserve these unused keys. Either remove the
::systemvariants if they're not needed, or clarify their purpose before refactoring.src/graphql/inputs/MutationCreateChatInput.ts (1)
5-17: Ignore this review comment—the suggested validation already exists in the resolver.The
createChatresolver (src/graphql/types/Mutation/createChat.ts) already enforces the business logic you've highlighted:
- Direct chats require exactly 2 participants and validates they are different (lines 130–155)
- Group chats allow optional participants (line 229)
- A participants hash is calculated for direct chats to ensure uniqueness
Adding the suggested
superRefinevalidation to the schema would duplicate this logic. The Biome formatting complaint is not a file issue but a configuration error in biome.jsonc (unknown keys "ignore" and "organizeImports").The review comment is based on incomplete information about the resolver's existing validation and a misconfigured Biome setup. The code as written is correct and handles the participant requirements appropriately.
Likely an incorrect or invalid review comment.
schema.graphql (3)
431-436: Enum addition looks good.
ChatTypeenum values align with usage in inputs and mutations.Ensure all resolvers and DB enums map 1:1 to these values (no casing/drift).
2012-2013: Minor doc tweak OK.No functional change; description is clearer.
4842-4856: New subscriptions read well and match topic taxonomy.Descriptions and field wiring are consistent with create/update/delete events.
Do we also need a parallel “::system” subscription variant or a flag to include system topics?
test/graphql/types/Mutation/createChat.test.ts (1)
181-189: Good: tests now pass required type.Supplying
type: "group"aligns with the new input contract.Also applies to: 211-217, 247-251, 319-323, 347-349, 371-373
| """Type of the chat: 'group' or 'direct'.""" | ||
| type: String | ||
|
|
There was a problem hiding this comment.
Use the new ChatType enum for Chat.type (and make it non-null).
Exposing Chat.type as String weakens the contract and diverges from the new ChatType enum. Switch to ChatType! for consistency and safer client typing.
Apply this schema diff:
- """Type of the chat: 'group' or 'direct'."""
- type: String
+ """Type of the chat."""
+ type: ChatType!📝 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.
| """Type of the chat: 'group' or 'direct'.""" | |
| type: String | |
| """Type of the chat.""" | |
| type: ChatType! | |
🤖 Prompt for AI Agents
In schema.graphql around lines 356 to 358, the Chat.type field is declared as a
String; change it to use the existing ChatType enum and make it non-nullable by
updating the field to ChatType! so the schema reads that Chat.type: ChatType!
ensuring stronger typing and consistency with the ChatType enum throughout the
schema.
| """Input for subscribing to chat message delete events.""" | ||
| input SubscriptionChatMessageDeleteInput { | ||
| """Global identifier of the chat to subscribe to.""" | ||
| id: ID! | ||
| } | ||
|
|
||
| """Input for subscribing to chat message update events.""" | ||
| input SubscriptionChatMessageUpdateInput { | ||
| """Global identifier of the chat to subscribe to.""" | ||
| id: ID! | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unify ID types across subscription inputs.
Here id: ID! is used, but SubscriptionChatMessageCreateInput still exposes id: String!. Align all three to ID! for consistency.
🤖 Prompt for AI Agents
In schema.graphql around lines 4864 to 4874, the subscription input types are
inconsistent: SubscriptionChatMessageDeleteInput and
SubscriptionChatMessageUpdateInput use id: ID! while
SubscriptionChatMessageCreateInput (elsewhere) uses id: String!; change the id
field on SubscriptionChatMessageCreateInput to id: ID! to match the others,
update any related schema documentation/comment if present, and run schema
validation and GraphQL codegen/tests to ensure no downstream breakage.
| /** | ||
| * Type of the chat: 'group' or 'direct'. | ||
| */ | ||
| type: text("type").notNull().default("group"), | ||
| /** | ||
| * Canonicalized participants hash for direct chats (e.g. "smallerId:largerId"). | ||
| * Used to ensure a single direct chat exists for a pair of users. | ||
| */ | ||
| directParticipantsHash: text("direct_participants_hash").$defaultFn( | ||
| () => sql`${null}`, | ||
| ), |
There was a problem hiding this comment.
Missing unique constraint on directParticipantsHash.
The directParticipantsHash field is described as being "Used to ensure a single direct chat exists for a pair of users." However, there is no unique constraint or index on this column. Without a unique constraint, multiple direct chats can be created between the same pair of users, especially under concurrent operations.
Add a unique index in the table definition:
(self) => [
index().on(self.creatorId),
index().on(self.name),
index().on(self.organizationId),
index().on(self.updaterId),
unique().on(self.directParticipantsHash),
],You'll also need a corresponding migration to add the unique constraint to existing tables.
🤖 Prompt for AI Agents
In src/drizzle/tables/chats.ts around lines 17 to 27, add a unique index on
directParticipantsHash to enforce one direct chat per participant-pair: update
the table's index list to include a unique().on(self.directParticipantsHash)
(preferably as a partial/filtered unique index limited to type='direct' and
direct_participants_hash IS NOT NULL to avoid affecting group chats), and create
a migration that adds this unique constraint to the existing table — the
migration must handle existing duplicate direct hashes (either deduplicate or
fail with a clear remediation step) and ensure the index semantics match the DB
(e.g., partial index for Postgres) so group chats with NULL hashes are
unaffected.
| directParticipantsHash: text("direct_participants_hash").$defaultFn( | ||
| () => sql`${null}`, | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify nullable default value.
The use of .$defaultFn(() => sql\${null}`)` is unnecessarily complex for a simple null default. Drizzle ORM allows nullable columns without explicit default functions.
Simplify to:
- directParticipantsHash: text("direct_participants_hash").$defaultFn(
- () => sql`${null}`,
- ),
+ directParticipantsHash: text("direct_participants_hash"),📝 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.
| directParticipantsHash: text("direct_participants_hash").$defaultFn( | |
| () => sql`${null}`, | |
| ), | |
| directParticipantsHash: text("direct_participants_hash"), |
🤖 Prompt for AI Agents
In src/drizzle/tables/chats.ts around lines 25 to 27, replace the current
default function that returns sql`${null}` with a plain nullable column
declaration; remove the .$defaultFn(() => sql`${null}`) and declare the column
as text("direct_participants_hash").nullable() so the column is nullable without
an explicit default function.
| await mercuriusClient.mutate(Mutation_deleteUser, { | ||
| headers: { authorization: `bearer ${adminToken}` }, | ||
| variables: { input: { id: user1Id } }, | ||
| variables: { input: { id: user1.user?.id } }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional chaining after non-nullish assertion is inconsistent.
Line 69 asserts that user1Res.data?.createUser is non-nullish, yet Line 74 uses optional chaining (user1.user?.id). If the assertion passes, user1.user should be non-null. Consider using non-null assertion instead: user1.user!.id.
This pattern repeats throughout the file (Lines 95, 118, 129, 137, 148, 170, 175, 180, 185, 315, 338, 365, 370, 426, 469, 490, 513, 540).
Apply this pattern to be more explicit:
- variables: { input: { id: user1.user?.id } },
+ variables: { input: { id: user1.user!.id } },📝 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.
| variables: { input: { id: user1.user?.id } }, | |
| variables: { input: { id: user1.user!.id } }, |
🤖 Prompt for AI Agents
In test/graphql/types/Mutation/MarkChatAsRead.test.ts around line 74 (and also
at lines 95, 118, 129, 137, 148, 170, 175, 180, 185, 315, 338, 365, 370, 426,
469, 490, 513, 540), the test first asserts that user1Res.data?.createUser is
non-nullish but then uses optional chaining (e.g., user1.user?.id); replace
these optional chains with non-null assertions (e.g., user1.user!.id) so the
code consistently reflects the prior non-null assertion and avoids redundant
optional chaining.
| await mercuriusClient.mutate(Mutation_deleteUser, { | ||
| headers: { authorization: `bearer ${adminToken}` }, | ||
| variables: { input: { id: memberId } }, | ||
| variables: { input: { id: member.user?.id } }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional chaining after non-nullish assertion is inconsistent.
Line 82 asserts that memberRes.data?.createUser is non-nullish, yet Line 87 uses optional chaining (member.user?.id). Consider using non-null assertion instead: member.user!.id.
This pattern repeats throughout the file.
🤖 Prompt for AI Agents
In test/graphql/types/Query/UnreadChats.test.ts around line 87 (and other places
in the file where optional chaining follows an earlier non-nullish assertion of
the same value), replace uses of optional chaining like member.user?.id with
non-null assertion member.user!.id to reflect the prior guarantee
(memberRes.data?.createUser) and keep type consistency; update all repeated
occurrences in the file and run TypeScript checks to ensure no remaining
optional-chaining-after-assertion instances remain.
| assertToBeNonNullish(unreadRes.data?.unreadChats); | ||
| const unreadList = unreadRes.data.unreadChats; | ||
| expect(unreadList.some((c: { id: string }) => c.id === chatId)).toBe(true); | ||
| expect(unreadList.some((c) => c.id === chatId)).toBe(true); |
There was a problem hiding this comment.
Fix TypeScript error: implicit any type.
The callback parameter c has an implicit any type, causing a TypeScript compilation error (confirmed by pipeline failures).
Add a type annotation to the callback parameter:
- expect(unreadList.some((c) => c.id === chatId)).toBe(true);
+ expect(unreadList.some((c: { id: string }) => c.id === chatId)).toBe(true);📝 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.
| expect(unreadList.some((c) => c.id === chatId)).toBe(true); | |
| expect(unreadList.some((c: { id: string }) => c.id === chatId)).toBe(true); |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 187-187: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
🤖 Prompt for AI Agents
In test/graphql/types/Query/UnreadChats.test.ts around line 187, the arrow
callback parameter `c` in unreadList.some(...) has an implicit any causing
TypeScript errors; add an explicit type annotation for `c` (for example use the
project's chat/unread type like `UnreadChat` or a minimal shape such as `(c: {
id: string })`) and import the proper type if needed so the callback becomes
typed (e.g., `(c: UnreadChat)` or `(c: { id: string }) => c.id === chatId`).
| expect(outsiderList.some((c: { id: string }) => c.id === chatId)).toBe( | ||
| false, | ||
| ); | ||
| expect(outsiderList.some((c) => c.id === chatId)).toBe(false); |
There was a problem hiding this comment.
Fix TypeScript error: implicit any type.
The callback parameter c has an implicit any type, causing a TypeScript compilation error (confirmed by pipeline failures).
Add a type annotation to the callback parameter:
- expect(outsiderList.some((c) => c.id === chatId)).toBe(false);
+ expect(outsiderList.some((c: { id: string }) => c.id === chatId)).toBe(false);📝 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.
| expect(outsiderList.some((c) => c.id === chatId)).toBe(false); | |
| expect(outsiderList.some((c: { id: string }) => c.id === chatId)).toBe(false); |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 207-207: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
🤖 Prompt for AI Agents
In test/graphql/types/Query/UnreadChats.test.ts around line 207, the
arrow-function callback c in outsiderList.some(...) has an implicit any type
causing a TypeScript error; add an explicit type annotation for the callback
parameter (use the actual chat element type used in tests, e.g. Chat or the
array element type via typeof outsiderList[number]) and import or reference that
type as needed so the callback becomes typed and the compilation error is
resolved.
| assertToBeNonNullish(res.data?.unreadChats); | ||
| const afterList = res.data.unreadChats; | ||
| expect(afterList.some((c: { id: string }) => c.id === chatId)).toBe(false); | ||
| expect(afterList.some((c) => c.id === chatId)).toBe(false); |
There was a problem hiding this comment.
Fix TypeScript error: implicit any type.
The callback parameter c has an implicit any type, causing a TypeScript compilation error (confirmed by pipeline failures).
Add a type annotation to the callback parameter:
- expect(afterList.some((c) => c.id === chatId)).toBe(false);
+ expect(afterList.some((c: { id: string }) => c.id === chatId)).toBe(false);📝 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.
| expect(afterList.some((c) => c.id === chatId)).toBe(false); | |
| expect(afterList.some((c: { id: string }) => c.id === chatId)).toBe(false); |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 337-337: TypeScript error TS7006: Parameter 'c' implicitly has an 'any' type.
🤖 Prompt for AI Agents
In test/graphql/types/Query/UnreadChats.test.ts around line 337, the arrow
callback passed to afterList.some has an implicit any for parameter c; update
the parameter to include an explicit type (e.g., (c: Chat) or (c: { id: string
}) depending on available types) and import or reference the correct
Chat/interface type at the top of the file so TypeScript no longer infers any.
|
i will rebase the branch later as of now we are not proceeding with changes cause this will break our flutter application |
Signed-off-by: imanishx <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graphql/types/Mutation/createChat.ts (1)
153-159: Fix possibly-uninitialized avatar fields.avatarName/avatarMimeType may be undefined at runtime; declare as possibly undefined or conditionally spread into .values.
- let avatarMimeType: z.infer<typeof imageMimeTypeEnum>; - let avatarName: string; + let avatarMimeType: z.infer<typeof imageMimeTypeEnum> | undefined; + let avatarName: string | undefined;Optionally, use conditional spread in insert values:
- .values({ - avatarMimeType, - avatarName, + .values({ + ...(avatarName && avatarMimeType + ? { avatarName, avatarMimeType } + : {}), creatorId: currentUserId, ... })Also applies to: 201-210, 291-299
♻️ Duplicate comments (4)
src/graphql/types/Mutation/createChat.ts (3)
164-179: Direct chats: require the acting user to be one of the two participants.Prevents creating chats between two third parties. This was previously requested.
if (chatType === "direct") { if (!Array.isArray(participants) || participants.length !== 2) { ... } const [a, b] = participants as [string, string]; + if (a !== currentUserId && b !== currentUserId) { + throw new TalawaGraphQLError({ + extensions: { + code: "invalid_arguments", + issues: [ + { + argumentPath: ["input", "participants"], + message: "Acting user must be one of the participants in a direct chat.", + }, + ], + }, + }); + } if (a === b) { ... }
198-213: Make direct-chat creation race-safe (onConflict + unique index).Without onConflict, concurrent requests can create duplicates. Use composite unique key and do-nothing fallback.
const [maybeCreated] = await tx .insert(chatsTable) .values({ ...(avatarName && avatarMimeType ? { avatarName, avatarMimeType } : {}), creatorId: currentUserId, description: parsedArgs.input.description, name: parsedArgs.input.name, organizationId: parsedArgs.input.organizationId, type: chatType, directParticipantsHash: participantsHash, }) + .onConflictDoNothing({ + target: [chatsTable.organizationId, chatsTable.directParticipantsHash, chatsTable.type], + }) .returning();Also add a DB unique partial index in a migration:
- Unique (organization_id, direct_participants_hash, type) WHERE type='direct'.
332-344: Group path: validate participants are members of the organization.Currently inserts memberships blindly; enforce org membership first.
- if (Array.isArray(participants) && participants.length > 0) { - await tx.insert(chatMembershipsTable)... - } + if (Array.isArray(participants) && participants.length > 0) { + const orgMembers = await tx.query.organizationMembershipsTable.findMany({ + columns: { memberId: true }, + where: (f, ops) => + ops.and( + ops.eq(f.organizationId, parsedArgs.input.organizationId), + ops.inArray(f.memberId, participants), + ), + }); + if (orgMembers.length !== participants.length) { + throw new TalawaGraphQLError({ + extensions: { + code: "invalid_arguments", + issues: [{ argumentPath: ["input","participants"], message: "All participants must be organization members." }], + }, + }); + } + await tx.insert(chatMembershipsTable)... + }test/graphql/types/Mutation/createChat.test.ts (1)
159-160: Replace empty catches and run formatter.Use a comment to satisfy linters and avoid swallowing unexpected errors silently.
- } catch (error) {} + } catch { + // ignore cleanup errors + }Also applies to: 168-169, 177-178
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/graphql/enums/ChatType.ts(1 hunks)src/graphql/inputs/MutationCreateChatInput.ts(3 hunks)src/graphql/types/Mutation/createChat.ts(2 hunks)test/graphql/types/Mutation/createChat.test.ts(11 hunks)test/graphql/types/gql.tada.d.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-10T18:57:27.311Z
Learnt from: shivasankaran18
PR: PalisadoesFoundation/talawa-api#3175
File: test/graphql/types/Mutation/updateAgendaItem.test.ts:33-49
Timestamp: 2025-02-10T18:57:27.311Z
Learning: In test/graphql/types/Mutation/updateAgendaItem.test.ts, while a global mock setup is used for the drizzle client, an alternative approach is to use specialized mocks initialized in beforeEach for each test scenario to reduce global state and ensure test isolation. However, this requires careful type handling to maintain type safety.
Applied to files:
test/graphql/types/Mutation/createChat.test.ts
📚 Learning: 2025-02-18T15:03:52.111Z
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3218
File: test/routes/graphql/Query/event.test.ts:107-140
Timestamp: 2025-02-18T15:03:52.111Z
Learning: In test/routes/graphql/Query/event.test.ts, the createAndDeleteTestUser function intentionally combines user creation and deletion in a single function to ensure proper cleanup after test execution, as every user creation needs to be cleaned up.
Applied to files:
test/graphql/types/Mutation/createChat.test.ts
📚 Learning: 2024-09-22T09:37:15.069Z
Learnt from: disha1202
PR: PalisadoesFoundation/talawa-api#2466
File: src/types/generatedGraphQLTypes.ts:3096-3101
Timestamp: 2024-09-22T09:37:15.069Z
Learning: Direct chats do not need to be associated with any organization, so the `organizationId` field in `ChatInput` remains optional.
Applied to files:
test/graphql/types/Mutation/createChat.test.ts
🧬 Code graph analysis (4)
src/graphql/enums/ChatType.ts (1)
src/drizzle/enums/chatType.ts (1)
chatTypeEnum(6-6)
test/graphql/types/Mutation/createChat.test.ts (2)
test/graphql/types/documentNodes.ts (1)
Mutation_createChat(835-842)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLFormattedError(282-284)
src/graphql/types/Mutation/createChat.ts (2)
src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/drizzle/tables/chats.ts (1)
chatsTable(14-101)
src/graphql/inputs/MutationCreateChatInput.ts (2)
src/drizzle/tables/chats.ts (1)
chatsTableInsertSchema(142-146)src/graphql/enums/ChatType.ts (1)
ChatType(4-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
src/graphql/enums/ChatType.ts (1)
4-7: LGTM — enum wiring and formatting are correct.src/graphql/inputs/MutationCreateChatInput.ts (1)
7-17: Confirm: should direct chats require organizationId?Current schema/flow require orgId for direct chats; earlier guidance indicated direct chats didn’t need an org. Intentional change? If not, we should make organizationId optional for direct and branch validation accordingly. Based on learnings.
| avatar: z.custom<Promise<FileUpload>>().nullish(), | ||
| participants: z.array(z.string()).optional(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce “participants required for direct” at schema-level.
Add a zod superRefine so validation fails early (exactly 2 distinct IDs when type='direct').
export const mutationCreateChatInputSchema = chatsTableInsertSchema
.pick({
description: true,
name: true,
organizationId: true,
type: true,
})
.extend({
avatar: z.custom<Promise<FileUpload>>().nullish(),
participants: z.array(z.string()).optional(),
- });
+ })
+ .superRefine((i, ctx) => {
+ if (i.type === "direct") {
+ if (!Array.isArray(i.participants) || i.participants.length !== 2) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ["participants"],
+ message: "Direct chats must have exactly 2 participants.",
+ });
+ } else if (i.participants[0] === i.participants[1]) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ["participants"],
+ message: "Participants must be two distinct users.",
+ });
+ }
+ }
+ });Also applies to: 30-35, 47-51
| const [p1, p2] = a < b ? [a, b] : [b, a]; | ||
| const participantsHash = `${p1}:${p2}`; | ||
|
|
||
| return await ctx.drizzleClient.transaction(async (tx) => { | ||
| const [maybeCreated] = await tx |
There was a problem hiding this comment.
Validate direct-chat participants are org members before insert.
Avoids adding non-members as chat members.
- const [p1, p2] = a < b ? [a, b] : [b, a];
+ const [p1, p2] = a < b ? [a, b] : [b, a];
+ const orgMembers = await ctx.drizzleClient.query.organizationMembershipsTable.findMany({
+ columns: { memberId: true },
+ where: (f, ops) =>
+ ops.and(
+ ops.eq(f.organizationId, parsedArgs.input.organizationId),
+ ops.inArray(f.memberId, [p1, p2]),
+ ),
+ });
+ if (orgMembers.length !== 2) {
+ throw new TalawaGraphQLError({
+ extensions: {
+ code: "invalid_arguments",
+ issues: [{ argumentPath: ["input", "participants"], message: "All participants must be organization members." }],
+ },
+ });
+ }Add import:
+import { organizationMembershipsTable } from "~/src/drizzle/tables/organizationMemberships";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/graphql/types/Mutation/createChat.ts around lines 195 to 199, before
creating the direct-chat record, validate that both participants are members of
the target organization and add the missing import for the org-members table;
query the org-members table (or use the existing isOrgMember helper) for the
provided orgId and both user IDs, throw a user-facing error if either is not a
member, and only then proceed with the tx insert; also add the import statement
for the org-members table/helper at the top of the file (e.g. import {
orgMembers } from 'src/db/schema' or import { isOrgMember } from
'src/lib/auth').
| .insert(chatMembershipsTable) | ||
| .values([ | ||
| { | ||
| chatId: maybeCreated.id, | ||
| memberId: p1, | ||
| creatorId: currentUserId, | ||
| role: "regular", | ||
| }, | ||
| { | ||
| chatId: maybeCreated.id, | ||
| memberId: p2, | ||
| creatorId: currentUserId, | ||
| role: "regular", | ||
| }, | ||
| ]) | ||
| .onConflictDoNothing(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Confirm unique constraint for chatMemberships and target it in onConflict.
Ensure a UNIQUE(chat_id, member_id) exists; if not, duplicates can still be inserted. Then target it explicitly in .onConflictDoNothing.
Also applies to: 324-344
🏁 Script executed:
#!/bin/bash
# Verify unique constraints for chat memberships and direct chats.
echo "== Locate chatMembershipsTable definition and unique constraints =="
rg -nP '(chatMembershipsTable|unique|index)\(' -C3
echo "== Check chats unique/partial index for direct participants hash =="
rg -nP 'direct_participants_hash|unique|index|partial' src/drizzle -n -C3Length of output: 121240
🏁 Script executed:
#!/bin/bash
# Get the createChat.ts file content around the two mentioned line ranges
echo "=== Lines 215-231 in createChat.ts ==="
sed -n '210,235p' src/graphql/types/Mutation/createChat.ts
echo ""
echo "=== Lines 324-344 in createChat.ts ==="
sed -n '320,350p' src/graphql/types/Mutation/createChat.ts
echo ""
echo "=== Check for onConflictDoNothing calls ==="
grep -n "onConflictDoNothing" src/graphql/types/Mutation/createChat.tsLength of output: 1588
🏁 Script executed:
#!/bin/bash
# Get more context around line 280
echo "=== Context around line 280 ==="
sed -n '270,290p' src/graphql/types/Mutation/createChat.ts
# Check if there are any explicit onConflictDoNothing with target() calls elsewhere
echo ""
echo "=== Search for onConflict patterns with explicit targets ==="
rg -n "onConflict.*target|\.target\(" src/graphql/types/Mutation/createChat.ts
# Check the full file for any conflict handling patterns
echo ""
echo "=== Check for unique constraint definitions or mentions ==="
rg -n "unique|constraint|conflict" src/drizzle/tables/chatMemberships.ts -iLength of output: 608
Add explicit ON CONFLICT target to chatMemberships inserts and ensure directParticipantsHash is uniquely constrained.
The primaryKey on (chatId, memberId) exists but onConflictDoNothing() calls at lines 230, 280, 331, and 343 must explicitly target it. Additionally, chats.directParticipantsHash (line 25 in schema) lacks a unique index to enforce single direct chat per user pair.
Fix chatMemberships inserts by adding target:
.onConflictDoNothing({ target: [chatMembershipsTable.chatId, chatMembershipsTable.memberId] })
Add to chats table indexes:
uniqueIndex("unique_direct_participants").on(self.directParticipantsHash).where(sql`${chatsTable.directParticipantsHash} IS NOT NULL`)
🤖 Prompt for AI Agents
In src/graphql/types/Mutation/createChat.ts around lines 215-231 (and similarly
update the other onConflictDoNothing calls at ~lines 230, 280, 331, 343), the
.onConflictDoNothing() calls must explicitly target the composite primary key to
avoid ambiguous conflict resolution; change them to call onConflictDoNothing
with target set to [chatMembershipsTable.chatId, chatMembershipsTable.memberId].
Also update the chats table schema (around the chats schema definition, ~line
25) to add a unique index on directParticipantsHash
(uniqueIndex("unique_direct_participants").on(self.directParticipantsHash).where(sql`${chatsTable.directParticipantsHash}
IS NOT NULL`)) so directParticipantsHash is uniquely constrained for non-null
values, ensuring only one direct chat per user pair.
| 'CancelMembershipResponse': { kind: 'OBJECT'; name: 'CancelMembershipResponse'; fields: { 'message': { name: 'message'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'success': { name: 'success'; type: { kind: 'SCALAR'; name: 'Boolean'; ofType: null; } }; }; }; | ||
| 'CategoriesByIdsInput': { kind: 'INPUT_OBJECT'; name: 'CategoriesByIdsInput'; isOneOf: false; inputFields: [{ name: 'ids'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'LIST'; name: never; ofType: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; }; }; }; defaultValue: null }]; }; | ||
| 'Chat': { kind: 'OBJECT'; name: 'Chat'; fields: { 'avatarMimeType': { name: 'avatarMimeType'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarURL': { name: 'avatarURL'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'createdAt': { name: 'createdAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'creator': { name: 'creator'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; 'description': { name: 'description'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'firstUnreadMessageId': { name: 'firstUnreadMessageId'; type: { kind: 'SCALAR'; name: 'ID'; ofType: null; } }; 'hasUnread': { name: 'hasUnread'; type: { kind: 'SCALAR'; name: 'Boolean'; ofType: null; } }; 'id': { name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; } }; 'lastMessage': { name: 'lastMessage'; type: { kind: 'OBJECT'; name: 'ChatMessage'; ofType: null; } }; 'members': { name: 'members'; type: { kind: 'OBJECT'; name: 'ChatMembersConnection'; ofType: null; } }; 'messages': { name: 'messages'; type: { kind: 'OBJECT'; name: 'ChatMessagesConnection'; ofType: null; } }; 'name': { name: 'name'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'organization': { name: 'organization'; type: { kind: 'OBJECT'; name: 'Organization'; ofType: null; } }; 'unreadMessagesCount': { name: 'unreadMessagesCount'; type: { kind: 'SCALAR'; name: 'Int'; ofType: null; } }; 'updatedAt': { name: 'updatedAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'updater': { name: 'updater'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; }; }; | ||
| 'Chat': { kind: 'OBJECT'; name: 'Chat'; fields: { 'avatarMimeType': { name: 'avatarMimeType'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarURL': { name: 'avatarURL'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'createdAt': { name: 'createdAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'creator': { name: 'creator'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; 'description': { name: 'description'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'firstUnreadMessageId': { name: 'firstUnreadMessageId'; type: { kind: 'SCALAR'; name: 'ID'; ofType: null; } }; 'hasUnread': { name: 'hasUnread'; type: { kind: 'SCALAR'; name: 'Boolean'; ofType: null; } }; 'id': { name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; } }; 'lastMessage': { name: 'lastMessage'; type: { kind: 'OBJECT'; name: 'ChatMessage'; ofType: null; } }; 'members': { name: 'members'; type: { kind: 'OBJECT'; name: 'ChatMembersConnection'; ofType: null; } }; 'messages': { name: 'messages'; type: { kind: 'OBJECT'; name: 'ChatMessagesConnection'; ofType: null; } }; 'name': { name: 'name'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'organization': { name: 'organization'; type: { kind: 'OBJECT'; name: 'Organization'; ofType: null; } }; 'type': { name: 'type'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'unreadMessagesCount': { name: 'unreadMessagesCount'; type: { kind: 'SCALAR'; name: 'Int'; ofType: null; } }; 'updatedAt': { name: 'updatedAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'updater': { name: 'updater'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; }; }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer Chat.type: ChatType over String.
Expose Chat.type as the ChatType enum to align API and client typing; update the Chat builder accordingly so introspection reflects the enum.
🤖 Prompt for AI Agents
In test/graphql/types/gql.tada.d.ts around line 25, the Chat.type field is
currently typed as a SCALAR String in the generated introspection; change it to
use the ChatType enum so the field shows kind: 'SCALAR' -> kind: 'ENUM' with
name: 'ChatType'. Update the Chat schema/builder where the field is defined to
reference the ChatType enum (not a plain string), then regenerate the TypeScript
GraphQL types so introspection and client typings reflect Chat.type as ChatType.
| 'Subscription': { kind: 'OBJECT'; name: 'Subscription'; fields: { 'chatMessageCreate': { name: 'chatMessageCreate'; type: { kind: 'OBJECT'; name: 'ChatMessage'; ofType: null; } }; 'chatMessageDelete': { name: 'chatMessageDelete'; type: { kind: 'OBJECT'; name: 'ChatMessage'; ofType: null; } }; 'chatMessageUpdate': { name: 'chatMessageUpdate'; type: { kind: 'OBJECT'; name: 'ChatMessage'; ofType: null; } }; }; }; | ||
| 'SubscriptionChatMessageCreateInput': { kind: 'INPUT_OBJECT'; name: 'SubscriptionChatMessageCreateInput'; isOneOf: false; inputFields: [{ name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'String'; ofType: null; }; }; defaultValue: null }]; }; | ||
| 'SubscriptionChatMessageDeleteInput': { kind: 'INPUT_OBJECT'; name: 'SubscriptionChatMessageDeleteInput'; isOneOf: false; inputFields: [{ name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; }; defaultValue: null }]; }; | ||
| 'SubscriptionChatMessageUpdateInput': { kind: 'INPUT_OBJECT'; name: 'SubscriptionChatMessageUpdateInput'; isOneOf: false; inputFields: [{ name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; }; defaultValue: null }]; }; |
There was a problem hiding this comment.
Normalize subscription input IDs (String vs ID).
SubscriptionChatMessageCreateInput.id is String while delete/update use ID; please standardize on ID for all three.
🤖 Prompt for AI Agents
In test/graphql/types/gql.tada.d.ts around lines 270 to 273, the
SubscriptionChatMessageCreateInput.id is typed as String while
SubscriptionChatMessageDeleteInput.id and SubscriptionChatMessageUpdateInput.id
are typed as ID; update
SubscriptionChatMessageCreateInput.inputFields[0].type.ofType.name from 'String'
to 'ID' so all three subscription input IDs consistently use the ID scalar, and
run any typegen/compile step to verify the change.
| test("creating the same direct chat twice returns the existing chat and does not duplicate memberships", async () => { | ||
| // create a fresh second user and add to organization | ||
| const secondUser = await createTestUser(adminAuthToken, "regular"); | ||
| createdUserIds.push(secondUser.userId); | ||
|
|
||
| await createOrganizationMembership( | ||
| adminAuthToken, | ||
| secondUser.userId, | ||
| organizationId, | ||
| "regular", | ||
| ); | ||
|
|
||
| const input = { | ||
| name: `Direct Idempotent ${faker.string.uuid()}`, | ||
| participants: [regularUserId, secondUser.userId], | ||
| organizationId, | ||
| type: "direct" as const, | ||
| }; | ||
|
|
||
| const first = await mercuriusClient.mutate(Mutation_createChat, { | ||
| headers: { authorization: `bearer ${regularUserAuthToken}` }, | ||
| variables: { input }, | ||
| }); | ||
|
|
||
| expect(first.errors).toBeUndefined(); | ||
| expect(first.data?.createChat).not.toBeNull(); | ||
| assertToBeNonNullish(first.data?.createChat?.id); | ||
| createdChatIds.push(first.data.createChat.id); | ||
|
|
||
| const second = await mercuriusClient.mutate(Mutation_createChat, { | ||
| headers: { authorization: `bearer ${regularUserAuthToken}` }, | ||
| variables: { input }, | ||
| }); | ||
|
|
||
| expect(second.errors).toBeUndefined(); | ||
| expect(second.data?.createChat).not.toBeNull(); | ||
| assertToBeNonNullish(second.data?.createChat?.id); | ||
|
|
||
| const memberships = | ||
| await server.drizzleClient.query.chatMembershipsTable.findMany({ | ||
| where: (f, ops) => ops.eq(f.chatId, first.data.createChat.id), | ||
| }); | ||
| const memberIds = new Set(memberships.map((m) => m.memberId)); | ||
| expect(memberIds.has(regularUserId)).toBe(true); | ||
| expect(memberIds.has(secondUser.userId)).toBe(true); | ||
| // ensure no duplicate rows per member | ||
| for (const id of memberIds) { | ||
| expect(memberships.filter((m) => m.memberId === id).length).toBe(1); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Strengthen idempotency test: assert same chat ID and no dup memberships.
Currently doesn’t prove the second call reuses the same chat.
- expect(second.errors).toBeUndefined();
- expect(second.data?.createChat).not.toBeNull();
- assertToBeNonNullish(second.data?.createChat?.id);
+ expect(second.errors).toBeUndefined();
+ expect(second.data?.createChat).not.toBeNull();
+ assertToBeNonNullish(second.data?.createChat?.id);
+ expect(second.data!.createChat!.id).toBe(first.data!.createChat!.id);🤖 Prompt for AI Agents
In test/graphql/types/Mutation/createChat.test.ts around lines 455 to 505, the
test does not assert that the second createChat call returned the same chat id
as the first, and it may push the same id twice into createdChatIds; add an
assertion that second.data?.createChat?.id === first.data?.createChat?.id (e.g.
expect(second.data?.createChat?.id).toEqual(first.data?.createChat?.id)), and
only push the chat id into createdChatIds if it isn't already present (or avoid
pushing after the second call) so the cleanup list doesn't contain duplicates.
| test("creator is administrator for created group chats", async () => { | ||
| const chatName = `CreatorAdmin ${faker.string.uuid()}`; | ||
| const result = await mercuriusClient.mutate(Mutation_createChat, { | ||
| headers: { authorization: `bearer ${regularUserAuthToken}` }, | ||
| variables: { | ||
| input: { | ||
| name: chatName, | ||
| organizationId: organizationId, | ||
| type: "group", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(result.errors).toBeUndefined(); | ||
| expect(result.data?.createChat).not.toBeNull(); | ||
| assertToBeNonNullish(result.data?.createChat?.id); | ||
| createdChatIds.push(result.data.createChat.id); | ||
|
|
||
| const membership = | ||
| await server.drizzleClient.query.chatMembershipsTable.findFirst({ | ||
| where: (f, ops) => | ||
| ops.and( | ||
| ops.eq(f.chatId, result.data.createChat.id), | ||
| ops.eq(f.memberId, regularUserId), | ||
| ), | ||
| }); | ||
|
|
||
| expect(membership).not.toBeUndefined(); | ||
| expect(membership?.role).toBe("administrator"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add missing negative tests (actor must be a participant; all participants must be org members).
Covers critical validations in the resolver.
// Append near other direct-chat tests
test("direct chat requires acting user to be one of the participants", async () => {
const outsider = await createTestUser(adminAuthToken, "regular");
createdUserIds.push(outsider.userId);
await createOrganizationMembership(adminAuthToken, outsider.userId, organizationId, "regular");
const result = await mercuriusClient.mutate(Mutation_createChat, {
headers: { authorization: `bearer ${regularUserAuthToken}` },
variables: {
input: {
name: `Bad Direct ${faker.string.uuid()}`,
participants: [outsider.userId, adminUserId], // excludes regularUserId
organizationId,
type: "direct",
},
},
});
expect(result.data?.createChat).toBeNull();
expect(result.errors?.[0]?.extensions?.code).toBe("invalid_arguments");
});
test("rejects participants who are not members of the organization", async () => {
const nonMember = await createTestUser(adminAuthToken, "regular");
createdUserIds.push(nonMember.userId);
// intentionally do NOT add org membership
const result = await mercuriusClient.mutate(Mutation_createChat, {
headers: { authorization: `bearer ${regularUserAuthToken}` },
variables: {
input: {
name: `Group With NonMember ${faker.string.uuid()}`,
organizationId,
type: "group",
participants: [nonMember.userId],
},
},
});
expect(result.data?.createChat).toBeNull();
expect(result.errors?.[0]?.extensions?.code).toBe("invalid_arguments");
});🤖 Prompt for AI Agents
test/graphql/types/Mutation/createChat.test.ts around lines 506-536: the suite
is missing two negative tests that validate important resolver checks; add one
test that verifies creating a direct chat fails if the acting user is not listed
as a participant (create a new outsider user, optionally add org membership for
them, call the Mutation_createChat with participants that exclude regularUserId,
assert createChat is null and the first error extensions.code equals
"invalid_arguments"), and add a second test that verifies creating a group chat
fails when any participant is not a member of the organization (create a
nonMember user without adding org membership, call the mutation with that user
in participants, assert createChat is null and error code is
"invalid_arguments"); ensure new users are pushed to createdUserIds for cleanup
and reuse existing helpers (createTestUser, createOrganizationMembership,
mercuriusClient.mutate) and follow the project test patterns for headers and
variables.
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
FYI: We have implemented automatic issue assignments |
2 participants
What kind of change does this PR introduce?
Issue Number:
Fixes #
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
implemented separate subscription topics for each events and implemented hashing and isGrroup filed to control direct and group chat workflow
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Bug Fixes / Behavior