feat(business-report): upgrade user-facing part of the social media data UI (BAL-2577) #2911
Conversation
|
|
Warning Rate limit exceeded@r4zendev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
WalkthroughThis pull request introduces several modifications to the UI package, including updates to the package configuration, component refactoring, and removal of certain adapters and components. The changes involve updating the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 1
🧹 Nitpick comments (4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (2)
114-116: Consider extracting and reusing shared logic.You have a
TODOindicating that this component can be decoupled and reused more broadly. Extracting the Ads/UI logic into smaller, composable pieces may improve maintainability and reduce duplication across other social media or ads components.Would you like me to open a detailed issue or provide a sample refactor to break out these responsibilities?
221-227: Remove or replace constant conditionals.Using
false ? 'opacity-100' : 'opacity-60'andfalse ? ( ... ) : ( ... )will never evaluate the true branch. This is likely a leftover placeholder. Consider replacing it with a dynamic condition or removing it altogether.Below is a suggested snippet:
- false ? 'opacity-100' : 'opacity-60', + adsImages?.length ? 'opacity-100' : 'opacity-60', - {false ? ( - <>The ads should be displayed here</> - ) : ( + {adsImages?.length ? ( + <>The ads should be displayed here</> + ) : ( <div className="flex items-center gap-2 text-gray-400"> ... </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/icons/FacebookIcon.tsx (1)
1-23: Consider adding ARIA attributes for accessibility.Including
role="img"and anaria-label(or title) helps screen readers describe the icon.-export const FacebookIcon = ({ className }: { className?: string }) => ( +export const FacebookIcon = ({ className }: { className?: string }) => { + const label = 'Facebook Icon'; + return ( + <svg + xmlns="http://www.w3.org/2000/svg" + viewBox="0 0 48 48" + className={className} + role="img" + aria-label={label} + > ... + </svg> + ); +}packages/ui/src/components/templates/report/components/AdsAndSocialMedia/icons/InstagramIcon.tsx (1)
1-47: Add ARIA acknowledgment for screen readers.As with the Facebook icon, providing
role="img"and anaria-labelwill make the icon more accessible.-export const InstagramIcon = ({ className }: { className?: string }) => ( +export const InstagramIcon = ({ className }: { className?: string }) => { + const label = 'Instagram Icon'; + return ( + <svg + xmlns="http://www.w3.org/2000/svg" + viewBox="0 0 48 48" + className={className} + role="img" + aria-label={label} + > ... + </svg> + ); +}
📜 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)
packages/ui/package.json(2 hunks)packages/ui/src/components/atoms/Button/Button.tsx(1 hunks)packages/ui/src/components/atoms/Image/interfaces.ts(1 hunks)packages/ui/src/components/templates/report/adapters/ads-provider-adapter/ads-provider-adapter.ts(0 hunks)packages/ui/src/components/templates/report/adapters/ads-provider-adapter/index.ts(0 hunks)packages/ui/src/components/templates/report/adapters/create-report-adapter/index.ts(0 hunks)packages/ui/src/components/templates/report/adapters/index.ts(0 hunks)packages/ui/src/components/templates/report/adapters/report-adapter.ts(4 hunks)packages/ui/src/components/templates/report/adapters/report-adapter/index.ts(0 hunks)packages/ui/src/components/templates/report/components/AdImageWithLink/AdImageWithLink.tsx(0 hunks)packages/ui/src/components/templates/report/components/AdImageWithLink/index.ts(0 hunks)packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx(1 hunks)packages/ui/src/components/templates/report/components/AdsAndSocialMedia/icons/FacebookIcon.tsx(1 hunks)packages/ui/src/components/templates/report/components/AdsAndSocialMedia/icons/InstagramIcon.tsx(1 hunks)packages/ui/src/components/templates/report/components/index.ts(0 hunks)packages/ui/src/components/templates/report/hooks/useReportTabs/useReportTabs.tsx(1 hunks)
💤 Files with no reviewable changes (8)
- packages/ui/src/components/templates/report/components/index.ts
- packages/ui/src/components/templates/report/adapters/index.ts
- packages/ui/src/components/templates/report/adapters/create-report-adapter/index.ts
- packages/ui/src/components/templates/report/adapters/ads-provider-adapter/index.ts
- packages/ui/src/components/templates/report/adapters/ads-provider-adapter/ads-provider-adapter.ts
- packages/ui/src/components/templates/report/components/AdImageWithLink/index.ts
- packages/ui/src/components/templates/report/components/AdImageWithLink/AdImageWithLink.tsx
- packages/ui/src/components/templates/report/adapters/report-adapter/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
[error] 144-144: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (12)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (1)
144-144: Verify custom shadow class usage.
shadow-l is not a standard Tailwind utility class and may be a typo or custom style. Ensure it’s intentional; otherwise, change it to a standard utility (e.g., shadow-lg) for consistency.
🧰 Tools
🪛 Biome (1.9.4)
[error] 144-144: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
packages/ui/src/components/atoms/Image/interfaces.ts (1)
7-8: LGTM on making width and height optional.
Allowing flexible dimensions improves reusability and responsiveness.
packages/ui/src/components/atoms/Button/Button.tsx (1)
18-19: New variant added successfully.
The new “browserLink” variant appears consistent with the existing style strategy.
packages/ui/src/components/templates/report/hooks/useReportTabs/useReportTabs.tsx (2)
2-2: Imports look consistent.
All imported modules are utilized appropriately within this file.
6-12: Refactored import block for clarity.
These imports are reorganized to include the AdsAndSocialMedia, WebsitesCompany, WebsiteLineOfBusiness, and WebsiteCredibility components. This is consistent with their usage below, ensuring minimal confusion and better readability.
packages/ui/src/components/templates/report/adapters/report-adapter.ts (5)
1-3: New imports enhance code clarity.
Using booleanToYesOrNo from @ballerine/common is a clean way to transform boolean fields into more descriptive text. This reduces code duplication.
25-49: Introduce toSocialMediaPresence function for structured data.
This function cleanly extracts and organizes social media data for Facebook and Instagram. The usage of booleanToYesOrNo for isBusinessAccount and isVerified is a succinct approach. Ensure any additional platforms in the future follow the same pattern for consistency.
51-65: Introduce toAdsImages function for ad images data.
Similarly, toAdsImages extracts and organizes Facebook and Instagram ad images. This design fosters separation of concerns, making it easier to extend or add new properties if more platforms or image types are needed.
84-84: Accurate mapping for adsAndSocialMediaPresence.
By calling toSocialMediaPresence(report?.socialMedia) directly, the code becomes more legible, removing the need for multi-step transformations. This streamlined approach leads to improved maintainability.
145-145: Include adsImages from toAdsImages.
Generating adsImages in a dedicated function keeps data transformations centralized, reducing complexity in the main adapter logic. Good move!
packages/ui/package.json (2)
21-21: New build watch script added.
The build:watch script will be helpful for rapid feedback in development.
57-57: Version bump for lucide-react.
Upgrading from ^0.144.0 to ^0.245.0 may introduce new icons and experimental API changes. Please test thoroughly to ensure backward compatibility of existing icons.
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (4)
1-27: Consider optimizing icon importsThe large number of individual icon imports could be optimized using a namespace import.
-import { - BanIcon, - BriefcaseIcon, - CalendarIcon, - CheckIcon, - InfoIcon, - LinkIcon, - MailIcon, - MapPinIcon, - PhoneIcon, - TagIcon, - ThumbsUpIcon, - UsersIcon, -} from 'lucide-react'; +import * as LucideIcons from 'lucide-react';This would make the code more maintainable and reduce the number of import lines.
28-74: Consider extracting social media mapper configurationThe social media mapper configuration is well-structured but tightly coupled to the component. Consider moving it to a separate configuration file for better maintainability and reusability.
Create a new file
socialMediaConfig.ts:import * as LucideIcons from 'lucide-react'; import { FacebookIcon } from './icons/FacebookIcon'; import { InstagramIcon } from './icons/InstagramIcon'; export const socialMediaConfig = { facebook: { icon: <FacebookIcon className="h-8 w-8" />, fields: { // ... existing fields }, }, instagram: { icon: <InstagramIcon className="h-8 w-8" />, fields: { // ... existing fields }, }, } as const;
100-112: Enhance URL validation in cleanLink functionThe cleanLink function could be more robust in handling edge cases and invalid URLs.
const cleanLink = (link: string) => { - if (!link || !z.string().url().safeParse(link).success) { + if (!link?.trim() || !z.string().url().safeParse(link).success) { return 'N/A'; } - let { hostname, pathname } = new URL(link); + try { + let { hostname, pathname } = new URL(link); - if (hostname.startsWith('www.')) { - hostname = hostname.slice(4); - } + hostname = hostname.replace(/^www\./, ''); + pathname = pathname === '/' ? '' : pathname; - return `${hostname}${pathname}`; + return `${hostname}${pathname}`; + } catch (error) { + return 'N/A'; + } };
136-210: Consider extracting social media card rendering logicThe social media card rendering logic is complex and could benefit from being extracted into a separate component for better readability and maintainability.
Consider creating a
SocialMediaCardcomponent:type SocialMediaCardProps = { provider: string; mediaPresence: AdsAndSocialMediaProps['mediaPresence'][keyof AdsAndSocialMediaProps['mediaPresence']]; adsImage: AdsAndSocialMediaProps['adsImages'][keyof AdsAndSocialMediaProps['adsImages']]; }; const SocialMediaCard: FunctionComponent<SocialMediaCardProps> = ({ provider, mediaPresence, adsImage, }) => { // Extract current card rendering logic here };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Show resolved
Hide resolved
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (3)
28-74: Consider extracting social media mapper to a separate fileThe social media mapper configuration is well-structured but could be moved to a separate file for better maintainability and reusability.
Consider creating a new file
socialMediaConfig.ts:import { ReactNode } from 'react'; import { FacebookIcon } from './icons/FacebookIcon'; import { InstagramIcon } from './icons/InstagramIcon'; import { CalendarIcon, PhoneIcon, // ... other icons } from 'lucide-react'; export const socialMediaMapper = { facebook: { // ... existing configuration }, instagram: { // ... existing configuration }, } as const;
100-112: Enhance URL validation error handlingThe
cleanLinkfunction could benefit from more robust error handling for malformed URLs.Consider this enhanced implementation:
const cleanLink = (link: string) => { - if (!link || !z.string().url().safeParse(link).success) { + if (!link) { return 'N/A'; } + + try { + if (!z.string().url().safeParse(link).success) { + return 'Invalid URL'; + } - let { hostname, pathname } = new URL(link); + let { hostname, pathname } = new URL(link); - if (hostname.startsWith('www.')) { - hostname = hostname.slice(4); - } + if (hostname.startsWith('www.')) { + hostname = hostname.slice(4); + } - return `${hostname}${pathname}`; + return `${hostname}${pathname}`; + } catch (error) { + console.error('Error parsing URL:', error); + return 'Invalid URL'; + } };
124-236: Consider breaking down the component for better maintainabilityThe component has grown complex with multiple responsibilities. Consider splitting it into smaller, focused components:
SocialMediaSectionAdsSectionSocialMediaCardWould you like me to help generate the component structure?
🧰 Tools
🪛 Biome (1.9.4)
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (3)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (3)
1-27: LGTM! Well-organized imports
The imports are logically grouped and all appear to be used within the component.
114-115: Component decoupling reminder
The TODO comments about component decoupling are still present and should be addressed.
214-232: Remove hardcoded conditions in ads section
The ads section contains hardcoded false conditions that should be replaced with actual logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 221-221: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 225-225: Unexpected constant condition.
(lint/correctness/noConstantCondition)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Great work Sasha! Two comments:
- A new version should be published. You can do it with 2 commands:
pnpm changesetand thenpnpm version-packages - Please hide the ads section. Currently it doesn't work well, we prefer to not show this section to our clients at the moments.
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Outdated
Show resolved
Hide resolved
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (4)
28-74: Consider moving socialMediaMapper to a separate configuration file.The social media mapper configuration is well-structured and type-safe, but it would be better organized in a dedicated configuration file to improve maintainability and separation of concerns.
Create a new file
socialMediaConfig.ts:import { ReactNode } from 'react'; import { FacebookIcon } from './icons/FacebookIcon'; import { InstagramIcon } from './icons/InstagramIcon'; import { CalendarIcon, PhoneIcon, MailIcon, MapPinIcon, ThumbsUpIcon, TagIcon, BriefcaseIcon, CheckIcon, UsersIcon, InfoIcon, } from 'lucide-react'; import type { AdsAndSocialMediaProps } from './AdsAndSocialMedia'; export const socialMediaMapper = { // ... existing configuration } as const;
100-112: Enhance URL validation and error handling in cleanLink function.While the function handles basic URL validation, it could be improved to handle more edge cases.
const cleanLink = (link: string) => { - if (!link || !z.string().url().safeParse(link).success) { + if (!link) { return 'N/A'; } + + try { + if (!z.string().url().safeParse(link).success) { + return 'N/A'; + } - let { hostname, pathname } = new URL(link); + let { hostname, pathname, search } = new URL(link); - if (hostname.startsWith('www.')) { - hostname = hostname.slice(4); - } + hostname = hostname.replace(/^www\./, ''); + pathname = pathname.replace(/\/$/, ''); - return `${hostname}${pathname}`; + return `${hostname}${pathname}${search}`; + } catch { + return 'N/A'; + } };
214-231: Consider using a placeholder component instead of commenting out code.Rather than commenting out the ads section, consider creating a placeholder component that clearly indicates the work-in-progress status.
const AdsPlaceholder: FC = () => ( <div> <h3 className="mb-2 text-base font-bold">Ads</h3> <Card className="flex w-full justify-between p-4 shadow-lg opacity-60"> <div className="flex items-center gap-2 text-gray-400"> <BanIcon className="h-5 w-5" /> <span className="text-sm">Ads section coming soon</span> </div> </Card> </div> );
136-142: Add type safety for provider access.The current implementation assumes the provider will always be either 'facebook' or 'instagram'. Add type safety to prevent potential runtime errors.
-AdsProviders.map(toLowerCase).map(provider => { +AdsProviders.map(toLowerCase).map((provider: keyof typeof socialMediaMapper) => { const { page, ...rest } = mediaPresence[provider] ?? {}; const { src, link } = adsImages[provider] ?? {}; // || because empty string is not a valid case const idValue = ('id' in rest ? rest.id : rest.userName) || null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/react-pdf-toolkit/CHANGELOG.md
- apps/kyb-app/package.json
- packages/react-pdf-toolkit/package.json
- apps/kyb-app/CHANGELOG.md
🔇 Additional comments (3)
packages/ui/src/components/templates/report/components/AdsAndSocialMedia/AdsAndSocialMedia.tsx (3)
1-27: LGTM! Imports are well-organized and aligned with PR objectives.
The imports are logically grouped and include the new Ban icon from the upgraded lucide-react library as mentioned in the PR objectives.
116-123: Props type is well-defined using adapter return types.
The use of adapter return types for props provides good type safety and maintainability.
193-199: Add error handling for image loading.
The Image component should handle loading errors gracefully.
Examples of the UI:
In 1920x1080 aspect ratio:
With disabled states:
Small explanation as to why the customers tab in the backoffice application was throwing 401 for me:
In
services/workflows-service/src/common/middlewares/auth-key.middleware.tsthere is a middleware that assigns thecustomerrole to any request going through it. Changing the role toadminlocally allowed me to make a request and see the customers tab. Just a note for you in case it needs to be fixed.Note: we are missing decent loading states.
Note 2: I wasn't sure how we maintain our change logs and bump packages, so I just bumped the ui package and updated the
CHANGELOG.mdmanually. I sawchangesetsbut IIRC they keep the changelog for the entire monorepo and store it in the root. I didn't see any kind of npm script or anything similar in theuipackage. Let me know if I should do something here.Summary by CodeRabbit
Summary by CodeRabbit
New Features
build:watchscript for building the project in watch mode.browserLink, to enhance visual presentation.FacebookIconandInstagramIconadded for social media representation.AdsAndSocialMediacomponent with improved layout and functionality.toSocialMediaPresenceandtoAdsImagesfor structured data extraction.Bug Fixes
widthandheightproperties optional in theImagePropsinterface.Removals
AdImageWithLinkcomponent and its associated exports.ads-provider-adapterandcreate-report-adapter, affecting accessibility in the application.report-adapterand its associated exports.Refactor
reportAdapterlogic for better data processing and structure.useReportTabsfor clearer component organization.