feat: team digests #31
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a new database structure for managing digests, including a new "Digest" table with associated enumerated types and constraints. It adds new GraphQL types and queries to facilitate interaction with the digest data, along with mutations for updating digests. New components and hooks are created to handle user interactions and data fetching related to digests. Additionally, several background workers are implemented for scheduling and sending digests, enhancing the application's capabilities in managing team metrics and work-in-progress notifications. Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
@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: 30
🧹 Outside diff range and nitpick comments (53)
apps/api/src/app/scalars/scalars.schema.ts (1)
8-8: Add documentation for the TimeZone scalar.Consider adding documentation comments to describe:
- The expected format (IANA timezone database)
- Example valid values
- Any validation rules
Example documentation:
+ """ + The TimeZone scalar represents a timezone string in IANA timezone database format. + Example values: 'America/New_York', 'Europe/London', 'Asia/Tokyo' + """ scalar TimeZoneapps/web/src/app/teams/[id]/use-team.ts (2)
1-2: Consider using path aliases for imports.The deep relative import path (
../../../) could be replaced with a path alias for better maintainability and readability.Example configuration in
tsconfig.json:{ "compilerOptions": { "paths": { "@exceptions/*": ["./src/exceptions/*"] } } }Then the import could be simplified to:
-import { ResourceNotFound } from "../../../exceptions/resource-not-found.exception"; +import { ResourceNotFound } from "@exceptions/resource-not-found.exception";
4-10: LGTM! Clean and focused implementation.The hook provides a clear, reusable way to extract and validate team IDs from URL parameters.
Consider adding TypeScript types for better type safety:
-export const useTeamId = () => { +export const useTeamId = (): string => { const { teamId } = useParams<{ teamId: string }>(); if (!teamId) throw new ResourceNotFound(); return teamId; };apps/api/src/app/digests/transformers/digest.transformer.ts (1)
11-11: Consider explicit null check for enabled property.The current default value handling uses a logical OR which could treat falsy values (like empty string) as false.
- enabled: digest.enabled || false, + enabled: digest.enabled ?? false,apps/api/src/app/scalars/scalars.resolver.ts (1)
7-7: LGTM! Good choice using TimeZoneResolver for timezone validation.The addition of TimeZoneResolver from graphql-scalars is a solid choice as it ensures proper validation against the IANA timezone database, which is crucial for the digest feature's timezone management.
Since this scalar will be used for digest timezone settings, it ensures that only valid timezone strings can be stored in the database, preventing potential issues with incorrect timezone data.
Also applies to: 18-18
apps/web/src/components/box-setting/box-setting.tsx (1)
11-16: Consider renaming 'leftElement' to 'labelElement' for consistency.The variable name still reflects the old 'left' prop naming convention, while we've moved to using 'label'.
- const leftElement = isString(label) ? <Text fz="lg">{label}</Text> : label; + const labelElement = isString(label) ? <Text fz="lg">{label}</Text> : label;apps/web/src/app/teams/[id]/digests/use-digests.tsx (1)
1-5: Consider using path aliases to improve import readabilityThe deep relative paths (
../../../../) make the imports harder to maintain. Consider using TypeScript path aliases to make these imports cleaner and more maintainable.Example configuration in
tsconfig.json:{ "compilerOptions": { "baseUrl": ".", "paths": { "@api/*": ["src/api/*"], "@providers/*": ["src/providers/*"] } } }Then imports could be simplified to:
import { useTeamDigestsQuery } from "@api/digest.api"; import { useWorkspace } from "@providers/workspace.provider";apps/web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/alert-enable-slack.tsx (3)
5-5: Consider defining specific prop interface.While using
AlertPropsworks, consider creating a more specific interface that extendsAlertPropsto document any additional props or constraints specific to this component's usage.+interface AlertEnableSlackProps extends AlertProps { + // Add any additional props specific to this component +} -export const AlertEnableSlack = (props: AlertProps) => { +export const AlertEnableSlack = (props: AlertEnableSlackProps) => {
7-17: Extract style configurations to constants.Consider extracting the style configurations and theme values to named constants for better maintainability and reusability.
+const ALERT_STYLES = { + wrapper: { + alignItems: "center", + }, +} as const; + +const ICON_STROKE_WIDTH = 1.5; + <Alert variant="light" color="violet" - styles={{ - wrapper: { - alignItems: "center", - }, - }} - icon={<IconAlertHexagon stroke={1.5} />} + styles={ALERT_STYLES} + icon={<IconAlertHexagon stroke={ICON_STROKE_WIDTH} />} {...props} >
7-33: Enhance accessibility with ARIA labels.Consider adding appropriate ARIA labels to improve accessibility:
<Alert + aria-label="Slack integration required" variant="light" color="violet" // ... other props > <Group justify="space-between"> <Text>Setup integration with Slack to enable this feature.</Text> <Button + aria-label="Enable Slack integration" // ... other props > Enable </Button> </Group> </Alert>apps/web/src/providers/integration.provider.tsx (2)
25-25: Consider making messaging apps more maintainableThe messaging apps array could be more maintainable and documented for future additions.
Consider this approach:
-const messagingApps = [IntegrationApp.SLACK]; +/** + * Supported messaging integration apps. + * Add new messaging apps here as they become supported. + */ +const MESSAGING_APPS = Object.freeze([ + IntegrationApp.SLACK, + // Future messaging apps... +]) as readonly IntegrationApp[];
27-38: Consider enhancing messaging integration selection logicThe current implementation has some limitations:
- It returns the first enabled messaging integration without considering priority
- The find operation runs on every render
- It's not explicit about returning only one integration
Consider this enhanced implementation:
export const useMessagingIntegration = () => { const { integrations, query } = useIntegrations(); if (!integrations) return { integration: undefined, query }; + // Memoize the found integration + const integration = useMemo(() => { + // Explicit about returning the primary messaging integration + return Object.values(integrations).find( + (i) => messagingApps.includes(i.app) && i.isEnabled, + ); + }, [integrations]); + return { - integration: Object.values(integrations).find( - (i) => messagingApps.includes(i.app) && i.isEnabled, - ), + integration, query, }; };Don't forget to add the useMemo import:
+import { useMemo } from 'react';apps/api/src/app/digests/resolvers/digests.schema.ts (2)
2-20: LGTM! Consider adding DAILY frequency option.The enum definitions are well-structured and follow GraphQL naming conventions. However, for some use cases, daily digests might be valuable to teams needing more frequent updates.
Consider adding a DAILY option to the Frequency enum:
enum Frequency { + DAILY WEEKLY MONTHLY }
38-40: Document the upsert behavior of updateDigest mutation.The absence of a separate createDigest mutation suggests that updateDigest handles both creation and updates. This should be explicitly documented to avoid confusion.
Add documentation comment:
extend type Mutation { + """ + Creates a new digest if it doesn't exist, otherwise updates the existing one. + """ updateDigest(input: UpdateDigestInput!): Digest! }apps/web/src/components/select-timezone/select-timezone.tsx (2)
5-5: Consider adding specific prop types and documentation.Instead of passing through all SelectProps, consider creating a specific interface for this component that only includes the props you need. This would make the component's API more explicit and maintainable.
-export const SelectTimezone = (props: SelectProps) => { +interface SelectTimezoneProps extends Pick<SelectProps, 'value' | 'onChange' | 'defaultValue'> { + /** Additional props documentation */ +} + +export const SelectTimezone = (props: SelectTimezoneProps) => {
32-40: Enhance accessibility and error handling.The Select component could benefit from improved accessibility and error handling capabilities.
return ( <Select label="Timezone" + aria-label="Select your timezone" searchable + error={props.error} + required={props.required} renderOption={renderSelectOption} data={timezones} - {...props} + {...(props as SelectProps)} /> );apps/web/src/components/select-hour/select-hour.tsx (1)
3-28: Consider optimizing the hours array implementation.The current implementation could be improved in several ways:
- Move the array outside the component file to a separate constants file
- Generate the array programmatically to reduce code size
- Add internationalization support for time formats
Here's a suggested implementation:
// src/constants/time.ts import { format } from 'date-fns'; export const generateHourOptions = () => { return Array.from({ length: 24 }, (_, index) => { const date = new Date(); date.setHours(index, 0, 0); return { label: format(date, 'hh:mm a'), // Localized 12-hour format value: format(date, 'HH:mm') // 24-hour format }; }); }; export const HOURS = generateHourOptions();Then import and use it in your component:
-const hours = [ - { label: "12:00 AM", value: "00:00" }, - // ... removed for brevity -]; +import { HOURS } from '@/constants/time';apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts (2)
8-11: Consider enhancing error handling and input validation.While the basic structure is good, consider these improvements:
- Add a try-catch block to properly log errors
- Validate input parameters before processing
Consider applying this pattern:
export const updateDigest = createMutationResolver({ updateDigest: async (_, { input }, context) => { logger.info("mutation.updateDigest", { input }); + + try { + // Validate input + if (!input.workspaceId || !input.type) { + throw new Error("Missing required fields"); + } + + // Existing logic here... + + } catch (error) { + logger.error("Failed to update digest", { + error, + input, + gitProfileId: context.currentToken.gitProfileId + }); + throw error; + }
32-34: Add error handling for digest transformation.Consider wrapping the transformation in a try-catch block to handle potential transformation errors gracefully.
- return transformDigest(digest); + try { + return transformDigest(digest); + } catch (error) { + logger.error("Failed to transform digest", { error, digest }); + throw new Error("Failed to process digest data"); + }apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql (1)
1-3: Consider future extensibility of DigestType enumThe enum types are well-defined, but
DigestTypemight need more values in the future. Consider documenting the process for adding new digest types, as enum modifications require careful database migration handling.apps/api/src/app/digests/services/digest.types.ts (2)
11-13: Consider strengthening type safety for settingsWhile the type definition is correct, consider making the settings type more specific to each digest type rather than using a generic
DigestSettingstype. This would provide better type safety and autocompletion support.export type Digest<T extends DigestSettings = DigestSettings> = Omit<DbDigest, "settings"> & { settings: T; };
1-62: Consider adding runtime validation for digest configurationsWhile the type system provides compile-time safety, consider adding runtime validation using a schema validation library like Zod or io-ts. This would help ensure that:
- The settings object matches the expected shape for each digest type
- The frequency and dayOfTheWeek combinations are valid
- The timeOfDay format is correct
- The timezone is valid
Would you like me to provide an example implementation using Zod?
apps/api/src/app/digests/resolvers/queries/team-digests.query.ts (2)
26-34: Remove unnecessary type assertion and add input validation.The type assertion is redundant, and the input type should be validated.
Consider this improvement:
+if (!input?.type) { + throw new Error("Digest type is required"); +} const digest = await findDigestByType({ workspaceId, teamId: team.id, type: input.type, }); if (!digest) return null; -return transformDigest(digest as Digest); +return transformDigest(digest);
36-52: Consider adding pagination and error handling.The implementation could benefit from some improvements:
- Similar workspace ID handling issue as in the digest resolver.
- No error handling for the database query.
- No pagination for potentially large result sets.
Consider these improvements:
digests: async (team) => { logger.info("query.team.digests", { team }); if (!team.id) { - throw new ResourceNotFoundException("Team not found"); + throw new ResourceNotFoundException(`Team not found: ${JSON.stringify(team)}`); } - const workspaceId = "workspaceId" in team && (team.workspaceId as number); + const workspaceId = team.workspaceId; if (!workspaceId) { - throw new ResourceNotFoundException("Workspace not found"); + throw new ResourceNotFoundException(`Workspace not found for team ${team.id}`); } + try { const digests = await findDigestsByTeam(workspaceId, team.id); return digests.map(transformDigest); + } catch (error) { + logger.error("Failed to fetch team digests", { error, teamId: team.id }); + throw error; + } },Consider adding pagination in a future iteration to handle large datasets efficiently:
digests: async (team, { first = 10, after } = {}) => { // ... existing validation ... const digests = await findDigestsByTeam(workspaceId, team.id, { first, after }); return { edges: digests.map(digest => ({ node: transformDigest(digest), cursor: digest.id })), pageInfo: { // ... pagination info } }; }apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx (4)
17-20: Add JSDoc documentation for the interface.Consider adding documentation to explain the purpose of the interface and its parameters, which will improve code maintainability and help other developers understand the expected usage.
+/** + * Props for the useDigest hook + * @param teamId - The ID of the team to fetch/update digest settings for + * @param type - The type of digest being managed + */ interface UseDigestsProps { teamId: string; type: DigestType; }
28-47: Enhance mutation error handling and type safety.The current error handling uses a generic message. Consider:
- Adding specific error messages based on the error type
- Adding type safety for the mutation input
- onError: () => { + onError: (error) => { + const message = error.response?.errors?.[0]?.message ?? + 'Something went wrong. Please try again.'; showErrorNotification({ - message: "Something went wrong. Please try again.", + message, }); }, }); - const mutate = (input: MutationUpdateDigestArgs["input"]) => { + const mutate = (input: Omit<MutationUpdateDigestArgs["input"], "teamId" | "type">) => { triggerMutation({ - input, + input: { + ...input, + teamId, + type, + }, }); };
49-55: Optimize query implementation with useMemo.Consider memoizing the query input to prevent unnecessary re-renders when parent components update.
+import { useMemo } from 'react'; const { data, ...query } = useTeamDigestQuery({ workspaceId: workspace.id, teamId, - input: { - type, - }, + input: useMemo(() => ({ + type, + }), [type]), });
57-65: Add explicit return type and improve type safety of merged data.Consider:
- Adding an explicit return type interface
- Being more explicit about property merging to prevent accidental overwrites
+interface DigestHookReturn { + digest: { + title: string; + description: string; + // Add other expected properties + }; + query: Omit<ReturnType<typeof useTeamDigestQuery>, 'data'>; + mutate: (input: Omit<MutationUpdateDigestArgs["input"], "teamId" | "type">) => void; + mutation: Omit<ReturnType<typeof useUpdateDigestMutation>, 'mutate'>; +} - return { + return { digest: { - ...digestCard, - ...data?.workspace.team?.digest, + ...digestCard, + enabled: data?.workspace.team?.digest?.enabled ?? false, + schedule: data?.workspace.team?.digest?.schedule, + // Explicitly list other properties to merge }, query, mutate, mutation, - }; + } satisfies DigestHookReturn;apps/web/src/app/automations/settings/pr-size-labeler/components/form-pr-size-labeler-settings/form-pr-size-labeler-settings.tsx (1)
2-2: Consider using path aliases for deep imports.The deep relative import path could be simplified using path aliases (e.g., '@components/box-setting') to improve maintainability and reduce the risk of path-related issues during refactoring.
apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx (3)
4-13: Consider adding JSDoc comments for better documentation.The interface is well-structured with clear property types. Consider adding JSDoc comments to document the purpose of each property, especially for
enabledvsavailablewhich might not be immediately clear to other developers.Example documentation:
+/** + * Represents the structure of a digest card with its metadata and functionality. + */ interface DigestCardData { + /** The type of digest this card represents */ type: DigestType; + /** Whether this digest is currently enabled for the team */ enabled: boolean; + /** Whether this digest feature is available for use */ available: boolean; // ... rest of the properties }
15-38: Consider improving maintainability of static content.While the implementation is solid, consider these improvements:
- Extract image paths to a constants file to make path updates easier
- Consider using a CMS or i18n solution for text content to support future localization
Example refactor:
// constants/paths.ts export const DIGEST_IMAGES = { TEAM_METRICS: '/images/digests/metrics.webp', TEAM_WIP: '/images/digests/wip.webp', } as const; // i18n/digests.ts export const DIGEST_CONTENT = { TEAM_METRICS: { title: 'Metrics Digest', description: '...', shortDescription: '...', }, // ... } as const;
48-59: Consider memoizing the return value.The hook implementation is clean and efficient. However, since it's processing static data, consider memoizing the return value to prevent unnecessary recalculations.
Example implementation:
export const useDigestsCards = (): UseDigestCards => { - const [availableDigests, futureDigests] = fork( - Object.values(digestCards), - (automation) => automation.available, - ); + const [availableDigests, futureDigests] = useMemo( + () => fork(Object.values(digestCards), (automation) => automation.available), + [] // No dependencies as digestCards is constant + ); return { digestCards, availableDigests, futureDigests, }; };apps/web/src/app/settings/integrations/page.tsx (1)
Line range hint
27-48: Consider making integration cards data-drivenThe current implementation hardcodes integration cards in the component. Consider refactoring to a data-driven approach where integrations are defined in a configuration object or fetched from an API. This would make it easier to:
- Add new integrations
- Manage integration properties
- Toggle feature availability
Example approach:
type Integration = { id: string; title: string; description: string; available: boolean; brand: 'slack' | 'msteams' | string; path: string; }; const INTEGRATIONS: Integration[] = [ { id: 'slack', title: 'Slack', description: 'Send notifications to your Slack workspace.', available: true, brand: 'slack', path: '/settings/integrations/slack' }, // ... other integrations ];apps/web/src/api/digest.api.ts (2)
19-42: Consider enhancing query configuration for better reliabilityWhile the implementation is functional, consider these improvements:
- Include
workspaceIdin the query key for more precise cache management- Add error handling configuration
- Configure retry behavior for network failures
export const useTeamDigestsQuery = ( args: TeamDigestsQueryVariables, options?: Partial<UseQueryOptions<TeamDigestsQuery>>, ) => useQuery({ - queryKey: ["digests", args.teamId], + queryKey: ["digests", args.workspaceId, args.teamId], queryFn: () => graphQLClient.request( graphql(/* GraphQL */ ` query TeamDigests($workspaceId: SweetID!, $teamId: SweetID!) { workspace(workspaceId: $workspaceId) { team(teamId: $teamId) { digests { type enabled } } } } `), args, ), + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), ...options, });
1-106: Consider implementing a custom hook composition patternThe current implementation is solid, but consider extracting common configurations into a shared hook factory or configuration object. This would help maintain consistency across query/mutation configurations and make it easier to update common settings like retry logic and error handling.
Example approach:
- Create a
createDigestHookfactory function- Define shared configurations (retry logic, error handling, cache key structure)
- Use the factory to create individual hooks
apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (3)
26-28: Consider adding prop validation for the form object.While the TypeScript typing is good, consider adding runtime validation to ensure the form object contains all required fields and initial values.
export const DigestBaseFields = ({ form }: DigestBaseFieldsProps) => { + if (!form) { + throw new Error('DigestBaseFields: form prop is required'); + } const isEnabled = form.values.enabled;
112-116: Improve timezone handling.Consider adding timezone validation and default to the user's local timezone.
<SelectTimezone {...form.getInputProps("timezone", { type: "input" })} label="Timezone" + defaultValue={Intl.DateTimeFormat().resolvedOptions().timeZone} leftSection={<IconWorld size={16} stroke={1.5} />} />
29-122: Consider enhancing accessibility.While the component is well-structured, consider these accessibility improvements:
- Add aria-labels to interactive elements
- Ensure proper focus management when sections are conditionally rendered
- Add appropriate role attributes to grouped elements
-<Stack p="md"> +<Stack p="md" role="group" aria-labelledby="settings-title"> - <Title order={5}>Settings</Title> + <Title order={5} id="settings-title">Settings</Title> // ... rest of the codeapps/web/src/components/select-timezone/timezones.ts (1)
119-122: Resolve timezone organization inconsistencies.There are a few organizational issues to address:
- Perth appears in both Asia group (with China/Singapore) and Australia group
- Some labels use raw identifiers (e.g., "America/Adak") while others use friendly names
Consider:
- Moving Perth to only the Australia group
- Using consistent friendly names for all timezones
- Splitting combined regions into separate entries when possible
- { - label: "China, Singapore, Perth", - value: "Asia/Shanghai", - }, + { + label: "China & Singapore Time", + value: "Asia/Shanghai", + },Also applies to: 168-170
apps/api/prisma/schema.prisma (1)
63-76: LGTM! Consider future extensibility of DigestType.The enum definitions are well-structured. While
DayOfTheWeekis comprehensive,DigestTypemight need expansion in the future.Consider documenting potential future digest types in comments to help track the evolution of this feature.
packages/graphql-types/frontend/gql.ts (1)
123-134: Consider reducing comment duplication.The JSDoc comment "The graphql function is used to parse GraphQL queries into a document that can be used by GraphQL clients" is repeated for each overload. Consider using a single comment with
@overloadtags./** * The graphql function is used to parse GraphQL queries into a document that can be used by GraphQL clients. + * + * @overload + * Query to fetch basic digest info for a team + */ export function graphql(source: "..."): typeof documents["..."]; + /** + * @overload + * Query to fetch detailed digest settings + */ export function graphql(source: "..."): typeof documents["..."]; + /** + * @overload + * Mutation to update digest settings + */ export function graphql(source: "..."): typeof documents["..."];apps/web/src/app/teams/[id]/digests/page.tsx (1)
43-46: Simplify theenabledprop logic for better readabilityThe current expression in the
enabledprop can be simplified. The use of|| falseis redundant since the logical AND operation already returns a boolean. Additionally, the double negation!!integrationcan be simplified ifintegrationis expected to be a truthy or falsy value.Consider simplifying the expression as follows:
enabled={ - (digests?.[digest.type]?.enabled && !!integration) || - false + digests?.[digest.type]?.enabled && integration }This refactor enhances readability by removing unnecessary operators and makes the intent of the code clearer.
apps/api/src/app/digests/services/digest.service.ts (3)
28-28: Improve the Clarity of Comments Regarding JSON TypesThe comment
// JSON types in Prisma is badis vague and doesn't provide sufficient context.Consider rephrasing the comment to clearly explain the issue:
- // JSON types in Prisma is bad + // Workaround due to limitations with Prisma's JSON type handlingThis makes it clearer to other developers why the casting is necessary.
Also applies to: 42-42
86-88: Ensure Deep Merging of Settings ObjectsWhen merging the existing
digest.settingswith the newsettings, theassignfunction fromradashperforms a shallow merge, which might not merge nested objects correctly.Consider using a deep merge function to accurately merge nested settings:
- ? assign(digest.settings as object, settings) + ? deepMerge(digest.settings as object, settings)Ensure that you import a reliable deep merge utility or implement one if necessary.
14-142: Add Unit Tests for New Service MethodsThe newly added service methods are critical to the application's functionality. Implementing unit tests will help validate their behavior and safeguard against future regressions.
Would you like assistance in creating unit tests for these methods or opening a GitHub issue to track this task?
apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx (2)
76-77: Avoid disabling ESLint rules by adjusting the dependency array inuseMemoDisabling
react-hooks/exhaustive-depsmay hide potential issues with missing dependencies. Ensure that all necessary dependencies are included in the dependency array ofuseMemo. Ifformis stable, consider addingform.isValidto the dependencies.Apply this diff to adjust the dependency array:
-// eslint-disable-next-line react-hooks/exhaustive-deps -const isFormValid = useMemo(() => form.isValid(), [form.values]); +const isFormValid = useMemo(() => form.isValid(), [form]);
51-51: Setfrequencyto a valid default value from theFrequencyenumCurrently,
frequencyis initialized to an empty string"", which may not correspond to any value in theFrequencyenum. This could cause issues during form validation or when casting the value. Consider settingfrequencyto a valid default frequency.Apply this diff to set a valid default value:
- frequency: "", + frequency: Frequency.DAILY, // Replace with an appropriate default valueEnsure you have imported the
Frequencyenum:import { DayOfTheWeek, DigestType, - Frequency, } from "@sweetr/graphql-types/frontend/graphql"; + Frequency, +} from "@sweetr/graphql-types/frontend/graphql";apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx (2)
28-28: Handle missingteamIdmore gracefully instead of throwing an exceptionThrowing an exception inside a React component can cause unhandled errors if not caught by an error boundary. It's better to render a fallback UI component or redirect to a 'Not Found' page to handle this scenario gracefully.
Apply this diff to render a fallback component:
- if (!teamId) throw new ResourceNotFound(); + if (!teamId) { + return <ResourceNotFound />; + }
76-77: Include all necessary dependencies inuseMemoand avoid disabling eslint rulesThe
useMemohook depends on bothformandform.values. To follow best practices and prevent potential issues, include all dependencies and avoid disabling thereact-hooks/exhaustive-depsrule.Apply this diff to fix the dependency array:
- // eslint-disable-next-line react-hooks/exhaustive-deps - const isFormValid = useMemo(() => form.isValid(), [form.values]); + const isFormValid = useMemo(() => form.isValid(), [form, form.values]);packages/graphql-types/api.ts (4)
182-182: Specify the expected format for the 'timeOfDay' fieldThe
timeOfDayfield is of typeString. To ensure consistent and valid time inputs, consider defining a custom scalar type or specifying the expected time format (e.g., 'HH:mm:ss').
515-516: Clarify the usage of 'digest' and 'digests' fields in the 'Team' typeHaving both
digest(optional) anddigests(array) in theTeamtype may cause confusion. Please clarify their purposes or consider consolidating them if appropriate.
196-199: Consider adding more frequency options to the 'Frequency' enumThe
Frequencyenum currently includesMONTHLYandWEEKLY. If other frequencies (e.g.,DAILY) are required now or in the future, consider adding them to enhance flexibility.
181-181: Define a specific type for the 'settings' fieldThe
settingsfield in theDigesttype is of typeJSONObject, which may be too generic. Defining a specific type forsettingscan improve type safety and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (45)
apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql(1 hunks)apps/api/prisma/schema.prisma(4 hunks)apps/api/src/app/digests/resolvers/digests.schema.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/services/digest.service.ts(1 hunks)apps/api/src/app/digests/services/digest.types.ts(1 hunks)apps/api/src/app/digests/transformers/digest.transformer.ts(1 hunks)apps/api/src/app/scalars/scalars.resolver.ts(2 hunks)apps/api/src/app/scalars/scalars.schema.ts(1 hunks)apps/api/src/lib/date.ts(1 hunks)apps/web/src/api/digest.api.ts(1 hunks)apps/web/src/app/automations/settings/pr-size-labeler/components/form-pr-size-labeler-settings/form-pr-size-labeler-settings.tsx(3 hunks)apps/web/src/app/automations/settings/pr-size-labeler/page.tsx(1 hunks)apps/web/src/app/automations/settings/pr-title-check/components/form-pr-title-check-settings/form-pr-title-check-settings.tsx(2 hunks)apps/web/src/app/automations/settings/pr-title-check/page.tsx(1 hunks)apps/web/src/app/automations/use-automation-cards.tsx(1 hunks)apps/web/src/app/settings/integrations/page.tsx(1 hunks)apps/web/src/app/settings/integrations/slack/page.tsx(2 hunks)apps/web/src/app/settings/integrations/use-integrations.tsx(0 hunks)apps/web/src/app/teams/[id]/digests/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/alert-enable-slack.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/index.ts(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/digest-loading-skeleton.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/index.ts(1 hunks)apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/types.ts(1 hunks)apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/use-digests.tsx(1 hunks)apps/web/src/app/teams/[id]/use-team.ts(1 hunks)apps/web/src/components/box-setting/box-setting.tsx(1 hunks)apps/web/src/components/select-hour/index.ts(1 hunks)apps/web/src/components/select-hour/select-hour.tsx(1 hunks)apps/web/src/components/select-timezone/select-timezone.tsx(1 hunks)apps/web/src/components/select-timezone/timezones.ts(1 hunks)apps/web/src/providers/date.provider.ts(2 hunks)apps/web/src/providers/integration.provider.tsx(1 hunks)apps/web/src/routes.tsx(2 hunks)packages/graphql-types/api.ts(17 hunks)packages/graphql-types/codegen.ts(1 hunks)packages/graphql-types/frontend/gql.ts(2 hunks)packages/graphql-types/frontend/graphql.ts(9 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/app/settings/integrations/use-integrations.tsx
✅ Files skipped from review due to trivial changes (6)
- apps/api/src/lib/date.ts
- apps/web/src/app/automations/use-automation-cards.tsx
- apps/web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/index.ts
- apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-loading-skeleton.tsx
- apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/index.ts
- apps/web/src/components/select-hour/index.ts
🔇 Additional comments (72)
apps/api/src/app/scalars/scalars.schema.ts (1)
8-8: LGTM! Verify TimeZone scalar implementation.
The addition of the TimeZone scalar is appropriate for handling timezone data in the digest functionality.
Let's verify the scalar implementation:
✅ Verification successful
TimeZone scalar is properly implemented using graphql-scalars package ✅
The TimeZone scalar is correctly implemented in apps/api/src/app/scalars/scalars.resolver.ts using the TimeZoneResolver from the graphql-scalars package, which provides proper validation and handling of IANA timezone formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for TimeZone scalar resolver implementation
# Expected: Find files containing TimeZone scalar resolver definition
# Search for TimeZone scalar resolver
echo "Searching for TimeZone scalar resolver implementation..."
rg -l "TimeZone.*scalar" --type ts
# Search for potential validation logic
echo "Searching for TimeZone validation logic..."
rg -l "TimeZone.*validate" --type ts
Length of output: 293
Script:
#!/bin/bash
# Let's try a broader search for TimeZone related implementations
echo "Searching for TimeZone in typescript files..."
rg -l "TimeZone" --type ts
# Search for scalar resolver files
echo "Searching for scalar resolver files..."
fd "scalar" --type f --extension ts
# Look for content of scalar resolver files
echo "Content of potential scalar resolver files..."
fd "scalar" --type f --extension ts --exec cat {}
Length of output: 1873
apps/api/src/app/digests/transformers/digest.transformer.ts (1)
1-6: LGTM! Clean and specific imports.
The imports are well-organized with clear distinction between API and local types.
apps/web/src/app/teams/[id]/digests/settings/types.ts (2)
1-2: LGTM! Clean and well-organized imports.
The imports follow good practices by being specific and well-ordered.
15-19: Verify the necessity of separate digest type definitions.
Currently, FormMetricsDigest and FormWipDigest are identical to FormDigest. If these types are expected to diverge in the future, consider adding a comment explaining the intention. Otherwise, consider using the base FormDigest type directly to reduce maintenance overhead.
apps/web/src/components/box-setting/box-setting.tsx (2)
1-9: LGTM! Good improvements to the interface.
The changes improve the component's API by:
- Extending PaperProps for better component composition
- Using more semantic prop naming ('label' vs 'left')
Line range hint 18-35: LGTM! Well-structured component with good accessibility.
The component follows good practices:
- Uses semantic form labeling
- Maintains consistent spacing
- Handles optional description gracefully
Let's verify other usages to ensure the prop rename was applied consistently:
✅ Verification successful
LGTM! Component usage is consistent with the current implementation
Looking at the component definition and all its usages, I can confirm that:
- The component correctly uses
labelprop in its interface definition - All instances of
BoxSettingacross the codebase use thelabelprop correctly - There are no remaining instances of any old
leftprop - The component is being used consistently in digest settings, PR title check, and PR size labeler features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old 'left' prop
# Expect: No results, as all instances should use 'label' now
rg -l "BoxSetting.*left="
Length of output: 27
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the component usage
# First, find all files that import or use BoxSetting
rg -l "BoxSetting"
# Then let's look for any prop named 'left' or 'leftElement' in these components
rg "BoxSetting.*(\bleft|\bleftElement)" -A 2
# Also search for the component definition to understand the props
ast-grep --pattern 'interface BoxSettingProps {
$$$
}'
Length of output: 574
Script:
#!/bin/bash
# Let's look at the actual BoxSetting component definition and its props
cat apps/web/src/components/box-setting/box-setting.tsx
# And let's see how it's being used in the other files
cat apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx
cat apps/web/src/app/automations/settings/pr-title-check/components/form-pr-title-check-settings/form-pr-title-check-settings.tsx
cat apps/web/src/app/automations/settings/pr-size-labeler/components/form-pr-size-labeler-settings/form-pr-size-labeler-settings.tsx
Length of output: 9018
apps/web/src/app/teams/[id]/digests/use-digests.tsx (2)
7-14: LGTM! Well-structured interfaces
The interfaces are well-defined with proper TypeScript types and follow good practices for type safety.
24-27: Verify objectify behavior with duplicate digest types
Let's verify how the objectify function from radash handles duplicate keys to ensure we're not silently dropping digests.
✅ Verification successful
No risk of duplicate digest types affecting objectify behavior
The verification shows that:
- DigestType is an enum with only two distinct values (TEAM_METRICS, TEAM_WIP)
- The data comes from a GraphQL query where types are enforced
- Each digest can only have one of these enum values, making duplicates impossible by design
Therefore, using objectify to transform the digests array using type as the key is safe in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or documentation about objectify behavior
rg -A 5 "objectify.*duplicate" node_modules/radash/
Length of output: 114
Script:
#!/bin/bash
# Let's first check if we can find the actual implementation of objectify in the codebase
rg -g '!*.test.*' -g '!*.spec.*' "objectify.*=" -A 5
# Also search for any tests related to objectify
rg -g '*.test.*' -g '*.spec.*' "objectify.*=" -A 5
# Let's also check if there are any other usages of objectify with digests
rg "objectify.*digest" -A 5
Length of output: 1149
Script:
#!/bin/bash
# Let's check the imports to find where objectify is coming from
rg "import.*objectify" -A 2
# Let's also check the DigestType enum definition to understand possible values
ast-grep --pattern 'enum DigestType {
$$$
}'
Length of output: 1857
apps/web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/alert-enable-slack.tsx (1)
1-3: LGTM! Imports are well-organized and properly utilized.
apps/web/src/providers/integration.provider.tsx (2)
1-8: LGTM! Well-organized imports
The imports are properly scoped and follow good practices with named imports.
1-38: Verify integration usage patterns
Let's verify how these hooks are being used across the codebase to ensure consistent patterns.
✅ Verification successful
Based on the search results, I can now generate the final response as the usage patterns are clear:
Integration hooks are used consistently across the codebase
The hooks are being used appropriately throughout the application:
useIntegrationsis used in integration settings pages for managing integrationsuseMessagingIntegrationis used exclusively in team digest related features- No direct access to
workspace.integrationsin the frontend code except through these hooks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hook usage patterns
echo "Searching for useIntegrations usage:"
rg "useIntegrations" -A 3
echo -e "\nSearching for useMessagingIntegration usage:"
rg "useMessagingIntegration" -A 3
echo -e "\nChecking for potential direct workspace.integrations access that should use these hooks:"
rg "workspace\.integrations" -A 3
Length of output: 7449
apps/api/src/app/digests/resolvers/digests.schema.ts (2)
33-36: LGTM! Team type extension is well-defined.
The extension provides both bulk and specific digest querying capabilities, which is a good practice for API design.
42-57: Consider adding settings validation per digest type.
While using JSONObject for settings provides flexibility, it could lead to type-safety issues. Consider adding runtime validation for settings based on the digest type.
Let's check if there are any type-specific settings being used:
Consider creating type-specific input types:
input TeamMetricsSettings {
# Add specific fields for TEAM_METRICS
}
input TeamWIPSettings {
# Add specific fields for TEAM_WIP
}
input UpdateDigestInput {
# ... other fields ...
settings: TeamMetricsSettings | TeamWIPSettings # Use union type
}apps/web/src/components/select-timezone/select-timezone.tsx (2)
1-3: LGTM! Clean and specific imports.
The imports are well-organized and include only the necessary components and icons.
3-3: Verify timezones data structure.
Let's ensure the imported timezones file exists and has the correct data structure.
✅ Verification successful
Timezone data structure is properly implemented
The timezones file exists and follows a valid structure with:
- Organized groups (e.g., "US/Canada")
- Each timezone entry containing label and value properties
- Values using standard IANA timezone format (e.g., "America/Los_Angeles")
- Properly consumed in SelectTimezone component as data prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if timezones file exists and verify its structure
echo "Checking timezones file..."
rg -A 5 "export const timezones" "apps/web/src/components/select-timezone/timezones.ts"
Length of output: 271
Script:
#!/bin/bash
# Let's get more context about the timezones structure
rg -A 15 "export const timezones" "apps/web/src/components/select-timezone/timezones.ts"
# Also check how it's being used in select-timezone component
rg -A 5 "timezones" "apps/web/src/components/select-timezone/select-timezone.tsx"
Length of output: 770
apps/web/src/providers/date.provider.ts (1)
2-2: LGTM: New imports are properly organized.
The new imports from date-fns are correctly placed and necessary for the new functionality.
Also applies to: 10-10
apps/web/src/components/select-hour/select-hour.tsx (2)
1-1: LGTM! Clean and properly typed import.
The import statement correctly imports both the Select component and its type definition.
1-32: Verify component usage across the codebase.
Let's check for potential duplicate implementations and current usage patterns.
✅ Verification successful
Based on the verification results, I can now provide a final response:
SelectHour component is properly implemented and used
The component is:
- Used appropriately in the digest settings form alongside SelectTimezone
- No duplicate time selection implementations found in the codebase
- Properly integrated with Mantine's form system
- Used in a consistent way with other form components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar time/hour selection implementations
echo "Searching for similar time selection implementations..."
rg -l "Select.*hour|hour.*select|time.*select|select.*time" --type tsx
# Search for current usage of SelectHour component
echo -e "\nChecking SelectHour component usage..."
rg "SelectHour" --type tsx
Length of output: 369
Script:
#!/bin/bash
# Search for similar time/hour selection implementations without file type restriction
echo "Searching for similar time selection implementations..."
rg -l "Select.*hour|hour.*select|time.*select|select.*time"
# Search for current usage of SelectHour component
echo -e "\nChecking SelectHour component usage..."
rg "SelectHour"
# List all TypeScript/React files to understand the codebase structure
echo -e "\nListing TypeScript/React files..."
fd "\.(tsx?|jsx?)$"
Length of output: 27261
Script:
#!/bin/bash
# Let's examine the digest-base-fields component where SelectHour is used
echo "Checking digest-base-fields implementation..."
cat "apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx"
# Let's also check if there are any similar time selection patterns
echo -e "\nChecking for similar time selection patterns..."
rg "Select.*time|time.*select" -A 5 -B 5
Length of output: 51081
apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts (2)
1-7: LGTM! Well-organized imports following separation of concerns.
The imports are properly structured, bringing in necessary utilities and services from their respective modules.
19-30: Consider adding input sanitization before updating digest.
The direct passing of input fields to the service layer could be risky. Consider:
- Adding explicit type checking
- Sanitizing input data
- Validating specific fields (e.g., timeOfDay format, timezone validity)
Let's check how input validation is handled in similar mutations:
packages/graphql-types/codegen.ts (2)
13-13: LGTM: TimeZone scalar addition
The TimeZone scalar is appropriately typed as string, which aligns with standard timezone string formats.
Line range hint 31-34: Verify the intentional scalar type differences between API and Frontend configurations
The scalar type mappings differ between API and Frontend configurations:
- API:
SweetID: "number",BigInt: "bigint" - Frontend:
SweetID: "string",BigInt: "number"
While this might be intentional (e.g., for type safety in the API and serialization in the frontend), let's verify this pattern is consistent with other ID and BigInt handling in the codebase.
Also applies to: 49-52
✅ Verification successful
Let me generate another verification script to check the actual implementations and usages of these scalar types:
This will help us understand:
- How these scalar types are actually implemented
- All scalar type configurations across the codebase
- Type definitions that use these scalars
This information will help us verify if the type differences are intentional and properly handled.
Let me try one more search to find scalar type implementations and configurations with a fixed regex:
This should help us understand how these scalar types are implemented and configured across the codebase.
Based on the gathered evidence, I can now provide a final response:
The scalar type differences between API and Frontend configurations are intentional and correct
The type mappings reflect the proper handling of these types across the system:
- In the API (backend),
SweetIDis correctly typed as "number" andBigIntas "bigint" for internal processing - In the Frontend, these are mapped to "string" and "number" respectively for proper serialization over the wire
- This is a common pattern when dealing with large numbers in GraphQL, where the frontend receives them as strings to prevent precision loss during JSON serialization
The schema definitions in apps/api/src/app/scalars/scalars.schema.ts and the base scalar configurations in packages/graphql-types/codegen.ts confirm this is a deliberate architectural decision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of SweetID and BigInt type handling
# Check for SweetID type usage patterns
echo "Checking SweetID type usage patterns..."
rg -A 3 "SweetID.*type" --type ts
# Check for BigInt type usage patterns
echo "Checking BigInt type usage patterns..."
rg -A 3 "BigInt.*type" --type ts
# Check GraphQL schema definitions
echo "Checking GraphQL schema definitions..."
rg -A 3 "scalar (SweetID|BigInt)" --type ts
Length of output: 95742
Script:
#!/bin/bash
# Check scalar type implementations and configurations
echo "Checking scalar type implementations..."
rg -A 5 "class.*SweetIDScalar|class.*BigIntScalar" --type ts
echo "Checking scalar type configurations..."
rg -A 5 "scalars.*=.*{" --type ts
echo "Checking GraphQL type definitions..."
rg -A 5 "type.*SweetID|type.*BigInt" --type ts
Length of output: 87240
Script:
#!/bin/bash
# Check scalar type implementations and configurations with fixed regex
echo "Checking scalar type implementations..."
rg -A 5 "class.*Scalar" --type ts
echo "Checking scalar type configurations..."
rg -A 5 "scalars\s*=\s*" --type ts
echo "Checking GraphQL schema definitions..."
rg -A 5 "scalar\s+(SweetID|BigInt)" --type ts
Length of output: 1127
apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql (2)
25-28: LGTM! Indexes are well-designed
The index choices are appropriate:
- Unique constraint prevents duplicate digest types per team
- Indexes on foreign keys will optimize join operations
30-34: LGTM! RLS implementation follows security best practices
The Row Level Security implementation is solid:
- Proper tenant isolation using workspaceId
- Bypass mechanism for administrative access
Let's verify that similar RLS policies exist on related tables:
✅ Verification successful
RLS implementation is consistent with existing tables
The verification confirms that the RLS implementation for the Digest table follows the same secure pattern used across all workspace-related tables in the system:
- All tables use identical tenant isolation policies based on workspaceId
- All tables have consistent bypass RLS policies using the same app.bypass_rls setting
- The implementation maintains the established security model used in earlier migrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check RLS policies on related tables
rg -A 5 "CREATE POLICY tenant_isolation_policy"
Length of output: 8653
apps/api/src/app/digests/services/digest.types.ts (5)
1-10: LGTM! Clean and well-organized imports.
The imports are properly structured and the alias for Digest as DbDigest helps avoid naming conflicts with the local Digest type.
15-19: LGTM! Well-designed generic interface.
The interface is properly typed with the generic constraint and includes all necessary fields for digest identification.
34-37: LGTM! Clear and focused interface.
The interface contains exactly what's needed for retrieving digest data.
39-45: LGTM! Well-structured interface with proper type utilities.
Good use of Pick utility type and proper typing of nested workspace relationships.
47-50: LGTM! Excellent type mapping implementation.
The type map provides strong type safety when working with different digest types.
apps/api/src/app/digests/resolvers/queries/team-digests.query.ts (2)
1-10: LGTM! Well-organized imports.
The imports are logically grouped and all are being used in the implementation.
12-53: Verify GraphQL schema integration.
The resolver implementation looks functionally correct, but we should verify its integration with the GraphQL schema.
✅ Verification successful
GraphQL schema integration is properly implemented
The resolver implementation is correctly integrated with the GraphQL schema:
- The
Teamtype in the schema correctly defines thedigestanddigestsfields - The
digestfield accepts the requiredDigestQueryInputparameter - The resolver's return types match the schema definitions
- The field resolver is properly created using
createFieldResolver("Team", {...})
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GraphQL schema integration for Team type and Digest fields
# Test: Search for Team type definition in schema files
echo "Searching for Team type definition..."
rg -t graphql "type Team" -A 10
# Test: Search for Digest input type definition
echo "Searching for Digest input type..."
rg -t graphql "input.*Digest.*Type" -A 5
Length of output: 304
Script:
#!/bin/bash
# Let's try searching for schema files and their content without relying on file type
# Find schema files
echo "Looking for schema files..."
fd "schema.ts|schema.graphql|.graphql$"
# Search for Team type definition in all files
echo "Searching for Team type definition..."
rg "type Team" -A 10
# Search for Digest-related types
echo "Searching for Digest types..."
rg "type Digest|input.*Digest" -A 5
# Search for resolver registration to verify if teamDigestsQuery is properly connected
echo "Searching for resolver registration..."
rg "teamDigestsQuery" -A 5
Length of output: 42904
apps/web/src/app/automations/settings/pr-size-labeler/components/form-pr-size-labeler-settings/form-pr-size-labeler-settings.tsx (2)
Line range hint 19-26: LGTM! Clean and semantic implementation.
The BoxSetting usage with the 'label' prop is semantic and well-structured, providing clear context for the enable/disable functionality.
Line range hint 28-54: LGTM! Well-structured implementation with good TypeScript practices.
The code demonstrates several good practices:
- Type-safe iteration using
keyof typeof - Consistent prop usage across BoxSetting components
- Helpful descriptions for each setting
- Clean form field handling with Mantine
apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx (3)
1-2: LGTM! Clean and focused imports.
The imports are minimal and purposeful, importing only what's needed.
40-40: LGTM! Well-typed export.
The type is correctly derived from the digestCards constant, ensuring type safety throughout the application.
42-46: LGTM! Clear interface definition.
The interface clearly defines the expected structure of the hook's return value with proper typing.
apps/web/src/app/settings/integrations/page.tsx (3)
8-8: LGTM: Good architectural improvement
Moving the integration logic to a provider pattern improves code organization and reusability.
18-18: LGTM: Consistent loading state management
The loading state prop update correctly reflects the new hook structure while maintaining proper loading state visualization.
11-11: Verify consistent hook usage across the codebase
The hook's return signature has changed to use a query object pattern. Let's verify this change is consistently applied across all components using this hook.
✅ Verification successful
Hook usage is consistent across the codebase
The search results confirm that all instances of useIntegrations() hook consistently use the new { integrations, query } destructuring pattern. No instances of the old isLoading destructuring pattern were found. The change has been properly propagated across all components using this hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct isLoading destructuring from useIntegrations
# which might indicate inconsistent usage
rg "const.*\{.*isLoading.*\}.*=.*useIntegrations\(\)" --type ts --type tsx
# Search for all useIntegrations usage to verify the new pattern
rg "useIntegrations\(\)" -A 2 --type ts --type tsx
Length of output: 185
Script:
#!/bin/bash
# Search for any remaining direct isLoading destructuring from useIntegrations
# which might indicate inconsistent usage
rg "const.*\{.*isLoading.*\}.*=.*useIntegrations\(\)"
# Search for all useIntegrations usage to verify the new pattern
rg "useIntegrations\(\)" -A 2
Length of output: 881
apps/web/src/app/automations/settings/pr-title-check/components/form-pr-title-check-settings/form-pr-title-check-settings.tsx (3)
Line range hint 1-57: Implementation looks good!
The component is well-structured with:
- Clear form organization and conditional rendering
- Helpful user guidance through descriptions and examples
- Good UX with external documentation link for regex patterns
- Proper form validation and constraints
2-2: Verify the updated import path.
The import path for BoxSetting has been updated to use a deeper relative path. Let's verify this change is correct and consistent.
✅ Verification successful
Import path is correct and consistent
The BoxSetting component is correctly located at apps/web/src/components/box-setting/box-setting.tsx, and the import path "../../../../../../components/box-setting" is consistent with other files using this component. All imports follow the same pattern of reaching up to the components directory from their respective locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the BoxSetting component location and its usage across the codebase
# Test 1: Verify the BoxSetting component exists at the specified path
fd --type f "box-setting.(tsx|ts)" components/
# Test 2: Check for any other imports of BoxSetting to ensure consistency
rg "import.*BoxSetting.*from" --type ts --type tsx
Length of output: 228
Script:
#!/bin/bash
# Let's try searching from the repository root without specifying a directory
# Search for BoxSetting component file
fd "box-setting" .
# Search for BoxSetting imports using ripgrep without file type filters
rg "import.*BoxSetting.*from"
# Search for any file containing BoxSetting class/component definition
ast-grep --pattern 'export const BoxSetting = $_'
Length of output: 7609
18-18: Verify BoxSetting prop API change.
The prop change from left to label suggests an API update. Let's verify this change is consistent with the BoxSetting component's API and other usages.
✅ Verification successful
BoxSetting component correctly uses label prop
The verification confirms that:
- The BoxSetting component's interface explicitly defines
labelas a required prop of typestring | ReactNode - All current usages in the codebase consistently use the
labelprop - There are no instances of the
leftprop being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BoxSetting component prop usage across the codebase
# Test 1: Check BoxSetting component implementation for prop types
ast-grep --pattern 'interface $_ {
$$$
label?: $_
$$$
}' components/box-setting
# Test 2: Check for any remaining usage of the 'left' prop
rg "BoxSetting.*left=" --type ts --type tsx
# Test 3: Check for consistent usage of the new 'label' prop
rg "BoxSetting.*label=" --type ts --type tsx
Length of output: 228
Script:
#!/bin/bash
# Let's try searching from the repository root with different patterns
# Find BoxSetting component file
fd "BoxSetting" .
# Check for BoxSetting component implementation and its props
rg -t typescript -t tsx "interface.*BoxSetting.*Props" -A 10
# Check for usage of 'left' prop
rg -t typescript -t tsx "BoxSetting.*left="
# Check for usage of 'label' prop
rg -t typescript -t tsx "BoxSetting.*label="
Length of output: 278
Script:
#!/bin/bash
# Let's try with simpler ripgrep patterns without file type specifications
# Find BoxSetting component definition and props
rg "interface.*BoxSetting.*Props" -A 10
# Find all BoxSetting usages with either prop
rg "BoxSetting.*(?:left|label)="
# Find the actual BoxSetting component file
fd -e tsx -e ts "BoxSetting"
Length of output: 1355
apps/web/src/api/digest.api.ts (2)
1-18: LGTM! Clean and well-organized imports
The imports are properly structured and include all necessary dependencies for GraphQL operations and React Query integration.
44-77: Verify settings field type and enhance query configuration
The implementation looks good but consider:
- Verify that the
settingsfield type is properly typed in the GraphQL schema - Apply the same reliability improvements as suggested for
useTeamDigestsQuery
apps/web/src/app/automations/settings/pr-title-check/page.tsx (1)
96-96: Consider handling the loading state explicitly.
The removal of conditional rendering for FormPrTitleCheckSettings might cause a flash of default form values before automationSettings loads. Consider either:
- Keeping the conditional render
- Adding a loading state to
FormPrTitleCheckSettings
Let's verify how FormPrTitleCheckSettings handles undefined settings:
Consider applying this diff to handle the loading state:
- <FormPrTitleCheckSettings form={form} />
+ {automationSettings ? (
+ <FormPrTitleCheckSettings form={form} />
+ ) : (
+ <Skeleton h={200} />
+ )}apps/web/src/app/automations/settings/pr-size-labeler/page.tsx (1)
116-116: LGTM! Simplified rendering logic.
The unconditional rendering of FormPrSizeLabelerSettings is a good change that simplifies the code while maintaining proper loading states through the parent LoadableContent component. The form's initialization handles missing data gracefully, making this a safe and clean improvement.
apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (1)
1-24: LGTM! Well-structured imports and type definitions.
The imports are well-organized and the component's props are properly typed with TypeScript.
apps/web/src/app/settings/integrations/slack/page.tsx (3)
19-19: LGTM! Good architectural improvement.
Moving the useIntegrations hook to a centralized provider location improves code organization and reusability.
96-96: LGTM! Loading state logic preserved.
The loading state logic remains correct while adapting to the new query object pattern. The combination with !isIntegrating correctly prevents showing loading state during the integration process.
27-27: Verify consistent query object usage across the codebase.
The change from direct isLoading to query.isLoading suggests a move towards a more structured data fetching pattern. Let's ensure this pattern is consistently applied.
✅ Verification successful
Let me verify the implementation of the useIntegrations hook to ensure the change is properly reflected at its source.
Let me try a different approach to locate and verify the hook implementation.
The change is correctly implemented and consistently used across the codebase
The verification shows that:
- The
useIntegrationshook inintegration.provider.tsxcorrectly returns{ integrations, query }wherequerycontains the loading state fromuseIntegrationsQuery - All three usages of the hook (
settings/integrations/page.tsx,settings/integrations/slack/page.tsx, andintegration.provider.tsx) consistently destructure{ integrations, query } - No instances of the old pattern using direct
isLoadingdestructuring were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of query object from useIntegrations hook
# Test: Search for direct isLoading destructuring vs query object usage
# Expect: All usages should follow the new pattern using query object
echo "Checking for old pattern usage..."
rg "const.*\{.*isLoading.*\}.*=.*useIntegrations\(\)" -g '!*.{json,md}'
echo "Checking for new pattern usage..."
rg "const.*\{.*query.*\}.*=.*useIntegrations\(\)" -g '!*.{json,md}'
Length of output: 606
Script:
#!/bin/bash
# Check the implementation of useIntegrations hook
ast-grep --pattern 'export function useIntegrations() {
$$$
}'
# Also search for any potential interface or type definitions
rg "interface.*Query" -A 5
Length of output: 95
Script:
#!/bin/bash
# Find the file containing useIntegrations hook
fd "integration" --type f --exec rg -l "useIntegrations"
# Look for the actual implementation with more context
rg "useIntegrations" -B 3 -A 10
Length of output: 7584
apps/web/src/components/select-timezone/timezones.ts (1)
1-1: Add documentation for timezone data maintenance.
To improve maintainability:
- Add a comment header explaining the data source
- Document the process for updating timezone data
- Consider adding validation for IANA timezone identifiers
Add a header comment like:
/**
* Timezone data based on IANA Time Zone Database.
* Last updated: <date>
*
* To update this file:
* 1. <update instructions>
* 2. Verify changes against https://www.iana.org/time-zones
*/Let's verify the IANA timezone identifiers:
✅ Verification successful
Let me generate another verification script to get the actual timezone data structure since the previous results seem to be affected by file paths being included in the matches:
Add documentation and validation for timezone data
The file contains valid IANA timezone identifiers organized in a structured format. To improve maintainability:
/**
* Timezone data based on IANA Time Zone Database (https://www.iana.org/time-zones)
* Last updated: November 2024
*
* This file contains a curated list of commonly used timezones organized by geographical regions.
* Each timezone entry includes a user-friendly label and its corresponding IANA identifier.
*
* To update this file:
* 1. Check IANA Time Zone Database for latest changes: https://data.iana.org/time-zones/releases/
* 2. Verify timezone identifiers against the current version
* 3. Update labels and values as needed
* 4. Update the "Last updated" date above
*/🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that timezone values match IANA timezone patterns
# and extract unique timezone identifiers for manual verification
echo "Extracting and validating timezone identifiers..."
rg 'value: "([^"]+)"' -o --replace '$1' | sort -u | while read -r tz; do
if [[ ! $tz =~ ^(Africa|America|Antarctica|Asia|Atlantic|Australia|Europe|Indian|Pacific|Etc)/[A-Za-z_/]+$ ]]; then
echo "Warning: $tz might not be a valid IANA timezone identifier"
fi
done
Length of output: 49004
Script:
#!/bin/bash
# Get the actual timezone data structure
echo "Examining timezone data structure..."
cat apps/web/src/components/select-timezone/timezones.ts
Length of output: 6764
apps/web/src/routes.tsx (2)
24-25: LGTM! Imports follow consistent patterns.
The new imports maintain the established project conventions for page components and follow a logical directory structure.
170-179: Verify navigation integration for new digest routes.
The route configuration looks good and follows the established patterns. However, let's verify the integration with navigation components.
Let's check for navigation references to these new routes:
✅ Verification successful
Navigation integration for new digest routes is properly implemented
The verification shows that:
- The main digest page (
TeamDigestsPage) correctly uses the new routes through thedigestCardsconfiguration - Routes are properly generated via
getRoutefunctions in the digest cards configuration - Navigation is implemented using React Router's
Linkcomponents - The UI properly handles loading states and integration status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to the new digest routes in navigation components
# Look for potential navigation components and links to these routes
rg -l "digests/(metrics|wip)" --type tsx
# Check for any navigation components in the digests directory
fd -e tsx -e ts $ "apps/web/src/app/teams/[id]/digests/"
Length of output: 1139
Script:
#!/bin/bash
# Let's check the content of the digest pages and navigation components
# Check the main digests page for navigation links
cat "apps/web/src/app/teams/[id]/digests/page.tsx"
# Check the card component which might contain navigation
cat "apps/web/src/app/teams/[id]/digests/components/card-digest/card-digest.tsx"
# Check if there are any Link components or navigation hooks being used
rg "useNavigate|Link" "apps/web/src/app/teams/[id]/digests/"
Length of output: 4954
Script:
#!/bin/bash
# Let's check the digest cards configuration to see how routes are generated
# Check the digest cards hook to see route generation
cat "apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx"
# Also check the types and configuration
rg "type DigestType" --type typescript
Length of output: 1840
apps/api/prisma/schema.prisma (1)
166-166: LGTM! Relations are properly defined.
The relations between Workspace, Team, and Digest models are correctly established with appropriate cascading delete behavior.
Also applies to: 377-377
packages/graphql-types/frontend/gql.ts (2)
30-32: LGTM! Well-structured GraphQL operations.
The new document definitions for team digests follow consistent patterns and provide appropriate granularity:
TeamDigestsfor listing basic digest infoTeamDigestfor detailed digest settingsUpdateDigestfor modifications
126-134: LGTM! Type-safe function overloads.
The new overloads are well-implemented:
- Proper type mapping between string literals and document types
- Consistent with the existing pattern in the file
- Ensures type safety for the GraphQL operations
apps/web/src/app/teams/[id]/digests/page.tsx (1)
1-66: Code changes are well-structured and maintainable
The implementation effectively integrates the new digests feature with appropriate loading states and conditional rendering based on the messaging integration. The use of hooks like useDigests, useTeamId, and useMessagingIntegration promotes code modularity and reusability. Overall, the code is clean and follows best practices.
apps/api/src/app/digests/services/digest.service.ts (1)
78-78: Verify Default Value Assignment for enabled Field
In the creation of a new digest, the code assigns enabled: enabled ?? false. If enabled is not provided, it defaults to false, which may inadvertently disable the digest.
Please confirm if defaulting enabled to false aligns with the desired behavior. If new digests should be enabled by default, consider changing the default value to true.
- enabled: enabled ?? false,
+ enabled: enabled ?? true,packages/graphql-types/api.ts (1)
1146-1148: Ensure 'TimeZone' scalar includes validation logic
While the TimeZone scalar type is defined, please confirm that it includes validation to accept only valid time zone identifiers.
packages/graphql-types/frontend/graphql.ts (13)
22-22: New scalar TimeZone added successfully
The TimeZone scalar has been correctly added to the Scalars type, enabling proper handling of time zone strings.
163-171: Enum DayOfTheWeek is well-defined
The DayOfTheWeek enum includes all seven days, providing a comprehensive set of options for scheduling.
173-183: Digest type definition looks good
The Digest type has been defined appropriately with all necessary fields, ensuring proper data structure for digests.
185-187: DigestQueryInput type added correctly
The DigestQueryInput type is properly defined with the required type field for querying digests.
189-193: DigestType enum is properly defined
The DigestType enum with values TEAM_METRICS and TEAM_WIP is correctly introduced.
194-197: Frequency enum added with appropriate values
The Frequency enum includes MONTHLY and WEEKLY, accurately representing possible digest frequencies.
250-250: Mutation updateDigest added to Mutation type
The updateDigest mutation is appropriately added, allowing updates to digest configurations.
290-293: Mutation arguments for updateDigest defined correctly
The MutationUpdateDigestArgs type is correctly specified with the input field of type UpdateDigestInput.
513-514: Added digest and digests fields to Team type
The Team type now includes optional digest and digests fields, enhancing the team's capability to manage digests.
523-527: Defined TeamDigestArgs for digest field
The TeamDigestArgs type is properly defined to accept DigestQueryInput for querying specific digests.
574-586: UpdateDigestInput type added with necessary fields
The UpdateDigestInput type includes all required fields for updating digest configurations, ensuring comprehensive input validation.
777-781: TeamDigestsQueryVariables type defined correctly
The TeamDigestsQueryVariables type is correctly specified, facilitating queries for team digests with workspaceId and teamId.
948-950: GraphQL documents for digest operations are well-formed
The GraphQL documents TeamDigestsDocument, TeamDigestDocument, and UpdateDigestDocument are properly defined, aligning with the new schema structures.
...web/src/app/teams/[id]/digests/settings/components/alert-enable-slack/alert-enable-slack.tsx
Show resolved
Hide resolved
apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (17)
apps/api/src/app/teams/resolvers/transformers/team.transformer.ts (1)
Line range hint
1-12: Consider adding type safety improvements.The transformer could benefit from stronger type safety:
- Use a more specific return type instead of spreading the team object
- Consider making the function generic to handle different team includes
import { Team as DatabaseTeam } from "@prisma/client"; import { Team as ApiTeam } from "@sweetr/graphql-types/dist/api"; +import { Digest as DatabaseDigest } from "@prisma/client"; + +type TeamIncludes = { + digests?: DatabaseDigest[]; + // Add other possible includes here +}; -export const transformTeam = (team: DatabaseTeam): ApiTeam => { +export const transformTeam = <T extends TeamIncludes = {}>( + team: DatabaseTeam & T +): Omit<ApiTeam, keyof T> & Pick<T, keyof T> => { return { - ...team, id: team.id, archivedAt: team.archivedAt?.toISOString(), members: [], digests: [], }; };apps/web/src/app/teams/[id]/digests/settings/types.ts (1)
18-22: Consider type-specific validation rules.The current implementation uses identical schemas for metrics and WIP digests. If these different types of digests might have different requirements in the future, consider:
- Creating separate schemas with specific validation rules
- Using schema composition to extend the base schema
Example approach:
export const FormMetricsDigest = FormDigest.extend({ // Add metrics-specific fields and validation }); export const FormWipDigest = FormDigest.extend({ // Add WIP-specific fields and validation });apps/api/src/app/digests/services/digest.types.ts (1)
12-14: Move DigestSettings type definition before Digest typeThe
DigestSettingstype should be defined before it's used in theDigesttype to maintain a clear type dependency flow.+export type DigestSettings = Prisma.JsonObject; + export type Digest = Omit<DbDigest, "settings"> & { settings: DigestSettings; }; - -export type DigestSettings = Prisma.JsonObject;Also applies to: 58-58
apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql (1)
15-16: Consider additional column improvements.
- Add a default value for timezone:
"timezone" TEXT NOT NULL DEFAULT 'UTC'
- Consider adding JSON schema validation for the settings column:
ALTER TABLE "Digest" ADD CONSTRAINT "valid_settings_json_schema" CHECK (jsonb_validate_schema( 'your_schema_definition_here', settings ));apps/web/src/app/teams/[id]/health-and-performance/page.tsx (1)
16-16: Consider adding type annotation for better type safety.While the code works correctly, adding a type annotation would improve type safety and documentation:
- const teamId = useTeamId(); + const teamId: string = useTeamId();apps/web/src/app/teams/[id]/digests/page.tsx (2)
43-46: Simplify the enabled prop calculationThe enabled prop calculation can be simplified using the logical AND operator.
-enabled={ - (digests?.[digest.type]?.enabled && !!integration) || - false -} +enabled={Boolean(digests?.[digest.type]?.enabled && integration)}
11-68: Consider adding error boundariesSince this component fetches data and renders dynamic content, consider wrapping it with an error boundary to gracefully handle potential runtime errors (e.g., network failures, data parsing issues).
apps/api/src/app/digests/services/digest.service.ts (1)
14-29: Consider adding a unique constraint to prevent race conditionsWhile the function implementation is correct, consider adding a unique constraint on (teamId, type) if not already present in the schema to prevent potential race conditions when multiple requests try to find/create digests simultaneously.
apps/web/src/app/teams/[id]/health-and-performance/components/chart-stacked-bars/chart-stacked-bars.tsx (1)
Line range hint
19-91: Consider performance and lifecycle improvements.While the current implementation works, there are several opportunities for improvement:
- Memoize the chart options to prevent unnecessary recalculations:
const getChartOptions = useCallback((chartData: NumericSeriesChartData, period: Period): ECOption => { // Move options object here return { // ... existing options }; }, []);
- Add cleanup and resize handling:
useEffect(() => { if (!chartData) return; const chartElement = document.getElementById("main"); const chart = echarts.init(chartElement, "dark"); const options = getChartOptions(chartData, period); chart.setOption(options); chart.renderToCanvas(); const handleResize = () => chart.resize(); window.addEventListener('resize', handleResize); return () => { window.removeEventListener('resize', handleResize); chart.dispose(); }; }, [chartData, period, getChartOptions]);These changes will:
- Improve performance by reducing unnecessary option recalculations
- Properly clean up resources when the component unmounts
- Ensure the chart remains responsive to window size changes
apps/web/src/app/teams/[id]/components/subnav-team/subnav-team.tsx (1)
16-16: LGTM! Good type restriction.The change from
Omit<Team, "members">toPick<Team, "id" | "name" | "icon">is a positive improvement that follows the principle of least privilege by explicitly declaring only the required properties.Consider extracting this type if it's used in other components:
type TeamBasicInfo = Pick<Team, "id" | "name" | "icon">;apps/web/src/app/teams/[id]/page.tsx (1)
Line range hint
31-47: LGTM! Well-structured refactoring.The refactoring improves code organization by:
- Centralizing team ID handling logic
- Maintaining consistent data fetching patterns
- Preserving component functionality
This change sets a good precedent for similar refactoring efforts across the application.
Consider applying this pattern to other URL parameter handling across the application for consistency.
apps/web/src/app/teams/[id]/health-and-performance/code-reviews/time-to-approve/page.tsx (1)
24-24: LGTM! Good refactoring.The replacement of
useParamswithuseTeamIdis a good improvement that:
- Centralizes the team ID extraction logic
- Reduces code duplication
- Standardizes error handling across components
Consider adding a comment documenting that this hook handles the error case of missing teamId, to prevent future developers from adding redundant error handling.
apps/web/src/app/teams/[id]/health-and-performance/pull-requests/time-to-merge/page.tsx (1)
Line range hint
25-144: Consider memoizing the closeUrl.While the implementation is correct, the
closeUrlinuseDrawerPageis being reconstructed on every render. Consider memoizing it withuseMemosince it only depends onteamId.Here's how you could optimize it:
+ import { useMemo } from 'react'; const teamId = useTeamId(); const { workspace } = useWorkspace(); + const closeUrl = useMemo(() => `/teams/${teamId}/health-and-performance/`, [teamId]); const drawerProps = useDrawerPage({ - closeUrl: `/teams/${teamId}/health-and-performance/`, + closeUrl, });apps/web/src/app/teams/[id]/health-and-performance/health/code-review-distribution/page.tsx (1)
23-23: LGTM! Good abstraction of team ID retrieval.The switch to
useTeamIdhook is a good improvement that:
- Centralizes the team ID retrieval logic
- Simplifies the component code
- Provides consistent error handling across the application
This is a good example of following the DRY principle by extracting common functionality into a reusable hook.
apps/api/prisma/schema.prisma (1)
78-81: Consider adding DAILY frequency option.The
Frequencyenum currently supportsWEEKLYandMONTHLY. Consider addingDAILYas it's a common requirement for digest systems and might be needed in the future.enum Frequency { + DAILY WEEKLY MONTHLY }packages/graphql-types/frontend/gql.ts (2)
202-202: Consider documenting the purpose of isActiveCustomer field.The addition of
isActiveCustomerto the workspace query is appropriate. Consider adding a comment explaining its usage for future maintainers.
30-32: Consider making digest types reusable across entities.The current implementation scopes digests to teams. If there's a possibility that digests might be needed for other entities in the future (e.g., repositories, workspaces), consider making the digest types and operations more generic or implementing them as interfaces that can be reused.
Also applies to: 123-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (35)
apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql(1 hunks)apps/api/prisma/schema.prisma(4 hunks)apps/api/src/app/billing/services/billing.service.ts(1 hunks)apps/api/src/app/digests/resolvers/queries/team-digests.query.ts(1 hunks)apps/api/src/app/digests/services/digest.service.ts(1 hunks)apps/api/src/app/digests/services/digest.types.ts(1 hunks)apps/api/src/app/digests/transformers/digest.transformer.ts(1 hunks)apps/api/src/app/github/services/github-installation.service.ts(0 hunks)apps/api/src/app/teams/resolvers/transformers/team.transformer.ts(1 hunks)apps/web/src/api/workspaces.api.ts(1 hunks)apps/web/src/app/automations/settings/pr-size-labeler/page.tsx(2 hunks)apps/web/src/app/page.tsx(1 hunks)apps/web/src/app/settings/integrations/components/image-integration-logo/image-integration-logo.tsx(0 hunks)apps/web/src/app/teams/[id]/components/subnav-team/subnav-team.tsx(1 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/digest-loading-skeleton.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx(1 hunks)apps/web/src/app/teams/[id]/digests/settings/types.ts(1 hunks)apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/code-reviews/time-to-approve/page.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/code-reviews/time-to-first-review/page.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/components/chart-stacked-bars/chart-stacked-bars.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/health/code-review-distribution/page.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/page.tsx(2 hunks)apps/web/src/app/teams/[id]/health-and-performance/pull-requests/cycle-time/page.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/pull-requests/size-distribution/page.tsx(1 hunks)apps/web/src/app/teams/[id]/health-and-performance/pull-requests/time-to-merge/page.tsx(1 hunks)apps/web/src/app/teams/[id]/members/page.tsx(1 hunks)apps/web/src/app/teams/[id]/page.tsx(2 hunks)apps/web/src/app/teams/[id]/pull-requests/page.tsx(1 hunks)apps/web/src/providers/date.provider.ts(2 hunks)packages/graphql-types/frontend/gql.ts(4 hunks)packages/graphql-types/frontend/graphql.ts(11 hunks)
💤 Files with no reviewable changes (2)
- apps/api/src/app/github/services/github-installation.service.ts
- apps/web/src/app/settings/integrations/components/image-integration-logo/image-integration-logo.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app/billing/services/billing.service.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/api/src/app/digests/resolvers/queries/team-digests.query.ts
- apps/api/src/app/digests/transformers/digest.transformer.ts
- apps/web/src/app/automations/settings/pr-size-labeler/page.tsx
- apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-loading-skeleton.tsx
- apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx
- apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx
- apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx
- apps/web/src/providers/date.provider.ts
🔇 Additional comments (51)
apps/api/src/app/teams/resolvers/transformers/team.transformer.ts (1)
10-10:
Verify the empty digests array initialization.
The digests array is always initialized as empty, which might not correctly represent the team's associated digests from the database. Consider passing and transforming the actual digest relationships.
Let's verify if the Team type includes digest relationships:
Suggested fix:
-export const transformTeam = (team: DatabaseTeam): ApiTeam => {
+import { Digest as DatabaseDigest } from "@prisma/client";
+import { transformDigest } from "../../digests/resolvers/transformers/digest.transformer";
+
+export const transformTeam = (
+ team: DatabaseTeam & { digests?: DatabaseDigest[] }
+): ApiTeam => {
return {
...team,
id: team.id,
archivedAt: team.archivedAt?.toISOString(),
members: [],
- digests: [],
+ digests: team.digests ? team.digests.map(transformDigest) : [],
};
};apps/web/src/app/teams/[id]/digests/settings/types.ts (2)
1-5: LGTM! Clean and well-organized imports.
The imports are appropriate, utilizing generated GraphQL types for better type safety.
7-14: 🛠️ Refactor suggestion
Strengthen validation rules for better data integrity.
The previous review comment about enhancing form validation is still applicable. Additionally, consider:
- Adding
.trim()to string fields to prevent whitespace-only inputs - Adding a custom error message for the
dayOfTheWeekminimum length requirement
export const FormDigest = z.object({
enabled: z.boolean(),
- channel: z.string().min(1),
+ channel: z.string().trim().min(1, "Channel is required"),
frequency: z.nativeEnum(Frequency),
- dayOfTheWeek: z.array(z.nativeEnum(DayOfTheWeek)).min(1),
+ dayOfTheWeek: z.array(z.nativeEnum(DayOfTheWeek)).min(1, "At least one day must be selected"),
timeOfDay: z.string().min(1),
timezone: z.string().min(1),
});apps/web/src/app/page.tsx (1)
20-22: Verify paywall redirection behavior.
The change in control flow might affect the user experience during paywall redirection. Please ensure that:
goToPaywall()handles navigation internally- There are no race conditions between showing the notification and redirecting
Let's verify the implementation of goToPaywall:
Consider adding a loading state during redirection:
if (shouldShowPaywall) {
showPaywallNotification();
goToPaywall();
- return <></>;
+ return <div aria-busy="true" aria-label="Redirecting to paywall">
+ {/* Optional: Add a loading spinner component here */}
+ </div>;
}✅ Verification successful
Empty fragment return after paywall redirection is correct
The implementation is verified and works as intended:
goToPaywallis a navigation function that redirects to "/settings/billing"- The empty fragment return is appropriate since:
- Navigation happens synchronously via the
navigatefunction - The component doesn't need to render anything during the redirect
- No race conditions exist as both notification and navigation are synchronous operations
- Navigation happens synchronously via the
The suggested loading state is unnecessary since the browser's native loading indicator will show during navigation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of goToPaywall to ensure it handles navigation
# Test: Search for the implementation and usage of goToPaywall
rg -A 5 "goToPaywall" --type typescript
Length of output: 74
Script:
#!/bin/bash
# Let's try different search approaches to find goToPaywall implementation
# 1. Search without file type restriction
rg -A 5 "goToPaywall"
# 2. Search for files with .ts or .tsx extension containing goToPaywall
fd -e ts -e tsx -x rg -A 5 "goToPaywall" {}
# 3. Use ast-grep to find function declarations/exports
ast-grep --pattern 'export function goToPaywall'
ast-grep --pattern 'function goToPaywall'
ast-grep --pattern 'const goToPaywall'
Length of output: 1206
apps/api/src/app/digests/services/digest.types.ts (2)
48-56: Previous review comment about specific settings types is still applicable
The suggestion to define specific settings interfaces for each digest type remains valid.
28-29: Validate frequency and dayOfTheWeek relationship
The current type definition allows any combination of frequency and dayOfTheWeek. Consider adding runtime validation or type constraints to ensure:
- Weekly frequency requires exactly one dayOfTheWeek
- Daily frequency should ignore dayOfTheWeek
- Monthly frequency might need a different day specification
apps/api/prisma/migrations/20241106021412_create_digests_table/migration.sql (5)
1-4: LGTM! Enum types are well-defined.
The enum types are correctly defined with appropriate values for digest types, days of the week, and frequency options.
7-21: Previous comment about missing constraints is still applicable.
The concerns about missing foreign key constraints and data validation remain unaddressed.
23-24: Previous comment about missing updatedAt trigger is still applicable.
The concern about automatic updates for the updatedAt column remains unaddressed.
26-29: LGTM! Indexes are well-designed.
The combination of indexes will effectively support:
- Uniqueness constraint for digest types per team
- Foreign key lookups
- Query performance optimization
31-35: LGTM! RLS implementation follows security best practices.
The combination of tenant isolation and bypass policies provides:
- Strong multi-tenant data separation
- Administrative access when needed
apps/web/src/app/teams/[id]/members/page.tsx (3)
3-3: LGTM: Import changes align with the new architecture
The import changes correctly reflect the transition from route parameters to the new team ID management approach.
Also applies to: 8-8
Line range hint 15-21: LGTM: Clean query implementation with proper conditional execution
The query implementation with conditional enabling based on teamId presence is a good practice. The component maintains proper loading and empty state handling.
12-12: Verify removal of teamId error handling
The removal of the ResourceNotFound error handling changes how missing team IDs are handled. Please confirm:
- Is this intentional as part of the architectural changes?
- Is error handling moved to the
useTeamIdhook? - How are invalid team IDs handled now?
Let's check the implementation of useTeamId:
apps/web/src/app/teams/[id]/health-and-performance/page.tsx (2)
2-2: LGTM! Good refactoring of team ID retrieval.
The switch to a custom useTeamId hook is a good architectural decision as it:
- Encapsulates team ID retrieval logic
- Reduces duplicate error handling across components
- Makes the code more maintainable
Also applies to: 13-13
Line range hint 19-80: LGTM! Well-structured component with clear organization.
The component demonstrates good practices:
- Logical grouping of metrics by category
- Clear, descriptive labels and descriptions
- Consistent URL structure for navigation
apps/web/src/app/teams/[id]/digests/page.tsx (4)
1-9: LGTM: Well-organized imports
The imports are logically grouped and each imported entity serves a clear purpose in the component.
12-15: LGTM: Clean hooks implementation
The hooks usage follows React best practices with proper dependency management and clear value destructuring.
66-66: LGTM: Proper nested routing setup
The Outlet component is correctly placed for handling nested routes within the TeamDigestsPage.
36-36: Verify the grow-on-hover class implementation
Ensure that the grow-on-hover class is properly defined in your CSS/styles.
✅ Verification successful
The grow-on-hover class is properly implemented
The class is correctly defined in apps/web/src/styles.css with appropriate hover effects including scale transform, box shadow, and cursor changes. It's also consistently used across multiple components in the codebase for interactive elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the grow-on-hover class definition
rg -l "grow-on-hover" --type css --type scss
Length of output: 73
Script:
#!/bin/bash
# Search for the grow-on-hover class definition in all files, including Tailwind classes
rg "grow-on-hover" -A 3 -B 3
# Also check for any CSS module files or styled-components
fd -e css -e scss -e styled.ts -e styled.tsx
Length of output: 8439
Script:
#!/bin/bash
# Let's check the actual implementation of grow-on-hover class
cat apps/web/src/styles.css
Length of output: 223
apps/web/src/api/workspaces.api.ts (1)
73-73: LGTM! Verify the field usage in digest-related components.
The addition of isActiveCustomer field maintains consistency with the UserWorkspaces query and follows the established pattern.
Let's verify how this field is used in digest-related components:
✅ Verification successful
The isActiveCustomer field is correctly used to gate digest functionality
The search results confirm that the isActiveCustomer field is used appropriately to control access to digest features:
- In the backend:
// apps/api/src/app/digests/services/digest.service.ts
export const canSendDigest = async ({
digest,
workspace,
}: CanSendDigestArgs): Promise<boolean> => {
if (!digest?.enabled) return false;
if (!isActiveCustomer(workspace)) return false;
// ...
};- In the frontend:
// apps/web/src/providers/billing.provider.tsx
const shouldShowPaywall =
!workspace.isActiveCustomer &&
!["/settings/billing", "/settings/workspace"].includes(pathname);The field is used consistently across the codebase to gate digest functionality behind active customer status, making its addition to the WorkspaceByInstallationId query appropriate and aligned with the existing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components that might use isActiveCustomer for digest-related functionality
# Search for files that reference both isActiveCustomer and digest
rg -l 'isActiveCustomer.*digest|digest.*isActiveCustomer' --type ts --type tsx
# Search for potential conditional rendering based on isActiveCustomer
ast-grep --pattern 'if ($expr.isActiveCustomer) { $$$ }'
Length of output: 167
Script:
#!/bin/bash
# Let's try a different approach to find usage of isActiveCustomer
# Search for files containing isActiveCustomer
rg "isActiveCustomer" --type typescript -C 3
# Find TypeScript/TSX files containing digest-related code
rg "digest" --type typescript -C 3
Length of output: 149
Script:
#!/bin/bash
# Let's try without file type filtering since rg doesn't recognize typescript
# Search for files containing isActiveCustomer
rg "isActiveCustomer" -C 3
# Find files containing digest-related code
rg "digest" -C 3
Length of output: 103671
apps/api/src/app/digests/services/digest.service.ts (2)
31-42: LGTM!
The function implementation is clean and straightforward.
106-115: LGTM!
The function has comprehensive checks in an optimal order (checking simple conditions first).
apps/web/src/app/teams/[id]/components/subnav-team/subnav-team.tsx (1)
Line range hint 73-79: Verify the digests route implementation.
The navigation link to the digests feature is enabled, suggesting this is an active feature. Let's verify that the corresponding route and components are properly implemented.
✅ Verification successful
Based on the results from both script executions, I can now generate a final response:
Digests feature is properly implemented with complete component structure
The digests feature is fully implemented with:
- A complete page component (
TeamDigestsPage) with proper routing via React Router - Comprehensive component hierarchy including cards, settings, and Slack integration
- Proper data fetching through custom hooks (
useDigests,useDigestsCards) - Support for both available and future digest types
- Integration with team messaging services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of digests-related components and routes
# Test 1: Look for digests route definition
echo "Checking for digests route definition..."
rg -l "path.*digests" --type ts --type tsx
# Test 2: Look for digests-related components
echo "Checking for digests-related components..."
fd -e tsx -e ts --full-path ".*digest.*"
# Test 3: Look for digests-related GraphQL operations
echo "Checking for digests-related GraphQL operations..."
rg -l "query.*digest|mutation.*digest" --type graphql
Length of output: 1890
Script:
#!/bin/bash
# Let's verify the route implementation and component structure
# Check the page component implementation
echo "Checking the digests page component..."
cat "apps/web/src/app/teams/[id]/digests/page.tsx"
# Check the route definition in the most common route definition files
echo "Checking route definitions..."
rg -l "path.*=.*digest" --type=ts --type=tsx
# Look for any imports of the digests page
echo "Checking imports of digests page..."
rg "import.*digests/page" --type=ts --type=tsx
Length of output: 2811
apps/web/src/app/teams/[id]/page.tsx (2)
15-15: LGTM! Clean import changes.
The import changes reflect a good separation of concerns by moving team ID handling logic into a dedicated hook.
Also applies to: 27-27
31-31: Verify error handling in useTeamId hook.
The refactoring moves error handling to the useTeamId hook. Let's verify its implementation to ensure all edge cases are covered.
✅ Verification successful
Error handling in useTeamId hook is properly implemented
The hook implementation is straightforward and handles the missing teamId case appropriately:
- Throws
ResourceNotFoundexception (404 status) when teamId param is missing - Used consistently across multiple team-related pages
- Exception class follows the standard error pattern with proper status code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of useTeamId hook
# Expected: Should find the hook implementation with proper error handling
# Search for the useTeamId implementation
ast-grep --pattern 'export const useTeamId = () => {
$$$
}'
# Search for error handling in the hook
rg -A 5 'ResourceNotFound.*teamId'
Length of output: 529
Script:
#!/bin/bash
# Let's verify the complete implementation context and error handling
# Check the imports and ResourceNotFound definition
rg -p "ResourceNotFound" apps/web/src/app/teams/\[id\]/use-team.ts -B 5
# Check if there are any tests for the hook
fd "use-team.test" -e ts -e tsx
# Look for other usages of useTeamId to understand the error handling expectations
rg "useTeamId"
Length of output: 3236
Script:
#!/bin/bash
# Let's check the ResourceNotFound exception implementation and its usage pattern
# Check the ResourceNotFound exception implementation
rg -p "class ResourceNotFound" -A 5
# Check if there's any error boundary component that handles this exception
rg -p "ResourceNotFound.*catch" -A 5
# Look for any tests related to ResourceNotFound
rg "ResourceNotFound.*test"
Length of output: 580
apps/web/src/app/teams/[id]/health-and-performance/pull-requests/cycle-time/page.tsx (2)
Line range hint 26-29: LGTM! Consistent usage of teamId.
The teamId is properly utilized in both the drawer navigation and the chart query parameters.
Also applies to: 45-54
21-21: LGTM! Good abstraction of team ID handling.
The change to use a custom useTeamId hook improves code maintainability by centralizing the team ID retrieval logic.
Let's verify the hook implementation:
Also applies to: 24-24
✅ Verification successful
Verified: useTeamId hook is well-implemented with proper error handling
The hook implementation in apps/web/src/app/teams/[id]/use-team.ts shows:
- Extracts teamId from URL params
- Throws
ResourceNotFoundif teamId is missing - Used consistently across team-related pages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useTeamId hook implementation and its error handling
# Test: Check the hook implementation
ast-grep --pattern 'export function useTeamId() {
$$$
}'
# Test: Check for error handling in the hook
rg -A 5 "useTeamId"
Length of output: 13259
apps/web/src/app/teams/[id]/health-and-performance/code-reviews/time-to-approve/page.tsx (1)
21-21: LGTM!
Clean import of the new useTeamId hook.
apps/web/src/app/teams/[id]/health-and-performance/code-reviews/time-to-first-review/page.tsx (2)
21-21: LGTM!
The import of the custom useTeamId hook follows the project's import conventions.
24-24: Verify error handling in useTeamId hook.
The change simplifies teamId retrieval by using a custom hook, which is good for maintainability. However, since the explicit error handling for missing teamId was removed, we should verify that the useTeamId hook properly handles this case.
Let's verify the error handling in the custom hook:
✅ Verification successful
Error handling in useTeamId hook is properly implemented
The useTeamId hook in apps/web/src/app/teams/[id]/use-team.ts has appropriate error handling:
- It extracts teamId from URL parameters using
useParams() - It explicitly checks for missing teamId with
if (!teamId) - It throws a
ResourceNotFounderror if teamId is missing
This is actually better error handling than the previous explicit check, as it provides a consistent error boundary for all components using this hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of useTeamId hook
# Expected: Find error handling logic in the hook implementation
# Search for the useTeamId hook implementation
ast-grep --pattern 'export const useTeamId = () => {
$$$
}'
# Search for any error handling or validation logic
rg -A 5 'useTeamId'
Length of output: 13691
apps/web/src/app/teams/[id]/health-and-performance/pull-requests/size-distribution/page.tsx (2)
22-22: LGTM: Clean import addition
The import of the custom useTeamId hook is properly placed and follows the project's import structure.
25-25: Verify consistent error handling across useTeamId implementations
The switch to useTeamId hook improves code maintainability by centralizing the team ID retrieval logic. However, let's verify that the error handling is consistent across all usages.
✅ Verification successful
Error handling is consistent across useTeamId implementations
The verification shows that error handling is consistent:
- The
useTeamIdhook inuse-team.tsproperly throwsResourceNotFoundwhenteamIdis missing - This matches the existing error handling pattern in components using the hook
- The change maintains the same guarantees as the previous implementation using
useParams
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling consistency in useTeamId hook usage
# Check the implementation of useTeamId hook
echo "Checking useTeamId implementation:"
ast-grep --pattern 'export const useTeamId = () => { $$$ }'
# Check for similar error handling patterns across components
echo "\nChecking error handling patterns:"
rg "if.*!teamId.*throw.*ResourceNotFound" -A 2
Length of output: 1645
apps/web/src/app/teams/[id]/health-and-performance/pull-requests/time-to-merge/page.tsx (1)
22-22: LGTM! Clean refactor to use the custom hook.
The change from useParams to useTeamId looks good and maintains the existing error handling behavior. This refactor improves code organization by centralizing the team ID retrieval logic.
Let's verify this refactor is consistent across other team-related components:
Also applies to: 25-25
✅ Verification successful
The refactor to useTeamId is consistently applied across the codebase
The verification shows that:
- The
useTeamIdhook is properly implemented inapps/web/src/app/teams/[id]/use-team.ts, maintaining the same error handling behavior as before - The hook is consistently used across multiple team-related pages and components
- No direct usage of
useParamsfor team ID extraction was found in team-related components - The refactor successfully centralizes the team ID retrieval logic, making it more maintainable
The changes are safe and improve code organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the adoption of useTeamId across team-related components
# Test: Search for any remaining useParams usage in team-related components
# Expect: No direct team ID extraction using useParams
# Check for any remaining useParams usage in team-related files
rg -g '*/teams/*' 'useParams.*id.*'
# Verify the consistent usage of useTeamId
rg -g '*/teams/*' 'useTeamId'
Length of output: 365
Script:
#!/bin/bash
# Let's try a different approach to search for files
# First, find all TypeScript/JavaScript files under teams directory
fd "\.(ts|tsx|js|jsx)$" "teams" | while read -r file; do
echo "=== $file ==="
# Check for useParams usage
rg "useParams.*id" "$file" || true
# Check for useTeamId usage
rg "useTeamId" "$file" || true
done
# Also search for the useTeamId hook definition
fd "use-team\.(ts|tsx|js|jsx)$" || true
Length of output: 213
Script:
#!/bin/bash
# Let's try with the full path pattern
fd "\.(ts|tsx|js|jsx)$" "apps/web/src/app/teams" | while read -r file; do
echo "=== $file ==="
# Check for useParams usage
rg "useParams.*id" "$file" || true
# Check for useTeamId usage
rg "useTeamId" "$file" || true
done
# Also check the implementation of useTeamId
echo "=== useTeamId Implementation ==="
cat "apps/web/src/app/teams/[id]/use-team.ts"
Length of output: 20903
apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (3)
1-25: LGTM: Clean imports and well-typed props interface.
The imports are properly organized and the props interface is well-defined with appropriate typing.
27-36: LGTM: Well-implemented initialization and focus management.
The useEffect hook correctly manages focus on the channel input when needed, providing a good user experience.
53-61: Previous review comment about Slack channel validation is still applicable.
apps/web/src/app/teams/[id]/health-and-performance/health/code-review-distribution/page.tsx (1)
20-20: LGTM!
The import statement is correctly placed and uses an appropriate relative path.
apps/web/src/app/teams/[id]/pull-requests/page.tsx (2)
31-31: LGTM: Clean import of the new hook.
The import statement is properly placed and follows the correct relative path convention.
34-34: Verify error handling in useTeamId hook.
The switch to useTeamId hook simplifies the code, but we need to ensure proper error handling is implemented within the hook since the explicit check for falsy teamId was removed from this component.
Let's check the implementation of the useTeamId hook:
✅ Verification successful
Based on the gathered information from all the executed scripts, I can now provide the final response:
Error handling in useTeamId hook is properly implemented
The useTeamId hook (in apps/web/src/app/teams/[id]/use-team.ts) has appropriate error handling:
- It checks for falsy
teamIdvalues - Throws a
ResourceNotFounderror when the team ID is not available - The error is a proper custom exception class (defined in
apps/web/src/exceptions/resource-not-found.exception.ts)
The removal of explicit error handling from the component is justified since the validation has been centralized in the hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in useTeamId hook
# Expected: Hook should handle cases where teamId is not available
# Look for the hook implementation
ast-grep --pattern 'export const useTeamId = () => {
$$$
}'
# Look for error handling patterns within the hook
rg -A 5 'throw.*Error.*team'
Length of output: 523
Script:
#!/bin/bash
# Let's verify the ResourceNotFound error implementation
rg -A 2 "class ResourceNotFound"
# Also check where this error is caught/handled
ast-grep --pattern 'try {
$$$
} catch (error) {
$$$
}'
# Check for error boundary components
rg -i "errorboundary|error.?boundary"
Length of output: 23166
Script:
#!/bin/bash
# Let's check for error handling at the page/layout level that might catch the ResourceNotFound error
rg -A 5 "error.?page|error.?layout" apps/web/src/app
# Also check for any global error handling setup
rg -A 5 "ErrorBoundary|error-boundary" apps/web/src/app
# Check if there's any specific handling for the ResourceNotFound error
rg -A 5 "catch.*ResourceNotFound" apps/web/src/app
Length of output: 165
apps/api/prisma/schema.prisma (3)
171-171: LGTM: Relations are properly defined.
The relations between Digest and both Workspace and Team models are correctly established with appropriate cardinality.
Also applies to: 382-382
408-434: Previous validation suggestions are still applicable.
The field validation suggestions from the previous review are still relevant for this model.
431-433: LGTM: Good constraint and index design.
The unique constraint @@unique([teamId, type]) ensures that a team can only have one digest of each type, which is a good design decision. The indexes on workspaceId and teamId will help with query performance.
packages/graphql-types/frontend/gql.ts (2)
30-32: LGTM: New GraphQL operations for team digests.
The new operations are well-structured and follow the established patterns in the codebase. They properly handle team digest querying and updates through the workspace->team hierarchy.
123-134: LGTM: Type-safe function overloads for digest operations.
The new function overloads maintain type safety and follow the established pattern for GraphQL operation typing.
packages/graphql-types/frontend/graphql.ts (6)
22-22: Well-structured type definitions for digest configuration!
The new scalar and enum types are well-defined and follow consistent naming conventions:
TimeZonescalar appropriately maps to stringDayOfTheWeekenum comprehensively covers all daysDigestTypeenum clearly identifies digest categoriesFrequencyenum provides standard time intervals
Also applies to: 163-171, 189-192, 194-197
173-183: Well-designed Digest type with comprehensive configuration fields!
The Digest type includes all necessary fields for configuring team digests:
- Integration settings (channel)
- Schedule configuration (dayOfTheWeek, timeOfDay, timezone, frequency)
- Control fields (enabled, type)
- Extensible settings as JSONObject
513-514: Clean integration of digest fields into Team type!
The Team type has been properly extended with:
- Optional
digestfield for querying specific digest types digestsfield for retrieving all team digests- Well-structured arguments for digest queries
Also applies to: 524-526
185-187: Well-defined input types for digest operations!
The input types are properly structured:
DigestQueryInput: Focused input for digest queriesUpdateDigestInput: Comprehensive input for digest configuration with all necessary fields
Also applies to: 574-585
777-783: Well-designed GraphQL operations for digest management!
The operations provide a complete set of functionality:
TeamDigests: List digests with basic infoTeamDigest: Get detailed digest configurationUpdateDigest: Modify digest settings
Also applies to: 785-792, 794-799
Line range hint 948-967: Auto-generated DocumentNode definitions
These are automatically generated type definitions that match the GraphQL operations defined above.
| export const upsertDigest = async ({ | ||
| workspaceId, | ||
| teamId, | ||
| type, | ||
| enabled, | ||
| channel, | ||
| frequency, | ||
| dayOfTheWeek, | ||
| timeOfDay, | ||
| timezone, | ||
| settings, | ||
| }: UpsertDigest) => { |
There was a problem hiding this comment.
Add input validation for time-related fields
The function accepts frequency, dayOfTheWeek, and timeOfDay without validation. Consider adding validation to ensure:
frequencymatches expected values (e.g., daily, weekly)dayOfTheWeekis between 0-6timeOfDayfollows a valid time format
Example validation:
const validateDigestInput = ({
frequency,
dayOfTheWeek,
timeOfDay,
}: Pick<UpsertDigest, 'frequency' | 'dayOfTheWeek' | 'timeOfDay'>) => {
if (frequency && !['daily', 'weekly'].includes(frequency)) {
throw new Error('Invalid frequency');
}
if (dayOfTheWeek !== undefined && (dayOfTheWeek < 0 || dayOfTheWeek > 6)) {
throw new Error('Day of week must be between 0 and 6');
}
if (timeOfDay && !/^([01]\d|2[0-3]):([0-5]\d)$/.test(timeOfDay)) {
throw new Error('Time must be in HH:mm format');
}
};Also applies to: 71-74
...c/app/teams/[id]/health-and-performance/components/chart-stacked-bars/chart-stacked-bars.tsx
Show resolved
Hide resolved
...web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx
Show resolved
Hide resolved
...web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (14)
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts (2)
1-8: Consider adding Slack-specific error handling.While the
ResourceNotFoundExceptionis appropriate for missing channels, consider handling Slack-specific errors (rate limits, authentication issues, etc.) for better error reporting and recovery.Add additional error types and import them:
import { SlackAuthenticationError, SlackRateLimitError, // ... other specific errors } from "../../integrations/slack/errors";
9-22: Consider implementing retry logic for resilience.Given that this service interacts with external APIs (Slack), consider implementing retry logic with exponential backoff for transient failures. This would improve the reliability of the digest delivery system.
Consider using a retry utility:
import { retry } from '../../utils/retry'; // Wrap the send operation in retry logic const result = await retry( async () => { // existing send logic }, { maxAttempts: 3, backoffMs: 1000, exponential: true, } );apps/api/src/app/digests/services/digest-team-metrics.service.ts (2)
9-22: Add logging for monitoring and debugging.Consider adding logging statements at key points in the workflow to aid in monitoring and debugging:
- When the digest sending starts
- After successful channel join
- When the message is sent
- If any errors occur
export const sendTeamMetricsDigest = async (digest: DigestWithWorkspace) => { + logger.info('Starting team metrics digest', { workspaceId: digest.workspaceId, channel: digest.channel }); const { slackClient } = await getWorkspaceSlackClient(digest.workspaceId); const slackChannel = await joinSlackChannel(slackClient, digest.channel); + logger.debug('Joined Slack channel', { channelId: slackChannel?.id }); if (!slackChannel?.id) { + logger.error('Channel not found', { channel: digest.channel }); throw new ResourceNotFoundException("Slack channel not found"); } await sendSlackMessage(slackClient, { channel: slackChannel.id, text: "Hello, world!", }); + logger.info('Successfully sent team metrics digest', { channelId: slackChannel.id }); };
9-22: Consider implementing retry logic for resilience.Since this is an integration with an external service (Slack), consider implementing retry logic for transient failures:
- Add retry mechanisms for:
- Slack client initialization
- Channel joining
- Message sending
- Consider using an exponential backoff strategy
- Set appropriate timeout values
Would you like me to provide an example implementation using a retry utility?
apps/api/src/app/digests/workers/digest-send.worker.ts (2)
1-9: Consider adding error handling utilities.While the imports are well-organized, consider importing error handling utilities to properly handle and log potential failures when sending digests.
Add these imports for better error handling:
import { CustomError } from '../../../lib/errors'; import { ErrorReporter } from '../../../lib/error-reporting';
10-12: Consider adding worker configuration for production reliability.The worker would benefit from explicit configuration for production use:
- Retry strategy for handling temporary failures
- Concurrency limits to prevent overload
- Job timeout settings
Consider adding these configurations:
export const digestSendWorker = createWorker( SweetQueue.DIGEST_SEND, async (job: Job<DigestWithWorkspace>) => { // ... existing implementation }, { concurrency: 5, // Process 5 jobs at a time timeout: 30000, // 30 seconds timeout attempts: 3, // Retry failed jobs 3 times backoff: { type: 'exponential', delay: 1000 // Start with 1 second delay } } );apps/api/src/app/digests/services/digest.types.ts (1)
35-41: Consider stricter typing for workspace relationships.The interface could be more explicit about required fields for sending digests.
export interface CanSendDigestArgs { digest: Pick<Digest, "enabled">; workspace: Workspace & { - subscription: Subscription | null; - installation: Installation | null; + subscription: Subscription; // If required for sending + installation: Installation; // If required for sending }; }apps/api/src/app/integrations/slack/services/slack-client.service.ts (5)
1-7: LGTM! Consider adding JSDoc documentation.The imports are appropriate and the base client creation is clean. Consider adding JSDoc documentation to clarify the purpose of this utility function.
+/** + * Creates a new instance of Slack WebClient without authentication. + * Primarily used for authorization flow where no token is needed. + */ export const getSlackClient = () => new WebClient();
26-36: Add configuration validation for OAuth credentials.The OAuth implementation is correct, but it should validate the required configuration values.
export const authorizeSlackWorkspace = ( slackClient: WebClient, code: string ) => { + if (!config.slack.clientId || !config.slack.clientSecret || !config.slack.redirectUrl) { + throw new Error("Missing required Slack OAuth configuration"); + } + return slackClient.oauth.v2.access({ client_id: config.slack.clientId, client_secret: config.slack.clientSecret, redirect_uri: config.slack.redirectUrl, code, }); };
38-43: Add type safety and error handling for uninstallation.The uninstallation implementation should include similar configuration validation and proper type definitions.
-export const uninstallSlackWorkspace = async (slackClient: WebClient) => { +export const uninstallSlackWorkspace = async ( + slackClient: WebClient +): Promise<void> => { + if (!config.slack.clientId || !config.slack.clientSecret) { + throw new Error("Missing required Slack configuration for uninstallation"); + } + return slackClient.apps.uninstall({ client_id: config.slack.clientId, client_secret: config.slack.clientSecret, }); };
54-71: LGTM! Consider adding specific error types.The implementation is solid with good error handling and membership validation.
Consider creating specific error types for different failure scenarios:
class SlackChannelNotFoundException extends Error { constructor(channelName: string) { super(`Slack channel "${channelName}" not found`); } } class SlackChannelJoinException extends Error { constructor(channelName: string, error: unknown) { super(`Failed to join Slack channel "${channelName}": ${error}`); } }
1-78: Consider adding monitoring and logging infrastructure.Given the critical nature of external API communications, consider implementing:
- Request/response logging for debugging
- Metrics collection for API calls
- Circuit breaker pattern for fault tolerance
Example implementation using a decorator:
function monitorSlackApi(target: any, propertyKey: string, descriptor: PropertyDescriptor) { const originalMethod = descriptor.value; descriptor.value = async function (...args: any[]) { const startTime = Date.now(); try { const result = await originalMethod.apply(this, args); metrics.recordApiCall('slack', propertyKey, 'success', Date.now() - startTime); return result; } catch (error) { metrics.recordApiCall('slack', propertyKey, 'error', Date.now() - startTime); logger.error(`Slack API error in ${propertyKey}:`, error); throw error; } }; return descriptor; }apps/api/src/app/integrations/slack/services/slack-integration.service.ts (2)
Line range hint
62-82: Consider wrapping operations in a transactionWhile the refactoring improves the code organization, consider wrapping the uninstall and delete operations in a transaction to ensure data consistency if either operation fails.
export const removeIntegration = async (workspace: Workspace) => { try { const { slackClient, integration } = await getWorkspaceSlackClient( workspace.id ); + await getPrisma(workspace.id).$transaction(async (tx) => { await uninstallSlackWorkspace(slackClient); - await getPrisma(workspace.id).integration.delete({ + await tx.integration.delete({ where: { id: integration.id, }, }); + }); } catch (error) { throw new IntegrationException( "Slack Uninstall failed. Please try again.", { originalError: error, } ); } };
Line range hint
97-117: Consider adding error handling for jsonObject accessThe use of jsonObject improves type safety, but consider adding error handling for the case where the team name is undefined.
return { app: IntegrationApp.SLACK, isEnabled: true, enabledAt: integration.createdAt.toISOString(), - target: jsonObject(integration.data).team?.name, + target: jsonObject(integration.data).team?.name ?? 'Unknown Team', };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
apps/api/package.json(1 hunks)apps/api/src/app/digests/services/digest-schedule.service.ts(1 hunks)apps/api/src/app/digests/services/digest-team-metrics.service.ts(1 hunks)apps/api/src/app/digests/services/digest-team-wip-digest.service.ts(1 hunks)apps/api/src/app/digests/services/digest.service.ts(1 hunks)apps/api/src/app/digests/services/digest.types.ts(1 hunks)apps/api/src/app/digests/workers/cron-schedule-digests.worker.ts(1 hunks)apps/api/src/app/digests/workers/digest-send.worker.ts(1 hunks)apps/api/src/app/integrations/services/integrations.service.ts(1 hunks)apps/api/src/app/integrations/slack/services/slack-client.service.ts(1 hunks)apps/api/src/app/integrations/slack/services/slack-integration.service.ts(4 hunks)apps/api/src/app/integrations/slack/workers/slack-app-uninstalled.worker.ts(1 hunks)apps/api/src/bull-mq/queues.ts(2 hunks)apps/api/src/bull-mq/schedule-cron-jobs.ts(1 hunks)apps/api/src/config/slack.config.ts(1 hunks)apps/api/src/lib/slack.ts(0 hunks)apps/api/src/prisma.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/lib/slack.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app/integrations/services/integrations.service.ts
🔇 Additional comments (23)
apps/api/src/config/slack.config.ts (2)
8-8: LGTM! The new Slack scopes align with digest feature requirements.
The added read-only scopes (channels:read, groups:read, mpim:read, im:read, reactions:read) follow the principle of least privilege and are appropriate for implementing team digest functionality, allowing the application to:
- List channels and groups for digest targeting
- Access direct/group messages for metrics
- Read reactions for engagement metrics
8-8: Verify scope completeness for digest feature.
Let's ensure all required scopes are included and no unnecessary scopes remain.
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Slack scopes are correctly configured for current functionality
The current scope configuration aligns with the actual Slack API usage in the codebase:
channels:join- Used injoinSlackChannelfunctionchannels:read,groups:read,mpim:read,im:read- Used forconversations.listchat:write,im:write- Used forchat.postMessageusers:read,users:read.email,users.profile:read- Required for user informationapp_mentions:read- For bot mentionsreactions:read- For reaction tracking
All scopes are actively used and no unnecessary permissions are included.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Slack API calls in the codebase to verify scope requirements
# Look for Slack API method calls in digest-related files
echo "Searching for Slack API calls in digest services..."
rg -A 2 "slack\.(conversations|chat|reactions|users|groups|im)" "apps/api/src/app/digests"
# Look for any TODO/FIXME comments mentioning Slack permissions
echo "Checking for TODO comments about Slack permissions..."
rg -i "todo.*slack.*(permission|scope|access)" "apps/api/src/app/digests"
# Look for error handling related to Slack API permissions
echo "Checking for Slack permission error handling..."
rg "not_in_channel|missing_scope|invalid_auth" "apps/api/src/app/digests"
Length of output: 573
Script:
#!/bin/bash
# Let's broaden our search to find Slack API usage across the codebase
# Search for Slack API method calls in the entire codebase
echo "Searching for Slack API method calls..."
rg -A 2 "\.slack\.(conversations|chat|reactions|users|groups|im|channels)\." --type ts
# Search for Slack WebClient instantiation and usage
echo "Searching for Slack WebClient usage..."
rg -A 2 "WebClient|@slack/web-api" --type ts
# Search for any Slack-related error handling
echo "Checking for Slack-related error handling..."
rg "SlackError|WebAPICallError" --type ts
Length of output: 3617
Script:
#!/bin/bash
# Let's check the actual Slack API method calls and their usage
echo "Searching for Slack API method calls in slack-client.service.ts..."
cat apps/api/src/app/integrations/slack/services/slack-client.service.ts
echo "Searching for any other files that might use Slack API..."
rg -l "slack\." --type ts
echo "Checking the Slack configuration file..."
cat apps/api/src/config/slack.config.ts
Length of output: 3318
apps/api/src/app/integrations/slack/workers/slack-app-uninstalled.worker.ts (2)
Line range hint 7-15: LGTM! Worker implementation remains clean and focused
The worker implementation is well-structured:
- Handles a single responsibility (uninstallation cleanup)
- Includes appropriate logging
- Has proper null-safety check for team_id
5-5: Verify the refactored service import
The import path has been updated to reflect a service refactoring. Let's verify the new service exists and maintains the same contract.
✅ Verification successful
Import path refactoring verified successfully
The service has been cleanly refactored:
- New service file exists at the correct location
- Function signature and implementation are properly maintained
- No remaining references to the old service path in the Slack integration module
- The function maintains the same contract, deleting integrations by team ID
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new service file exists and the function signature remains unchanged
# Check if the new service file exists
fd "slack-integration.service.ts" apps/api/src/app/integrations/slack/services
# Verify the function signature in both old and new files
echo "Function signature in old service:"
rg -A 2 "removeIntegrationByTeamId" apps/api/src/app/integrations/slack/services/slack.service.ts
echo "Function signature in new service:"
rg -A 2 "removeIntegrationByTeamId" apps/api/src/app/integrations/slack/services/slack-integration.service.ts
Length of output: 748
Script:
#!/bin/bash
# Let's verify if this is a clean refactoring by checking for any other files that might still be using the old import path
# Search for any references to the old service path
rg -l "slack\.service" apps/api/src/app/integrations/slack/
# Check the function implementation in the new service to ensure it's complete
rg -A 5 "removeIntegrationByTeamId" apps/api/src/app/integrations/slack/services/slack-integration.service.ts
Length of output: 367
apps/api/src/bull-mq/schedule-cron-jobs.ts (1)
21-25: Consider making the cron pattern configurable and adding logging.
The implementation looks good, but here are some suggestions to improve maintainability and observability:
- Move the cron pattern "0 * * * *" to the config file to make it easily configurable
- Add logging to track when jobs are scheduled
- Consider adding error handling around the
addJobcall
Here's a suggested improvement:
+ import { logger } from "../logger";
- await addJob(SweetQueue.CRON_SCHEDULE_DIGESTS, null, {
- repeat: {
- pattern: "0 * * * *",
- },
- });
+ try {
+ await addJob(SweetQueue.CRON_SCHEDULE_DIGESTS, null, {
+ repeat: {
+ pattern: config.digests.cronPattern ?? "0 * * * *",
+ },
+ });
+ logger.info("Scheduled digest cron job successfully");
+ } catch (error) {
+ logger.error("Failed to schedule digest cron job", { error });
+ throw error;
+ }Let's verify if the cron pattern is already defined in the config:
✅ Verification successful
Consider adding error handling, but cron pattern should remain hardcoded
After reviewing the codebase, I'm adjusting my initial suggestion. The digest scheduling system is designed with a sophisticated time-based scheduling mechanism in digest-schedule.service.ts that:
- Uses configurable frequency (weekly/monthly)
- Respects timezone settings
- Allows flexible day and time configurations per digest
The hourly cron job is intentionally fixed as it only triggers the scheduler to check for digests that should run at that hour. The actual scheduling logic is handled by the service layer.
Suggested improvement:
+ import { logger } from "../logger";
+ try {
await addJob(SweetQueue.CRON_SCHEDULE_DIGESTS, null, {
repeat: {
pattern: "0 * * * *",
},
});
+ logger.info("Scheduled digest cron job successfully");
+ } catch (error) {
+ logger.error("Failed to schedule digest cron job", { error });
+ throw error;
+ }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if digest cron pattern exists in config
rg -g '**/*config*' -l 'digests.*cronPattern|DIGEST.*CRON'
Length of output: 60
Script:
#!/bin/bash
# Let's check for any config files first
fd -t f "config" --exec echo {}
# Then let's check for any environment variables related to digests or cron
fd -t f "env" --exec echo {}
# Finally, let's look for any digest-related configuration patterns
rg -g '!node_modules' -l "digest|DIGEST"
Length of output: 2485
Script:
#!/bin/bash
# Let's check the content of app config and env files for any digest-related configuration
cat apps/api/src/config/app.config.ts
cat apps/api/src/env.ts
# Let's also check the digest service and worker files for any existing configuration patterns
cat apps/api/src/app/digests/services/digest.service.ts
cat apps/api/src/app/digests/workers/cron-schedule-digests.worker.ts
Length of output: 6697
Script:
#!/bin/bash
# Let's check the digest schedule service to understand the scheduling logic
cat apps/api/src/app/digests/services/digest-schedule.service.ts
# And check if there are any existing digest-related environment variables
rg "DIGEST|CRON" apps/api/src/env.ts
Length of output: 2600
apps/api/src/app/digests/workers/cron-schedule-digests.worker.ts (2)
1-4: LGTM! Imports are well-organized and properly scoped.
The imports are cleanly organized and properly sourced from internal modules.
11-20: Consider architectural improvements for scalability.
The current implementation processes all digests in a single batch, which might cause issues with large datasets. Consider these architectural improvements:
- Implement batching for large digest sets
- Add data validation before queueing
- Implement locking mechanism to prevent concurrent executions
Let's verify the potential impact:
apps/api/src/app/digests/services/digest-team-metrics.service.ts (1)
1-8: LGTM! Well-structured imports with proper error handling.
The imports are well-organized, using specific imports rather than wildcards, and the custom exception for error handling shows good separation of concerns.
apps/api/src/app/digests/workers/digest-send.worker.ts (1)
7-8: Verify service implementations for consistent error handling.
Let's verify that the imported service functions implement proper error handling and match the expected signatures.
✅ Verification successful
Service implementations have proper error handling and consistent signatures
Both service functions implement proper error handling through:
- Explicit error handling for missing Slack channels using
ResourceNotFoundException - Consistent async/await usage for all external operations
- Identical function signatures accepting
DigestWithWorkspacetype - Proper error propagation from the imported Slack integration services
The implementations are well-structured and follow consistent patterns across both services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check service implementations for proper error handling
# Check service function implementations
ast-grep --pattern 'export async function sendTeamWipDigest($_) {
$$$
}'
ast-grep --pattern 'export async function sendTeamMetricsDigest($_) {
$$$
}'
# Look for try-catch blocks in these files
rg -l "try\s*{" "apps/api/src/app/digests/services/digest-team-wip-digest.service.ts" "apps/api/src/app/digests/services/digest-team-metrics.service.ts"
Length of output: 386
Script:
#!/bin/bash
# Let's try a different approach to verify the service implementations
# First, check if these service files exist
fd "digest-team-wip-digest.service.ts|digest-team-metrics.service.ts" apps/api/src/app/digests/services/
# Check the content of these files if they exist
rg -A 5 "export.*function sendTeamWipDigest" apps/api/src/app/digests/services/
rg -A 5 "export.*function sendTeamMetricsDigest" apps/api/src/app/digests/services/
# Look for try-catch blocks with a simpler pattern
rg "try.*{" apps/api/src/app/digests/services/
Length of output: 532
Script:
#!/bin/bash
# Let's examine the content of these service files directly
# Check the content of the wip digest service
echo "=== Content of digest-team-wip-digest.service.ts ==="
cat apps/api/src/app/digests/services/digest-team-wip-digest.service.ts
echo -e "\n=== Content of digest-team-metrics.service.ts ==="
cat apps/api/src/app/digests/services/digest-team-metrics.service.ts
Length of output: 1836
apps/api/src/app/digests/services/digest.types.ts (5)
1-14: LGTM! Clean imports and well-defined base type.
The imports are comprehensive and the Digest type is properly defined with a custom settings field.
16-20: LGTM! Well-designed generic interface.
The interface properly uses generics to ensure type safety when finding digests.
22-33: Skip comment as past review is still valid.
The previous review comment about enhancing type safety for the UpsertDigest interface is still applicable.
43-56: Skip comment as past review is still valid.
The previous review comment about defining specific settings types for each digest type is still applicable.
58-58: Skip comment as covered by previous reviews.
The suggestion to improve type safety for settings is already covered in previous review comments.
apps/api/src/prisma.ts (2)
2-3: LGTM: Appropriate imports for JSON handling
The imports are well-chosen for type safety (JsonValue) and object detection (isObject).
55-59: 🛠️ Refactor suggestion
Add error handling and type validation to the JSON parser
The current implementation could throw unhandled exceptions and might not guarantee type safety.
Consider this safer implementation:
export const jsonObject = <T extends Record<string, any>>(
value: string | object | JsonValue
): T => {
- return isObject(value) ? value : JSON.parse(value as string);
+ try {
+ const parsed = isObject(value) ? value : JSON.parse(value as string);
+
+ // Basic validation that we have an object
+ if (!isObject(parsed)) {
+ throw new Error('Parsed value is not an object');
+ }
+
+ return parsed as T;
+ } catch (error) {
+ throw new Error(`Failed to parse JSON object: ${error.message}`);
+ }
};Let's verify how this function might be used with JSONB fields:
apps/api/package.json (1)
45-45: LGTM! Appropriate addition for timezone support.
The addition of date-fns-tz is well-justified for the new Digest feature's timezone handling requirements. The version is compatible with the existing date-fns dependency.
Let's verify the timezone functionality is properly implemented:
✅ Verification successful
Timezone handling is properly implemented across the codebase
The date-fns-tz package is being used correctly in the Digest feature:
- Proper import and usage in
digest-schedule.service.tswithtoZonedTimefunction - Timezone support is integrated throughout the feature:
- GraphQL schema includes timezone field
- Frontend components handle timezone selection
- Browser timezone detection is implemented
- Timezone data is properly passed through the API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper timezone handling implementation
# Look for timezone-related code in the codebase
# Search for timezone-related imports
echo "Checking timezone imports:"
rg "from 'date-fns-tz'" --type ts
# Search for common timezone conversion function usage
echo -e "\nChecking timezone function usage:"
rg "formatInTimeZone|zonedTimeToUtc|utcToZonedTime" --type ts
# Look for timezone-related tests
echo -e "\nChecking timezone-related tests:"
rg "timezone|timeZone" --type test
Length of output: 398
Script:
#!/bin/bash
# Let's try a more comprehensive search for timezone-related code
# Search for date-fns-tz imports with different quote styles and extensions
echo "Checking date-fns-tz imports:"
rg "from ['\"]date-fns-tz['\"]" -g "*.{ts,js,tsx,jsx}"
# Search for timezone-related function usage with broader patterns
echo -e "\nChecking timezone-related code:"
rg "timeZone|timezone|formatIn|zonedTime|utcTo" -g "*.{ts,js,tsx,jsx}"
# Look for timezone-related tests in common test file patterns
echo -e "\nChecking timezone-related tests:"
rg "timeZone|timezone" -g "*.{test,spec}.{ts,js,tsx,jsx}"
# List files that might be related to the Digest feature
echo -e "\nChecking Digest feature files:"
fd -e ts -e js -e tsx -e jsx "digest" -i
Length of output: 8904
apps/api/src/app/digests/services/digest.service.ts (2)
29-40: LGTM! Clean and straightforward implementation.
The function is well-structured and correctly handles the team digest retrieval.
1-11: LGTM! Well-organized imports and clear type definitions.
The file structure is clean and imports are properly organized.
apps/api/src/bull-mq/queues.ts (2)
26-28: LGTM! Verify worker implementation.
The DIGEST_SEND queue is properly organized in a new "Digests" section and follows the naming convention.
Let's verify the worker implementation:
#!/bin/bash
# Description: Check the worker implementation for DIGEST_SEND queue
# Search for the worker file and its processor
rg -A 10 "DIGEST_SEND.*processor"
# Search for the job type definition
ast-grep --pattern 'type DigestWithWorkspace = {
$$$
}'13-13: LGTM! Verify cron schedule configuration.
The CRON_SCHEDULE_DIGESTS queue name follows the established pattern for cron jobs.
Let's verify the cron schedule configuration:
✅ Verification successful
LGTM! Cron schedule configuration verified.
The CRON_SCHEDULE_DIGESTS queue is properly configured to run hourly with the cron pattern "0 * * * *" in schedule-cron-jobs.ts. The implementation is consistent with other cron jobs in the system and has a corresponding worker implementation in cron-schedule-digests.worker.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the cron schedule configuration for CRON_SCHEDULE_DIGESTS
# Search for cron schedule configuration
rg -A 5 "CRON_SCHEDULE_DIGESTS.*schedule"
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for cron schedule configuration
rg -l "cron\.schedule_digests"
# Also search for any cron schedule patterns
rg "CRON_SCHEDULE_DIGESTS" -A 5 -B 5
Length of output: 2691
apps/api/src/app/integrations/slack/services/slack-integration.service.ts (2)
1-16: LGTM! Clean import organization
The imports are well-organized and properly reflect the refactored Slack client functionality.
Line range hint 18-60: Verify error handling coverage
The refactoring to use authorizeSlackWorkspace improves code organization while maintaining robust error handling. The upsert operation ensures atomic updates.
Let's verify the error handling coverage:
✅ Verification successful
Let me verify the error handling implementation by checking the authorizeSlackWorkspace function and its error handling patterns.
Let me try a different approach to find the authorizeSlackWorkspace implementation.
Let me check the complete implementation of the authorizeSlackWorkspace function to verify the error handling chain.
Error handling is properly implemented and layered
The error handling implementation is well-structured with appropriate layers:
authorizeSlackWorkspaceuses Slack's WebClient which throws native errorsinstallIntegrationcatches these errors and wraps them inIntegrationExceptionwith context- Two distinct error scenarios are handled:
- API call failures (try-catch block)
- Invalid response status (!response.ok)
The error handling follows a consistent pattern across the integration service, providing meaningful error messages and preserving error context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error handling patterns across related files
rg -A 2 "IntegrationException"
Length of output: 2513
Script:
#!/bin/bash
# Check authorizeSlackWorkspace implementation
ast-grep --pattern 'authorizeSlackWorkspace($_, $_) {
$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Search for authorizeSlackWorkspace function definition
rg -A 5 "authorizeSlackWorkspace" --type ts
Length of output: 1858
Script:
#!/bin/bash
# Get the complete implementation of authorizeSlackWorkspace
rg -A 15 "^export const authorizeSlackWorkspace" apps/api/src/app/integrations/slack/services/slack-client.service.ts
Length of output: 592
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
apps/api/src/app/people/services/people.service.ts (1)
87-87: Consider extracting GitHub URL to configurationThe TODO comment indicates future support for other Git providers. To make this easier:
- Extract the base URL to configuration
- Consider creating an enum or type for supported Git providers
Would you like me to help create a more provider-agnostic implementation or create an issue to track this enhancement?
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts (4)
27-32: Consider implementing rate limiting handling for Slack API callsWhen sending messages to Slack, consider implementing retry logic with exponential backoff to handle rate limits. The Slack API has rate limits that could affect the digest delivery during high load.
81-84: Simplify type definition using type aliasThe type definition for
pullRequestsparameter is complex and hard to read. Consider creating a type alias.type GroupedPullRequests = Awaited<ReturnType<typeof getPullRequestsGroupedByState>>; type PullRequestGroup = GroupedPullRequests[keyof GroupedPullRequests]; const getPullRequestSectionBlock = ( header: string, pullRequests: PullRequestGroup ) => {
109-117: Extract size label logic to a separate functionThe conditional rendering of the size label could be extracted to improve readability and maintainability.
const getSizeLabelElement = (size?: string) => { if (!size) return []; return [{ type: "text", text: capitalize(size), style: { code: true } }]; }; // Usage: ...elements: [ ...getSizeLabelElement(pullRequest.tracking?.size), { type: "link", text: ` ${pullRequest.title}`, url: pullRequest.gitUrl, },
171-187: Optimize state grouping logicThe current implementation uses multiple array iterations. Consider using
reducefor a more efficient grouping operation.const groupedPRs = pullRequests.reduce((acc, pr) => { if (pr.state === PullRequestState.DRAFT) { acc.drafted.push(pr); } else if (pr.codeReviews.some(review => review.state === CodeReviewState.APPROVED)) { acc.approved.push(pr); } else { acc.open.push(pr); } return acc; }, { drafted: [], open: [], approved: [] } as Record<string, typeof pullRequests>); return groupedPRs;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts(1 hunks)apps/api/src/app/people/services/people.service.ts(2 hunks)
🔇 Additional comments (1)
apps/api/src/app/people/services/people.service.ts (1)
7-7: LGTM: Import addition is appropriate
The GitProfile type import is correctly added alongside existing Prisma imports.
apps/api/src/app/digests/services/digest-team-wip-digest.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
apps/api/src/app/digests/workers/digest-send.worker.ts (1)
16-16: Enhance logging for better observability.Consider adding structured logging with additional context and job completion status:
- logger.info("digestSendWorker", { extra: { digest } }); + logger.info("digestSendWorker:started", { + extra: { + digestId: digest.id, + type: digest.type, + workspaceId: digest.workspaceId + } + });Also, add completion logging at the end of the worker:
logger.info("digestSendWorker:completed", { extra: { digestId: digest.id, type: digest.type, duration: Date.now() - startTime } });apps/api/src/app/digests/services/digest-schedule.service.ts (3)
13-14: Consider configuring the monthly digest windowThe hardcoded value
currentDayOfMonth < 8assumes monthly digests should only run in the first week. Consider making this configurable or documenting the business requirement.+// Configuration for monthly digest scheduling +const MONTHLY_DIGEST_MAX_DAY = 7; + export const findScheduledDigests = async (date: Date): Promise<Digest[]> => { const currentDayOfMonth = getDate(date); - const includeMonthly = currentDayOfMonth < 8; + const includeMonthly = currentDayOfMonth <= MONTHLY_DIGEST_MAX_DAY;
44-48: Enhance error context in exception handlingThe error message could be more specific about which validation failed.
captureException( new InputValidationException("Digest has invalid day of the week", { - extra: { digest }, + extra: { + digestId: digest.id, + invalidDay: getDay(zonedDate), + timezone: digest.timezone, + }, })
72-78: Consider additional validations in canSendDigestThe function could benefit from more comprehensive validation checks.
export const canSendDigest = (digest: DigestWithWorkspace): boolean => { if (!isActiveCustomer(digest.workspace)) return false; if (!digest.workspace.installation) return false; + // Validate digest configuration + if (!digest.dayOfTheWeek?.length) return false; + if (!digest.timeOfDay) return false; + if (!digest.timezone) return false; + return true; };apps/api/src/app/digests/services/digest-team-wip-digest.service.ts (3)
36-43: Add validation for empty digest contentThe function should validate the grouped pull requests and potentially return a meaningful message when there are no PRs to report.
const getDigestMessageBlocks = async ( digest: DigestWithWorkspace ): Promise<AnyBlock[]> => { const { drafted, open, approved } = await getPullRequestsGroupedByState( digest.workspaceId, digest.teamId ); + + const hasContent = drafted.length > 0 || open.length > 0 || approved.length > 0; + if (!hasContent) { + return [ + { + type: "section", + text: { + type: "plain_text", + text: "No work in progress found for your team at this time.", + }, + }, + ]; + }
81-86: Simplify complex type annotationThe return type annotation is unnecessarily complex and could be simplified for better readability.
-const getPullRequestSectionBlock = ( - header: string, - pullRequests: Awaited< - ReturnType<typeof getPullRequestsGroupedByState> - >[keyof Awaited<ReturnType<typeof getPullRequestsGroupedByState>>] -) => { +type GroupedPullRequests = Awaited<ReturnType<typeof getPullRequestsGroupedByState>>; +type PullRequestGroup = GroupedPullRequests[keyof GroupedPullRequests]; + +const getPullRequestSectionBlock = ( + header: string, + pullRequests: PullRequestGroup +) => {
164-213: Optimize database query and add type safetyThe function could benefit from index hints and stronger type safety for the grouped results.
Consider adding an index hint for the query:
-- Add to your migrations CREATE INDEX idx_pull_requests_team_status ON "PullRequest" ("teamId", "mergedAt", "createdAt") WHERE "mergedAt" IS NULL;Also, consider adding type safety:
interface GroupedPullRequests { drafted: PullRequest[]; open: PullRequest[]; approved: PullRequest[]; } const getPullRequestsGroupedByState = async ( workspaceId: number, teamId: number ): Promise<GroupedPullRequests> => { // ... existing implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/api/.env.example(1 hunks)apps/api/src/app/digests/services/digest-schedule.service.ts(1 hunks)apps/api/src/app/digests/services/digest-team-wip-digest.service.ts(1 hunks)apps/api/src/app/digests/services/digest.types.ts(1 hunks)apps/api/src/app/digests/workers/digest-send.worker.ts(1 hunks)apps/api/src/app/integrations/slack/services/slack-client.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/.env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/digests/services/digest.types.ts
🔇 Additional comments (5)
apps/api/src/app/digests/workers/digest-send.worker.ts (2)
1-10: LGTM! Imports are well-organized and complete.
The imports cover all necessary dependencies for the worker implementation, including queue management, logging, type definitions, and service functions.
11-32: Previous review comment about error handling remains applicable.
The implementation still needs improvements in error handling and job processing as mentioned in the previous review.
apps/api/src/app/digests/services/digest-schedule.service.ts (2)
31-35: 🛠️ Refactor suggestion
Replace type casting with type guards
Using type assertions (as) can mask potential type mismatches. Consider using type guards instead.
- const scheduledDigests = digests.filter((digest) =>
- isScheduled(digest as DigestWithWorkspace, date)
- );
-
- return scheduledDigests as DigestWithWorkspace[];
+ function isDigestWithWorkspace(digest: any): digest is DigestWithWorkspace {
+ return digest.workspace !== undefined;
+ }
+
+ const scheduledDigests = digests
+ .filter(isDigestWithWorkspace)
+ .filter((digest) => isScheduled(digest, date));
+
+ return scheduledDigests;Likely invalid or redundant comment.
39-41: Verify timezone handling across the codebase
Let's ensure consistent timezone handling across the application.
✅ Verification successful
Timezone handling is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timezone-related code to verify consistency
echo "Searching for timezone handling patterns..."
# Look for date-fns-tz usage
rg "date-fns-tz" -l
# Look for timezone conversion patterns
rg "toZonedTime|formatInTimeZone" -A 2
# Look for timezone storage/configuration
rg "timezone.*=|timezone.*:" -A 2
Length of output: 12771
apps/api/src/app/integrations/slack/services/slack-client.service.ts (1)
82-87: Implement retries and error handling for sending messages
The sendSlackMessage function does not handle potential errors from chat.postMessage. This could lead to unhandled exceptions if the message fails to send.
Refer to the previous review comment regarding adding error handling and a retry mechanism for message sending.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
apps/api/src/app/digests/services/digest-team-metrics.types.ts (1)
1-5: Add JSDoc documentation and consider type refinementsThe interface structure is clean and well-organized. However, consider these improvements:
- Add JSDoc documentation to explain:
- The purpose of the interface
- Usage examples
- Property descriptions
- Consider type refinements:
- If
valuealways represents numeric data, consider usingnumbertype- Add readonly modifier if properties shouldn't be modified after creation
Here's a suggested implementation:
+/** + * Represents elements of a metric line used in team digest messages. + * + * @example + * { + * label: "Pull Requests", + * value: "42", + * change: 5 + * } + */ -export interface MetricLineElements { +export interface MetricLineElements { + /** The display name or identifier of the metric */ label: string; + /** The current value of the metric */ value: string; + /** The change in value compared to the previous period */ change: number; }apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx (4)
1-13: LGTM! Consider enhancing type safety for the route parameter.The interface definition is well-structured. However, we could improve type safety for the
teamIdparameter in thegetRoutemethod.Consider using a more specific type for
teamId:- getRoute: (teamId: string) => string; + getRoute: (teamId: UUID) => string;
32-33: Fix grammatical issue in description.The description has a minor grammatical error.
- "Sends a digest of Pull Requests that are in draft or awaiting for review or merge." + "Sends a digest of Pull Requests that are in draft or awaiting review or merge."
48-59: Consider consistent naming between export and usage.The hook name has a slight inconsistency between its export name and its usage in the interface.
- export const useDigestsCards = (): UseDigestCards => { + export const useDigestCards = (): UseDigestCards => {This would make it consistent with:
- The file name (
use-digest-cards.tsx)- The interface name (
UseDigestCards)- The constant name (
digestCards)
15-38: Consider making digest cards configuration more maintainable.The digest cards configuration is currently hardcoded in the component. As the number of digest types grows, this could become harder to maintain.
Consider:
- Moving the digest cards configuration to a separate configuration file
- Implementing a factory pattern for creating digest cards
- Adding validation for required properties when creating new digest types
This would make it easier to:
- Add new digest types
- Maintain consistency across digest configurations
- Test digest card configurations independently
apps/api/src/app/digests/services/digest-team-metrics.service.ts (3)
32-37: Address TODO comment about metrics data.The TODO comment indicates that metrics data implementation is pending. This is a critical component that needs to be implemented.
Would you like help implementing the metrics data collection? This could include:
- Defining the metrics calculation logic
- Setting up data aggregation
- Implementing the date range calculations
170-172: Enhance change indicator logic.The current emoji logic is binary (better/worse). Consider adding more granular indicators:
const getChangeEmoji = (change: number) => { - return change < 0 ? "🟠 " : "🟢 "; + if (change === 0) return "⚪️ "; + if (change > 15) return "🟢 "; + if (change > 0) return "🟡 "; + if (change > -15) return "🟠 "; + return "🔴 "; };
174-180: Improve change label formatting.The change label could be more descriptive and handle edge cases better.
const getChangeLabel = (change: number) => { if (change === 0) { return "0% change"; } - return `${Math.abs(change)}% ${change > 0 ? "better" : "worse"}`; + const absChange = Math.abs(change); + const direction = change > 0 ? "better" : "worse"; + if (absChange >= 100) { + return `${absChange}% ${direction} (significant change)`; + } + return `${absChange}% ${direction}`; };apps/api/src/app/digests/services/digest-team-wip-digest.service.ts (1)
44-58: Enhance digest header with more informative summaryThe header and summary sections could be more informative by including PR counts and making the text configurable.
+interface DigestHeaderConfig { + title?: string; + summaryTemplate?: string; +} + return [ { type: "header", text: { type: "plain_text", - text: "Work In Progress Digest", + text: digest.settings?.title || "Work In Progress Digest", }, }, { type: "section", text: { type: "plain_text", - text: "Summary of the team's open pull requests.", + text: digest.settings?.summaryTemplate?.replace( + '{total}', + `${drafted.length + open.length + approved.length}` + ) || `Found ${drafted.length} drafted, ${open.length} open, and ${approved.length} approved PRs.`, }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/api/src/app/digests/services/digest-team-metrics.service.ts(1 hunks)apps/api/src/app/digests/services/digest-team-metrics.types.ts(1 hunks)apps/api/src/app/digests/services/digest-team-wip-digest.service.ts(1 hunks)apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx(1 hunks)
🔇 Additional comments (2)
apps/web/src/app/teams/[id]/digests/use-digest-cards.tsx (1)
24-24: Verify the existence of digest image assets.
The code references image assets that should exist in the public directory.
Also applies to: 35-35
✅ Verification successful
Image assets are present but path needs adjustment
The image assets exist at apps/web/public/images/digests/ but the code references them from /images/digests/. This is correct since the public directory in Next.js apps is served at the root path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the referenced image assets exist in the public directory
# Search for the image files
fd -t f "metrics.webp|wip.webp" public/images/digests/
# If no files found, check if they might be in a different location
fd -t f "metrics.webp|wip.webp"
Length of output: 284
apps/api/src/app/digests/services/digest-team-metrics.service.ts (1)
1-12: LGTM! Imports are well-organized and necessary.
The imports are properly structured and all appear to be used within the implementation.
| return [ | ||
| { | ||
| type: "header", | ||
| text: { | ||
| type: "plain_text", | ||
| text: "Metrics Digest", | ||
| }, | ||
| }, | ||
| { | ||
| type: "section", | ||
| text: { | ||
| type: "plain_text", | ||
| text: "Avg of Sept 15th—Sept 21st (vs Sept 8th—Sept 14th).", | ||
| }, | ||
| }, | ||
| { | ||
| type: "divider", | ||
| }, | ||
| { | ||
| type: "rich_text", | ||
| elements: [ | ||
| { | ||
| type: "rich_text_section", | ||
| elements: getMetricLineElements({ | ||
| label: "📂 PR Size", | ||
| value: "321 lines", | ||
| change: 7, | ||
| }), | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| type: "divider", | ||
| }, | ||
| { | ||
| type: "rich_text", | ||
| elements: [ | ||
| { | ||
| type: "rich_text_section", | ||
| elements: getMetricLineElements({ | ||
| label: "⏱️ PR Cycle Time", | ||
| value: "32 hours", | ||
| change: -12, | ||
| }), | ||
| }, | ||
| { | ||
| type: "rich_text_list", | ||
| elements: [ | ||
| { | ||
| type: "rich_text_section", | ||
| elements: getMetricLineElements({ | ||
| label: "Time to First Review", | ||
| value: "4 hours", | ||
| change: 12, | ||
| }), | ||
| }, | ||
| { | ||
| type: "rich_text_section", | ||
| elements: getMetricLineElements({ | ||
| label: "Time to Approve", | ||
| value: "12 hours", | ||
| change: -50, | ||
| }), | ||
| }, | ||
| { | ||
| type: "rich_text_section", | ||
| elements: getMetricLineElements({ | ||
| label: "Time to Merge", | ||
| value: "1 hour", | ||
| change: 0, | ||
| }), | ||
| }, | ||
| ], | ||
| style: "bullet", | ||
| indent: 1, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| type: "divider", | ||
| }, | ||
| { | ||
| type: "actions", | ||
| elements: [ | ||
| { | ||
| type: "button", | ||
| text: { | ||
| type: "plain_text", | ||
| text: "Explore Metrics", | ||
| }, | ||
| url: `${env.FRONTEND_URL}/teams/${encodeId(digest.teamId)}/digests/wip`, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Replace hardcoded values with dynamic data.
The message blocks contain hardcoded values that should be replaced with actual metrics:
- Date ranges (Sept 15th—Sept 21st)
- PR Size (321 lines)
- PR Cycle Time (32 hours)
- Time metrics (4 hours, 12 hours, 1 hour)
Consider implementing a metrics calculation service to provide these values dynamically.
| export const sendTeamMetricsDigest = async (digest: DigestWithWorkspace) => { | ||
| const { slackClient } = await getWorkspaceSlackClient(digest.workspaceId); | ||
|
|
||
| const slackChannel = await joinSlackChannel(slackClient, digest.channel); | ||
|
|
||
| if (!slackChannel?.id) { | ||
| throw new ResourceNotFoundException("Slack channel not found"); | ||
| } | ||
|
|
||
| const blocks = await getDigestMessageBlocks(digest); | ||
|
|
||
| await sendSlackMessage(slackClient, { | ||
| channel: slackChannel.id, | ||
| blocks, | ||
| unfurl_links: false, | ||
| unfurl_media: false, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive error handling and retry mechanism.
While the channel validation is good, the function needs more robust error handling for Slack API operations.
Consider implementing this more resilient version:
export const sendTeamMetricsDigest = async (digest: DigestWithWorkspace) => {
+ try {
const { slackClient } = await getWorkspaceSlackClient(digest.workspaceId);
- const slackChannel = await joinSlackChannel(slackClient, digest.channel);
+ const slackChannel = await retry(
+ async () => await joinSlackChannel(slackClient, digest.channel),
+ { maxAttempts: 3, backoff: 'exponential' }
+ );
if (!slackChannel?.id) {
throw new ResourceNotFoundException("Slack channel not found");
}
const blocks = await getDigestMessageBlocks(digest);
- await sendSlackMessage(slackClient, {
- channel: slackChannel.id,
- blocks,
- unfurl_links: false,
- unfurl_media: false,
- });
+ await retry(
+ async () => await sendSlackMessage(slackClient, {
+ channel: slackChannel.id,
+ blocks,
+ unfurl_links: false,
+ unfurl_media: false,
+ }),
+ { maxAttempts: 3, backoff: 'exponential' }
+ );
+ } catch (error) {
+ // Log the error for monitoring
+ console.error('Failed to send team metrics digest:', error);
+ throw new Error(`Failed to send team metrics digest: ${error.message}`);
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const limit = 10; | ||
| const hasMoreThanLimit = pullRequests.length > limit; | ||
|
|
There was a problem hiding this comment.
Fix inconsistent limit handling
The limit implementation has a discrepancy: slice(0, limit - 1) shows one less PR than the limit, but the "more" count calculation assumes the full limit was used.
const limit = 10;
-const hasMoreThanLimit = pullRequests.length > limit;
+const displayLimit = limit - 1;
+const hasMore = pullRequests.length > displayLimit;
-...pullRequests.slice(0, limit - 1).map((pullRequest) => ({
+...pullRequests.slice(0, displayLimit).map((pullRequest) => ({
-...(hasMoreThanLimit
+...(hasMore
? [
{
type: "rich_text_section",
elements: [
{
type: "text",
- text: `${pullRequests.length - limit} more...`,
+ text: `${pullRequests.length - displayLimit} more...`,
},
],
},
]
: []),Also applies to: 111-111, 139-151
| export const sendTeamWipDigest = async (digest: DigestWithWorkspace) => { | ||
| const { slackClient } = await getWorkspaceSlackClient(digest.workspaceId); | ||
|
|
||
| const slackChannel = await joinSlackChannel(slackClient, digest.channel); | ||
|
|
||
| if (!slackChannel?.id) { | ||
| throw new ResourceNotFoundException("Slack channel not found"); | ||
| } | ||
|
|
||
| const blocks = await getDigestMessageBlocks(digest); | ||
|
|
||
| await sendSlackMessage(slackClient, { | ||
| channel: slackChannel.id, | ||
| blocks, | ||
| unfurl_links: false, | ||
| unfurl_media: false, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry logic and rate limit handling for Slack operations
The function should handle Slack API rate limits and implement retries for transient failures.
+import { delay } from 'radash';
+import { WebAPIRateLimitedError } from '@slack/web-api';
+
+const MAX_RETRIES = 3;
+const BASE_DELAY = 1000;
+
export const sendTeamWipDigest = async (digest: DigestWithWorkspace) => {
+ let retries = 0;
+
+ while (retries < MAX_RETRIES) {
+ try {
const { slackClient } = await getWorkspaceSlackClient(digest.workspaceId);
const slackChannel = await joinSlackChannel(slackClient, digest.channel);
if (!slackChannel?.id) {
throw new ResourceNotFoundException("Slack channel not found");
}
const blocks = await getDigestMessageBlocks(digest);
await sendSlackMessage(slackClient, {
channel: slackChannel.id,
blocks,
unfurl_links: false,
unfurl_media: false,
});
+ break;
+ } catch (error) {
+ if (error instanceof WebAPIRateLimitedError) {
+ if (retries === MAX_RETRIES - 1) throw error;
+ await delay(BASE_DELAY * Math.pow(2, retries));
+ retries++;
+ continue;
+ }
+ throw error;
+ }
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const getPullRequestsGroupedByState = async ( | ||
| workspaceId: number, | ||
| teamId: number | ||
| ) => { | ||
| const pullRequests = await getPrisma(workspaceId).pullRequest.findMany({ | ||
| where: { | ||
| author: { | ||
| teamMemberships: { | ||
| some: { | ||
| teamId, | ||
| }, | ||
| }, | ||
| }, | ||
| mergedAt: null, | ||
| createdAt: { | ||
| gte: subMonths(new Date(), 3), | ||
| }, | ||
| }, | ||
| take: take(100), | ||
| include: { | ||
| codeReviews: true, | ||
| author: true, | ||
| tracking: true, | ||
| }, | ||
| }); | ||
|
|
||
| const drafted: typeof pullRequests = []; | ||
| const open: typeof pullRequests = []; | ||
| const approved: typeof pullRequests = []; | ||
|
|
||
| for (const pullRequest of pullRequests) { | ||
| if (pullRequest.state === PullRequestState.DRAFT) { | ||
| drafted.push(pullRequest); | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| pullRequest.codeReviews.some( | ||
| (review) => review.state === CodeReviewState.APPROVED | ||
| ) | ||
| ) { | ||
| approved.push(pullRequest); | ||
| continue; | ||
| } | ||
|
|
||
| open.push(pullRequest); | ||
| } | ||
|
|
||
| return { drafted, open, approved }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database query and make date filter configurable
The function could be optimized by:
- Moving the categorization logic to the database query
- Making the date filter configurable through digest settings
const getPullRequestsGroupedByState = async (
workspaceId: number,
- teamId: number
+ teamId: number,
+ options?: {
+ monthsBack?: number;
+ }
) => {
- const pullRequests = await getPrisma(workspaceId).pullRequest.findMany({
- where: {
- author: {
- teamMemberships: {
- some: {
- teamId,
- },
- },
- },
- mergedAt: null,
- createdAt: {
- gte: subMonths(new Date(), 3),
- },
- },
- take: take(100),
- include: {
- codeReviews: true,
- author: true,
- tracking: true,
- },
- });
+ const prisma = getPrisma(workspaceId);
+ const monthsBack = options?.monthsBack ?? 3;
- const drafted: typeof pullRequests = [];
- const open: typeof pullRequests = [];
- const approved: typeof pullRequests = [];
+ const [drafted, open, approved] = await Promise.all([
+ prisma.pullRequest.findMany({
+ where: {
+ author: {
+ teamMemberships: { some: { teamId } }
+ },
+ state: PullRequestState.DRAFT,
+ mergedAt: null,
+ createdAt: { gte: subMonths(new Date(), monthsBack) }
+ },
+ take: take(100),
+ include: { author: true, tracking: true }
+ }),
+ prisma.pullRequest.findMany({
+ where: {
+ author: {
+ teamMemberships: { some: { teamId } }
+ },
+ state: PullRequestState.OPEN,
+ mergedAt: null,
+ createdAt: { gte: subMonths(new Date(), monthsBack) },
+ NOT: {
+ codeReviews: {
+ some: { state: CodeReviewState.APPROVED }
+ }
+ }
+ },
+ take: take(100),
+ include: { author: true, tracking: true }
+ }),
+ prisma.pullRequest.findMany({
+ where: {
+ author: {
+ teamMemberships: { some: { teamId } }
+ },
+ state: PullRequestState.OPEN,
+ mergedAt: null,
+ createdAt: { gte: subMonths(new Date(), monthsBack) },
+ codeReviews: {
+ some: { state: CodeReviewState.APPROVED }
+ }
+ },
+ take: take(100),
+ include: { author: true, tracking: true }
+ })
+ ]);
- for (const pullRequest of pullRequests) {
- if (pullRequest.state === PullRequestState.DRAFT) {
- drafted.push(pullRequest);
- continue;
- }
-
- if (
- pullRequest.codeReviews.some(
- (review) => review.state === CodeReviewState.APPROVED
- )
- ) {
- approved.push(pullRequest);
- continue;
- }
-
- open.push(pullRequest);
- }
-
return { drafted, open, approved };
};📝 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 getPullRequestsGroupedByState = async ( | |
| workspaceId: number, | |
| teamId: number | |
| ) => { | |
| const pullRequests = await getPrisma(workspaceId).pullRequest.findMany({ | |
| where: { | |
| author: { | |
| teamMemberships: { | |
| some: { | |
| teamId, | |
| }, | |
| }, | |
| }, | |
| mergedAt: null, | |
| createdAt: { | |
| gte: subMonths(new Date(), 3), | |
| }, | |
| }, | |
| take: take(100), | |
| include: { | |
| codeReviews: true, | |
| author: true, | |
| tracking: true, | |
| }, | |
| }); | |
| const drafted: typeof pullRequests = []; | |
| const open: typeof pullRequests = []; | |
| const approved: typeof pullRequests = []; | |
| for (const pullRequest of pullRequests) { | |
| if (pullRequest.state === PullRequestState.DRAFT) { | |
| drafted.push(pullRequest); | |
| continue; | |
| } | |
| if ( | |
| pullRequest.codeReviews.some( | |
| (review) => review.state === CodeReviewState.APPROVED | |
| ) | |
| ) { | |
| approved.push(pullRequest); | |
| continue; | |
| } | |
| open.push(pullRequest); | |
| } | |
| return { drafted, open, approved }; | |
| }; | |
| const getPullRequestsGroupedByState = async ( | |
| workspaceId: number, | |
| teamId: number, | |
| options?: { | |
| monthsBack?: number; | |
| } | |
| ) => { | |
| const prisma = getPrisma(workspaceId); | |
| const monthsBack = options?.monthsBack ?? 3; | |
| const [drafted, open, approved] = await Promise.all([ | |
| prisma.pullRequest.findMany({ | |
| where: { | |
| author: { | |
| teamMemberships: { some: { teamId } } | |
| }, | |
| state: PullRequestState.DRAFT, | |
| mergedAt: null, | |
| createdAt: { gte: subMonths(new Date(), monthsBack) } | |
| }, | |
| take: take(100), | |
| include: { author: true, tracking: true } | |
| }), | |
| prisma.pullRequest.findMany({ | |
| where: { | |
| author: { | |
| teamMemberships: { some: { teamId } } | |
| }, | |
| state: PullRequestState.OPEN, | |
| mergedAt: null, | |
| createdAt: { gte: subMonths(new Date(), monthsBack) }, | |
| NOT: { | |
| codeReviews: { | |
| some: { state: CodeReviewState.APPROVED } | |
| } | |
| } | |
| }, | |
| take: take(100), | |
| include: { author: true, tracking: true } | |
| }), | |
| prisma.pullRequest.findMany({ | |
| where: { | |
| author: { | |
| teamMemberships: { some: { teamId } } | |
| }, | |
| state: PullRequestState.OPEN, | |
| mergedAt: null, | |
| createdAt: { gte: subMonths(new Date(), monthsBack) }, | |
| codeReviews: { | |
| some: { state: CodeReviewState.APPROVED } | |
| } | |
| }, | |
| take: take(100), | |
| include: { author: true, tracking: true } | |
| }) | |
| ]); | |
| return { drafted, open, approved }; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
apps/api/src/lib/validate-input.ts (2)
4-19: Consider enhancing error handling and context.While the implementation is solid, consider these improvements:
- Include more context in the error message, such as the validation target (e.g., "Invalid digest input").
- Consider using "warning" or "error" severity for validation errors instead of "debug".
- Enhance Zod error messages with field names for better debugging.
- throw new InputValidationException(`Invalid input`, { + throw new InputValidationException(`Invalid ${schema.description ?? 'input'}`, { validationErrors: error.errors.map((e) => e.message), - severity: "debug", + severity: "warning", });
4-7: Add JSDoc documentation for better maintainability.Consider adding comprehensive documentation to explain the function's purpose, parameters, and return type.
+/** + * Validates input against a Zod schema and throws a formatted error if validation fails. + * @param schema - The Zod schema to validate against + * @param input - The input to validate + * @returns The validated and typed input + * @throws {InputValidationException} When validation fails + */ export const validateInputOrThrow = <T>( schema: z.ZodSchema<T>, input: unknown ): T => {apps/web/src/providers/error-message.provider.ts (2)
4-24: Add error logging for better debugging capabilities.Consider adding error logging to help with debugging issues in production:
+import { logger } from "../utils/logger"; + export const getErrorMessage = (error: unknown) => { + logger.error("Error processing request:", { + error: error instanceof Error ? error.message : String(error), + stack: error instanceof Error ? error.stack : undefined, + }); + // ... rest of the function }
23-23: Consider customizing the fallback error message.The generic fallback message "Something went wrong" could be more helpful to users.
Consider a more informative message like:
- return "Something went wrong. Please try again."; + return "An unexpected error occurred. If this persists, please contact support.";apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts (1)
12-12: Enhance logging with structured fieldsConsider structuring the log fields for better searchability and analysis.
- logger.info("mutation.updateDigest", { input }); + logger.info("mutation.updateDigest", { + workspaceId: input.workspaceId, + teamId: input.teamId, + type: input.type, + channel: input.channel + });apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx (1)
29-48: Consider making the success message more specific.The current success message "Digest updated" could be more informative by including the type of digest that was updated.
- message: `Digest updated.`, + message: `${type} digest settings updated.`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts(1 hunks)apps/api/src/lib/validate-input.ts(1 hunks)apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx(1 hunks)apps/web/src/providers/date.provider.ts(1 hunks)apps/web/src/providers/error-message.provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/providers/date.provider.ts
🔇 Additional comments (6)
apps/api/src/lib/validate-input.ts (1)
1-2: LGTM! Clean and focused imports.
The imports are appropriate and well-scoped for the validation functionality.
apps/web/src/providers/error-message.provider.ts (1)
1-2: LGTM! Imports are appropriate and well-defined.
The imported types are correctly used for type-safe error handling.
apps/api/src/app/digests/resolvers/mutations/update-digest.mutation.ts (1)
10-43: Verify GraphQL mutation implementation against schema
Let's ensure the mutation implementation matches its schema definition.
✅ Verification successful
GraphQL mutation implementation matches schema definition
The implementation aligns correctly with the schema definition. The updateDigest mutation:
- Accepts the required
UpdateDigestInputtype with all specified fields - Returns a
Digesttype as defined in the schema - Handles all input fields defined in the schema: workspaceId, teamId, type, enabled, channel, frequency, dayOfTheWeek, timeOfDay, timezone, and settings
The validation and transformation of the input matches the schema requirements, including the string length validation for the channel field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mutation implementation matches the GraphQL schema
# Search for the mutation definition in schema files
rg -A 10 "type Mutation" --type graphql
# Search for the input type definition
rg -A 15 "input UpdateDigestInput" --type graphql
Length of output: 157
Script:
#!/bin/bash
# Let's try finding GraphQL schema files with different extensions and patterns
fd -e gql -e graphql . || true
fd "schema" -e ts -e js || true
# Then search for mutation and input type definitions
rg -A 10 "type Mutation"
rg -A 15 "input UpdateDigestInput"
Length of output: 14039
apps/web/src/app/teams/[id]/digests/settings/use-digest.tsx (3)
1-21: LGTM! Well-organized imports and clear interface definition.
The imports are logically grouped and the interface is properly typed with the necessary properties.
23-27: Previous review comment about error handling is still applicable.
The code still needs to handle cases where workspace or digest card is missing.
58-66: Verify proper handling of undefined digest data in consumers.
The spread operator on optional chained properties (data?.workspace.team?.digest) could result in undefined values. Ensure that components consuming this hook properly handle cases where digest data might be undefined.
✅ Verification successful
Proper handling of undefined digest data is implemented in consumers
The consuming components (TeamMetricsDigestPage and TeamWipDigestPage) properly handle potential undefined values from the useDigest hook:
- Both components wrap their content in
LoadableContentthat shows a loading skeleton while data is being fetched - The form initialization is guarded by
digestQuery.isFetchedcheck - All digest properties are accessed with null coalescing operators (
??) to provide default values - The form's initial values are set with safe defaults before the digest data is loaded
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components using the useDigest hook and verify their null handling
# Find files importing or using useDigest
rg -l "useDigest" --type tsx
# Find usage of the digest object to check null handling
ast-grep --pattern 'const { digest } = useDigest($$$)'
Length of output: 113
Script:
#!/bin/bash
# Search for files containing useDigest with ripgrep
rg -l "useDigest"
# Search for usage patterns of the digest object
rg -A 3 "useDigest"
# Look for direct imports of useDigest
rg "import.*useDigest"
Length of output: 5187
Script:
#!/bin/bash
# Check how the digest data is used in the consuming components
rg -B 2 -A 5 "const.*digest.*useDigest" apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx
# Look for any null checks or error handling around digest usage
rg "digest\?" apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx
# Check form initialization with digest data
rg -A 5 "useForm.*digest" apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx
Length of output: 487
Script:
#!/bin/bash
# Get the full content of the consuming components to analyze the usage
cat apps/web/src/app/teams/[id]/digests/settings/team-metrics/page.tsx
echo "---"
cat apps/web/src/app/teams/[id]/digests/settings/team-wip/page.tsx
Length of output: 8075
| export const getErrorMessage = (error: unknown) => { | ||
| if (typeof error === "object" && error !== null && "response" in error) { | ||
| const graphQLErrors = (error as ClientError).response | ||
| .errors as GraphQLError[]; | ||
|
|
||
| const firstError = graphQLErrors[0]; | ||
|
|
||
| const extensions = firstError?.extensions as | ||
| | { | ||
| code: string; | ||
| userFacingMessage: string; | ||
| } | ||
| | undefined; | ||
|
|
||
| if (extensions?.code === "INPUT_VALIDATION_FAILED") { | ||
| return extensions.userFacingMessage; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks and improve error handling robustness.
The current implementation has potential safety issues that should be addressed:
- Missing null/empty array check before accessing
graphQLErrors[0] - Type assertions could be replaced with type guards for better type safety
- No validation of the extensions structure
Consider applying this safer implementation:
export const getErrorMessage = (error: unknown) => {
- if (typeof error === "object" && error !== null && "response" in error) {
- const graphQLErrors = (error as ClientError).response
- .errors as GraphQLError[];
-
- const firstError = graphQLErrors[0];
-
- const extensions = firstError?.extensions as
- | {
- code: string;
- userFacingMessage: string;
- }
- | undefined;
-
- if (extensions?.code === "INPUT_VALIDATION_FAILED") {
- return extensions.userFacingMessage;
+ if (!(error instanceof ClientError)) {
+ return "Something went wrong. Please try again.";
+ }
+
+ const graphQLErrors = error.response.errors;
+ if (!Array.isArray(graphQLErrors) || graphQLErrors.length === 0) {
+ return "Something went wrong. Please try again.";
+ }
+
+ const firstError = graphQLErrors[0];
+ const extensions = firstError.extensions;
+
+ if (
+ extensions &&
+ typeof extensions === "object" &&
+ "code" in extensions &&
+ extensions.code === "INPUT_VALIDATION_FAILED" &&
+ "userFacingMessage" in extensions &&
+ typeof extensions.userFacingMessage === "string"
+ ) {
+ return extensions.userFacingMessage;
}
}📝 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 getErrorMessage = (error: unknown) => { | |
| if (typeof error === "object" && error !== null && "response" in error) { | |
| const graphQLErrors = (error as ClientError).response | |
| .errors as GraphQLError[]; | |
| const firstError = graphQLErrors[0]; | |
| const extensions = firstError?.extensions as | |
| | { | |
| code: string; | |
| userFacingMessage: string; | |
| } | |
| | undefined; | |
| if (extensions?.code === "INPUT_VALIDATION_FAILED") { | |
| return extensions.userFacingMessage; | |
| } | |
| } | |
| export const getErrorMessage = (error: unknown) => { | |
| if (!(error instanceof ClientError)) { | |
| return "Something went wrong. Please try again."; | |
| } | |
| const graphQLErrors = error.response.errors; | |
| if (!Array.isArray(graphQLErrors) || graphQLErrors.length === 0) { | |
| return "Something went wrong. Please try again."; | |
| } | |
| const firstError = graphQLErrors[0]; | |
| const extensions = firstError.extensions; | |
| if ( | |
| extensions && | |
| typeof extensions === "object" && | |
| "code" in extensions && | |
| extensions.code === "INPUT_VALIDATION_FAILED" && | |
| "userFacingMessage" in extensions && | |
| typeof extensions.userFacingMessage === "string" | |
| ) { | |
| return extensions.userFacingMessage; | |
| } | |
| } |
| settings: input.settings, | ||
| }); | ||
|
|
||
| return transformDigest(digest); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety check before transformation
Ensure the digest exists before transformation to prevent runtime errors.
- return transformDigest(digest);
+ if (!digest) {
+ throw new Error("Failed to create or update digest");
+ }
+ return transformDigest(digest);📝 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.
| return transformDigest(digest); | |
| if (!digest) { | |
| throw new Error("Failed to create or update digest"); | |
| } | |
| return transformDigest(digest); |
| validateInputOrThrow( | ||
| z.object({ | ||
| channel: z.string().max(80), | ||
| }), | ||
| input | ||
| ); |
There was a problem hiding this comment.
Add validation for all input fields
The current validation only checks the channel field. Consider validating all input fields to ensure data integrity.
validateInputOrThrow(
z.object({
+ workspaceId: z.string().uuid(),
+ teamId: z.string().uuid(),
+ type: z.enum(['DAILY', 'WEEKLY']),
channel: z.string().max(80),
+ enabled: z.boolean(),
+ frequency: z.enum(['DAILY', 'WEEKLY']),
+ dayOfTheWeek: z.number().min(0).max(6).optional(),
+ timeOfDay: z.string().regex(/^([0-1]?[0-9]|2[0-3]):[0-5][0-9]$/),
+ timezone: z.string(),
+ settings: z.record(z.unknown()).optional()
}),
input
);Committable suggestion skipped: line range outside the PR's diff.
| const digest = await upsertDigest({ | ||
| workspaceId: input.workspaceId, | ||
| teamId: input.teamId, | ||
| type: input.type, | ||
| channel: input.channel, | ||
| enabled: input.enabled, | ||
| frequency: input.frequency, | ||
| dayOfTheWeek: input.dayOfTheWeek, | ||
| timeOfDay: input.timeOfDay, | ||
| timezone: input.timezone, | ||
| settings: input.settings, | ||
| }); |
There was a problem hiding this comment.
Add error handling for the upsert operation
The upsert operation could fail for various reasons (e.g., database constraints, concurrent updates). Consider adding try-catch block with appropriate error handling.
- const digest = await upsertDigest({
- workspaceId: input.workspaceId,
- teamId: input.teamId,
- type: input.type,
- channel: input.channel,
- enabled: input.enabled,
- frequency: input.frequency,
- dayOfTheWeek: input.dayOfTheWeek,
- timeOfDay: input.timeOfDay,
- timezone: input.timezone,
- settings: input.settings,
- });
+ try {
+ const digest = await upsertDigest({
+ workspaceId: input.workspaceId,
+ teamId: input.teamId,
+ type: input.type,
+ channel: input.channel,
+ enabled: input.enabled,
+ frequency: input.frequency,
+ dayOfTheWeek: input.dayOfTheWeek,
+ timeOfDay: input.timeOfDay,
+ timezone: input.timezone,
+ settings: input.settings,
+ });
+ } catch (error) {
+ logger.error("Failed to update digest", {
+ error,
+ workspaceId: input.workspaceId,
+ teamId: input.teamId
+ });
+ throw new Error("Failed to update digest. Please try again later.");
+ }Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores