refactor: separate routing #1673
Conversation
WalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on enhancing error handling, routing, and the introduction of new constants. Key changes include the addition of Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)packages/extension-ui/src/Popup/index.tsx (2)
Investigate blank screen issue on request closure The PR comments mention a blank screen issue when closing Auth/Sign requests. This could be related to the Routes component not handling request closure states properly. Let's verify the routing behavior during request closure: Consider these potential solutions:
Would you like me to help investigate the specific implementation in the Routes component?
Consider using route constants and add request subscription cleanup While the routing logic has improved with the isMounted flag, there are two areas for enhancement:
Consider these improvements: + // At the top of the file
+ const ROUTES = {
+ AUTHORIZE: '/authorize',
+ METADATA: '/metadata',
+ SIGNING: '/signing',
+ HOME: '/'
+ } as const;
useEffect(() => {
let isMounted = true;
const handleRouting = () => {
if (!isMounted || !authRequests || !metaRequests || !signRequests) {
return;
}
if (authRequests.length) {
- _onAction('/authorize');
+ _onAction(ROUTES.AUTHORIZE);
} else if (metaRequests.length) {
- _onAction('/metadata');
+ _onAction(ROUTES.METADATA);
} else if (signRequests.length) {
- _onAction('/signing');
+ _onAction(ROUTES.SIGNING);
- } else if (['/authorize', '/metadata', '/signing'].includes(pathname)) {
+ } else if ([ROUTES.AUTHORIZE, ROUTES.METADATA, ROUTES.SIGNING].includes(pathname)) {
- _onAction('/');
+ _onAction(ROUTES.HOME);
}
};
handleRouting();
return () => {
isMounted = false;
};
}, [_onAction, authRequests, metaRequests, signRequests, pathname]);Let's verify the request subscription cleanup: 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: 5
🧹 Outside diff range and nitpick comments (10)
packages/extension-ui/src/Popup/components/RouteWrapper.tsx (3)
7-11: Add JSDoc documentation and consider more specific prop namesWhile the interface is well-typed, it would benefit from documentation and more specific naming:
+/** + * Props for the RouteWrapper component + * @property {React.ComponentType} wrappedComponent - The component to be wrapped + * @property {string} [errorTrigger] - Trigger identifier for error boundary + * @property {Record<string, unknown>} [componentProps] - Props to pass to the wrapped component + */ interface RouteWrapperProps { - component: React.ComponentType; - trigger?: string; - props?: Record<string, unknown>; + wrappedComponent: React.ComponentType; + errorTrigger?: string; + componentProps?: Record<string, unknown>; }
13-17: Enhance type safety and add validationThe component implementation could benefit from improved type safety:
-const RouteWrapper = ({ component: Component, props, trigger }: RouteWrapperProps) => ( +const RouteWrapper: React.FC<RouteWrapperProps> = ({ + component: Component, + props, + trigger +}) => { + if (!Component) { + throw new Error('RouteWrapper: Component is required'); + } + + return ( <ErrorBoundary trigger={trigger}> - <Component {...props} /> + <Component {...(props || {})} /> </ErrorBoundary> -); + ); +};
1-19: Consider enhancing route-specific type safetySince this is part of a routing refactor, consider creating a type-safe route configuration system:
- Define a route configuration type with path, component, and metadata
- Create a route registry to manage all routes
- Add route-specific prop types validation
This would provide better type safety and maintainability for the routing system.
Example implementation:
type RouteConfig = { path: string; component: React.ComponentType<any>; meta?: { requiresAuth?: boolean; roles?: string[]; }; }; type RouteRegistry = Record<string, RouteConfig>;packages/extension-ui/src/Popup/components/ErrorBoundary.tsx (2)
11-15: Review dependency direction between extension-ui and extension-polkagateThe component is importing from
@polkadot/extension-polkagatewhile residing inextension-ui. This dependency direction might create circular dependencies and could indicate that some components should be moved to a shared location.Consider:
- Moving shared components to a common package
- Or restructuring the imports to maintain a clear dependency hierarchy between extensions
Line range hint
70-102: Consider extracting error UI into a separate componentThe render method handles multiple rendering scenarios. Consider extracting the error UI into a separate component for better maintainability.
Example refactor:
const ErrorDisplay: React.FC<{ error: Error; onHome: () => void }> = ({ error, onHome, t }) => ( <> <HeaderBrand showBrand showMenu text={EXTENSION_NAME} /> <Grid container justifyContent='center' px='15px'> <Typography fontSize='18px' mt='35px'> {t('An error occurred')} </Typography> {/* ... rest of error UI ... */} </Grid> <PButton _onClick={onHome} text={t('Back to home')} /> </> );Then simplify the render method:
render(): React.ReactNode { const { children } = this.props; const { error } = this.state; return error ? <ErrorDisplay error={error} onHome={this.#goHome} /> : <> {children} {!this.isExtensionPopup && <AlertBox />} </>; }packages/extension-polkagate/src/components/index.ts (1)
34-34: Good architectural decision to centralize error handling!Moving the ErrorBoundary to a shared location in extension-ui promotes code reuse and consistent error handling across the extension.
packages/extension-ui/src/Popup/index.tsx (2)
12-12: Clarify the use ofuseLocationThe
useLocationhook is imported and called without using its return value:useLocation(); // just to trigger component to fix forgot pass issueWhile this may be intended to trigger a re-render, consider adding a comment to explain this non-standard use or finding a more explicit method to achieve the desired effect for better code readability.
104-118: Simplify dependencies in theuseEffecthookThe dependencies array includes both the arrays and their lengths:
[_onAction, authRequests, authRequests?.length, metaRequests, metaRequests?.length, signRequests, signRequests?.length]Including both the arrays and their lengths is unnecessary since changes to the arrays will trigger the effect. Simplify the dependencies array to improve clarity:
- }, [_onAction, authRequests, authRequests?.length, metaRequests, metaRequests?.length, signRequests, signRequests?.length]); + }, [_onAction, authRequests, metaRequests, signRequests]);packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx (2)
201-201: Fix the typo in thetriggerpropertyThere's a typo in the
triggerproperty at line 201:'pool-poolInfromation'should be'pool-poolInformation'.Apply this diff to correct the typo:
- trigger: 'pool-poolInfromation' + trigger: 'pool-poolInformation'
82-431: Consider refactoring route definitions into separate files for better maintainabilityThe file contains a large number of route definitions spanning lines 82-431, which can make it challenging to navigate and maintain. Splitting the route configurations into separate files or modules based on their categories (e.g.,
ACCOUNT_ROUTES,STAKING_ROUTES) would improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/extension-base/src/defaults.ts(2 hunks)packages/extension-polkagate/src/components/index.ts(1 hunks)packages/extension-ui/src/Popup/components/ErrorBoundary.tsx(4 hunks)packages/extension-ui/src/Popup/components/RouteWrapper.tsx(1 hunks)packages/extension-ui/src/Popup/index.tsx(3 hunks)packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx(1 hunks)packages/extension/package.json(1 hunks)packages/extension/src/packageInfo.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extension/package.json
- packages/extension/src/packageInfo.ts
🔇 Additional comments (4)
packages/extension-base/src/defaults.ts (2)
49-50: LGTM!
The export statements are properly organized and clearly marked as PolkaGate additions.
19-23: Verify if ROOT_PATH entries should be included in ALLOWED_PATH
The newly added ROOT_PATH contains fundamental extension operation paths. Consider whether these paths should also be included in the ALLOWED_PATH constant for consistency and proper access control.
packages/extension-ui/src/Popup/components/ErrorBoundary.tsx (1)
Line range hint 52-63: LGTM: Error handling implementation is robust
The error handling logic is well-implemented with:
- Proper static error state derivation
- Controlled error reset based on trigger prop changes
packages/extension-ui/src/Popup/index.tsx (1)
274-274: Ensure error handling after removing the error boundary
The <Routes /> component is now rendered directly without being wrapped in an error boundary. Verify that errors within the routing components are properly handled to prevent unhandled exceptions from affecting the application.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/extension-polkagate/src/hooks/useFullscreen.ts (1)
Line range hint
7-19: Consider adding error handling for missing root elementWhile the code checks for root element existence, it silently fails if the element is not found. Consider logging a warning in development to help catch potential issues.
useLayoutEffect(() => { /** to change app width to full screen */ const root = document.getElementById('root'); + if (process.env.NODE_ENV === 'development' && !root) { + console.warn('Root element not found for fullscreen hook'); + } + if (root) { root.style.width = '100%'; }packages/extension-ui/src/Popup/index.tsx (4)
104-116: Optimize navigation logic dependenciesThe navigation logic has redundant dependencies that could lead to unnecessary re-renders.
Consider this optimization:
useEffect(() => { if (!authRequests || !metaRequests || !signRequests) { return; } if (authRequests.length) { _onAction('/authorize'); } else if (metaRequests.length) { _onAction('/metadata'); } else if (signRequests.length) { _onAction('/signing'); } - }, [_onAction, authRequests, authRequests?.length, metaRequests, metaRequests?.length, signRequests, signRequests?.length]); + }, [_onAction, authRequests, metaRequests, signRequests]);
Line range hint
118-127: Add error handling for worker initializationThe worker initialization lacks error handling and status tracking.
Consider adding error handling and status tracking:
useEffect(() => { + try { workerRef.current = new Worker(new URL('../../../extension-polkagate/src/util/workers/sharedWorker.js', import.meta.url)); + workerRef.current.onerror = (error) => { + console.error('Worker initialization failed:', error); + // Handle worker initialization failure + }; + } catch (error) { + console.error('Failed to create worker:', error); + // Handle worker creation failure + } return () => { // Cleanup on unmount if (workerRef.current) { workerRef.current.terminate(); } }; }, []);
Line range hint
165-196: Improve price fetching error handling and race condition managementThe price fetching logic has several areas for improvement:
- Race conditions could occur if currency changes during fetch
- No retry mechanism for failed network requests
- Errors are silently logged
Consider implementing:
- Request cancellation for pending fetches when currency changes
- Retry mechanism with exponential backoff
- Proper error handling with user feedback
Example implementation:
useEffect(() => { if (priceIds && currency?.code && !isFetchingPricesRef.current) { isFetchingPricesRef.current = true; + const controller = new AbortController(); getStorage('pricesInCurrencies') .then((res) => { const savedPricesInCurrencies = (res || {}) as PricesInCurrencies; const maybeSavedPriceInCurrentCurrencyCode = savedPricesInCurrencies[currency.code]; if (maybeSavedPriceInCurrentCurrencyCode && isPriceUpToDate(maybeSavedPriceInCurrentCurrencyCode.date)) { return; } - getPrices(priceIds, currency.code.toLowerCase()) + getPrices(priceIds, currency.code.toLowerCase(), { signal: controller.signal }) .then((newPrices) => { delete (newPrices as Prices).currencyCode; savedPricesInCurrencies[currency.code] = newPrices; setStorage('pricesInCurrencies', savedPricesInCurrencies) - .catch(console.error); + .catch((error) => { + if (!controller.signal.aborted) { + setAlerts(prev => [...prev, { message: 'Failed to save prices', type: 'error' }]); + console.error('Storage error:', error); + } + }); }) - .catch(console.error) + .catch((error) => { + if (!controller.signal.aborted) { + setAlerts(prev => [...prev, { message: 'Failed to fetch prices', type: 'error' }]); + console.error('Fetch error:', error); + } + }) .finally(() => { - isFetchingPricesRef.current = false; + if (!controller.signal.aborted) { + isFetchingPricesRef.current = false; + } }); }) .catch(console.error) .finally(() => { isFetchingPricesRef.current = false; }); + return () => { + controller.abort(); + isFetchingPricesRef.current = false; + }; } }, [currency?.code, priceIds, setAlerts]);
Line range hint
261-289: Consider organizing context providers for better maintainabilityThe deep nesting of context providers makes the code harder to maintain and understand.
Consider grouping related contexts into composed providers:
// Create composed providers for related contexts const AccountRelatedProviders: React.FC = ({ children }) => ( <AccountIconThemeContext.Provider value={{ accountIconTheme, setAccountIconTheme }}> <AccountContext.Provider value={accountCtx}> <AccountsAssetsContext.Provider value={{ accountsAssets, setAccountsAssets }}> {children} </AccountsAssetsContext.Provider> </AccountContext.Provider> </AccountIconThemeContext.Provider> ); const RequestRelatedProviders: React.FC = ({ children }) => ( <AuthorizeReqContext.Provider value={authRequests}> <MetadataReqContext.Provider value={metaRequests}> <SigningReqContext.Provider value={signRequests}> {children} </SigningReqContext.Provider> </MetadataReqContext.Provider> </AuthorizeReqContext.Provider> );Then use these composed providers to reduce nesting:
- <AccountIconThemeContext.Provider value={{ accountIconTheme, setAccountIconTheme }}> - <AccountContext.Provider value={accountCtx}> - <AccountsAssetsContext.Provider value={{ accountsAssets, setAccountsAssets }}> - // ... more providers - </AccountsAssetsContext.Provider> - </AccountContext.Provider> - </AccountIconThemeContext.Provider> + <AccountRelatedProviders> + <RequestRelatedProviders> + <Routes /> + </RequestRelatedProviders> + </AccountRelatedProviders>packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx (2)
74-80: Add JSDoc documentation for the RouteConfig interfaceConsider adding JSDoc documentation to describe the purpose and usage of each property in the
RouteConfiginterface. This will improve code maintainability and help other developers understand the routing configuration.+/** + * Configuration interface for defining routes in the application. + * @interface RouteConfig + * @property {string} path - The URL path pattern for the route + * @property {React.ComponentType<any>} Component - The React component to render for this route + * @property {string} trigger - Identifier used for error boundaries and analytics + * @property {Record<string, unknown>} [props] - Optional props to pass to the component + * @property {boolean} [exact] - Whether the path should match exactly + */ interface RouteConfig { path: string; Component: React.ComponentType<any>; trigger: string; props?: Record<string, unknown>; exact?: boolean; }
433-454: Consider adding a top-level error boundaryWhile individual routes have error handling through
RouteWrapper, consider adding an error boundary at theSwitchlevel to catch routing-related errors that might occur outside individual routes.const Routes = () => { const routeComponents = useMemo(() => ALL_ROUTES.map(({ Component, exact, path, props, trigger }) => ( <Route exact={exact} key={path} path={path} > <RouteWrapper component={Component} props={props} trigger={trigger} /> </Route> )), []); return ( + <ErrorBoundary trigger="routing"> <Switch> {routeComponents} </Switch> + </ErrorBoundary> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/extension-polkagate/src/components/translate.ts(1 hunks)packages/extension-polkagate/src/hooks/useFullscreen.ts(1 hunks)packages/extension-ui/src/Popup/components/ErrorBoundary.tsx(3 hunks)packages/extension-ui/src/Popup/components/RouteWrapper.tsx(1 hunks)packages/extension-ui/src/Popup/index.tsx(3 hunks)packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-ui/src/Popup/components/ErrorBoundary.tsx
- packages/extension-ui/src/Popup/components/RouteWrapper.tsx
🔇 Additional comments (6)
packages/extension-polkagate/src/components/translate.ts (2)
4-4: LGTM! Clean import structure
The import statements are well-organized with proper separation of types and implementation imports. The aliasing of useTranslationBase is clear and maintains good readability.
Also applies to: 6-6
8-10: LGTM! Well-typed translation hook wrapper
The useTranslation function provides a clean, type-safe wrapper around the base translation hook. The explicit return type annotation ensures type safety for consumers of this hook.
packages/extension-polkagate/src/hooks/useFullscreen.ts (1)
4-4: Good choice using useLayoutEffect!
The switch from useEffect to useLayoutEffect is a good improvement as it ensures the width changes are applied synchronously before the browser paints, preventing potential visual flicker.
Also applies to: 7-7
packages/extension-ui/src/Popup/index.tsx (1)
274-274: Verify error boundary implementation in Routes
The removal of direct error boundary wrapping shifts error handling responsibility to the Routes component.
Let's verify the error boundary implementation:
✅ Verification successful
Let me gather more information about the Routes component to see how it's using the ErrorBoundary.
Let me try a different approach to find how Routes and ErrorBoundary are connected.
Let me check the route definitions to understand how RouteWrapper is used with the routes.
Error boundary implementation is properly handled
The error handling is correctly implemented through the RouteWrapper component. Each route in the application is wrapped with RouteWrapper, which in turn wraps all route components with ErrorBoundary. The ErrorBoundary component has proper error catching and rendering implementation, including:
- Error state management
- Error UI rendering with translated messages
- Recovery mechanism through the "Back to home" button
- Special handling for extension popup vs full-screen views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Routes component properly implements error boundary
ast-grep --pattern 'class ErrorBoundary extends React.Component {
$$$
}'
# Check if RouteWrapper uses ErrorBoundary
ast-grep --pattern 'function RouteWrapper($$$) {
$$$
<ErrorBoundary$$$>
$$$
</ErrorBoundary>
$$$
}'
Length of output: 7155
Script:
#!/bin/bash
# Check Routes component implementation
ast-grep --pattern 'function Routes() {
$$$
}'
# Check if Routes is wrapped with ErrorBoundary somewhere
rg -A 5 "<ErrorBoundary.*>.*<Routes"
Length of output: 89
Script:
#!/bin/bash
# Find Routes component definition and its usage
rg -l "Routes" packages/extension-ui/src/Popup/
# Check content of Routes file
cat packages/extension-ui/src/Popup/Routes.tsx
# Look for ErrorBoundary usage in the same directory
rg -A 3 "ErrorBoundary" packages/extension-ui/src/Popup/
Length of output: 1646
Script:
#!/bin/bash
# Check RouteDefinitions content
cat packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx
# Look for RouteWrapper usage
rg -A 3 "RouteWrapper" packages/extension-ui/src/Popup/routes/
Length of output: 14189
packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx (2)
1-456: Overall implementation looks good!
The routing system is well-organized, maintainable, and follows React Router best practices. The categorization of routes makes the code easy to understand and extend.
85-88: Verify path parameter validation
Several routes contain path parameters (e.g., :address, :assetId, :paramAssetId). Ensure that these parameters are properly validated in their respective components to prevent security issues like path traversal or injection attacks.
Also applies to: 120-123, 290-292, 339-342
✅ Verification successful
Let me analyze the validation of path parameters by checking the components that handle them. I'll search for validation logic in the relevant components.
Let me try a different search strategy to find the relevant component and validation logic.
Based on the analysis of the code, I can now provide a final response regarding the path parameter validation concern.
Path parameters are properly validated through multiple layers
The codebase shows robust validation of address parameters through:
-
Type safety: All route components use TypeScript generics to enforce parameter types (
useParams<{ address: string }>) -
Address validation utilities:
isValidAddress()function invalidateAddress.tsthat validates addresses using Polkadot's cryptographic functions- Address validation is implemented in components like
AddressInput,Identity, and other UI components - Invalid addresses trigger appropriate UI feedback and prevent further processing
- Parameter usage:
- Addresses are validated before being used in API calls or transactions
- Components use hooks like
useInfo,useApi, etc. that handle invalid addresses gracefully - Invalid addresses are prevented from causing runtime errors through proper validation checks
The implementation follows blockchain security best practices for handling address parameters, protecting against path traversal and injection attacks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for parameter validation in components that handle path parameters
# Search for path parameter usage in component files
echo "Checking parameter validation in components..."
rg -A 5 "useParams|props\.(address|assetId|paramAssetId)" packages/extension-polkagate/src/
Length of output: 77493
Script:
#!/bin/bash
# Search for validation logic in components that handle path parameters
# First check AccountEx component since it's referenced in the route
echo "Checking AccountEx component validation..."
fd AccountEx --exec cat {}
# Check validation patterns in route handling components
echo -e "\nChecking route parameter validation patterns..."
rg -A 5 "useParams.*address.*}" --type typescript packages/extension-ui/src/
Length of output: 350
Script:
#!/bin/bash
# Search for AccountEx component and route parameter validation
# Find AccountEx component
echo "Looking for AccountEx component..."
rg -l "AccountEx"
# Search for validation patterns in route components
echo -e "\nChecking route parameter validation..."
rg -A 10 "const \{ .*address.* \} = useParams"
# Look for address validation utilities
echo -e "\nChecking address validation utilities..."
rg -A 5 "validateAddress|isValidAddress|checkAddress"
Length of output: 82616
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx (4)
6-71: Consider organizing imports into logical groupsThe imports could be better organized into logical groups with separating newlines:
- React and routing imports
- Extension base imports
- Feature-specific imports (fullscreen, popup)
Consider reorganizing the imports like this:
import React, { useMemo } from 'react'; import { Route, Switch } from 'react-router'; + import { PHISHING_PAGE_REDIRECT } from '@polkadot/extension-base/defaults'; + // Fullscreen components import AccountFS from '@polkadot/extension-polkagate/src/fullscreen/accountDetails'; // ... other fullscreen imports + // Popup components import AccountEx from '@polkadot/extension-polkagate/src/popup/account'; // ... other popup imports
74-80: Add JSDoc documentation and improve type safetyThe
RouteConfiginterface would benefit from documentation and more specific types.Consider adding documentation and improving types:
+/** + * Configuration for a route in the application + */ interface RouteConfig { + /** The URL path pattern for the route */ path: string; - Component: React.ComponentType<any>; + /** The component to render for this route */ + Component: React.ComponentType<unknown>; + /** Unique identifier for analytics and error tracking */ trigger: string; + /** Additional props to pass to the component */ props?: Record<string, unknown>; + /** Whether the path should match exactly */ exact?: boolean; }
82-431: Extract path patterns into constantsCommon path patterns are repeated across routes. Consider extracting these into constants for better maintainability.
Example implementation:
const PATHS = { ACCOUNT: '/account/:address', STAKING: { POOL: '/pool/:address', SOLO: '/solo/:address' } } as const;This would make path updates easier and less error-prone.
433-456: Implementation looks good with room for enhancementThe Routes component is well-implemented with proper memoization. Consider adding error boundaries at the route level for better error isolation.
Example enhancement:
const Routes = () => { const routeComponents = useMemo(() => ALL_ROUTES.map(({ Component, exact, path, props, trigger }) => ( <Route exact={exact} key={path} path={path} > + <ErrorBoundary> <RouteWrapper component={Component} props={props} trigger={trigger} /> + </ErrorBoundary> </Route> )), []); return ( <Switch> {routeComponents} </Switch> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension-polkagate/src/components/index.ts(0 hunks)packages/extension-ui/src/Popup/components/RouteWrapper.tsx(1 hunks)packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extension-polkagate/src/components/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-ui/src/Popup/components/RouteWrapper.tsx
🔇 Additional comments (1)
packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx (1)
423-431: Verify route path uniqueness
Let's ensure there are no duplicate route paths that could cause routing conflicts.
✅ Verification successful
No routing conflicts detected - duplicate triggers are used with unique paths
The verification shows that while there are duplicate triggers, each route has a unique path:
accounttrigger: Used for/account/:genesisHash/:address/and/accountfs/:address/:paramAssetIdgovernancetrigger: Used for/governance/:address/:topMenu/:postIdand/governance/:address/:topMenumanage-validators-fullscreentrigger: Used for/manageValidators/:addressand/managePoolValidators/:address
Since the path property is unique for each route and there are no duplicate paths found, the routing configuration is valid. Triggers are typically used for navigation grouping or menu highlighting and having multiple routes share the same trigger is a common pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate route paths in RouteDefinitions.tsx
# Expected: No duplicate paths should be found
echo "Checking for duplicate route paths..."
rg -o 'path: .([^,}]*)' packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx | \
sort | uniq -d
echo "Checking for duplicate route triggers..."
rg -o 'trigger: .([^,}]*)' packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx | \
sort | uniq -d
Length of output: 465
Script:
#!/bin/bash
# Let's examine the context around these duplicate triggers to understand if they're problematic
echo "Checking context for duplicate 'account' trigger..."
rg "trigger: 'account'" -B 2 -A 2 packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx
echo -e "\nChecking context for duplicate 'governance' trigger..."
rg "trigger: 'governance'" -B 2 -A 2 packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx
echo -e "\nChecking context for duplicate 'manage-validators-fullscreen' trigger..."
rg "trigger: 'manage-validators-fullscreen'" -B 2 -A 2 packages/extension-ui/src/Popup/routes/RouteDefinitions.tsx
Length of output: 1441
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx (1)
120-120: Add fallback UI for undefined URL.Consider showing a placeholder or message when
_urlis undefined to maintain a good user experience.- {extractBaseUrl(_url)} + {extractBaseUrl(_url) || t('Unknown application')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/popup/PhishingDetected.tsx(1 hunks)packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/popup/PhishingDetected.tsx
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx (3)
Line range hint 47-59: LGTM! Consistent dependency array update.
The effect properly uses the new _url variable and includes appropriate error handling.
36-36: Verify useFavIcon hook handles undefined URLs.
The hook receives a potentially undefined URL. Let's ensure it handles this case gracefully.
✅ Verification successful
useFavIcon hook properly handles undefined URLs
The hook's implementation shows it explicitly accepts undefined or null URLs in its type signature: function useFavIcon (url: string | null | undefined): string | null | undefined. The hook is properly typed to handle the potentially undefined _url parameter, making this usage safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of useFavIcon hook
ast-grep --pattern 'export function useFavIcon($_) {
$$$
}'
Length of output: 63
Script:
#!/bin/bash
# Let's try searching for useFavIcon with ripgrep to locate its implementation
rg -l "useFavIcon"
# Then let's look at the content of files containing useFavIcon
rg "useFavIcon" -A 10
Length of output: 7183
35-35: LGTM! Consider handling undefined case explicitly.
The introduction of optional chaining is a good defensive programming practice. However, verify if requests[requestIndex] being undefined represents an error condition that should be handled explicitly.
✅ Verification successful
Optional chaining is appropriate here, no changes needed
The optional chaining is correct and well-protected because:
- The
requestIndexstate is properly managed in the parent component, ensuring it's always within bounds of the requests array - There are UI safeguards preventing access to invalid indices (disabled navigation buttons when at array bounds)
- The component has explicit checks for
requests[requestIndex]existence before rendering sensitive content - Error handling is in place for the auth request operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any error boundaries or error handling for invalid requestIndex
ast-grep --pattern 'if (!requests[requestIndex]) {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Let's check for any error handling patterns around requests or requestIndex
rg -A 5 "requests\[requestIndex\]"
# Also check for any error boundaries or error handling components
fd "ErrorBoundary|error" --type f
Length of output: 3414
Script:
#!/bin/bash
# Let's check the Request component to understand how it handles undefined requests
ast-grep --pattern 'interface RequestProps {
$$$
}'
# Also check how requestIndex is managed
rg -B 3 -A 3 "requestIndex" --type ts
Length of output: 14401
|
his helps make the code cleaner. However, in the previous Root routes in extension mode, closing the Auth or Sign request results in a blank screen. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/popup/authorize/Request.tsx (2)
65-67: Consider enhancing error handling and user feedbackThe function handles two different scenarios (new auth and existing auth) but provides no user feedback on success/failure. Consider:
- Adding toast notifications or status messages
- Different navigation paths based on the scenario
- Proper error handling instead of just console.error
-).then(() => onAction('/')) - .catch((error: Error) => console.error(error)); +).then(() => { + showToast(t('Authorization updated successfully')); + onAction('/'); +}) +.catch((error: Error) => { + console.error(error); + showToast(t('Failed to update authorization'), 'error'); +});
56-57: Implement comprehensive error handling strategyThe current error handling approach using console.error is insufficient for production. Consider implementing:
- A global error boundary for authorization flows
- User-friendly error messages
- Error recovery mechanisms
- Error tracking/logging
Consider creating a dedicated error handling service:
// services/errorHandling.ts export class AuthorizationError extends Error { constructor(message: string, public readonly code: string) { super(message); this.name = 'AuthorizationError'; } } export const handleAuthError = (error: Error): void => { if (error instanceof AuthorizationError) { // Handle specific auth errors showToast(error.message, 'error'); trackError(error); } else { // Handle unexpected errors showToast(t('Unexpected error occurred'), 'error'); trackError(error); } };Also applies to: 66-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx(4 hunks)packages/extension-polkagate/src/popup/authorize/Request.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx
🔇 Additional comments (1)
packages/extension-polkagate/src/popup/authorize/Request.tsx (1)
56-57: Verify navigation behavior across authorization flows
The explicit navigation to '/' might be related to the blank screen issue mentioned in PR comments. We should verify that this navigation pattern is consistent with other authorization components.
✅ Verification successful
Based on the search results, I can see that navigating to '/' after authorization actions is a consistent pattern across the codebase. Specifically:
-
In authorization-related components:
AuthFullScreenMode.tsxnavigates to '/' after both approve and rejectManageAuthorizedDapps.tsxnavigates to '/' after actions- Other auth management components follow the same pattern
-
In similar flows:
- Password management
- Account management
- Various review components
The change in Request.tsx to navigate to '/' after authorization actions aligns with the established pattern in the codebase, particularly for authorization flows. This is the expected behavior to return users to the home screen after completing authorization actions.
Navigation pattern is consistent with codebase standards
The explicit navigation to '/' in the authorization request handler follows the established pattern across the codebase for auth flows and helps prevent blank screens by ensuring a consistent return to home screen.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar navigation patterns in authorization-related components
rg -A 2 "onAction\(" --glob "packages/extension-polkagate/src/**/*.{tsx,ts}"
Length of output: 37447
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/popup/signing/index.tsx (1)
57-59: Consider improving error handling in the cancellation flow.The current implementation only logs cancellation errors to the console. Consider:
- Showing a user-friendly error message using the existing
setError- Handling navigation failures
cancelSignRequest(request.id) - .then(() => onAction('/')) - .catch((error: Error) => console.error(error)); + .then(() => onAction('/')) + .catch((error: Error) => { + console.error(error); + setError(t('Failed to cancel the request. Please try again.')); + });packages/extension-polkagate/src/popup/signing/Request/SignArea.tsx (1)
Line range hint
61-66: Consider adding error handling for navigationThe navigation after successful signing lacks error handling. If the routing fails, it could lead to UI inconsistencies.
approveSignPassword(signId, savePass, password) .then((): void => { setIsBusy(false); - onAction('/'); + try { + onAction('/'); + } catch (e) { + console.error('Navigation failed:', e); + setError(t('Failed to navigate after signing')); + } }) .catch((error: Error): void => {packages/extension-polkagate/src/popup/metadata/Request.tsx (1)
28-43: Consider implementing a navigation serviceGiven that this is part of a larger routing refactor, consider extracting navigation logic into a dedicated service. This would:
- Centralize navigation rules
- Make it easier to modify navigation behavior consistently
- Help prevent issues like blank screens by enforcing valid navigation states
Example approach:
// navigation.service.ts export class NavigationService { afterMetadataApproval(): string { return this.shouldReturnToRoot() ? '/' : '/previous-route'; } private shouldReturnToRoot(): boolean { // Add logic to determine correct navigation return true; } }packages/extension-polkagate/src/popup/signing/Request/index.tsx (2)
104-104: Consider enhancing error handling and type safetyWhile the routing change maintains consistency with
_onSignature, consider these improvements:
- Add specific error types/codes for better error handling
- Consider adding a loading state during signature approval
- Add type guards for the extrinsic payload
Example implementation:
interface SignatureError extends Error { code?: string; type?: 'signature' | 'extrinsic' | 'routing'; } // Usage in catch block .catch((error: SignatureError): void => { const errorMessage = error.code ? t(`errors.${error.code}`) : error.message; setError(errorMessage); console.error(`Signature error (${error.type}):`, error); });
Line range hint
1-24: Consider architectural improvements for better maintainabilityThe component handles multiple signature types and uses a global TypeRegistry. Consider these architectural improvements:
- Split into smaller components per signature type
- Move TypeRegistry to a context or service
- Add proper cleanup for registry modifications
Example structure:
// SignatureContext.tsx export const SignatureContext = React.createContext<TypeRegistry>(null); export function SignatureProvider({ children }) { const registry = useMemo(() => new TypeRegistry(), []); useEffect(() => { return () => { // Cleanup registry modifications registry.setSignedExtensions([]); }; }, [registry]); return ( <SignatureContext.Provider value={registry}> {children} </SignatureContext.Provider> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/extension-polkagate/src/popup/metadata/Request.tsx(2 hunks)packages/extension-polkagate/src/popup/metadata/index.tsx(2 hunks)packages/extension-polkagate/src/popup/signing/Request/SignArea.tsx(1 hunks)packages/extension-polkagate/src/popup/signing/Request/index.tsx(2 hunks)packages/extension-polkagate/src/popup/signing/index.tsx(1 hunks)
🔇 Additional comments (8)
packages/extension-polkagate/src/popup/metadata/index.tsx (2)
29-29: LGTM! Clean translation usage
The simplified translation call without the generic type parameter makes the code cleaner while maintaining the same functionality.
19-19: Verify navigation behavior after metadata rejection
The addition of explicit route '/' aligns with the routing refactor, but given the reported blank screen issues when closing requests, we should verify this change doesn't contribute to similar problems.
Let's check for consistent navigation handling across similar components:
✅ Verification successful
Based on the search results, I can now generate the final response as I have enough information to verify the navigation behavior:
Navigation to root route '/' is consistent across request rejections
The search results show that navigating to the root route '/' after rejecting requests is a consistent pattern across the codebase:
- In signing components:
packages/extension-polkagate/src/popup/signing/index.tsxuses.then(() => onAction('/')) - In authorization components:
packages/extension-polkagate/src/popup/authorize/Request.tsxuses.then(() => onAction('/')) - In metadata components: Both
index.tsxandRequest.tsxuse.then(() => onAction('/'))
This confirms that the navigation change aligns with the established pattern for handling request rejections across the extension.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check navigation patterns in rejection handlers across components
# Looking for inconsistencies in route handling that might cause blank screens
# Search for onAction calls in rejection handlers
rg -A 3 "onAction\(" packages/extension-polkagate/src/popup/
# Search for similar rejection patterns
ast-grep --pattern 'rejectMetaRequest($_).then(() => $_)'
Length of output: 33571
packages/extension-polkagate/src/popup/signing/index.tsx (1)
58-58: Verify if this fixes the blank screen issue.
The change to navigate explicitly to '/' might be related to the blank screen issue mentioned in PR comments. Let's verify the routing behavior.
✅ Verification successful
Navigation to root path '/' is consistently used across the codebase for cancellation flows
The verification shows that navigating to '/' after cancellation is a consistent pattern used throughout the codebase:
- All cancel/close actions in signing flows (
packages/extension-polkagate/src/popup/signing/) navigate to '/' - Similar navigation pattern is used in related flows like authorization (
/authorize), metadata (/metadata), and other UI components - The routing behavior in
_onCancelmatches the established pattern in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar routing patterns and potential issues
# Look for other onAction calls to verify consistency
rg "onAction\(['\"]/" -A 2
# Check for other cancel handlers that might need similar updates
ast-grep --pattern 'cancelSignRequest($_).then($_)'
Length of output: 18652
packages/extension-polkagate/src/popup/signing/Request/SignArea.tsx (1)
62-62: Verify navigation behavior after signing
The change to onAction('/') might be related to the reported blank screen issue when closing Auth/Sign requests. Let's verify the routing implementation.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Navigation to root path '/' is consistent with signing flow
The change to onAction('/') is verified to be correct as:
- All signing-related components consistently navigate to root path '/' after successful signing:
packages/extension-polkagate/src/popup/signing/index.tsxpackages/extension-polkagate/src/popup/signing/Request/index.tsxpackages/extension-polkagate/src/popup/signing/Request/SignArea.tsx
- The
onActionis properly obtained fromActionContextand used consistently across components - This navigation pattern aligns with the routing refactor mentioned in the PR objectives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other onAction calls in signing-related components
echo "Checking related signing components for consistent navigation:"
rg -A 2 "onAction\(['\"]/" packages/extension-polkagate/src/popup/signing/
# Check if ROOT_PATH constant is used instead of hardcoded '/'
echo "Checking for ROOT_PATH constant usage:"
rg "ROOT_PATH" packages/extension-polkagate/src/
Length of output: 1534
Script:
#!/bin/bash
# Let's check the routing implementation and navigation patterns
echo "Checking routing implementation:"
rg -A 3 "const\s+routes?\s*=|createMemoryRouter|createBrowserRouter|BrowserRouter" packages/extension-polkagate/src/
# Check navigation patterns in the codebase
echo "Checking navigation patterns:"
rg -A 2 "onAction\s*=\s*{" packages/extension-polkagate/src/
# Check if there are any other navigation paths used
echo "Checking navigation paths:"
rg "navigate\(['\"]|to=['\"]" packages/extension-polkagate/src/
Length of output: 554
Script:
#!/bin/bash
# Let's check the routing implementation with fixed regex
echo "Checking routing implementation:"
rg -A 3 "createRouter|Router\s+" packages/extension-polkagate/src/
# Check navigation patterns with fixed regex
echo "Checking onAction usage:"
rg -A 2 "onAction.*=" packages/extension-polkagate/src/
# Check the component props and types
echo "Checking component props and types:"
ast-grep --pattern 'interface $_ {
onAction: $$$
}'
Length of output: 26796
packages/extension-polkagate/src/popup/metadata/Request.tsx (3)
37-43: LGTM: Consistent navigation handling
The rejection flow now matches the approval flow, providing consistent navigation behavior in both cases.
114-115: LGTM: Props updated to match callback names
The button handlers have been correctly updated to use the new callback names, maintaining consistency throughout the component.
28-34: Verify navigation behavior after metadata approval
The addition of onAction('/') might be related to the blank screen issue mentioned in the PR comments. Please verify that navigating to root is the intended behavior after metadata approval, as it might affect the user experience in extension mode.
✅ Verification successful
Navigation to root after metadata operations is consistent with codebase patterns
The navigation behavior using onAction('/') after metadata approval/rejection is verified to be the correct approach based on the following evidence:
- The same navigation pattern is consistently used across the codebase in similar scenarios:
- In the parent component
metadata/index.tsxfor handling metadata rejections - In signing operations (
signing/Request/index.tsx,signing/index.tsx) - In authorization flows (
authorize/Request.tsx,authorize/AuthFullScreenMode.tsx) - Across various other user actions that complete a workflow (staking, crowdloans, account management)
- In the parent component
The navigation to root after completing metadata operations aligns with the established patterns in the codebase and helps address the reported blank screen issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar navigation patterns in other request handlers
rg -A 3 "approveMetaRequest|rejectMetaRequest" --type ts --type tsx
# Look for other components using onAction with root path
rg "onAction\(['\"]\/['\"]\)" --type ts --type tsx
Length of output: 257
Script:
#!/bin/bash
# Check for similar navigation patterns in other request handlers
rg -A 3 "approveMetaRequest|rejectMetaRequest"
# Look for other components using onAction with root path
rg "onAction.*'/'"
# Check for other navigation patterns after request handling
rg -A 3 "then\s*\(\s*\)\s*=>\s*onAction"
Length of output: 7977
packages/extension-polkagate/src/popup/signing/Request/index.tsx (1)
80-80: Verify routing behavior after signature completion
The change to redirect to root path '/' might be related to the reported blank screen issue when closing Auth/Sign requests. Please ensure this routing change:
- Properly closes the signature request
- Maintains expected navigation flow
- Doesn't cause the blank screen issue mentioned in PR comments
Let's check for similar routing patterns in other signature-related components:
✅ Verification successful
Routing to root path is a consistent pattern across the codebase
The search results show that redirecting to the root path '/' after completing an action is a common and established pattern throughout the codebase, particularly in:
- Other signature-related components (
packages/extension-polkagate/src/popup/signing/index.tsx,packages/extension-polkagate/src/popup/signing/Request/SignArea.tsx) - Authorization flows (
packages/extension-polkagate/src/popup/authorize/Request.tsx,packages/extension-polkagate/src/popup/authorize/AuthFullScreenMode.tsx) - Metadata handling (
packages/extension-polkagate/src/popup/metadata/Request.tsx)
The change aligns with the codebase's routing conventions and is consistent with how other components handle completion of their respective operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar routing patterns in signature-related components
rg -A 3 "onAction\(['\"]/" packages/extension-polkagate/src/popup/
Length of output: 18053
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/extension-ui/src/Popup/index.tsx (1)
276-276: Consider extracting provider compositionThe deeply nested context providers could impact maintainability and performance. Consider extracting the provider composition to a separate component.
Create a new component to manage context providers:
// ContextProviders.tsx export const ContextProviders: React.FC<{ children: React.ReactNode }> = ({ children }) => ( <ActionContext.Provider value={_onAction}> <SettingsContext.Provider value={settingsCtx}> {/* ... other providers ... */} {children} </SettingsContext.Provider> </ActionContext.Provider> ); // Usage in Popup component <ContextProviders> <Routes /> </ContextProviders>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-ui/src/Popup/index.tsx(4 hunks)
🔇 Additional comments (1)
packages/extension-ui/src/Popup/index.tsx (1)
12-12: Verify routing changes to prevent blank screen issues
The centralization of routing logic through the new Routes component aligns with the refactoring objective. However, this change needs careful verification due to reported blank screen issues when closing Auth/Sign requests.
Also applies to: 28-28
close: #1672
Summary by CodeRabbit
Release Notes
New Features
RouteWrappercomponent for improved routing.Improvements
ErrorBoundarycomponent with context-aware rendering.Popupcomponent for streamlined navigation.AuthFullScreenModeby safely accessing request URLs.useFullscreenhook for better timing of style changes.Metadatacomponent.Version Updates
0.33.0.