chore: refactor chain select modals#2131
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces local chain-selection UIs with a shared Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReceiveModal as Receive Modal
participant Hook as useGenesisHashOptions
participant Cache as metadataCacheByType
participant ChainPicker as ChainPickerList
User->>ReceiveModal: open chain selector
ReceiveModal->>Hook: request options ({ isEthereum, withRelay })
Hook->>Cache: load or reuse metadataChains for chainType
Cache-->>Hook: metadataChains
Hook-->>ReceiveModal: return DropdownOption[] (sorted/reordered)
ReceiveModal->>ChainPicker: render(options)
User->>ChainPicker: search / select option
ChainPicker-->>ReceiveModal: onSelect(option)
ReceiveModal-->>User: show receive/QR/share for selected chain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Nick-1979
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/extension-polkagate/src/fullscreen/components/ChainPickerList.tsx (1)
86-115: Consider usingchain.valueas the key instead ofindex.Using
indexas a React key can cause issues when items are reordered or filtered, potentially leading to incorrect state retention or rendering artifacts. Sincechain.value(genesisHash) should be unique, it would be a more stable key.♻️ Suggested change
- <React.Fragment key={index}> + <React.Fragment key={chain.value}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-polkagate/src/fullscreen/components/ChainPickerList.tsx` around lines 86 - 115, The list is using index as the React key which can break identity when items reorder; update the Fragment key to use the stable unique identifier chain.value (genesisHash) instead of index in the ChainPickerList rendering (the React.Fragment that currently has key={index}); ensure any sibling elements like GradientDivider don't also rely on index for identity so the list items keep correct state when filtered or reordered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-polkagate/src/fullscreen/components/ChainListModal.tsx`:
- Around line 85-90: In the map that builds options (the return of _options.map
inside ChainListModal.tsx), avoid mutating the original DropdownOption objects
by returning a new object instead of assigning to o.value; replace the in-place
mutation with creating a shallow copy (e.g., spread the original option and set
value: String(o.value)) so callers of allChains/externalOptions are not affected
and React references remain stable.
In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts`:
- Around line 88-93: The map transformation for assetHubFirstChains can leave a
trailing space when removing ASSET_HUB (e.g., "Polkadot Asset Hub" -> "Polkadot
"), so update the mapping in useGenesisHashOptions.ts to trim the replaced
string; in the map callback that builds the text (the code using
assetHubFirstChains.map and ASSET_HUB) ensure you call trim() on the result of
replace (or trim the final text value) so any leftover whitespace is removed.
---
Nitpick comments:
In `@packages/extension-polkagate/src/fullscreen/components/ChainPickerList.tsx`:
- Around line 86-115: The list is using index as the React key which can break
identity when items reorder; update the Fragment key to use the stable unique
identifier chain.value (genesisHash) instead of index in the ChainPickerList
rendering (the React.Fragment that currently has key={index}); ensure any
sibling elements like GradientDivider don't also rely on index for identity so
the list items keep correct state when filtered or reordered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6fad7ec-b73b-440f-9c27-0409fd2fca41
📒 Files selected for processing (15)
packages/extension-polkagate/src/fullscreen/accountDetails/rightColumn/Receive.tsxpackages/extension-polkagate/src/fullscreen/components/AccountChainSelect.tsxpackages/extension-polkagate/src/fullscreen/components/ChainListModal.tsxpackages/extension-polkagate/src/fullscreen/components/ChainPickerList.tsxpackages/extension-polkagate/src/fullscreen/notification/NotificationSettingsFS.tsxpackages/extension-polkagate/src/fullscreen/stake/stakingPositions/PositionItem.tsxpackages/extension-polkagate/src/fullscreen/stake/stakingPositions/index.tsxpackages/extension-polkagate/src/hooks/useChainSelectionSettings.tspackages/extension-polkagate/src/hooks/useGenesisHashOptions.tspackages/extension-polkagate/src/hooks/useNotifications.tspackages/extension-polkagate/src/popup/receive/Receive.tsxpackages/extension-polkagate/src/popup/staking/EarningOptions.tsxpackages/extension-polkagate/src/popup/staking/StakingPositions.tsxpackages/extension-ui/src/Popup/contexts/GenesisHashOptionsProvider.tsxpackages/extension/public/locales/en/translation.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts`:
- Around line 22-41: The getMetadataChains function currently stores a rejected
promise in metadataRequestByType when getAllMetadata() fails, preventing
retries; update the logic where metadataRequestByType[chainType] is assigned
(the branch that calls getAllMetadata()) to attach a .catch (or try/catch around
an async/await) that clears metadataRequestByType[chainType] on error (and
optionally set metadataCacheByType[chainType] = []), then rethrow or propagate
the error so callers can handle it; reference metadataRequestByType,
metadataCacheByType, getAllMetadata, and getMetadataChains when making the
change.
- Around line 60-66: protocolFilteredChains is not applying the same protocol
filter as metadataChains when isEthereum is false, causing Ethereum static
chains to mix with substrate-only metadata; update the protocolFilteredChains
calculation (which uses isEthereum and testnetFilteredChains) so that when
isEthereum is false it filters testnetFilteredChains by !chain.isEthereum
(matching metadataChains filtering), ensuring protocol consistency between
static chains (chains/testnetFilteredChains) and metadata-only chains
(metadataChains/metadataOnlyChains).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82879eb6-55e3-40cc-a8ec-de794f0c89ac
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/components/ChainListModal.tsxpackages/extension-polkagate/src/hooks/useGenesisHashOptions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/fullscreen/components/ChainListModal.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts (1)
66-69:⚠️ Potential issue | 🔴 CriticalProtocol filtering inconsistency for substrate mode remains unaddressed.
When
isEthereumisfalse,protocolFilteredChainsincludes all static chains (both Ethereum and substrate), butmetadataChainsis filtered to only'substrate'type on line 54. This creates an inconsistency where Ethereum chains from the static list are mixed with substrate-only chains from metadata.Apply the same filtering to static chains:
const protocolFilteredChains = isEthereum ? testnetFilteredChains.filter((chain) => chain.isEthereum) - : testnetFilteredChains; + : testnetFilteredChains.filter((chain) => !chain.isEthereum);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts` around lines 66 - 69, protocolFilteredChains currently only filters for Ethereum when isEthereum is true, leaving static chains unfiltered for substrate mode; update the else branch to mirror the metadataChains substrate filtering so static chains are limited to substrate-only. Modify the assignment of protocolFilteredChains (the ternary using isEthereum and testnetFilteredChains) so the false branch applies the same substrate filter used by metadataChains (e.g., testnetFilteredChains.filter(chain => chain.type === 'substrate' or chain.isSubstrate)), ensuring protocolFilteredChains and metadataChains remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts`:
- Around line 66-69: protocolFilteredChains currently only filters for Ethereum
when isEthereum is true, leaving static chains unfiltered for substrate mode;
update the else branch to mirror the metadataChains substrate filtering so
static chains are limited to substrate-only. Modify the assignment of
protocolFilteredChains (the ternary using isEthereum and testnetFilteredChains)
so the false branch applies the same substrate filter used by metadataChains
(e.g., testnetFilteredChains.filter(chain => chain.type === 'substrate' or
chain.isSubstrate)), ensuring protocolFilteredChains and metadataChains remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b162d86-284c-4fd6-9b25-0c3d71db236e
📒 Files selected for processing (1)
packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts (1)
67-70:⚠️ Potential issue | 🔴 CriticalFilter the substrate branch the same way as metadata.
Line 70 still falls back to the full static list, so
useGenesisHashOptions({})mixes Ethereum chains with substrate-only metadata.packages/extension-polkagate/src/hooks/useChainSelectionSettings.tsalready uses the default{}path, so this regression is live outside the modal refactor too.Suggested fix
const protocolFilteredChains = isEthereum ? testnetFilteredChains.filter((chain) => chain.isEthereum) - : testnetFilteredChains; + : testnetFilteredChains.filter((chain) => !chain.isEthereum);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts` around lines 67 - 70, The protocolFilteredChains assignment in useGenesisHashOptions.ts lets the substrate branch fall back to the full static list and thus mixes Ethereum chains into substrate-only options; update the else branch so that when isEthereum is false you also filter testnetFilteredChains to exclude Ethereum chains (e.g., filter by chain.isEthereum === false or !chain.isEthereum). Locate the symbol protocolFilteredChains and the variables isEthereum and testnetFilteredChains and apply the same filtering logic used for the Ethereum branch to the substrate branch so substrate-only metadata is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts`:
- Around line 51-58: When isEthereum changes the effect should clear and guard
metadataChains to avoid stale overwrites: inside the useEffect that calls
getMetadataChains(chainType) (the effect using isEthereum, getMetadataChains,
metadataChains and setMetadataChains), immediately call setMetadataChains([])
when the protocol flips and add a local cancelled flag (e.g., let cancelled =
false) and check it before calling setMetadataChains in the promise resolution;
also set cancelled = true in the effect cleanup to ignore any late-resolving
promises from previous requests so old results cannot overwrite the new state.
---
Duplicate comments:
In `@packages/extension-polkagate/src/hooks/useGenesisHashOptions.ts`:
- Around line 67-70: The protocolFilteredChains assignment in
useGenesisHashOptions.ts lets the substrate branch fall back to the full static
list and thus mixes Ethereum chains into substrate-only options; update the else
branch so that when isEthereum is false you also filter testnetFilteredChains to
exclude Ethereum chains (e.g., filter by chain.isEthereum === false or
!chain.isEthereum). Locate the symbol protocolFilteredChains and the variables
isEthereum and testnetFilteredChains and apply the same filtering logic used for
the Ethereum branch to the substrate branch so substrate-only metadata is
returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ae6e26f-2c6d-447c-8c6b-b72af0edd1e0
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/components/layout/Bread.tsxpackages/extension-polkagate/src/hooks/useGenesisHashOptions.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Documentation