Website: Clean up RecipientStatsCalculator and various small fixes/adjustments #1045
Conversation
mkue
requested review from
andrashee,
ssandino and
socialincome-dev
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes enhance type safety, refactor recipient statistics processing, adjust test cases, and update UI components. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Calculator as RecipientStatsCalculator
UI->>Calculator: Call allStats()
Calculator->>Calculator: Compute recipientsCountByStatus()
Calculator->>Calculator: Compute recipientsCountByOrganisationAndStatus()
Calculator-->>UI: Return aggregated stats
sequenceDiagram
participant Page
participant Redirector
Page->>Page: Evaluate currency value
alt Currency is truthy
Page->>Redirector: Invoke redirect()
else Currency is falsy
Note right of Page: No redirect occurs
end
Suggested labels
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (1)
ui/src/components/card/glow-hover-card.tsx (1)
13-13: Consider using modern ref patterns.The
LegacyReftype casting could be replaced withReact.RefObjectfor better type safety.-ref={refCard as LegacyRef<HTMLDivElement>} +ref={refCard as React.RefObject<HTMLDivElement>}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
shared/src/types/contribution.ts(2 hunks)shared/src/utils/stats/RecipientStatsCalculator.test.ts(6 hunks)shared/src/utils/stats/RecipientStatsCalculator.ts(1 hunks)ui/src/components/card/glow-hover-card.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx(2 hunks)website/src/app/[lang]/[region]/(website)/transparency/recipient-selection/page.tsx(1 hunks)website/src/components/navbar/navbar-client.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
website/src/app/[lang]/[region]/(website)/transparency/recipient-selection/page.tsx (1)
Learnt from: DarkMenacer
PR: socialincome-san/public#1001
File: website/src/app/[lang]/[region]/(website)/transparency/recipient-selection/page.tsx:10-12
Timestamp: 2025-01-08T15:36:41.682Z
Learning: In Next.js, when implementing redirects where you don't want the current URL to be stored in browser history (e.g., intermediate pages), use `redirect` from 'next/navigation' instead of `useRouter.push()`. This prevents the back button from getting stuck in a redirect loop and provides a more intuitive navigation experience.
🪛 Biome (1.9.4)
shared/src/utils/stats/RecipientStatsCalculator.ts
[error] 42-42: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 54-54: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 57-57: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test website
- GitHub Check: Test shared code
- GitHub Check: Test functions
- GitHub Check: Test admin
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (8)
shared/src/types/contribution.ts (2)
2-2: LGTM!Clean import addition for Campaign type.
37-37: LGTM! Type safety improvement.Good refinement of campaign_path to explicitly reference Campaign documents.
website/src/app/[lang]/[region]/(website)/transparency/recipient-selection/page.tsx (1)
10-12: LGTM! Good use of conditional redirect.The code correctly uses
redirectfrom 'next/navigation' and includes a proper currency check.website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx (1)
41-44: LGTM! Good use of type-safe enums.The calculation correctly excludes waitlisted recipients using the RecipientProgramStatus enum.
website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx (1)
48-49: LGTM: Enhanced recipient statistics accessThe change to use
recipientsCountByOrganisationAndStatusprovides more detailed recipient data by status.shared/src/utils/stats/RecipientStatsCalculator.test.ts (2)
31-41: LGTM: Improved organization referencesUsing proper Firestore references instead of inline objects enhances type safety and maintainability.
135-167: LGTM: Comprehensive test coverageThe test cases thoroughly verify:
- Total recipient counts
- Distribution by status
- Organization-specific counts
- Edge cases (undefined statuses)
website/src/components/navbar/navbar-client.tsx (1)
292-296: LGTM: Improved navigation structureThe reordering of links and addition of spacing enhances the visual hierarchy and user experience.
|
Visit the preview URL for this PR (updated for commit c2dbcf3): https://si-admin-staging--pr1045-michael-cleanup-reci-95krfwp3.web.app (expires Thu, 20 Feb 2025 08:36:46 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
| <div className="absolute left-0 mt-[50px] hidden flex-col justify-start overflow-visible whitespace-nowrap group-hover/navbar:flex group-active/navbar:flex"> | ||
| <NavbarLink href={`/${lang}/${region}/me`}>{translations.myProfile}</NavbarLink> | ||
| <div className="absolute left-0 mt-[50px] hidden flex-col justify-start space-y-2 overflow-visible whitespace-nowrap group-hover/navbar:flex group-active/navbar:flex"> | ||
| <NavbarLink href={`/${lang}/${region}/journal`}>{translations.journal}</NavbarLink> |
There was a problem hiding this comment.
Placing the journal link at the top level was intentional (discussed with @Gavriil-Tzortzakis, @ssandino and Katie, who made the changes in the Figma). However, we can revisit this in a later commit.
There was a problem hiding this comment.
Ok, sorry🙃 The spacing is too narrow imo, and also the Donate with the yellow font in the middle looks a bit strange in my eyes. I'll revert.
Summary by CodeRabbit
Bug Fixes
Refactor
Style