feat: alerts #45
feat: alerts #45
Conversation
sweetr-dev
bot
added
the
large
WalkthroughThis pull request introduces a comprehensive alerts system for teams within the application. The changes span multiple components across the API, web interface, and GraphQL types. A new database migration creates an Changes
Possibly related PRs
✨ 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
CodeRabbit Configuration File (
|
sweetr-dev
bot
added
huge
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (27)
packages/graphql-types/frontend/gql.ts (1)
16-18: LGTM! Consider adding error handling fields.The new GraphQL operations are well-structured and follow a consistent pattern. Each operation is focused and returns the necessary fields.
Consider adding error handling fields like
errorsto the mutation response to handle potential failures gracefully:mutation UpdateAlert($input: UpdateAlertInput!) { updateAlert(input: $input) { type enabled + errors { + code + message + } } }apps/web/src/providers/date.provider.ts (1)
19-25: Add JSDoc comment to document the constant's purpose.The
weekDaysconstant is well-structured and appropriately includes only business days. Consider adding a JSDoc comment to document its purpose and usage.+/** + * Array of business days (Monday through Friday) for configuring alert schedules. + */ export const weekDays = [ DayOfTheWeek.MONDAY, DayOfTheWeek.TUESDAY, DayOfTheWeek.WEDNESDAY, DayOfTheWeek.THURSDAY, DayOfTheWeek.FRIDAY, ];apps/web/src/providers/form.provider.ts.ts (1)
10-14: Consider enhancing the hook with additional features.Since this hook is used in alert management components, consider adding:
- Error handling for invalid form values
- Loading state management
- Debouncing for frequent updates
Would you like me to provide an example implementation with these enhancements?
apps/api/src/app/alerts/resolvers/mutations/update-alert.mutation.ts (1)
14-19: Validate additional fields if needed.
Although the channel field is validated for length, consider adding validations for other fields in the input object (e.g.,type,teamId) if you expect constraints. This ensures consistent data quality and prevents invalid states.apps/web/src/components/badge-on-off/badge-on-off.tsx (2)
3-6: Add JSDoc comments to document the component props.Consider adding documentation to improve component usability:
+/** + * Props for the BadgeOnOff component + * @property {boolean} enabled - Whether the feature is currently enabled + * @property {boolean} available - Whether the feature is available for use + */ interface BadgeOnOffProps extends BadgeProps { enabled: boolean; available: boolean; }
8-34: Consider enhancing accessibility with ARIA attributes.The component could benefit from improved accessibility:
export const BadgeOnOff = ({ enabled, available, ...props }: BadgeOnOffProps) => { if (!available) { return ( - <Badge color="gray" variant="light" {...props}> + <Badge + color="gray" + variant="light" + role="status" + aria-label="Feature coming soon" + {...props} + > Coming Soon </Badge> ); } if (enabled) { return ( - <Badge color="green" variant="light" {...props}> + <Badge + color="green" + variant="light" + role="status" + aria-label="Feature enabled" + {...props} + > On </Badge> ); } return ( - <Badge color="violet" variant="light" {...props}> + <Badge + color="violet" + variant="light" + role="status" + aria-label="Feature disabled" + {...props} + > Off </Badge> ); };apps/api/src/app/alerts/resolvers/alerts.schema.ts (2)
2-8: Add descriptions to AlertType enum values.Consider documenting the purpose of each alert type:
enum AlertType { + """ + Alert for pull requests that have been in review for longer than the configured threshold + """ SLOW_REVIEW + """ + Alert for approved pull requests that haven't been merged within the configured timeframe + """ SLOW_MERGE + """ + Alert when pull requests are merged without the required number of approvals + """ MERGED_WITHOUT_APPROVAL + """ + Alert for pull requests with high activity or urgent labels + """ HOT_PR + """ + Alert for changes that have been merged but not released to production + """ UNRELEASED_CHANGES }
10-15: Consider using stricter types for settings instead of JSONObject.The
settingsfield usingJSONObjectcould lead to runtime errors if not properly validated.Consider using type-specific settings:
type Alert { type: AlertType! enabled: Boolean! channel: String! - settings: JSONObject! + settings: AlertSettings! } + + """ + Union type for alert-specific settings + """ + union AlertSettings = + SlowReviewSettings | + SlowMergeSettings | + MergedWithoutApprovalSettings | + HotPRSettings | + UnreleasedChangesSettings + + type SlowReviewSettings { + thresholdHours: Int! + } + + type SlowMergeSettings { + thresholdHours: Int! + } + + type MergedWithoutApprovalSettings { + requiredApprovals: Int! + } + + type HotPRSettings { + urgentLabels: [String!]! + commentThreshold: Int! + } + + type UnreleasedChangesSettings { + thresholdHours: Int! + }apps/web/src/app/teams/[id]/alerts/use-alerts.tsx (1)
23-26: Add error handling for the alerts queryThe hook currently doesn't handle query errors. Consider adding error handling to provide better feedback to consumers of this hook.
const { data, isLoading } = useTeamAlertsQuery({ workspaceId: workspace.id, teamId, }); +const { data, isLoading, error } = useTeamAlertsQuery({ + workspaceId: workspace.id, + teamId, +}); + +if (error) { + console.error('Failed to fetch team alerts:', error); +} const alerts: Record<AlertType, Alert> | undefined = objectify( data?.workspace.team?.alerts || [], (i) => i.type, ); return { alerts, isLoading, + error, };apps/web/src/app/teams/[id]/alerts/settings/types.ts (1)
16-29: Simplify alert type exportsThe current implementation has redundant type exports. Since all alert types use the same base schema, we can simplify this by using type aliases.
-export const FormSlowMergeAlert = FormAlert; -export type FormSlowMergeAlert = FormAlert; - -export const FormSlowReviewAlert = FormAlert; -export type FormSlowReviewAlert = FormAlert; - -export const FormMergedWithoutReviewAlert = FormAlert; -export type FormMergedWithoutReviewAlert = FormAlert; - -export const FormHotPrAlert = FormAlert; -export type FormHotPrAlert = FormAlert; - -export const FormUnreleasedChangesAlert = FormAlert; -export type FormUnreleasedChangesAlert = FormAlert; +// Alert type aliases +export type { + FormAlert as FormSlowMergeAlert, + FormAlert as FormSlowReviewAlert, + FormAlert as FormMergedWithoutReviewAlert, + FormAlert as FormHotPrAlert, + FormAlert as FormUnreleasedChangesAlert, +};apps/web/src/components/card-openable-settings/card-openable-settings.tsx (2)
39-48: Enhance accessibility for interactive elementsThe anchor element should have an aria-label to improve accessibility for screen readers.
return href ? ( <Anchor component={Link} to={href} underline="never" c="var(--mantine-color-text)" + aria-label={`Open ${title} settings`} > {content} </Anchor>
22-37: Consider adding keyboard interaction supportThe card should support keyboard focus and interaction when it's clickable.
const content = ( - <Paper p="md" withBorder className={href ? "grow-on-hover" : ""}> + <Paper + p="md" + withBorder + className={href ? "grow-on-hover" : ""} + tabIndex={href ? 0 : undefined} + role={href ? "button" : undefined} + > <Group justify="space-between" wrap="nowrap">apps/api/src/app/alerts/services/alert.types.ts (2)
10-12: Consider using a more specific type for settings.Instead of using the generic
AlertSettingstype alias forPrisma.JsonObject, consider creating specific interfaces for each alert type's settings to improve type safety and developer experience.Example:
interface SlowMergeSettings { threshold: number; // in hours excludedLabels?: string[]; } interface SlowReviewSettings { threshold: number; // in hours minReviewers: number; } export type AlertSettings = { [AlertType.SLOW_MERGE]: SlowMergeSettings; [AlertType.SLOW_REVIEW]: SlowReviewSettings; // ... other alert types }[AlertType];
45-68: Reduce code duplication in alert interfaces.The alert interfaces share the same structure with only the type and settings varying. Consider using a generic type to reduce duplication.
type AlertInterface<T extends AlertType> = Omit<Alert, "settings"> & { type: T; settings: Prisma.JsonObject; }; export type AlertSlowMerge = AlertInterface<typeof AlertType.SLOW_MERGE>; export type AlertSlowReview = AlertInterface<typeof AlertType.SLOW_REVIEW>; // ... other alert typesapps/web/src/app/teams/[id]/alerts/settings/use-alert.tsx (2)
77-78: Optimize form validation.The current implementation runs validation on every render. Consider using
useCallbackand adding proper dependencies.-// eslint-disable-next-line react-hooks/exhaustive-deps -const isFormValid = useMemo(() => !form.validate().hasErrors, [form.values]); +const isFormValid = useMemo( + () => !form.validate().hasErrors, + [form.values, form.validate] +);
84-96: Consider memoizing the return object.The return object is recreated on every render. Consider using
useMemoto optimize performance.-return { +return useMemo(() => ({ alert: { ...alertCard, ...data?.workspace.team?.alert, }, query, mutate, mutation, isUpdating: mutation.isPending, handleUpdate, isFormValid, drawerProps, -}; +}), [alertCard, data, query, mutate, mutation, isUpdating, handleUpdate, isFormValid, drawerProps]);apps/web/src/api/alert.api.ts (1)
75-102: Consider using a more specific error type.The mutation implementation is good, but consider replacing
unknownwith a more specific error type to improve error handling.export const useUpdateAlertMutation = ( options?: UseMutationOptions< UpdateAlertMutation, - unknown, + Error, MutationUpdateAlertArgs, unknown >, ) =>apps/web/src/app/teams/[id]/alerts/settings/slow-merge/page.tsx (1)
83-87: Remove unnecessary fragment.The fragment wrapping
AlertBaseFieldsis redundant as it contains only one child.{integration && ( - <> <AlertBaseFields form={form} /> - </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx (1)
83-87: Remove unnecessary fragment.The fragment wrapping
AlertBaseFieldsis redundant as it contains only one child.{integration && ( - <> <AlertBaseFields form={form} /> - </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx (1)
83-87: Remove unnecessary Fragment wrapper.The Fragment wrapper is redundant as it contains only one child component.
- {integration && ( - <> - <AlertBaseFields form={form} /> - </> - )} + {integration && <AlertBaseFields form={form} />}🧰 Tools
🪛 Biome (1.9.4)
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/alert-base-fields.tsx (1)
55-60: Add pattern validation for Slack channel names.Consider adding pattern validation to ensure valid Slack channel names (lowercase letters, numbers, hyphens, and underscores only).
<Input ref={channelRef} leftSection={<>#</>} flex="1" + pattern="^[a-z0-9-_]+$" {...form.getInputProps("channel")} />apps/web/src/app/teams/[id]/alerts/page.tsx (1)
46-53: Simplify boolean expression.The enabled prop's boolean expression can be simplified.
- enabled={ - (alerts?.[alert.type]?.enabled && !!integration) || - false - } + enabled={!!alerts?.[alert.type]?.enabled && !!integration}packages/graphql-types/api.ts (3)
28-46: Consider improving type safety for the settings field.While the types are well-defined, the
settingsfield uses a genericJSONObjecttype. Consider creating specific types for each alert type's settings to improve type safety and developer experience.Example implementation:
type AlertSettings = { SLOW_REVIEW: { threshold: number; // other settings specific to slow review alerts }; SLOW_MERGE: { threshold: number; // other settings specific to slow merge alerts }; // ... other alert type settings } export type Alert = { __typename?: 'Alert'; channel: Scalars['String']['output']; enabled: Scalars['Boolean']['output']; settings: AlertSettings[keyof AlertSettings]; type: AlertType; };
563-564: Add documentation for the alert fields.Consider adding documentation comments to explain:
- The difference between
alertandalertsfields- The purpose of the
AlertQueryInputinTeamAlertArgsExample:
export type Team = { /** Get a specific alert by type */ alert?: Maybe<Alert>; /** Get all alerts for the team */ alerts: Array<Alert>; // ... other fields }; export type TeamAlertArgs = { /** Input to query a specific alert by type */ input: AlertQueryInput; };Also applies to: 578-580
626-633: Consider adding input validation.The
UpdateAlertInputtype could benefit from runtime validation to ensure:
- Valid channel format
- Valid settings structure for each alert type
Consider using a validation library like Zod:
import { z } from 'zod'; const UpdateAlertInputSchema = z.object({ channel: z.string().min(1), enabled: z.boolean(), settings: z.record(z.unknown()), // Define specific schemas per alert type teamId: z.number().int().positive(), type: z.nativeEnum(AlertType), workspaceId: z.number().int().positive(), });apps/api/prisma/migrations/20250201041204_create_alerts_table/migration.sql (1)
4-17: Consider adding foreign key constraints.The table structure is good, but consider adding foreign key constraints for
teamIdandworkspaceIdto ensure referential integrity.Add these constraints:
ALTER TABLE "Alert" ADD CONSTRAINT "Alert_teamId_fkey" FOREIGN KEY ("teamId") REFERENCES "Team"("id") ON DELETE CASCADE; ALTER TABLE "Alert" ADD CONSTRAINT "Alert_workspaceId_fkey" FOREIGN KEY ("workspaceId") REFERENCES "Workspace"("id") ON DELETE CASCADE;apps/api/prisma/schema.prisma (1)
476-495: Add documentation comments to the Alert model.Consider adding documentation to explain the purpose of each field and any important constraints.
Example:
/// Represents an alert configuration for a team model Alert { /// Unique identifier for the alert id Int @id @default(autoincrement()) /// Type of the alert type AlertType /// Whether the alert is active enabled Boolean /// Slack channel where alerts will be sent channel String /// Alert-type specific configuration settings Json /// The team this alert belongs to teamId Int team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) /// The workspace this alert belongs to workspace Workspace @relation(fields: [workspaceId], references: [id], onDelete: Cascade) workspaceId Int createdAt DateTime @default(now()) updatedAt DateTime @default(now()) @updatedAt @@unique([teamId, type]) @@index([workspaceId]) @@index([teamId]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
apps/api/prisma/migrations/20250201041204_create_alerts_table/migration.sql(1 hunks)apps/api/prisma/migrations/migration_lock.toml(1 hunks)apps/api/prisma/schema.prisma(4 hunks)apps/api/src/app/alerts/resolvers/alerts.schema.ts(1 hunks)apps/api/src/app/alerts/resolvers/mutations/update-alert.mutation.ts(1 hunks)apps/api/src/app/alerts/resolvers/queries/team-alerts.query.ts(1 hunks)apps/api/src/app/alerts/resolvers/transformers/alert.transformer.ts(1 hunks)apps/api/src/app/alerts/services/alert.service.ts(1 hunks)apps/api/src/app/alerts/services/alert.types.ts(1 hunks)apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts(1 hunks)apps/api/src/app/digests/resolvers/queries/team-digests.query.ts(1 hunks)apps/api/src/app/digests/resolvers/transformers/digest.transformer.ts(1 hunks)apps/api/src/app/integrations/resolvers/mutations/remove-integration.mutation.ts(1 hunks)apps/api/src/app/teams/resolvers/transformers/team.transformer.ts(1 hunks)apps/web/src/api/alert.api.ts(1 hunks)apps/web/src/app/automations/settings/pr-size-labeler/page.tsx(1 hunks)apps/web/src/app/automations/settings/pr-title-check/page.tsx(1 hunks)apps/web/src/app/settings/components/card-openable-settings/card-openable-settings.tsx(0 hunks)apps/web/src/app/settings/pull-request-settings/page.tsx(2 hunks)apps/web/src/app/settings/pull-request-settings/size/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/alert-base-fields.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/index.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/components/alert-loading-skeleton/alert-loading-skeleton.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/components/alert-loading-skeleton/index.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/slow-merge/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/types.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/use-alert.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/use-alert-cards.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/use-alerts.tsx(1 hunks)apps/web/src/app/teams/[id]/components/subnav-team/subnav-team.tsx(0 hunks)apps/web/src/app/teams/[id]/digests/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/index.ts(0 hunks)apps/web/src/app/teams/[id]/digests/settings/components/digest-loading-skeleton/index.ts(1 hunks)apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx(2 hunks)apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx(2 hunks)apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx(2 hunks)apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx(1 hunks)apps/web/src/components/badge-on-off/badge-on-off.tsx(1 hunks)apps/web/src/components/badge-on-off/index.ts(1 hunks)apps/web/src/components/card-openable-settings/card-openable-settings.tsx(1 hunks)apps/web/src/providers/date.provider.ts(2 hunks)apps/web/src/providers/form.provider.ts.ts(1 hunks)apps/web/src/routes.tsx(3 hunks)packages/graphql-types/api.ts(14 hunks)packages/graphql-types/frontend/gql.ts(2 hunks)packages/graphql-types/frontend/graphql.ts(8 hunks)
💤 Files with no reviewable changes (3)
- apps/web/src/app/teams/[id]/components/subnav-team/subnav-team.tsx
- apps/web/src/app/settings/components/card-openable-settings/card-openable-settings.tsx
- apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/index.ts
✅ Files skipped from review due to trivial changes (8)
- apps/api/prisma/migrations/migration_lock.toml
- apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/index.ts
- apps/web/src/app/teams/[id]/digests/settings/components/digest-loading-skeleton/index.ts
- apps/web/src/components/badge-on-off/index.ts
- apps/api/src/app/integrations/resolvers/mutations/remove-integration.mutation.ts
- apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx
- apps/api/src/app/digests/resolvers/transformers/digest.transformer.ts
- apps/web/src/app/teams/[id]/alerts/components/alert-loading-skeleton/index.ts
🧰 Additional context used
📓 Learnings (2)
apps/web/src/app/automations/settings/pr-size-labeler/page.tsx (1)
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#33
File: apps/web/src/app/automations/settings/pr-size-labeler/page.tsx:57-57
Timestamp: 2024-12-01T02:32:45.503Z
Learning: In the `AutomationPrSizeLabelerPage` component (`apps/web/src/app/automations/settings/pr-size-labeler/page.tsx`), when initializing the form, the `repositories` array should be reset to an empty array when saving new label settings.
apps/web/src/app/automations/settings/pr-title-check/page.tsx (1)
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#27
File: apps/web/src/app/automations/settings/pr-title-check/page.tsx:0-0
Timestamp: 2024-11-12T14:50:04.629Z
Learning: When a `useEffect` hook updates the `form` object, including `form` in the dependency array can cause an infinite loop. In such cases, it's acceptable to disable the `react-hooks/exhaustive-deps` ESLint rule.
🪛 Biome (1.9.4)
apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
apps/web/src/app/teams/[id]/alerts/settings/slow-merge/page.tsx
[error] 84-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (50)
packages/graphql-types/frontend/gql.ts (1)
73-84: LGTM! Type-safe implementation.The new function overloads are well-implemented, providing proper type safety and following the established pattern. The documentation is consistent with existing overloads.
apps/web/src/providers/date.provider.ts (1)
1-1: LGTM!The import of
DayOfTheWeekfrom the GraphQL types is appropriate for the newweekDaysconstant.apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx (3)
16-23: LGTM!The imports are well-organized and correctly reference the required components and utilities.
Line range hint
47-55: LGTM!The form initialization with default values is well-structured, and the
weekDaysconstant is appropriately used as the default value fordayOfTheWeek.
69-69: LGTM!The
resetDirtycall is correctly placed aftersetValuesto ensure proper form state management.apps/web/src/providers/form.provider.ts.ts (1)
1-8: LGTM! Well-structured type definitions.The interface and imports are well-defined, providing good type safety and reusability through generics.
apps/api/src/app/alerts/resolvers/mutations/update-alert.mutation.ts (1)
1-9: Good use of structured imports.
All imported utilities are clear and purposeful, matching their usage in subsequent lines.packages/graphql-types/frontend/graphql.ts (12)
26-32: Alert type looks good.
This type definition is clear and well-structured.
34-36: Input shape is minimal but sufficient.
This input structure is straightforward. Ensure your backend also supports future optional fields if they’re foreseen.
38-44: Enum definitions are comprehensive.
AlertType enumerates relevant cases. Great for preventing invalid alert types.
561-562: Team fields for alerts are coherent.
Having bothalertandalertscan be helpful for querying single or multiple alerts. This approach is flexible.
576-578: TeamAlertArgs usage clarity.
It’s clear how this argument object is used to retrieve a single alert by type.
624-631: UpdateAlertInput structure aligns well with backend.
All essential fields (channel, settings, etc.) are included. Ensure the shapes remain consistent if changes occur on the backend.
777-784: TeamAlertsQuery captures array retrieval.
The query effectively fetches multiple alerts. No issues found.
785-793: TeamAlertQuery is succinct.
This query adequately retrieves a single alert by type and is structured consistently with your schema.
794-800: UpdateAlertMutation matches the server mutation.
These types accurately reflect theupdateAlertoperation.
1086-1086: TeamAlertsDocument generation is correct.
This auto-generated document appears correct and aligns with the TeamAlerts query.
1087-1087: TeamAlertDocument generation is correct.
This auto-generated document follows the single alert query signature.
1088-1088: UpdateAlertDocument generation is correct.
This mutation document is consistent with your codebase’s codegen approach.apps/web/src/app/teams/[id]/alerts/components/alert-loading-skeleton/alert-loading-skeleton.tsx (2)
1-2: Imports are straightforward.
Using Mantine’sSkeletonandStackis appropriate for a loading skeleton component.
3-12: Component structure is consistent.
TheAlertLoadingSkeletonprovides multiple skeleton lines of varying heights, offering good visual placeholders. Consider additional skeleton variations or theming if you have multiple alert UIs.apps/api/src/app/teams/resolvers/transformers/team.transformer.ts (1)
10-10: LGTM! Initializing alerts array.The empty array initialization for alerts is correct and matches the GraphQL schema definition.
apps/web/src/app/settings/pull-request-settings/page.tsx (1)
20-26: LGTM!The changes correctly adapt to the new CardOpenableSettings API and maintain consistent styling.
apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts (1)
8-8: LGTM! Improved naming and import path.The changes improve code organization:
- The export name
updateDigestMutationis more descriptive and consistent with its purpose- The import path for
transformDigesthas been updated to reflect the new directory structureAlso applies to: 10-10
apps/api/src/app/digests/resolvers/queries/team-digests.query.ts (1)
9-9: LGTM! Consistent import path update.The import path for
transformDigesthas been updated to maintain consistency with the new directory structure.apps/api/src/app/alerts/resolvers/queries/team-alerts.query.ts (2)
11-32: LGTM! Well-structured alert query implementation.The
alertfield resolver implementation:
- Has proper error handling for missing team and workspace IDs
- Follows the same pattern as the digest queries
- Correctly transforms the alert data
33-47: LGTM! Well-structured alerts list query implementation.The
alertsfield resolver implementation:
- Has proper error handling for missing team and workspace IDs
- Efficiently maps the transform function over the alerts array
apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx (1)
45-45: LGTM! Improved hook naming and data structure.The changes improve code quality:
- The hook name
useDigestCardsis grammatically correct (singular form)- Removal of the
enabledproperty aligns with the new alert management systemapps/web/src/app/teams/[id]/digests/page.tsx (1)
4-4: LGTM!The changes are straightforward and correct:
- Updated import path for AlertEnableSlack component
- Renamed hook from useDigestsCards to useDigestCards
- Updated hook usage accordingly
Also applies to: 8-8, 13-13
apps/web/src/app/teams/[id]/alerts/use-alert-cards.tsx (2)
12-68: LGTM! Well-structured alert card definitions.The
AlertCardDatainterface andalertCardsobject provide a clear and maintainable way to define alert types. The descriptions are informative, and the route generation is consistent.
78-89: LGTM! Clean and efficient implementation.The
useAlertCardshook effectively uses theforkutility to separate available and future alerts, making it easy to display them differently in the UI.apps/web/src/api/alert.api.ts (2)
19-42: LGTM! Well-structured query implementation.The
useTeamAlertsQueryhook follows React Query best practices with proper query keys and GraphQL query structure.
44-73: LGTM! Consistent query implementation.The
useTeamAlertQueryhook maintains consistency with the team alerts query while adding specific fields for individual alerts.apps/web/src/app/settings/pull-request-settings/size/page.tsx (1)
51-51: LGTM! Good practice to reset form state.The addition of
form.resetDirty()ensures that the form's dirty state is properly reset after setting initial values from workspace settings.apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx (1)
19-22: LGTM! Well-structured form initialization and alert handling.The form setup with initial values and validation is well-implemented. Good use of the
useAlerthook for managing alert state.Also applies to: 24-41
apps/web/src/app/automations/settings/pr-title-check/page.tsx (1)
39-39: LGTM! Consistent form state management.The addition of
form.resetDirty()maintains consistency with other form handling patterns in the codebase.apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/alert-base-fields.tsx (1)
23-96: LGTM! Well-structured reusable component.The component is well-implemented with good error handling, user feedback, and focus management. The conditional rendering and form integration are properly handled.
apps/web/src/app/automations/settings/pr-size-labeler/page.tsx (1)
60-60: LGTM! Good form state management.The addition of
form.resetDirty()ensures proper form state management by resetting the dirty state after loading initial values.apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx (1)
66-66: LGTM! Consistent form state management.The addition of
form.resetDirty()maintains consistency with other form components in the codebase.apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (1)
97-103: LGTM! Improved UX with conditional rendering.Good improvement to show the Slack channel tip only when the digest is enabled, making the UI cleaner and more contextual.
apps/web/src/routes.tsx (2)
24-24: LGTM!The imports are correctly defined and follow the project's import pattern.
Also applies to: 26-29, 51-51
185-202: LGTM!The route configuration is well-structured and follows RESTful patterns. The nested routes for different alert types are organized logically.
packages/graphql-types/api.ts (2)
272-272: LGTM!The
updateAlertmutation is correctly defined and follows the pattern of other mutations in the file.
315-319: LGTM!The
MutationUpdateAlertArgstype is correctly defined and follows the pattern of other mutation argument types.apps/api/prisma/migrations/20250201041204_create_alerts_table/migration.sql (3)
1-2: LGTM!The
AlertTypeenum is correctly defined and matches the GraphQL types.
19-26: LGTM!The indexes are well-chosen for query optimization:
Alert_workspaceId_idxfor filtering by workspaceAlert_teamId_idxfor filtering by teamAlert_teamId_type_keyensures uniqueness of alert types per team
28-32: LGTM!The Row Level Security (RLS) configuration follows security best practices:
- Forces RLS to prevent accidental exposure
- Implements tenant isolation by workspace
- Provides a controlled bypass mechanism
apps/api/prisma/schema.prisma (3)
68-74: LGTM!The
AlertTypeenum definition is consistent with both the migration and GraphQL types.
181-181: LGTM!The
Alertrelation is correctly added to theWorkspacemodel.
422-422: LGTM!The
Alertrelation is correctly added to theTeammodel.
apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/web/src/app/teams/[id]/alerts/settings/use-alert.tsx (1)
72-77:⚠️ Potential issueAdd settings validation before mutation.
The mutation is sending form values directly without validating or merging settings, which could override existing settings.
Apply this diff to merge with existing settings:
await mutate({ ...form.values, teamId, type, + settings: { + ...data?.workspace.team?.alert?.settings, + ...form.values.settings, + }, workspaceId: workspace.id, });
🧹 Nitpick comments (39)
apps/api/src/app/alerts/services/alert-slow-merge.types.ts (1)
8-12: Add JSDoc documentation for better code maintainability.While the type definition is well-structured, adding documentation would improve maintainability and help other developers understand its purpose in the alerts system.
Consider adding JSDoc documentation:
+/** + * Represents a Pull Request with its related entities for alert processing. + * Used in slow merge alert detection to access PR details and tracking information. + */ export type PullRequestWithRelations = PullRequest & { author: GitProfile; repository: Repository; tracking: PullRequestTracking | null; };apps/web/src/app/teams/[id]/alerts/settings/components/trigger-description/trigger-description.tsx (1)
9-30: Consider these improvements for better maintainability and accessibility.The component implementation is solid, but could benefit from these enhancements:
export const TriggerDescription = ({ type, label, }: TriggerDescriptionProps) => { + const icon = type === "cron" ? ( + <IconCalendar stroke={1.5} size={20} aria-hidden="true" /> + ) : ( + <IconWebhook stroke={1.5} size={20} aria-hidden="true" /> + ); + + const prefix = type === "cron" ? "Runs checks" : "Runs when"; + return ( - <Paper withBorder p="xs"> + <Paper withBorder p="xs" role="status"> <Group gap="xs" align="center"> - {type === "cron" && <IconCalendar stroke={1.5} size={20} />} - {type === "event" && <IconWebhook stroke={1.5} size={20} />} + {icon} <div> <Text fw={500}> - {type === "cron" ? "Cron-based: " : "Event-based: "} - <Text component="span"> - {type === "cron" ? "Runs checks " : "Runs when "} - {label}. - </Text> + <Text component="span" fw={500}>{type === "cron" ? "Cron-based: " : "Event-based: "}</Text> + {prefix} {label} </Text> </div> </Group> </Paper> ); };Changes:
- Extracted icon logic to improve readability
- Added
aria-hidden="true"to icons as they're decorative- Added
role="status"for better accessibility- Simplified text structure to reduce nesting
- Extracted text prefix logic for better maintainability
apps/api/src/app/automations/sweets/pr-title-check/pr-title-check.service.ts (1)
76-89: Consider optimizing the workspace retrieval.The function could be simplified by moving the null checks into the options object of
findWorkspaceByGitInstallationId.Here's a suggested optimization:
const findWorkspace = async (gitInstallationId: number) => { - const workspace = await findWorkspaceByGitInstallationId( - gitInstallationId.toString(), - { - subscription: true, - } - ); - - if (!workspace) return null; - - if (!workspace.gitProfile && !workspace.organization) return null; - - return workspace; + return findWorkspaceByGitInstallationId( + gitInstallationId.toString(), + { + subscription: true, + where: { + OR: [ + { gitProfile: { isNot: null } }, + { organization: { isNot: null } } + ] + } + } + ); };This change:
- Moves the profile/organization check into the query itself
- Reduces the number of lines while maintaining the same functionality
- Potentially improves performance by filtering at the database level
apps/api/src/app/workspace-authorization.service.ts (1)
34-36: Consider creating a type alias for better readability.The function signature change looks good. However, consider creating a type alias for
Workspace & { subscription?: Subscription | null }to improve readability and reusability.type WorkspaceWithSubscription = Workspace & { subscription?: Subscription | null }; export const isActiveCustomer = (workspace: WorkspaceWithSubscription) => {apps/web/src/app/teams/[id]/alerts/settings/types.ts (3)
3-15: Enhance channel validation and add documentation.Consider the following improvements to the base schema:
- Add more comprehensive channel validation (e.g., max length, allowed characters)
- Document the purpose of the empty settings object
export const BaseFormAlert = z.object({ enabled: z.boolean().default(false), channel: z .string() .min(1, "Field is required") + .max(80, "Channel name is too long") + .regex(/^[a-z0-9-_]+$/, "Channel can only contain lowercase letters, numbers, hyphens, and underscores") .refine((val) => !val.startsWith("#"), { - message: "Channel should not start with #", + message: "Please enter the channel name without the # prefix", }) .default(""), + // Base settings object that specific alert types can extend settings: z.object({}).default({}), });
17-22: Add documentation and improve error messages for maxWaitInHours.The validation looks good, but consider adding documentation and more descriptive error messages.
export const FormSlowMergeAlert = BaseFormAlert.extend({ settings: z.object({ - maxWaitInHours: z.number().min(1).max(168).default(1), + // Maximum time to wait before triggering a slow merge alert (1 hour to 1 week) + maxWaitInHours: z + .number() + .min(1, "Wait time must be at least 1 hour") + .max(168, "Wait time cannot exceed 1 week (168 hours)") + .default(1), }), });
24-29: Reduce duplication by extracting common validation logic.The maxWaitInHours validation is duplicated between FormSlowMergeAlert and FormSlowReviewAlert.
+// Common validation for time-based alerts +const timeBasedAlertSettings = z.object({ + maxWaitInHours: z + .number() + .min(1, "Wait time must be at least 1 hour") + .max(168, "Wait time cannot exceed 1 week (168 hours)") + .default(1), +}); + -export const FormSlowReviewAlert = BaseFormAlert.extend({ - settings: z.object({ - maxWaitInHours: z.number().min(1).max(168).default(1), - }), -}); +export const FormSlowReviewAlert = BaseFormAlert.extend({ + settings: timeBasedAlertSettings, +});Update FormSlowMergeAlert similarly to use the shared validation.
apps/api/src/app/alerts/services/alert.types.ts (1)
15-25: Consider clarifying data-loading assumptions for these relation types.
AlertWithRelationsandAlertWithTeamassume that theteamandworkspaceobjects are always loaded. If you rely on lazy loading or partial fetches, you might encounter runtime errors. Clarifying whether they are guaranteed to be loaded by the time these types are used could enhance maintainability.apps/api/src/app/alerts/services/alert-slow-merge.service.ts (3)
9-18: Review potential concurrency approach.
Processing each PR sequentially with a 100ms sleep can stall the flow if there are many pull requests. Consider processing them concurrently using Promise.all or a queue for better throughput.
20-38: Consider adding error handling around alert sending and event creation.
IfsendAlertoralertEvent.createfails, the process won't log or handle the exception. A try/catch block could help identify and recover from issues or log them for future debugging.
85-132: Optional fallback for missing data in Slack message blocks.
IfpullRequest.author.avatarorpullRequest.repositoryare missing/undefined, it could lead to incomplete messages. Consider adding a fallback or skipping those fields for a robust user experience.apps/api/src/app/alerts/services/alert-slow-review.service.ts (2)
9-18: Sequential processing may slow throughput.
Similar to slow-merge alerts, consider parallelizing or batching review alert dispatches if performance becomes a concern.
20-38: Add error logging or recovery strategy.
As with slow-merge alerts, any failure insendAlertor DB writes may remain silent. Implementing minimal error handling could aid troubleshooting.apps/api/src/app/alerts/services/alert-merged-without-approval.service.ts (5)
8-8: Consider leveraging built-in JavaScript utilities for uniqueness.The
radashlibrary is fine, but for a small set of alerts, a Set-based approach or inline uniqueness checks might suffice without adding an extra dependency.
22-25: Add explanatory logging when skipping inactive workspaces.When the customer is inactive, the function exits silently. Consider logging a debug statement to track skipped alert processes for troubleshooting and better observability.
36-38: Consider logging PR conditions to clarify early returns.If a PR is approved or not merged, the function returns without any alert. For transparency, adding a log can help future debugging or auditing of skipped alerts.
40-60: Handle partial Slack send failures gracefully.You are using
Promise.allto send multiple alerts concurrently. If one Slack API call fails, all promises may reject. Consider adding a try-catch block or a fallback to handle partial successes.
127-141: UsefindUniqueifgitPullRequestIdis unique.If your schema enforces uniqueness on
gitPullRequestId, you can replacefindFirstwithfindUniqueto be more explicit about the expected one-to-one mapping. It also communicates intent more clearly.apps/api/src/app/alerts/services/send-alert.types.ts (1)
3-8: Consider extending alert options for Slack integrability.Adding optional properties (e.g., thread IDs or ephemeral flag) might allow more advanced Slack usage, such as threaded discussions or ephemeral messages, without changing the interface later.
apps/api/src/app/alerts/services/alert-slow-review.types.ts (1)
8-12: Extract common PR relationship types into a shared module.This
PullRequestWithRelationstype appears similar to the one inalert-merged-without-approval.types.ts. Consider consolidating them in a single, shared definition to avoid duplication and enhance maintainability.apps/api/src/app/alerts/workers/alert-slow-merge.worker.ts (1)
8-15: Add error handling for alert processing.While the worker implementation is good, consider adding error handling for the
processSlowMergeAlertcall to ensure failed alerts are properly logged and handled.export const alertSlowMergeWorker = createWorker( SweetQueue.ALERT_SLOW_MERGE, async ({ data: alert }: Job<AlertWithRelations<"SLOW_MERGE">>) => { logger.info("alertSlowMergeWorker", { alert }); - await processSlowMergeAlert(alert); + try { + await processSlowMergeAlert(alert); + } catch (error) { + logger.error("Failed to process slow merge alert", { error, alert }); + throw error; // Re-throw to let BullMQ handle the failure + } } );apps/api/src/app/alerts/services/send-alert.service.ts (1)
9-30: Consider enhancing error handling and adding retry mechanism.While the implementation is solid, consider these improvements:
- Add specific error handling for Slack API failures.
- Implement a retry mechanism for transient Slack API issues.
- Consider logging failed attempts for monitoring.
Here's a suggested implementation:
export const sendAlert = async ({ workspaceId, channel, blocks, text, }: SendAlertArgs) => { + const maxRetries = 3; + let attempt = 0; + + while (attempt < maxRetries) { try { const { slackClient } = await getWorkspaceSlackClient(workspaceId); const slackChannel = await joinSlackChannelOrThrow(slackClient, channel); if (!slackChannel?.id) { throw new ResourceNotFoundException("Slack channel not found"); } await sendSlackMessage(slackClient, { channel: slackChannel.id, blocks, text, unfurl_links: false, unfurl_media: false, }); + return; + } catch (error) { + attempt++; + if (error instanceof ResourceNotFoundException || attempt === maxRetries) { + throw error; + } + logger.warn(`Failed to send alert (attempt ${attempt}/${maxRetries}):`, error); + await new Promise(resolve => setTimeout(resolve, 1000 * attempt)); } + } };apps/api/src/app/alerts/workers/cron-schedule-alerts.worker.ts (1)
8-26: Enhance error handling and add operational logging.The worker implementation could benefit from:
- Error handling for job failures
- Logging of processed alert counts
- Batch size limit for processing alerts
Here's a suggested implementation:
export const cronAlertScheduleWorker = createWorker( SweetQueue.CRON_SCHEDULE_ALERTS, async () => { - logger.info("cronAlertScheduleWorker"); + try { + logger.info("Starting cronAlertScheduleWorker"); - const alerts = await findScheduledAlerts([ - AlertType.SLOW_REVIEW, - AlertType.SLOW_MERGE, - ]); + const alerts = await findScheduledAlerts([ + AlertType.SLOW_REVIEW, + AlertType.SLOW_MERGE, + ]); - const [slowReviewAlerts, slowMergeAlerts] = fork( - alerts, - (alert) => alert.type === AlertType.SLOW_REVIEW - ); + const [slowReviewAlerts, slowMergeAlerts] = fork( + alerts, + (alert) => alert.type === AlertType.SLOW_REVIEW + ); - await addJobs(SweetQueue.ALERT_SLOW_REVIEW, slowReviewAlerts); - await addJobs(SweetQueue.ALERT_SLOW_MERGE, slowMergeAlerts); + logger.info(`Processing ${slowReviewAlerts.length} slow review alerts and ${slowMergeAlerts.length} slow merge alerts`); + + const BATCH_SIZE = 100; + for (let i = 0; i < slowReviewAlerts.length; i += BATCH_SIZE) { + await addJobs( + SweetQueue.ALERT_SLOW_REVIEW, + slowReviewAlerts.slice(i, i + BATCH_SIZE) + ); + } + + for (let i = 0; i < slowMergeAlerts.length; i += BATCH_SIZE) { + await addJobs( + SweetQueue.ALERT_SLOW_MERGE, + slowMergeAlerts.slice(i, i + BATCH_SIZE) + ); + } + + logger.info("Completed cronAlertScheduleWorker"); + } catch (error) { + logger.error("Error in cronAlertScheduleWorker:", error); + throw error; + } } );apps/api/src/app/alerts/services/alert-schedule.service.ts (2)
6-28: Consider adding pagination and improving type safety.The implementation could benefit from:
- Pagination for large result sets
- Removal of type assertion
- Batch processing capability
Here's a suggested implementation:
-export const findScheduledAlerts = async (types: AlertType[]) => { +export const findScheduledAlerts = async ( + types: AlertType[], + { take = 100, skip = 0 }: { take?: number; skip?: number } = {} +) => { const alerts = await getBypassRlsPrisma().alert.findMany({ where: { enabled: true, type: { in: types, }, }, + take, + skip, include: { team: true, workspace: { include: { subscription: true, installation: true, }, }, }, }); const sendableAlerts = alerts.filter(canSendAlert); - return sendableAlerts as AlertWithRelations<AlertType>[]; + return sendableAlerts; };
30-36: Consider adding more comprehensive validation.The validation logic could be enhanced with:
- Channel validation
- Rate limiting checks
- Alert settings validation
Here's a suggested implementation:
export const canSendAlert = (alert: AlertWithRelations<AlertType>): boolean => { + // Validate customer status if (!isActiveCustomer(alert.workspace)) return false; + // Validate installation if (!alert.workspace.installation) return false; + // Validate channel + if (!alert.settings?.channel) return false; + + // Validate rate limiting + const lastSentAt = alert.settings?.lastSentAt; + if (lastSentAt) { + const cooldownPeriod = 1000 * 60 * 15; // 15 minutes + if (Date.now() - new Date(lastSentAt).getTime() < cooldownPeriod) { + return false; + } + } + return true; };apps/api/src/app/alerts/workers/alert-merged-without-approval.worker.ts (1)
12-15: Enhance logging for better observability.Consider adding more context to the log entry such as the PR action and merge status.
logger.info("alertMergedWithoutApprovalWorker", { pullRequestId: job.data.pull_request?.node_id, installationId: job.data.installation?.id, + action: job.data.action, + merged: job.data.pull_request?.merged, });apps/api/src/app/workspaces/resolvers/queries/workspace.query.ts (1)
59-59: Simplify redundant null check in return statement.The workspace existence is already checked, making the null check in the return statement redundant.
- return workspace ? transformWorkspace(workspace) : null; + return transformWorkspace(workspace);apps/web/src/app/teams/[id]/alerts/settings/use-alert.tsx (1)
80-81: Consider memoizing form validation.The ESLint comment suggests disabling the exhaustive-deps rule, but we can improve this by properly memoizing the validation function.
Apply this diff to properly memoize the validation:
-// eslint-disable-next-line react-hooks/exhaustive-deps -const isFormValid = useMemo(() => !form.validate().hasErrors, [form.values]); +const validateForm = useCallback(() => form.validate(), [form]); +const isFormValid = useMemo(() => !validateForm().hasErrors, [validateForm]);apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/alert-base-fields.tsx (1)
28-33: Add cleanup to useEffect.The effect that focuses the channel input should be cleaned up to prevent memory leaks.
Apply this diff to add cleanup:
useEffect(() => { + const currentRef = channelRef.current; if (isEnabled && !form.values.channel) { - channelRef.current?.focus(); + currentRef?.focus(); } + return () => { + if (currentRef?.contains(document.activeElement)) { + (document.activeElement as HTMLElement).blur(); + } + }; }, [isEnabled, form.values.channel]);apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx (2)
20-24: Consider moving initial values to a constant.The initial values object should be moved outside the component to prevent recreation on each render.
Apply this diff to optimize the code:
+const INITIAL_VALUES = { + enabled: false, + channel: "", + settings: {}, +} as const; + export const MergedWithoutApprovalAlertPage = () => { const teamId = useTeamId(); const { integration, query: integrationQuery } = useMessagingIntegration(); - - const initialValues = { - enabled: false, - channel: "", - settings: {}, - };
90-94: Remove unnecessary Fragment.The Fragment wrapping
AlertBaseFieldsis redundant as it contains only one child.Apply this diff to simplify the code:
{integration && ( - <> - <AlertBaseFields form={form} /> - </> + <AlertBaseFields form={form} /> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 91-93: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/api/src/bull-mq/queues.ts (2)
63-67: Consider adjusting retry strategy for alerts.The default retry strategy with exponential backoff starting at 60 seconds might be too aggressive for alert-related jobs.
Consider adjusting the retry strategy based on the queue type:
defaultJobOptions: { attempts: 3, - backoff: { - type: "exponential", - delay: 60000, // 60 seconds - }, + backoff: (attemptsMade: number, type: string) => { + // Alerts should retry more quickly + if (type.startsWith('{alert.')) { + return Math.min(1000 * Math.pow(2, attemptsMade), 30000); // Max 30s + } + // Default exponential backoff + return 60000 * Math.pow(2, attemptsMade); // Starts at 60s + }, },
98-99: Improve logging for bulk jobs.The log message for bulk jobs could be more descriptive to aid in debugging.
Apply this diff to enhance logging:
- logger.info(`🐂✉️ BullMQ: Adding ${data.length} job to ${queueName}`); + logger.info(`🐂✉️ BullMQ: Adding ${data.length} jobs to ${queueName}`, { + queueName, + jobCount: data.length, + jobType: data[0]?.type || 'unknown', + });apps/api/src/app/workspaces/services/workspace.service.ts (1)
34-34: Enhance type safety of the include parameter.The
includeparameter usesPartial<Prisma.WorkspaceInclude>which allows for runtime errors if invalid relations are specified.Consider creating a custom type that only allows specific relations to be included:
type AllowedIncludes = Pick<Prisma.WorkspaceInclude, 'subscription' | 'organization' | 'installation' | 'gitProfile'>;apps/api/src/app/automations/services/automation.service.ts (1)
111-116: Consider standardizing the gitInstallationId type.The function converts a number to string for the API call. This type conversion could be avoided by standardizing the type of
gitInstallationIdacross the codebase.Consider:
- Using string type consistently for
gitInstallationId- Adding a type definition:
type GitInstallationId = stringpackages/graphql-types/frontend/graphql.ts (1)
26-44: LGTM! Well-structured type definitions for alerts.The type definitions for the alerts feature are comprehensive and well-organized. Consider adding JSDoc comments to document the purpose of each alert type and its settings.
Add documentation for the AlertType enum values:
export enum AlertType { + /** Alert for pull requests with high activity or importance */ HOT_PR = 'HOT_PR', + /** Alert for pull requests merged without required approvals */ MERGED_WITHOUT_APPROVAL = 'MERGED_WITHOUT_APPROVAL', + /** Alert for pull requests taking too long to merge */ SLOW_MERGE = 'SLOW_MERGE', + /** Alert for pull requests waiting too long for reviews */ SLOW_REVIEW = 'SLOW_REVIEW', + /** Alert for changes not yet released to production */ UNRELEASED_CHANGES = 'UNRELEASED_CHANGES' }apps/api/prisma/migrations/20250215202910_create_alert_event_table/migration.sql (1)
1-11: Table Creation: "AlertEvent" Definition
The table is defined with clear columns includingid,alertId,pullRequestId,workspaceId, and timestamp columns with millisecond precision. Consider whether you need an automatic mechanism (such as a trigger) to updateupdatedAton row modifications or if this is handled at the application level.apps/api/prisma/schema.prisma (2)
68-74: New EnumAlertType:
The introduction of theAlertTypeenum with valuesSLOW_REVIEW,SLOW_MERGE,MERGED_WITHOUT_APPROVAL,HOT_PR, andUNRELEASED_CHANGESclearly categorizes alert types. For enhanced clarity and future maintenance, consider adding inline comments or documentation specifying the context or conditions under which each value should be used.
478-500: New ModelAlert:
TheAlertmodel is designed with essential fields such astype,enabled,channel, andsettings, and properly establishes relations with bothTeamandWorkspace(with cascade on delete). To improve clarity, consider adding inline documentation on the expected formats and usage of thechannelandsettingsfields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
apps/api/prisma/migrations/20250215202910_create_alert_event_table/migration.sql(1 hunks)apps/api/prisma/schema.prisma(5 hunks)apps/api/src/app/alerts/resolvers/queries/team-alerts.query.ts(1 hunks)apps/api/src/app/alerts/services/alert-merged-without-approval.service.ts(1 hunks)apps/api/src/app/alerts/services/alert-merged-without-approval.types.ts(1 hunks)apps/api/src/app/alerts/services/alert-schedule.service.ts(1 hunks)apps/api/src/app/alerts/services/alert-slow-merge.service.ts(1 hunks)apps/api/src/app/alerts/services/alert-slow-merge.types.ts(1 hunks)apps/api/src/app/alerts/services/alert-slow-review.service.ts(1 hunks)apps/api/src/app/alerts/services/alert-slow-review.types.ts(1 hunks)apps/api/src/app/alerts/services/alert.service.ts(1 hunks)apps/api/src/app/alerts/services/alert.types.ts(1 hunks)apps/api/src/app/alerts/services/send-alert.service.ts(1 hunks)apps/api/src/app/alerts/services/send-alert.types.ts(1 hunks)apps/api/src/app/alerts/workers/alert-merged-without-approval.worker.ts(1 hunks)apps/api/src/app/alerts/workers/alert-slow-merge.worker.ts(1 hunks)apps/api/src/app/alerts/workers/alert-slow-review.worker.ts(1 hunks)apps/api/src/app/alerts/workers/cron-schedule-alerts.worker.ts(1 hunks)apps/api/src/app/automations/services/automation.service.ts(3 hunks)apps/api/src/app/automations/sweets/pr-title-check/pr-title-check.service.ts(2 hunks)apps/api/src/app/billing/resolvers/queries/is-active-customer.query.ts(1 hunks)apps/api/src/app/billing/services/billing.service.ts(1 hunks)apps/api/src/app/github/services/github-code-review.service.ts(3 hunks)apps/api/src/app/github/services/github-member.service.ts(2 hunks)apps/api/src/app/github/services/github-pull-request.service.ts(2 hunks)apps/api/src/app/github/services/github-repository-pull-requests.service.ts(2 hunks)apps/api/src/app/github/services/github-repository.service.ts(2 hunks)apps/api/src/app/github/workers/github-sync-pull-request.worker.ts(3 hunks)apps/api/src/app/integrations/slack/slack.router.ts(1 hunks)apps/api/src/app/workspace-authorization.service.ts(1 hunks)apps/api/src/app/workspaces/resolvers/queries/workspace.query.ts(1 hunks)apps/api/src/app/workspaces/services/workspace.service.ts(2 hunks)apps/api/src/bull-mq/queues.ts(2 hunks)apps/api/src/bull-mq/schedule-cron-jobs.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/components/alert-base-fields/alert-base-fields.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/components/trigger-description/index.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/components/trigger-description/trigger-description.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/slow-merge/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/types.ts(1 hunks)apps/web/src/app/teams/[id]/alerts/settings/use-alert.tsx(1 hunks)apps/web/src/app/teams/[id]/alerts/use-alert-cards.tsx(1 hunks)apps/web/src/components/badge-on-off/badge-on-off.tsx(1 hunks)packages/graphql-types/frontend/gql.ts(2 hunks)packages/graphql-types/frontend/graphql.ts(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/teams/[id]/alerts/settings/components/trigger-description/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api/src/app/alerts/resolvers/queries/team-alerts.query.ts
- apps/web/src/app/teams/[id]/alerts/use-alert-cards.tsx
- apps/web/src/app/teams/[id]/alerts/page.tsx
- apps/web/src/app/teams/[id]/alerts/settings/slow-review/page.tsx
- apps/web/src/components/badge-on-off/badge-on-off.tsx
- apps/web/src/app/teams/[id]/alerts/settings/slow-merge/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/src/app/teams/[id]/alerts/settings/merged-without-approval/page.tsx
[error] 91-93: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (42)
apps/api/src/app/alerts/services/alert-slow-merge.types.ts (1)
1-6: LGTM! Well-organized imports.The imports are clean, well-organized, and properly sourced from @prisma/client.
apps/web/src/app/teams/[id]/alerts/settings/components/trigger-description/trigger-description.tsx (2)
1-2: LGTM!Imports are well-organized and all imported components are utilized in the code.
4-7: LGTM!The interface is well-defined with appropriate types that match the component's usage.
apps/api/src/app/automations/sweets/pr-title-check/pr-title-check.service.ts (1)
1-1: LGTM! Import changes align with the refactoring.The imports have been correctly updated to reflect the new workspace retrieval approach.
Also applies to: 8-8
apps/api/src/app/billing/resolvers/queries/is-active-customer.query.ts (1)
19-19: LGTM! The change aligns with the updated function signature.The modification correctly reflects the refactored
isActiveCustomerfunction signature, which now expects the subscription data to be included within the workspace object.apps/api/src/app/workspace-authorization.service.ts (1)
41-41: LGTM! The conditional check is correctly updated.The conditional check has been properly updated to use
workspace.subscription, maintaining the same logic flow.apps/api/src/app/billing/services/billing.service.ts (1)
152-152: LGTM! The change aligns with the updated function signature.The modification correctly reflects the refactored
isActiveCustomerfunction signature, maintaining the same logic flow while simplifying the parameter passing.apps/web/src/app/teams/[id]/alerts/settings/types.ts (1)
31-38: Verify schema requirements for alerts without specific settings.Please confirm that FormMergedWithoutReviewAlert, FormHotPrAlert, and FormUnreleasedChangesAlert don't require any additional settings beyond the base schema.
Consider adding documentation to explain the purpose and behavior of each alert type:
+// Alert for PRs that were merged without required reviews export const FormMergedWithoutReviewAlert = BaseFormAlert; export type FormMergedWithoutReviewAlert = BaseFormAlert; +// Alert for PRs that require immediate attention export const FormHotPrAlert = BaseFormAlert; export type FormHotPrAlert = BaseFormAlert; +// Alert for changes that haven't been released to production export const FormUnreleasedChangesAlert = BaseFormAlert; export type FormUnreleasedChangesAlert = BaseFormAlert;apps/api/src/app/github/services/github-repository-pull-requests.service.ts (3)
8-8: LGTM!The import of
findWorkspaceByGitInstallationIdis well-placed and follows the existing import structure.
152-156: LGTM! Improved error handling.The new error handling is more robust and provides better context:
- Explicit null check for the workspace
- Clear error message with relevant context (gitInstallationId)
148-150:✅ Verification successful
Verify type consistency for gitInstallationId.
The conversion of
gitInstallationIdfromnumbertostringsuggests a potential type mismatch between the service and database layers.Let's check the type definitions and usage across the codebase:
🏁 Script executed:
#!/bin/bash # Search for gitInstallationId type definitions and usages ast-grep --pattern 'gitInstallationId: $_' rg -A 2 'gitInstallationId.*:.*' --type typescriptLength of output: 132
Manual Verification: Check
gitInstallationIdType ConsistencyThe conversion of
gitInstallationIdfrom a number to a string in the call tofindWorkspaceByGitInstallationIdsuggests there might be a type inconsistency between the service layer and the database layer. Please manually verify that the type declaration forgitInstallationIdis consistent throughout the codebase and that the conversion is intentional. Consider reviewing any associated type definitions or interfaces to confirm that this casting is the desired behavior.
- Check the type declaration for
gitInstallationIdin your domain models or interfaces.- Ensure that the database layer and any other consumers of this value are aligned with this conversion.
🏁 Script executed:
#!/bin/bash echo "Searching for explicit type definitions of gitInstallationId (TypeScript files):" rg -A 2 'gitInstallationId\s*:' --type ts echo "Searching for any occurrence of gitInstallationId:" rg -A 2 'gitInstallationId' .Length of output: 51624
Type consistency verified and error handling approved
- The review correctly highlights the conversion of
gitInstallationIdfrom a number to a string. Our search confirms that this conversion is a consistent pattern across the codebase—service layers use a numeric type while database queries (and GraphQL inputs) expect a string.- The explicit null check with the
ResourceNotFoundExceptionimproves robustness and clarity, and its implementation aligns with similar error handling in other modules.Please ensure that the intentional type conversion is well documented to help future maintainability.
apps/api/src/app/alerts/services/alert.types.ts (7)
1-9: Imports look fine.
No immediate concerns with the imported types and interfaces from Prisma.
11-13: Clever reuse of the DbAlert type using Omit.
This approach neatly redefinessettingswith a more precise type and maintains overall type safety.
27-37: Search parameters look consistent.
TheFindActiveAlertsandFindAlertByTypeArgsinterfaces are concise and clear.
39-46: UpsertAlert interface is sufficient for basic creation or update flows.
No major concerns; the shape aligns well with typical upsert usage.
48-54: [Duplicate] Consider adding runtime validation for workspace data.
This comment mirrors a previous suggestion about ensuring thatsubscriptionorinstallationis valid (or both). If either is mandatory in specific scenarios, add checks or refine these types to prevent null references.
56-62: AlertTypeMap indexing is consistent with the enumerated AlertTypes.
No issues. This mapping approach streamlines referencing each alert’s unique settings.
64-98: Solid explicit interfaces for various alert types.
Each alert interface clearly describes the shape of its unique settings, andAlertSettingsunifies them. This fosters clarity and consistency across different alert types.apps/api/src/app/alerts/services/alert-slow-merge.service.ts (2)
1-8: Initial imports are appropriate.
All dependencies appear relevant to handling slow-merge alerts.
40-83: ValidatemaxWaitInHoursedge cases.
IfmaxWaitInHoursis zero or negative, the logic to filter pull requests might not behave as intended. Also confirm that the 24-hour repeat logic matches all alerting requirements.apps/api/src/app/alerts/services/alert-slow-review.service.ts (3)
1-8: Imports are consistent with the slow-review alert workflow.
No concerns here.
40-104: Confirm logic forORclauses and partial reviews.
The condition that all code reviews must be older thanmaxWaitInHoursor none exist at all is sensible. Double-check how it interacts with theTODOcomment aboutfullyApprovedAt. Also ensure that the 24-hour rate limiting meets your product requirements.
106-154: Slack block content is comprehensive.
Information about the PR, repository, and authors is well structured. No obvious issues.apps/api/src/app/alerts/services/alert-merged-without-approval.service.ts (2)
62-109: Verify compliance when displaying user info.You are exposing user avatar URLs and names in Slack messages. Ensure that this data sharing is permitted by your privacy policy or other relevant compliance requirements.
111-121: Implementation looks correct.The
getWorkspaceOrThrowfunction follows good defensive programming practices by throwing an exception if the workspace is not found.apps/api/src/app/alerts/services/alert-merged-without-approval.types.ts (1)
1-13: LGTM! Well-structured type definition.The
PullRequestWithRelationstype is well-defined, properly extending the PrismaPullRequesttype with necessary relations and correct nullability.apps/api/src/app/alerts/workers/alert-slow-review.worker.ts (1)
8-15: Add error handling for alert processing.Similar to the slow merge worker, consider adding error handling for the
processSlowReviewAlertcall.apps/api/src/app/alerts/services/send-alert.service.ts (1)
1-8: LGTM! Well-organized imports and proper type usage.The imports are well-structured, with clear separation of error handling, Slack services, and type definitions.
apps/api/src/app/alerts/workers/cron-schedule-alerts.worker.ts (1)
1-7: LGTM! Well-organized imports.All necessary dependencies are properly imported with clear organization.
apps/api/src/bull-mq/schedule-cron-jobs.ts (1)
29-33: LGTM! Consistent implementation of alert scheduling.The new cron job follows the established pattern and uses an appropriate schedule interval.
apps/api/src/app/alerts/services/alert.service.ts (1)
62-110: Improve error handling and type safety in upsertAlert.The previous review comment about improving error handling and type safety is still applicable.
apps/api/src/app/github/services/github-member.service.ts (1)
123-125:❓ Verification inconclusive
Verify required workspace relations.
The function calls
findWorkspaceByGitInstallationIdwithout specifying any relations to include. This might cause issues if the code assumes certain relations are loaded.Run this script to check for potential missing relation access:
🏁 Script executed:
#!/bin/bash # Search for workspace property access in the function rg -A 5 "workspace\." "apps/api/src/app/github/services/github-member.service.ts"Length of output: 588
Action Required: Confirm Workspace Relation Inclusion
The code currently retrieves the workspace by calling
findWorkspaceByGitInstallationIdand then only usesworkspace.iddirectly. However, later operations (such as deleting workspace memberships) rely on variables likeworkspaceGitProfiles. Please verify that all necessary relations (e.g., the git profiles expected for membership management) are either included in the workspace object returned byfindWorkspaceByGitInstallationIdor are being loaded/handled elsewhere. If additional relational data is required, update the query accordingly or add a comment clarifying how these relations are populated.
- Location:
apps/api/src/app/github/services/github-member.service.ts(Lines 123-125)- Action: Review the implementation in
apps/api/src/app/workspaces/services/workspace.service.tsto ensure that required relations are properly loaded.apps/api/src/app/github/services/github-repository.service.ts (1)
134-141: LGTM! Well-structured null checks.The implementation properly handles:
- Conversion of gitInstallationId to string
- Explicit null checks for workspace and its relations
apps/api/src/app/github/services/github-code-review.service.ts (1)
7-8: LGTM! Good architectural improvement.The changes improve the code architecture by:
- Moving workspace retrieval logic to a dedicated service
- Removing direct Prisma access from this service
- Maintaining consistent workspace retrieval across services
This refactoring enhances maintainability and follows separation of concerns.
Also applies to: 23-23, 494-497
apps/api/src/app/github/services/github-pull-request.service.ts (1)
28-29: LGTM! Consistent refactoring approach.The changes maintain consistency with the refactoring pattern seen in
github-code-review.service.ts, improving the overall codebase architecture.Also applies to: 494-496
packages/graphql-types/frontend/gql.ts (1)
16-18: LGTM! Auto-generated GraphQL operations.The new GraphQL operations for team alerts are correctly generated and maintain consistency with the existing pattern.
apps/api/prisma/migrations/20250215202910_create_alert_event_table/migration.sql (4)
13-15: Index onworkspaceId:
The index"AlertEvent_workspaceId_idx"is properly defined to boost query performance for operations filtering byworkspaceId.
16-18: Index onpullRequestId:
Creating the"AlertEvent_pullRequestId_idx"index is a good step to enhance lookup speed for queries involvingpullRequestId.
19-21: Index onalertId:
The"AlertEvent_alertId_idx"index will help optimize queries filtering byalertIdand appears to be correctly implemented.
22-26: Row Level Security (RLS) Configuration:
Enabling and forcing RLS on the"AlertEvent"table and defining both thetenant_isolation_policy(which restricts access based onworkspaceIdvia a session setting) and thebypass_rls_policy(allowing an override) is a robust security measure. Make sure that the session settings (app.current_workspace_idandapp.bypass_rls) are reliably managed by the application.apps/api/prisma/schema.prisma (2)
179-182: Enhancements to theWorkspaceModel:
The updatedWorkspacemodel now includes relations toAlertandAlertEvent. This integration is key to supporting the comprehensive alerts system. Please verify that these relations are consistent with the migration scripts and that any application logic that references workspaces is updated accordingly.
501-519: New ModelAlertEvent:
TheAlertEventmodel appropriately logs events related to alerts by linking toAlertandPullRequestmodels, and the indexes onworkspaceId,pullRequestId, andalertIdwill aid performance. Ensure that the on-delete behaviors and data retention policies align with your overall strategy.
| slackRouter.post( | ||
| "/slack/interactive", | ||
| catchErrors(async (req, res) => { | ||
| return res.status(200).send(); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhance security and functionality of the Slack interactive endpoint.
The current implementation lacks several important aspects:
- Request validation (Slack signs requests with a secret)
- Processing of interactive events
- Event logging
- Error handling for malformed payloads
Consider implementing these security and functional improvements:
slackRouter.post(
"/slack/interactive",
+ validateSlackSignature, // Add middleware to verify request signature
catchErrors(async (req, res) => {
+ const payload = req.body.payload ? JSON.parse(req.body.payload) : req.body;
+
+ logger.info("Received Slack interactive event", {
+ type: payload.type,
+ action: payload.actions?.[0]?.action_id
+ });
+
+ try {
+ await processInteractiveEvent(payload);
+ return res.status(200).send();
+ } catch (error) {
+ logger.error("Failed to process interactive event", { error, payload });
+ return res.status(500).send();
+ }
- return res.status(200).send();
})
);Let's verify if the Slack SDK is available for implementing request signature validation:
🏁 Script executed:
#!/bin/bash
# Check if @slack/bolt package is installed
npm list @slack/bolt || echo "Package not found"Length of output: 116
Action Required: Enhance Slack Interactive Endpoint Security and Functionality
The current implementation of the /slack/interactive endpoint lacks several critical security and reliability features. In particular, it does not validate the Slack request signature, properly parse and process the interactive event payload, log event details, or handle errors robustly. Additionally, our recent verification confirms that the @slack/bolt package is not present in the codebase. If you intend to use @slack/bolt for request signature validation, it must be added as a dependency. Alternatively, please implement a custom validateSlackSignature middleware that verifies the signing secret.
Please ensure to address the following:
-
Request Validation:
Introduce a middleware (e.g.,validateSlackSignature) to verify the request signature. If relying on@slack/bolt, install it; otherwise, develop a custom solution. -
Payload Processing:
Check if the payload is nested (e.g.,req.body.payload) and parse it accordingly before further processing. -
Logging:
Log key details (such as the event type and action ID) from the payload for debugging and auditing purposes. -
Error Handling:
Wrap event processing in a try/catch block and return an appropriate error response upon failure.
Below is the proposed diff illustrating the changes:
slackRouter.post(
"/slack/interactive",
+ validateSlackSignature, // Add middleware to verify request signature
catchErrors(async (req, res) => {
+ const payload = req.body.payload ? JSON.parse(req.body.payload) : req.body;
+
+ logger.info("Received Slack interactive event", {
+ type: payload.type,
+ action: payload.actions?.[0]?.action_id
+ });
+
+ try {
+ await processInteractiveEvent(payload);
+ return res.status(200).send();
+ } catch (error) {
+ logger.error("Failed to process interactive event", { error, payload });
+ return res.status(500).send();
+ }
- return res.status(200).send();
})
);📝 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.
| slackRouter.post( | |
| "/slack/interactive", | |
| catchErrors(async (req, res) => { | |
| return res.status(200).send(); | |
| }) | |
| ); | |
| slackRouter.post( | |
| "/slack/interactive", | |
| validateSlackSignature, // Add middleware to verify request signature | |
| catchErrors(async (req, res) => { | |
| const payload = req.body.payload ? JSON.parse(req.body.payload) : req.body; | |
| logger.info("Received Slack interactive event", { | |
| type: payload.type, | |
| action: payload.actions?.[0]?.action_id | |
| }); | |
| try { | |
| await processInteractiveEvent(payload); | |
| return res.status(200).send(); | |
| } catch (error) { | |
| logger.error("Failed to process interactive event", { error, payload }); | |
| return res.status(500).send(); | |
| } | |
| }) | |
| ); |
| if (job.data.action === "closed") { | ||
| await addJob(SweetQueue.ALERT_MERGED_WITHOUT_APPROVAL, job.data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify PR merge status before enqueueing alert.
The alert should only be triggered for merged PRs, not just closed ones.
- if (job.data.action === "closed") {
+ if (job.data.action === "closed" && job.data.pull_request?.merged === true) {
await addJob(SweetQueue.ALERT_MERGED_WITHOUT_APPROVAL, job.data);
}📝 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 (job.data.action === "closed") { | |
| await addJob(SweetQueue.ALERT_MERGED_WITHOUT_APPROVAL, job.data); | |
| } | |
| if (job.data.action === "closed" && job.data.pull_request?.merged === true) { | |
| await addJob(SweetQueue.ALERT_MERGED_WITHOUT_APPROVAL, job.data); | |
| } |
| export const findTeamActiveAlerts = async <T extends AlertType>({ | ||
| workspaceId, | ||
| teamIds, | ||
| type, | ||
| }: FindActiveAlerts<T>) => { | ||
| const alerts = await getPrisma(workspaceId).alert.findMany({ | ||
| where: { | ||
| teamId: { | ||
| in: teamIds, | ||
| }, | ||
| type, | ||
| enabled: true, | ||
| }, | ||
| include: { | ||
| team: true, | ||
| }, | ||
| }); | ||
|
|
||
| return alerts.map((alert) => alert as AlertWithTeam<T>); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety in findTeamActiveAlerts.
The function needs input validation and safer type casting:
- Add validation for workspaceId and teamIds
- Use type assertion with type guard instead of direct casting
export const findTeamActiveAlerts = async <T extends AlertType>({
workspaceId,
teamIds,
type,
}: FindActiveAlerts<T>) => {
+ if (!workspaceId || !teamIds?.length) {
+ throw new Error('Invalid workspace ID or team IDs');
+ }
+
const alerts = await getPrisma(workspaceId).alert.findMany({
where: {
teamId: {
in: teamIds,
},
type,
enabled: true,
},
include: {
team: true,
},
});
- return alerts.map((alert) => alert as AlertWithTeam<T>);
+ return alerts.map((alert) => {
+ if (!alert.team) {
+ throw new Error('Alert team not found');
+ }
+ return alert as AlertWithTeam<T>;
+ });
};📝 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.
| export const findTeamActiveAlerts = async <T extends AlertType>({ | |
| workspaceId, | |
| teamIds, | |
| type, | |
| }: FindActiveAlerts<T>) => { | |
| const alerts = await getPrisma(workspaceId).alert.findMany({ | |
| where: { | |
| teamId: { | |
| in: teamIds, | |
| }, | |
| type, | |
| enabled: true, | |
| }, | |
| include: { | |
| team: true, | |
| }, | |
| }); | |
| return alerts.map((alert) => alert as AlertWithTeam<T>); | |
| }; | |
| export const findTeamActiveAlerts = async <T extends AlertType>({ | |
| workspaceId, | |
| teamIds, | |
| type, | |
| }: FindActiveAlerts<T>) => { | |
| if (!workspaceId || !teamIds?.length) { | |
| throw new Error('Invalid workspace ID or team IDs'); | |
| } | |
| const alerts = await getPrisma(workspaceId).alert.findMany({ | |
| where: { | |
| teamId: { | |
| in: teamIds, | |
| }, | |
| type, | |
| enabled: true, | |
| }, | |
| include: { | |
| team: true, | |
| }, | |
| }); | |
| return alerts.map((alert) => { | |
| if (!alert.team) { | |
| throw new Error('Alert team not found'); | |
| } | |
| return alert as AlertWithTeam<T>; | |
| }); | |
| }; |
| <Tooltip label="Send a test message to the channel" withArrow> | ||
| <Button | ||
| variant="default" | ||
| onClick={() => { | ||
| sendTestMessage(form.values.channel); | ||
| }} | ||
| leftSection={<IconBrandSlack size={16} stroke={1.5} />} | ||
| loading={isSendingTestMessage} | ||
| > | ||
| Test | ||
| </Button> | ||
| </Tooltip> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for test message.
The test message functionality should handle errors and provide feedback to users.
Apply this diff to add error handling:
<Tooltip label="Send a test message to the channel" withArrow>
<Button
variant="default"
- onClick={() => {
- sendTestMessage(form.values.channel);
+ onClick={async () => {
+ try {
+ await sendTestMessage(form.values.channel);
+ } catch (error) {
+ showErrorNotification({
+ message: 'Failed to send test message. Please verify the channel name.',
+ });
+ }
}}
leftSection={<IconBrandSlack size={16} stroke={1.5} />}
loading={isSendingTestMessage}
>
Test
</Button>
</Tooltip>📝 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.
| <Tooltip label="Send a test message to the channel" withArrow> | |
| <Button | |
| variant="default" | |
| onClick={() => { | |
| sendTestMessage(form.values.channel); | |
| }} | |
| leftSection={<IconBrandSlack size={16} stroke={1.5} />} | |
| loading={isSendingTestMessage} | |
| > | |
| Test | |
| </Button> | |
| </Tooltip> | |
| <Tooltip label="Send a test message to the channel" withArrow> | |
| <Button | |
| variant="default" | |
| onClick={async () => { | |
| try { | |
| await sendTestMessage(form.values.channel); | |
| } catch (error) { | |
| showErrorNotification({ | |
| message: 'Failed to send test message. Please verify the channel name.', | |
| }); | |
| } | |
| }} | |
| leftSection={<IconBrandSlack size={16} stroke={1.5} />} | |
| loading={isSendingTestMessage} | |
| > | |
| Test | |
| </Button> | |
| </Tooltip> |
| export const findWorkspaceByGitInstallationId = ( | ||
| gitInstallationId: string, | ||
| include: Partial<Prisma.WorkspaceInclude> = {} | ||
| ) => { | ||
| return getBypassRlsPrisma().workspace.findFirst({ | ||
| where: { | ||
| installation: { | ||
| gitInstallationId, | ||
| }, | ||
| }, | ||
| include: { | ||
| repositories: true, | ||
| organization: true, | ||
| installation: true, | ||
| memberships: true, | ||
| gitProfile: true, | ||
| ...include, | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review security implications of bypassing RLS.
The function uses getBypassRlsPrisma() which bypasses row-level security. While this might be necessary for certain operations, it could pose security risks if not properly controlled.
Consider:
- Adding documentation explaining why RLS bypass is necessary
- Implementing access control checks before allowing the bypass
- Logging usage of this function for security auditing
🌐 Web query:
What are the security implications of bypassing Row Level Security in Prisma?
💡 Result:
Bypassing Row Level Security (RLS) in Prisma introduces critical security risks by allowing unrestricted access to sensitive data. Here’s a breakdown of the implications:
1. Unauthorized Data Exposure
- Bypassing RLS removes granular access controls, enabling users or applications to read/modify all records in a table[1][4]. For example:
- A Prisma client using a PostgreSQL role with
BYPASSRLSprivileges can ignore all RLS policies[4]. - Service keys (e.g., Supabase’s "Service Key") bypass RLS by design[1], risking accidental exposure if used in client-side code.
- A Prisma client using a PostgreSQL role with
2. Increased Attack Surface for ORM Leaks
- Prisma ORM vulnerabilities (e.g., unrestricted
wherefilters) become more dangerous without RLS enforcement[7]. Attackers could:- Use relational filtering to infer sensitive fields (e.g., passwords) via time-based attacks.
- Exploit many-to-many relationships to leak unpublished records or crash databases via resource exhaustion[7].
3. Privilege Escalation Risks
- PostgreSQL superusers and roles with
BYPASSRLSinherently ignore RLS policies[4]. If Prisma connects via such roles:- Application logic becomes the sole defense layer.
- Misconfigured policies or code flaws could expose all data.
4. Misconfiguration Pitfalls
- Table owners bypass RLS by default unless
FORCE ROW LEVEL SECURITYis explicitly enabled[4]. This can lead to accidental exposure if:- Applications connect using the table owner’s role.
- Developers assume RLS is active without verifying policies.
Key Recommendations
- Avoid BYPASSRLS roles for standard application operations[1][4].
- Separate database roles for application connections (e.g.,
authenticated/anon) from administrative roles[1][8]. - Validate user inputs rigorously to prevent ORM-based leaks[7].
- Use service keys only for backend/admin tasks—never expose them publicly[1].
By circumventing RLS, you shift security responsibility entirely to application-layer controls, which are more error-prone and harder to audit than database-level policies[1][4][7].
Citations:
- 1: https://supabase.com/docs/guides/database/postgres/row-level-security
- 2: https://docs.prismacloud.io/en/enterprise-edition/content-collections/runtime-security/vulnerability-management/vulnerability-management-policies
- 3: https://docs.prismacloud.io/en/enterprise-edition/content-collections/application-security/risk-management/monitor-and-manage-code-build/enforcement
- 4: https://aws.amazon.com/blogs/database/multi-tenant-data-isolation-with-postgresql-row-level-security/
- 5: https://docs.prismacloud.io/en/compute-edition/22-12/admin-guide/vulnerability-management/configure-vuln-management-rules
- 6: https://www.paloaltonetworks.com/blog/prisma-cloud/redshift-inside-out-part-1/
- 7: https://www.elttam.com/blog/plorming-your-primsa-orm/
- 8: https://github.com/cerebruminc/yates
- 9: Support for row-level security (RLS) prisma/prisma#12735
Action Required: Address RLS Bypass Risks and Validate Include Parameter
- The use of
getBypassRlsPrisma()bypasses Row Level Security, which may expose sensitive data by removing database-level access controls. In light of this, please:- Add documentation explaining why bypassing RLS is necessary.
- Implement strict access control checks to ensure that only authorized operations can invoke this bypass.
- Consider logging calls to this function for audit purposes.
- Additionally, review the type safety of the
includeparameter (typed asPartial<Prisma.WorkspaceInclude>). Ensure that merging the default includes (i.e.,organization,installation, andgitProfile) with user-supplied values doesn’t inadvertently expose unexpected relations or data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/src/app/alerts/services/alert-slow-review.service.ts (4)
9-18: Add error handling and consider configurable rate limiting.The function should handle potential errors during alert processing and allow for configurable delays between alerts.
Consider applying these improvements:
export const processSlowReviewAlert = async ( alert: AlertWithRelations<"SLOW_REVIEW"> ) => { + try { const pullRequests = await findAlertableSlowReviewPullRequests(alert); for (const pullRequest of pullRequests) { - await handleAlertEvent(alert, pullRequest); - await sleep(100); + try { + await handleAlertEvent(alert, pullRequest); + await sleep(alert.settings.delayBetweenAlerts ?? 100); + } catch (error) { + console.error(`Failed to handle alert for PR ${pullRequest.id}:`, error); + // Continue processing other PRs even if one fails + } } + } catch (error) { + console.error('Failed to process slow review alert:', error); + throw error; + } };
56-56: Address the TODO comment regarding approval tracking.The comment indicates a pending change to use
fullyApprovedAtinstead offirstApprovalAt.Would you like me to help implement the
fullyApprovedAttracking functionality or create an issue to track this task?
92-99: Make alert frequency configurable.The 24-hour alert frequency is hardcoded. Consider making this configurable through alert settings.
- return isBefore(lastEvent.createdAt, subHours(new Date(), 24)); + const alertFrequencyHours = alert.settings.alertFrequencyHours ?? 24; + return isBefore(lastEvent.createdAt, subHours(new Date(), alertFrequencyHours));
104-151: Improve type safety and configuration management.The message block structure could benefit from type definitions, and external URLs should be configurable.
Consider these improvements:
- Add type definitions for message blocks:
type MessageBlock = { type: 'section' | 'context' | 'divider'; text?: { type: 'mrkdwn'; text: string; }; elements?: Array<{ type: 'image' | 'mrkdwn'; image_url?: string; alt_text?: string; text?: string; }>; };
- Move configuration to environment variables:
- image_url: "https://github.githubassets.com/favicons/favicon.png", + image_url: process.env.GITHUB_FAVICON_URL ?? "https://github.githubassets.com/favicons/favicon.png",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/app/alerts/services/alert-slow-review.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
| const handleAlertEvent = async ( | ||
| alert: AlertWithRelations<"SLOW_REVIEW">, | ||
| pullRequest: PullRequestWithRelations | ||
| ) => { | ||
| await sendAlert({ | ||
| workspaceId: alert.workspaceId, | ||
| channel: alert.channel, | ||
| blocks: await getMessageBlocks(alert, pullRequest), | ||
| text: "A pull request has been pending review for a while", | ||
| }); | ||
|
|
||
| await getPrisma(alert.workspaceId).alertEvent.create({ | ||
| data: { | ||
| alertId: alert.id, | ||
| pullRequestId: pullRequest.id, | ||
| workspaceId: alert.workspaceId, | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure data consistency with transactions and add error handling.
The alert sending and event creation should be wrapped in a transaction to maintain data consistency.
Consider applying these improvements:
const handleAlertEvent = async (
alert: AlertWithRelations<"SLOW_REVIEW">,
pullRequest: PullRequestWithRelations
) => {
+ const prisma = getPrisma(alert.workspaceId);
+
+ return prisma.$transaction(async (tx) => {
+ try {
await sendAlert({
workspaceId: alert.workspaceId,
channel: alert.channel,
blocks: await getMessageBlocks(alert, pullRequest),
text: "A pull request has been pending review for a while",
});
- await getPrisma(alert.workspaceId).alertEvent.create({
+ return tx.alertEvent.create({
data: {
alertId: alert.id,
pullRequestId: pullRequest.id,
workspaceId: alert.workspaceId,
},
});
+ } catch (error) {
+ console.error('Failed to handle alert event:', error);
+ throw error;
+ }
+ });
};📝 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 handleAlertEvent = async ( | |
| alert: AlertWithRelations<"SLOW_REVIEW">, | |
| pullRequest: PullRequestWithRelations | |
| ) => { | |
| await sendAlert({ | |
| workspaceId: alert.workspaceId, | |
| channel: alert.channel, | |
| blocks: await getMessageBlocks(alert, pullRequest), | |
| text: "A pull request has been pending review for a while", | |
| }); | |
| await getPrisma(alert.workspaceId).alertEvent.create({ | |
| data: { | |
| alertId: alert.id, | |
| pullRequestId: pullRequest.id, | |
| workspaceId: alert.workspaceId, | |
| }, | |
| }); | |
| }; | |
| const handleAlertEvent = async ( | |
| alert: AlertWithRelations<"SLOW_REVIEW">, | |
| pullRequest: PullRequestWithRelations | |
| ) => { | |
| const prisma = getPrisma(alert.workspaceId); | |
| return prisma.$transaction(async (tx) => { | |
| try { | |
| await sendAlert({ | |
| workspaceId: alert.workspaceId, | |
| channel: alert.channel, | |
| blocks: await getMessageBlocks(alert, pullRequest), | |
| text: "A pull request has been pending review for a while", | |
| }); | |
| return tx.alertEvent.create({ | |
| data: { | |
| alertId: alert.id, | |
| pullRequestId: pullRequest.id, | |
| workspaceId: alert.workspaceId, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error('Failed to handle alert event:', error); | |
| throw error; | |
| } | |
| }); | |
| }; |
Summary by CodeRabbit
New Features
AlertBaseFields,AlertLoadingSkeleton,TeamAlertsPage, and alert-specific settings pages.TriggerDescriptionfor displaying trigger details.Bug Fixes
Documentation
Refactor