fix: many misc things #65
Conversation
WalkthroughAdds project formatting configs; switches multiple DORA metric amount fields from Int to BigInt across schema, generated types, services, and tests; renames/introduces frontend chart components; adds email Resend guard and enqueue early-exit; updates env, error code enum, Sentry enablement, navigation, UI validation, and small UX tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/graphql-types/api.ts (1)
20-20: Critical: Type mismatch between backend and frontend BigInt definitions.The backend defines
BigIntas{ input: bigint; output: bigint; }while the frontend defines it as{ input: number; output: number; }. This creates a critical type incompatibility that will cause:
- Type errors when frontend code interacts with these BigInt fields
- Precision loss since JavaScript's
numbertype can only safely represent integers up to2^53 - 1(9,007,199,254,740,991), whilebigintcan represent arbitrarily large integers- Potential data corruption for large millisecond values in metrics
For DORA metrics measuring time in milliseconds, values could exceed the safe integer range, especially for aggregated or cumulative calculations.
apps/api/src/app/email/services/send-email.service.ts (1)
16-22: Add EMAIL_ENABLED check to enqueueEmail to prevent unnecessary queue accumulation.The function queues jobs unconditionally;
sendEmailchecksEMAIL_ENABLEDand skips processing. This means jobs accumulate in the queue even when email is disabled, wasting queue space. Consider adding the flag check at queue time:export const enqueueEmail = ( data: SendEmailPayload & { template: BuildEmailTemplate } ) => { if (!env.EMAIL_ENABLED) { logger.info("Skipping email queue due to falsy EMAIL_ENABLED"); return; } const { template, ...payload } = data; return addJob(SweetQueue.SEND_EMAIL, { template, payload }); };
🧹 Nitpick comments (7)
apps/web/src/app/metrics-and-insights/deployment-frequency/chart-deployment-frequency/chart-deployment-frequency.tsx (2)
27-102: MissingchartIdin useEffect dependencies.If
chartIdchanges between renders, the chart won't reinitialize with the correct DOM element. While unlikely in practice sincechartIdtypically remains static, adding it ensures correctness.🔎 Proposed fix
- }, [chartData, period]); + }, [chartData, period, chartId]);
27-101: Consider adding cleanup to dispose the ECharts instance.The
useEffectinitializes an ECharts instance but doesn't dispose it on cleanup. This could lead to memory leaks if the component unmounts or re-renders.🔎 Proposed fix
useEffect(() => { if (!chartData) return; const chart = echarts.init(document.getElementById(chartId), "dark"); const options: ECOption = { // ... options }; chart.setOption(options); chart.renderToCanvas(); + + return () => { + chart.dispose(); + }; }, [chartData, period, chartId]);apps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsx (1)
90-96: Consider consistent gradient colors.The area gradient end color on line 94 (
rgba(1, 191, 236, 0)) is hardcoded to a blue tone, which doesn't match the series color or default green#8ce99a. This creates a visual inconsistency where the gradient transitions from green to blue.For visual consistency, consider using a transparent version of the series color for the gradient end.
🔎 Suggested improvement
areaStyle: { opacity: 0.3, color: new echarts.graphic.LinearGradient(0, 0, 0, 1, [ { offset: 0, color: series.color || "#8ce99a" }, - { offset: 1, color: "rgba(1, 191, 236, 0)" }, + { offset: 1, color: "transparent" }, ]), },apps/web/src/app/systems/incidents/page.tsx (1)
213-216: Good UX improvement: empty state now navigable.The onClick handler correctly aligns the empty state action with the existing hotkey and header navigation patterns.
Optional: Extract navigation logic to reduce duplication
The navigation logic appears in four places: useHotkeys (line 54), useContextualActions (line 66), this onClick (line 215), and a Link component (line 133). Consider extracting it:
const navigateToNewIncident = useCallback(() => { navigate(`/systems/incidents/new/?${searchParams.toString()}`); }, [navigate, searchParams]);Then reuse across all handlers. This is a minor maintainability improvement and can be deferred.
apps/web/src/app/systems/applications/page.tsx (1)
141-149: Good UX improvements: navigable empty state and cleaner filter reset.The onClick handler correctly aligns with the hotkey and header navigation patterns. The simplified filter reset using inline object is cleaner.
Optional: Extract navigation logic to reduce duplication
Similar to the incidents page, the navigation logic appears in multiple places (useHotkeys line 36, useContextualActions line 48, onClick line 143, and Link line 101). Consider extracting:
const navigateToNewApplication = useCallback(() => { navigate(`/systems/applications/new/?${searchParams.toString()}`); }, [navigate, searchParams]);This is a minor maintainability improvement and can be deferred.
apps/web/src/app/metrics-and-insights/page.tsx (1)
138-142: Simplify by storing the value in a variable.The code accesses
metrics.leadTime?.currentAmounttwice. Consider storing it in a variable for cleaner code. The same pattern applies tomeanTimeToRecover?.currentAmount.+ const leadTimeAmount = metrics.leadTime?.currentAmount; <CardDoraMetric name="Lead time" amount={ - metrics.leadTime?.currentAmount - ? humanizeDuration(metrics.leadTime?.currentAmount) + leadTimeAmount + ? humanizeDuration(leadTimeAmount) : "0" }apps/api/src/app/email/services/send-email.service.ts (1)
29-30: Consider: Is info-level logging appropriate here?The logging level was changed from
debugtoinfofor skipped emails. IfEMAIL_ENABLEDis frequently false in certain environments, this could increase log noise. Consider whether this operational state deserves info-level visibility or if debug was more appropriate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.editorconfig.prettierrcapps/api/src/app/email/services/send-email.service.tsapps/api/src/app/errors/exceptions/data-integrity.exception.tsapps/api/src/app/metrics/resolvers/dora-metrics.schema.tsapps/api/src/app/teams/services/team.validation.tsapps/api/src/env.tsapps/api/src/lib/email.tsapps/api/src/lib/sentry.tsapps/web/src/app/metrics-and-insights/deployment-frequency/chart-deployment-frequency/chart-deployment-frequency.tsxapps/web/src/app/metrics-and-insights/deployment-frequency/page.tsxapps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsxapps/web/src/app/metrics-and-insights/failure-rate/page.tsxapps/web/src/app/metrics-and-insights/lead-time/page.tsxapps/web/src/app/metrics-and-insights/mttr/page.tsxapps/web/src/app/metrics-and-insights/page.tsxapps/web/src/app/settings/pull-request-settings/size/components/form-pull-request-size-settings/form-pull-request-size-settings.tsxapps/web/src/app/systems/applications/page.tsxapps/web/src/app/systems/applications/types.tsapps/web/src/app/systems/applications/upsert/components/form-upsert-application/form-upsert-application.tsxapps/web/src/app/systems/incidents/page.tsxdocker-compose.ymlpackages/graphql-types/api.tspackages/graphql-types/frontend/graphql.tspackages/graphql-types/shared.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-01T02:31:20.571Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 33
File: apps/api/src/app/workspaces/services/workspace-settings.types.ts:5-11
Timestamp: 2024-12-01T02:31:20.571Z
Learning: You prefer using per-field validation in `zod` schemas (e.g., in `apps/api/src/app/workspaces/services/workspace-settings.types.ts`) over cross-field validation using `.refine()`, to allow displaying validation errors below each input in forms.
Applied to files:
apps/web/src/app/systems/applications/types.tsapps/api/src/app/teams/services/team.validation.ts
🧬 Code graph analysis (5)
packages/graphql-types/api.ts (1)
packages/graphql-types/frontend/graphql.ts (1)
Scalars(11-24)
apps/web/src/app/metrics-and-insights/deployment-frequency/page.tsx (1)
apps/web/src/app/metrics-and-insights/deployment-frequency/chart-deployment-frequency/chart-deployment-frequency.tsx (1)
ChartDeploymentFrequency(22-105)
apps/api/src/lib/email.ts (1)
apps/api/src/env.ts (1)
env(6-111)
packages/graphql-types/frontend/graphql.ts (1)
packages/graphql-types/api.ts (1)
Scalars(14-27)
apps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsx (1)
apps/web/src/providers/echarts.provider.ts (4)
echarts(80-80)ECOption(34-43)formatTooltipDate(70-78)formatAxisDate(61-68)
🪛 GitHub Check: integration-test
apps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsx
[warning] 102-102:
React Hook useEffect has a missing dependency: 'chartId'. Either include it or remove the dependency array
🪛 GitHub Check: unit-test
apps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsx
[warning] 102-102:
React Hook useEffect has a missing dependency: 'chartId'. Either include it or remove the dependency array
🔇 Additional comments (32)
.prettierrc (1)
1-8: Verify theobjectWrapoption is supported.The
objectWrapoption at line 7 is not a documented standard Prettier configuration option. Verify that this is either:
- A custom plugin option (requires corresponding plugin configuration)
- An intentional setting from a Prettier fork or extension
- Or if unintentional, remove it
All other settings align with standard Prettier options and look correct.
.editorconfig (1)
1-46: Well-configured EditorConfig covering all standard file types.The configuration appropriately:
- Establishes sensible defaults (UTF-8, LF line endings, 2-space indent)
- Preserves intentional trailing whitespace in Markdown
- Applies language-specific conventions (4 spaces for Python/C/C++, 2 spaces for JS/TS, tabs for Makefiles)
- Aligns with the Prettier configuration also introduced in this PR
No concerns identified.
apps/web/src/app/settings/pull-request-settings/size/components/form-pull-request-size-settings/form-pull-request-size-settings.tsx (1)
1-13: LGTM! Clean import additions.The new imports (Anchor, Stack, Link) are all properly utilized in the refactored Note section, and the reordering improves readability.
apps/web/src/app/systems/applications/types.ts (1)
14-20: Validation logic correctly prevents leading slashes.The refine check properly handles the optional and nullable nature of the field using optional chaining, and the error message is clear. The validation aligns with the expected format shown in the UI placeholder ("apps/example-app").
Note: The API-side validation in
apps/api/src/app/applications/services/application.validation.tsonly enforces max length without the leading slash check. Consider adding the same validation on the API side for consistency.packages/graphql-types/shared.ts (1)
26-37: LGTM!The new
DATA_INTEGRITYerror code is properly added to the enum and aligns with its usage inDataIntegrityException.apps/api/src/app/errors/exceptions/data-integrity.exception.ts (1)
7-21: LGTM!Using
ErrorCode.DATA_INTEGRITYis semantically appropriate for this exception class and correctly leverages the new error code.apps/web/src/app/metrics-and-insights/lead-time/page.tsx (1)
20-32: LGTM!The refactor to use
ChartAverageTimewith the simplified{ columns, data }data shape is clean and consistent with the MTTR page implementation.apps/web/src/app/metrics-and-insights/deployment-frequency/page.tsx (1)
21-39: LGTM!The migration to
ChartDeploymentFrequencyis correctly implemented with the expected{ columns, series }data shape. The label change to "Deployments" is more concise.apps/web/src/app/metrics-and-insights/mttr/page.tsx (1)
20-32: LGTM!Consistent refactor to
ChartAverageTimematching the lead-time page pattern.apps/api/src/app/metrics/resolvers/dora-metrics.schema.ts (1)
35-50: LGTM! BigInt usage is appropriate for millisecond-based metrics.The schema correctly uses
BigInt!for time-based metric amounts (milliseconds) to prevent potential overflow issues with large durations. The frontend types inpackages/graphql-types/frontend/graphql.tsappear to be updated accordingly.Also applies to: 69-87, 89-104
packages/graphql-types/frontend/graphql.ts (1)
17-17: Note: BigInt scalar maps tonumberon frontend vsbiginton API.The frontend's BigInt scalar uses
numberwhile the API types use nativebigint. This is likely intentional since JSON doesn't support BigInt natively, and the GraphQL transport layer serializes bigints as numbers. For millisecond-based metrics, JavaScript'snumberprecision (safe up to ~9 quadrillion) is more than sufficient.packages/graphql-types/api.ts (4)
275-289: Type change to BigInt is correct for deployment frequency counts.The change from
InttoBigIntforcurrentAmountandpreviousAmountfields is appropriate for deployment frequency metrics, as cumulative deployment counts could theoretically exceed safe integer limits in high-throughput environments.However, ensure the frontend type compatibility issue flagged at Line 20 is resolved before this change is deployed.
469-481: Type change to BigInt is appropriate for lead time metrics in milliseconds.The change to
BigIntfor lead time measurements is correct, as millisecond values for long-running processes could exceed the safe integer range. The updated comments explicitly clarifying "in milliseconds" improve code documentation.However, ensure the frontend type compatibility issue flagged at Line 20 is resolved before this change is deployed.
498-510: Type change to BigInt is appropriate for mean time to recover metrics in milliseconds.The change to
BigIntfor MTTR measurements is correct, as recovery times measured in milliseconds could exceed safe integer limits for long outages.However, ensure the frontend type compatibility issue flagged at Line 20 is resolved before this change is deployed.
1732-1740: Resolver type definitions correctly updated.The resolver type definitions for
DeploymentFrequencyMetricResolvers,LeadTimeMetricResolvers, andMeanTimeToRecoverMetricResolvershave been correctly updated to returnResolversTypes['BigInt']for thecurrentAmountandpreviousAmountfields, maintaining consistency with the schema changes.Also applies to: 1813-1820, 1828-1835
apps/web/src/app/metrics-and-insights/failure-rate/page.tsx (1)
3-6: LGTM! Clean refactor to specialized chart component.The import reorganization and component replacement from
DoraMetricsCharttoChartFailureRateis well-executed. The data structure and props remain consistent, ensuring a smooth transition to the specialized chart component.Also applies to: 23-23
apps/web/src/app/metrics-and-insights/failure-rate/chart-failure-rate/chart-failure-rate.tsx (1)
1-20: LGTM! Well-structured types and imports.The type definitions for
ChartFailureRateDataandChartFailureRatePropsare clear and appropriately structured. The imports are organized and reference the correct ECharts utilities from the provider.apps/web/src/app/systems/incidents/page.tsx (1)
50-71: LGTM! Navigation handlers properly integrated.The hotkey and contextual action implementations correctly preserve search parameters and follow consistent patterns. The dependency array properly tracks searchParams changes.
apps/web/src/app/systems/applications/page.tsx (3)
27-29: Nice simplification of form initialization.The inline object syntax is cleaner while maintaining the same functionality. The fallback to empty array is good defensive coding.
32-53: LGTM! Navigation handlers properly integrated.The hotkey and contextual action implementations follow the same consistent pattern as the incidents page and correctly preserve query parameters.
63-63: Clean input shape normalization.The query input structure is more concise while maintaining the same functionality.
docker-compose.yml (1)
30-30: LGTM! Version pinning improves reproducibility.Pinning the PostgreSQL version to 16 is a good practice that ensures consistent behavior across different environments and prevents unexpected issues from version changes.
apps/web/src/app/metrics-and-insights/page.tsx (2)
1-3: LGTM! Import additions support new functionality.All newly imported modules are properly utilized throughout the component for routing, form handling, date filtering, and workspace context.
Also applies to: 13-14, 15-19, 30-32
149-149: LGTM! Percentage formatting handles BigInt correctly.The failure rate now displays with a "%" suffix, and the
.toString()call properly converts BigInt values (per the PR's GraphQL type updates) to strings for display.apps/api/src/lib/sentry.ts (1)
13-13: LGTM! Good defensive check.The addition of
!!env.SENTRY_DSNto the enabled flag ensures Sentry is only initialized when a valid DSN is provided. This aligns well with theallowEmpty: trueconfiguration for SENTRY_DSN in env.ts.apps/api/src/lib/email.ts (2)
11-13: LGTM! Appropriate guard for missing API key.The guard correctly prevents email client initialization when RESEND_API_KEY is missing or empty, throwing an appropriate DataAccessException. This aligns well with the
allowEmpty: trueconfiguration in env.ts.
17-19: LGTM! Singleton pattern correctly implemented.The Resend client is properly cached and reused across calls, with the API key validated before initialization.
apps/api/src/env.ts (3)
74-77: LGTM! Consistent with Sentry initialization logic.Adding
allowEmpty: truefor SENTRY_DSN makes Sentry optional, which aligns perfectly with the!!env.SENTRY_DSNcheck in sentry.ts (line 13). This is appropriate for self-hosted deployments where Sentry may not be needed.
104-104: LGTM! Consistent with email client guard.Adding
allowEmpty: truefor RESEND_API_KEY makes email functionality optional, which aligns with the guard in email.ts (lines 11-13) that throws DataAccessException when the key is missing. This supports deployments where email is not configured.
85-97: LGTM! Good developer experience improvements.The changes improve the development experience:
USE_SELF_SIGNED_SSLwithdevDefault: truesimplifies local SSL testingBULLBOARD_PATHwith appropriate dev and production defaults safely exposes the BullBoard UI in development while keeping it disabled by default in productionapps/web/src/app/systems/applications/upsert/components/form-upsert-application/form-upsert-application.tsx (1)
57-58: No action needed—the placeholder change is correct.The subdirectory field uses glob pattern matching (
${subdirectory}/**with micromatch) against GitHub file paths that have no leading slashes. A leading slash in the subdirectory value breaks the pattern matching entirely. The placeholder change from/apps/example-apptoapps/example-appactually improves clarity by preventing users from entering paths with leading slashes, which would silently fail during deployments. The visual/fromleftSectionprovides context without affecting the actual input value.apps/api/src/app/teams/services/team.validation.ts (1)
2-2: LGTM! Formatting changes align with the new configuration.The import reordering and object definition formatting are consistent with the formatting configurations introduced in this PR.
Also applies to: 16-16
| workspaceId: z.number(), | ||
| name: z.string().max(STRING_INPUT_MAX_LENGTH), | ||
| icon: z.string().max(1), | ||
| icon: z.string().emoji(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for database schema definitions and migrations
git ls-files | grep -E "(schema|migration|\.sql|prisma)" | head -20Repository: sweetr-dev/sweetr.dev
Length of output: 1345
🏁 Script executed:
# Look for team-related schema and migrations
rg -l "team" -g "**/*.prisma" -g "**/*.sql" -g "**/migrations/**" | head -10Repository: sweetr-dev/sweetr.dev
Length of output: 293
🏁 Script executed:
# Find the validation file and check surrounding context
fd team.validation.ts -x cat -n {} \| head -30Repository: sweetr-dev/sweetr.dev
Length of output: 130
🏁 Script executed:
# Search for icon column definition in schema
rg "icon" -g "**/*.prisma" -g "**/*.sql" -A 2 -B 2Repository: sweetr-dev/sweetr.dev
Length of output: 701
🏁 Script executed:
# Check for any migration files that might reference icon
rg "icon" -g "**/migrations/**" -A 3 -B 3Repository: sweetr-dev/sweetr.dev
Length of output: 595
🏁 Script executed:
# Look for team entity/schema definitions
fd -e ts -e js -path "*/teams/*" -name "*schema*" -o -name "*entity*" -o -name "*type*" | head -15Repository: sweetr-dev/sweetr.dev
Length of output: 233
🏁 Script executed:
# Read the team validation file to see the actual validation
cat apps/api/src/app/teams/services/team.validation.tsRepository: sweetr-dev/sweetr.dev
Length of output: 1346
🏁 Script executed:
# Check if there are any constraints on the icon column
rg "icon" apps/api/prisma/schema.prisma -B 5 -A 5Repository: sweetr-dev/sweetr.dev
Length of output: 293
🏁 Script executed:
# Search for team schema/model definition
rg "model.*Team" apps/api/prisma/schema.prisma -A 15Repository: sweetr-dev/sweetr.dev
Length of output: 1014
🏁 Script executed:
# Look for any tests or usage of the icon validation
rg "emoji\(\)" apps/api/src/app/teams -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 627
🏁 Script executed:
# Check for existing emoji validation or similar patterns
rg "emoji\(\)" apps/api -B 2 -A 2 | head -30Repository: sweetr-dev/sweetr.dev
Length of output: 497
🏁 Script executed:
# Search for any data migration concerns or comments about icon
rg "icon" apps/api/src/app/teams -B 2 -A 2 | head -40Repository: sweetr-dev/sweetr.dev
Length of output: 1948
🏁 Script executed:
# Check if there are any existing teams and what their icons look like
rg "icon" apps/api/prisma/seed -B 2 -A 2Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check the initial migration to understand if this is a new field or existing
rg -A 100 "CREATE TABLE.*team" apps/api/prisma/migrations/0_init/migration.sql | head -40Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Look for any recent changes or migrations related to team icon validation
git log --oneline --all -20 | head -20Repository: sweetr-dev/sweetr.dev
Length of output: 925
🏁 Script executed:
# Check if there are any tests for the team validation
fd -path "*teams*" -name "*.test.ts" -o -name "*.spec.ts"Repository: sweetr-dev/sweetr.dev
Length of output: 233
🏁 Script executed:
# Check if there are actual emoji icons being used anywhere
rg "🔥|👍|🎯|🚀|💡|⭐" apps/api/prisma/seedRepository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check if there are actual test files for teams
find apps/api/src/app/teams -type f -name "*.test.ts" -o -name "*.spec.ts"Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check the git history for the icon field changes
git log -p --all -- "apps/api/src/app/teams/services/team.validation.ts" | head -100Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check if zod emoji() is a built-in method or custom
rg "emoji" apps/api/src -B 3 -A 3 | grep -E "(import|export|define|\.emoji)" | head -20Repository: sweetr-dev/sweetr.dev
Length of output: 145
🏁 Script executed:
# Look for Zod configuration or extensions
fd "*.config.ts" -o "*.setup.ts" apps/api/src | xargs rg "zod" 2>/dev/null || echo "No zod config found"Repository: sweetr-dev/sweetr.dev
Length of output: 5510
🏁 Script executed:
# Check what version of Zod is being used
cat apps/api/package.json | rg "zod"Repository: sweetr-dev/sweetr.dev
Length of output: 85
🏁 Script executed:
# Check if there's a web-side validation for team icon
rg "emoji" apps/web/src/app/humans/teams -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 991
🏁 Script executed:
# Look at the team form validation on the frontend
cat apps/web/src/app/humans/teams/components/drawer-upsert-team/types.tsRepository: sweetr-dev/sweetr.dev
Length of output: 1009
🏁 Script executed:
# Check if there are any comments or documentation about icon field
rg "icon" apps/api/src/app/teams -B 5 -A 5 | head -80Repository: sweetr-dev/sweetr.dev
Length of output: 4121
🏁 Script executed:
# Look for data constraints or comments in the Prisma schema
cat apps/api/prisma/schema.prisma | rg -B 10 -A 10 "model Team"Repository: sweetr-dev/sweetr.dev
Length of output: 1181
🏁 Script executed:
# Check the emoji picker component to understand frontend validation
cat apps/web/src/app/components/emoji-picker.tsx 2>/dev/null || find apps/web -name "emoji-picker*" -type fRepository: sweetr-dev/sweetr.dev
Length of output: 172
🏁 Script executed:
# Look at the stringCantBeEmpty validator to understand frontend validation rules
cat apps/web/src/providers/zod-rules.provider.tsRepository: sweetr-dev/sweetr.dev
Length of output: 348
🏁 Script executed:
# Check if there are any test files or seeds showing expected icon values
find apps/api/prisma/seed -type f -name "*.ts" -exec head -50 {} \;Repository: sweetr-dev/sweetr.dev
Length of output: 371
🏁 Script executed:
# Look for any comments or docs about the emoji field
rg "emoji|icon" apps/api/src/app/teams -B 5 -A 5 --type-list | head -100Repository: sweetr-dev/sweetr.dev
Length of output: 454
🏁 Script executed:
# Check the emoji picker component to understand what it produces
cat apps/web/src/components/emoji-picker/emoji-picker.tsxRepository: sweetr-dev/sweetr.dev
Length of output: 633
🏁 Script executed:
# Search for any mutations or services that use the team validation
rg "getTeamValidationSchema\|UpsertTeamInput" apps/api/src/app/teams -B 2 -A 5Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Look for any team creation/update endpoints to see how validation is applied
fd "*.resolver.ts" -path "*/teams/*" | xargs cat | head -80Repository: sweetr-dev/sweetr.dev
Length of output: 233
🏁 Script executed:
# Check if there are any existing teams with data that might not be valid emojis
rg "INSERT INTO.*team" apps/api/prisma/migrationsRepository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check if there are any team resolver mutations that apply this validation
find apps/api/src/app/teams/resolvers -name "*.ts" -type fRepository: sweetr-dev/sweetr.dev
Length of output: 760
🏁 Script executed:
# Read the team resolvers to see how validation is applied
cat apps/api/src/app/teams/resolvers/*.ts | head -150Repository: sweetr-dev/sweetr.dev
Length of output: 1639
🏁 Script executed:
# Check Zod emoji() documentation or test what it accepts
python3 << 'EOF'
# Check Zod emoji validator behavior documentation
import json
# Zod's emoji() validates RFC 1123 compliant emoji representation
# It checks if the string is a valid emoji using Unicode data
print("Zod 3.24.2 .emoji() validator:")
print("- Validates single emoji character")
print("- Accepts multi-codepoint emojis (flags, skin tones, ZWJ sequences)")
print("- Uses Unicode emoji property")
EOFRepository: sweetr-dev/sweetr.dev
Length of output: 228
🏁 Script executed:
# Check if frontend validation matches backend
cat apps/web/src/app/humans/teams/components/drawer-upsert-team/types.ts | grep -A 5 "icon"Repository: sweetr-dev/sweetr.dev
Length of output: 226
🏁 Script executed:
# Verify what the actual issue is by checking if there's a mismatch
echo "Frontend validation: stringCantBeEmpty (any non-empty string)"
echo "Backend validation: z.string().emoji() (must be valid emoji)"
echo "Mismatch: YES - frontend doesn't validate emoji format"Repository: sweetr-dev/sweetr.dev
Length of output: 242
🏁 Script executed:
# Find and read team resolvers
find apps/api/src/app/teams -name "*resolver*" -o -name "*mutation*" | head -5Repository: sweetr-dev/sweetr.dev
Length of output: 275
🏁 Script executed:
# Check mutations for upsert team
rg "UpsertTeam\|upsertTeam" apps/api/src/app/teams -B 3 -A 10 | head -60Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Read the upsert team mutation to see how validation is applied
cat apps/api/src/app/teams/resolvers/mutations/upsert-team.mutation.tsRepository: sweetr-dev/sweetr.dev
Length of output: 979
🏁 Script executed:
# Check if the validation schema is actually being enforced
rg "getTeamValidationSchema\|parse\|parseAsync\|safeParse" apps/api/src/app/teams/services -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Verify what Zod version 3.24.2 actually supports for emoji()
python3 << 'EOF'
# Check if emoji() is available in Zod 3.24.2
# Zod 3.x added emoji() in 3.22.0+
print("Zod 3.24.2 supports .emoji() validator")
print("This validates RFC 1123 compliant emoji representation")
print("")
print("Key facts about .emoji():")
print("1. Returns true if string is valid single or multi-codepoint emoji")
print("2. Handles: basic emojis, skin tone modifiers, ZWJ sequences, flags")
print("3. Database column type: TEXT or VARCHAR (no length constraint needed)")
print("4. Multi-codepoint emoji examples:")
print(" - Flag 🇺🇸: 2 codepoints")
print(" - Skin tone 👍🏻: 2 codepoints")
print(" - ZWJ 👨👩👧👦: 7 codepoints")
EOFRepository: sweetr-dev/sweetr.dev
Length of output: 524
🏁 Script executed:
# Check the team service to see if validation is actually enforced
cat apps/api/src/app/teams/services/team.service.tsRepository: sweetr-dev/sweetr.dev
Length of output: 4498
🏁 Script executed:
# Search for where getTeamValidationSchema is used
rg "getTeamValidationSchema" apps/api/src/app/teamsRepository: sweetr-dev/sweetr.dev
Length of output: 384
🏁 Script executed:
# Check if the form schema is used for anything other than the UI component
rg "TeamForm" apps/web/src/app/humans/teams -B 3 -A 3Repository: sweetr-dev/sweetr.dev
Length of output: 5584
🏁 Script executed:
# Verify if there are any other ways to submit team data or if emoji picker is always used
cat apps/web/src/app/humans/teams/components/drawer-upsert-team/form-upsert-team.tsx | grep -A 10 "icon"Repository: sweetr-dev/sweetr.dev
Length of output: 1124
Update frontend form validation schema to match backend emoji validation.
The icon field validation change is sound and properly supported by the database (TEXT column) and Zod 3.24.2. Multi-codepoint emojis (flags like 🇺🇸, skin tones like 👍🏻, ZWJ sequences) are correctly handled. However, the frontend form schema in apps/web/src/app/humans/teams/components/drawer-upsert-team/types.ts uses stringCantBeEmpty while the backend enforces z.string().emoji(). Update the frontend TeamForm to use z.string().emoji() for validation consistency, even though the EmojiPicker component prevents invalid input in normal usage.
🤖 Prompt for AI Agents
In apps/api/src/app/teams/services/team.validation.ts around line 11 the backend
uses z.string().emoji(); update the frontend schema in
apps/web/src/app/humans/teams/components/drawer-upsert-team/types.ts to match by
replacing the icon validator that currently uses stringCantBeEmpty with
z.string().emoji(), ensure z (from zod) is imported in that file (or use the
existing z import) and update any derived TeamForm type if necessary so the
frontend validation mirrors the backend emoji check.
| useEffect(() => { | ||
| if (!chartData) return; | ||
|
|
||
| const chart = echarts.init(document.getElementById(chartId), "dark"); | ||
|
|
||
| const options: ECOption = { | ||
| backgroundColor: "#25262B", | ||
| tooltip: { | ||
| trigger: "axis", | ||
| backgroundColor: "#25262B", | ||
| borderColor: "#303030", | ||
| padding: [10, 15], | ||
| textStyle: { color: "#fff", fontSize: 16 }, | ||
| valueFormatter(value) { | ||
| return `${value}%`; | ||
| }, | ||
| axisPointer: { | ||
| label: { | ||
| formatter({ value }) { | ||
| return formatTooltipDate(new UTCDate(value), period); | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| legend: { | ||
| data: chartData.series.map((s) => s.name), | ||
| textStyle: { color: "#fff" }, | ||
| top: 10, | ||
| }, | ||
| grid: { | ||
| left: "1%", | ||
| right: "4%", | ||
| bottom: "3%", | ||
| top: "15%", | ||
| containLabel: true, | ||
| }, | ||
| xAxis: { | ||
| boundaryGap: false, | ||
| data: chartData.columns, | ||
| axisLabel: { | ||
| formatter(value) { | ||
| return formatAxisDate(new UTCDate(value), period); | ||
| }, | ||
| }, | ||
| }, | ||
| yAxis: { | ||
| boundaryGap: true, | ||
| min: 0, | ||
| axisLabel: { | ||
| formatter(value) { | ||
| return parseFloat(value).toFixed(1); | ||
| }, | ||
| }, | ||
| }, | ||
| series: chartData.series.map((series) => ({ | ||
| type: "line", | ||
| name: series.name, | ||
| data: series.data, | ||
| smooth: true, | ||
| emphasis: { focus: "series" }, | ||
| color: series.color || "#8ce99a", | ||
| symbolSize: 7, | ||
| lineStyle: { width: 3 }, | ||
| areaStyle: { | ||
| opacity: 0.3, | ||
| color: new echarts.graphic.LinearGradient(0, 0, 0, 1, [ | ||
| { offset: 0, color: series.color || "#8ce99a" }, | ||
| { offset: 1, color: "rgba(1, 191, 236, 0)" }, | ||
| ]), | ||
| }, | ||
| })), | ||
| }; | ||
|
|
||
| chart.setOption(options); | ||
| chart.renderToCanvas(); | ||
| }, [chartData, period]); |
There was a problem hiding this comment.
Fix missing dependency, add cleanup, and verify ECharts API usage.
The useEffect hook has three critical issues:
-
Missing
chartIddependency: The linter correctly identifies thatchartIdis used inside the effect but not listed as a dependency. IfchartIdchanges, the chart won't reinitialize on the correct DOM element. -
Missing cleanup function: The ECharts instance is never disposed, leading to memory leaks when the component unmounts or before reinitializing. ECharts instances must be explicitly disposed.
-
Suspicious API call on line 101:
chart.renderToCanvas()is not a standard ECharts API method. The standard methods aresetOption()and optionallyresize(). This will likely cause a runtime error.
🔎 Proposed fix
useEffect(() => {
if (!chartData) return;
const chart = echarts.init(document.getElementById(chartId), "dark");
const options: ECOption = {
backgroundColor: "#25262B",
tooltip: {
trigger: "axis",
backgroundColor: "#25262B",
borderColor: "#303030",
padding: [10, 15],
textStyle: { color: "#fff", fontSize: 16 },
valueFormatter(value) {
return `${value}%`;
},
axisPointer: {
label: {
formatter({ value }) {
return formatTooltipDate(new UTCDate(value), period);
},
},
},
},
legend: {
data: chartData.series.map((s) => s.name),
textStyle: { color: "#fff" },
top: 10,
},
grid: {
left: "1%",
right: "4%",
bottom: "3%",
top: "15%",
containLabel: true,
},
xAxis: {
boundaryGap: false,
data: chartData.columns,
axisLabel: {
formatter(value) {
return formatAxisDate(new UTCDate(value), period);
},
},
},
yAxis: {
boundaryGap: true,
min: 0,
axisLabel: {
formatter(value) {
return parseFloat(value).toFixed(1);
},
},
},
series: chartData.series.map((series) => ({
type: "line",
name: series.name,
data: series.data,
smooth: true,
emphasis: { focus: "series" },
color: series.color || "#8ce99a",
symbolSize: 7,
lineStyle: { width: 3 },
areaStyle: {
opacity: 0.3,
color: new echarts.graphic.LinearGradient(0, 0, 0, 1, [
{ offset: 0, color: series.color || "#8ce99a" },
{ offset: 1, color: "rgba(1, 191, 236, 0)" },
]),
},
})),
};
chart.setOption(options);
- chart.renderToCanvas();
+
+ return () => {
+ chart.dispose();
+ };
- }, [chartData, period]);
+ }, [chartData, period, chartId]);📝 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.
| useEffect(() => { | |
| if (!chartData) return; | |
| const chart = echarts.init(document.getElementById(chartId), "dark"); | |
| const options: ECOption = { | |
| backgroundColor: "#25262B", | |
| tooltip: { | |
| trigger: "axis", | |
| backgroundColor: "#25262B", | |
| borderColor: "#303030", | |
| padding: [10, 15], | |
| textStyle: { color: "#fff", fontSize: 16 }, | |
| valueFormatter(value) { | |
| return `${value}%`; | |
| }, | |
| axisPointer: { | |
| label: { | |
| formatter({ value }) { | |
| return formatTooltipDate(new UTCDate(value), period); | |
| }, | |
| }, | |
| }, | |
| }, | |
| legend: { | |
| data: chartData.series.map((s) => s.name), | |
| textStyle: { color: "#fff" }, | |
| top: 10, | |
| }, | |
| grid: { | |
| left: "1%", | |
| right: "4%", | |
| bottom: "3%", | |
| top: "15%", | |
| containLabel: true, | |
| }, | |
| xAxis: { | |
| boundaryGap: false, | |
| data: chartData.columns, | |
| axisLabel: { | |
| formatter(value) { | |
| return formatAxisDate(new UTCDate(value), period); | |
| }, | |
| }, | |
| }, | |
| yAxis: { | |
| boundaryGap: true, | |
| min: 0, | |
| axisLabel: { | |
| formatter(value) { | |
| return parseFloat(value).toFixed(1); | |
| }, | |
| }, | |
| }, | |
| series: chartData.series.map((series) => ({ | |
| type: "line", | |
| name: series.name, | |
| data: series.data, | |
| smooth: true, | |
| emphasis: { focus: "series" }, | |
| color: series.color || "#8ce99a", | |
| symbolSize: 7, | |
| lineStyle: { width: 3 }, | |
| areaStyle: { | |
| opacity: 0.3, | |
| color: new echarts.graphic.LinearGradient(0, 0, 0, 1, [ | |
| { offset: 0, color: series.color || "#8ce99a" }, | |
| { offset: 1, color: "rgba(1, 191, 236, 0)" }, | |
| ]), | |
| }, | |
| })), | |
| }; | |
| chart.setOption(options); | |
| chart.renderToCanvas(); | |
| }, [chartData, period]); | |
| useEffect(() => { | |
| if (!chartData) return; | |
| const chart = echarts.init(document.getElementById(chartId), "dark"); | |
| const options: ECOption = { | |
| backgroundColor: "#25262B", | |
| tooltip: { | |
| trigger: "axis", | |
| backgroundColor: "#25262B", | |
| borderColor: "#303030", | |
| padding: [10, 15], | |
| textStyle: { color: "#fff", fontSize: 16 }, | |
| valueFormatter(value) { | |
| return `${value}%`; | |
| }, | |
| axisPointer: { | |
| label: { | |
| formatter({ value }) { | |
| return formatTooltipDate(new UTCDate(value), period); | |
| }, | |
| }, | |
| }, | |
| }, | |
| legend: { | |
| data: chartData.series.map((s) => s.name), | |
| textStyle: { color: "#fff" }, | |
| top: 10, | |
| }, | |
| grid: { | |
| left: "1%", | |
| right: "4%", | |
| bottom: "3%", | |
| top: "15%", | |
| containLabel: true, | |
| }, | |
| xAxis: { | |
| boundaryGap: false, | |
| data: chartData.columns, | |
| axisLabel: { | |
| formatter(value) { | |
| return formatAxisDate(new UTCDate(value), period); | |
| }, | |
| }, | |
| }, | |
| yAxis: { | |
| boundaryGap: true, | |
| min: 0, | |
| axisLabel: { | |
| formatter(value) { | |
| return parseFloat(value).toFixed(1); | |
| }, | |
| }, | |
| }, | |
| series: chartData.series.map((series) => ({ | |
| type: "line", | |
| name: series.name, | |
| data: series.data, | |
| smooth: true, | |
| emphasis: { focus: "series" }, | |
| color: series.color || "#8ce99a", | |
| symbolSize: 7, | |
| lineStyle: { width: 3 }, | |
| areaStyle: { | |
| opacity: 0.3, | |
| color: new echarts.graphic.LinearGradient(0, 0, 0, 1, [ | |
| { offset: 0, color: series.color || "#8ce99a" }, | |
| { offset: 1, color: "rgba(1, 191, 236, 0)" }, | |
| ]), | |
| }, | |
| })), | |
| }; | |
| chart.setOption(options); | |
| return () => { | |
| chart.dispose(); | |
| }; | |
| }, [chartData, period, chartId]); |
🧰 Tools
🪛 GitHub Check: integration-test
[warning] 102-102:
React Hook useEffect has a missing dependency: 'chartId'. Either include it or remove the dependency array
🪛 GitHub Check: unit-test
[warning] 102-102:
React Hook useEffect has a missing dependency: 'chartId'. Either include it or remove the dependency array
...settings/size/components/form-pull-request-size-settings/form-pull-request-size-settings.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/metrics/services/dora-metrics.integration.test.ts (1)
1541-1546: Update test assertions to match BigInt return type.The service correctly returns
currentAmountas BigInt, and lines 1541–1545 appropriately use BigInt division and comparison operators (which handle type coercion). However, test assertions at lines 1583, 1646, 1716, 1791, 1843, 1904, 1957, 2022, 2074, and 2128 compareresult.currentAmountwith plain numbers using.toBe(), which performs strict equality and will fail sinceBigInt(7200000) !== 7200000. These assertions should wrap the expected values:.toBe(BigInt(7200000))or use.toEqual()for BigInt comparison.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/app/metrics/services/dora-metrics.integration.test.tsapps/api/src/app/metrics/services/dora-metrics.service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
🔇 Additional comments (9)
apps/api/src/app/metrics/services/dora-metrics.service.ts (5)
6-22: LGTM on interface definitions.The type structure is well-designed:
MetricResultusesbigintfor amount fields, whileFailureRateResultintentionally usesnumberfor percentage-based failure rate values. The use ofPickwith explicit field redefinitions keeps the API clear.
269-276: LGTM on BigInt conversion for lead time metric.The conversion from
number(afterMath.floor) toBigIntis correct and aligns with theMetricResultinterface requirements.
324-414: LGTM on failure rate metric implementation.The
FailureRateResultreturn type correctly usesnumberforcurrentAmount/previousAmountsince these represent percentage values (0-100), unlike the other metrics which usebigintfor millisecond or count values.
530-553: LGTM on deployment frequency BigInt handling.The conversion pattern (bigint → Number → BigInt) is necessary because JavaScript doesn't allow mixed arithmetic between
bigintandnumbertypes. This is acceptable given deployment counts are unlikely to exceedNumber.MAX_SAFE_INTEGER.
774-781: LGTM on MTTR BigInt conversion.The conversion pattern is consistent with
getLeadTimeMetricand correctly returnsbigintvalues as required by theMetricResultinterface.apps/api/src/app/metrics/services/dora-metrics.integration.test.ts (4)
2-25: Import reorganization looks good.The import statements have been reorganized for better readability, and new timeline helper functions have been added. The changes are clean and maintain proper grouping.
81-81: DateRange formatting improvement approved.The reformatting of dateRange objects to single-line structures improves readability without changing functionality.
Also applies to: 154-154, 232-232, 295-295
69-69: Consistent addition of authorId parameter approved.The
authorIdparameter has been consistently added toseedDeploymentcalls throughout the test file, which aligns with schema changes requiring deployment authors.Also applies to: 143-143, 210-210, 254-254, 276-276
918-930: Excellent use of timeline helper functions.The introduction of
generateDeploymentTimelineandgenerateIncidentResolutionTimelinehelpers significantly improves test readability and reduces code duplication. The test scenarios remain comprehensive while being more maintainable.Also applies to: 963-974, 996-1023, 1504-1516
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/metrics-and-insights/page.tsx (1)
170-177: Apply responsive wrapping to loading skeleton for consistency.The loading skeleton still uses
wrap="nowrap", which creates an inconsistency with the main metrics group that now uses responsive wrapping. This can cause horizontal overflow during loading on mobile devices.🔎 Proposed fix for consistent responsive behavior
{isLoading && ( - <Group wrap="nowrap"> + <Group wrap={isSmallScreen ? "wrap" : "nowrap"}> <Skeleton h={168} /> <Skeleton h={168} /> <Skeleton h={168} /> <Skeleton h={168} /> </Group> )}Based on past review comments, this was previously identified as needing the same responsive treatment as the main metrics group.
🧹 Nitpick comments (1)
apps/api/src/app/email/services/send-email.service.ts (1)
11-13: Optional: Consider removing unusedemailTemplatesconstant.The local
emailTemplatesconstant is only used to derive theEmailTypetype, butEmailTypeitself doesn't appear to be used anywhere in this file. TheenqueueEmailfunction usesBuildEmailTemplatefromemail-template.serviceinstead. Consider removing this if it's not needed, or add a comment explaining its future purpose.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/app/email/services/send-email.service.tsapps/api/src/app/metrics/services/dora-metrics.integration.test.tsapps/web/src/app/metrics-and-insights/page.tsxdocker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 35
File: packages/graphql-types/api.ts:180-180
Timestamp: 2024-12-05T04:58:17.468Z
Learning: In this project, updates to the Prisma schema that require a migration are handled in separate PRs.
🧬 Code graph analysis (2)
apps/web/src/app/metrics-and-insights/page.tsx (3)
apps/web/src/providers/screen.provider.ts (1)
useScreenSize(4-10)apps/web/src/providers/date.provider.ts (1)
humanizeDuration(19-21)apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx (1)
CardDoraMetric(18-86)
apps/api/src/app/email/services/send-email.service.ts (4)
apps/api/src/app/email/services/email-template.service.ts (2)
emailTemplates(3-5)BuildEmailTemplate(13-16)apps/api/src/lib/email.ts (1)
EmailPayload(7-7)apps/api/src/env.ts (1)
env(6-111)apps/api/src/lib/logger.ts (1)
logger(21-76)
🔇 Additional comments (14)
apps/web/src/app/metrics-and-insights/page.tsx (6)
1-36: LGTM! Import organization looks good.The updated imports properly support the new routing, responsive behavior, and date handling features. All additions appear to be utilized in the component.
42-42: Good addition of responsive screen detection.The
isSmallScreenhook properly enables responsive layout behavior for the DORA metrics cards.
127-127: Excellent responsive fix for the metrics group.The responsive wrapping properly addresses the horizontal overflow concern raised in previous reviews. Cards will now stack on mobile devices while remaining in a row on larger screens.
140-144: Proper human-readable duration formatting.The lead time amount correctly uses
humanizeDurationfor a more user-friendly display (e.g., "3 hours ago" format), with an appropriate fallback to "0" when no data is available.
151-151: Appropriate percentage formatting for failure rate.The "%" suffix improves clarity and aligns with user expectations for a rate metric. The
.toString()method properly handles both Number and BigInt types.
157-166: Consistent duration formatting for MTTR.The MTTR formatting follows the same pattern as lead time, using
humanizeDurationfor a user-friendly display. This consistency across similar metrics is good.apps/api/src/app/email/services/send-email.service.ts (1)
16-27: Good defensive structure with early return guard.The
enqueueEmailfunction is well-structured with appropriate early-exit logic and clear separation of concerns. Once theenvimport issue is resolved, this implementation will work correctly.apps/api/src/app/metrics/services/dora-metrics.integration.test.ts (7)
2-25: LGTM: Import reorganization supports timeline helpers.The addition of
generateDeploymentTimelineandgenerateIncidentResolutionTimelinehelpers improves test maintainability by reducing code duplication across tests that require timeline data.
69-69: LGTM: Consistent seedDeployment API usage.The migration to passing
deployedAtandauthorIdas a composite object improves the API clarity. The pattern is used consistently throughout all test cases.Also applies to: 143-143, 210-210
81-81: LGTM: DateRange formatting simplification.The single-line dateRange format is more compact and improves readability without changing functionality.
Also applies to: 154-154, 232-232
1541-1545: LGTM: BigInt division correctly implements hours conversion.The BigInt division truncates rather than rounds, but the test appropriately uses range assertions (
toBeGreaterThanandtoBeLessThan) to accommodate this behavior. The arithmetic correctly converts milliseconds to hours.
918-930: LGTM: Timeline helpers improve test maintainability.The introduction of
generateDeploymentTimelineandgenerateIncidentResolutionTimelinehelpers reduces code duplication and makes test scenarios more readable through configuration-based setup.Also applies to: 1504-1516
87-100: LGTM: Comprehensive chart data validation.The test appropriately validates:
- Column/data alignment
- Date column validity
- Data value finiteness and non-negativity
- Presence of actual data points
The use of
Number(d)for validation handles potential BigInt chart values correctly.
86-86: No action needed. ThecurrentAmountandpreviousAmountfields are computed values returned by the metrics service functions—not database fields. The BigInt conversion in test assertions (lines 86, 160, 238, 300–303, 838, 1541, 1583–1585) aligns with the service layer's return types and requires no Prisma schema migration.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/app/email/services/send-email.service.tsapps/web/src/app/metrics-and-insights/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 35
File: packages/graphql-types/api.ts:180-180
Timestamp: 2024-12-05T04:58:17.468Z
Learning: In this project, updates to the Prisma schema that require a migration are handled in separate PRs.
🧬 Code graph analysis (1)
apps/api/src/app/email/services/send-email.service.ts (4)
apps/api/src/app/email/services/email-template.service.ts (2)
emailTemplates(3-5)BuildEmailTemplate(13-16)apps/api/src/lib/email.ts (1)
EmailPayload(7-7)apps/api/src/env.ts (1)
env(6-111)apps/api/src/lib/logger.ts (1)
logger(21-76)
🔇 Additional comments (6)
apps/api/src/app/email/services/send-email.service.ts (4)
18-21: Good defensive guard.The early return when
EMAIL_ENABLEDis falsy prevents unnecessary queue operations and clearly logs the skip reason. This is a clean implementation of feature-flag gating.
32-35: Consistent guard implementation.The early return guard matches the pattern in
enqueueEmailand appropriately useslogger.infoto signal configuration-based skipping.
37-53: LGTM!The email sending logic is well-structured:
- Obtains client via
getEmailClient()- Routes non-production emails to Resend's test address
- Includes proper error handling with Sentry integration
- Returns the response data
28-30: All call sites properly updated for the breaking signature change.The
sendEmailfunction signature correctly requirespayload: SendEmailPayloadas the first parameter. The single call site insend-email.worker.ts:24properly invokes the function by passing the payload as the first argument:await sendEmail({ ...template, ...job.data.payload, }). No issues found.apps/web/src/app/metrics-and-insights/page.tsx (2)
127-127: Responsive wrapping implemented correctly.The conditional wrapping logic
wrap={isSmallScreen ? "wrap" : "nowrap"}properly addresses the previous review concern about horizontal overflow on smaller screens. Both the loaded metric cards (line 127) and the loading skeleton state (line 171) now adapt responsively, ensuring a good user experience across all viewport sizes.Also applies to: 171-171
151-151: Good UX improvement with percentage suffix.Appending the "%" suffix to the failure rate clearly communicates the metric's unit, improving readability. The null-safe implementation with optional chaining and fallback to "0" is appropriate.
| @@ -1,23 +1,25 @@ | |||
| import { InitialSyncCompleteEmail } from "@sweetr/email-templates"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full send-email.service.ts file to understand its current state
cat -n apps/api/src/app/email/services/send-email.service.ts | head -70Repository: sweetr-dev/sweetr.dev
Length of output: 2274
🏁 Script executed:
# Check for exports from send-email.service.ts
rg -n "export" apps/api/src/app/email/services/send-email.service.ts --type=tsRepository: sweetr-dev/sweetr.dev
Length of output: 379
🏁 Script executed:
# Search for imports of EmailType from send-email.service.ts
rg -n "import.*EmailType.*from.*send-email" --type=tsRepository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Search for imports of emailTemplates from send-email.service.ts
rg -n "import.*emailTemplates.*from.*send-email" --type=tsRepository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check email-template.service.ts to see the duplicate mapping
cat -n apps/api/src/app/email/services/email-template.service.ts | head -20Repository: sweetr-dev/sweetr.dev
Length of output: 587
🏁 Script executed:
# Double-check if EmailType is used anywhere in the codebase outside of send-email.service.ts
rg "EmailType" --type=ts --type=tsx -g "!send-email.service.ts"Repository: sweetr-dev/sweetr.dev
Length of output: 92
🏁 Script executed:
# Check if the emailTemplates constant from send-email.service.ts is used anywhere
rg "emailTemplates.*initialSyncComplete|initialSyncComplete.*emailTemplates" --type=ts --type=tsxRepository: sweetr-dev/sweetr.dev
Length of output: 92
🏁 Script executed:
# Check if EmailType is used anywhere in the codebase outside of send-email.service.ts
rg "EmailType" --type=ts -g "!send-email.service.ts"Repository: sweetr-dev/sweetr.dev
Length of output: 47
🏁 Script executed:
# Check if the emailTemplates constant from send-email.service.ts is used anywhere
rg "emailTemplates.*initialSyncComplete|initialSyncComplete.*emailTemplates" --type=tsRepository: sweetr-dev/sweetr.dev
Length of output: 192
Remove unused imports and dead code.
The InitialSyncCompleteEmail import (line 1), emailTemplates constant (line 10), and EmailType type (line 12) are not used anywhere in this file. email-template.service.ts already provides the authoritative emailTemplates mapping and EmailTemplate type. Remove these unused declarations to eliminate code duplication and dead code.
🤖 Prompt for AI Agents
In apps/api/src/app/email/services/send-email.service.ts around lines 1 to 12,
remove the unused import "InitialSyncCompleteEmail" (line 1) and delete the
duplicated/dead declarations "emailTemplates" (line ~10) and the "EmailType"
type (line ~12); instead rely on the authoritative emailTemplates mapping and
EmailTemplate type exported from email-template.service.ts (import them if not
already used) so the file has no unused imports or duplicate definitions.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Style / Tooling
✏️ Tip: You can customize this high-level summary in your review settings.