Website: User location based flag #986
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces geolocation-based flag functionality across the Social Income website. The changes centralize flag image URL generation, add a geolocation API route, create a geolocation hook, and modify the navbar to display the user's country flag dynamically. The implementation decouples flag display from currency and aims to enhance user experience by showing location-specific flags. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
dnhn
force-pushed
the
nhan/user-location-flag
branch
from
e52ee55 to
89ce6f0
Compare
dnhn
requested review from
andrashee,
mkue,
ssandino and
socialincome-dev
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ui/src/lib/utils.ts (1)
8-10: Consider caching flag SVGs locallyThe current implementation relies on an external GitHub repository. Consider:
- Caching flags locally
- Using a CDN
- Implementing error handling for failed requests
website/src/app/[lang]/[region]/(website)/me/hooks.ts (1)
175-191: Consider adding error handling and retry logicThe geolocation hook could benefit from:
- Retry logic for failed requests
- Type validation for API response
- Periodic refresh strategy instead of infinite staleTime
export const useGeolocation = () => { const api = useApi(); const { data: geolocation, isLoading, error, } = useQuery({ queryKey: ['geolocation'], queryFn: async () => { const response = await api.get('/api/geolocation'); - return (await response.json()) as Geo; + const data = await response.json(); + if (!data.city || !data.country) { + throw new Error('Invalid geolocation response'); + } + return data as Geo; }, - staleTime: Infinity, + staleTime: 1000 * 60 * 60, // 1 hour + retry: 3, + retryDelay: 1000, }); return { geolocation, isLoading, error }; };website/src/components/navbar/navbar-client.tsx (1)
352-362: Extract flag component to reduce duplicationThe flag implementation is duplicated between mobile and desktop navigation. Consider extracting it to a reusable component.
type FlagProps = { country: string; size: number; className?: string; }; const CountryFlag = ({ country, size, className }: FlagProps) => ( <Image src={getFlagImageURL(country)} width={size} height={size} alt={`${country} flag`} priority className={classNames('rounded-full', className)} /> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
ui/src/lib/utils.ts(1 hunks)website/package.json(1 hunks)website/src/app/[lang]/[region]/(website)/me/hooks.ts(2 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3-cards.tsx(1 hunks)website/src/app/[lang]/[region]/index.ts(1 hunks)website/src/app/api/geolocation/route.ts(1 hunks)website/src/components/navbar/navbar-client.tsx(7 hunks)
🔇 Additional comments (7)
website/src/app/[lang]/[region]/index.ts (1)
9-9: Verify the optional country parameter usage
The country parameter is marked optional, which is correct. However, let's verify all components handle the undefined case properly.
✅ Verification successful
Optional country parameter is safely handled
The codebase shows proper handling of the optional country parameter:
- In
bestGuessCurrency, undefined country is handled with a fallback - Forms and components properly handle the optional nature through TypeScript types
- No direct access to the parameter without null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of country parameter to ensure proper null checks
rg -t typescript "params\.country"
Length of output: 71
Script:
#!/bin/bash
# Search for usage of country parameter with proper file extension
rg "country.*?:" -A 2 -g "*.ts" -g "*.tsx"
# Search for usage of country in function parameters or object destructuring
ast-grep --pattern 'country'
Length of output: 27346
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3-cards.tsx (1)
4-4: LGTM: Good refactoring to centralize flag URL generation
Moving the getFlagImageURL function to a shared utility module improves code reuse and maintainability.
website/package.json (1)
31-31: Verify @vercel/functions version compatibility
The caret (^) allows minor version updates. Let's verify the latest stable version and any potential breaking changes.
✅ Verification successful
Version ^1.5.2 is up-to-date with the latest release
The specified version 1.5.2 matches the latest available release from December 2024, and the version range (^1.5.2) is appropriate for receiving minor updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and release notes
npm view @vercel/functions versions --json | jq -r '.[-1]'
npm view @vercel/functions time --json | jq -r 'to_entries | .[] | select(.key | contains("1.5.")) | "\(.key): \(.value)"'
Length of output: 287
website/src/app/[lang]/[region]/(website)/me/hooks.ts (1)
184-185: Verify API endpoint implementation
Ensure the /api/geolocation endpoint properly handles the Vercel geolocation data.
✅ Verification successful
API endpoint implementation is correct
The /api/geolocation endpoint correctly uses Vercel's geolocation function and includes proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if geolocation API route exists and uses Vercel's geolocation
fd --type f "route.ts" "website/src/app/api/geolocation" --exec rg -l "geolocation.*@vercel/functions" {}
Length of output: 146
Script:
#!/bin/bash
# Check the implementation of the geolocation route
cat website/src/app/api/geolocation/route.ts
Length of output: 305
website/src/components/navbar/navbar-client.tsx (3)
4-4: LGTM: Required imports added for geolocation functionality
The new imports support the geolocation and flag image display features.
Also applies to: 14-14, 17-17
421-421: LGTM: Clean geolocation integration
The geolocation hook is properly integrated and its data is correctly passed to both navigation components.
Also applies to: 425-426
421-426: 🛠️ Refactor suggestion
Add error and loading states for geolocation
Consider handling geolocation errors and loading states to improve user experience.
// Example implementation
const { geolocation, error, isLoading } = useGeolocation();
// Handle loading state
{isLoading && <LoadingSpinner />}
// Handle error state
{error && <FallbackFlag />}
// Show flag when data is available
{geolocation?.country && (
<CountryFlag country={geolocation.country} />
)}
|
Great job @dnhn. I am testing three different scenarios: 1. URL without language or location (socialincome.org)Test URL: https://public-git-nhan-user-location-flag-social-income.vercel.app on TestedLocally Result: ✅ shows location specific flags 
2. URL with language and "int" location (socialincome.org/de/int)Test URL: https://public-git-nhan-user-location-flag-social-income.vercel.app/de/int on TestedLocally Result: ✅ shows location specific flags 
3. URL with language and location, for which we have a specific website setup (only Switzerland for now = ch) (socialincome.org/de/ch)Test URL: https://public-git-nhan-user-location-flag-social-income.vercel.app/de/ch on TestedLocally Test result ❌ The international flags are displayed, but the Swiss flag should be shown in this case, as the page is routed to a specific country page which we serve (in this case /ch/). For all other cases see Nr. 2 above, which is fine. 
PS. Not sure how good this TestedLocally site is for testing this. But didn't have a VPN installed to test otherwise. |
@ssandino the currency flag is displayed only if the region is different from international, isn’t it?
This looks good enough to me 👍🏼 |
|
So the logic for the location flag should be as follows:
Currently, we only have a Swiss-specific site, and the “int” version serves as a catch-all for other regions. However, in the future, we will have additional country-specific pages that should function similarly to the “ch” case. |
Does this mean that the currency flag is always Swiss even if we select other currencies? 
|
|
Yes, the currency field is no longer related to the flag. Only the location determines the flag. (Sorry if there was any confusion.) |
| @@ -211,7 +221,17 @@ const MobileNavigation = ({ lang, region, languages, regions, currencies, naviga | |||
| {translations.myProfile} | |||
| </NavbarLink> | |||
| <div className="flex-inline flex items-center"> | |||
| {Flag && <Flag className="mx-3 h-6 w-6 rounded-full" />} | |||
| {country && ( | |||
| <Image | |||
There was a problem hiding this comment.
Should we also do it like this in website/src/components/ui/currency-selector.tsx? Then we could uninstall the package. Wdyt?
There was a problem hiding this comment.
The library we are using https://raw.githubusercontent.com/lipis/flag-icons/a87d8b256743c9b0df05f20de2c76a7975119045/flags/… only accepts country code, do you have any library suggestions for currency flags?
I would work on this in another PR since this involves with other components.
There was a problem hiding this comment.
Do you mean currency icons? Otherwise, we would have to create some mapping to map currencies to countries (similiar to the countryToCurrency mapping in currency.ts).
FYI – I created an MR (#992) to use a middleware to set the country code. Let me know what you think :)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/components/navbar/navbar-client.tsx(7 hunks)
🔇 Additional comments (3)
website/src/components/navbar/navbar-client.tsx (3)
354-364: 🛠️ Refactor suggestion
Apply the same improvements to desktop flag image
The same improvements should be applied here for consistency.
-{region && country && (
+{country && (
<Image
src={getFlagImageURL(isIntRegion ? country : region)}
width={20}
height={20}
- alt=""
+ alt={`${isIntRegion ? country : region} flag`}
priority
- unoptimized
className="m-auto mx-2 rounded-full"
/>
)}Likely invalid or redundant comment.
225-235: 🛠️ Refactor suggestion
Improve flag image implementation
- Add descriptive alt text for accessibility
- Remove unnecessary
unoptimizedprop - Simplify conditional logic
-{region && country && (
+{country && (
<Image
src={getFlagImageURL(isIntRegion ? country : region)}
width={24}
height={24}
- alt=""
+ alt={`${isIntRegion ? country : region} flag`}
priority
- unoptimized
className="mx-3 rounded-full"
/>
)}Likely invalid or redundant comment.
423-428: Verify geolocation hook behavior
Let's verify the geolocation hook implementation and error handling.
✅ Verification successful
Geolocation implementation is properly handled with error states
The geolocation hook implementation is well-structured with proper error handling:
- Uses React Query for data fetching and caching
- Includes loading and error states
- Has infinite stale time to prevent unnecessary refetches
- Properly handles API errors through the error boundary
The optional chaining in geolocation?.country safely handles null/undefined cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check geolocation hook implementation and error handling
# Search for useGeolocation hook implementation
ast-grep --pattern 'export function useGeolocation() {
$$$
}'
# Search for error handling in geolocation usage
rg -A 5 'useGeolocation'
Length of output: 1789
Script:
#!/bin/bash
# Let's check the API hook implementation and error handling
ast-grep --pattern 'const useApi = () => {
$$$
}'
# Check where geolocation data is used to understand error impact
rg -B2 -A2 'geolocation\?'
# Check the Navigation components to see how they handle the country prop
ast-grep --pattern 'function DesktopNavigation($_) {
$$$
}'
ast-grep --pattern 'function MobileNavigation($_) {
$$$
}'
Length of output: 1147
Script:
#!/bin/bash
# Let's check the complete geolocation hook implementation
cat website/src/app/[lang]/[region]/\(website\)/me/hooks.ts
# Check if there are any error boundaries or error handling components
rg -A 3 'ErrorBoundary|onError|catch'
Length of output: 25937
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/src/components/navbar/navbar-client.tsx(6 hunks)website/src/hooks/queries.ts(1 hunks)
🔇 Additional comments (2)
website/src/components/navbar/navbar-client.tsx (2)
230-233: Add alt text and remove unoptimized prop
Also applies to: 359-362
423-428: LGTM: Clean integration of geolocation
The geolocation hook is properly integrated into the NavbarClient component.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/src/components/navbar/navbar-client.tsx (1)
423-428: Add error handling for geolocation.Consider handling cases where geolocation is unavailable or fails.
- const { geolocation } = useGeolocation(); + const { geolocation, error } = useGeolocation(); + const country = error ? undefined : geolocation?.country; return ( <nav className={twMerge('theme-blue group/navbar fixed inset-x-0 top-0 z-20 flex flex-col', backgroundColor)}> - <DesktopNavigation {...props} country={geolocation?.country} /> - <MobileNavigation {...props} country={geolocation?.country} /> + <DesktopNavigation {...props} country={country} /> + <MobileNavigation {...props} country={country} /> </nav> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/components/navbar/navbar-client.tsx(6 hunks)
🔇 Additional comments (1)
website/src/components/navbar/navbar-client.tsx (1)
225-235: Add alt text and optimize flag image.
dnhn
force-pushed
the
nhan/user-location-flag
branch
from
351a7bb to
e0b74be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/src/components/navbar/navbar-client.tsx (1)
354-364: Inconsistent image sizes between mobile and desktop.The flag image sizes differ between mobile (24x24) and desktop (20x20) implementations. Consider standardizing the size for consistency.
- width={20} - height={20} + width={24} + height={24}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/src/app/[lang]/[region]/index.ts(1 hunks)website/src/components/navbar/navbar-client.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/app/[lang]/[region]/index.ts
🔇 Additional comments (3)
website/src/components/navbar/navbar-client.tsx (3)
10-10: LGTM: Import statements are correctly placed and necessary.
The new imports for geolocation functionality and flag URL generation are properly organized.
Also applies to: 14-14
225-235: Previous review comments still apply.
The implementation needs improvements for accessibility and optimization as noted in previous reviews.
423-423: Verify geolocation fallback behavior.
Ensure the UI gracefully handles cases where geolocation data is unavailable or loading.
Also applies to: 427-428
✅ Verification successful
Geolocation handling is properly implemented with fallbacks
The code shows appropriate handling of geolocation:
- The
useGeolocationhook returnsisLoadinganderrorstates along with the data - The navbar gracefully handles undefined geolocation by using optional chaining (
geolocation?.country) - The UI works with or without the country flag being present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for geolocation error handling
rg -A 5 "useGeolocation" . --type ts --type tsx
Length of output: 75
Script:
#!/bin/bash
# Search for geolocation related code with proper file extensions
rg -A 5 "useGeolocation" --type typescript --type tsx
# Search for any geolocation related hooks or components
rg -A 10 "geolocation" --type typescript --type tsx
Length of output: 175
Script:
#!/bin/bash
# Search for geolocation related code
rg -A 5 "useGeolocation"
# Search for geolocation error handling patterns
rg -A 10 "geolocation.*error|error.*geolocation"
# Find files containing geolocation references
fd . -e ts -e tsx -x rg -l "geolocation"
Length of output: 1883
Script:
#!/bin/bash
# Check the implementation of useGeolocation hook
cat website/src/hooks/queries.ts
# Check how geolocation is used in the navbar component
cat website/src/components/navbar/navbar-client.tsx
Length of output: 14598
* Set cookie with location of request country * fix typing issue * Prioritize region if not set to 'int' * Fix image positioning
|
Visit the preview URL for this PR (updated for commit fac497e): https://si-admin-staging--pr986-nhan-user-location-f-vctj4689.web.app (expires Sat, 04 Jan 2025 16:03:11 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
website/src/middleware.ts (1)
17-30: Consider fallback logic for missing geo country data.
Ifrequest.geo?.countryis undefined (e.g., during local development), it might lead to no cookie being set. For an improved user experience, consider defaulting to a known region.+ const fallbackCountry = 'CH'; // For example, default to Switzerland if (request.geo?.country) { response.cookies.set({ name: COUNTRY_COOKIE, value: ... }); } else { + response.cookies.set({ + name: COUNTRY_COOKIE, + value: fallbackCountry as CountryCode, + ... + }); }website/src/components/providers/context-providers.tsx (1)
200-201: Standardized 7-day expiry.
Aligning all cookies to a shorter duration benefits user privacy and keeps data updated.Also applies to: 203-203, 205-205, 207-207
website/src/components/navbar/navbar-client.tsx (1)
336-346: Improve accessibility and handle Swiss flag edge case.
- Add descriptive alt text
- Handle Swiss flag edge case
- Remove unoptimized prop unless required
-{(!isIntRegion || (isIntRegion && country)) && ( - <Image - className="m-auto mx-2 rounded-full" - src={getFlagImageURL(isIntRegion ? country! : region)} - width={24} - height={24} - alt="Country flag" - priority - unoptimized - /> -)} +{(!isIntRegion || (isIntRegion && country)) && ( + <FlagImage + country={country!} + region={region} + className="m-auto mx-2" + /> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
shared/src/types/country.ts(1 hunks)shared/src/utils/stats/ContributionStatsCalculator.ts(2 hunks)ui/src/lib/utils.ts(1 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3-cards.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3.tsx(2 hunks)website/src/app/[lang]/[region]/index.ts(1 hunks)website/src/components/navbar/navbar-client.tsx(5 hunks)website/src/components/providers/context-providers.tsx(4 hunks)website/src/middleware.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/app/[lang]/[region]/index.ts
🔇 Additional comments (15)
website/src/middleware.ts (6)
1-3: Imports look consistent with usage.
These imports are necessary for handling cookies and validating country codes, aligning well with the rest of the file.
14-16: Doc comment is purposeful.
Nicely clarifies the intent of setting country cookies based on headers.
32-34: Doc comment remains clear.
Explicit statement about checking currency cookies is helpful.
Line range hint 78-106: Internalizing i18nRedirectMiddleware.
Scoping it as a const is consistent with usage in this file. No issues noted.
107-109: Execution order of middlewares.
Country middleware runs before currency middleware. Confirm that this sequence matches business rules.
Line range hint 46-77: Confirm in-file usage of non-exported function.
Renaming redirectMiddleware to a private const might break external references if used elsewhere. Verify that no external code relies on its export.
✅ Verification successful
Function redirectMiddleware is only used internally within middleware.ts
The function is only referenced within the same file and is not exported, making it safe to keep as a private const.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search references to redirectMiddleware outside this file
rg -A 3 'redirectMiddleware'
Length of output: 618
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3.tsx (1)
Line range hint 2-14: Enhanced type safety for country field.
Changing country from string to CountryCode ensures correctness and reduces the risk of invalid values.
shared/src/types/country.ts (1)
253-254: Validation helper is well-defined.
isValidCountryCode improves reliability by guarding against invalid inputs.
shared/src/utils/stats/ContributionStatsCalculator.ts (1)
Line range hint 5-34: Consistent usage of CountryCode.
This aligns with the broader changes and ensures homogeneous handling of locale data.
website/src/components/providers/context-providers.tsx (3)
Line range hint 3-13: Imports synergize with cookie-based country management.
No immediate issues.
144-145: Context interface increment.
Inclusion of country and setCountry is coherent with the app's state management strategy.
195-195: Cookie state for country.
Initialization via useCookieState is consistent with other cookies for language and currency.
ui/src/lib/utils.ts (2)
1-2: LGTM!
The imports are correctly typed and organized.
10-14:
Add input validation and caching for flag images.
The current implementation has potential issues:
- No validation of country input
- No fallback for missing flags
- Direct dependency on external GitHub repository
- No caching strategy
Consider this implementation:
-export const getFlagImageURL = (country: CountryCode | Exclude<WebsiteRegion, 'int'>) =>
- `https://raw.githubusercontent.com/lipis/flag-icons/a87d8b256743c9b0df05f20de2c76a7975119045/flags/1x1/${country.toLowerCase()}.svg`;
+export const getFlagImageURL = (country: CountryCode | Exclude<WebsiteRegion, 'int'>) => {
+ if (!country) return '/default-flag.svg';
+
+ const sanitizedCountry = country.toLowerCase().trim();
+ // Consider hosting flags locally or using a CDN
+ return `https://raw.githubusercontent.com/lipis/flag-icons/a87d8b256743c9b0df05f20de2c76a7975119045/flags/1x1/${sanitizedCountry}.svg`;
+};Likely invalid or redundant comment.
website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-3-cards.tsx (1)
3-5: LGTM!
The changes improve type safety by using CountryCode and centralize flag URL generation.
Also applies to: 9-9
Using
geolocationfrom@vercel/functionsto get information based on user request.https://vercel.com/docs/functions/vercel-functions-package#geolocation
Resolve #978
Summary by CodeRabbit
New Features
Improvements
DefaultParamsinterface to include an optionalcountryproperty.Dependency Updates
Bug Fixes