-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/communities #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Copilot <[email protected]>
🚀 refactor: tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a communities feature that allows users to create and manage communities, invite members, and automatically clean up expired invitations. The changes include new database tables, API endpoints, and a scheduled cron job for maintenance.
Key changes:
- Adds communities, community members, and invitation link management with full CRUD operations
- Implements cron-based cleanup of expired invitations
- Refactors database schema organization from a single
schemafolder to structuredtablessubfolders - Removes session token refresh logic from authentication middleware
Reviewed changes
Copilot reviewed 59 out of 61 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Removed test script from root workspace |
| bun.lock | Added croner dependency for cron job scheduling |
| backend/package.json | Updated dev script to use --watch, added db:regenerate script, removed test script |
| backend/src/config/validation.config.ts | New validation configuration centralizing username, password, and community name constraints |
| backend/src/config/index.ts | Exports new validation config |
| backend/src/config/auth.config.ts | Reorganized comments for better readability |
| backend/src/db/tables/** | Restructured schema files into organized table modules with separate table definitions, schemas, and utilities |
| backend/src/db/relations.ts | Added relational definitions for communities, members, and invitations |
| backend/src/db/schema/index.ts | Removed (replaced by tables structure) |
| backend/drizzle.config.ts | Updated schema path to new tables structure |
| backend/src/modules/communities/** | New communities module with router, service, and validation schemas for create/update/delete operations |
| backend/src/modules/invitations/** | New invitations module with router, service, cron job, and validation schemas |
| backend/src/modules/auth/** | Updated to use centralized validation config and new table imports, added OpenAPI tags, fixed Google OAuth user agent handling |
| backend/src/middleware/session.middleware.ts | Removed session token refresh logic |
| backend/src/repositories/session-token.repository.ts | Removed unused refresh method |
| backend/src/lib/utils.ts | Added generateInvitationId utility function |
| backend/src/lib/factory.ts | Updated User import path |
| backend/src/lib/base-router.interface.ts | New interface defining router contract |
| backend/src/app/** | Updated to initialize new routers and cron job |
| backend/tests/** | Added integration and unit tests for communities and invitations, refactored existing auth tests |
| backend/scripts/** | New build and database regeneration scripts |
| backend/drizzle/** | New migration files for communities feature |
| .husky/pre-commit | Simplified test command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,2 +1,2 @@ | |||
| bun run test | |||
| bun test | |||
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test script has been removed from package.json at the root level but the pre-commit hook still references 'bun test'. While 'bun test' will still work (it's a built-in Bun command), this creates an inconsistency in how tests are run. Consider either keeping the test script for consistency or documenting why it was removed.
| const [community] = await this.db.query.communities.findMany({ | ||
| where: { | ||
| id: communityId, | ||
| }, | ||
| }) |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using findMany with array destructuring const [community] = await this.db.query.communities.findMany(...) when querying by unique identifier (id) is inefficient. Consider using findFirst instead, which is semantically correct for single-record lookups and more performant since it stops after finding the first match. This pattern appears multiple times in the service (lines 15, 24, 38, 62).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findFirst throws on not found drizzle-team/drizzle-orm#5189
| const _deleteExpiredInvitationsJob = new Cron('* * * * * *', { | ||
| interval: this.expiredInvitationsCleanupInterval, | ||
| protect: true, |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cron pattern '* * * * * *' (every second) combined with an interval of 1 hour creates a confusing configuration. The cron pattern suggests execution every second, but the interval option overrides this to run every hour. Consider using either a clear cron pattern like '0 * * * *' (every hour) or removing the cron pattern entirely and using only the interval option for clarity.
| const [communityExists] = await this.db.query.communities.findMany({ | ||
| where: { | ||
| name: body.name, | ||
| }, | ||
| }) | ||
|
|
||
| if (communityExists) | ||
| throw ApiException.BadRequest('Community with this name already exists', 'COMMUNITY_EXISTS') |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The community name uniqueness check is global across all users, but the test suggests this should be per-user (see test at line 59: "should not create a community with same name for same user"). However, the communities table has no unique constraint on the name field, and the service checks for any existing community with that name regardless of owner. This creates an inconsistency - either the constraint should be per-user (checking both name and ownerId) or the test description should be updated to reflect the global uniqueness requirement.
| if (membership?.role !== 'owner') | ||
| throw ApiException.Forbidden('You do not have permission to create an invitation for this community', 'FORBIDDEN') | ||
|
|
||
| const link = `https://${Bun.env.FRONTEND_URL}/invite/${generateInvitationId()}` |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL protocol is hardcoded to 'https://' but other parts of the codebase use Bun.env.FRONTEND_URL without explicitly specifying the protocol (see auth.router.ts line 291 and auth.service.ts lines 40, 170). This creates an inconsistency - if FRONTEND_URL already includes the protocol, this will create an invalid URL like 'https://https://...'. Consider either removing the 'https://' prefix here or ensuring FRONTEND_URL is consistently used without protocol across the codebase.
| const [updatedCommunity] = await this.db | ||
| .update(communities) | ||
| .set({ name: data.name }) | ||
| .where(eq(communities.id, communityId)) | ||
| .returning() | ||
|
|
||
| return updatedCommunity |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updateCommunity method also returns community data without timestamp transformation, causing the same validation/serialization issue as createCommunity.
| await this.db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(communityMembers) | ||
| .where(eq(communityMembers.communityId, communityId)) | ||
|
|
||
| await tx | ||
| .delete(communities) | ||
| .where(eq(communities.id, communityId)) | ||
| }) |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual deletion of communityMembers before deleting the community is redundant because the community_members table already has 'onDelete: cascade' defined in its foreign key constraint (see community-members.table.ts line 14). The database will automatically delete all related community members when the community is deleted. This manual deletion adds unnecessary overhead and can be removed from the transaction.
| const [community] = await this.db.query.communities.findMany({ | ||
| where: { | ||
| id: communityId, | ||
| }, | ||
| }) |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using findMany with array destructuring when querying by id is inefficient. Use findFirst instead for better performance and semantic clarity.
| const link = `https://${Bun.env.FRONTEND_URL}/invite/${generateInvitationId()}` | ||
|
|
||
| const [invitation] = await this.db | ||
| .insert(invitations) | ||
| .values({ | ||
| communityId, | ||
| expiresAt: data.expiresAt ? new Date(data.expiresAt) : null, | ||
| link, | ||
| }) |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invitation link is being stored in the database with the full URL including the invitation ID, but the invitation ID is generated randomly for each invitation rather than using the database-generated invitation.id. This means the invitation ID in the link cannot be used to look up the invitation in the database. Consider either: (1) generating the link after insertion using invitation.id, or (2) storing only the invitation token/ID separately and constructing the full URL at retrieval time, or (3) adding an indexed 'token' column to enable lookups by the random ID in the link.
Co-authored-by: Copilot <[email protected]>
|
@iiivanpopov I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.