Skip to content

Commit 9f5539e

Browse files
author
CsB-Polymesh
committed
refactor: implement 9 Copilot review suggestions
- Optimize deduplicateAssetsByID O(n²) → O(n) using Set - Add input validation to convertKeyDataToFormData - Fix misleading error message in permissions handler - Add case-insensitive ticker validation - Add semantic labels to search input and checkboxes - Rename variable for clarity (allSelectedTxTags → allSelectedTxTagsSet) All changes follow the webapp's accessibility pattern using <label htmlFor> instead of aria-label attributes for better semantic HTML consistency.
1 parent b600ea8 commit 9f5539e

5 files changed

Lines changed: 84 additions & 31 deletions

File tree

src/layouts/SecondaryKeys/components/AddPermission/components/AssetPermissionSelector.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,13 +331,14 @@ export const AssetPermissionSelector = ({
331331
}
332332

333333
// Check if it could be a ticker (12 characters or less, uppercase alphanumeric - Polymesh ticker format)
334+
const uppercasedValue = trimmedValue.toUpperCase();
334335
const isTickerFormat =
335-
trimmedValue.length <= 12 && /^[A-Z0-9]+$/.test(trimmedValue);
336+
uppercasedValue.length <= 12 && /^[A-Z0-9]+$/.test(uppercasedValue);
336337

337338
if (isTickerFormat) {
338339
// Try to fetch asset by ticker
339340
try {
340-
const asset = await fetchAsset(trimmedValue);
341+
const asset = await fetchAsset(uppercasedValue);
341342
if (asset) {
342343
return { error: '', assetId: asset.id };
343344
}

src/layouts/SecondaryKeys/components/AddPermission/components/ExtrinsicPermissionSelector.tsx

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ const GroupCheckbox = styled.input.attrs({ type: 'checkbox' })`
9292
}
9393
`;
9494

95-
const GroupLabel = styled.div`
95+
const GroupLabel = styled.label`
9696
font-weight: 500;
9797
font-size: 14px;
9898
color: ${({ theme }) => theme.colors.textPrimary};
@@ -145,6 +145,13 @@ const TransactionLabel = styled.span`
145145
word-break: break-word;
146146
`;
147147

148+
const TransactionCheckboxLabel = styled.label`
149+
display: flex;
150+
align-items: center;
151+
width: 100%;
152+
cursor: pointer;
153+
`;
154+
148155
export const ExtrinsicPermissionSelector = ({
149156
selectedExtrinsics,
150157
onChange,
@@ -189,18 +196,18 @@ export const ExtrinsicPermissionSelector = ({
189196
const emitSelection = useCallback(
190197
(updated: Record<TxGroup, Set<string>>) => {
191198
// Get all selected transactions across all groups
192-
const allSelectedTxTags = new Set<string>();
199+
const allSelectedTxTagsSet = new Set<string>();
193200

194201
ALL_TX_GROUPS.forEach((group) => {
195202
updated[group].forEach((tx) => {
196-
allSelectedTxTags.add(tx);
203+
allSelectedTxTagsSet.add(tx);
197204
});
198205
});
199206

200207
// Group transactions by pallet
201208
const palletMap = new Map<string, Set<string>>();
202209

203-
allSelectedTxTags.forEach((txTag) => {
210+
allSelectedTxTagsSet.forEach((txTag) => {
204211
const [pallet, extrinsic] = txTag.split('.');
205212
if (!palletMap.has(pallet)) {
206213
palletMap.set(pallet, new Set());
@@ -306,6 +313,7 @@ export const ExtrinsicPermissionSelector = ({
306313
<GroupHeader>
307314
<GroupHeaderLeft>
308315
<GroupCheckbox
316+
id={`group-${group}`}
309317
checked={checked}
310318
ref={(checkboxInput: HTMLInputElement | null) => {
311319
if (checkboxInput) {
@@ -317,7 +325,9 @@ export const ExtrinsicPermissionSelector = ({
317325
}}
318326
onChange={() => toggleGroup(group)}
319327
/>
320-
<GroupLabel>{TX_GROUP_LABELS[group]}</GroupLabel>
328+
<GroupLabel htmlFor={`group-${group}`}>
329+
{TX_GROUP_LABELS[group]}
330+
</GroupLabel>
321331
</GroupHeaderLeft>
322332

323333
<ExpandButton
@@ -338,11 +348,18 @@ export const ExtrinsicPermissionSelector = ({
338348

339349
return (
340350
<TransactionItem key={`${group}-${transaction}`}>
341-
<TransactionCheckbox
342-
checked={isTransactionSelected || false}
343-
onChange={() => toggleTransaction(group, transaction)}
344-
/>
345-
<TransactionLabel>{transaction}</TransactionLabel>
351+
<TransactionCheckboxLabel
352+
htmlFor={`tx-${group}-${transaction}`}
353+
>
354+
<TransactionCheckbox
355+
id={`tx-${group}-${transaction}`}
356+
checked={isTransactionSelected || false}
357+
onChange={() =>
358+
toggleTransaction(group, transaction)
359+
}
360+
/>
361+
<TransactionLabel>{transaction}</TransactionLabel>
362+
</TransactionCheckboxLabel>
346363
</TransactionItem>
347364
);
348365
})}

src/layouts/SecondaryKeys/components/AddPermission/components/PortfolioPermissionSelector.tsx

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ const DropdownMenu = styled.div<{ $isOpen: boolean }>`
4949
z-index: 1000;
5050
`;
5151

52+
const SearchInputLabel = styled.label`
53+
display: block;
54+
padding: 8px 16px 4px 16px;
55+
font-size: 12px;
56+
font-weight: 500;
57+
color: ${({ theme }) => theme.colors.textSecondary};
58+
`;
59+
5260
const SearchInput = styled.input`
5361
width: 100%;
5462
padding: 12px 16px;
@@ -68,6 +76,13 @@ const SearchInput = styled.input`
6876
}
6977
`;
7078

79+
const PortfolioCheckboxLabel = styled.label`
80+
display: flex;
81+
align-items: center;
82+
width: 100%;
83+
cursor: pointer;
84+
`;
85+
7186
const PortfolioOption = styled.div<{ $isSelected: boolean }>`
7287
display: flex;
7388
align-items: center;
@@ -195,7 +210,11 @@ export const PortfolioPermissionSelector = ({
195210
</SelectedPortfoliosDisplay>
196211

197212
<DropdownMenu $isOpen={isOpen}>
213+
<SearchInputLabel htmlFor="portfolioSearch">
214+
Search Portfolios
215+
</SearchInputLabel>
198216
<SearchInput
217+
id="portfolioSearch"
199218
type="text"
200219
placeholder="Search by ID or name..."
201220
value={searchTerm}
@@ -212,21 +231,24 @@ export const PortfolioPermissionSelector = ({
212231
$isSelected={isPortfolioSelected(portfolio.id)}
213232
onClick={() => handleTogglePortfolio(portfolio)}
214233
>
215-
<Checkbox
216-
type="checkbox"
217-
checked={isPortfolioSelected(portfolio.id)}
218-
readOnly
219-
/>
220-
<PortfolioInfo>
221-
<PortfolioId>
222-
{portfolio.id === 'default'
223-
? 'Default Portfolio'
224-
: `Portfolio ${portfolio.id}`}
225-
</PortfolioId>
226-
{portfolio.name && portfolio.id !== 'default' && (
227-
<PortfolioName>{portfolio.name}</PortfolioName>
228-
)}
229-
</PortfolioInfo>
234+
<PortfolioCheckboxLabel htmlFor={`portfolio-${portfolio.id}`}>
235+
<Checkbox
236+
id={`portfolio-${portfolio.id}`}
237+
type="checkbox"
238+
checked={isPortfolioSelected(portfolio.id)}
239+
readOnly
240+
/>
241+
<PortfolioInfo>
242+
<PortfolioId>
243+
{portfolio.id === 'default'
244+
? 'Default Portfolio'
245+
: `Portfolio ${portfolio.id}`}
246+
</PortfolioId>
247+
{portfolio.name && (
248+
<PortfolioName>{portfolio.name}</PortfolioName>
249+
)}
250+
</PortfolioInfo>
251+
</PortfolioCheckboxLabel>
230252
</PortfolioOption>
231253
))
232254
)}

src/layouts/SecondaryKeys/index.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ const SecondaryKeys = () => {
167167
if (process.env.NODE_ENV === 'development') {
168168
// Log error in development to aid debugging while keeping production behavior unchanged
169169
// eslint-disable-next-line no-console
170-
console.error('Failed to remove secondary key permissions', error);
170+
console.error('Failed to modify secondary key permissions', error);
171171
}
172172
}
173173
},

src/layouts/SecondaryKeys/utils.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,15 @@ export const deduplicateAssetsByID = (
2222
): AssetDetails[] => {
2323
const combined = [...ownedAssets, ...managedAssets];
2424
// Remove duplicates by ID, keeping first occurrence
25-
return combined.filter(
26-
(asset, index, self) => index === self.findIndex((a) => a.id === asset.id),
27-
);
25+
const seenIds = new Set<string>();
26+
const deduplicated: AssetDetails[] = [];
27+
combined.forEach((asset) => {
28+
if (!seenIds.has(asset.id)) {
29+
seenIds.add(asset.id);
30+
deduplicated.push(asset);
31+
}
32+
});
33+
return deduplicated;
2834
};
2935

3036
/**
@@ -294,6 +300,13 @@ const isPortfolioValueObject = (
294300
export const convertKeyDataToFormData = (
295301
key: ISecondaryKeyData,
296302
): IPermissionFormData => {
303+
// Validate input
304+
if (!key || !key.address || !key.permissions) {
305+
throw new Error(
306+
'Invalid secondary key data: missing address or permissions',
307+
);
308+
}
309+
297310
// Handle portfolio values - convert from string array to object array if needed
298311
let portfolioValues: Array<{ id: string; name?: string }> = [];
299312

0 commit comments

Comments
 (0)