refactor: useDicidingCount.ts #1622
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/hooks/useDecidingCount.ts (1)
35-35: Remove debugging console.log statementThe
console.log('counts:', counts);statement appears to be for debugging purposes. Consider removing it to prevent unnecessary console output in the production environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/hooks/useDecidingCount.ts(1 hunks)
🔇 Additional comments (3)
packages/extension-polkagate/src/hooks/useDecidingCount.ts (3)
32-33: Verify 'name' handling in track mappings
In the useMemo hooks for trackIds and fellowshipTrackIds, you're using name.toString(). Please ensure that name is always defined and that calling toString() is necessary. If name is already a string, this may be redundant. Additionally, if name could be undefined or null, this could lead to runtime errors.
38-42: Ensure consistent data types when comparing 'genesisHash'
When comparing chain?.genesisHash with api?.genesisHash?.toString(), verify that both are of the same type. If chain.genesisHash is already a string, calling toString() on api.genesisHash may be unnecessary. This will help avoid unexpected behavior due to type mismatches.
122-128: Review conditional logic for fellowship counts aggregation
The else if condition at line 122 checks if fellowshipDecidingCounts.length is truthy. However, it's unclear when this block would execute if fellowshipTrackIds and fellowshipTracks are falsy. Please review this logic to ensure all scenarios are correctly handled, and consider adding comments for clarity.
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
packages/extension-polkagate/src/fullscreen/governance/Bread.tsx (1)
Line range hint
18-22: Consider reviewing the type consistency.There appears to be an inconsistency between the type definition and usage:
setSelectedSubMenurequires a string (non-optional)subMenuis also required (non-optional)- However, the render logic uses
subMenu || 'All'suggestingsubMenucould be falsyConsider either:
- Making
subMenuoptional in the Props interface to match the runtime behavior, or- Removing the fallback to 'All' since the type system guarantees
subMenuwill be presentpackages/extension-polkagate/src/fullscreen/governance/TrackStats.tsx (1)
Line range hint
108-110: Consider extracting and improving the remaining slots calculation.The remaining slots calculation involves multiple optional chains and type casts. Consider extracting it into a helper function for better readability and type safety.
Here's a suggested refactor:
+ const calculateRemainingSlots = ( + decidingCounts: DecidingCount | undefined, + track: Track | undefined, + selectedSubMenu: string + ): string | undefined => { + const maxDeciding = track?.[1]?.maxDeciding; + if (!decidingCounts || !maxDeciding) return undefined; + + const max = Number(maxDeciding); + const current = findItemDecidingCount(selectedSubMenu, decidingCounts); + return `${max - current} out of ${max}`; + }; <LabelValue label={t('Remaining Slots')} - value={decidingCounts && track?.[1]?.maxDeciding && `${track[1].maxDeciding as unknown as number - findItemDecidingCount(selectedSubMenu, decidingCounts)} out of ${track?.[1]?.maxDeciding as unknown as number}`} + value={calculateRemainingSlots(decidingCounts, track, selectedSubMenu)} />packages/extension-polkagate/src/fullscreen/governance/Toolbar.tsx (2)
Line range hint
44-54: Consider cleanup of timeouts to prevent race conditions.The timeout-based hover behavior could lead to race conditions if multiple rapid hover events occur. Consider using
useEffectcleanup to ensure proper timeout handling:const onTopMenuMenuMouseEnter = useCallback((item: TopMenu) => { - ref.current.timeoutId && clearTimeout(ref.current.timeoutId); + if (ref.current.timeoutId) { + clearTimeout(ref.current.timeoutId); + ref.current.timeoutId = null; + } ref.current.timeoutId = setTimeout(() => { setHoveredTopMenu(item.toLowerCase() as 'referenda' | 'fellowship'); setMenuOpen(true); }, MENU_DELAY) as unknown as number; }, [setMenuOpen]);
Line range hint
147-152: Add fallback handling for undefined decidingCounts.The component passes decidingCounts directly to child components without handling the undefined case. Consider adding default values or conditional rendering to handle loading states.
{menuOpen && hoveredTopMenu === 'referenda' && - <ReferendaMenu address={address} decidingCounts={decidingCounts?.referenda} setMenuOpen={setMenuOpen} setSelectedSubMenu={setSelectedSubMenu} /> + <ReferendaMenu + address={address} + decidingCounts={decidingCounts?.referenda ?? { ongoing: 0, total: 0 }} + setMenuOpen={setMenuOpen} + setSelectedSubMenu={setSelectedSubMenu} + /> } {menuOpen && hoveredTopMenu === 'fellowship' && - <FellowshipMenu address={address} decidingCounts={decidingCounts?.fellowship} setMenuOpen={setMenuOpen} setSelectedSubMenu={setSelectedSubMenu} /> + <FellowshipMenu + address={address} + decidingCounts={decidingCounts?.fellowship ?? { ongoing: 0, total: 0 }} + setMenuOpen={setMenuOpen} + setSelectedSubMenu={setSelectedSubMenu} + /> }packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx (3)
Line range hint
7-7: Consider adding null checks for decidingCountsThe decidingCounts prop is marked as optional, but there's no explicit handling of undefined cases in the component. Consider adding null checks to improve robustness.
- decidingCounts: Count[] | undefined; + decidingCounts?: Count[]; const decidingCount = findItemDecidingCount(item, decidingCounts); + const displayCount = decidingCount ?? 0;Also applies to: 24-25
Line range hint
13-13: Consider extracting findItemDecidingCount to a shared utilityThe findItemDecidingCount helper is imported from ReferendaMenu, which suggests it might be better placed in a shared utility file to avoid potential circular dependencies and improve code organization.
Consider moving this helper to a shared location like
utils/governance/helpers.tsfor better maintainability and reusability.Also applies to: 41-41
Line range hint
32-83: Consider extracting MenuItem to a separate componentThe MenuItem component has significant complexity with multiple responsibilities (styling, interaction, display). Consider extracting it to a separate file to improve maintainability and reusability.
Benefits:
- Improved code organization
- Easier testing
- Better separation of concerns
Consider moving MenuItem to
components/governance/MenuItem.tsxand importing it here. This would also make it easier to share this component with other similar menus if needed.packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx (3)
Line range hint
23-32: Consider memoizing findItemDecidingCount for performanceThe function is called for each MenuItem render. Consider memoizing it using useMemo or moving it inside the MenuItem component and memoizing there.
-export const findItemDecidingCount = (item: string, decidingCounts: Count[] | undefined): number | undefined => { +export const findItemDecidingCount = React.useMemo(() => (item: string, decidingCounts: Count[] | undefined): number | undefined => { if (!decidingCounts) { return undefined; } const itemKey = item.toLowerCase().replaceAll(' ', '_'); const filtered = decidingCounts.find(([key]) => key === itemKey); return filtered?.[1]; -}; +}, []);
34-92: Consider restructuring MenuItem for better maintainabilityWhile the component is well-structured, there are a few suggestions for improvement:
- The click handler should be on the Grid container instead of Typography for better accessibility and UX
- Consider extracting styles to a separate constant or using styled-components for better maintainability
- <Grid alignItems='center' container item + <Grid + alignItems='center' + container + item + onClick={clickable ? onSubMenuClick : undefined} sx={{ '&:hover': clickable ? { fontWeight: 700, textDecoration: 'underline' } : undefined, // ... other styles }} > {icon} - <Typography onClick={onSubMenuClick} sx={{ display: 'inline-block', fontWeight: fontWeight || 'inherit' }}> + <Typography sx={{ display: 'inline-block', fontWeight: fontWeight || 'inherit' }}> {item} </Typography>
94-299: Consider extracting constants and simplifying prop passingThe component structure is well-organized, but could benefit from:
- Extracting grid width values as named constants for better maintainability
- Using a common props object for shared MenuItem properties
// Extract as constants const GRID_WIDTHS = { ALL: '7%', ROOT: '13%', REFERENDUM: '16%', ADMIN: '23%', TREASURY: '38%' } as const; // Common props object const commonMenuItemProps = { address, decidingCounts, setMenuOpen, setSelectedSubMenu };Then use them in the component:
- <MenuItem - address={address} - decidingCounts={decidingCounts} - fontWeight={500} - icon={<All sx={{ fontSize: 20, fontWeight: 500, mr: '10px' }} />} - item='All' - setMenuOpen={setMenuOpen} - setSelectedSubMenu={setSelectedSubMenu} - top - width='7%' - /> + <MenuItem + {...commonMenuItemProps} + fontWeight={500} + icon={<All sx={{ fontSize: 20, fontWeight: 500, mr: '10px' }} />} + item='All' + top + width={GRID_WIDTHS.ALL} + />packages/extension-polkagate/src/fullscreen/governance/index.tsx (2)
Line range hint
80-91: Consider standardizing track name formattingThe current implementation handles track names inconsistently:
- Fellowship tracks don't use underscores while other tracks do
- The code tries both formatted and unformatted versions for comparison
Consider standardizing the track name format across all track types and using a shared utility function:
+ const formatTrackName = (name: string): string => name.toLowerCase().replaceAll(' ', '_'); + - const referendaTrackId = tracks?.find((t) => String(t[1].name) === selectedSubMenu.toLowerCase().replaceAll(' ', '_'))?.[0]?.toNumber(); + const referendaTrackId = tracks?.find((t) => String(t[1].name) === formatTrackName(selectedSubMenu))?.[0]?.toNumber(); return (tracks || []).concat(fellowshipTracks || []).find((t) => - String(t[1].name) === selectedSubMenu.toLowerCase().replaceAll(' ', '_') || - String(t[1].name) === selectedSubMenu.toLowerCase() // fellowship tracks have no underscore! + String(t[1].name) === formatTrackName(selectedSubMenu) );
Line range hint
227-292: Improve error handling in fetchRefThe current error handling in
fetchRefonly handles null responses but doesn't properly handle network failures or other errors. This could leave the UI in an inconsistent state.Consider implementing comprehensive error handling:
async function fetchRef(key: string) { + const handleError = (error: Error) => { + setIsLoadingMore(false); + setIsFetching(false); + console.error('Failed to fetch referenda:', error); + // Consider showing an error message to the user + return null; + }; if (!chainName) { - return; + return handleError(new Error('Chain name is not available')); } try { // ... existing code ... + } catch (error) { + return handleError(error as Error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/extension-polkagate/src/fullscreen/governance/Bread.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/Toolbar.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/TrackStats.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/index.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/governance/tracks/kusama.ts(1 hunks)packages/extension-polkagate/src/fullscreen/governance/tracks/polkadot.ts(1 hunks)packages/extension-polkagate/src/fullscreen/governance/utils/types.tsx(1 hunks)
🔇 Additional comments (8)
packages/extension-polkagate/src/fullscreen/governance/TrackStats.tsx (1)
37-37: LGTM! This fixes the track name matching issue.
The change from replace to replaceAll ensures correct matching of track names containing multiple spaces, fixing the incorrect deciding counts issue mentioned in #1592.
Let's verify the track names that contain multiple spaces:
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient evidence about the track name handling in the codebase.
The change to replaceAll is correct and consistent with the codebase's track name handling
The verification confirms that:
- Track names with spaces are consistently transformed using
replaceAll(' ', '_')across multiple files:TrackStats.tsxReferendaMenu.tsxindex.tsx
- The change aligns with the codebase's pattern of handling track names in both referenda and fellowship tracks
- The transformation is used in key matching operations for deciding counts and track identification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find governance track names with multiple spaces to confirm the fix's impact
# Search for track name definitions in governance files
rg -g '*.{ts,tsx}' -A 1 'name.*:.*\s+\w+\s+\w'
# Search for track name usage in components
rg -g '*.{ts,tsx}' 'selectedSubMenu.*=.*\s+\w+\s+\w'
Length of output: 2590
Script:
#!/bin/bash
# Let's search for track-related data structures and configurations to find track names
# Search for track configurations or definitions
rg -g '*.{ts,tsx}' -B 2 -A 2 'track.*name.*:|tracks.*name.*:|trackName.*=|TRACK_NAMES'
# Search specifically in the governance-related files
fd . -e ts -e tsx --exec grep -l "governance" {} | xargs rg -l "track"
# Look for any constants or enums related to track names
ast-grep --pattern 'const $_ = {
$$$
name: $_,
$$$
}'
Length of output: 4745
Script:
#!/bin/bash
# Let's look specifically at the track names in the context where they are used
# Search for track names in the specific file and its imports
rg -g '*.{ts,tsx}' -B 3 -A 3 'decidingCounts.*\[.*\]' packages/extension-polkagate/src/fullscreen/governance/
# Look for track name transformations
rg -g '*.{ts,tsx}' -B 2 -A 2 'toLowerCase\(\).*replace(All)?\(' packages/extension-polkagate/src/fullscreen/governance/
# Check the track data structure
ast-grep --pattern 'interface $_ {
$$$
trackName: $_
$$$
}'
Length of output: 8578
packages/extension-polkagate/src/fullscreen/governance/utils/types.tsx (1)
10-10: LGTM! Verify usage of the new origin.
The addition of 'wish_for_change' to the Origins type is well-structured and follows the existing pattern.
Let's verify that this new origin is consistently used across the codebase:
✅ Verification successful
The 'wish_for_change' origin is properly integrated and consistent
The verification shows that the new origin is properly integrated in both Polkadot and Kusama track definitions:
- Defined as track ID 2 in both networks
- Has consistent naming and origin mapping (
{ Origins: 'WishForChange' }) - Includes proper descriptive text: "Origin for signaling that the network wishes for some change"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new 'wish_for_change' origin across the codebase
# Search for direct string references
echo "Direct references to 'wish_for_change':"
rg "wish_for_change"
# Search for potential track definitions using this origin
echo "\nTrack definitions potentially using this origin:"
rg -A 5 "origins.*wish_for_change|wish_for_change.*origins"
# Look for any TODO or FIXME comments related to origins
echo "\nPotential TODOs or FIXMEs related to origins:"
rg "TODO|FIXME|XXX" -A 2 -B 2 "origins"
Length of output: 1027
Script:
#!/bin/bash
# Let's check the track definitions in the found files to understand how 'wish_for_change' is used
echo "Content of polkadot.ts track definition:"
cat packages/extension-polkagate/src/fullscreen/governance/tracks/polkadot.ts
echo -e "\nContent of kusama.ts track definition:"
cat packages/extension-polkagate/src/fullscreen/governance/tracks/kusama.ts
# Let's also check if there are any other track definition files
echo -e "\nOther potential track definition files:"
fd "tracks.*\.ts"
Length of output: 13298
packages/extension-polkagate/src/fullscreen/governance/tracks/kusama.ts (1)
129-134: LGTM! Verify integration with related files.
The new 'wish_for_change' track follows the established structure and maintains ID sequence consistency.
Let's verify the integration with related files:
✅ Verification successful
Integration verified - track is properly synchronized
The 'wish_for_change' track is correctly integrated across the codebase:
- Identical track definition exists in both Kusama and Polkadot track files with matching structure
- Origin handling is type-safe through the Track type system
- No UI-specific changes needed as origins are handled generically
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the WishForChange origin is properly defined in types and if there's a corresponding track in polkadot.ts
# Check for WishForChange in types definition
echo "Checking types definition..."
rg -l "WishForChange" "packages/extension-polkagate/src/fullscreen/governance/utils/"
# Check for corresponding track in polkadot.ts
echo "Checking polkadot tracks..."
rg "wish_for_change" "packages/extension-polkagate/src/fullscreen/governance/tracks/polkadot.ts"
# Check for any UI components that might need updates
echo "Checking UI components..."
rg -l "referenda\[.*\]\.origin" "packages/extension-polkagate/src/fullscreen/governance/"
Length of output: 492
Script:
#!/bin/bash
# Let's verify the integration more thoroughly
# 1. Check the structure of the corresponding track in polkadot.ts
echo "=== Checking polkadot track structure ==="
rg -A 4 "wish_for_change" "packages/extension-polkagate/src/fullscreen/governance/tracks/polkadot.ts"
# 2. Check for WishForChange origin in any type definitions or interfaces
echo "=== Checking for WishForChange origin definitions ==="
rg "WishForChange" "packages/extension-polkagate/src/"
# 3. Check how origins are handled in UI components
echo "=== Checking origin handling in UI ==="
rg "\.origin\." "packages/extension-polkagate/src/fullscreen/governance/"
# 4. Check for any track-related type definitions
echo "=== Checking track-related types ==="
rg "type.*Track" "packages/extension-polkagate/src/fullscreen/governance/"
Length of output: 2599
packages/extension-polkagate/src/fullscreen/governance/tracks/polkadot.ts (1)
129-134: LGTM! Verify related changes.
The new "wish_for_change" track is well-structured and follows the established pattern for referenda tracks.
Let's verify the related changes:
✅ Verification successful
Let me try a different approach to verify the Origins type definition.
Changes are consistent and properly typed
The verification confirms:
WishForChangeis properly included in theOriginstype definition intypes.tsx- The track is consistently implemented in both Polkadot and Kusama with identical structure:
- Same track ID (2)
- Same origin (
{ Origins: 'WishForChange' }) - Same descriptive text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Origins type includes WishForChange and if Kusama tracks are consistent
# Check if Origins type includes WishForChange
echo "Checking Origins type definition..."
rg -A 1 "type Origins.*=.*{"
# Check Kusama tracks implementation
echo "Checking Kusama tracks implementation..."
fd -e ts -e tsx kusama.ts$ | xargs rg "wish_for_change"
Length of output: 391
Script:
#!/bin/bash
# Search for Origins type definition and WishForChange
echo "Searching for Origins type and WishForChange..."
rg -A 5 "type\s+Origins?\s*="
echo -e "\nSearching for WishForChange in type definitions..."
rg "WishForChange"
echo -e "\nChecking Kusama tracks implementation..."
fd kusama.ts$ | xargs cat
Length of output: 8329
packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx (1)
19-19: Verify the removal of undefined from setSelectedSubMenu type
The type change enforces stricter type checking by removing undefined as a possible value. While this aligns with similar changes in other components (as mentioned in the AI summary), we should verify that this won't break existing code.
Let's verify all usages of setSelectedSubMenu to ensure they never pass undefined:
✅ Verification successful
Let me check the actual implementation of these components to understand the context better:
Let me check where this component is initialized to understand how setSelectedSubMenu is used:
Based on the code analysis, I can now generate the final response:
Type change for setSelectedSubMenu is safe and correct
The removal of undefined from the type of setSelectedSubMenu is correct because:
- In the parent component (Governance),
selectedSubMenuis initialized with a string default value:state?.selectedSubMenu || 'All' - All calls to
setSelectedSubMenuacross components only pass string values:- In Bread.tsx:
setSelectedSubMenu(subMenu)andsetSelectedSubMenu('All') - In ReferendaMenu.tsx:
setSelectedSubMenu(item) - In FellowshipMenu.tsx:
setSelectedSubMenu(item)
- In Bread.tsx:
The type change enforces the actual usage pattern where undefined is never passed to this state setter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any potential undefined assignments to setSelectedSubMenu
rg -l "setSelectedSubMenu\(.*undefined.*\)"
# Search for direct usage patterns of setSelectedSubMenu
ast-grep --pattern 'setSelectedSubMenu($_)'
Length of output: 522
Script:
#!/bin/bash
# Get the content of files using setSelectedSubMenu
cat packages/extension-polkagate/src/fullscreen/governance/Bread.tsx packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx
# Also check where these components are used to see how they're initialized
rg -l "FellowshipMenu|ReferendaMenu|Bread"
Length of output: 20999
Script:
#!/bin/bash
# Check the parent component that uses FellowshipMenu
cat packages/extension-polkagate/src/fullscreen/governance/index.tsx
Length of output: 18625
packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx (2)
7-7: LGTM: Improved type safety for setSelectedSubMenu
The stricter typing for setSelectedSubMenu ensures better type safety by removing undefined as a possible value.
Also applies to: 19-19
52-52: Verify the deciding count display logic
The implementation looks correct, but let's verify that the deciding counts are being displayed properly for all governance tracks.
Also applies to: 88-89
✅ Verification successful
Let me gather more information about the deciding count implementation to verify its consistency.
Let me check the ReferendaMenu implementation since it contains the findItemDecidingCount function that both menus use.
After analyzing the implementation, I can now provide the final response:
Deciding count implementation is correctly integrated
The implementation is verified to be working correctly because:
- The
useDecidingCounthook properly fetches and processes counts for both referenda and fellowship tracks - The
findItemDecidingCountfunction correctly normalizes item names to match the API's format - Both ReferendaMenu and FellowshipMenu consistently use the same counting logic
- The display format is uniform across all governance tracks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of deciding count display across the codebase
# Check for any other components using decidingCounts
rg -l "decidingCounts.*Count\[\]"
# Check for any test files related to deciding counts
fd -g "*test*" | rg -l "decidingCount"
# Check for any similar menu components that might need the same updates
fd -g "*Menu.tsx" | rg -v "ReferendaMenu"
Length of output: 976
Script:
#!/bin/bash
# Check the implementation of useDecidingCount hook
cat packages/extension-polkagate/src/hooks/useDecidingCount.ts
# Check FellowshipMenu implementation for comparison
cat packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx
# Check for the findItemDecidingCount function implementation
rg -p "function findItemDecidingCount" -A 10
Length of output: 10562
Script:
#!/bin/bash
# Check ReferendaMenu implementation
cat packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx
# Look for any tests related to findItemDecidingCount
rg -p "findItemDecidingCount.*test"
Length of output: 10418
packages/extension-polkagate/src/fullscreen/governance/index.tsx (1)
Line range hint 321-364: LGTM: Well-implemented infinite scrolling
The intersection observer implementation is clean and properly handles:
- Observer cleanup
- Loading state checks
- Proper threshold configuration
- Early returns for edge cases
|
|
||
| fetchRef(_key).then(() => setIsFetching(false)).catch(console.error); | ||
| setReferenda(refsContext.refs?.[_key]); |
There was a problem hiding this comment.
Potential race condition in state updates
The immediate state update after the async operation might lead to a race condition:
fetchRef(_key).then(() => setIsFetching(false)).catch(console.error);
setReferenda(refsContext.refs?.[_key]); // This line executes before fetchRef completesConsider moving the setReferenda call inside the promise chain:
- fetchRef(_key).then(() => setIsFetching(false)).catch(console.error);
- setReferenda(refsContext.refs?.[_key]);
+ fetchRef(_key)
+ .then(() => {
+ setIsFetching(false);
+ setReferenda(refsContext.refs?.[_key]);
+ })
+ .catch(console.error);
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/fullscreen/governance/post/index.tsx (1)
Line range hint
1-224: Consider architectural improvements for better maintainability.The component has grown complex with multiple responsibilities:
- State management for voting, menus, and treasury
- Route handling
- UI rendering with multiple conditional sections
Consider:
- Extracting route handling logic into a custom hook
- Breaking down the component into smaller, focused components
- Adding cleanup functions to useEffect hooks to prevent memory leaks
packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx (2)
48-92: Consider moving onClick handler to the Grid containerWhile the component is well-structured, the click handler is currently attached to the Typography component instead of the entire Grid container. This creates a smaller clickable area than what the UI suggests.
Consider this modification:
- <Grid alignItems='center' container item + <Grid alignItems='center' container item onClick={clickable ? onSubMenuClick : undefined} sx={{ '&:hover': clickable ? { fontWeight: 700, textDecoration: 'underline' } : undefined, // ... other styles }} > {icon} - <Typography onClick={onSubMenuClick} sx={{ display: 'inline-block', fontWeight: fontWeight || 'inherit' }}> + <Typography sx={{ display: 'inline-block', fontWeight: fontWeight || 'inherit' }}> {item} </Typography>
94-298: LGTM: Well-structured menu organizationThe menu items are logically grouped and consistently implement the deciding count functionality, addressing the original issue of incorrect deciding counts display.
Consider extracting common props to reduce repetition
Many ToolbarMenuItem instances share the same props. Consider creating a helper function or default props object.
Example approach:
const commonProps = { address, decidingCounts, setMenuOpen, setSelectedSubMenu, width: '100%' }; // Usage <ToolbarMenuItem {...commonProps} borderWidth='1px' item='Wish For Change' />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/extension-polkagate/src/fullscreen/governance/Bread.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/Toolbar.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/governance/post/index.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/extension-polkagate/src/fullscreen/governance/Bread.tsx
- packages/extension-polkagate/src/fullscreen/governance/Toolbar.tsx
- packages/extension-polkagate/src/fullscreen/governance/topMenu/FellowshipMenu.tsx
🔇 Additional comments (4)
packages/extension-polkagate/src/fullscreen/governance/post/index.tsx (3)
83-83: Ensure path variables are properly sanitized.
The pathname construction uses multiple variables (address and topMenu). While React Router provides some safety, it's good practice to validate these values before using them in paths.
#!/bin/bash
# Description: Check for path validation in the codebase
# Search for path validation patterns
rg 'validatePath|sanitizePath|path\.normalize' -A 2
# Look for similar pathname constructions
ast-grep --pattern 'pathname: `/governance/${$_}/${$_}`'147-147:
Remove @ts-ignore by fixing the underlying type issue.
Using @ts-ignore bypasses TypeScript's type checking, which could hide potential runtime issues. Consider fixing the underlying type mismatch instead.
Let's investigate why this type ignore is needed:
- Check if
topMenufrom useParams needs type refinement - Verify if the Bread component's prop types are correctly defined
#!/bin/bash
# Description: Investigate the type issue
# Look for Bread component's prop types
ast-grep --pattern 'interface $_ {
$$$
topMenu: $_
$$$
}'
# Check other usages of topMenu
rg 'topMenu.*:.*TopMenu' -A 253-53: Verify handling of empty string state in child components.
The state initialization has been changed from allowing undefined to using an empty string. While this improves type safety, ensure that components receiving selectedSubMenu properly handle the empty string case.
✅ Verification successful
Change from undefined to empty string is safe
Based on the codebase analysis, changing the initial state from undefined to empty string is safe because:
- The
selectedSubMenuprop inTrackStatscomponent expects a string type, not string | undefined - The state is immediately updated with actual values from either
state?.selectedSubMenuor 'All' in the parent component - All usages properly handle string values including empty strings through string operations
- No components check for undefined state of
selectedSubMenu
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any components expect undefined for selectedSubMenu
# Search for components that might expect undefined
ast-grep --pattern 'selectedSubMenu?: string | undefined'
# Search for potential undefined checks
rg 'selectedSubMenu(\s+===|\s*==|\s*\!==|\s*\!=)\s*(undefined|null)' -A 2
Length of output: 3494
Script:
#!/bin/bash
# Let's search for usage of selectedSubMenu in components
rg "selectedSubMenu" -B 2 -A 2
# Also check for any props interface that accepts selectedSubMenu
ast-grep --pattern 'interface $_ {
$$$
selectedSubMenu: $_
$$$
}'
# Check where this state is being used
ast-grep --pattern 'setSelectedSubMenu($_)'
Length of output: 18657
packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx (1)
16-21: LGTM: Props interface improvements
The stricter typing for setSelectedSubMenu improves type safety by removing the undefined possibility, which aligns with the component's actual behavior.
packages/extension-polkagate/src/fullscreen/governance/topMenu/ReferendaMenu.tsx
Show resolved
Hide resolved
Close: #1592
Summary by CodeRabbit
New Features
Refactor
useDecidingCounthook for better clarity and maintainability.ReferendaMenucomponent by creating a newToolbarMenuItemcomponent.Bread,Toolbar, andFellowshipMenucomponents to enforce stricter type management for submenu selection.Style Updates
MyVotecomponent.TotalBalancePieChartcomponent.Type Updates
setSelectedSubMenuto enforce stricter state management.