-
-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Website: Transparency page (fixing bugs) #972
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@dnhn is attempting to deploy a commit to the Social Income Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx
(2 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-2.tsx
(7 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3.tsx
(1 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-4.tsx
(13 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (9)
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx (1)
25-25
: LGTM: Currency formatting configuration
The addition of maximumFractionDigits: 0
correctly implements the requirement to remove decimal places.
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-2.tsx (1)
21-25
: LGTM: Consistent currency formatting across all displays
The addition of maximumFractionDigits: 0
is consistently applied to all currency amounts, ensuring uniform display without decimal places throughout the section.
Also applies to: 37-37, 52-52, 63-63, 78-78, 93-93, 102-102
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3.tsx (1)
37-40
: LGTM: Currency formatting in country cards
The addition of maximumFractionDigits: 0
maintains consistent currency formatting across country cards.
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-4.tsx (6)
32-32
: LGTM: Expenses formatting aligns with requirements
The addition of maximumFractionDigits: 0
correctly implements the decimal place removal requirement.
Line range hint 45-68
: LGTM: Payment amounts consistently formatted
All payment-related amounts (total payments, last month's payments, and future payments) now consistently use maximumFractionDigits: 0
.
83-83
: LGTM: Project costs and fees formatting standardized
All financial metrics (project costs, donation fees, delivery fees, exchange rate loss, account fees) consistently implement the decimal place removal.
Also applies to: 93-93, 110-110, 127-127, 144-144
215-219
: LGTM: Reserves formatting matches specification
The reserves amount formatting correctly implements the decimal removal requirement while maintaining the calculation logic.
Line range hint 1-255
: Verify consistency across other sections
Let's ensure this decimal place removal is consistently implemented across all other sections of the transparency page.
✅ Verification successful
All sections consistently use maximumFractionDigits: 0
The search results confirm that all sections (1-4) in the transparency/finances pages consistently implement maximumFractionDigits: 0
for currency formatting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other currency formatting implementations
rg "maximumFractionDigits" --type ts
Length of output: 2755
159-163
: Verify staff costs calculation
While the formatting is correct, let's ensure the staff costs calculation is accurate since it affects transparency reporting.
✅ Verification successful
Let me search for the calculation logic to verify this properly.
Staff costs calculation is correctly implemented
The calculation is accurate and well-structured:
- Expenses are stored in CHF and properly converted using exchange rates
- Staff costs are aggregated using lodash's sumBy function
- The type system ensures proper categorization via ExpenseType enum
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for staff costs calculation logic
rg -A 5 "staff.*costs" --type ts
Length of output: 1603
Script:
#!/bin/bash
# Search for the expenses stats type definition and calculation
rg -l "totalExpensesByType" --type ts
# Then cat the relevant files to see the calculation logic
ast-grep --pattern 'interface $_ {
totalExpensesByType: {
$$$
}
}'
Length of output: 342
Script:
#!/bin/bash
# Let's examine the expenses calculator implementation
cat shared/src/utils/stats/ExpensesStatsCalculator.ts
Length of output: 2183
Script:
#!/bin/bash
# Let's check the expense type definition
cat shared/src/types/expense.ts
Length of output: 443
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@dnhn Great. Getting rid of the decimal places is working. The Check amount to avoid NaN seems not yet to catch the NaN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thanks for picking this up :)
Fixes #728
i18next
currency options inherits number options fromIntl.NumberFormat
https://www.i18next.com/translation-function/formatting#currency
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat
I have also fixed the rounding function to avoid
NaN
if the value does not exist, we can see it happens on the staging site.https://staging.socialincome.org/en/int/transparency/finances/usd
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation