Notify Clients on Credit Limit Reached #2997
Conversation
|
WalkthroughThe pull request introduces changes across several files, focusing on enhancing error messages in the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/workflows-service/src/business-report/business-report.service.ts (2)
34-34: Consider including the actual limit in the error message.While the new message effectively encourages users to contact support for additional features, it removes transparency about the actual quota limit. This could frustrate users who need this information for immediate decision-making.
Consider this balanced approach:
- `You've hit your reports limit. Talk to us to unlock additional features and continue effective risk management with Ballerine.`, + `You've reached your limit of ${maxBusinessReports} reports. Contact us to unlock additional features and continue effective risk management with Ballerine.`,
Line range hint
123-127: Fix typo in error message.There's a spelling error in the error message: "qouta" should be "quota".
- } left from a qouta of ${maxBusinessReports})`, + } left from a quota of ${maxBusinessReports})`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/workflows-service/src/business-report/business-report.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
- GitHub Check: test_linux
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backoffice-v2/src/domains/business-reports/fetchers.ts (1)
26-26: Consider consistent date transformation across schema.The
displayDatefield doesn't transform the string to a Date object, unlikecreatedAtandupdatedAt. This inconsistency might lead to different date handling across the application.- displayDate: z.string().datetime(), + displayDate: z.string().datetime().transform(value => new Date(value)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/backoffice-v2/src/domains/business-reports/fetchers.ts(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/components/MerchantMonitoringTable/columns.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx(1 hunks)services/workflows-service/src/business-report/merchant-monitoring-client.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/backoffice-v2/src/pages/MerchantMonitoring/components/MerchantMonitoringTable/columns.tsx (1)
Line range hint
156-173: LGTM! Clean migration to displayDate.The change from
createdAttodisplayDateis implemented correctly, maintaining the existing UTC to local time conversion and formatting logic.services/workflows-service/src/business-report/merchant-monitoring-client.ts (2)
53-56: LGTM! Consistent date handling implementation.The
displayDatefield is properly implemented with datetime validation and Date object transformation, maintaining consistency with other date fields in the schema.
Line range hint
1-1: Verify PR title and implementation alignment.The PR is titled "Notify Clients on Credit Limit Reached", but the changes only introduce a new date display field. Either the PR title needs updating, or the notification implementation is missing.
| {businessReport?.displayDate && | ||
| dayjs(new Date(businessReport?.displayDate)).format('HH:mm MMM Do, YYYY')} |
There was a problem hiding this comment.
💡 Codebase verification
Date formatting and timezone handling needs standardization
The current implementation has several issues:
- The date format
'HH:mm MMM Do, YYYY'is inconsistent with other date formats in the codebase - Redundant
new Date()wrapper is unnecessary as dayjs can parse the date string directly - Timezone handling should be explicitly defined (UTC vs local) for consistency
Suggested changes:
dayjs(businessReport?.displayDate).format('DD/MM/YYYY HH:mm')🔗 Analysis chain
LGTM! Verify date handling across time zones.
The change from createdAt to displayDate is consistent with the schema updates. The date formatting remains correct.
Please ensure that:
- The date is correctly displayed across different time zones
- The format
'HH:mm MMM Do, YYYY'is consistently used across the application - Dayjs plugins (utc, timezone) are properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent date formatting across the codebase
echo "Checking for date format consistency:"
rg -l "dayjs\(.*\)\.format\(" | xargs rg "format\(['\"].*['\"]\)"
Length of output: 8189
Summary by CodeRabbit
displayDateproperty to enhance business report data structures.displayDateinstead ofcreatedAt.