feat(reminders): specify recipients #3453
Conversation
|
""" WalkthroughThis change introduces support for specifying and managing reminder recipients for secrets within the application. On the backend, a new table Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/services/secret/secret-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (1)
106-106:⚠️ Potential issueFix the TypeScript error in form values type
The pipeline failure indicates a type error where
SecretV3RawSanitizedis not assignable to the expected type. This suggests that the type definition for the form values needs to be updated to include the newreminderRecipientsfield.#!/bin/bash # Find the TypeScript type definition for SecretV3RawSanitized rg -l "SecretV3RawSanitized" --type ts | grep -v node_modules | xargs grep -l "interface\|type" | head -n 5🧰 Tools
🪛 GitHub Actions: Check Frontend Type and Lint check
[error] 106-106: TypeScript error TS2322: Type 'SecretV3RawSanitized' is not assignable to the expected type at this location.
🧹 Nitpick comments (2)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (1)
10-32: Improve type safety in the findUsersBySecretId methodThe
findUsersBySecretIdquery is well-structured with appropriate joins and filtering, but it returnsPromise<any[]>which lacks type safety. Consider defining an interface for the return type to make the code more robust and easier to maintain.- const findUsersBySecretId = async (secretId: string) => { + interface SecretReminderRecipientWithUser { + id: string; + secretId: string; + userId: string; + projectId: string; + email: string; + username: string; + firstName: string | null; + lastName: string | null; + } + + const findUsersBySecretId = async (secretId: string): Promise<SecretReminderRecipientWithUser[]> => {frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx (1)
197-197: Consider resetting recipients when deleting a reminder.When deleting a reminder, the code sets days and note to null but doesn't explicitly set recipients to null. This might lead to unexpected behavior if the form is later used to create a new reminder.
Consider updating the delete handler to also reset recipients:
- onClick={() => onOpenChange(false, { days: null, note: null })} + onClick={() => onOpenChange(false, { days: null, note: null, recipients: [] })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/src/@types/knex.d.ts(2 hunks)backend/src/db/migrations/20250419004044_secret-reminder-recipients.ts(1 hunks)backend/src/db/schemas/certificates.ts(1 hunks)backend/src/db/schemas/kmip-org-server-certificates.ts(1 hunks)backend/src/db/schemas/models.ts(1 hunks)backend/src/db/schemas/oidc-configs.ts(1 hunks)backend/src/db/schemas/organizations.ts(1 hunks)backend/src/db/schemas/secret-reminder-recipients.ts(1 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts(1 hunks)backend/src/lib/api-docs/constants.ts(1 hunks)backend/src/server/routes/index.ts(4 hunks)backend/src/server/routes/v3/secret-router.ts(2 hunks)backend/src/services/project-membership/project-membership-service.ts(6 hunks)backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(1 hunks)backend/src/services/secret/secret-queue.ts(10 hunks)backend/src/services/secret/secret-service.ts(3 hunks)backend/src/services/secret/secret-types.ts(3 hunks)frontend/src/hooks/api/secrets/mutations.tsx(2 hunks)frontend/src/hooks/api/secrets/types.ts(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx(5 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(3 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.tsx(6 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/src/server/routes/index.ts (1)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (1)
secretReminderRecipientsDALFactory(7-35)
backend/src/services/project-membership/project-membership-service.ts (1)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (1)
TSecretReminderRecipientsDALFactory(5-5)
backend/src/@types/knex.d.ts (1)
backend/src/db/schemas/secret-reminder-recipients.ts (3)
TSecretReminderRecipients(19-19)TSecretReminderRecipientsInsert(20-20)TSecretReminderRecipientsUpdate(21-23)
backend/src/services/secret/secret-queue.ts (3)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (1)
TSecretReminderRecipientsDALFactory(5-5)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (1)
TCreateSecretReminderDTO(234-238)backend/src/services/secret/secret-types.ts (1)
TCreateSecretReminderDTO(409-414)
backend/src/db/schemas/secret-reminder-recipients.ts (1)
backend/src/db/schemas/models.ts (1)
TImmutableDBKeys(153-153)
backend/src/services/secret/secret-types.ts (1)
backend/src/db/schemas/secrets.ts (1)
TSecrets(36-36)
backend/src/server/routes/v3/secret-router.ts (1)
backend/src/lib/api-docs/constants.ts (1)
RAW_SECRETS(709-789)
🪛 GitHub Actions: Check Frontend Type and Lint check
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
[error] 106-106: TypeScript error TS2322: Type 'SecretV3RawSanitized' is not assignable to the expected type at this location.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check TS and Lint
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
🔇 Additional comments (69)
backend/src/db/schemas/organizations.ts (1)
26-26: New organization-level setting for secret sharingThis new boolean field
secretShareSendToAnyonecontrols whether secrets can be shared with anyone or only specific recipients. It defaults totrue(allowing sharing with anyone) and is properly defined as nullable and optional to maintain backward compatibility with existing database records.backend/src/db/schemas/models.ts (1)
149-150: New table entries for secret rotation and reminder recipientsThe addition of
SecretReminderRecipientsto theTableNameenum properly registers the new database table for storing recipients of secret reminders. This is a key component of the new feature to specify recipients for secret reminders.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.utils.ts (1)
50-50: Added support for recipient selection in form schemaThe new
reminderRecipientsfield in the form schema correctly defines the expected data structure for secret reminder recipients as an array of UUIDs. This validates user input when selecting specific recipients for secret reminders.backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts (1)
36-36: Looks good - new field integrates properly with approval workflow types.The new
secretReminderRecipientsfield is appropriately typed as an optional string array or null, allowing for specification of specific user IDs to receive reminders, while maintaining backward compatibility.frontend/src/hooks/api/secrets/mutations.tsx (1)
86-86: Implementation correctly handles sending reminder recipients through the API.The mutation hook parameter and request payload are properly extended to include the new
secretReminderRecipientsfield, enabling the frontend to specify which users should receive secret reminders.Also applies to: 97-97
backend/src/lib/api-docs/constants.ts (1)
765-766: Documentation clearly explains the new parameter's purpose and default behavior.The API documentation for
secretReminderRecipientsaccurately describes that this parameter accepts an array of user IDs and that the default behavior (when not specified) is to send reminders to all project members, which aligns with the PR objectives.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
539-543: Good optimization to reduce the data passed to the queue service.This change narrows down the data passed to
secretQueueService.handleSecretReminder()by only including the specific fields needed (id,secretReminderNote, andsecretReminderRepeatDays) instead of the entire secret object. This is more efficient and creates a cleaner contract between the services.backend/src/server/routes/index.ts (4)
217-217: Integration of new DAL for secret reminder recipients.Correctly importing the newly created DAL factory for the secret reminder recipients feature.
421-421: Proper instantiation of the secret reminder recipients DAL.The DAL is properly instantiated with the database connection, following the established pattern in the codebase.
726-726: Injecting DAL into project membership service for recipient cleanup.Adding the
secretReminderRecipientsDALto the project membership service enables the cleanup of reminder recipient records when project memberships are removed, ensuring referential integrity.
960-960: Injecting DAL into secret queue service for targeted reminders.This injection allows the secret queue service to access and use the recipient information when processing secret reminders, enabling targeted notifications instead of sending to all project members.
frontend/src/hooks/api/secrets/types.ts (2)
45-45: Added support for reminder recipients in the secret model.The
reminderRecipientsproperty has been added to theSecretV3RawSanitizedtype to store the IDs of users who should receive reminders, aligning with the backend changes.
181-181: Extended DTO to support updating reminder recipients.The
secretReminderRecipientsfield has been added to the update DTO, allowing the frontend to specify recipient IDs when updating a secret. This enables the new feature to target reminders to specific users.backend/src/server/routes/v3/secret-router.ts (2)
651-651: Schema addition for recipient management looks goodThis new optional field properly implements the ability to specify which users should receive secret reminder emails, aligning with the API documentation constants.
682-682: Handler correctly passes the new field to the serviceThe handler properly extracts and passes the
secretReminderRecipientsfrom the request body to theupdateSecretRawservice method, ensuring the data flows correctly to backend services.backend/src/@types/knex.d.ts (2)
426-430: New types imported correctlyThe necessary TypeScript types for the new
SecretReminderRecipientstable are properly imported from the schema file.
1002-1006: Table declaration for new SecretReminderRecipients tableThe Knex table declaration is correctly defined using the appropriate CompositeTableType, properly integrating the new table into the TypeScript type system.
backend/src/services/secret/secret-service.ts (1)
1789-1790: Integration of secretReminderRecipients parameter looks good.The parameter is correctly integrated into the interface and properly passed to
secretV2BridgeService.updateSecretat line 1843, maintaining consistent parameter ordering.backend/src/services/secret/secret-types.ts (4)
25-27: Appropriate extension of TPartialSecret type.The secretReminderRecipients field is properly typed as an optional string array or null, which aligns with the pattern used for other optional fields in this type.
29-31: Appropriate extension of TPartialInputSecret type.The type definition follows the same pattern as the TPartialSecret type, ensuring consistency across related types.
265-265: Appropriate addition to TUpdateSecretRawDTO.The secretReminderRecipients field is correctly added to the update DTO type, enabling proper validation and type checking when updating secrets.
413-413: Consistent typing for TCreateSecretReminderDTO recipients field.The field is named "recipients" here rather than "secretReminderRecipients", maintaining naming consistency with other properties in this specific DTO.
backend/src/db/schemas/secret-reminder-recipients.ts (2)
10-17: Well-structured schema definition for SecretReminderRecipients.The schema properly defines all necessary fields with appropriate validation:
- UUID validation for id, secretId, and userId
- Required date fields for timestamps
- All foreign key fields are marked as required
This aligns with the database schema created in the migration file.
19-23: Clean type exports following project conventions.The types follow the established pattern in the project:
- Full type (TSecretReminderRecipients)
- Insert type omitting immutable fields
- Update type making fields optional and omitting immutable fields
This ensures type safety across database operations.
backend/src/db/migrations/20250419004044_secret-reminder-recipients.ts (2)
10-26: Well-designed table schema with appropriate constraints.The table design includes:
- Primary key with UUID generation
- Proper foreign key relationships with CASCADE on delete
- Index on secretId for query performance
- Unique constraint on the combination of secretId, userId, and projectId to prevent duplicates
Good comment explaining the decision to use userId instead of project membership ID for future extensibility.
17-19:✅ Verification successful
Consider implementing a cleanup mechanism for project membership removal.
The comment acknowledges the need for manual cleanup when users are removed from projects. Ensure this cleanup is implemented in the project membership service to prevent dangling recipient records.
🏁 Script executed:
#!/bin/bash # Check if cleanup is implemented in project-membership service echo "Searching for code that handles cleanup of SecretReminderRecipients when users are removed from projects..." rg -A 5 -B 5 "SecretReminderRecipients.*delete" --glob "**/*project-membership*"Length of output: 1752
🏁 Script executed:
#!/bin/bash echo "Locating usage of secretReminderRecipientsDAL in project-membership-service.ts..." rg -n "secretReminderRecipientsDAL" -A 10 -B 10 backend/src/services/project-membership/project-membership-service.tsLength of output: 3945
Cleanup Already Implemented in Project Membership Service
The service calls
secretReminderRecipientsDAL.deletewhenever project memberships are removed:
- In single‐membership deletion (lines 391–399)
- In bulk‐membership deletion (lines 476–484)
- In self‐removal flow (lines 548–555)
No further action is needed here.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (5)
223-225: Support for reminderRecipients added to function parameterThe function signature has been properly extended to accept
reminderRecipientsas an optional parameter, allowing for specific users to be selected as recipients for secret reminders.
228-228: Reminder recipients included in secret payloadThe
reminderRecipientsfield is now properly included in the payload sent toonSaveSecret, which ensures the selected recipients are saved with the rest of the reminder data.
237-237: Added form state tracking for reminderRecipientsThe component now watches the
reminderRecipientsfield in the form state, which allows it to respond to changes in the recipients selection.
294-294: Passing recipients to CreateReminderForm componentThe
CreateReminderFormcomponent now receives the watchedsecretReminderRecipientsvalue as a prop, enabling it to display and manage the selected recipients.
300-307: Processing and passing reminder recipients from form dataThis logic correctly extracts recipient data from the form, transforms it to the expected format (array of string IDs), updates the form state, and passes it to the submit handler. The conditional check ensures that empty selections are handled properly.
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (4)
5-5: Good type definition for the DAL factoryProperly exporting the type of the DAL factory's return value makes it easier to use this module in other parts of the codebase with strong typing support.
7-9: DAL factory initialization follows project patternsThe factory pattern used here is consistent with other DAL implementations in the project, taking a database client and initializing an ORM for the
SecretReminderRecipientstable.
21-23: Confirm intended behavior for active org memberships filterThe query filters for active organization memberships (
where('${TableName.OrgMembership}.isActive', true)). This means users who have been deactivated in the organization won't receive reminders, even if they're still listed as recipients. Please confirm this is the intended behavior.
34-35: DAL exports both ORM methods and custom queryThe DAL correctly returns both the auto-generated ORM methods and the custom
findUsersBySecretIdquery method, providing a complete interface for interacting with reminder recipients data.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.tsx (6)
88-89: Reminder recipients added to function parametersThe
handleSecretOperationfunction signature has been properly extended to include the newreminderRecipientsparameter.
100-101: Type definition updated for reminderRecipientsThe type definition properly specifies that
reminderRecipientsis an optional parameter that can be either an array of strings or null.
136-137: Reminder recipients included in API call payloadThe new
secretReminderRecipientsfield is now included in the payload for theupdateSecretV3API call, ensuring the backend receives the recipient information.
178-179: Destructuring reminderRecipients from secret dataThe component now correctly extracts the
reminderRecipientsfield from the modified secret for further processing.
196-197: Added reminderRecipients to equality checkThe
reminderRecipientsfield is now included in the list of fields used to determine if a shared secret has been changed, ensuring updates to recipients trigger the appropriate save actions.
232-233: Passing reminderRecipients to handleSecretOperationThe
reminderRecipientsfield is correctly passed to thehandleSecretOperationfunction when updating a shared secret.backend/src/services/project-membership/project-membership-service.ts (6)
26-27: Import for SecretReminderRecipientsDAL typeThe import of the
TSecretReminderRecipientsDALFactorytype is correctly added to support the new dependency.
57-58: Added secretReminderRecipientsDAL to service dependenciesThe service factory dependencies have been properly extended to include the
secretReminderRecipientsDAL, specifying that only thedeletemethod is needed.
76-77: Included secretReminderRecipientsDAL in factory parametersThe parameter list for the service factory function now includes the
secretReminderRecipientsDAL, allowing it to be used within the service.
394-400: Clean up reminder recipients when deleting a project membershipThis code properly deletes reminder recipients associated with a user when their project membership is deleted, ensuring data integrity. The deletion is performed within the same transaction as the membership deletion.
478-486: Clean up reminder recipients when deleting multiple project membershipsWhen deleting multiple project memberships, this code correctly cleans up all associated reminder recipients in the same transaction, using the appropriate filter to match the memberships being deleted.
549-555: Clean up reminder recipients when a user leaves a projectThis code ensures that when a user leaves a project, any reminder recipients associated with that user in the project are deleted, maintaining data consistency.
backend/src/services/secret/secret-queue.ts (12)
9-9: Improved type safety by using enum value instead of string literal.Using
SecretTypeenum instead of string literals enhances code readability and type safety.
57-57: Good addition of dependency for recipient management.Adding the import for
TSecretReminderRecipientsDALFactoryproperly sets up the type structure needed for the new recipient functionality.
114-117: Clean interface design for recipient data access.The interface properly specifies the required methods needed for recipient management: delete, find, insert, and transaction support.
179-179: Properly injected the new DAL dependency.The dependency is correctly injected into the factory function, making it available throughout the service.
189-190: Good cleanup implementation for recipients.Deleting recipients before stopping the reminder job ensures there are no orphaned records in the database.
236-236: Signature update maintains backward compatibility.The function signature has been updated to support recipients while maintaining backward compatibility with existing code.
262-274: Well-implemented transaction for atomicity.Using a transaction ensures that the deletion and insertion of recipients is atomic, preventing partial updates in case of errors.
311-311: Improved type safety with enum usage.Replacing the string literal "personal" with
SecretType.Personalenum provides better type safety and code maintainability.
320-320: Properly passing recipients to the reminder creation function.The code correctly passes the recipients from the new secret to the addSecretReminder function.
1099-1099: Implemented recipient lookup for email notifications.Added code to fetch users by secret ID using the new DAL, supporting the targeted notification feature.
1118-1120: Smart fallback behavior for email recipients.This implementation correctly handles the core functionality - sending emails to specific recipients if available, otherwise defaulting to all project members as described in the PR objectives.
1125-1125: Email notification using selected recipients.The code correctly uses the selected recipients for email notifications, completing the feature implementation.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx (8)
9-19: Added necessary imports for recipient selection functionality.The imports correctly include the FilterableSelect component and the hooks needed to access workspace data and users.
23-30: Well-structured form schema for recipients.The schema properly defines recipients as an optional array of objects with the expected structure (label and UUID value).
43-43: Updated props interface for recipients.The ReminderFormProps interface has been updated to accept the optional recipients prop.
52-52: Added destructuring for recipients prop.The recipients prop is now correctly destructured from the component props.
54-57: Fetching required data for recipient selection.The component now fetches the current workspace and its users, which are needed for the recipient selection functionality.
86-97: Recipients initialization has a potential issue.The effect correctly initializes recipients based on the members and passed props, but it doesn't include recipients in its dependency array. If recipients change externally without the modal being reopened, the effect won't run.
Consider updating the effect to include recipients in its dependency array:
- }, [members, isOpen]); + }, [members, isOpen, recipients]);
114-114: Good UX improvement by marking reminder interval as required.Adding the isRequired prop to the form control clearly indicates to users that this field must be filled.
152-181: Well-implemented recipient selector with helpful tooltip.The FilterableSelect component is correctly integrated for recipient selection with a helpful tooltip explaining the default behavior when no recipients are selected. The component is properly wired to the form control.
backend/src/db/schemas/certificates.ts (1)
23-23:❓ Verification inconclusive
Check if default value removal is intentional
The
.default("")clause was removed from thealtNamesfield schema. This changes the behavior from having an empty string as default to potentially being undefined or null, which could impact code that expects this field to always have at least an empty string value.This change appears unrelated to the main PR objective of adding secret reminder recipients functionality. Is this change intentional or did it happen as part of the schema auto-generation process?
🏁 Script executed:
#!/bin/bash # Check for usage of certificates.altNames to identify potential impacts rg -A 2 -B 2 "certificates.*altNames" --type tsLength of output: 49
🏁 Script executed:
#!/bin/bash # Search for all occurrences of altNames in the TypeScript codebase rg -n "altNames" --type tsLength of output: 6880
🏁 Script executed:
#!/bin/bash # Search for .default() usage in DB schema files rg -n '\.default\(' --type ts backend/src/db/schemasLength of output: 15788
Confirm intentional removal of the Zod default("") on altNames
We’ve dropped the
.default("")from thealtNamesfield in your Zod schemas, which means:
- Incoming payloads no longer get coerced to
""whenaltNamesis absent.- Consumers of the parsed data may now see
undefined(ornull) instead of"".- Database migrations still define a column‐level default of
""on insert, but updates or explicitnullinputs could diverge.Please verify that this was intentional and that higher‐level logic, API clients, and downstream code can handle
undefined/nullhere.Files affected:
- backend/src/db/schemas/certificates.ts:23
- backend/src/db/schemas/kmip-org-server-certificates.ts:16
backend/src/db/schemas/kmip-org-server-certificates.ts (1)
16-16:✅ Verification successful
Potential breaking change in field requirement
The
altNamesfield has been changed from a required non-nullable string to an optional nullable string. This is a significant change that might affect validation rules and type checking throughout the codebase.Since this change appears unrelated to the main PR objective (adding secret reminder recipients), please verify this change is intentional. If so, ensure:
- Any code that relies on
altNamesbeing present is updated- Database migrations properly handle this schema change
🏁 Script executed:
#!/bin/bash # Search for usage of KmipOrgServerCertificates.altNames to identify potential impacts rg -A 2 -B 2 "KmipOrgServerCertificates.*altNames" --type tsLength of output: 62
🏁 Script executed:
#!/bin/bash # Search for all occurrences of the `altNames` field across the codebase rg -n "altNames" -C 2 .Length of output: 28946
No action needed:
altNamesoptional/nullable change is safe and intentionalWe’ve confirmed the Zod schema update for kmip certificates aligns with the existing generic
certificatesschema (altNames: z.string().nullable().optional()), and:
- The database column remains non‑nullable with a default of
"", so all existing rows continue to return a string.- Code paths that consume
altNamesguard against falsy values (if (altNames) …) and default to""in the UI and service layers.- Input validation for API routes uses
validateAltNamesField, not the DB schema, and already accepts optional values.- Migrations for length and column addition do not conflict with this schema change.
No further updates or migrations are required here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds functionality to specify recipients for secret reminders and fixes broken reminder functionality in Secrets V2, with key database and service layer changes to support targeted notifications.
- Added new
secret_reminder_recipientstable with appropriate foreign key constraints and unique composite index in/backend/src/db/migrations/20250419004044_secret-reminder-recipients.ts - Added
secretReminderRecipientsfield (string[] | null) to secret update DTOs and form schemas to support recipient selection - Fixed broken Secrets V2 reminders by updating column references and adding cleanup logic when project members are removed
- Added FilterableSelect component in frontend to allow selecting specific recipients from workspace members
- Modified email sending logic to target specified recipients while maintaining backward compatibility by defaulting to all project members
23 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts
Show resolved
Hide resolved
...rc/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx
Outdated
Show resolved
Hide resolved
backend/src/db/migrations/20250419004044_secret-reminder-recipients.ts
Outdated
Show resolved
Hide resolved
|
Pending: Application testing |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR enhances the secret reminder functionality by adding recipient specification and fixes Secrets V2 reminder issues. Here are the key changes:
- Added new
secret_reminder_recipientstable with proper foreign key constraints and unique composite index on (secretId, userId) in/backend/src/db/migrations/20250419004044_secret-reminder-recipients.ts - Implemented
secretReminderRecipientsDALwith proper type safety and active membership filtering in/backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts - Added cleanup logic in project membership service to remove reminder recipients when users leave projects
- Modified email sending logic to target specified recipients while maintaining backward compatibility
- Fixed broken Secrets V2 reminders by updating column references in the queue service
The changes look well-structured with proper type safety and data integrity considerations. The implementation follows existing patterns and maintains backward compatibility.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts (1)
187-400:⚠️ Potential issueMissing recipients support in the find method.
Unlike findOne and findByFolderIds, the find method doesn't include JOIN operations and field selections for reminder recipients. This inconsistency may cause recipients data to be missing when using this method.
Consider adding similar JOIN operations and field selections to the find method:
const query = (tx || db)(TableName.SecretV2) // eslint-disable-next-line @typescript-eslint/no-misused-promises .where(buildFindFilter(filter)) .leftJoin( TableName.SecretV2JnTag, `${TableName.SecretV2}.id`, `${TableName.SecretV2JnTag}.${TableName.SecretV2}Id` ) .leftJoin( TableName.SecretTag, `${TableName.SecretV2JnTag}.${TableName.SecretTag}Id`, `${TableName.SecretTag}.id` ) .leftJoin(TableName.ResourceMetadata, `${TableName.SecretV2}.id`, `${TableName.ResourceMetadata}.secretId`) .leftJoin( TableName.SecretRotationV2SecretMapping, `${TableName.SecretV2}.id`, `${TableName.SecretRotationV2SecretMapping}.secretId` ) + .leftJoin( + TableName.SecretReminderRecipients, + `${TableName.SecretV2}.id`, + `${TableName.SecretReminderRecipients}.secretId` + ) + .leftJoin(TableName.Users, `${TableName.SecretReminderRecipients}.userId`, `${TableName.Users}.id`)And add selections for the recipient fields:
.select( db.ref("id").withSchema(TableName.ResourceMetadata).as("metadataId"), db.ref("key").withSchema(TableName.ResourceMetadata).as("metadataKey"), db.ref("value").withSchema(TableName.ResourceMetadata).as("metadataValue") ) .select(selectAllTableCols(TableName.SecretV2)) .select(db.ref("id").withSchema(TableName.SecretTag).as("tagId")) .select(db.ref("color").withSchema(TableName.SecretTag).as("tagColor")) .select(db.ref("slug").withSchema(TableName.SecretTag).as("tagSlug")) .select(db.ref("rotationId").withSchema(TableName.SecretRotationV2SecretMapping)) + .select(db.ref("id").withSchema(TableName.SecretReminderRecipients).as("reminderRecipientId")) + .select(db.ref("username").withSchema(TableName.Users).as("reminderRecipientUsername")) + .select(db.ref("email").withSchema(TableName.Users).as("reminderRecipientEmail")) + .select(db.ref("id").withSchema(TableName.Users).as("reminderRecipientUserId"))Also add a child mapper for recipients:
childrenMapper: [ { key: "tagId", label: "tags" as const, mapper: ({ tagId: id, tagColor: color, tagSlug: slug }) => ({ id, color, slug, name: slug }) }, { key: "metadataId", label: "secretMetadata" as const, mapper: ({ metadataKey, metadataValue, metadataId }) => ({ id: metadataId, key: metadataKey, value: metadataValue }) }, + { + key: "reminderRecipientId", + label: "secretReminderRecipients" as const, + mapper: ({ + reminderRecipientId, + reminderRecipientUsername, + reminderRecipientEmail, + reminderRecipientUserId + }) => ({ + user: { + id: reminderRecipientUserId, + username: reminderRecipientUsername, + email: reminderRecipientEmail + }, + id: reminderRecipientId + }) + } ]
♻️ Duplicate comments (1)
backend/src/services/secret/secret-queue.ts (1)
264-278:⚠️ Potential issueHandle edge case with empty recipients array.
The insertMany operation may fail if recipients is an empty array. An explicit check should be added to avoid this potential issue.
if (recipients?.length) { await secretReminderRecipientsDAL.transaction(async (tx) => { await secretReminderRecipientsDAL.delete({ secretId: newSecret.id }, tx); await secretReminderRecipientsDAL.insertMany( recipients.map((r) => ({ secretId: newSecret.id, userId: r, projectId })), tx ); }); } else if (existingRecipients) { await secretReminderRecipientsDAL.delete({ secretId: newSecret.id }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/src/server/routes/v1/dashboard-router.ts(2 hunks)backend/src/services/secret-reminder-recipients/secret-reminder-recipients-types.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts(5 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(3 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(1 hunks)backend/src/services/secret/secret-fns.ts(1 hunks)backend/src/services/secret/secret-queue.ts(10 hunks)backend/src/services/secret/secret-service.ts(4 hunks)frontend/src/hooks/api/secrets/queries.tsx(1 hunks)frontend/src/hooks/api/secrets/types.ts(5 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx(6 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(4 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.tsx(8 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/src/services/secret-reminder-recipients/secret-reminder-recipients-types.ts
- frontend/src/hooks/api/secrets/queries.tsx
- backend/src/services/secret/secret-fns.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts
- backend/src/services/secret/secret-service.ts
- frontend/src/hooks/api/secrets/types.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretListView.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts (1)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-types.ts (1)
TSecretReminderRecipient(1-8)
backend/src/services/secret/secret-queue.ts (3)
backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts (1)
TSecretReminderRecipientsDALFactory(5-5)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (1)
TCreateSecretReminderDTO(234-238)backend/src/services/secret/secret-types.ts (1)
TCreateSecretReminderDTO(409-414)
backend/src/server/routes/v1/dashboard-router.ts (1)
backend/src/db/schemas/users.ts (1)
UsersSchema(10-31)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
🔇 Additional comments (17)
backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts (3)
13-13: Clean and appropriate import addition.The addition of
TSecretReminderRecipienttype import is necessary for the new parameter in thereshapeBridgeSecretfunction.
672-673: Appropriate type extension for the secret object parameter.The
secretReminderRecipientsoptional property is correctly added to the parameter type, allowing for this data to be passed to the reshaping function.
704-704: Well implemented default value for secretReminderRecipients.The implementation correctly handles the case where
secretReminderRecipientsis undefined by defaulting to an empty array, ensuring consistent output structure regardless of input.backend/src/server/routes/v1/dashboard-router.ts (1)
597-602: Well-structured schema for secretReminderRecipients.The schema correctly defines the structure of secret reminder recipients in the API response, including only the necessary user information (id, email, username) and the recipient ID.
backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts (6)
98-103: Correct LEFT JOIN implementation for reminder recipients.The LEFT JOIN operations correctly connect the SecretReminderRecipients and Users tables, allowing for the retrieval of recipient data along with secrets.
105-108: Comprehensive selection of required recipient fields.The code properly selects all necessary fields from the joined tables with appropriate aliases to avoid column name conflicts.
133-149: Well-structured child mapper for secretReminderRecipients.The mapper correctly transforms the raw database results into a structured object with nested user information, making the data easier to work with in the application.
637-643: Consistent JOIN implementation across query methods.The JOIN operations in findByFolderIds match those in findOne, ensuring consistent data retrieval across different query paths.
671-674: Consistent field selection across query methods.The selected fields match those in the findOne method, maintaining consistency in the returned data structure.
719-735: Consistent child mapper implementation.The mapper structure is consistent with the one in findOne, ensuring the same data transformation logic is applied across different query methods.
backend/src/services/secret/secret-queue.ts (7)
9-9: Good type safety improvement.Replacing string literals with the SecretType enum improves type safety and makes the code more maintainable.
57-57: Proper dependency integration.The TSecretReminderRecipientsDALFactory is correctly imported and added to the factory dependencies with appropriate method selections.
Also applies to: 114-117
189-190: Improved cleanup in removeSecretReminder.Deleting recipient records before stopping the reminder job ensures complete cleanup of all related data.
236-236: Appropriate function signature update.Adding the optional recipients parameter allows for specifying which users should receive the reminder.
315-320: Well-implemented recipients change detection.The code properly detects when recipients have been added or removed, triggering reminder updates only when necessary.
322-334: Improved type safety with SecretType enum.Using SecretType.Personal instead of string literals improves type safety and clarity.
1111-1112: Proper recipient data retrieval.Fetching recipients by secretId is correctly implemented, providing the necessary data for the email sending logic.
Description 📣
This PR adds support for specifying recipients for secret reminders. The previous behavior was that all members of the project would receive the reminder. To keep things inline with the existing implementation, the default behavior remains that all project members receive the reminder unless one or more recipients are specified.
Additionally I also realized that secret reminders have been broken for Secrets V2 since we rolled out Secrets V2 due to column renaming in the new secrets table. This has also been patched in this PR.
Type ✨
Summary by CodeRabbit
New Features
Bug Fixes
Documentation