Bug: direct link to plant location does not work (first time) #2350
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
npm warn config production Use WalkthroughThe pull request introduces subtle modifications across four files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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.
Same issue as already remarked in PR #2352 : Loading-time is too long
mariahosfeld
dismissed
their stale review
Currently dismissed as we need to monitor performance on actual system.
src/utils/projectV2.ts
Outdated
| @@ -263,7 +263,7 @@ export const getLocalizedPath = (path: string, locale: string): string => { | |||
| } | |||
|
|
|||
| // If path already starts with locale, return as is | |||
| if (cleanPath.startsWith(`/${locale}/`)) { | |||
| if (cleanPath.startsWith(`/${locale}`)) { | |||
There was a problem hiding this comment.
Couldn't this cause an issue?
What if the cleanPath starts with /france-salesforce where the first 2 letters fr match a locale?
This is currently unlikely to happen, since the number of places internally linking to the project details page are limited, but will be difficult to debug if it occurs (after adding other sources).
There was a problem hiding this comment.
@sunilsabatp There are a few issues with the changes that add a null check for plantLocations before loading data
- Instead of checking
plantLocations = nulland blocking site selection (url update) / map zoom when not needed, introduce a dedicatedisLoadingPlantLocationsstate. This way the purpose is more explicit. - Check the
isLoadingPlantLocationsonly when needed e.g. whenplocis passed as a url query param, or a selectedPlantLocation exists. - The current implementation prevents site loading when the project is not a
treepurpose project. e.g. conservation-aa-andes
You might want to consider separating site selection logic from plant location handling (perhaps not now, but as part of a near future update)
- Add check for single locale path (e.g., '/en') in getLocalizedPath
- Treat single locale paths same as root path, returning '/{locale}'
- Fixes API call errors caused by malformed URLs with duplicate locales
Instead of introducing a new state, I have followed a different approach. Please review it and provide your feedback. |
Fix :
Previously, visiting a
plant location via a direct linkin an incognito tab (e.g., https://dev.pp.eco/en/yucatan?ploc=JOCK9R) redirected to the default site instead of the specified plant location. This fix ensures the correct plant location is resolved and displayed when accessing such links, even in incognito mode.