fix: prevent context color flickering while navigating #1365
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@mbacherycz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 41 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. 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
101-103: Optimize company status determinationWhile the logic addresses the flickering issue, it could be optimized for better performance and readability.
Consider memoizing the company status to prevent unnecessary recalculations:
+ const isCompanyFromParams = useMemo( + () => Boolean(searchParams.get('party') || searchParams.get('allParties')), + [searchParams] + ); + + const isCompany = useMemo( + () => + isCompanyFromParams || + allOrganizationsSelected || + selectedParties?.[0]?.partyType === 'Organization', + [isCompanyFromParams, allOrganizationsSelected, selectedParties] + ); - const isCompanyFromParams = Boolean(party || allParties); - const isCompany = - isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx(1 hunks)
🔇 Additional comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
101-103: Verify the fix prevents color flickering
Let's verify that the changes effectively prevent color flickering during navigation.
✅ Verification successful
The fix successfully prevents color flickering during navigation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other components that might affect the background color during navigation
# Look for style definitions that could cause flickering
rg -g '*.css' -g '*.scss' 'background.*transition|transition.*background'
# Check for other components that might modify the background
ast-grep --pattern 'className={$_background$_}'
Length of output: 328
Script:
#!/bin/bash
# Search for backgroundColor modifications in JavaScript and TypeScript files
rg --glob "*.js" --glob "*.ts" "backgroundColor"
# Search for conditional className assignments related to navigation
rg --glob "*.tsx" "className=.*navigation"
Length of output: 383
Script:
#!/bin/bash
# Search for the definition of the 'navigation' class in CSS and SCSS files
rg --glob "*.css" --glob "*.scss" "\.navigation\b"
Length of output: 124
Script:
#!/bin/bash
# Extract the .navigation class definition and search for background or transition properties
rg --context 5 "\.navigation\s*\{" packages/frontend/src/components/Header/header.module.css | rg "background|transition"
Length of output: 123
| const urlParams = new URLSearchParams(window.location.search); | ||
| const party = urlParams.get('party'); | ||
| const allParties = urlParams.get('allParties'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor URL parameter extraction to use existing hook
The code directly accesses window.location.search while the component already has access to the searchParams from useSearchParams hook. This creates inconsistency and could cause hydration issues.
Replace with:
- const urlParams = new URLSearchParams(window.location.search);
- const party = urlParams.get('party');
- const allParties = urlParams.get('allParties');
+ const party = searchParams.get('party');
+ const allParties = searchParams.get('allParties');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const urlParams = new URLSearchParams(window.location.search); | |
| const party = urlParams.get('party'); | |
| const allParties = urlParams.get('allParties'); | |
| const party = searchParams.get('party'); | |
| const allParties = searchParams.get('allParties'); |
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
881fbb7
to
67b4b22
Compare
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
67b4b22
to
7da2c4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
100-102: Add comments explaining the company status determination logicConsider adding comments to explain why URL parameters are used as an additional source of truth for company status. This will help future maintainers understand the relationship between URL parameters and color flickering prevention.
+ // Use URL parameters as an initial source of truth to prevent color flickering during navigation + // while the selected parties are being loaded const isCompanyFromParams = Boolean(party || allParties); const isCompany = isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx(1 hunks)
🔇 Additional comments (2)
packages/frontend/src/components/PageLayout/PageLayout.tsx (2)
98-99: LGTM! Clean implementation of URL parameter extraction
The code correctly uses the useSearchParams hook to access URL parameters, following React Router best practices.
100-102: Verify the relationship between URL parameters and company status
The implementation looks good and should help prevent color flickering by maintaining consistent styling during navigation. However, let's verify that URL parameters always correctly reflect the company status.
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
7da2c4a
to
56e512c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
99-101: Consider improving readability of the company status determinationWhile the logic is correct, the complex boolean expression could be more descriptive and easier to maintain.
Consider refactoring to make the conditions more explicit:
- const isCompany = - isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization'; + const isCompany = ( + isCompanyFromParams || + allOrganizationsSelected || + selectedParties?.[0]?.partyType === 'Organization' + );Or even better, with named conditions:
const hasCompanyPartyType = selectedParties?.[0]?.partyType === 'Organization'; const isCompany = ( isCompanyFromParams || allOrganizationsSelected || hasCompanyPartyType );packages/frontend/src/api/useParties.ts (1)
Line range hint
135-159: Review implementation strategy for flickering fixWhile adding URL-based company detection is a good approach, the current implementation might contribute to the flickering issue due to:
- Unnecessary function recreation on each render
- Unoptimized value computation in the return object
- Potential rapid state changes during navigation
Consider implementing a more stable state management approach:
- Memoize the company status computation
- Use URL parameters as the source of truth
- Implement proper state transitions during navigation
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/api/useParties.ts(3 hunks)packages/frontend/src/components/PageLayout/PageLayout.tsx(1 hunks)
🔇 Additional comments (4)
packages/frontend/src/components/PageLayout/PageLayout.tsx (2)
92-92: LGTM! Good separation of concerns
Moving the URL parameter logic to the useParties hook improves code organization and helps prevent flickering by maintaining consistent company status.
92-101: Verify CSS transitions for background color changes
To ensure the flickering is fully resolved, we should verify if CSS transitions are properly handled when the company status changes.
packages/frontend/src/api/useParties.ts (2)
21-21: LGTM: Interface addition is clear and well-typed
The new property follows TypeScript best practices and has a descriptive name that clearly indicates its purpose.
135-139: 🛠️ Refactor suggestion
Consider optimizing the company check implementation
The current implementation has several potential improvements:
- The function is recreated on every render
- The logic assumes any party param indicates a company, which might need verification
Consider this optimization:
- const isCompanyFromParams = () => {
- const party = searchParams.get('party');
- const allParties = searchParams.get('allParties');
- return Boolean(party || allParties);
- };
+ const isCompanyFromParams = useMemo(() => {
+ const party = searchParams.get('party');
+ // Only consider it a company if the party param contains 'organization'
+ const isPartyCompany = party && !party.includes('person');
+ const allParties = searchParams.get('allParties');
+ return Boolean(isPartyCompany || allParties);
+ }, [searchParams]);Let's verify the assumption about party parameters:
| @@ -149,5 +156,6 @@ export const useParties = (): UsePartiesOutput => { | |||
| currentEndUser: data?.parties.find((party) => party.isCurrentEndUser), | |||
| deletedParties: data?.deletedParties ?? [], | |||
| allOrganizationsSelected, | |||
| isCompanyFromParams: isCompanyFromParams(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize return value computation
The direct function call in the return statement could contribute to unnecessary re-renders, potentially related to the flickering issue you're trying to fix.
After implementing the previous memoization suggestion, update the return statement:
- isCompanyFromParams: isCompanyFromParams(),
+ isCompanyFromParams,Committable suggestion skipped: line range outside the PR's diff.
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
56e512c
to
7d2b072
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/frontend/src/components/PageLayout/PageLayout.tsx (2)
99-101: Consider memoizing the isCompany calculationWhile the logic effectively prevents color flickering by considering URL parameters first, the calculation could be memoized to optimize performance, especially since it affects styling and renders.
Consider applying this optimization:
- const isCompany = - isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization'; + const isCompany = useMemo( + () => + isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization', + [isCompanyFromParams, allOrganizationsSelected, selectedParties] + );
99-101: Document the URL parameter-based styling logicThe relationship between URL parameters and styling behavior should be documented to help future maintainers understand this flickering prevention mechanism.
Add a comment explaining the logic:
+ // Determine company status from URL parameters first to prevent style flickering during navigation, + // falling back to selected organization status if parameters are not present const isCompany = isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/api/useParties.ts(4 hunks)packages/frontend/src/components/PageLayout/PageLayout.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/api/useParties.ts
🔇 Additional comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
92-92: LGTM: Clean hook usage
The addition of isCompanyFromParams to the destructured values from useParties is well-structured and follows React conventions.
| const [searchParams] = useSearchParams(); | ||
|
|
||
| useProfile(); | ||
|
|
||
| const name = allOrganizationsSelected ? t('parties.labels.all_organizations') : selectedParties?.[0]?.name || ''; | ||
| const isCompany = allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization'; | ||
|
|
||
| const isCompany = |
There was a problem hiding this comment.
Maybe useParties should calculate and expose currentProfile instead? Either company or person.
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
7d2b072
to
d0bf077
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/api/useParties.ts (1)
136-140: Consider a more descriptive name for the memoized value.While the implementation is correct, the name
isCompanyFromParamscould be more specific about its purpose, such ashasCompanyRelatedQueryParamsorisCompanyContextInUrl.- const isCompanyFromParams = useMemo(() => { + const hasCompanyRelatedQueryParams = useMemo(() => { const party = searchParams.get('party'); const allParties = searchParams.get('allParties'); return Boolean(party || allParties); }, [searchParams]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/api/useParties.ts(4 hunks)packages/frontend/src/components/PageLayout/PageLayout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/components/PageLayout/PageLayout.tsx
🔇 Additional comments (3)
packages/frontend/src/api/useParties.ts (3)
21-22: LGTM: Interface extensions are well-typed and descriptive.
The new interface properties isCompanyProfile and selectedProfile are properly typed and follow naming conventions.
164-165: LGTM: Return value changes are consistent.
The new properties are correctly added to the return object and align with the interface definition.
149-151: Verify that all potential sources of flickering are addressed.
While the changes look good, let's verify that we've covered all potential sources of flickering:
Also applies to: 164-165
✅ Verification successful
Flickering issue has been comprehensively addressed
The verification shows that the profile-related state changes are well-contained and properly managed:
- The
isCompanyProfilestate is derived from stable sources (isCompanyFromParams,allOrganizationsSelected,selectedParties) - The
selectedProfileis directly derived fromisCompanyProfilewithout intermediate state - The only component consuming these states is
PageLayout, which receives them through theusePartieshook - Other profile type checks in the codebase are isolated one-time computations (
mapToPartyOption,useDialogs,useDialogById) that don't cause re-renders
The current implementation ensures stable profile state transitions, effectively preventing UI flickering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other potential sources of flickering in related components
# Look for components that might consume these profile states
rg -l "useParties.*isCompanyProfile|useParties.*selectedProfile" --type tsx
# Check for other profile-related state changes that might cause flickering
rg "const.*Profile.*=.*useState|const.*Profile.*=.*useMemo" --type tsx
Length of output: 206
Script:
#!/bin/bash
# Description: Check for other potential sources of flickering in related components
# Look for components that might consume these profile states
rg -l "useParties.*isCompanyProfile|useParties.*selectedProfile"
# Check for other profile-related state changes that might cause flickering
rg "const.*Profile.*=.*useState|const.*Profile.*=.*useMemo"
# Look for any other profile type checks that might cause flickering
rg "partyType.*===.*'Organization'|isCompanyProfile"
# Check for any conditional rendering based on profile types
rg "\\{.*isCompanyProfile.*\\?|\\{.*selectedProfile.*\\?"
Length of output: 1335
| const isCompanyProfile = | ||
| isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization'; | ||
| const selectedProfile = (isCompanyProfile ? 'company' : 'person') as 'company' | 'person'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing derived states to prevent unnecessary re-renders.
The derived states could contribute to flickering if their dependencies change frequently. Consider memoizing them:
- const isCompanyProfile =
- isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization';
- const selectedProfile = (isCompanyProfile ? 'company' : 'person') as 'company' | 'person';
+ const isCompanyProfile = useMemo(
+ () =>
+ isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization',
+ [isCompanyFromParams, allOrganizationsSelected, selectedParties]
+ );
+ const selectedProfile = useMemo(
+ () => (isCompanyProfile ? 'company' : 'person'),
+ [isCompanyProfile]
+ ) as const;The as const assertion provides better type inference than the explicit union type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isCompanyProfile = | |
| isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization'; | |
| const selectedProfile = (isCompanyProfile ? 'company' : 'person') as 'company' | 'person'; | |
| const isCompanyProfile = useMemo( | |
| () => | |
| isCompanyFromParams || allOrganizationsSelected || selectedParties?.[0]?.partyType === 'Organization', | |
| [isCompanyFromParams, allOrganizationsSelected, selectedParties] | |
| ); | |
| const selectedProfile = useMemo( | |
| () => (isCompanyProfile ? 'company' : 'person'), | |
| [isCompanyProfile] | |
| ) as const; |
| @@ -149,5 +161,7 @@ export const useParties = (): UsePartiesOutput => { | |||
| currentEndUser: data?.parties.find((party) => party.isCurrentEndUser), | |||
| deletedParties: data?.deletedParties ?? [], | |||
| allOrganizationsSelected, | |||
| isCompanyProfile, | |||
There was a problem hiding this comment.
drop isCompanyProfile and rather pass selectedProfile === 'company' to Background, or better (?), change prop to Background to be profile instead?
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
c6f6e29
to
186a0b7
Compare
mbacherycz
force-pushed
the
fix/flickering_profile_color_while_navigating
branch
from
186a0b7
to
89f9345
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
92-92: Consider adding type safety for profile values.The string literals 'company' and 'person' are used directly in comparisons. Consider defining an enum or union type for better type safety and maintainability.
type ProfileType = 'company' | 'person'; // In useParties hook selectedProfile: ProfileType; // In PageLayoutContent props profile: ProfileType;Also applies to: 106-106, 109-109
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/api/useParties.ts(4 hunks)packages/frontend/src/components/PageLayout/PageLayout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/api/useParties.ts
🔇 Additional comments (1)
packages/frontend/src/components/PageLayout/PageLayout.tsx (1)
92-92: LGTM! Effective solution to prevent color flickering.
The changes effectively prevent context color flickering by centralizing profile determination in the useParties hook and eliminating local computations. This ensures a single source of truth for the profile type, avoiding race conditions during navigation.
Also applies to: 106-106, 109-109
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit