add drizzle/graphql implementation for community #2866
Conversation
WalkthroughThis pull request introduces comprehensive changes to the Talawa API, focusing on adding community-related configuration and environment variables. The modifications span multiple files across the project, introducing a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL
participant Server
participant Database
Client->>GraphQL: updateCommunity mutation
GraphQL->>Server: Validate input
Server->>Server: Check authentication
Server->>Database: Update community details
Database-->>Server: Return updated community
Server-->>GraphQL: Return updated community data
GraphQL-->>Client: Confirm community update
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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: 37
🔭 Outside diff range comments (1)
src/plugins/index.ts (1)
Line range hint
1-13: Consider adding error handling for plugin initialization.Since this plugin is crucial for application startup and involves database operations, consider adding error handling to gracefully handle initialization failures.
export const plugins = fastifyPlugin(async (fastify) => { + try { fastify.register(drizzleClient); fastify.register(minioClient); fastify.register(seedInitialData); + } catch (error) { + fastify.log.error('Failed to initialize plugins:', error); + throw error; + } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (33)
compose.yaml(1 hunks)documentation/environment_variables.md(2 hunks)drizzle_migrations/20250118125419_luxuriant_gamma_corps.sql(2 hunks)drizzle_migrations/meta/20250118125419_snapshot.json(2 hunks)drizzle_migrations/meta/_journal.json(1 hunks)envFiles/.env.ci(1 hunks)envFiles/.env.devcontainer(1 hunks)envFiles/.env.production(1 hunks)schema.graphql(4 hunks)src/drizzle/schema.ts(1 hunks)src/drizzle/tables/communities.ts(1 hunks)src/drizzle/tables/users.ts(2 hunks)src/envConfigSchema.ts(2 hunks)src/graphql/inputs/MutationUpdateCommunityInput.ts(1 hunks)src/graphql/inputs/index.ts(1 hunks)src/graphql/types/Chat/avatarURL.ts(1 hunks)src/graphql/types/Community/Community.ts(1 hunks)src/graphql/types/Community/index.ts(1 hunks)src/graphql/types/Community/logoURL.ts(1 hunks)src/graphql/types/Community/updatedAt.ts(1 hunks)src/graphql/types/Community/updater.ts(1 hunks)src/graphql/types/Mutation/index.ts(1 hunks)src/graphql/types/Mutation/updateCommunity.ts(1 hunks)src/graphql/types/Organization/avatarURL.ts(1 hunks)src/graphql/types/Query/community.ts(1 hunks)src/graphql/types/Query/index.ts(1 hunks)src/graphql/types/User/avatarURL.ts(1 hunks)src/graphql/types/index.ts(1 hunks)src/plugins/index.ts(1 hunks)src/plugins/seedInitialData.ts(3 hunks)test/drizzle.config.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(4 hunks)test/server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Deployment to https://docs-api.talawa.io
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (30)
src/plugins/index.ts (1)
9-9: LGTM! Plugin registration order is correct.The registration order is appropriate:
drizzleClient(database)minioClient(storage)seedInitialData(data initialization)This ensures database connectivity is available before seeding begins.
src/graphql/types/Community/updater.ts (2)
20-22: Handle potential database query exceptions.Wrap the database query in a try-catch block to catch and handle any runtime errors that may occur during the query execution.
24-30:⚠️ Potential issueUpdate error code for missing user to
forbidden_action.If
currentUserisundefined, it suggests an issue with the user's existence in the database. Throwing aforbidden_actionerror might provide a clearer indication of the problem.Apply this diff:
if (currentUser === undefined) { throw new TalawaGraphQLError({ extensions: { - code: "forbidden_action", + code: "unexpected", }, }); }Likely invalid or redundant comment.
src/graphql/types/Mutation/updateCommunity.ts (1)
64-70: 🧹 Nitpick (assertive)Use
ensureAuthenticated()for authentication check.Simplify the authentication logic by using
ctx.currentClient.ensureAuthenticated()to handle unauthenticated clients.Apply this diff:
if (!ctx.currentClient.isAuthenticated) { throw new TalawaGraphQLError({ extensions: { code: "unauthenticated", }, }); } + ctx.currentClient.ensureAuthenticated();Likely invalid or redundant comment.
src/graphql/types/Organization/avatarURL.ts (1)
8-13: Same issues as previous files.This implementation has the same concerns regarding path traversal and code duplication as discussed in the previous files.
src/graphql/types/index.ts (1)
9-9: LGTM!The Community module import is correctly placed and follows the established pattern.
src/graphql/types/Query/community.ts (1)
5-28: Add authentication check for the query.The query currently lacks authentication verification. Even though this might be public data, it's good practice to explicitly define the authentication requirements in the code.
Consider adding authentication check if required:
builder.queryField("community", (t) => t.field({ description: "Query field to read the community.", resolve: async (_parent, _args, ctx) => { + // If authentication is required + if (!ctx.user) { + throw new TalawaGraphQLError({ + extensions: { + code: "UNAUTHENTICATED" + } + }); + } + const community = await ctx.drizzleClient.query.communitiesTable.findFirst({});Let me verify if authentication is required for similar queries:
test/server.ts (1)
14-17: LGTM! Well-documented configuration.The added documentation clearly explains the purpose of setting
API_PORTto undefined and includes a helpful link to the official documentation.src/graphql/inputs/index.ts (1)
52-52: LGTM!The import is correctly placed in alphabetical order.
src/drizzle/schema.ts (1)
39-42: LGTM!The exports are correctly placed and maintain consistency with the file's structure.
src/drizzle/tables/communities.ts (1)
71-77: Verify the timestamp handling across time zones.The
updatedAtfield uses timezone-aware timestamps. Ensure that the application consistently handles these timestamps across different time zones.✅ Verification successful
Timestamp handling is properly implemented ✅
The codebase demonstrates consistent and correct timezone-aware timestamp handling:
- All database timestamps are stored with timezone information in PostgreSQL
- GraphQL interface uses standardized RFC 3339/ISO 8601 formats
- No complex date manipulations that could cause timezone issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for timestamp handling consistency across the codebase rg -l "timestamp.*withTimezone:\s*true" | xargs rg "new Date\(\)"Length of output: 2504
Script:
#!/bin/bash # Check for date/time manipulation libraries and custom handling rg -l "import.*(?:moment|dayjs|date-fns)" rg "new Date\(" -A 2 # Get context for date operations rg "(?i)timezone|utc|parse.*date|format.*date"Length of output: 16531
src/plugins/seedInitialData.ts (1)
6-9: LGTM!The imports for community-related tables and schemas are properly organized.
src/drizzle/tables/users.ts (1)
344-349: LGTM!The one-to-many relationship between users and communities is properly defined through the updater_id field, following the established pattern in the codebase.
test/routes/graphql/gql.tada.d.ts (1)
31-31: LGTM!The GraphQL schema changes are comprehensive and well-structured:
- Community type properly defines all necessary fields
- Mutation and Query types correctly include community operations
- Input types for community operations are properly defined
Also applies to: 52-52, 104-104, 151-151
src/graphql/types/Community/index.ts (1)
1-4: LGTM! Well-structured type definitions.The file follows a clean modular structure by separating Community-related GraphQL type definitions into focused files.
drizzle_migrations/meta/_journal.json (1)
8-9: Verify migration timestamp validity.The migration timestamp (1737204859885) is set to year 2025, which could cause issues with migration ordering when new migrations are added in 2024. Consider regenerating the migration with current timestamp.
Run this script to check other migration timestamps:
src/graphql/types/Query/index.ts (1)
8-8: LGTM! Clean integration of community query.The community query import is properly placed and follows the established pattern of organizing imports alphabetically.
test/drizzle.config.ts (1)
1-3: LGTM! Improved documentation clarity.The updated comment provides better context about the file's purpose and its relationship to package.json scripts.
envFiles/.env.production (1)
3-4: Verify administrator account configuration.The administrator email and name are empty, which might affect the initial setup of the application. Consider:
- Documenting the impact of empty values
- Providing default values or making these required
src/graphql/types/Mutation/index.ts (1)
54-54: LGTM!The import for
updateCommunitymutation is correctly placed and follows the existing pattern.envFiles/.env.ci (4)
4-4: LGTM!The administrator username configuration aligns with the existing pattern of administrator-related environment variables.
7-8: Ensure all social media URLs use HTTPS.All social media URLs are correctly using HTTPS protocol, which is a security best practice.
Also applies to: 10-11, 13-17
12-12: LGTM!The community name configuration is appropriately set.
9-9: Verify the inactivity timeout duration.The timeout duration of 900 seconds (15 minutes) seems reasonable for session management.
Run the following script to check if this timeout value aligns with industry standards and other timeout configurations in the codebase:
✅ Verification successful
The inactivity timeout duration of 900 seconds (15 minutes) is appropriate and well-implemented.
The value is properly validated, stored in the database with appropriate constraints, and can be configured per community through the GraphQL API. The duration aligns with common session management practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other timeout configurations in the codebase # Search for timeout-related configurations rg -i "timeout|session.*duration|expire" --type ts --type jsLength of output: 5672
envFiles/.env.devcontainer (1)
4-4: LGTM!The environment variables are consistent with the CI environment configuration.
Also applies to: 7-17
compose.yaml (1)
36-36: LGTM!The environment variables are:
- Properly configured as required using
:?error- Correctly ordered alphabetically
- Consistent with the environment file definitions
Also applies to: 39-49
documentation/environment_variables.md (1)
17-20: LGTM!The documentation for the new environment variables is:
- Well-structured and clear
- Includes proper descriptions and external links
- Maintains consistency with existing documentation style
Also applies to: 29-72
drizzle_migrations/20250118125419_luxuriant_gamma_corps.sql (1)
458-458: LGTM!The foreign key constraint follows the established pattern in the schema and has appropriate actions defined.
schema.graphql (1)
2666-2668: LGTM!The query field design appropriately reflects the singleton nature of community settings.
drizzle_migrations/meta/20250118125419_snapshot.json (1)
2051-2188: LGTM!The JSON schema metadata accurately reflects the SQL table definition.
| @@ -1,12 +1,12 @@ | |||
| import fastifyPlugin from "fastify-plugin"; | |||
| import drizzleClient from "./drizzleClient"; | |||
| import minioClient from "./minioClient"; | |||
| import seedDatabase from "./seedPostgres"; | |||
| import seedInitialData from "./seedInitialData"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Update documentation and plugin name to match new implementation
The old seeding mechanism has been properly removed, but there are two minor inconsistencies in src/plugins/seedInitialData.ts:
- The JSDoc example still shows the old import statement
- The plugin is registered with the old name "seedDatabase"
While these don't affect functionality, they should be updated for consistency.
🔗 Analysis chain
Verify the complete removal of the old seeding mechanism.
The switch from seedDatabase to seedInitialData appears to be a significant refactor. Let's verify that all references to the old seeding mechanism have been properly removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old seedDatabase
rg -l "seedDatabase" --type ts
Length of output: 61
Script:
#!/bin/bash
# Check the content of seedInitialData.ts to understand the context
rg "seedDatabase" src/plugins/seedInitialData.ts -A 3 -B 3
Length of output: 426
| if (currentUser.role !== "administrator") { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthorized_action", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Re-evaluate role-based access restriction.
Access to the updatedAt field is currently restricted to administrators. Consider whether other roles, such as "moderator" or "member", should also have access to this information based on the application's requirements.
| if (!ctx.currentClient.isAuthenticated) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const currentUserId = ctx.currentClient.user.id; | ||
|
|
||
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | ||
| columns: { | ||
| role: true, | ||
| }, | ||
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | ||
| }); | ||
|
|
||
| if (currentUser === undefined) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (currentUser.role !== "administrator") { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthorized_action", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return parent.updatedAt; | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reduce duplication in error handling logic.
Multiple blocks in the code handle errors in a similar fashion. Consider creating helper functions for throwing standardized errors to improve maintainability and reduce code duplication.
| if (currentUser === undefined) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Update error code for undefined user to forbidden_action.
If currentUser is undefined, it implies that the user record does not exist despite being authenticated. Throwing an unauthenticated error might be misleading. Consider using forbidden_action to better reflect the situation.
Apply this diff to adjust the error code:
if (currentUser === undefined) {
throw new TalawaGraphQLError({
extensions: {
- code: "unauthenticated",
+ code: "forbidden_action",
},
});
}📝 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.
| if (currentUser === undefined) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "unauthenticated", | |
| }, | |
| }); | |
| } | |
| if (currentUser === undefined) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "forbidden_action", | |
| }, | |
| }); | |
| } |
| if (!ctx.currentClient.isAuthenticated) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify authentication check using ensureAuthenticated().
You can streamline the authentication check by using ctx.currentClient.ensureAuthenticated(), which throws an appropriate error if the user is not authenticated.
Apply this diff to simplify the code:
- if (!ctx.currentClient.isAuthenticated) {
- throw new TalawaGraphQLError({
- extensions: {
- code: "unauthenticated",
- },
- });
- }
+ ctx.currentClient.ensureAuthenticated();📝 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.
| if (!ctx.currentClient.isAuthenticated) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "unauthenticated", | |
| }, | |
| }); | |
| } | |
| ctx.currentClient.ensureAuthenticated(); | |
| API_COMMUNITY_FACEBOOK_URL= | ||
| API_COMMUNITY_GITHUB_URL= | ||
| API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=900 | ||
| API_COMMUNITY_INSTAGRAM_URL= | ||
| API_COMMUNITY_LINKEDIN_URL= | ||
| API_COMMUNITY_NAME= | ||
| API_COMMUNITY_REDDIT_URL= | ||
| API_COMMUNITY_SLACK_URL= | ||
| API_COMMUNITY_WEBSITE_URL= | ||
| API_COMMUNITY_X_URL= | ||
| API_COMMUNITY_YOUTUBE_URL= |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider security implications of community URLs.
The community-related URLs are exposed in environment variables. Consider:
- Adding URL format validation
- Implementing rate limiting for these endpoints
- Adding security headers when redirecting to these URLs
| API_ADMINISTRATOR_USER_PASSWORD= | ||
| API_BASE_URL= | ||
| API_COMMUNITY_FACEBOOK_URL= | ||
| API_COMMUNITY_GITHUB_URL= | ||
| API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=900 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make timeout duration configurable.
The inactivity timeout duration is hardcoded to 900 seconds (15 minutes). Consider:
- Making this value configurable based on deployment needs
- Adding validation for minimum/maximum values
| CREATE TABLE "communities" ( | ||
| "created_at" timestamp (3) with time zone DEFAULT now() NOT NULL, | ||
| "facebook_url" text, | ||
| "github_url" text, | ||
| "id" uuid PRIMARY KEY NOT NULL, | ||
| "inactivity_timeout_duration" integer, | ||
| "instagram_url" text, | ||
| "linkedin_url" text, | ||
| "logo_mime_type" text, | ||
| "logo_name" text, | ||
| "name" text NOT NULL, | ||
| "reddit_url" text, | ||
| "slack_url" text, | ||
| "updated_at" timestamp (3) with time zone, | ||
| "updater_id" uuid, | ||
| "website_url" text, | ||
| "x_url" text, | ||
| "youtube_url" text, | ||
| CONSTRAINT "communities_name_unique" UNIQUE("name") | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding indexes for frequently queried fields.
The table schema looks good overall, but consider adding indexes for frequently accessed fields like created_at and updater_id to match the pattern used in other tables.
Apply this diff to add the indexes:
CREATE TABLE "communities" (
...
);
+CREATE INDEX "communities_created_at_index" ON "communities" USING btree ("created_at");
+CREATE INDEX "communities_updater_id_index" ON "communities" USING btree ("updater_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.
| CREATE TABLE "communities" ( | |
| "created_at" timestamp (3) with time zone DEFAULT now() NOT NULL, | |
| "facebook_url" text, | |
| "github_url" text, | |
| "id" uuid PRIMARY KEY NOT NULL, | |
| "inactivity_timeout_duration" integer, | |
| "instagram_url" text, | |
| "linkedin_url" text, | |
| "logo_mime_type" text, | |
| "logo_name" text, | |
| "name" text NOT NULL, | |
| "reddit_url" text, | |
| "slack_url" text, | |
| "updated_at" timestamp (3) with time zone, | |
| "updater_id" uuid, | |
| "website_url" text, | |
| "x_url" text, | |
| "youtube_url" text, | |
| CONSTRAINT "communities_name_unique" UNIQUE("name") | |
| ); | |
| CREATE TABLE "communities" ( | |
| "created_at" timestamp (3) with time zone DEFAULT now() NOT NULL, | |
| "facebook_url" text, | |
| "github_url" text, | |
| "id" uuid PRIMARY KEY NOT NULL, | |
| "inactivity_timeout_duration" integer, | |
| "instagram_url" text, | |
| "linkedin_url" text, | |
| "logo_mime_type" text, | |
| "logo_name" text, | |
| "name" text NOT NULL, | |
| "reddit_url" text, | |
| "slack_url" text, | |
| "updated_at" timestamp (3) with time zone, | |
| "updater_id" uuid, | |
| "website_url" text, | |
| "x_url" text, | |
| "youtube_url" text, | |
| CONSTRAINT "communities_name_unique" UNIQUE("name") | |
| ); | |
| CREATE INDEX "communities_created_at_index" ON "communities" USING btree ("created_at"); | |
| CREATE INDEX "communities_updater_id_index" ON "communities" USING btree ("updater_id"); |
| """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
| """ | ||
| inactivityTimeoutDuration: Int |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance documentation for inactivityTimeoutDuration.
The documentation could be clearer about the purpose and impact of this setting.
Apply this diff to improve the documentation:
- """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.
+ """ Duration in seconds after which inactive clients will be automatically logged out from their authenticated session in client-side Talawa applications.
+ Set to null or 0 to disable the timeout feature.
"""📝 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.
| """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | |
| """ | |
| inactivityTimeoutDuration: Int | |
| """ Duration in seconds after which inactive clients will be automatically logged out from their authenticated session in client-side Talawa applications. | |
| Set to null or 0 to disable the timeout feature. | |
| """ | |
| inactivityTimeoutDuration: Int |
schema.graphql
Outdated
| """""" | ||
| input MutationUpdateCommunityInput { | ||
| """URL to the facebook account of the community.""" | ||
| facebookURL: String | ||
|
|
||
| """URL to the gitGub account of the community.""" | ||
| githubURL: String | ||
|
|
||
| """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
| """ | ||
| inactivityTimeoutDuration: Int | ||
|
|
||
| """URL to the instagram account of the community.""" | ||
| instagramURL: String | ||
|
|
||
| """URL to the linkedin account of the community.""" | ||
| linkedinURL: String | ||
|
|
||
| """Mime type of the logo of the community.""" | ||
| logo: Upload | ||
|
|
||
| """Name of the community.""" | ||
| name: String | ||
|
|
||
| """URL to the reddit account of the community.""" | ||
| redditURL: String | ||
|
|
||
| """URL to the slack account of the community.""" | ||
| slackURL: String | ||
|
|
||
| """URL to the website of the community.""" | ||
| websiteURL: String | ||
|
|
||
| """URL to the x account of the community.""" | ||
| xURL: String | ||
|
|
||
| """URL to the youtube account of the community.""" | ||
| youtubeURL: String | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add validation directives for input fields.
Consider adding validation directives to ensure data integrity.
Apply this diff to add validations:
input MutationUpdateCommunityInput {
- """URL to the facebook account of the community."""
- facebookURL: String
+ """URL to the facebook account of the community. Must be a valid Facebook URL."""
+ facebookURL: String @pattern(regexp: "^https?://(www\\.)?facebook\\.com/.*$")
- """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.
- """
- inactivityTimeoutDuration: Int
+ """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.
+ Must be a positive integer or zero.
+ """
+ inactivityTimeoutDuration: Int @range(min: 0)Committable suggestion skipped: line range outside the PR's diff.
|
@palisadoes I still think storing this data in the database is useless since it has nothing to do with business logic on the server side and has no meaningful relationships with other entities. All these fields are specific to the client applications only and can easily be configured in them using environment variables. Ideally, redeploying the client applications should be the way to update these fields and not making mutations to the postgres database. This also incurs the cost of an additional api call for each user who wants to just view the sign in or sign up pages on talawa client applications for no meaningful reason at all. Just wanted you to know. |
|
Don't merge yet. |
xoldd
force-pushed
the
add_global_community_features
branch
from
eb746d3 to
a3c63be
Compare
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
|
xoldd
force-pushed
the
add_global_community_features
branch
from
a3c63be to
dd3cda7
Compare
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
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 19
🔭 Outside diff range comments (1)
drizzle_migrations/meta/20250118150033_snapshot.json (1)
Line range hint
1-2188: Requires special approval for sensitive file changes.This change modifies a sensitive database schema file. According to the pipeline failure, this requires the 'ignore-sensitive-files-pr' label to proceed.
Please:
- Review the changes with the database administrator
- Get approval from the security team
- Add the required label after approvals
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
♻️ Duplicate comments (15)
src/graphql/types/Community/updatedAt.ts (4)
9-16: 🧹 Nitpick (assertive)Simplify authentication check using
ensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated();which throws an appropriate error if the user is not authenticated.Apply this diff:
- if (!ctx.currentClient.isAuthenticated) { - throw new TalawaGraphQLError({ - extensions: { - code: "unauthenticated", - }, - }); - } + ctx.currentClient.ensureAuthenticated();
19-25: 🧹 Nitpick (assertive)Handle potential exceptions during database query.
Wrap the database query in a try-catch block to handle any unexpected exceptions that may occur during the query operation. This ensures that errors are properly caught and handled.
Apply this diff:
- const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ - columns: { - role: true, - }, - where: (fields, operators) => operators.eq(fields.id, currentUserId), - }); + let currentUser; + try { + currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ + columns: { + role: true, + }, + where: (fields, operators) => operators.eq(fields.id, currentUserId), + }); + } catch (error) { + throw new TalawaGraphQLError({ + extensions: { + code: "database_error", + message: error.message, + }, + }); + }
26-32: 🧹 Nitpick (assertive)Update error code to
forbidden_actionwhen user is undefined.If
currentUseris undefined after authentication, it implies a forbidden action. Throwing anunauthenticatederror might be misleading. Consider usingforbidden_actionto better reflect the situation.Apply this diff:
if (currentUser === undefined) { throw new TalawaGraphQLError({ extensions: { - code: "unauthenticated", + code: "forbidden_action", }, }); }
34-40: 🧹 Nitpick (assertive)Re-evaluate role-based access restriction.
Access to the
updatedAtfield is currently restricted to administrators. Consider whether other roles, such as "moderator" or "member", should also have access to this information based on the application's requirements.src/graphql/types/Community/updater.ts (5)
10-16: 🧹 Nitpick (assertive)Simplify authentication check using
ensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated();which automatically handles unauthenticated users.Apply this diff:
- if (!ctx.currentClient.isAuthenticated) { - throw new TalawaGraphQLError({ - extensions: { - code: "unauthenticated", - }, - }); - } + ctx.currentClient.ensureAuthenticated();
20-23: 🧹 Nitpick (assertive)Handle potential exceptions during database query.
Wrap the database query in a try-catch block to handle any unexpected errors that may occur during the query operation.
Apply this diff:
- const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ - where: (fields, operators) => operators.eq(fields.id, currentUserId), - }); + let currentUser; + try { + currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ + where: (fields, operators) => operators.eq(fields.id, currentUserId), + }); + } catch (error) { + throw new TalawaGraphQLError({ + extensions: { + code: "database_error", + message: error.message, + }, + }); + }
24-30: 🧹 Nitpick (assertive)Update error code to
forbidden_actionwhen user is undefined.If
currentUseris undefined after authentication, it suggests a forbidden action. Consider usingforbidden_actioninstead ofunauthenticated.Apply this diff:
if (currentUser === undefined) { throw new TalawaGraphQLError({ extensions: { - code: "unauthenticated", + code: "forbidden_action", }, }); }
32-38: 🧹 Nitpick (assertive)Consider refining role permissions for the
updaterfield.Currently, only administrators can access the
updaterfield. Assess whether other roles, such as "moderator" or "editor", should also have access based on their responsibilities.
56-67: 🧹 Nitpick (assertive)Implement alerting for potential data integrity issues.
When
existingUserisundefined, it may indicate data corruption. In addition to logging a warning, consider integrating an alerting mechanism to notify the development team for immediate investigation.src/drizzle/tables/communities.ts (2)
37-37: 🧹 Nitpick (assertive)Add validation for
inactivityTimeoutDurationto prevent excessive values.Consider adding an upper bound to
inactivityTimeoutDurationto prevent excessively large timeout durations.Apply this diff:
inactivityTimeoutDuration: integer("inactivity_timeout_duration") + .check(sql`inactivity_timeout_duration <= 86400`), // Maximum of 24 hours
81-84: 🧹 Nitpick (assertive)Consider the implications of setting
updaterIdto null on user deletion.When a user is deleted, setting
updaterIdto null loses the audit trail of who last updated the community. Consider preserving this information or logging it before nullification.src/plugins/seedInitialData.ts (2)
94-114: 🛠️ Refactor suggestionConsider using a more reliable field for community existence check.
Currently, the code checks for an existing community using
logoMimeType, which might be optional or null. It's recommended to use a more reliable field likeidornameto ensure accurate existence checks.Apply this diff to improve the reliability of the existence check:
let existingCommunity: - | Pick<typeof communitiesTable.$inferSelect, "logoMimeType"> + | Pick<typeof communitiesTable.$inferSelect, "id"> | undefined; try { existingCommunity = await fastify.drizzleClient.query.communitiesTable.findFirst({ columns: { - logoMimeType: true, + id: true, }, });
125-146: 🛠️ Refactor suggestionAdd validation for required fields and improve error handling.
Before inserting the community data, validate required fields like
nameto prevent potential runtime errors. Including error details in the logs will aid in debugging.Apply this diff to enhance validation and error handling:
try { + const communityData = { + facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, + githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, + inactivityTimeoutDuration: + fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, + instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, + linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, + name: fastify.envConfig.API_COMMUNITY_NAME, + redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, + slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, + websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, + xURL: fastify.envConfig.API_COMMUNITY_X_URL, + youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, + }; + // Validate required fields + if (!communityData.name) { + throw new Error("Community name is required"); + } await fastify.drizzleClient.insert(communitiesTable).values( - communitiesTableInsertSchema.parse({ - facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, - githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, - inactivityTimeoutDuration: - fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, - instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, - linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, - name: fastify.envConfig.API_COMMUNITY_NAME, - redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, - slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, - websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, - xURL: fastify.envConfig.API_COMMUNITY_X_URL, - youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, - }), + communitiesTableInsertSchema.parse(communityData), ); } catch (error) { - throw new Error("Failed to create the community in the database.", { + throw new Error(`Failed to create the community in the database: ${error.message}`, { cause: error, }); }src/plugins/index.ts (1)
4-4: 🛠️ Refactor suggestionUpdate plugin export name for consistency.
The plugin
seedInitialDatais registered correctly, but its exported name remains"seedDatabase". To maintain consistency and avoid confusion, update the plugin name to"seedInitialData".Apply this change in
src/plugins/seedInitialData.ts:export default fastifyPlugin(plugin, { - name: "seedDatabase", + name: "seedInitialData", });src/graphql/types/Chat/avatarURL.ts (1)
8-13: 🛠️ Refactor suggestion
⚠️ Potential issueAdd input validation and extract duplicated URL logic.
The URL generation logic has two issues:
- Missing input validation for
avatarNameto prevent path traversal- This pattern is duplicated across Chat, User, and Organization types
Create a shared utility function with proper validation:
// src/utils/urlGenerator.ts export const generateObjectURL = ( objectName: string | null, baseURL: string ): string | null => { if (!objectName || !/^[a-zA-Z0-9._-]+$/.test(objectName)) { return null; } return new URL(`/objects/${objectName}`, baseURL).toString(); };Then use it consistently:
resolve: async (parent, _args, ctx) => - parent.avatarName === null - ? null - : new URL( - `/objects/${parent.avatarName}`, - ctx.envConfig.API_BASE_URL, - ).toString(), + generateObjectURL(parent.avatarName, ctx.envConfig.API_BASE_URL),
🛑 Comments failed to post (19)
src/graphql/inputs/QuerySignInInput.ts (1)
18-21: 🛠️ Refactor suggestion
⚠️ Potential issueEnsure email address validation is maintained.
Changing
emailAddressfrom a custom field type to a standard string might bypass email validation, potentially allowing invalid email formats. Ensure that email addresses are properly validated to maintain data integrity.Consider implementing email validation using Zod schemas or reintroducing the custom scalar for
EmailAddress. For example:import { z } from "zod"; import { usersTableInsertSchema } from "~/src/drizzle/tables/users"; import { builder } from "~/src/graphql/builder"; +const emailSchema = z + .string() + .email("Invalid email format") + .min(1) + .max(255); export const querySignInInputSchema = usersTableInsertSchema .pick({ - emailAddress: true, + emailAddress: false, }) .extend({ + emailAddress: emailSchema, password: z.string().min(1).max(64), }); export const QuerySignInInput = builder .inputRef<z.infer<typeof querySignInInputSchema>>("QuerySignInInput") .implement({ description: "", fields: (t) => ({ emailAddress: t.string({ description: "Email address of the user.", required: true, + validate: { schema: emailSchema }, }), password: t.string({ description: "Password of the user to sign in to talawa.", required: true, }), }), });Committable suggestion skipped: line range outside the PR's diff.
src/graphql/inputs/MutationUpdateCommunityInput.ts (1)
6-22: 🧹 Nitpick (assertive)
Consider client-side configuration as suggested by PR author.
Given your concerns about storing this data in the database:
- These settings could be managed via environment variables in client applications
- This would eliminate the need for additional API calls during sign-in/sign-up
- Updates could be handled through client application deployments
Consider either:
- Moving these configurations to client-side environment variables
- Or documenting why database storage is necessary (e.g., dynamic updates without deployment)
src/drizzle/schema.ts (1)
39-42: 🧹 Nitpick (assertive)
Consider alternatives to database storage.
While the export follows the correct pattern, given your concerns about storing community settings in the database:
- Consider if these settings truly need database persistence
- Evaluate if client-side configuration would be more appropriate
- Document the rationale if database storage is deemed necessary
This would help determine if these database table exports are actually needed.
src/envConfigSchema.ts (3)
37-41: 🛠️ Refactor suggestion
Add URL format validation for social media URLs.
The social media URL fields should include URL format validation to ensure valid URLs are provided. Consider adding the
format: "uri"validator, which is already configured in theenvSchemaAjv.Example for one field:
API_COMMUNITY_FACEBOOK_URL: Type.Optional( Type.String({ minLength: 1, + format: "uri", }), ),Also applies to: 61-65, 69-73, 83-87, 91-95, 99-103, 107-111, 115-119
53-57: 🧹 Nitpick (assertive)
Add maximum value constraint for inactivity timeout.
The
API_COMMUNITY_INACTIVITY_TIMEOUT_DURATIONshould have a reasonable maximum value to prevent potential issues with extremely large timeout values.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION: Type.Optional( Type.Integer({ minimum: 1, + maximum: 2147483647, // ~24.8 days in milliseconds }), ),📝 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.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION: Type.Optional( Type.Integer({ minimum: 1, maximum: 2147483647, // ~24.8 days in milliseconds }), ),
15-21: 🧹 Nitpick (assertive)
Consider adding regex pattern validation for administrator name.
The
API_ADMINISTRATOR_USER_NAMEfield could benefit from a regex pattern to ensure the name follows specific formatting rules (e.g., no special characters).API_ADMINISTRATOR_USER_NAME: Type.String({ minLength: 1, maxLength: 256, + pattern: "^[a-zA-Z0-9_ ]+$", }),📝 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./** * Email address of the user with "administrator" role that is guaranteed to exist in the postgres database at the startup time of talawa api. */ API_ADMINISTRATOR_USER_NAME: Type.String({ minLength: 1, maxLength: 256, pattern: "^[a-zA-Z0-9_ ]+$", }),src/graphql/types/Community/logoURL.ts (1)
7-13: 🛠️ Refactor suggestion
Enhance URL construction security and error handling.
The current URL construction lacks proper error handling for malformed URLs and validation for the
logoNameparameter.Consider this safer implementation:
- resolve: async (parent, _args, ctx) => - parent.logoName === null - ? null - : new URL( - `/objects/${parent.logoName}`, - ctx.envConfig.API_BASE_URL, - ).toString(), + resolve: async (parent, _args, ctx) => { + if (!parent.logoName) return null; + if (!/^[a-zA-Z0-9._-]+$/.test(parent.logoName)) { + ctx.log.error(`Invalid logo name format: ${parent.logoName}`); + return null; + } + try { + return new URL( + `/objects/${parent.logoName}`, + ctx.envConfig.API_BASE_URL + ).toString(); + } catch (error) { + ctx.log.error(`Failed to construct logo URL: ${error}`); + return null; + } + },📝 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.resolve: async (parent, _args, ctx) => { if (!parent.logoName) return null; if (!/^[a-zA-Z0-9._-]+$/.test(parent.logoName)) { ctx.log.error(`Invalid logo name format: ${parent.logoName}`); return null; } try { return new URL( `/objects/${parent.logoName}`, ctx.envConfig.API_BASE_URL ).toString(); } catch (error) { ctx.log.error(`Failed to construct logo URL: ${error}`); return null; } },src/graphql/types/Query/community.ts (2)
9-10: 🧹 Nitpick (assertive)
Consider caching the community configuration.
Given that community data is relatively static and this endpoint will be frequently accessed, consider implementing caching to reduce database load.
// Example cache implementation const CACHE_TTL = 3600; // 1 hour let communityCache: { data: typeof community; timestamp: number; } | null = null; // In resolver: if (communityCache && Date.now() - communityCache.timestamp < CACHE_TTL) { return communityCache.data; }
1-29: 🧹 Nitpick (assertive)
Reconsider database storage approach for community configuration.
Based on the PR comments, storing community configuration in the database might not be the optimal approach:
- These settings are primarily client-side configurations
- Changes require database mutations instead of simple environment updates
- Each client needs an extra API call to fetch this data
Consider alternatives:
- Environment variables for client applications
- Configuration files that can be updated during deployment
- Feature flags service for dynamic updates without database overhead
envFiles/.env.ci (1)
7-17: 🧹 Nitpick (assertive)
Use more realistic test values for social media URLs.
Current URLs point to main domains which could be misleading in tests. Consider:
- Using example.com domain (e.g.,
https://example.com/talawa)- Adding test-specific paths to differentiate between platforms
- Using localhost URLs for local testing
src/graphql/types/Community/Community.ts (1)
20-20: 🧹 Nitpick (assertive)
Fix minor issues in field descriptions.
- "gitGub" should be "GitHub"
- Remove leading space in inactivityTimeoutDuration description
- "avatar" should be "logo" in logoMimeType description to maintain consistency
Also applies to: 27-29, 36-38
envFiles/.env.devcontainer (1)
4-4: 🧹 Nitpick (assertive)
Consider security implications for production deployment.
While these default URLs are suitable for development, ensure that:
- Production deployments use proper security configurations
- Documentation clearly states these are development values
Also applies to: 7-17
src/graphql/types/Mutation/updateCommunity.ts (3)
64-70: 🧹 Nitpick (assertive)
Consider rate limiting for the mutation.
Add rate limiting to prevent abuse of the community update functionality.
15-51: 🛠️ Refactor suggestion
Consider input sanitization for URLs.
The mutation accepts multiple URLs without sanitization. Consider adding URL validation and sanitization to prevent security issues.
const mutationUpdateCommunityArgumentsSchema = z.object({ input: mutationUpdateCommunityInputSchema.transform(async (arg, ctx) => { + const sanitizeURL = (url?: string) => { + if (!url) return url; + try { + const sanitized = new URL(url); + return sanitized.toString(); + } catch { + ctx.addIssue({ + code: "custom", + message: `Invalid URL: ${url}`, + }); + return undefined; + } + }; + + const sanitizedInput = { + ...arg, + facebookURL: sanitizeURL(arg.facebookURL), + githubURL: sanitizeURL(arg.githubURL), + // Add other URLs... + }; let logo: // ... rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
146-204: 🧹 Nitpick (assertive)
Add retry mechanism for file operations.
File operations with MinIO could fail due to temporary network issues. Consider implementing a retry mechanism.
+const retry = async (operation: () => Promise<any>, attempts = 3) => { + for (let i = 0; i < attempts; i++) { + try { + return await operation(); + } catch (error) { + if (i === attempts - 1) throw error; + await new Promise(resolve => setTimeout(resolve, 1000 * Math.pow(2, i))); + } + } +}; return await ctx.drizzleClient.transaction(async (tx) => { // ... existing code ... if (isNotNullish(parsedArgs.input.logo)) { - await ctx.minio.client.putObject( + await retry(() => ctx.minio.client.putObject( ctx.minio.bucketName, logoName, parsedArgs.input.logo.createReadStream(), undefined, { "content-type": parsedArgs.input.logo.mimetype, }, - ); + )); } // ... rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
drizzle_migrations/20250118150033_needy_tigra.sql (1)
134-153: 🧹 Nitpick (assertive)
Add indexes for frequently queried fields.
Consider adding indexes for frequently queried fields like
nameandupdater_idto improve query performance.Apply this diff to add the indexes:
CREATE TABLE "communities" ( ... ); +CREATE INDEX "communities_name_index" ON "communities" USING btree ("name"); +CREATE INDEX "communities_updater_id_index" ON "communities" USING btree ("updater_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.CREATE TABLE "communities" ( "created_at" timestamp (3) with time zone DEFAULT now() NOT NULL, "facebook_url" text, "github_url" text, "id" uuid PRIMARY KEY NOT NULL, "inactivity_timeout_duration" integer, "instagram_url" text, "linkedin_url" text, "logo_mime_type" text, "logo_name" text, "name" text NOT NULL, "reddit_url" text, "slack_url" text, "updated_at" timestamp (3) with time zone, "updater_id" uuid, "website_url" text, "x_url" text, "youtube_url" text, CONSTRAINT "communities_name_unique" UNIQUE("name") ); CREATE INDEX "communities_name_index" ON "communities" USING btree ("name"); CREATE INDEX "communities_updater_id_index" ON "communities" USING btree ("updater_id");🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
drizzle_migrations/meta/20250118150033_snapshot.json (3)
2051-2188: 🧹 Nitpick (assertive)
Reconsider database storage for client-side configuration data.
Based on the author's comments and analysis of the schema, storing community configuration in the database raises several concerns:
- These settings appear to be client-specific configuration that could be managed through environment variables or client-side configuration files
- Each user accessing sign-in/sign-up pages would require an additional API call to fetch this data
- The schema includes social media URLs and logo settings that rarely change and don't have meaningful relationships with other entities
Consider alternatives like:
- Environment variables for client applications
- Client-side configuration files
- CDN-hosted JSON configuration
- Feature flags service
This would reduce:
- Database load
- API latency
- Deployment complexity
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
2080-2085: 🛠️ Refactor suggestion
Validate inactivity timeout duration constraints.
The
inactivity_timeout_durationfield allows any integer value without constraints. Consider adding:
- A minimum value to prevent timeouts that are too short
- A maximum value to prevent unreasonably long timeouts
- A check constraint to ensure the value is positive
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
2159-2160: 🛠️ Refactor suggestion
Add indexes for frequently accessed fields.
The
communitiestable lacks indexes which could impact query performance, especially for fields that will be frequently queried likename.Add the following indexes:
- "indexes": {}, + "indexes": { + "communities_name_index": { + "name": "communities_name_index", + "columns": [ + { + "expression": "name", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } + },📝 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."indexes": { "communities_name_index": { "name": "communities_name_index", "columns": [ { "expression": "name", "isExpression": false, "asc": true, "nulls": "last" } ], "isUnique": false, "concurrently": false, "method": "btree", "with": {} } }, "foreignKeys": {🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
|
@palisadoes ready for merge |
There was a problem hiding this comment.
Actionable comments posted: 24
🔭 Outside diff range comments (3)
src/graphql/inputs/MutationUpdateUserInput.ts (1)
Line range hint
63-66: Consider implementing centralized email validation.The systematic removal of the
EmailAddressscalar type across multiple mutation inputs suggests a need for a centralized email validation strategy. This could be implemented as:
- A custom scalar type (recommended)
- A shared Zod schema
- A validation utility function
This would ensure consistent email validation across all user-related mutations.
Also applies to: 71-73, 78-80
src/envConfigSchema.ts (1)
Line range hint
15-119: Consider storing community configuration in client applications.Based on the PR comments, storing these community configurations in the database may not be the optimal approach since:
- The data appears to be client-specific and lacks meaningful relationships with other entities.
- Updates would require database mutations instead of simple client redeployments.
- Each user accessing sign-in/sign-up pages would need an extra API call.
Consider moving these configurations to client-side environment variables for better efficiency.
drizzle_migrations/meta/20250118150033_snapshot.json (1)
Line range hint
343-395: Consider trade-offs between database and environment variables for community settings.Regarding the author's concerns about storing community data in the database vs. environment variables:
Pros of database storage:
- Dynamic updates without redeployment
- Audit trail through created_at/updated_at
- Role-based access control
- Transaction support with other data changes
Cons:
- Extra API call on sign-in/sign-up
- Slightly more complex implementation
Recommendation: Keep the database implementation as it provides more flexibility and features that would be difficult to achieve with environment variables.
To minimize the performance impact:
- Implement client-side caching of community settings
- Consider using a distributed cache like Redis for frequently accessed settings
- Batch load settings during initial app bootstrap
Also applies to: 2051-2188
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
♻️ Duplicate comments (5)
src/graphql/types/Community/logoURL.ts (1)
5-13: 🧹 Nitpick (assertive)Enhance type safety and validation in
logoURLresolverThe
logoURLfield resolver constructs a URL based onparent.logoNamewithout validation, which may lead to security issues iflogoNamecontains unexpected characters.As previously suggested, implement type safety and input validation for URL generation:
// src/utils/urlGenerator.ts export interface ObjectURLParams { objectName: string | null; objectType: 'avatar' | 'logo'; baseURL: string; } export const generateObjectURL = ({ objectName, objectType, baseURL, }: ObjectURLParams): string | null => { if (!objectName || !/^[\w.-]+$/.test(objectName)) { return null; } return new URL(`/objects/${objectType}/${objectName}`, baseURL).toString(); }; // Usage in logoURL.ts import { generateObjectURL } from '~/src/utils/urlGenerator'; Community.implement({ fields: (t) => ({ logoURL: t.field({ description: "URL to the logo of the community.", resolve: async (parent, _args, ctx) => generateObjectURL({ objectName: parent.logoName, objectType: 'logo', baseURL: ctx.envConfig.API_BASE_URL, }), type: "String", }), }), });This approach enhances security by validating
logoNameand provides better maintainability.src/graphql/types/User/avatarURL.ts (1)
8-13: 🛠️ Refactor suggestion
⚠️ Potential issueAddress security vulnerability and code duplication.
- The code is vulnerable to path traversal attacks through
avatarName. Add input validation.- This URL generation logic is duplicated across User, Chat, and Organization types.
Create a shared utility function with proper validation:
// src/utils/urlGenerator.ts export const generateObjectURL = ( objectName: string | null, baseURL: string ): string | null => { if (!objectName || !/^[a-zA-Z0-9._-]+$/.test(objectName)) { return null; } return new URL(`/objects/${objectName}`, baseURL).toString(); };src/graphql/types/Chat/avatarURL.ts (1)
8-13:⚠️ Potential issueSame issues as User/avatarURL.ts apply here.
This implementation has the same security vulnerability and code duplication issues as identified in User/avatarURL.ts.
src/graphql/types/Organization/avatarURL.ts (1)
8-13:⚠️ Potential issueSame issues as User/avatarURL.ts apply here.
This implementation has the same security vulnerability and code duplication issues as identified in User/avatarURL.ts.
schema.graphql (1)
356-358: 🛠️ Refactor suggestionEnhance validation and documentation for inactivityTimeoutDuration.
The field needs validation to prevent negative values and clearer documentation about timeout behavior.
Apply this diff to improve validation and documentation:
- """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. + """ Duration in seconds after which inactive clients will be automatically logged out from their authenticated session in client-side Talawa applications. + Set to null or 0 to disable the timeout feature. Must be a positive integer. """ - inactivityTimeoutDuration: Int + inactivityTimeoutDuration: Int @range(min: 0)🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
compose.yaml(1 hunks)documentation/environment_variables.md(2 hunks)drizzle_migrations/20250118150033_needy_tigra.sql(2 hunks)drizzle_migrations/meta/20250118150033_snapshot.json(2 hunks)drizzle_migrations/meta/_journal.json(1 hunks)envFiles/.env.ci(1 hunks)envFiles/.env.devcontainer(1 hunks)envFiles/.env.production(1 hunks)package.json(2 hunks)schema.graphql(10 hunks)src/drizzle/schema.ts(1 hunks)src/drizzle/tables/communities.ts(1 hunks)src/drizzle/tables/users.ts(3 hunks)src/envConfigSchema.ts(2 hunks)src/graphql/inputs/MutationCreateUserInput.ts(1 hunks)src/graphql/inputs/MutationSignUpInput.ts(1 hunks)src/graphql/inputs/MutationUpdateCommunityInput.ts(1 hunks)src/graphql/inputs/MutationUpdateCurrentUserInput.ts(1 hunks)src/graphql/inputs/MutationUpdateUserInput.ts(1 hunks)src/graphql/inputs/QuerySignInInput.ts(1 hunks)src/graphql/inputs/index.ts(1 hunks)src/graphql/scalars/EmailAddress.ts(0 hunks)src/graphql/scalars/index.ts(0 hunks)src/graphql/types/Chat/avatarURL.ts(1 hunks)src/graphql/types/Community/Community.ts(1 hunks)src/graphql/types/Community/index.ts(1 hunks)src/graphql/types/Community/logoURL.ts(1 hunks)src/graphql/types/Community/updatedAt.ts(1 hunks)src/graphql/types/Community/updater.ts(1 hunks)src/graphql/types/Mutation/index.ts(1 hunks)src/graphql/types/Mutation/updateCommunity.ts(1 hunks)src/graphql/types/Organization/avatarURL.ts(1 hunks)src/graphql/types/Query/community.ts(1 hunks)src/graphql/types/Query/index.ts(1 hunks)src/graphql/types/User/avatarURL.ts(1 hunks)src/graphql/types/User/emailAddress.ts(1 hunks)src/graphql/types/index.ts(1 hunks)src/plugins/index.ts(1 hunks)src/plugins/seedInitialData.ts(3 hunks)test/drizzle.config.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(9 hunks)test/server.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/graphql/scalars/EmailAddress.ts
- src/graphql/scalars/index.ts
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
package.json
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
schema.graphql
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
drizzle_migrations/meta/_journal.json
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
drizzle_migrations/20250118150033_needy_tigra.sql
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
drizzle_migrations/meta/20250118150033_snapshot.json
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
🔇 Additional comments (49)
src/graphql/types/Community/updatedAt.ts (4)
9-15: Simplify authentication check usingensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated(), which throws an appropriate error if the user is not authenticated.
19-25: Handle potential exceptions during database query.Wrap the database query in a try-catch block to handle any unexpected exceptions that may occur during the query operation. This ensures that errors are properly caught and handled.
26-32: Update error code for undefined user toforbidden_action.If
currentUseris undefined, it implies that the user record does not exist despite being authenticated. Throwing anunauthenticatederror might be misleading. Consider usingforbidden_actionto better reflect the situation.
34-40: Re-evaluate role-based access restriction.Access to the
updatedAtfield is currently restricted to administrators. Consider whether other roles, such as "moderator" or "member", should also have access to this information based on the application's requirements.src/graphql/types/Community/updater.ts (4)
10-16: Simplify authentication check usingensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated()to automatically handle unauthenticated users.
24-30: Update error code for undefined user toforbidden_action.If
currentUseris undefined, throwing aforbidden_actionerror may better represent the access issue, as the user is authenticated but lacks the necessary permissions.
32-38: Consider refining role permissions for theupdaterfield.Currently, only administrators can access the
updaterfield. Assess whether other roles, such as "moderator" or "editor", should also have access based on their responsibilities.
56-67: Implement alerting for potential data integrity issues.When
existingUserisundefined, it may indicate data corruption. In addition to logging a warning, consider integrating an alerting mechanism to notify the development team for immediate investigation.src/plugins/seedInitialData.ts (2)
94-114: Consider using a more reliable field for community existence check.The current implementation uses
logoMimeTypeto check if a community exists. This field might not be the most reliable indicator since it's optional and not a primary identifier.Consider using a more reliable field like
idorname:Apply this diff:
existingCommunity = await fastify.drizzleClient.query.communitiesTable.findFirst({ columns: { - logoMimeType: true, + id: true, }, });
125-146: Enhance error handling and add validation.Two suggestions for improvement:
- Add validation for required fields before insertion.
- Include error details in the log for better debugging.
Consider this enhanced implementation:
try { + const communityData = { + facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, + githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, + inactivityTimeoutDuration: + fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, + instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, + linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, + name: fastify.envConfig.API_COMMUNITY_NAME, + redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, + slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, + websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, + xURL: fastify.envConfig.API_COMMUNITY_X_URL, + youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, + }; + // Validate required fields + if (!communityData.name) { + throw new Error("Community name is required"); + } await fastify.drizzleClient.insert(communitiesTable).values( - communitiesTableInsertSchema.parse({ - facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, - githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, - inactivityTimeoutDuration: - fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, - instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, - linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, - name: fastify.envConfig.API_COMMUNITY_NAME, - redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, - slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, - websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, - xURL: fastify.envConfig.API_COMMUNITY_X_URL, - youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, - }), + communitiesTableInsertSchema.parse(communityData), ); } catch (error) { - throw new Error("Failed to create the community in the database.", { + throw new Error(`Failed to create the community in the database: ${error.message}`, { cause: error, }); }test/routes/graphql/gql.tada.d.ts (9)
31-31: 'Community' type addition is well-structuredThe new
Communitytype is correctly defined with all the necessary fields, enhancing the GraphQL schema to support community-related data.
51-51: 'updateCommunity' mutation added appropriatelyThe
updateCommunitymutation has been successfully included in theMutationtype, allowing updates to community information.
70-70: Verify consistency inMutationCreateUserInputEnsure that the changes in
MutationCreateUserInputalign with updates made to input types across the schema, especially regarding field definitions and validations.
94-94: Confirm schema alignment inMutationSignUpInputPlease verify that
MutationSignUpInputreflects the intended schema modifications, particularly for critical fields likeemailAddress.
103-103:MutationUpdateCommunityInputis correctly definedThe
MutationUpdateCommunityInputis well-implemented, enabling updates to community fields, including social media URLs and logo uploads.
115-115: Verify updates inMutationUpdateUserInputEnsure that
MutationUpdateUserInputmaintains proper validations and field definitions, especially for sensitive fields likeemailAddressandpassword.
150-150:communityfield addition toQuerytype is appropriateThe inclusion of the
communityfield in theQuerytype enables clients to fetch community data effectively.
104-104:⚠️ Potential issueEnsure email validation in
MutationUpdateCurrentUserInputChanging
emailAddressto a simple string may remove email format validation, potentially allowing invalid emails.Consider adding explicit validation to ensure only valid email addresses are accepted:
emailAddress: t.string({ description: "Email address of the user.", required: false, + validate: { email: true }, }),Alternatively, reintroduce the
EmailAddressscalar or use a validation library likezodto enforce email format.Likely invalid or redundant comment.
163-163:⚠️ Potential issuePotential loss of email validation in
QuerySignInInputSwitching
emailAddressfrom a custom scalar tot.string()may remove built-in email validation.Add explicit email validation to prevent invalid email inputs:
emailAddress: t.string({ description: "Email address of the user.", required: true, + validate: { email: true }, }),Likely invalid or redundant comment.
src/graphql/types/Community/Community.ts (2)
9-10: Fix typo in description.There's a typo in the description text: "Communitys" should be "Communities".
16-56: Consider adding URL validation for social media fields.The social media URL fields are exposed as plain strings without validation.
src/graphql/inputs/MutationUpdateCommunityInput.ts (1)
20-22: Consider adding validation messages to the schema refinement.src/drizzle/schema.ts (1)
39-42: LGTM! Export additions align with community implementation.The new exports for
communitiesTableandcommunitiesTableRelationsfollow the established pattern and are correctly placed in the schema exports file.src/graphql/inputs/MutationUpdateCurrentUserInput.ts (1)
71-73: 🛠️ Refactor suggestionConsider retaining email validation.
Changing from
EmailAddressscalar type tostringremoves built-in email format validation. This could allow invalid email addresses to be stored.Let's check if email validation exists elsewhere in the codebase:
Consider either:
- Retaining the
EmailAddressscalar type for built-in validation- Adding explicit email validation using Zod in the input schema:
emailAddress: usersTableInsertSchema.shape.emailAddress.optional(), + .refine((email) => { + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return email ? emailRegex.test(email) : true; + }, "Invalid email format"),src/graphql/inputs/MutationCreateUserInput.ts (1)
63-66: Same email validation concern applies here.Similar to
MutationUpdateCurrentUserInput, the change fromEmailAddressscalar tostringremoves email validation. This is particularly important here as this is a required field for user creation.src/graphql/inputs/MutationUpdateUserInput.ts (1)
78-80: Same email validation concern applies here.Similar to the other mutation inputs, the change from
EmailAddressscalar tostringremoves email validation.src/envConfigSchema.ts (1)
51-57: Fix incorrect documentation comment.The documentation comment for
API_COMMUNITY_INACTIVITY_TIMEOUT_DURATIONis incorrectly showing Instagram URL description.src/drizzle/tables/users.ts (2)
344-349: LGTM! Well-documented relationship definition.The one-to-many relationship between users and communities is properly defined with clear documentation.
656-656: LGTM! Enhanced email validation.The email validation has been improved using the schema's email validator.
src/graphql/types/Community/index.ts (1)
1-4: LGTM! Well-organized type exports.The Community-related GraphQL types are properly organized following the project's established patterns.
drizzle_migrations/meta/_journal.json (1)
8-9: LGTM! Valid migration metadata.The migration timestamp and tag are properly updated.
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
src/plugins/index.ts (1)
4-4: Verify documentation consistency with new implementation.The plugin has been properly updated from
seedDatabasetoseedInitialData. However, please ensure that all related documentation and JSDoc examples have been updated to reflect this change.Also applies to: 9-9
src/graphql/types/index.ts (1)
9-9: LGTM!The import follows the established pattern in the file.
src/graphql/types/Query/community.ts (1)
18-23: Use a more specific error code and message.The current error handling uses a generic "unexpected" code, which might not be the most appropriate for this scenario.
throw new TalawaGraphQLError({ extensions: { - code: "unexpected", + code: "COMMUNITY_NOT_FOUND", + message: "No community configuration found. Please contact your administrator.", }, });test/server.ts (1)
14-17: LGTM! Clear and helpful documentation.The added comment effectively explains the purpose of setting API_PORT to undefined and its benefits in preventing port conflicts during testing.
test/drizzle.config.ts (1)
1-3: LGTM! Improved documentation clarity.The updated comment provides better context about the file's purpose and its relationship with package.json scripts.
envFiles/.env.production (2)
7-17: Consider security implications of community URLs.The community-related URLs are exposed in environment variables.
Consider:
- Adding URL format validation
- Implementing rate limiting for these endpoints
- Adding security headers when redirecting to these URLs
9-9: Make timeout duration configurable.The inactivity timeout duration is hardcoded to 900 seconds (15 minutes).
Consider:
- Making this value configurable based on deployment needs
- Adding validation for minimum/maximum values
package.json (1)
41-41: Package updates require special approval.The changes to package versions appear safe (minor updates), but modifying
package.jsonrequires the 'ignore-sensitive-files-pr' label as indicated by the pipeline failure.Also applies to: 51-51
✅ Verification successful
Package versions are up-to-date but still require special approval
The updated versions (3.0.2) match the latest stable releases from npm. While the version updates are safe, the 'ignore-sensitive-files-pr' label is still required for modifying package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these are the latest stable versions curl -s https://registry.npmjs.org/@vitest/coverage-v8 | jq -r '."dist-tags".latest' curl -s https://registry.npmjs.org/vitest | jq -r '."dist-tags".latest'Length of output: 172
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
envFiles/.env.ci (1)
7-17: 🧹 Nitpick (assertive)Validate social media URLs in application code.
While the default URLs are valid, ensure that these URLs are properly validated when used in the application to prevent security issues like open redirects.
Consider implementing URL validation using a library like
validator.jsor similar.envFiles/.env.devcontainer (1)
7-17: Environment variables match .env.ci configuration.src/graphql/types/Mutation/index.ts (1)
54-54: LGTM! Import follows established pattern.The addition of the updateCommunity mutation import is consistent with the existing codebase structure.
src/graphql/inputs/index.ts (1)
52-52: LGTM! Import follows established pattern.The addition of the MutationUpdateCommunityInput import is consistent with the existing codebase structure.
src/drizzle/tables/communities.ts (1)
37-37: 🛠️ Refactor suggestionConsider adding validation for inactivity timeout duration.
The
inactivityTimeoutDurationfield should have reasonable bounds to prevent excessive values that could impact system performance.- inactivityTimeoutDuration: integer("inactivity_timeout_duration"), + inactivityTimeoutDuration: integer("inactivity_timeout_duration").check( + sql`inactivity_timeout_duration >= 60 AND inactivity_timeout_duration <= 86400` + ),Likely invalid or redundant comment.
src/graphql/types/Mutation/updateCommunity.ts (2)
106-112:⚠️ Potential issueUpdate error code for missing user.
If
currentUseris undefined after authentication, it indicates a data integrity issue rather than an authentication failure.if (currentUser === undefined) { throw new TalawaGraphQLError({ extensions: { - code: "unauthenticated", + code: "forbidden_action", + message: "User record not found despite authentication", }, }); }Likely invalid or redundant comment.
183-201:⚠️ Potential issueAdd error handling for MinIO operations.
File operations with MinIO should be wrapped in try-catch blocks to handle potential failures gracefully.
if (isNotNullish(parsedArgs.input.logo)) { + try { await ctx.minio.client.putObject( ctx.minio.bucketName, logoName, parsedArgs.input.logo.createReadStream(), undefined, { "content-type": parsedArgs.input.logo.mimetype, }, ); + } catch (error) { + throw new TalawaGraphQLError({ + extensions: { + code: "file_upload_error", + message: "Failed to upload logo file", + }, + }); + } }Likely invalid or redundant comment.
drizzle_migrations/20250118150033_needy_tigra.sql (1)
Line range hint
1-1: Sensitive file changes detected.This migration file requires the 'ignore-sensitive-files-pr' label as per the pipeline failure. Please add the required label to proceed.
✅ Verification successful
The review comment is correct - SQL migration files require special approval.
The pipeline configuration confirms that changes to sensitive files, including SQL migrations, require the 'ignore-sensitive-files-pr' label as a security measure. This is a legitimate requirement to ensure database schema changes are properly reviewed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the PR has the required label gh pr view 2866 --json labels -q '.labels[].name' | grep -q "ignore-sensitive-files-pr"Length of output: 87
Script:
#!/bin/bash # Check for pipeline configurations and documentation fd -e yml -e yaml . .github/workflows --exec cat {} \; | grep -A 5 -B 5 "sensitive-files" # Also check for any documentation about sensitive files fd "README|CONTRIBUTING" -e md . -x cat {}Length of output: 40759
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
schema.graphql (1)
1958-1996: 🛠️ Refactor suggestionAdd validation directives to input fields.
The input type should have the same validation constraints as the Community type.
Apply this diff to add validations:
input MutationUpdateCommunityInput { - """URL to the facebook account of the community.""" - facebookURL: String + """URL to the facebook account of the community. Must be a valid Facebook URL.""" + facebookURL: String @pattern(regexp: "^https?://(www\\.)?facebook\\.com/.*$") - """URL to the gitGub account of the community.""" - githubURL: String + """URL to the GitHub account of the community. Must be a valid GitHub URL.""" + githubURL: String @pattern(regexp: "^https?://(www\\.)?github\\.com/.*$") - """Duration in seconds it should take for inactive clients to get timed out.""" - inactivityTimeoutDuration: Int + """Duration in seconds after which inactive clients will be logged out.""" + inactivityTimeoutDuration: Int @range(min: 0) # Similar validations for other URLs... - """Logo of the community.""" - logo: Upload + """Logo of the community. Must be a JPEG, PNG, GIF or SVG image under 5MB.""" + logo: Upload @fileSize(max: 5242880) @mimeType(types: ["image/jpeg", "image/png", "image/gif", "image/svg+xml"]) - """Name of the community.""" - name: String + """Name of the community. Length must be between 1 and 255 characters.""" + name: String @length(min: 1, max: 255)Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
documentation/environment_variables.md (1)
17-19: 🧹 Nitpick (assertive)Documentation formatting needs alignment with existing style.
The heading for
API_ADMINISTRATOR_USER_NAMEhas an inconsistent number of blank lines after it compared to other sections.## API_ADMINISTRATOR_USER_NAME -Likely invalid or redundant comment.
| @@ -176,7 +177,7 @@ export type introspection_types = { | |||
| 'TagFolderTagsConnection': { kind: 'OBJECT'; name: 'TagFolderTagsConnection'; fields: { 'edges': { name: 'edges'; type: { kind: 'LIST'; name: never; ofType: { kind: 'OBJECT'; name: 'TagFolderTagsConnectionEdge'; ofType: null; }; } }; 'pageInfo': { name: 'pageInfo'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'OBJECT'; name: 'PageInfo'; ofType: null; }; } }; }; }; | |||
| 'TagFolderTagsConnectionEdge': { kind: 'OBJECT'; name: 'TagFolderTagsConnectionEdge'; fields: { 'cursor': { name: 'cursor'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'String'; ofType: null; }; } }; 'node': { name: 'node'; type: { kind: 'OBJECT'; name: 'Tag'; ofType: null; } }; }; }; | |||
| 'Upload': unknown; | |||
| 'User': { kind: 'OBJECT'; name: 'User'; fields: { 'addressLine1': { name: 'addressLine1'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'addressLine2': { name: 'addressLine2'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarMimeType': { name: 'avatarMimeType'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarURL': { name: 'avatarURL'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'birthDate': { name: 'birthDate'; type: { kind: 'SCALAR'; name: 'Date'; ofType: null; } }; 'city': { name: 'city'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'countryCode': { name: 'countryCode'; type: { kind: 'ENUM'; name: 'Iso3166Alpha2CountryCode'; 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; } }; 'educationGrade': { name: 'educationGrade'; type: { kind: 'ENUM'; name: 'UserEducationGrade'; ofType: null; } }; 'emailAddress': { name: 'emailAddress'; type: { kind: 'SCALAR'; name: 'EmailAddress'; ofType: null; } }; 'employmentStatus': { name: 'employmentStatus'; type: { kind: 'ENUM'; name: 'UserEmploymentStatus'; ofType: null; } }; 'homePhoneNumber': { name: 'homePhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; 'id': { name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; } }; 'isEmailAddressVerified': { name: 'isEmailAddressVerified'; type: { kind: 'SCALAR'; name: 'Boolean'; ofType: null; } }; 'maritalStatus': { name: 'maritalStatus'; type: { kind: 'ENUM'; name: 'UserMaritalStatus'; ofType: null; } }; 'mobilePhoneNumber': { name: 'mobilePhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; 'name': { name: 'name'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'natalSex': { name: 'natalSex'; type: { kind: 'ENUM'; name: 'UserNatalSex'; ofType: null; } }; 'organizationsWhereMember': { name: 'organizationsWhereMember'; type: { kind: 'OBJECT'; name: 'UserOrganizationsWhereMemberConnection'; ofType: null; } }; 'postalCode': { name: 'postalCode'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'role': { name: 'role'; type: { kind: 'ENUM'; name: 'UserRole'; ofType: null; } }; 'state': { name: 'state'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'updatedAt': { name: 'updatedAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'updater': { name: 'updater'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; 'workPhoneNumber': { name: 'workPhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; }; }; | |||
| 'User': { kind: 'OBJECT'; name: 'User'; fields: { 'addressLine1': { name: 'addressLine1'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'addressLine2': { name: 'addressLine2'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarMimeType': { name: 'avatarMimeType'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'avatarURL': { name: 'avatarURL'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'birthDate': { name: 'birthDate'; type: { kind: 'SCALAR'; name: 'Date'; ofType: null; } }; 'city': { name: 'city'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'countryCode': { name: 'countryCode'; type: { kind: 'ENUM'; name: 'Iso3166Alpha2CountryCode'; 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; } }; 'educationGrade': { name: 'educationGrade'; type: { kind: 'ENUM'; name: 'UserEducationGrade'; ofType: null; } }; 'emailAddress': { name: 'emailAddress'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'employmentStatus': { name: 'employmentStatus'; type: { kind: 'ENUM'; name: 'UserEmploymentStatus'; ofType: null; } }; 'homePhoneNumber': { name: 'homePhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; 'id': { name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'ID'; ofType: null; }; } }; 'isEmailAddressVerified': { name: 'isEmailAddressVerified'; type: { kind: 'SCALAR'; name: 'Boolean'; ofType: null; } }; 'maritalStatus': { name: 'maritalStatus'; type: { kind: 'ENUM'; name: 'UserMaritalStatus'; ofType: null; } }; 'mobilePhoneNumber': { name: 'mobilePhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; 'name': { name: 'name'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'natalSex': { name: 'natalSex'; type: { kind: 'ENUM'; name: 'UserNatalSex'; ofType: null; } }; 'organizationsWhereMember': { name: 'organizationsWhereMember'; type: { kind: 'OBJECT'; name: 'UserOrganizationsWhereMemberConnection'; ofType: null; } }; 'postalCode': { name: 'postalCode'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'role': { name: 'role'; type: { kind: 'ENUM'; name: 'UserRole'; ofType: null; } }; 'state': { name: 'state'; type: { kind: 'SCALAR'; name: 'String'; ofType: null; } }; 'updatedAt': { name: 'updatedAt'; type: { kind: 'SCALAR'; name: 'DateTime'; ofType: null; } }; 'updater': { name: 'updater'; type: { kind: 'OBJECT'; name: 'User'; ofType: null; } }; 'workPhoneNumber': { name: 'workPhoneNumber'; type: { kind: 'SCALAR'; name: 'PhoneNumber'; ofType: null; } }; }; }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure compliance with data privacy regulations in User type
When handling user data in the User type, verify that sensitive information is appropriately protected and that access controls are in place.
Consider restricting access to sensitive fields or implementing authorization checks to enhance security.
| @@ -15,10 +15,9 @@ export const QuerySignInInput = builder | |||
| .implement({ | |||
| description: "", | |||
| fields: (t) => ({ | |||
| emailAddress: t.field({ | |||
| emailAddress: t.string({ | |||
There was a problem hiding this comment.
Potential issue: Loss of email format validation
Changing emailAddress from a custom EmailAddress scalar to t.string() may remove email format validation, allowing invalid emails during sign-in.
Add explicit validation to ensure only valid email addresses are processed:
emailAddress: t.string({
description: "Email address of the user.",
required: true,
+ validate: { email: true },
}),Alternatively, consider using validation schemas like zod to enforce email formatting.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -3,7 +3,7 @@ import { User } from "./User"; | |||
|
|
|||
| User.implement({ | |||
| fields: (t) => ({ | |||
| emailAddress: t.field({ | |||
| emailAddress: t.string({ | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding email format validation.
The change from a custom EmailAddress type to string removes potential built-in validation. Consider adding email format validation using a regex pattern or a validation library.
- emailAddress: t.string({
+ emailAddress: t.string({
+ validate: {
+ email: (value) => {
+ const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+ if (!emailRegex.test(value)) {
+ throw new Error('Invalid email format');
+ }
+ return true;
+ }
+ },Committable suggestion skipped: line range outside the PR's diff.
| fields: (t) => ({ | ||
| createdAt: t.expose("createdAt", { | ||
| description: "Date time at the time the community was created.", | ||
| type: "DateTime", | ||
| }), | ||
| facebookURL: t.exposeString("facebookURL", { | ||
| description: "URL to the facebook account of the community.", | ||
| }), | ||
| githubURL: t.exposeString("githubURL", { | ||
| description: "URL to the gitGub account of the community.", | ||
| }), | ||
| id: t.exposeID("id", { | ||
| description: "Global identifier of the community.", | ||
| nullable: false, | ||
| }), | ||
| inactivityTimeoutDuration: t.exposeInt("inactivityTimeoutDuration", { | ||
| description: | ||
| " Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.", | ||
| }), | ||
| instagramURL: t.exposeString("instagramURL", { | ||
| description: "URL to the instagram account of the community.", | ||
| }), | ||
| linkedinURL: t.exposeString("linkedinURL", { | ||
| description: "URL to the linkedin account of the community.", | ||
| }), | ||
| logoMimeType: t.exposeString("logoMimeType", { | ||
| description: "Mime type of the avatar of the community.", | ||
| }), | ||
| name: t.exposeString("name", { | ||
| description: "Name of the community.", | ||
| }), | ||
| redditURL: t.exposeString("redditURL", { | ||
| description: "URL to the reddit account of the community.", | ||
| }), | ||
| slackURL: t.exposeString("slackURL", { | ||
| description: "URL to the slack account of the community.", | ||
| }), | ||
| websiteURL: t.exposeString("websiteURL", { | ||
| description: "URL to the website of the community.", | ||
| }), | ||
| xURL: t.exposeString("xURL", { | ||
| description: "URL to the x account of the community.", | ||
| }), | ||
| youtubeURL: t.exposeString("youtubeURL", { | ||
| description: "URL to the youtube account of the community.", | ||
| }), | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Specify nullability for optional fields.
Social media URLs and other optional fields should be explicitly marked as nullable to better communicate the API contract to clients.
facebookURL: t.exposeString("facebookURL", {
description: "URL to the facebook account of the community.",
+ nullable: 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.
| fields: (t) => ({ | |
| createdAt: t.expose("createdAt", { | |
| description: "Date time at the time the community was created.", | |
| type: "DateTime", | |
| }), | |
| facebookURL: t.exposeString("facebookURL", { | |
| description: "URL to the facebook account of the community.", | |
| }), | |
| githubURL: t.exposeString("githubURL", { | |
| description: "URL to the gitGub account of the community.", | |
| }), | |
| id: t.exposeID("id", { | |
| description: "Global identifier of the community.", | |
| nullable: false, | |
| }), | |
| inactivityTimeoutDuration: t.exposeInt("inactivityTimeoutDuration", { | |
| description: | |
| " Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.", | |
| }), | |
| instagramURL: t.exposeString("instagramURL", { | |
| description: "URL to the instagram account of the community.", | |
| }), | |
| linkedinURL: t.exposeString("linkedinURL", { | |
| description: "URL to the linkedin account of the community.", | |
| }), | |
| logoMimeType: t.exposeString("logoMimeType", { | |
| description: "Mime type of the avatar of the community.", | |
| }), | |
| name: t.exposeString("name", { | |
| description: "Name of the community.", | |
| }), | |
| redditURL: t.exposeString("redditURL", { | |
| description: "URL to the reddit account of the community.", | |
| }), | |
| slackURL: t.exposeString("slackURL", { | |
| description: "URL to the slack account of the community.", | |
| }), | |
| websiteURL: t.exposeString("websiteURL", { | |
| description: "URL to the website of the community.", | |
| }), | |
| xURL: t.exposeString("xURL", { | |
| description: "URL to the x account of the community.", | |
| }), | |
| youtubeURL: t.exposeString("youtubeURL", { | |
| description: "URL to the youtube account of the community.", | |
| }), | |
| }), | |
| fields: (t) => ({ | |
| createdAt: t.expose("createdAt", { | |
| description: "Date time at the time the community was created.", | |
| type: "DateTime", | |
| }), | |
| facebookURL: t.exposeString("facebookURL", { | |
| description: "URL to the facebook account of the community.", | |
| nullable: true, | |
| }), | |
| githubURL: t.exposeString("githubURL", { | |
| description: "URL to the gitGub account of the community.", | |
| nullable: true, | |
| }), | |
| id: t.exposeID("id", { | |
| description: "Global identifier of the community.", | |
| nullable: false, | |
| }), | |
| inactivityTimeoutDuration: t.exposeInt("inactivityTimeoutDuration", { | |
| description: | |
| " Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications.", | |
| }), | |
| instagramURL: t.exposeString("instagramURL", { | |
| description: "URL to the instagram account of the community.", | |
| nullable: true, | |
| }), | |
| linkedinURL: t.exposeString("linkedinURL", { | |
| description: "URL to the linkedin account of the community.", | |
| nullable: true, | |
| }), | |
| logoMimeType: t.exposeString("logoMimeType", { | |
| description: "Mime type of the avatar of the community.", | |
| }), | |
| name: t.exposeString("name", { | |
| description: "Name of the community.", | |
| }), | |
| redditURL: t.exposeString("redditURL", { | |
| description: "URL to the reddit account of the community.", | |
| nullable: true, | |
| }), | |
| slackURL: t.exposeString("slackURL", { | |
| description: "URL to the slack account of the community.", | |
| nullable: true, | |
| }), | |
| websiteURL: t.exposeString("websiteURL", { | |
| description: "URL to the website of the community.", | |
| nullable: true, | |
| }), | |
| xURL: t.exposeString("xURL", { | |
| description: "URL to the x account of the community.", | |
| nullable: true, | |
| }), | |
| youtubeURL: t.exposeString("youtubeURL", { | |
| description: "URL to the youtube account of the community.", | |
| nullable: true, | |
| }), | |
| }), |
| logo: t.field({ | ||
| description: "Mime type of the logo of the community.", | ||
| type: "Upload", | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file upload validation for logo.
The logo field accepts any file upload without size or type validation. Consider adding constraints to ensure only valid image files within acceptable size limits are uploaded.
logo: z.custom<Promise<FileUpload>>().nullish()
+ .refine(
+ async (file) => {
+ if (!file) return true;
+ const upload = await file;
+ const maxSize = 5 * 1024 * 1024; // 5MB
+ const allowedTypes = ['image/jpeg', 'image/png', 'image/gif'];
+ return upload.mimetype &&
+ allowedTypes.includes(upload.mimetype) &&
+ upload.createReadStream().bytesRead <= maxSize;
+ },
+ { message: "Logo must be a valid image file (JPEG, PNG, GIF) under 5MB" }
+ ),Committable suggestion skipped: line range outside the PR's diff.
| - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:?error} | ||
| - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:?error} | ||
| - API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=${API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION:?error} | ||
| - API_COMMUNITY_INSTAGRAM_URL=${API_COMMUNITY_INSTAGRAM_URL:?error} | ||
| - API_COMMUNITY_LINKEDIN_URL=${API_COMMUNITY_LINKEDIN_URL:?error} | ||
| - API_COMMUNITY_NAME=${API_COMMUNITY_NAME:?error} | ||
| - API_COMMUNITY_REDDIT_URL=${API_COMMUNITY_REDDIT_URL:?error} | ||
| - API_COMMUNITY_SLACK_URL=${API_COMMUNITY_SLACK_URL:?error} | ||
| - API_COMMUNITY_WEBSITE_URL=${API_COMMUNITY_WEBSITE_URL:?error} | ||
| - API_COMMUNITY_X_URL=${API_COMMUNITY_X_URL:?error} | ||
| - API_COMMUNITY_YOUTUBE_URL=${API_COMMUNITY_YOUTUBE_URL:?error} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider default values for optional URLs.
Social media URLs should be optional with empty string defaults instead of being required.
- - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:?error}
+ - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:-}
- - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:?error}
+ - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:-}📝 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.
| - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:?error} | |
| - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:?error} | |
| - API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=${API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION:?error} | |
| - API_COMMUNITY_INSTAGRAM_URL=${API_COMMUNITY_INSTAGRAM_URL:?error} | |
| - API_COMMUNITY_LINKEDIN_URL=${API_COMMUNITY_LINKEDIN_URL:?error} | |
| - API_COMMUNITY_NAME=${API_COMMUNITY_NAME:?error} | |
| - API_COMMUNITY_REDDIT_URL=${API_COMMUNITY_REDDIT_URL:?error} | |
| - API_COMMUNITY_SLACK_URL=${API_COMMUNITY_SLACK_URL:?error} | |
| - API_COMMUNITY_WEBSITE_URL=${API_COMMUNITY_WEBSITE_URL:?error} | |
| - API_COMMUNITY_X_URL=${API_COMMUNITY_X_URL:?error} | |
| - API_COMMUNITY_YOUTUBE_URL=${API_COMMUNITY_YOUTUBE_URL:?error} | |
| - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:-} | |
| - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:-} | |
| - API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=${API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION:?error} | |
| - API_COMMUNITY_INSTAGRAM_URL=${API_COMMUNITY_INSTAGRAM_URL:-} | |
| - API_COMMUNITY_LINKEDIN_URL=${API_COMMUNITY_LINKEDIN_URL:-} | |
| - API_COMMUNITY_NAME=${API_COMMUNITY_NAME:?error} | |
| - API_COMMUNITY_REDDIT_URL=${API_COMMUNITY_REDDIT_URL:-} | |
| - API_COMMUNITY_SLACK_URL=${API_COMMUNITY_SLACK_URL:-} | |
| - API_COMMUNITY_WEBSITE_URL=${API_COMMUNITY_WEBSITE_URL:-} | |
| - API_COMMUNITY_X_URL=${API_COMMUNITY_X_URL:-} | |
| - API_COMMUNITY_YOUTUBE_URL=${API_COMMUNITY_YOUTUBE_URL:-} |
| """Query field to read the community.""" | ||
| community: Community | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance query documentation and error handling.
The query field lacks documentation about error cases and potential future pagination needs.
Apply this diff to improve documentation:
- """Query field to read the community."""
+ """Query field to read the community configuration.
+ Returns null if no community is configured.
+ Note: Currently returns a single community. If multiple communities are needed in the future,
+ consider adding pagination and search capabilities.
+ """
community: Community📝 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.
| """Query field to read the community.""" | |
| community: Community | |
| """Query field to read the community configuration. | |
| Returns null if no community is configured. | |
| Note: Currently returns a single community. If multiple communities are needed in the future, | |
| consider adding pagination and search capabilities. | |
| """ | |
| community: Community | |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
| type Community { | ||
| """Date time at the time the community was created.""" | ||
| createdAt: DateTime | ||
|
|
||
| """URL to the facebook account of the community.""" | ||
| facebookURL: String | ||
|
|
||
| """URL to the gitGub account of the community.""" | ||
| githubURL: String | ||
|
|
||
| """Global identifier of the community.""" | ||
| id: ID! | ||
|
|
||
| """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
| """ | ||
| inactivityTimeoutDuration: Int | ||
|
|
||
| """URL to the instagram account of the community.""" | ||
| instagramURL: String | ||
|
|
||
| """URL to the linkedin account of the community.""" | ||
| linkedinURL: String | ||
|
|
||
| """Mime type of the avatar of the community.""" | ||
| logoMimeType: String | ||
|
|
||
| """URL to the logo of the community.""" | ||
| logoURL: String | ||
|
|
||
| """Name of the community.""" | ||
| name: String | ||
|
|
||
| """URL to the reddit account of the community.""" | ||
| redditURL: String | ||
|
|
||
| """URL to the slack account of the community.""" | ||
| slackURL: String | ||
|
|
||
| """Date time at the time the community was last updated.""" | ||
| updatedAt: DateTime | ||
|
|
||
| """User who last updated the community.""" | ||
| updater: User | ||
|
|
||
| """URL to the website of the community.""" | ||
| websiteURL: String | ||
|
|
||
| """URL to the x account of the community.""" | ||
| xURL: String | ||
|
|
||
| """URL to the youtube account of the community.""" | ||
| youtubeURL: String | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for social media URLs and text fields.
The Community type lacks validation for URLs and field constraints. Consider:
- URL format validation for social media links
- Length constraints for text fields
- MIME type restrictions for logo uploads
Apply these changes to add validations:
type Community {
"""Date time at the time the community was created."""
createdAt: DateTime
- """URL to the facebook account of the community."""
- facebookURL: String
+ """URL to the facebook account of the community. Must be a valid Facebook URL."""
+ facebookURL: String @pattern(regexp: "^https?://(www\\.)?facebook\\.com/.*$")
- """URL to the gitGub account of the community."""
- githubURL: String
+ """URL to the GitHub account of the community. Must be a valid GitHub URL."""
+ githubURL: String @pattern(regexp: "^https?://(www\\.)?github\\.com/.*$")
# Similar validations for other social media URLs...
- """Mime type of the avatar of the community."""
- logoMimeType: String
+ """Mime type of the avatar of the community. Must be a valid image MIME type."""
+ logoMimeType: String @pattern(regexp: "^image/(jpeg|png|gif|svg\\+xml)$")
- """Name of the community."""
- name: String
+ """Name of the community. Length must be between 1 and 255 characters."""
+ name: String @length(min: 1, max: 255)
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "linkedin_url": { | ||
| "name": "linkedin_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "logo_mime_type": { | ||
| "name": "logo_mime_type", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "logo_name": { | ||
| "name": "logo_name", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "name": { | ||
| "name": "name", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "reddit_url": { | ||
| "name": "reddit_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "slack_url": { | ||
| "name": "slack_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "updated_at": { | ||
| "name": "updated_at", | ||
| "type": "timestamp (3) with time zone", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "updater_id": { | ||
| "name": "updater_id", | ||
| "type": "uuid", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "website_url": { | ||
| "name": "website_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "x_url": { | ||
| "name": "x_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "youtube_url": { | ||
| "name": "youtube_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| } | ||
| }, | ||
| "indexes": {}, | ||
| "foreignKeys": { | ||
| "communities_updater_id_users_id_fk": { | ||
| "name": "communities_updater_id_users_id_fk", | ||
| "tableFrom": "communities", | ||
| "tableTo": "users", | ||
| "columnsFrom": [ | ||
| "updater_id" | ||
| ], | ||
| "columnsTo": [ | ||
| "id" | ||
| ], | ||
| "onDelete": "set null", | ||
| "onUpdate": "cascade" | ||
| } | ||
| }, | ||
| "compositePrimaryKeys": {}, | ||
| "uniqueConstraints": { | ||
| "communities_name_unique": { | ||
| "name": "communities_name_unique", | ||
| "nullsNotDistinct": false, | ||
| "columns": [ | ||
| "name" | ||
| ] | ||
| } | ||
| }, | ||
| "policies": {}, | ||
| "checkConstraints": {}, | ||
| "isRLSEnabled": false | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add indexes for frequently queried fields.
The communities table lacks indexes on fields that are likely to be frequently queried.
Add indexes for:
- created_at - for chronological sorting
- updated_at - for change tracking
- inactivity_timeout_duration - for timeout queries
Apply this diff to add indexes:
"indexes": {
+ "communities_created_at_index": {
+ "name": "communities_created_at_index",
+ "columns": [
+ {
+ "expression": "created_at",
+ "isExpression": false,
+ "asc": true,
+ "nulls": "last"
+ }
+ ],
+ "isUnique": false,
+ "concurrently": false,
+ "method": "btree",
+ "with": {}
+ },
+ "communities_updated_at_index": {
+ "name": "communities_updated_at_index",
+ "columns": [
+ {
+ "expression": "updated_at",
+ "isExpression": false,
+ "asc": true,
+ "nulls": "last"
+ }
+ ],
+ "isUnique": false,
+ "concurrently": false,
+ "method": "btree",
+ "with": {}
+ },
+ "communities_inactivity_timeout_duration_index": {
+ "name": "communities_inactivity_timeout_duration_index",
+ "columns": [
+ {
+ "expression": "inactivity_timeout_duration",
+ "isExpression": false,
+ "asc": true,
+ "nulls": "last"
+ }
+ ],
+ "isUnique": false,
+ "concurrently": false,
+ "method": "btree",
+ "with": {}
+ }
},📝 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.
| "public.communities": { | |
| "name": "communities", | |
| "schema": "", | |
| "columns": { | |
| "created_at": { | |
| "name": "created_at", | |
| "type": "timestamp (3) with time zone", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| }, | |
| "facebook_url": { | |
| "name": "facebook_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "github_url": { | |
| "name": "github_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "id": { | |
| "name": "id", | |
| "type": "uuid", | |
| "primaryKey": true, | |
| "notNull": true | |
| }, | |
| "inactivity_timeout_duration": { | |
| "name": "inactivity_timeout_duration", | |
| "type": "integer", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "instagram_url": { | |
| "name": "instagram_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "linkedin_url": { | |
| "name": "linkedin_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "logo_mime_type": { | |
| "name": "logo_mime_type", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "logo_name": { | |
| "name": "logo_name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "name": { | |
| "name": "name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "reddit_url": { | |
| "name": "reddit_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "slack_url": { | |
| "name": "slack_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "updated_at": { | |
| "name": "updated_at", | |
| "type": "timestamp (3) with time zone", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "updater_id": { | |
| "name": "updater_id", | |
| "type": "uuid", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "website_url": { | |
| "name": "website_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "x_url": { | |
| "name": "x_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "youtube_url": { | |
| "name": "youtube_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| } | |
| }, | |
| "indexes": {}, | |
| "foreignKeys": { | |
| "communities_updater_id_users_id_fk": { | |
| "name": "communities_updater_id_users_id_fk", | |
| "tableFrom": "communities", | |
| "tableTo": "users", | |
| "columnsFrom": [ | |
| "updater_id" | |
| ], | |
| "columnsTo": [ | |
| "id" | |
| ], | |
| "onDelete": "set null", | |
| "onUpdate": "cascade" | |
| } | |
| }, | |
| "compositePrimaryKeys": {}, | |
| "uniqueConstraints": { | |
| "communities_name_unique": { | |
| "name": "communities_name_unique", | |
| "nullsNotDistinct": false, | |
| "columns": [ | |
| "name" | |
| ] | |
| } | |
| }, | |
| "policies": {}, | |
| "checkConstraints": {}, | |
| "isRLSEnabled": false | |
| }, | |
| "public.communities": { | |
| "name": "communities", | |
| "schema": "", | |
| "columns": { | |
| "created_at": { | |
| "name": "created_at", | |
| "type": "timestamp (3) with time zone", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| }, | |
| "facebook_url": { | |
| "name": "facebook_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "github_url": { | |
| "name": "github_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "id": { | |
| "name": "id", | |
| "type": "uuid", | |
| "primaryKey": true, | |
| "notNull": true | |
| }, | |
| "inactivity_timeout_duration": { | |
| "name": "inactivity_timeout_duration", | |
| "type": "integer", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "instagram_url": { | |
| "name": "instagram_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "linkedin_url": { | |
| "name": "linkedin_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "logo_mime_type": { | |
| "name": "logo_mime_type", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "logo_name": { | |
| "name": "logo_name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "name": { | |
| "name": "name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "reddit_url": { | |
| "name": "reddit_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "slack_url": { | |
| "name": "slack_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "updated_at": { | |
| "name": "updated_at", | |
| "type": "timestamp (3) with time zone", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "updater_id": { | |
| "name": "updater_id", | |
| "type": "uuid", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "website_url": { | |
| "name": "website_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "x_url": { | |
| "name": "x_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "youtube_url": { | |
| "name": "youtube_url", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| } | |
| }, | |
| "indexes": { | |
| "communities_created_at_index": { | |
| "name": "communities_created_at_index", | |
| "columns": [ | |
| { | |
| "expression": "created_at", | |
| "isExpression": false, | |
| "asc": true, | |
| "nulls": "last" | |
| } | |
| ], | |
| "isUnique": false, | |
| "concurrently": false, | |
| "method": "btree", | |
| "with": {} | |
| }, | |
| "communities_updated_at_index": { | |
| "name": "communities_updated_at_index", | |
| "columns": [ | |
| { | |
| "expression": "updated_at", | |
| "isExpression": false, | |
| "asc": true, | |
| "nulls": "last" | |
| } | |
| ], | |
| "isUnique": false, | |
| "concurrently": false, | |
| "method": "btree", | |
| "with": {} | |
| }, | |
| "communities_inactivity_timeout_duration_index": { | |
| "name": "communities_inactivity_timeout_duration_index", | |
| "columns": [ | |
| { | |
| "expression": "inactivity_timeout_duration", | |
| "isExpression": false, | |
| "asc": true, | |
| "nulls": "last" | |
| } | |
| ], | |
| "isUnique": false, | |
| "concurrently": false, | |
| "method": "btree", | |
| "with": {} | |
| } | |
| }, | |
| "foreignKeys": { | |
| "communities_updater_id_users_id_fk": { | |
| "name": "communities_updater_id_users_id_fk", | |
| "tableFrom": "communities", | |
| "tableTo": "users", | |
| "columnsFrom": [ | |
| "updater_id" | |
| ], | |
| "columnsTo": [ | |
| "id" | |
| ], | |
| "onDelete": "set null", | |
| "onUpdate": "cascade" | |
| } | |
| }, | |
| "compositePrimaryKeys": {}, | |
| "uniqueConstraints": { | |
| "communities_name_unique": { | |
| "name": "communities_name_unique", | |
| "nullsNotDistinct": false, | |
| "columns": [ | |
| "name" | |
| ] | |
| } | |
| }, | |
| "policies": {}, | |
| "checkConstraints": {}, | |
| "isRLSEnabled": false | |
| }, |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.
| This environment variable is used to configure the url to the [facebook](https://facebook.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_GITHUB_URL | ||
|
|
||
| This environment variable is used to configure the url to the [github](https://github.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION | ||
|
|
||
| This environment variable is used to configure the duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
|
|
||
| ## API_COMMUNITY_INSTAGRAM_URL | ||
|
|
||
| This environment variable is used to configure the url to the [instagram](https://instagram.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_LINKEDIN_URL | ||
|
|
||
| This environment variable is used to configure the url to the [linkedin](https://linkedin.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_NAME | ||
|
|
||
| This environment variable is used to configure the name of the community. | ||
|
|
||
| ## API_COMMUNITY_REDDIT_URL | ||
|
|
||
| This environment variable is used to configure the url to the [reddit](https://reddit.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_SLACK_URL | ||
|
|
||
| This environment variable is used to configure the url to the [slack](https://slack.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_WEBSITE_URL | ||
|
|
||
| This environment variable is used to configure the url to the website of the community. | ||
|
|
||
| ## API_COMMUNITY_X_URL | ||
|
|
||
| This environment variable is used to configure the url to the [x](https://x.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_YOUTUBE_URL | ||
|
|
||
| This environment variable is used to configure the url to the [youtube](https://youtube.com) account of the community. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance documentation with validation rules.
For URL configurations, the documentation should specify:
- URL format validation rules
- Required URL protocols (http/https)
- Maximum length constraints
- Error handling when URLs are invalid
palisadoes
added
the
ignore-sensitive-files-pr
|
The typical user won't have a |
|
Is this ready? |
. |
|
there's too much message clutter because of coderabbitai leading to non bot messages being hard to notice |
xoldd
force-pushed
the
add_global_community_features
branch
from
dd3cda7 to
f7b8fb0
Compare
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
|
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (2)
drizzle_migrations/20250118150033_needy_tigra.sql (2)
Line range hint
2051-2188: Add indexes for frequently queried fields.The communities table lacks indexes on fields that will be frequently queried.
CREATE INDEX "communities_created_at_index" ON "communities" USING btree ("created_at"); CREATE INDEX "communities_updated_at_index" ON "communities" USING btree ("updated_at"); CREATE INDEX "communities_inactivity_timeout_duration_index" ON "communities" USING btree ("inactivity_timeout_duration");
Line range hint
2080-2085: Add check constraint for timeout duration.Add a check constraint to ensure the timeout duration is non-negative.
ALTER TABLE "communities" ADD CONSTRAINT "communities_inactivity_timeout_duration_check" CHECK (inactivity_timeout_duration >= 0);
♻️ Duplicate comments (31)
src/graphql/inputs/QuerySignInInput.ts (1)
18-20:⚠️ Potential issueAdd email format validation.
The change from custom
EmailAddressscalar tot.string()removes any built-in email validation. This could allow invalid email formats during sign-in.Add validation to ensure only valid email addresses are processed:
emailAddress: t.string({ description: "Email address of the user.", required: true, + validate: { email: true }, }),src/graphql/types/User/emailAddress.ts (1)
6-6: 🛠️ Refactor suggestionAdd email format validation to maintain data integrity.
The change from
EmailAddressscalar tostringremoves built-in format validation. While the authorization logic is robust, we should ensure data integrity through format validation.Consider adding validation as suggested in the previous review:
emailAddress: t.string({ description: "Email address of the user.", + validate: { + regex: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, + message: "Invalid email format" + }, resolve: async (parent, _args, ctx) => {src/graphql/types/Community/updatedAt.ts (4)
9-15: 🧹 Nitpick (assertive)Simplify authentication check using
ensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated()to automatically handle unauthenticated users.Apply this diff:
- if (!ctx.currentClient.isAuthenticated) { - throw new TalawaGraphQLError({ - extensions: { - code: "unauthenticated", - }, - }); - } + ctx.currentClient.ensureAuthenticated();
19-25: 🧹 Nitpick (assertive)Handle potential exceptions during database query.
Wrap the database query in a try-catch block to handle any unexpected exceptions that may occur during the query operation. This ensures that errors are properly caught and handled.
Apply this diff:
- const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ - columns: { - role: true, - }, - where: (fields, operators) => operators.eq(fields.id, currentUserId), - }); + let currentUser; + try { + currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ + columns: { + role: true, + }, + where: (fields, operators) => operators.eq(fields.id, currentUserId), + }); + } catch (error) { + throw new TalawaGraphQLError({ + extensions: { + code: "database_error", + message: error.message, + }, + }); + }
26-32: 🧹 Nitpick (assertive)Update error code for undefined user to
forbidden_action.If
currentUseris undefined, it implies that the user record does not exist despite being authenticated. Throwing anunauthenticatederror might be misleading. Consider usingforbidden_actionto better reflect the situation.Apply this diff:
if (currentUser === undefined) { throw new TalawaGraphQLError({ extensions: { - code: "unauthenticated", + code: "forbidden_action", }, }); }
9-43: 🧹 Nitpick (assertive)Reduce duplication in error handling logic.
Multiple blocks in the code handle errors in a similar fashion. Consider creating helper functions for throwing standardized errors to improve maintainability and reduce code duplication.
src/graphql/types/Community/updater.ts (2)
10-16: 🧹 Nitpick (assertive)Simplify authentication check using
ensureAuthenticated().You can streamline the authentication check by using
ctx.currentClient.ensureAuthenticated()to automatically handle unauthenticated users.Apply this diff:
- if (!ctx.currentClient.isAuthenticated) { - throw new TalawaGraphQLError({ - extensions: { - code: "unauthenticated", - }, - }); - } + ctx.currentClient.ensureAuthenticated();
32-38: 🧹 Nitpick (assertive)Consider refining role permissions for the
updaterfield.Currently, only administrators can access the
updaterfield. Assess whether other roles, such as "moderator" or "editor", should also have access based on their responsibilities.src/graphql/inputs/MutationUpdateCommunityInput.ts (4)
35-35: 🧹 Nitpick (assertive)Fix typo in
githubURLfield description.The description has a typo: "gitGub" should be "GitHub".
Apply this diff to correct the typo:
description: "URL to the gitGub account of the community.", + description: "URL to the GitHub account of the community.",
48-49: 🧹 Nitpick (assertive)Update incorrect description for the
logofield.The description for the
logofield incorrectly states it's for the mime type, but this field is for the actual logo file upload.Apply this diff to update the description:
description: "Mime type of the logo of the community.", + description: "Logo file for the community.",
20-22: 🧹 Nitpick (assertive)Enhance validation message in schema refinement.
Consider providing a more specific error message indicating which fields are optional and encouraging users to provide at least one.
Apply this diff:
.refine((arg) => Object.values(arg).some((value) => value !== undefined), { - message: "At least one optional argument must be provided.", + message: "At least one field must be provided. Available fields: name, logo, social media URLs, or inactivity timeout duration.", });
17-17: 🧹 Nitpick (assertive)Add file upload validation for
logo.The
logofield accepts any file upload without size or type validation. Consider adding constraints to ensure only valid image files within acceptable size limits are uploaded.Apply this diff to add validation:
logo: z.custom<Promise<FileUpload>>().nullish() + .refine( + async (file) => { + if (!file) return true; + const upload = await file; + const allowedTypes = ['image/jpeg', 'image/png', 'image/gif']; + if (!allowedTypes.includes(upload.mimetype)) { + return false; + } + const maxSize = 5 * 1024 * 1024; // 5MB + let totalSize = 0; + const stream = upload.createReadStream(); + for await (const chunk of stream) { + totalSize += chunk.length; + if (totalSize > maxSize) { + return false; + } + } + return true; + }, + { message: "Logo must be a valid image file (JPEG, PNG, GIF) under 5MB" } + ),src/drizzle/tables/communities.ts (4)
37-37: 🛠️ Refactor suggestionAdd validation for
inactivityTimeoutDurationto prevent excessive values.Currently,
inactivityTimeoutDurationhas a minimum value set in the insert schema but lacks an upper bound. Consider adding an upper limit to prevent excessively large timeout durations that could lead to security vulnerabilities.Apply this diff to add a maximum value check:
-inactivityTimeoutDuration: integer("inactivity_timeout_duration"), +inactivityTimeoutDuration: integer("inactivity_timeout_duration").check( + sql`inactivity_timeout_duration <= 86400` // Maximum of 24 hours +),
49-51:⚠️ Potential issueAdd a check constraint for
logoMimeTypeto enforce allowed MIME types.The
logoMimeTypefield specifies an enum but lacks a database-level constraint to enforce the allowed values. Adding a check constraint ensures only valid MIME types are stored.Apply this diff to add the check constraint:
-logoMimeType: text("logo_mime_type", { - enum: imageMimeTypeEnum.options, -}), +logoMimeType: text("logo_mime_type", { + enum: imageMimeTypeEnum.options, +}).check( + sql`logo_mime_type = ANY(ARRAY['image/jpeg', 'image/png', 'image/gif'])` +),
81-84:⚠️ Potential issueReconsider setting
updaterIdto null on user deletion to maintain audit trails.Setting
updaterIdto null when a user is deleted results in loss of audit information. Consider retaining theupdaterIdeven if the user is deleted, or implement a soft delete mechanism for users to preserve historical data.
88-96: 🧹 Nitpick (assertive)Add length validation for URL fields to prevent abuse.
The URL fields do not have a maximum length specified. To prevent potential abuse or database issues, consider adding a maximum length constraint.
Apply this diff to add
.max(2048)to the URL fields:-websiteURL: text("website_url"), +websiteURL: text("website_url").max(2048), -xURL: text("x_url"), +xURL: text("x_url").max(2048), -youtubeURL: text("youtube_url"), +youtubeURL: text("youtube_url").max(2048),Repeat for all URL fields.
src/plugins/seedInitialData.ts (2)
97-98: 🛠️ Refactor suggestionUse a more reliable field to check for community existence.
Using
logoMimeTypeto check if a community exists may not be reliable since it can be null. Consider usingidornamewhich are more reliable identifiers.Apply this diff:
-let existingCommunity: - | Pick<typeof communitiesTable.$inferSelect, "logoMimeType"> +let existingCommunity: + | Pick<typeof communitiesTable.$inferSelect, "id"> | undefined; existingCommunity = await fastify.drizzleClient.query.communitiesTable.findFirst({ columns: { - logoMimeType: true, + id: true, }, });
125-146: 🧹 Nitpick (assertive)Enhance error handling and validate required fields before insertion.
Currently, there is no validation for required fields, and error logging lacks detail. Improve the error handling by validating required fields and including error details in logs.
Apply this refactored implementation:
try { + const communityData = { + facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, + githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, + inactivityTimeoutDuration: + fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, + instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, + linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, + name: fastify.envConfig.API_COMMUNITY_NAME, + redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, + slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, + websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, + xURL: fastify.envConfig.API_COMMUNITY_X_URL, + youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, + }; + + // Validate required fields + if (!communityData.name) { + throw new Error("Community name is required"); + } + await fastify.drizzleClient.insert(communitiesTable).values( - communitiesTableInsertSchema.parse({ - facebookURL: fastify.envConfig.API_COMMUNITY_FACEBOOK_URL, - githubURL: fastify.envConfig.API_COMMUNITY_GITHUB_URL, - inactivityTimeoutDuration: - fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, - instagramURL: fastify.envConfig.API_COMMUNITY_INSTAGRAM_URL, - linkedinURL: fastify.envConfig.API_COMMUNITY_LINKEDIN_URL, - name: fastify.envConfig.API_COMMUNITY_NAME, - redditURL: fastify.envConfig.API_COMMUNITY_REDDIT_URL, - slackURL: fastify.envConfig.API_COMMUNITY_SLACK_URL, - websiteURL: fastify.envConfig.API_COMMUNITY_WEBSITE_URL, - xURL: fastify.envConfig.API_COMMUNITY_X_URL, - youtubeURL: fastify.envConfig.API_COMMUNITY_YOUTUBE_URL, - }), + communitiesTableInsertSchema.parse(communityData), ); } catch (error) { - throw new Error("Failed to create the community in the database.", { + throw new Error(`Failed to create the community in the database: ${error.message}`, { cause: error, }); }test/routes/graphql/gql.tada.d.ts (1)
115-115: 🛠️ Refactor suggestionEnsure compliance with data privacy regulations in
Usertype.When handling user data in the
Usertype, verify that sensitive information is appropriately protected and that access controls are in place.Consider restricting access to sensitive fields or implementing authorization checks to enhance security.
src/graphql/types/User/avatarURL.ts (1)
8-13:⚠️ Potential issueAdd input validation for avatarName to prevent path traversal.
While the null checking is good, the implementation remains vulnerable to path traversal attacks.
Apply this diff to add validation:
resolve: async (parent, _args, ctx) => - parent.avatarName === null + !parent.avatarName || !/^[a-zA-Z0-9._-]+$/.test(parent.avatarName) ? null : new URL( `/objects/${parent.avatarName}`, ctx.envConfig.API_BASE_URL, ).toString(),src/graphql/types/Chat/avatarURL.ts (1)
8-13: 🧹 Nitpick (assertive)Extract duplicated URL generation logic into a shared utility.
The URL generation pattern is duplicated across multiple avatar implementations. Consider extracting this into a shared utility function.
src/graphql/types/Community/logoURL.ts (1)
1-17: 🧹 Nitpick (assertive)Consider implementing type safety for URL generation.
The URL generation pattern could benefit from stronger typing and validation.
Verify the necessity of database-stored logo URLs.
Based on the PR discussion about storing community data, consider if logo URLs could be handled through environment variables or client-side configuration instead of database storage.
Run this script to analyze the usage pattern:
#!/bin/bash # Check if logoURL is frequently queried rg -l "logoURL" | xargs -I {} rg -A 5 "logoURL" {} # Check if there are existing environment-based configurations rg -l "API_BASE_URL|BASE_URL" | xargs -I {} rg -A 5 "API_BASE_URL|BASE_URL" {}src/graphql/types/Query/index.ts (1)
8-8: 🧹 Nitpick (assertive)Reconsider the approach to community data storage.
Based on the PR discussion, storing community data in the database might not be the optimal approach if:
- The data lacks meaningful relationships with other entities
- The fields are client-specific configurations
- Each user access requires an additional API call
src/graphql/types/Query/community.ts (2)
5-28: 🧹 Nitpick (assertive)Consider implementing caching for community data.
Since community configuration is relatively static and frequently queried (on sign-in/sign-up), implementing caching would improve performance and reduce database load.
18-23: 🛠️ Refactor suggestionUse a more specific error code and message.
The current error handling uses a generic "unexpected" code. Consider using a more specific error code and message.
throw new TalawaGraphQLError({ extensions: { - code: "unexpected", + code: "COMMUNITY_NOT_FOUND", + message: "No community configuration found. Please contact your administrator.", }, });envFiles/.env.production (3)
3-4:⚠️ Potential issueProduction environment is missing critical security credentials.
Empty administrator credentials in production environment pose a security risk.
7-17: 🛠️ Refactor suggestionConsider security implications of community URLs.
The community-related URLs are exposed in environment variables. Consider implementing URL validation and security headers.
9-9: 🧹 Nitpick (assertive)Make timeout duration configurable.
The inactivity timeout duration is hardcoded to 900 seconds (15 minutes).
envFiles/.env.ci (1)
9-9: 🧹 Nitpick (assertive)Document the inactivity timeout duration.
The timeout duration of 900 seconds (15 minutes) should be documented to explain why this specific duration was chosen for the CI environment.
src/graphql/types/Mutation/updateCommunity.ts (2)
183-201:⚠️ Potential issueEnhance error handling for file operations.
File upload and deletion operations lack proper error handling and logging.
64-120: 🧹 Nitpick (assertive)Refactor authentication checks into a reusable utility.
The authentication and authorization checks could be moved to a reusable utility function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
compose.yaml(1 hunks)documentation/environment_variables.md(2 hunks)drizzle_migrations/20250118150033_needy_tigra.sql(2 hunks)drizzle_migrations/meta/20250118150033_snapshot.json(2 hunks)drizzle_migrations/meta/_journal.json(1 hunks)envFiles/.env.ci(1 hunks)envFiles/.env.devcontainer(1 hunks)envFiles/.env.production(1 hunks)package.json(2 hunks)schema.graphql(10 hunks)src/drizzle/schema.ts(1 hunks)src/drizzle/tables/communities.ts(1 hunks)src/drizzle/tables/users.ts(3 hunks)src/envConfigSchema.ts(2 hunks)src/graphql/inputs/MutationCreateUserInput.ts(1 hunks)src/graphql/inputs/MutationSignUpInput.ts(1 hunks)src/graphql/inputs/MutationUpdateCommunityInput.ts(1 hunks)src/graphql/inputs/MutationUpdateCurrentUserInput.ts(1 hunks)src/graphql/inputs/MutationUpdateUserInput.ts(1 hunks)src/graphql/inputs/QuerySignInInput.ts(1 hunks)src/graphql/inputs/index.ts(1 hunks)src/graphql/scalars/EmailAddress.ts(0 hunks)src/graphql/scalars/index.ts(0 hunks)src/graphql/types/Chat/avatarURL.ts(1 hunks)src/graphql/types/Community/Community.ts(1 hunks)src/graphql/types/Community/index.ts(1 hunks)src/graphql/types/Community/logoURL.ts(1 hunks)src/graphql/types/Community/updatedAt.ts(1 hunks)src/graphql/types/Community/updater.ts(1 hunks)src/graphql/types/Mutation/index.ts(1 hunks)src/graphql/types/Mutation/updateCommunity.ts(1 hunks)src/graphql/types/Organization/avatarURL.ts(1 hunks)src/graphql/types/Query/community.ts(1 hunks)src/graphql/types/Query/index.ts(1 hunks)src/graphql/types/User/avatarURL.ts(1 hunks)src/graphql/types/User/emailAddress.ts(1 hunks)src/graphql/types/index.ts(1 hunks)src/plugins/index.ts(1 hunks)src/plugins/seedInitialData.ts(3 hunks)test/drizzle.config.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(9 hunks)test/server.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/graphql/scalars/EmailAddress.ts
- src/graphql/scalars/index.ts
🔇 Additional comments (34)
src/graphql/inputs/MutationSignUpInput.ts (1)
58-61: 🧹 Nitpick (assertive)
⚠️ Potential issueImplement consistent email validation across signup flow.
The email validation should be consistent with the sign-in flow to ensure data integrity throughout the user lifecycle.
Add the same email validation here:
emailAddress: t.string({ description: "Email address of the user.", required: true, + validate: { email: true }, }),Likely invalid or redundant comment.
src/graphql/inputs/MutationCreateUserInput.ts (1)
63-66: Verify existing email validation implementations.Let's check if email validation is handled elsewhere in the codebase to avoid duplication.
✅ Verification successful
Email validation implementation is consistent with codebase patterns
The current implementation using
t.string()matches the pattern used across all other email-related GraphQL inputs in the codebase. No additional validation is required as it follows the established convention.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing email validation implementations # Look for email validation utilities echo "=== Searching for email validation utilities ===" rg -l "email.*valid|validateEmail|isEmail" src/ # Check for validation in resolvers echo "=== Checking resolvers for email validation ===" rg -A 5 "email.*valid|validateEmail|isEmail" "src/graphql/resolvers" # Look for validation middleware echo "=== Checking for validation middleware ===" fd -t f "validation|validator" src/middleware # Check for Zod schema validations echo "=== Checking Zod schema validations ===" rg "z\.string\(\).*email" src/Length of output: 1205
Script:
#!/bin/bash # Check the content of input files for email validation patterns echo "=== Checking input files for email validation patterns ===" rg -A 3 "emailAddress.*string" src/graphql/inputs/ # Look for any shared validation utilities echo "=== Checking for shared validation utilities ===" rg -l "import.*valid" src/ rg -l "import.*email" src/ # Check for any validation-related utilities echo "=== Checking for validation utilities ===" fd util src/ | grep -i validLength of output: 1948
src/graphql/types/User/emailAddress.ts (1)
Line range hint
8-44: LGTM! Robust authorization checks.The authorization logic properly handles:
- Authentication verification
- Role-based access control
- Error cases with appropriate error codes
src/graphql/types/Community/updatedAt.ts (1)
34-40: Re-evaluate role-based access restriction.Access to the
updatedAtfield is currently restricted to administrators. Consider whether other roles, such as "moderator" or "editor", should also have access based on the application's requirements.src/graphql/types/Community/updater.ts (1)
50-67: 🧹 Nitpick (assertive)Handle potential exceptions during database query for
existingUserand implement alerting.Wrap the database query in a try-catch block to handle exceptions. Additionally, consider integrating an alerting mechanism to notify the development team when
existingUserisundefined, as this may indicate data corruption that requires immediate attention.Apply this diff:
+ let existingUser; + try { + existingUser = await ctx.drizzleClient.query.usersTable.findFirst({ + where: (fields, operators) => operators.eq(fields.id, updaterId), + }); + } catch (error) { + throw new TalawaGraphQLError({ + extensions: { + code: "database_error", + message: error.message, + }, + }); + } // Updater id existing but the associated user not existing is either a business logic error which means that the corresponding data in the database is in a corrupted state or it is a rare race condition. It must be investigated and fixed as soon as possible to prevent further data corruption if the former case is true. if (existingUser === undefined) { + // Send alert to development team + ctx.alertingService.notify( + "Data Integrity Issue", + "Updater ID exists but associated user not found." + ); ctx.log.warn( "Postgres select operation returned an empty array for a community's updater id that isn't null.", ); throw new TalawaGraphQLError({ extensions: { code: "unexpected", }, }); }Likely invalid or redundant comment.
src/drizzle/tables/communities.ts (2)
59-59: Ensurenamefield has a maximum length to prevent excessive input.The
namefield currently has a maximum length of 256 characters. Verify that this length aligns with business requirements and consider reducing it if appropriate to prevent excessively long names.
116-128: 🛠️ Refactor suggestionAdd maximum length validation for URL fields in the insert schema.
In the
communitiesTableInsertSchema, the URL fields should have a.max(2048)constraint to prevent excessively long inputs.Apply this diff to the insert schema:
-facebookURL: (schema) => schema.url().optional(), +facebookURL: (schema) => schema.url().max(2048).optional(), -githubURL: (schema) => schema.url().optional(), +githubURL: (schema) => schema.url().max(2048).optional(), -instagramURL: (schema) => schema.url().optional(), +instagramURL: (schema) => schema.url().max(2048).optional(), -linkedinURL: (schema) => schema.url().optional(), +linkedinURL: (schema) => schema.url().max(2048).optional(), -redditURL: (schema) => schema.url().optional(), +redditURL: (schema) => schema.url().max(2048).optional(), -slackURL: (schema) => schema.url().optional(), +slackURL: (schema) => schema.url().max(2048).optional(), -websiteURL: (schema) => schema.url().optional(), +websiteURL: (schema) => schema.url().max(2048).optional(), -xURL: (schema) => schema.url().optional(), +xURL: (schema) => schema.url().max(2048).optional(), -youtubeURL: (schema) => schema.url().optional(), +youtubeURL: (schema) => schema.url().max(2048).optional(),Likely invalid or redundant comment.
src/plugins/seedInitialData.ts (1)
131-131:⚠️ Potential issueAdd default value or validation for
inactivityTimeoutDuration.The
inactivityTimeoutDurationmay beundefinedif the environment variable is not set. Consider adding a default value or validating that it is provided to prevent runtime errors.Apply this diff to set a default value:
-inactivityTimeoutDuration: - fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION, +inactivityTimeoutDuration: + fastify.envConfig.API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION || 3600, // Default to 1 hourAlternatively, add validation to ensure it is provided.
Likely invalid or redundant comment.
test/routes/graphql/gql.tada.d.ts (2)
31-31: Verify the completeness and correctness of theCommunitytype definition.Ensure that all required fields in the
Communitytype align with the database schema and business requirements.
150-150: Review the addedcommunityquery field for security and validation.The new
communityquery should be examined to ensure it handles input validation properly and does not expose any sensitive data.src/plugins/index.ts (1)
4-4: Update documentation and plugin name to match new implementation.The old seeding mechanism has been properly removed, but there are two minor inconsistencies in
src/plugins/seedInitialData.ts:
- The JSDoc example still shows the old import statement
- The plugin is registered with the old name "seedDatabase"
While these don't affect functionality, they should be updated for consistency.
Also applies to: 9-9
src/graphql/types/Community/Community.ts (2)
9-10: Fix typo in description.There's a typo in the description text: "Communitys" should be "Communities".
16-56: Consider adding URL validation and security measures.
- URL fields lack validation to ensure data integrity.
- Exposing all social media URLs by default might not be necessary for all clients.
Consider:
- Adding URL format validation using a custom scalar
- Implementing field-level permissions for sensitive fields
src/drizzle/schema.ts (1)
39-42: LGTM!The new exports follow the established pattern and are correctly placed alphabetically in the file.
src/envConfigSchema.ts (1)
51-57: Fix incorrect documentation comment.The documentation comment for
API_COMMUNITY_INACTIVITY_TIMEOUT_DURATIONis incorrectly showing Instagram URL description./** - * URL to the instagram account of the community. + * Duration in milliseconds after which a user is considered inactive. */src/drizzle/tables/users.ts (3)
32-32: LGTM!The import of
communitiesTableis correctly added to support the new relationship definition.
344-349: LGTM!The one-to-many relationship between users and communities is properly defined with clear documentation.
656-656: LGTM!The email validation is improved using the built-in email validator.
src/graphql/types/Community/index.ts (1)
1-4: LGTM!The barrel exports are well-organized and follow the project's module organization pattern.
drizzle_migrations/meta/_journal.json (1)
8-9: Add required label for sensitive file changes.While the migration journal updates are correct, this file is considered sensitive and requires special handling.
Please add the 'ignore-sensitive-files-pr' label to proceed with the changes.
src/graphql/types/Organization/avatarURL.ts (1)
8-13: Implementation matches Chat.avatarURL pattern.This implementation is identical to Chat.avatarURL, further supporting the need for a shared utility function.
src/graphql/types/index.ts (1)
9-9: LGTM!The Community module import follows the established pattern and maintains alphabetical order.
test/server.ts (1)
14-17: LGTM! Documentation improvement.The added comment clearly explains the purpose of setting API_PORT to undefined for test instances.
test/drizzle.config.ts (1)
1-3: LGTM! Enhanced documentation.The updated comment provides better context about the file's purpose.
src/graphql/types/Mutation/index.ts (1)
54-54: LGTM! Consistent import pattern.The new import follows the established pattern in the file.
src/graphql/inputs/index.ts (1)
52-52: LGTM!The new import follows the established pattern and maintains alphabetical order.
package.json (1)
41-41: Verify compatibility with the Node.js version.The dependency updates look good, but let's verify compatibility with Node.js v22.13.0 specified in the engines field.
Also applies to: 51-51
✅ Verification successful
✓ Node.js compatibility verified
The updated dependencies are compatible with Node.js v22.13.0 as they support Node.js versions >=22.0.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the updated versions are compatible with Node.js v22.13.0 npm view @vitest/coverage-v8@^3.0.2 engines npm view vitest@^3.0.2 enginesLength of output: 125
envFiles/.env.devcontainer (1)
7-17: Same issues as in .env.ci file.The same recommendations about placeholder domains and timeout documentation apply here.
compose.yaml (2)
41-41: 🛠️ Refactor suggestionAdd default value for inactivity timeout.
The inactivity timeout should have a sensible default value instead of being required.
- - API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=${API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION:?error} + - API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION=${API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION:-900}Likely invalid or redundant comment.
39-49: 🛠️ Refactor suggestionMake social media URLs optional with empty defaults.
Based on the schema and PR objectives, social media URLs are optional fields. They should have empty string defaults instead of being required.
- - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:?error} + - API_COMMUNITY_FACEBOOK_URL=${API_COMMUNITY_FACEBOOK_URL:-} - - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:?error} + - API_COMMUNITY_GITHUB_URL=${API_COMMUNITY_GITHUB_URL:-} - - API_COMMUNITY_INSTAGRAM_URL=${API_COMMUNITY_INSTAGRAM_URL:?error} + - API_COMMUNITY_INSTAGRAM_URL=${API_COMMUNITY_INSTAGRAM_URL:-} - - API_COMMUNITY_LINKEDIN_URL=${API_COMMUNITY_LINKEDIN_URL:?error} + - API_COMMUNITY_LINKEDIN_URL=${API_COMMUNITY_LINKEDIN_URL:-}Likely invalid or redundant comment.
schema.graphql (3)
2662-2664: 🧹 Nitpick (assertive)Enhance query documentation and error handling.
The query field lacks documentation about error cases and potential future pagination needs.
- """Query field to read the community.""" + """Query field to read the community configuration. + Returns null if no community is configured. + Note: Currently returns a single community. If multiple communities are needed in the future, + consider adding pagination and search capabilities. + """ community: CommunityLikely invalid or redundant comment.
356-358: 🛠️ Refactor suggestionEnhance documentation for inactivityTimeoutDuration.
The documentation should clarify the purpose and impact of this setting.
- """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. + """ Duration in seconds after which inactive clients will be automatically logged out from their authenticated session in client-side Talawa applications. + Set to null or 0 to disable the timeout feature. + Default: 900 seconds (15 minutes) """Likely invalid or redundant comment.
1957-1996: 🛠️ Refactor suggestionAdd validation directives for input fields.
The input fields lack validation directives to ensure data integrity.
input MutationUpdateCommunityInput { - """URL to the facebook account of the community.""" - facebookURL: String + """URL to the facebook account of the community. Must be a valid Facebook URL.""" + facebookURL: String @pattern(regexp: "^https?://(www\\.)?facebook\\.com/.*$") - """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. - """ - inactivityTimeoutDuration: Int + """ Duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. + Must be a positive integer or zero. + """ + inactivityTimeoutDuration: Int @range(min: 0)Likely invalid or redundant comment.
drizzle_migrations/meta/20250118150033_snapshot.json (1)
2051-2188: 🛠️ Refactor suggestionAdd indexes for frequently queried fields.
The communities table should have indexes for:
- created_at - for chronological sorting
- updated_at - for change tracking
- inactivity_timeout_duration - for timeout queries
Apply this diff to add indexes:
"indexes": { + "communities_created_at_index": { + "name": "communities_created_at_index", + "columns": [ + { + "expression": "created_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "communities_updated_at_index": { + "name": "communities_updated_at_index", + "columns": [ + { + "expression": "updated_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "communities_inactivity_timeout_duration_index": { + "name": "communities_inactivity_timeout_duration_index", + "columns": [ + { + "expression": "inactivity_timeout_duration", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } },Likely invalid or redundant comment.
| emailAddress: t.string({ | ||
| description: "Email address of the user.", | ||
| type: "EmailAddress", | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add email validation for user updates.
Email updates should be validated to maintain data consistency and prevent invalid email addresses.
Add email validation:
emailAddress: t.string({
description: "Email address of the user.",
+ validate: { email: 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.
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| type: "EmailAddress", | |
| }), | |
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| validate: { email: true }, | |
| }), |
| emailAddress: t.string({ | ||
| description: "Email address of the user.", | ||
| required: true, | ||
| type: "EmailAddress", | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure email validation during user creation.
Administrative user creation should enforce the same email validation standards as other flows.
Add email validation:
emailAddress: t.string({
description: "Email address of the user.",
required: true,
+ validate: { email: true },
}),Consider extracting the email validation logic into a shared utility to maintain consistency across all email-related inputs.
📝 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.
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| required: true, | |
| type: "EmailAddress", | |
| }), | |
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| required: true, | |
| validate: { email: true }, | |
| }), |
| emailAddress: t.string({ | ||
| description: "Email address of the user.", | ||
| type: "EmailAddress", | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding GraphQL-level email validation.
The change from custom EmailAddress scalar to string removes GraphQL-level format validation. While Zod validation exists through usersTableInsertSchema, having validation at both layers provides better security and earlier invalid input detection.
Consider adding validation constraints:
emailAddress: t.string({
description: "Email address of the user.",
+ validate: {
+ regex: /^[^\s@]+@[^\s@]+\.[^\s@]+$/,
+ message: "Invalid email format"
+ }
}),📝 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.
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| type: "EmailAddress", | |
| }), | |
| emailAddress: t.string({ | |
| description: "Email address of the user.", | |
| validate: { | |
| regex: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, | |
| message: "Invalid email format" | |
| } | |
| }), |
| if (!ctx.currentClient.isAuthenticated) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const currentUserId = ctx.currentClient.user.id; | ||
|
|
||
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | ||
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | ||
| }); | ||
|
|
||
| if (currentUser === undefined) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "forbidden_action", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (currentUser.role !== "administrator") { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthorized_action", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (parent.updaterId === null) { | ||
| return null; | ||
| } | ||
|
|
||
| if (parent.updaterId === currentUserId) { | ||
| return currentUser; | ||
| } | ||
|
|
||
| const updaterId = parent.updaterId; | ||
|
|
||
| const existingUser = await ctx.drizzleClient.query.usersTable.findFirst( | ||
| { | ||
| where: (fields, operators) => operators.eq(fields.id, updaterId), | ||
| }, | ||
| ); | ||
|
|
||
| // Updater id existing but the associated user not existing is either a business logic error which means that the corresponding data in the database is in a corrupted state or it is a rare race condition. It must be investigated and fixed as soon as possible to prevent further data corruption if the former case is true. | ||
| if (existingUser === undefined) { | ||
| ctx.log.warn( | ||
| "Postgres select operation returned an empty array for a community's updater id that isn't null.", | ||
| ); | ||
|
|
||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unexpected", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reduce duplication in error handling logic.
Multiple blocks in the code handle errors similarly. Consider creating helper functions for standardized error throwing to improve maintainability and reduce code duplication.
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | ||
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle potential exceptions during database query for currentUser.
Wrap the database query in a try-catch block to handle any unexpected exceptions that may occur during the query operation. This ensures that errors are properly caught and handled.
Apply this diff:
- const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({
- where: (fields, operators) => operators.eq(fields.id, currentUserId),
- });
+ let currentUser;
+ try {
+ currentUser = await ctx.drizzleClient.query.usersTable.findFirst({
+ where: (fields, operators) => operators.eq(fields.id, currentUserId),
+ });
+ } catch (error) {
+ throw new TalawaGraphQLError({
+ extensions: {
+ code: "database_error",
+ message: error.message,
+ },
+ });
+ }📝 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.
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | |
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | |
| }); | |
| let currentUser; | |
| try { | |
| currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | |
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | |
| }); | |
| } catch (error) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "database_error", | |
| message: error.message, | |
| }, | |
| }); | |
| } |
| ## API_COMMUNITY_FACEBOOK_URL | ||
|
|
||
| This environment variable is used to configure the url to the [facebook](https://facebook.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_GITHUB_URL | ||
|
|
||
| This environment variable is used to configure the url to the [github](https://github.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION | ||
|
|
||
| This environment variable is used to configure the duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
|
|
||
| ## API_COMMUNITY_INSTAGRAM_URL | ||
|
|
||
| This environment variable is used to configure the url to the [instagram](https://instagram.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_LINKEDIN_URL | ||
|
|
||
| This environment variable is used to configure the url to the [linkedin](https://linkedin.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_NAME | ||
|
|
||
| This environment variable is used to configure the name of the community. | ||
|
|
||
| ## API_COMMUNITY_REDDIT_URL | ||
|
|
||
| This environment variable is used to configure the url to the [reddit](https://reddit.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_SLACK_URL | ||
|
|
||
| This environment variable is used to configure the url to the [slack](https://slack.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_WEBSITE_URL | ||
|
|
||
| This environment variable is used to configure the url to the website of the community. | ||
|
|
||
| ## API_COMMUNITY_X_URL | ||
|
|
||
| This environment variable is used to configure the url to the [x](https://x.com) account of the community. | ||
|
|
||
| ## API_COMMUNITY_YOUTUBE_URL | ||
|
|
||
| This environment variable is used to configure the url to the [youtube](https://youtube.com) account of the community. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add URL validation rules and constraints.
For all URL-related environment variables (Facebook, GitHub, Instagram, etc.), the documentation should specify:
- Required URL format and validation rules
- Allowed protocols (http/https)
- Maximum length constraints
- Error handling behavior for invalid URLs
🧹 Nitpick (assertive)
Consider client-side configuration based on PR feedback.
Based on the author's concerns in the PR comments, storing community data in the database might introduce unnecessary complexity. Consider:
- Moving these configurations to client-side environment variables
- Using a static configuration file
- Implementing a caching mechanism to reduce API calls
This aligns with the author's concerns about:
- Unnecessary database storage
- Extra API calls for sign-in/sign-up pages
- Deployment complexity
| ## API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION | ||
|
|
||
| This environment variable is used to configure the duration in seconds it should take for inactive clients to get timed out of their authenticated session within client-side talawa applications. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify timeout duration constraints and security implications.
The documentation for API_COMMUNITY_INACTIVITY_TIMEOUT_DURATION should include:
- Minimum and maximum allowed values
- Default value if not specified
- Security implications of very short/long timeouts
- Unit of measurement (seconds)
| ## API_COMMUNITY_NAME | ||
|
|
||
| This environment variable is used to configure the name of the community. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document community name constraints.
The documentation for API_COMMUNITY_NAME should specify:
- Maximum/minimum length requirements
- Allowed characters and format
- Whether this is a required field
- Impact on existing data if changed
| "inactivity_timeout_duration": { | ||
| "name": "inactivity_timeout_duration", | ||
| "type": "integer", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add constraints for inactivity_timeout_duration.
The inactivity_timeout_duration field should have:
- A CHECK constraint for minimum value (e.g., >= 0)
- A CHECK constraint for maximum value to prevent unreasonable timeouts
Apply this diff to add constraints:
"checkConstraints": {
+ "communities_inactivity_timeout_duration_check": {
+ "name": "communities_inactivity_timeout_duration_check",
+ "expression": "inactivity_timeout_duration >= 0 AND inactivity_timeout_duration <= 86400"
+ }
},Committable suggestion skipped: line range outside the PR's diff.
| "facebook_url": { | ||
| "name": "facebook_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "github_url": { | ||
| "name": "github_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "id": { | ||
| "name": "id", | ||
| "type": "uuid", | ||
| "primaryKey": true, | ||
| "notNull": true | ||
| }, | ||
| "inactivity_timeout_duration": { | ||
| "name": "inactivity_timeout_duration", | ||
| "type": "integer", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "instagram_url": { | ||
| "name": "instagram_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "linkedin_url": { | ||
| "name": "linkedin_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "logo_mime_type": { | ||
| "name": "logo_mime_type", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "logo_name": { | ||
| "name": "logo_name", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "name": { | ||
| "name": "name", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "reddit_url": { | ||
| "name": "reddit_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "slack_url": { | ||
| "name": "slack_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "updated_at": { | ||
| "name": "updated_at", | ||
| "type": "timestamp (3) with time zone", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "updater_id": { | ||
| "name": "updater_id", | ||
| "type": "uuid", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "website_url": { | ||
| "name": "website_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "x_url": { | ||
| "name": "x_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "youtube_url": { | ||
| "name": "youtube_url", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add URL field validations.
All URL fields (facebook_url, github_url, etc.) should have:
- CHECK constraints for valid URL formats
- Maximum length constraints
Apply this diff to add URL validation:
"checkConstraints": {
+ "communities_facebook_url_check": {
+ "name": "communities_facebook_url_check",
+ "expression": "facebook_url ~ '^https?://([^/]+\\.)?facebook\\.com/.*$'"
+ },
+ "communities_github_url_check": {
+ "name": "communities_github_url_check",
+ "expression": "github_url ~ '^https?://([^/]+\\.)?github\\.com/.*$'"
+ }
},Committable suggestion skipped: line range outside the PR's diff.
palisadoes
merged commit d474572
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
feature
Issue Number:
Fixes #
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Added the missing implementation for storing community data.
Session timeout feature not yet implemented unless it
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Release Notes
New Features
Configuration Updates
Database Changes
communitiestable to store community-related information.Documentation