refactor: use theme color for static stroke color #1627
Conversation
- reducing account name font size in extension home - handle some type issues
WalkthroughThe changes in this pull request primarily involve modifications to various components within the codebase, focusing on enhancing type safety, improving prop handling, and refining visual styling. Notable updates include adjustments to prop types in components like Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Component
User->>App: Interacts with UI
App->>Component: Update props
Component->>Component: Handle props changes
Component->>App: Render updated UI
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)
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: 8
🧹 Outside diff range and nitpick comments (29)
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/TAccountDisplay.tsx (1)
14-14: Consider extracting props interface for better maintainability.While the typing is correct, consider extracting the props interface:
+interface TAccountsDisplayProps { + chain: Chain | null | undefined; + address: string; + api: ApiPromise | undefined; + selectedTrustedAddress: string | undefined; +} + -export default function TAccountsDisplay ({ address, api, chain, selectedTrustedAddress }: { chain: Chain | null | undefined, address: string, api: ApiPromise | undefined, selectedTrustedAddress: string | undefined }): React.ReactElement { +export default function TAccountsDisplay ({ address, api, chain, selectedTrustedAddress }: TAccountsDisplayProps): React.ReactElement {packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx (1)
41-41: LGTM! Consider extracting theme-specific styles.The change to use
secondary.lightfor dark mode border color improves visual consistency with other components. However, the inline styles could be moved to a theme-based styling solution for better maintainability.Consider extracting the Paper styles to a theme-based solution:
const paperStyles = { backgroundImage: 'none', bgcolor: 'background.paper', border: '1px solid', borderColor: { dark: 'secondary.light', light: 'transparent' }, borderRadius: '7px', boxShadow: { dark: '0px 4px 4px rgba(255, 255, 255, 0.25)', light: '0px 0px 25px 0px rgba(0, 0, 0, 0.50)' }, pt: '5px' };This would make it easier to maintain consistent styling across different themes and components.
packages/extension-polkagate/src/components/ChainLogo.tsx (1)
Line range hint
44-67: Consider extracting style constants and using theme colors consistently.The styling implementation could benefit from some refinements:
- The fontSize '15px' is hardcoded
- Border styling could leverage theme colors for consistency
Consider applying these improvements:
<FontAwesomeIcon - fontSize='15px' + fontSize={`${Math.floor(size * 0.6)}px`} icon={fas[convertToCamelCase(_logo)]} style={{ - border: '0.5px solid', + border: `0.5px solid ${theme.palette.divider}`, borderRadius: '50%', filter, height: size, width: size }} />packages/extension-polkagate/src/components/MenuItem.tsx (1)
64-64: Consider extracting complex stylesWhile the theme-based color change is good, the inline styles are quite complex. Consider:
- Extracting styles to a separate const or using styled components
- Simplifying the rotation logic using a helper function or mapping
+ const arrowIconStyles = (theme: Theme, showChevron?: boolean, showSubMenu?: boolean) => ({ + color: 'secondary.light', + fontSize: 18, + m: 'auto', + stroke: theme.palette.secondary.light, + strokeWidth: '2px', + transform: getRotation(showChevron, showSubMenu), + transitionDuration: '0.3s', + transitionProperty: 'transform' + }); + + const getRotation = (showChevron?: boolean, showSubMenu?: boolean) => { + if (showChevron) return 'none'; + return showSubMenu ? 'rotate(-90deg)' : 'rotate(90deg)'; + }; + - <ArrowForwardIosIcon sx={{ color: 'secondary.light', fontSize: 18, m: 'auto', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: showChevron ? 'none' : (showSubMenu ? 'rotate(-90deg)' : 'rotate(90deg)'), transitionDuration: '0.3s', transitionProperty: 'transform' }} /> + <ArrowForwardIosIcon sx={arrowIconStyles(theme, showChevron, showSubMenu)} />packages/extension-polkagate/src/popup/newAccount/deriveAccount/modal/AddressDropdown.tsx (1)
52-52: Consider extracting complex stylesWhile the theme color integration is good, the sx prop contains many style properties that could be extracted into a constant or styled component for better maintainability.
Example refactor:
const arrowIconStyles = { color: 'secondary.light', fontSize: 18, m: 'auto', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: isDropdownVisible ? 'rotate(-90deg)' : 'rotate(90deg)', transitionDuration: '0.3s', transitionProperty: 'transform' }; // Usage: <ArrowForwardIosIcon sx={arrowIconStyles} />packages/extension-polkagate/src/popup/history/modal/HistoryItemModal.tsx (1)
96-96: LGTM: Improved theme consistencyGood improvement replacing hardcoded colors with theme-based styling. The icon now properly responds to theme changes while maintaining its visual properties.
Consider extracting the icon styles to a separate constant or theme override for better reusability:
const iconStyles = { color: 'secondary.light', fontSize: '24px', stroke: theme.palette.secondary.light, strokeWidth: 2 }; // Usage <ArrowForwardIosRoundedIcon sx={iconStyles} />packages/extension-polkagate/src/popup/history/partials/HistoryItem.tsx (2)
97-97: LGTM: Consistent theme-based styling.The icon styling now properly uses the theme's secondary light color for both color and stroke properties, ensuring consistent theming across the application.
Consider extracting commonly used theme-based styles into a shared styles configuration to maintain consistency across components.
Line range hint
26-112: Consider component refactoring for better maintainability.While the current implementation is functional, consider these improvements:
- Extract the transaction details section into a separate component to improve maintainability
- Move magic numbers in styling (e.g.,
fontSize: '24px',strokeWidth: 2) to theme constantspackages/extension-polkagate/src/popup/newAccount/deriveAccount/AddressDropdown.tsx (1)
64-64: Consider breaking down the style properties for better maintainabilityWhile the theme color usage is good, the style properties could be organized better for improved readability and maintenance.
Consider restructuring the styles like this:
- <ArrowForwardIosIcon sx={{ color: 'secondary.light', fontSize: 18, m: 'auto', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: isDropdownVisible ? 'rotate(-90deg)' : 'rotate(90deg)', transitionDuration: '0.3s', transitionProperty: 'transform' }} /> + <ArrowForwardIosIcon + sx={{ + color: 'secondary.light', + fontSize: 18, + m: 'auto', + stroke: theme.palette.secondary.light, + strokeWidth: '2px', + transform: isDropdownVisible ? 'rotate(-90deg)' : 'rotate(90deg)', + transitionDuration: '0.3s', + transitionProperty: 'transform' + }} + />packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx (1)
102-102: Consider extracting animation stylesThe icon's transform and transition styles could be moved to a separate style constant or theme configuration for better reusability.
+ const TOGGLE_ICON_STYLES = { + transform: (showMore: boolean) => showMore ? 'rotate(-180deg)' : 'rotate(0deg)', + transitionDuration: '0.2s', + transitionProperty: 'transform' + }; <ArrowDropDownIcon sx={{ color: 'secondary.light', fontSize: '20px', stroke: theme.palette.secondary.light, strokeWidth: '2px', - transform: showMore ? 'rotate(-180deg)' : 'rotate(0deg)', - transitionDuration: '0.2s', - transitionProperty: 'transform' + ...TOGGLE_ICON_STYLES(showMore) }} />packages/extension-polkagate/src/fullscreen/governance/components/AddressDropdown.tsx (1)
57-57: Consider consistent style property usageWhile the theme color changes are good, there's inconsistent usage of style properties:
- Line 57 uses
margin: 0- Other places use shorthand
m: 0Consider standardizing the approach across the component.
Choose one style and apply it consistently:
Either use longhand everywhere:
- m: 0 + margin: 0Or use shorthand everywhere:
- margin: 0 + m: 0Also applies to: 67-67, 81-93
packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/ChooseDelegator.tsx (1)
Line range hint
122-124: Consider accessibility for the "Clear" link color contrast.The dynamic color styling for the "Clear" link might not maintain sufficient contrast ratio in all theme modes, especially when disabled. Consider using Material-UI's built-in disabled styles or ensuring WCAG compliant contrast ratios.
- <Typography fontSize='16px' fontWeight={400} sx={{ color: selectedTrustedAddress ? theme.palette.mode === 'dark' ? 'text.primary' : 'primary.main' : theme.palette.text.disabled, cursor: selectedTrustedAddress ? 'pointer' : 'default', textAlign: 'left', textDecorationLine: 'underline' }}> + <Typography + component="button" + disabled={!selectedTrustedAddress} + fontSize='16px' + fontWeight={400} + sx={(theme) => ({ + background: 'none', + border: 'none', + color: selectedTrustedAddress ? theme.palette.primary.main : theme.palette.action.disabled, + cursor: selectedTrustedAddress ? 'pointer' : 'default', + textAlign: 'left', + textDecorationLine: 'underline', + '&:hover': { + color: selectedTrustedAddress ? theme.palette.primary.dark : theme.palette.action.disabled + } + })} + >packages/extension-polkagate/src/fullscreen/accountDetails/components/AOC.tsx (2)
169-169: Consider reducing stroke width for better icon appearance.While the theme-based stroke color is a good improvement, the stroke width of 2px might be too thick for a 20px icon. Consider reducing it to 1px for better visual balance.
-<ArrowDropDownIcon sx={{ color: 'secondary.light', fontSize: '20px', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: showMore ? 'rotate(-180deg)' : 'rotate(0deg)', transitionDuration: '0.2s', transitionProperty: 'transform' }} /> +<ArrowDropDownIcon sx={{ color: 'secondary.light', fontSize: '20px', stroke: theme.palette.secondary.light, strokeWidth: '1px', transform: showMore ? 'rotate(-180deg)' : 'rotate(0deg)', transitionDuration: '0.2s', transitionProperty: 'transform' }} />
Line range hint
15-22: Improve type safety for component props.The
hideNumbersprop type can be simplified, and some props could benefit from more specific types:interface Props { address: string | undefined; selectedAsset: FetchedBalance | undefined; accountAssets: FetchedBalance[] | null | undefined; onclick: (asset: FetchedBalance | undefined) => void; mode?: 'Home' | 'Detail'; - hideNumbers?: boolean | undefined + hideNumbers?: boolean }packages/extension-polkagate/src/popup/home/AccountDetail.tsx (1)
Line range hint
1-199: Consider splitting components into separate filesThe file contains several self-contained components (EyeButton, NoChainAlert, Price, Balance, BalanceRow) that could be moved to separate files to improve maintainability and reduce file complexity.
Consider:
- Moving each component to its own file under a
componentssubdirectory- Creating an
index.tsto re-export all components- Updating imports accordingly
This would improve:
- Code organization
- File maintainability
- Component reusability
- Testing isolation
packages/extension-polkagate/src/fullscreen/governance/ReferendumSummary.tsx (1)
114-114: Consider extracting section separator logic.While the usage of
VerticalBaris consistent and creates good visual separation, the repetitive pattern of conditionally rendering sections withVerticalBarcould be abstracted into a reusable pattern.Consider creating a helper component to encapsulate this pattern:
const SectionWithDivider: React.FC<{ show?: boolean; children: React.ReactNode }> = ({ children, show = true }) => { if (!show) return null; return ( <> <VerticalBar /> {children} </> ); };This would allow you to simplify the sections like:
<SectionWithDivider show={origin}> <Grid item sx={{ /* ... */ }}> {capitalizeFirstLetter(origin.replace(/([A-Z])/g, ' $1').trim())} </Grid> </SectionWithDivider>Also applies to: 120-120, 126-126, 133-133, 152-152
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/ReferendaTracks.tsx (1)
Line range hint
55-205: Consider architectural improvements for maintainability.The component has grown complex with multiple responsibilities. Consider:
- Breaking down into smaller sub-components (e.g., separate TrackItem component)
- Adding comprehensive unit tests for the complex logic
- Adding prop-types or TypeScript interfaces for better type safety
This would improve maintainability and make the code easier to test.
Would you like help with:
- Creating a separate TrackItem component structure?
- Setting up unit tests for the complex logic?
packages/extension-polkagate/src/fullscreen/governance/SearchBox.tsx (3)
Line range hint
13-24: Consider improving type safety for the component's props and state.While addressing the theme colors, we could also improve type safety:
- The
Filterinterface could be more strictly typed with literal types for status- The
LatestReferendatype (imported from './utils/types') should be reviewed for completenessConsider applying these type improvements:
interface Filter { advanced: { refIndex?: boolean; titles?: boolean; proposers?: boolean; methods?: boolean; } myReferenda: boolean; myVotes: boolean; - status: string; + status: typeof REFERENDA_STATUS[number][0] | 'All'; }
Line range hint
5-7: Consider removing unnecessary ESLint disable directives.These ESLint rules are disabled globally for the file, but some might be unnecessary:
react/jsx-no-bind: Modern React handles this well with useCallbackreact/jsx-max-props-per-lineandreact/jsx-first-prop-new-line: These are formatting concerns that could be handled by PrettierConsider removing these directives and fixing the underlying issues or configuring these rules project-wide if they're consistently disabled.
Remove
@ts-nocheckand fix type issues across the codebaseThe
@ts-nocheckdirective is extensively used throughout the codebase, with over 200 files disabling TypeScript type checking. This indicates a systemic issue with type safety across the project. The file under review is part of this broader pattern.Key observations:
- The directive is used in most React components, hooks, and utility files
- It appears to be a common practice rather than an isolated case
- Many core functionality files have type checking disabled
- The pattern spans across different packages in the monorepo
Recommendations:
- Remove
@ts-nocheckfrom this file and properly type the component- Create a dedicated task to gradually remove
@ts-nocheckdirectives across the codebase- Start with core utility files and shared components
- Consider using
// @ts-expect-errorfor specific lines that need temporary type bypassing- Add proper TypeScript configurations and type definitions for external dependencies
🔗 Analysis chain
Line range hint
4-4: Remove@ts-nocheckand fix type issues.The file has TypeScript checking completely disabled. This goes against type safety best practices and could hide potential runtime issues. Consider removing the
@ts-nocheckdirective and properly typing the component.Let's check if other files in the codebase also have this issue:
The previous command failed because the file type wasn't recognized. Let's try without type filters to see all occurrences of
@ts-nocheckin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files with @ts-nocheck rg "@ts-nocheck" --type typescript --type typescriptreactLength of output: 92
Script:
#!/bin/bash # Search for files with @ts-nocheck using ripgrep without file type filters rg "@ts-nocheck"Length of output: 25951
packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/Delegate.tsx (1)
62-62: Consider extracting icon styles for better maintainability.The ArrowForwardIosIcon has complex inline styles that could be extracted into a separate style object or theme component for better maintainability.
Consider refactoring to:
const iconStyles = { color: 'secondary.light', fontSize: 18, m: 'auto 8px', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: showAdvance ? 'rotate(-90deg)' : 'rotate(90deg)', transitionDuration: '0.3s', transitionProperty: 'transform' }; // Usage <ArrowForwardIosIcon sx={iconStyles} />packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (2)
76-83: Consider using type predicates for safer type assertions.While the type assertions work, they could be made safer using TypeScript type predicates or proper type guards.
Here's a suggested approach:
interface Changes { commission: { payee: string; value?: number; }; newPoolName: string; newRoles: { newBouncer: string; newNominator: string; newRoot: string; }; } function isString(value: unknown): value is string { return typeof value === 'string'; } function isNumber(value: unknown): value is number { return typeof value === 'number'; } // Then use these guards instead of direct type assertions const changes: Changes = { commission: { payee: isString(newCommissionPayee) ? newCommissionPayee : '', value: isNumber(newCommissionValue) ? newCommissionValue : undefined }, // ... rest of the changes };
98-98: Simplify getChangedValue function and improve type safety.The function's type signature could be improved with generics for better type safety.
Consider this improvement:
function getChangedValue<T extends string | number>( newValue: T | null | undefined, oldValue: T | null | undefined ): T | null | undefined { if (newValue === null || newValue === undefined) { return oldValue ? null : undefined; } return newValue === oldValue ? undefined : newValue; }packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx (1)
45-45: Add JSDoc comment to document the null case.Consider adding a JSDoc comment to explain when
delegateeAddresscan benullvsundefinedfor better maintainability.+/** Address of the delegatee. Can be: + * - undefined: when not yet selected + * - null: when explicitly cleared/reset + * - string: when a valid address is selected + */ delegateeAddress?: string | null;packages/extension-polkagate/src/popup/staking/partial/PoolMoreInfo.tsx (1)
194-204: LGTM! Theme integration looks good.The changes properly integrate with MUI's theming system and improve visual consistency. The border color now uses the semantic 'divider' color, and the icon's stroke color is properly theme-aware.
Consider extracting the icon styles to a constant to improve readability:
- <ArrowForwardIosIcon sx={{ color: 'secondary.light', fontSize: 18, m: 'auto', stroke: theme.palette.secondary.light, strokeWidth: '2px', transform: show ? 'rotate(-90deg)' : 'rotate(90deg)' }} /> + <ArrowForwardIosIcon + sx={{ + color: 'secondary.light', + fontSize: 18, + m: 'auto', + stroke: theme.palette.secondary.light, + strokeWidth: '2px', + transform: show ? 'rotate(-90deg)' : 'rotate(90deg)' + }} + />packages/extension-polkagate/src/fullscreen/stake/solo/partials/RewardsChart.tsx (4)
231-231: Consider enhancing error handling.While catching and logging errors is good, consider handling API errors more gracefully by:
- Showing an error message to the user
- Implementing retry logic for transient failures
- }).catch(console.error); + }).catch((error) => { + console.error('Failed to fetch rewards:', error); + // TODO: Show error message to user + setRewardsInfo(null); // Fallback to no-data state + });
234-234: Consider using more specific types for event handler.The event handler could benefit from more specific types.
- const handleAccordionChange = useCallback((panel: number) => (_event: React.SyntheticEvent, isExpanded: boolean) => { + const handleAccordionChange = useCallback((panel: number) => (_event: React.SyntheticEvent<Element, Event>, isExpanded: boolean) => {
269-284: Consider using Chart.js specific types.The tooltip callbacks could benefit from using Chart.js specific types instead of custom types.
- label: function (TooltipItem: { formattedValue: string } | undefined) { + label: function (context: import('chart.js').TooltipItem<'bar'>) { - title: function (TooltipItem: { label: string }[] | undefined) { + title: function (context: import('chart.js').TooltipItem<'bar'>[]) {
359-382: Consider extracting Arrows component.For better maintainability and reusability, consider moving the Arrows component to a separate file.
This would:
- Reduce file complexity
- Make the component more reusable
- Improve code organization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (42)
packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx(3 hunks)packages/extension-polkagate/src/components/ChainLogo.tsx(1 hunks)packages/extension-polkagate/src/components/Checkbox2.tsx(1 hunks)packages/extension-polkagate/src/components/Identity.tsx(3 hunks)packages/extension-polkagate/src/components/MenuItem.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/accountDetails/components/AOC.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/addNewChain/utils.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/ReferendumSummary.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/governance/SearchBox.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/components/AddressDropdown.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/ChooseDelegator.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/Delegate.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/partial/ReferendaTracks.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/partial/TAccountDisplay.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/remove/RemoveDelegate.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/governance/old-SupportUs.tsx(0 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencySwitch.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/HomeMenu.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ImportAccSubMenuFullScreen.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx(10 hunks)packages/extension-polkagate/src/fullscreen/stake/pool/partials/ClaimedRewardsChart.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/stake/solo/partials/ActiveValidators.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/stake/solo/partials/RewardsChart.tsx(13 hunks)packages/extension-polkagate/src/partials/FullScreenChainSwitch.tsx(2 hunks)packages/extension-polkagate/src/partials/ImportAccSubMenu.tsx(1 hunks)packages/extension-polkagate/src/partials/RemoteNodeSelectorWithSignals.tsx(2 hunks)packages/extension-polkagate/src/popup/history/modal/HistoryItemModal.tsx(3 hunks)packages/extension-polkagate/src/popup/history/partials/HistoryItem.tsx(3 hunks)packages/extension-polkagate/src/popup/home/AccountDetail.tsx(3 hunks)packages/extension-polkagate/src/popup/import/importLedger/LegacyApps.tsx(1 hunks)packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx(1 hunks)packages/extension-polkagate/src/popup/newAccount/deriveAccount/AddressDropdown.tsx(3 hunks)packages/extension-polkagate/src/popup/newAccount/deriveAccount/modal/AddressDropdown.tsx(3 hunks)packages/extension-polkagate/src/popup/newAccount/deriveFromAccountsFullscreen/AddressDropdownFullScreen.tsx(2 hunks)packages/extension-polkagate/src/popup/staking/partial/PoolMoreInfo.tsx(2 hunks)packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/CollapseIt.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extension-polkagate/src/fullscreen/governance/old-SupportUs.tsx
✅ Files skipped from review due to trivial changes (13)
- packages/extension-polkagate/src/components/Checkbox2.tsx
- packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx
- packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencySwitch.tsx
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/HomeMenu.tsx
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ImportAccSubMenuFullScreen.tsx
- packages/extension-polkagate/src/fullscreen/stake/pool/partials/ClaimedRewardsChart.tsx
- packages/extension-polkagate/src/fullscreen/stake/solo/partials/ActiveValidators.tsx
- packages/extension-polkagate/src/partials/FullScreenChainSwitch.tsx
- packages/extension-polkagate/src/partials/ImportAccSubMenu.tsx
- packages/extension-polkagate/src/partials/RemoteNodeSelectorWithSignals.tsx
- packages/extension-polkagate/src/popup/import/importLedger/LegacyApps.tsx
- packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx
🔇 Additional comments (65)
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/TAccountDisplay.tsx (2)
6-10: LGTM! Import organization and type imports look good.
The explicit type import for ApiPromise and grouped Material-UI imports improve code organization and type safety.
21-21: Verify Identity component's handling of null/undefined chain prop.
The chain prop can be null or undefined. Let's verify that the Identity component properly handles these cases.
✅ Verification successful
Chain prop is properly handled with null/undefined checks
The Identity component safely handles null/undefined chain prop through:
- Optional chaining for all chain property access:
chain?.genesisHash,chain?.icon,chain?.ss58Format - Fallback values using nullish coalescing:
_chain?.icon ?? 'polkadot',_chain?.ss58Format ?? 42 - Chain prop is properly typed as
Chain | null | undefinedin the interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Identity component's chain prop type and handling
ast-grep --pattern 'interface $_ {
chain: $_
$$$
}'
# Check for null checks on chain prop
rg -A 3 'chain(\?|!)?\.' packages/extension-polkagate/src/components/Identity
Length of output: 4435
Script:
#!/bin/bash
# Let's check the actual file and component where chain prop is used
fd TAccountDisplay.tsx
cat $(fd TAccountDisplay.tsx)
# Check for Identity component usage and implementation
ast-grep --pattern 'const Identity = $_'
# Search for Identity component file
fd Identity.tsx
Length of output: 2061
Script:
#!/bin/bash
# Let's check the Identity component implementation
cat packages/extension-polkagate/src/components/Identity.tsx
# Also check for any null checks or handling of chain prop
rg -A 3 'chain(\?|!)?\.' packages/extension-polkagate/src/components/Identity.tsx
Length of output: 10497
packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/CollapseIt.tsx (3)
7-7: LGTM! Clean import addition for theme support.
18-21: LGTM! Good use of React hooks.
The implementation of useTheme and useCallback follows React best practices. The callback memoization will prevent unnecessary re-renders.
25-25: LGTM! Improved theme consistency.
Good use of Material-UI's theme system:
- Using
borderBottomColor: 'divider'for consistent border styling - Using
theme.palette.secondary.lightfor icon stroke color
Also applies to: 30-30
packages/extension-polkagate/src/fullscreen/addNewChain/utils.tsx (1)
31-31: LGTM! Type enhancement maintains consistency across components.
The addition of null type to _genesisHash parameter aligns with the broader type safety improvements across the codebase, particularly matching the changes in the ChainLogo component.
Let's verify that this type enhancement is consistently applied across related components:
✅ Verification successful
Let me gather more information to verify the type consistency across components.
Let me analyze the provided code to verify if adding null type to _genesisHash parameter in useUserAddedChainColor function has any impact.
Let me check how the genesisHash parameter is passed to useUserAddedChainColor in ChainLogo component.
Let me try a different search pattern to find where ChainLogo is used with potentially null genesisHash.
Based on the findings, I can now provide a final response:
The addition of null type to _genesisHash parameter in useUserAddedChainColor is a valid type safety improvement
The change is necessary because:
- The
ChainLogocomponent acceptsgenesisHashprop that can bestring | undefined | null - This prop is directly passed to
useUserAddedChainColorhook - There is at least one case where
nullis explicitly passed as shown inAccountInformationForDetails.tsx:genesisHash={chain === null ? null : genesisHash}
The type enhancement ensures type safety by matching the actual usage patterns in the codebase, particularly in handling edge cases where chain information may be null.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent genesis hash type handling across components
# Expected: All related components should handle null genesis hash values
# Search for genesisHash type definitions and usages
rg -t typescript -t tsx "genesisHash.*:.*string" --no-heading
# Search for components using this utility function
ast-grep --pattern 'useUserAddedChainColor($$$)'
Length of output: 282
Script:
#!/bin/bash
# Check the ChainLogo component implementation and type definition
cat packages/extension-polkagate/src/components/ChainLogo.tsx
# Search for other files that might handle genesisHash
rg -g '*.{ts,tsx}' 'genesisHash' -A 2 -B 2
# Look for type definitions related to chain or genesis hash
fd -e ts -e tsx | xargs grep -l "type.*Chain"
Length of output: 366569
Script:
#!/bin/bash
# Search for usages of useUserAddedChainColor to understand how it's used
ast-grep --pattern 'useUserAddedChainColor($$$)'
# Search for any direct usages of the function's return value
rg -B 2 -A 2 'useUserAddedChainColor'
Length of output: 1800
Script:
#!/bin/bash
# Check ChainLogo component implementation
cat packages/extension-polkagate/src/components/ChainLogo.tsx
# Search for places where ChainLogo is used with null genesisHash
rg -B 2 -A 2 '<ChainLogo.*genesisHash={.*null.*}'
Length of output: 2745
Script:
#!/bin/bash
# Search for ChainLogo usages with null genesisHash
rg -B 2 -A 2 '<ChainLogo.*genesisHash.*null'
# Search for places where ChainLogo is used with chain being null
rg -B 2 -A 2 'chain === null.*ChainLogo'
rg -B 2 -A 2 'chain === null.*genesisHash'
Length of output: 906
packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx (4)
6-7: LGTM: Clean import organization
The Chain type import is properly placed and necessary for the component's type definitions.
28-28: LGTM: Improved function signature formatting
The restructured function signature enhances readability while maintaining proper prop destructuring.
39-41: LGTM: Improved type safety by removing unnecessary type assertions
The removal of explicit type assertions for address and chain props is a good practice, allowing TypeScript's type inference to work naturally. This change aligns with the PR's objective of addressing type issues.
Also applies to: 53-53
21-21: Verify the impact of setAddress type change
The simplified type for setAddress improves type safety by standardizing on string representation. However, we should verify that this change doesn't break any existing usage patterns.
packages/extension-polkagate/src/components/ChainLogo.tsx (1)
18-18: Type enhancement looks good but verify null handling.
The addition of null to the genesisHash type aligns with the PR's goal of addressing type issues and matches similar changes in related components. The component's logic already handles undefined values safely, making this a compatible change.
Let's verify other components are consistently updated to handle null values:
✅ Verification successful
Based on the extensive search results, I can now make a definitive assessment about the genesisHash type change in ChainLogo.tsx.
Type enhancement is safe and consistent with codebase patterns
The addition of null to the genesisHash type is verified to be safe and consistent because:
- The ChainLogo component is widely used across the codebase with various genesisHash patterns
- Many other components and interfaces already handle null genesisHash:
useMetadatahook acceptsstring | null | undefinedAddresscomponent acceptsstring | null- Multiple interfaces in the codebase declare
genesisHash?: string | null
- The component's internal logic already handles undefined values safely through the
useUserAddedChainColorhook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent null handling in related components
# Look for components that consume ChainLogo and verify their type definitions
# Search for ChainLogo imports and usage
rg -l "import.*ChainLogo" --type typescript --type typescriptreact
# Search for genesisHash type definitions in other components
ast-grep --pattern 'genesisHash?: string'
Length of output: 147
Script:
#!/bin/bash
# Let's try a different approach to find ChainLogo usage and genesisHash patterns
# Find TypeScript/React files that import ChainLogo
rg -l "import.*ChainLogo" --type-add 'ts:*.{ts,tsx}' --type ts
# Look for genesisHash type definitions and usages
rg -A 2 "genesisHash.*:" --type-add 'ts:*.{ts,tsx}' --type ts
# Find files that might use ChainLogo component
rg -l "<ChainLogo" --type-add 'ts:*.{ts,tsx}' --type ts
# Look for interface or type definitions containing genesisHash
ast-grep --pattern 'interface $_ {
$$$
genesisHash$_
$$$
}'
Length of output: 79804
packages/extension-polkagate/src/components/MenuItem.tsx (2)
7-7: LGTM: Clean import addition
The useTheme hook import is properly grouped with other MUI imports.
28-29: LGTM: Clean hook implementation
The useTheme hook is properly implemented at the component's top level, following React hooks best practices.
packages/extension-polkagate/src/popup/newAccount/deriveAccount/modal/AddressDropdown.tsx (3)
7-7: LGTM: Clean import addition
The useTheme hook import is properly grouped with other @mui/material imports.
21-21: LGTM: Improved function signature formatting
The reformatting enhances readability while maintaining proper typing.
22-23: LGTM: Proper theme hook usage
The useTheme hook is correctly initialized at the component's top level.
packages/extension-polkagate/src/popup/history/modal/HistoryItemModal.tsx (2)
9-9: LGTM: Clean import addition
The addition of useTheme hook follows proper import conventions and supports the theme-based styling changes.
30-30: LGTM: Proper hook initialization
The theme hook is correctly initialized at the component's top level, following React hooks best practices.
packages/extension-polkagate/src/popup/history/partials/HistoryItem.tsx (2)
9-9: LGTM: Clean import organization.
The addition of useTheme to the existing Material-UI imports is properly organized and follows best practices.
30-31: LGTM: Proper theme hook initialization.
The useTheme hook is correctly initialized at the component level, following React hooks best practices.
packages/extension-polkagate/src/popup/newAccount/deriveAccount/AddressDropdown.tsx (3)
7-7: LGTM: Clean import addition
The useTheme hook import is correctly grouped with other Material-UI imports.
22-22: LGTM: Improved function signature formatting
The added space after the function name enhances readability while maintaining functionality.
24-25: LGTM: Proper theme hook initialization
The useTheme hook is correctly initialized and follows React hooks rules.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx (3)
9-9: LGTM: Clean theme integration
The addition of the theme hook and its initialization follows Material-UI best practices for theme-based styling.
Also applies to: 58-58
99-101: LGTM: Appropriate typography adjustment
The font size reduction for the toggle text improves visual hierarchy while maintaining readability.
102-102: LGTM: Well-implemented theme-based styling
The icon styling demonstrates good practices:
- Consistent use of theme colors
- Smooth animation for state transitions
- Proper stroke width for visibility
packages/extension-polkagate/src/popup/newAccount/deriveFromAccountsFullscreen/AddressDropdownFullScreen.tsx (1)
65-65: LGTM! Good improvement using theme-based color.
The change from hardcoded color to theme.palette.secondary.light aligns with Material-UI best practices and improves theme consistency.
packages/extension-polkagate/src/fullscreen/governance/components/AddressDropdown.tsx (4)
6-10: LGTM: Import changes enhance type safety
The addition of explicit type imports and the Material-UI theme hook aligns well with the PR objectives.
25-27: LGTM: Proper theme initialization
The theme setup follows Material-UI best practices and maintains type safety.
30-32: LGTM: Clean state management
The hook organization is clean and follows React best practices.
39-39: Consider adding runtime type validation
While the type assertion helps with compile-time safety, consider adding runtime validation before the type assertion to prevent potential errors.
Consider this safer approach:
- addr && chainGenesis && tieAccount(addr, chainGenesis as HexString).then(() => setTimeout(() => onSelect(addr), 150)).catch(console.error);
+ if (!addr || !chainGenesis) return;
+ if (!/^0x[0-9a-fA-F]*$/.test(chainGenesis)) {
+ console.error('Invalid chain genesis format');
+ return;
+ }
+ tieAccount(addr, chainGenesis as HexString)
+ .then(() => setTimeout(() => onSelect(addr), 150))
+ .catch(console.error);packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/ChooseDelegator.tsx (4)
6-7: LGTM! Import consolidation improves code organization.
The consolidation of API-related hooks into useInfo and the explicit import of DelegateInformation type enhance code maintainability and type safety.
Also applies to: 13-13
23-23: LGTM! Consistent function declaration style.
The function signature formatting change maintains consistency with the codebase's style guidelines.
25-26: LGTM! Improved hook organization and theme support.
The consolidation of API-related hooks into useInfo reduces code duplication, while the addition of theme support enables consistent styling across the application.
Also applies to: 29-29
78-78: LGTM! Clear indication of unused parameter.
The use of _event prefix clearly indicates that the event parameter is intentionally unused, following common TypeScript conventions.
packages/extension-polkagate/src/fullscreen/accountDetails/components/AOC.tsx (1)
119-120: LGTM: Theme initialization follows best practices.
The theme hook is correctly initialized at the component level, following Material-UI conventions.
packages/extension-polkagate/src/popup/home/AccountDetail.tsx (2)
54-54: LGTM: Consistent theme usage for icon styling
The change properly implements theme-based styling for the ArrowForwardIosIcon, improving consistency with the application's theming system.
Also applies to: 63-63
166-168: LGTM: Font size adjustment improves visual hierarchy
The font size reduction from 28px to 24px is appropriate and maintains readability while properly handling text overflow.
Let's verify consistent font size usage across related components:
✅ Verification successful
Based on the search results, I can now provide a final response regarding the font size change in AccountDetail.tsx:
Font size change is consistent with design patterns
The font size reduction from 28px to 24px for account names aligns with the codebase's typography patterns:
- Account names in similar contexts use 24px (e.g. in social recovery and identity management)
- The change maintains readability while improving visual hierarchy
- Text overflow properties are properly configured to handle long names
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent font size usage in related components
# Expect: All account name Typography components should use the same font size
rg -U "Typography.*fontSize.*(?:24|28)px" --type tsx
Length of output: 82
Script:
#!/bin/bash
# Let's try without file type restriction since the extension might not be recognized
rg -U "Typography.*fontSize.*(?:24|28)px"
Length of output: 8699
Script:
#!/bin/bash
# Let's also check for any Typography components with font size in general
# to understand the pattern across the codebase
rg -U "Typography.*fontSize.*px"
Length of output: 140975
Script:
#!/bin/bash
# Let's also check specifically in the AccountDetail.tsx file and its directory
fd AccountDetail.tsx --exec cat {}
Length of output: 7541
packages/extension-polkagate/src/fullscreen/governance/ReferendumSummary.tsx (2)
33-35: LGTM! Good improvement to theme usage.
The refactoring of VerticalBar to use the theme's divider color directly instead of passing the theme as a prop is a good improvement. This change:
- Simplifies the component's API
- Better utilizes Material-UI's theming system
- Maintains visual consistency
117-119: LGTM! Consistent theme color usage.
The change to use theme.palette.divider for the border color aligns well with the component's visual hierarchy and maintains consistency with the theme-based styling approach.
packages/extension-polkagate/src/fullscreen/governance/delegate/remove/RemoveDelegate.tsx (2)
53-54: LGTM: Type safety improvement for delegateeName.
The type assertion and optional chaining ensure proper handling of potentially undefined values when fetching the delegate's identity.
150-150: Consider adding null check for delegateeAddress.
While the type assertion works, consider adding a null check since delegateeAddress could be null based on the DelegateInformation interface.
Consider this safer approach:
-formatted={delegateeAddress as string | undefined}
+formatted={delegateeAddress ?? undefined}This ensures null values are properly converted to undefined, maintaining type safety while preventing potential runtime issues.
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/ReferendaTracks.tsx (3)
Line range hint 33-53: LGTM! Good improvement to prop types.
Making withCheckBox optional with a default value improves the component's flexibility while maintaining type safety.
148-153: LGTM! Good use of theme colors.
The change to use secondary.light from the theme system improves consistency and maintainability of the UI styling.
4-11: 🛠️ Refactor suggestion
Consider removing @ts-nocheck directive.
While the explicit type imports are a good addition, using @ts-nocheck suppresses all TypeScript checks in the file. Consider addressing the underlying type issues instead of bypassing them completely.
Let's check for TypeScript errors that might be getting suppressed:
packages/extension-polkagate/src/fullscreen/governance/SearchBox.tsx (1)
162-162: LGTM: Theme color standardization.
Good change! Using theme.palette.secondary.light instead of hardcoded colors improves theme consistency and maintainability. This aligns with the PR's objective of standardizing theme colors across components.
packages/extension-polkagate/src/fullscreen/governance/delegate/delegate/Delegate.tsx (2)
15-15: LGTM: Theme hook import added correctly.
The addition of useTheme hook is properly grouped with other Material-UI imports and supports the theme-based styling changes.
54-54: LGTM: Theme hook and Typography color.
The theme hook is properly initialized and the Typography component correctly uses the theme's secondary light color.
Also applies to: 59-61
packages/extension-polkagate/src/components/Identity.tsx (3)
14-14: LGTM! Improved type safety with proper MUI type imports.
The addition of SxProps and Theme types from Material-UI improves type safety and provides better IDE support for styling properties.
40-40: LGTM! Enhanced style prop typing.
The style prop now properly supports both Material-UI's SxProps<Theme> and React's CSSProperties, providing more flexibility in styling while maintaining type safety.
42-42: Verify the impact of making withShortAddress optional.
Making withShortAddress optional is a breaking change that could affect existing component usage. Let's verify all current usages to ensure they handle the optional prop correctly.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (2)
240-241: LGTM: Typography changes improve consistency.
The font size reduction and text simplification align with the PR objectives and enhance UI consistency.
243-243: LGTM: Theme-based stroke color enhances maintainability.
Good improvement replacing the hardcoded color with theme-based styling. This change:
- Improves maintainability by centralizing color management
- Ensures consistent theming across the application
- Supports both light and dark themes automatically
packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (3)
7-11: LGTM: Type imports enhance type safety.
The added type imports provide better type safety and clarity for the component's props and state.
54-54: LGTM: Proper typing for maxCommission state.
The state type change to Perbill correctly matches the API's return type, improving type safety.
150-150: LGTM: Consistent chain prop usage.
The chain prop is consistently passed to all AddressInput components, improving type safety and component consistency.
Also applies to: 162-162, 174-174, 186-186, 239-239
packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx (1)
45-45: Verify the type change impact across the codebase.
The type enhancement to allow explicit null values for delegateeAddress is a good practice for better type safety. However, let's verify that this change is handled correctly in all components using this interface.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about all usages of delegateeAddress across the codebase.
Type change is safe and properly handled across the codebase
The change to allow null values for delegateeAddress is safe as all usages properly handle the nullability:
- In
index.tsx, the null check is properly done withdelegateInformation?.delegateeAddress - In
ChooseDelegator.tsx, assignment is done with nullish coalescing - In other components like
Review.tsx,ModifyDelegate.tsx, andRemoveDelegate.tsx, the value is always accessed after existence checks or through optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for components using DelegateInformation interface and verify type handling
# Test 1: Find all files importing or using DelegateInformation
echo "Files importing or using DelegateInformation:"
rg -l "DelegateInformation"
# Test 2: Find all usages of delegateeAddress property
echo -e "\nUsages of delegateeAddress:"
rg "delegateeAddress" -A 2 -B 2
Length of output: 19580
packages/extension-polkagate/src/popup/staking/partial/PoolMoreInfo.tsx (1)
12-12: LGTM!
The useTheme import is correctly added and properly organized with other Material-UI imports.
packages/extension-polkagate/src/fullscreen/stake/solo/partials/RewardsChart.tsx (7)
8-9: LGTM! Import organization improvement.
Moving type imports to the top of the file follows TypeScript best practices.
61-61: LGTM! Type safety improvement.
Explicit typing of dateOptions as Intl.DateTimeFormatOptions enhances type safety.
138-138: LGTM! Null safety improvement.
Added null check for the date property prevents potential runtime errors.
243-244: LGTM! Null safety improvement.
Added optional chaining operators prevent potential null reference errors.
306-308: LGTM! Code safety improvements.
Added optional chaining and improved parameter naming enhance code safety and readability.
Also applies to: 340-340, 344-344
445-445: LGTM! Type safety improvement.
Added required chain prop to Identity component.
474-477: LGTM! UI improvements.
The changes properly implement theme-based styling and font size adjustments as per PR objectives.
|
|
||
| interface Props { | ||
| title: string; | ||
| show: boolean; | ||
| open: (title: 'Roles' | 'Commission') => void; | ||
| open: (title: string) => void; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can now generate the final response:
The union type constraint should be maintained for the open prop
Looking at the actual usage of CollapseIt component across the codebase, it's exclusively used with two specific title values: 'Roles' and 'Commission' (wrapped in translation function t()). Loosening the type to string would reduce type safety without providing any practical benefit.
packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/index.tsx: Uses witht('Roles')andt('Commission')packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx: Uses witht('Roles')andt('Commission')
- open: (title: string) => void;
+ open: (title: 'Roles' | 'Commission') => void;🔗 Analysis chain
Consider maintaining strict type safety.
The change from open: (title: 'Roles' | 'Commission') => void to open: (title: string) => void makes the component more flexible but reduces type safety. If this component is specifically designed for 'Roles' and 'Commission', consider keeping the union type to prevent potential runtime issues with invalid strings.
- open: (title: string) => void;
+ open: (title: 'Roles' | 'Commission' | /* add other valid values if needed */) => void;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other valid title values being used
rg -A 1 "<CollapseIt.*title="
Length of output: 31
Script:
#!/bin/bash
# Let's try to find where CollapseIt is used and how the open prop is called
rg -A 3 "CollapseIt"
# Also search for the component definition to understand its usage context
ast-grep --pattern 'const CollapseIt = $_'
Length of output: 4609
| @@ -24,7 +24,9 @@ interface Props { | |||
| withHoverEffect?: boolean; | |||
| } | |||
|
|
|||
| export default function MenuItem({ children, disabled = false, fontSize, icon, iconComponent, onClick, pl = '0', py = '8px', showChevron, showSubMenu = false, text, withHoverEffect }: Props): React.ReactElement<Props> { | |||
| export default function MenuItem ({ children, disabled = false, fontSize, icon, iconComponent, onClick, pl = '0', py = '8px', showChevron, showSubMenu = false, text, withHoverEffect }: Props): React.ReactElement<Props> { | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can see that the prevalent style in the codebase is to have no space between the function name and the opening parenthesis for function parameters. The vast majority of function declarations follow this pattern, like function MenuItem({, function Button({, etc.
Keep consistent function declaration style
The added space after the function name should be removed to maintain consistency with the rest of the codebase. Change MenuItem ({ to MenuItem({.
🔗 Analysis chain
Verify style guide for function spacing
The added space after the function name appears to be a stylistic change. Could you confirm if this aligns with the project's style guide? If not, we should maintain consistency with the existing codebase style.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the prevalent function declaration style in the codebase
rg -U "export default function \w+\s*\(" --type typescript
Length of output: 95
Script:
#!/bin/bash
# Check the prevalent function declaration style in the codebase using ts/tsx extensions
rg -U "export default function \w+\s*\(" -t ts -t tsx
Length of output: 83
Script:
#!/bin/bash
# Search for function declarations in TypeScript/React files using file extensions
rg -U "export default function \w+\s*\(" --glob "*.{ts,tsx}"
Length of output: 106994
| //@ts-ignore | ||
| const arrayData = data?.[chainName] ? Object.values(data[chainName] as object) as string[] : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace @ts-ignore with proper type definitions.
Using @ts-ignore bypasses TypeScript's type checking, which could hide potential runtime errors. Consider defining proper types for the GitHub response data.
- //@ts-ignore
- const arrayData = data?.[chainName] ? Object.values(data[chainName] as object) as string[] : undefined;
+ interface TrustedAccountsData {
+ [chainName: string]: {
+ [key: string]: string;
+ };
+ }
+ const arrayData = data?.[chainName as keyof TrustedAccountsData]
+ ? Object.values(data[chainName as keyof TrustedAccountsData])
+ : undefined;Committable suggestion skipped: line range outside the PR's diff.
| //@ts-ignore | ||
| const availableTracks = tracks.filter((track) => !(track[0] in existingVotes)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace @ts-ignore with proper type handling.
Using @ts-ignore masks potential type safety issues. Consider:
- Adding proper type assertions
- Updating the Track interface to match the expected structure
Example approach:
const availableTracks = tracks.filter((track) => {
const [trackId] = track;
return !(trackId.toString() in existingVotes);
});| @@ -1,6 +1,8 @@ | |||
| // Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| //@ts-nocheck | |||
There was a problem hiding this comment.
Remove the ts-nocheck directive and fix type issues properly.
Adding @ts-nocheck disables all TypeScript type checking for this file, which contradicts the PR's objective of addressing type issues. This is considered an anti-pattern as it:
- Bypasses TypeScript's type safety benefits
- Could hide potential bugs
- Makes the codebase harder to maintain
Instead of disabling type checking, we should properly fix the underlying type issues. Would you like assistance in identifying and resolving the specific type issues in this file?
| @@ -120,7 +123,7 @@ export default function Edit({ api, chain, changes, onClose, pool, setChanges, s | |||
| return value === undefined; | |||
| }); | |||
|
|
|||
| const onNewCommission = useCallback((e) => { | |||
| const onNewCommission = useCallback((e: { target: { value: any; }; }) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific types for event handler.
Replace the any type with a more specific type for better type safety.
- const onNewCommission = useCallback((e: { target: { value: any; }; }) => {
+ const onNewCommission = useCallback((e: React.ChangeEvent<HTMLInputElement>) => {📝 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 onNewCommission = useCallback((e: { target: { value: any; }; }) => { | |
| const onNewCommission = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { |
| //@ts-ignore | ||
| const maybeCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined; | ||
| //@ts-ignore | ||
| const maybeCommission = (pool?.bondedPool?.commission?.current?.[0] || 0) as number; |
There was a problem hiding this comment.
Remove @ts-ignore and handle types properly.
Instead of using @ts-ignore, consider properly typing the pool.bondedPool.commission structure or using type guards to handle potential undefined values safely.
Here's a suggested approach:
- //@ts-ignore
- const maybeCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString() as string | undefined;
- //@ts-ignore
- const maybeCommission = (pool?.bondedPool?.commission?.current?.[0] || 0) as number;
+ interface Commission {
+ current?: [number, string];
+ }
+ interface BondedPool {
+ commission?: Commission;
+ roles: {
+ depositor?: string;
+ // ... other roles
+ };
+ }
+ const maybeCommissionPayee = pool?.bondedPool?.commission?.current?.[1]?.toString();
+ const maybeCommission = pool?.bondedPool?.commission?.current?.[0] ?? 0;Committable suggestion skipped: line range outside the PR's diff.
| api?.query['nominationPools']['globalMaxCommission']().then((maybeResponse) => { | ||
| const res = maybeResponse as Option<Perbill>; | ||
|
|
||
| if (res.isSome) { | ||
| setMaxCommission(res.unwrap()); | ||
| } | ||
| }); | ||
| }).catch(console.error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in API query.
The API query could benefit from better error handling and type safety.
Consider this improvement:
- api?.query['nominationPools']['globalMaxCommission']().then((maybeResponse) => {
- const res = maybeResponse as Option<Perbill>;
-
- if (res.isSome) {
- setMaxCommission(res.unwrap());
- }
- }).catch(console.error);
+ try {
+ const res = await api?.query['nominationPools']['globalMaxCommission']();
+
+ if (res && res.isSome) {
+ setMaxCommission(res.unwrap());
+ }
+ } catch (error) {
+ console.error('Failed to fetch max commission:', error);
+ // Consider showing user feedback
+ }Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation