feat: improved traffic visualization (BAL-3271) #2922
Conversation
|
WalkthroughThis pull request introduces updates across multiple packages in the Ballerine ecosystem. The changes primarily focus on version increments and dependency updates, with a notable addition of a new Tooltip component in the UI package. The Changes
Possibly related PRs
Suggested reviewers
Poem
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 (3)
packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (2)
67-73: Consider renaming “montlyVisitsIndicators” to “monthlyVisitsIndicators”.
Even though the property name is carried over from an external data source, the misspelling may confuse other developers. Consider adding a small comment clarifying why the type is spelled that way (due to upstream data naming) or, if possible, safely adjusting the name to maintain consistency.
146-179: Responsive layout advice for traffic charts.
Large charts in a side-by-side layout may not behave predictably on smaller screens, especially given the reported Tailwind conflict. Consider adopting the recommended approach of prefixing or sharing the config to ensure that breakpoints work properly when rendering these charts in various screen widths.packages/ui/src/components/templates/report/adapters/report-adapter.ts (1)
182-192: Confirm safe mapping for traffic data arrays.
Your transformations rely onnormalizeHyphenedDataStringreturning valid numeric entries for thevalue. If the field is absent or non-numeric, it may cause further issues down the line. Ensure that all upstream data is validated or defaulted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(3 hunks)packages/ui/src/components/atoms/Tooltip/Tooltip.tsx(1 hunks)packages/ui/src/components/atoms/Tooltip/index.ts(1 hunks)packages/ui/src/components/atoms/index.ts(1 hunks)packages/ui/src/components/templates/report/adapters/report-adapter.ts(2 hunks)packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx(2 hunks)services/workflows-service/CHANGELOG.md(1 hunks)services/workflows-service/package.json(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/ui/src/components/atoms/Tooltip/index.ts
- packages/ui/CHANGELOG.md
- services/workflows-service/package.json
- packages/react-pdf-toolkit/CHANGELOG.md
- apps/backoffice-v2/package.json
- services/workflows-service/prisma/data-migrations
- services/workflows-service/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- apps/backoffice-v2/CHANGELOG.md
🔇 Additional comments (10)
packages/ui/package.json (2)
4-4: LGTM: Version increment follows semver
The version bump from 0.5.56 to 0.5.57 for a feature addition follows semantic versioning guidelines.
47-47: LGTM: New dependencies align with visualization requirements
The added dependencies support the PR objectives:
@radix-ui/react-tooltip: Provides accessible tooltips for monthly visitors metricsrecharts: Robust charting library for traffic visualization
Both are well-maintained packages with compatible version ranges.
Also applies to: 65-65
packages/react-pdf-toolkit/package.json (1)
4-4: LGTM: Consistent version updates
Package version and @ballerine/ui dependency are correctly synchronized with the UI package changes.
Also applies to: 30-30
apps/kyb-app/package.json (1)
4-4: LGTM: Consistent version updates
Package version and @ballerine/ui dependency are correctly synchronized with the UI package changes.
Also applies to: 20-20
packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (3)
45-57: Validate numeric and date-formatting assumptions.
The longDateFormatter and longNumberFormatter rely on assumptions about how date strings and large numbers flow in from the API. Ensure that all relevant call sites provide data matching these expectations (for instance, 'MMMM YYYY' with dayjs).
183-191: Avoid item parsing errors.
The parsing in the bar chart, pie chart, and engagement sections (which use value: parseFloat(...)) could throw NaN if the API returns unexpected text. Add minimal guards or default transformations to avoid runtime issues if the external data changes.
248-283: Tooltip usage looks great.
The approach with TooltipProvider, TooltipTrigger, and TooltipContent is consistent and clear. Awesome work.
packages/ui/src/components/atoms/index.ts (1)
26-26: Great addition of the Tooltip export.
Exporting the Tooltip components from this index file allows broader access across different modules in your application.
packages/ui/src/components/atoms/Tooltip/Tooltip.tsx (1)
13-29: Ensure consistent styling for tooltip content.
The usage of ctw and Radix primitives is solid. Just be mindful if you ever need to override default transitions or theming—this might require additional class merging or conditional logic. Overall, this is a neat and maintainable approach.
packages/ui/src/components/templates/report/adapters/report-adapter.ts (1)
75-82: Potential edge cases in string splitting.
normalizeHyphenedDataString splits on ' - '. If upstream data contains additional hyphens or no hyphens at all, the returned label and value might break your intended logic. Consider adding fallback logic or robust validation.
r4zendev
requested review from
alonp99,
MatanYadaev,
Omri-Levy and
nitzanballerine
...ages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (2)
132-133: Remove commented codeThere are several blocks of commented-out code related to responsive layouts. These should be removed to maintain code cleanliness.
Apply this cleanup:
- {/* <div className="flex flex-col 2xl:flex-row gap-4 w-full h-auto 2xl:h-96"> - <div className="h-[24rem] 2xl:h-full w-full 2xl:w-3/5"> */} + <div className="flex gap-4 h-[30rem] w-full"> - {/* <div className="flex 2xl:flex-col gap-4 h-[12rem] 2xl:h-full w-full 2xl:w-2/5"> - <div className="h-full 2xl:h-1/2 w-1/2 2xl:w-full"> */} + <div className="flex flex-col gap-4 h-full w-2/5"> - {/* <Card className="h-full 2xl:h-1/2 w-1/2 2xl:w-full"> */} + <Card className="h-1/2 w-full">Also applies to: 172-173, 232-233
134-135: Address responsive design issuesThe PR mentions issues with Tailwind CSS classes being loaded multiple times, preventing media queries from working correctly. The current implementation uses fixed proportions (w-3/5, w-2/5) which might not work well on all screen sizes.
Consider these solutions mentioned in the PR:
- Add a prefix to UI Tailwind classes
- Use the UI package from source while sharing base Tailwind config as preset
Additionally, consider using more flexible responsive classes:
- <div className="flex gap-4 h-[30rem] w-full"> - <Card className="h-full w-3/5"> + <div className="flex flex-col lg:flex-row gap-4 min-h-[30rem] w-full"> + <Card className="h-full w-full lg:w-3/5">Also applies to: 174-175, 233-234
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(3 hunks)packages/ui/src/components/atoms/Tooltip/Tooltip.tsx(1 hunks)packages/ui/src/components/atoms/Tooltip/index.ts(1 hunks)packages/ui/src/components/atoms/index.ts(1 hunks)packages/ui/src/components/templates/report/adapters/report-adapter.ts(2 hunks)packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/ui/src/components/atoms/index.ts
- packages/ui/src/components/atoms/Tooltip/index.ts
- packages/react-pdf-toolkit/CHANGELOG.md
- packages/ui/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- apps/backoffice-v2/CHANGELOG.md
- apps/kyb-app/package.json
- packages/ui/src/components/templates/report/adapters/report-adapter.ts
- packages/react-pdf-toolkit/package.json
- services/workflows-service/prisma/data-migrations
- packages/ui/package.json
- apps/backoffice-v2/package.json
- packages/ui/src/components/atoms/Tooltip/Tooltip.tsx
🔇 Additional comments (4)
packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (4)
1-23: LGTM! Clean import organization
The imports are well-organized and all dependencies are necessary for the new visualization features.
24-41: LGTM! Well-structured metrics configuration
The engagement metrics mapping is well-organized with clear descriptions and type-safe configuration.
150-150: LGTM! Efficient number formatting
Using Intl.NumberFormat with compact notation is a good choice for formatting large numbers, as discussed in the previous review comments.
53-59: Verify the "montlyVisitsIndicators" typo
There's a typo in the property name "montlyVisitsIndicators" (missing 'h'). The PR mentions this might match the external API structure.
Run this script to check the consistency of this property name across the codebase:
✅ Verification successful
Property name "montlyVisitsIndicators" is consistently used across the codebase
The property name "montlyVisitsIndicators" (without 'h') is consistently used in all occurrences across the codebase:
- In the adapter:
report-adapter.ts(2 occurrences) - In the component:
WebsiteCredibility.tsx(2 occurrences)
Since there are no occurrences of "monthlyVisitsIndicators" (with 'h') and the property name is used consistently in both the adapter and component, this appears to be an intentional naming to match the external API structure as mentioned in the PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of montlyVisitsIndicators vs monthlyVisitsIndicators
echo "Searching for 'montlyVisitsIndicators'..."
rg "montlyVisitsIndicators"
echo -e "\nSearching for 'monthlyVisitsIndicators'..."
rg "monthlyVisitsIndicators"
Length of output: 844
...ages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/components/templates/report/adapters/report-adapter.ts (1)
75-82: LGTM! Consider adding input validation.The function implementation is clean and handles edge cases well. However, consider adding input validation to handle undefined/null inputs gracefully.
const normalizeHyphenedDataString = (str: string) => { + if (!str) return { label: '', value: undefined }; const parts = str.split(' - '); return { label: parts.length > 1 ? parts.slice(0, -1).join(' - ') : parts.at(0), value: parts.at(-1), }; };packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (1)
67-80: Consider moving data transformation to the backend.The traffic sources transformation logic, including the "Other" category calculation, should ideally be handled on the backend as noted in the TODO comment. This would:
- Reduce client-side processing
- Ensure consistent data transformation across different clients
- Simplify the frontend code
Would you like me to help create a GitHub issue to track moving this transformation logic to the backend?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(3 hunks)packages/ui/src/components/atoms/Tooltip/Tooltip.tsx(1 hunks)packages/ui/src/components/atoms/Tooltip/index.ts(1 hunks)packages/ui/src/components/atoms/index.ts(1 hunks)packages/ui/src/components/templates/report/adapters/report-adapter.ts(2 hunks)packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx(7 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/ui/CHANGELOG.md
- packages/ui/src/components/atoms/Tooltip/index.ts
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/CHANGELOG.md
- apps/kyb-app/package.json
- packages/ui/src/components/atoms/index.ts
- packages/react-pdf-toolkit/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- apps/backoffice-v2/package.json
- packages/ui/package.json
- packages/react-pdf-toolkit/package.json
- packages/ui/src/components/atoms/Tooltip/Tooltip.tsx
🔇 Additional comments (4)
packages/ui/src/components/templates/report/adapters/report-adapter.ts (1)
182-192: Verify the "montly" typo with the API team.
The property name "montlyVisitsIndicators" is missing an 'h'. While this might match the external API structure, it's worth confirming if this should be corrected for consistency.
✅ Verification successful
Let me check the specific usage of "montly" in the files to understand the context better.
Let me check for type definitions using a different approach.
Consistent use of "montly" spelling suggests it matches API contract
Based on the search results, we found that "montly" (without 'h') is consistently used in both the adapter (report-adapter.ts) and the component (WebsiteCredibility.tsx). The fact that it appears in the type definition in WebsiteCredibility.tsx as a key in Record<'trafficSources' | 'montlyVisitsIndicators' | 'engagements',...> further suggests this spelling is intentional and matches the API contract.
- The spelling is consistent across the codebase
- No instances of "monthly" were found in related frontend code
- The type definitions reinforce this spelling choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of "montly" vs "monthly" in the codebase
echo "Searching for 'montly' occurrences:"
rg "montly" -l
echo -e "\nSearching for 'monthly' occurrences:"
rg "monthly" -l
Length of output: 542
Script:
#!/bin/bash
# Check the context of "montly" usage
echo "Context in report-adapter.ts:"
rg "montly" -C 2 packages/ui/src/components/templates/report/adapters/report-adapter.ts
echo -e "\nContext in WebsiteCredibility.tsx:"
rg "montly" -C 2 packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx
# Check if there's a type definition that might show the API interface
echo -e "\nSearching for potential type definitions:"
rg "interface.*Traffic" -A 10
Length of output: 1322
Script:
#!/bin/bash
# Search for type definitions related to traffic analysis
echo "Searching for traffic analysis types:"
ast-grep --pattern 'interface $_ {
$$$
trafficAnalysis?: {
$$$
}
$$$
}'
# Also check for type imports that might define these structures
echo -e "\nSearching for type imports:"
rg "import.*type.*Traffic" -A 2
Length of output: 275
packages/ui/src/components/templates/report/components/WebsiteCredibility/WebsiteCredibility.tsx (3)
24-41: LGTM! Well-structured engagement metrics mapping.
The constant provides clear descriptions and formatting rules for each metric, making it maintainable and type-safe.
154-181: LGTM! Clean chart implementation with proper number formatting.
The monthly visitors chart implementation is well-structured and follows the recommended number formatting approach using Intl.NumberFormat.
245-282:
Address Tailwind CSS class conflicts.
The engagement metrics UI implementation is clean and user-friendly. However, as mentioned in the PR objectives, there are Tailwind CSS class conflicts affecting responsiveness. Consider:
- Adding a prefix to UI Tailwind classes
- Using the UI package from source with a shared base configuration
This PR improves the data visualization in the "Website Credibility" section of the merchant monitoring report.
Examples
Full HD screen view
Monthly visitors tooltip
Engagements metrics tooltip
Note (Tailwind conflict)
Responsiveness is not completely preserved for smaller laptops (although the design still looks fine), because tailwind classes are being loaded twice (from
uipackage andbackofficeapps), which makes them override each other, so basically none of the media-queries work, which makes it impossible to add responsiveness to the design using tailwind for now. The idea was to move two rightmost blocks to the bottom so that they take up 1/2 of the entire space width and let the bar chart take the entire width too, like so:However, right now the design looks the same for all kinds of screens. I left code that could be uncommented when the Tailwind issue is resolved to add responsiveness and better looks for smaller screens.
To resolve the issue in question two possible solutions could be implemented:
ui-and rewrite all of the classes in this package to use this prefixNote 2 (typo)
There was a typo in the data object processed by report-adapter -
montlyVisitsIndicators(note the absence ofhinmonthly). I left it as-is, because apparently this typo was also in the seeding scripts, so I was afraid that this can also be the data shape coming from external API. I left it in there and I need confirmation on whether this typo should be fixed.Summary by CodeRabbit
New Features
Dependencies
@ballerine/uito version 0.5.57@ballerine/react-pdf-toolkitto version 1.2.57@radix-ui/react-tooltipandrechartsdependenciesImprovements