Skip to content

fix: search ref problem #1602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 23, 2024
Merged

fix: search ref problem #1602

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 21 additions & 32 deletions packages/extension-polkagate/src/fullscreen/governance/SearchBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
};
}), []);

const applySearchFilter = useCallback((referendaToFilter: LatestReferenda[], keyword: string) => {
const filtered = referendaToFilter.filter((r) =>
(filter.advanced.refIndex && String(r.post_id) === keyword) ||
(filter.advanced.titles && r.title && r.title.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.methods && r.method && r.method.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.proposers && r.proposer === keyword)
);

return filtered;
}, [filter.advanced.methods, filter.advanced.proposers, filter.advanced.refIndex, filter.advanced.titles]);
Comment on lines +63 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve search filter robustness

The filtering logic could be made more robust with the following improvements:

  1. Convert ref_id to string for comparison to handle potential type mismatches
  2. Consider case-insensitive comparison for proposer addresses
 const applySearchFilter = useCallback((referendaToFilter: LatestReferenda[], keyword: string) => {
   const filtered = referendaToFilter.filter((r) =>
-    (filter.advanced.refIndex && String(r.post_id) === keyword) ||
+    (filter.advanced.refIndex && String(r.post_id) === String(keyword)) ||
     (filter.advanced.titles && r.title && r.title.toLowerCase().includes(keyword.toLowerCase())) ||
     (filter.advanced.methods && r.method && r.method.toLowerCase().includes(keyword.toLowerCase())) ||
-    (filter.advanced.proposers && r.proposer === keyword)
+    (filter.advanced.proposers && r.proposer.toLowerCase() === keyword.toLowerCase())
   );

   return filtered;
 }, [filter.advanced.methods, filter.advanced.proposers, filter.advanced.refIndex, filter.advanced.titles]);
📝 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.

Suggested change
const applySearchFilter = useCallback((referendaToFilter: LatestReferenda[], keyword: string) => {
const filtered = referendaToFilter.filter((r) =>
(filter.advanced.refIndex && String(r.post_id) === keyword) ||
(filter.advanced.titles && r.title && r.title.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.methods && r.method && r.method.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.proposers && r.proposer === keyword)
);
return filtered;
}, [filter.advanced.methods, filter.advanced.proposers, filter.advanced.refIndex, filter.advanced.titles]);
const applySearchFilter = useCallback((referendaToFilter: LatestReferenda[], keyword: string) => {
const filtered = referendaToFilter.filter((r) =>
(filter.advanced.refIndex && String(r.post_id) === String(keyword)) ||
(filter.advanced.titles && r.title && r.title.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.methods && r.method && r.method.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.proposers && r.proposer.toLowerCase() === keyword.toLowerCase())
);
return filtered;
}, [filter.advanced.methods, filter.advanced.proposers, filter.advanced.refIndex, filter.advanced.titles]);


const onAdvanced = useCallback(() => {
setShowAdvanced(!showAdvanced);
}, [showAdvanced]);
Expand All @@ -79,27 +90,8 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
}, []);

const onSearch = useCallback((keyword: string) => {
if (!referenda) {
return;
}

if (!keyword) {
return setFilteredReferenda([...referenda]);
}

keyword = keyword.trim();

setSearchKeyword(keyword);

const _filtered = referenda?.filter((r) =>
(filter.advanced.refIndex && String(r.post_id) === keyword) ||
(filter.advanced.titles && r.title && r.title.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.methods && r.method && r.method.toLowerCase().includes(keyword.toLowerCase())) ||
(filter.advanced.proposers && r.proposer === keyword)
);

setFilteredReferenda([..._filtered]);
}, [filter, referenda, setFilteredReferenda]);
}, []);

const onMyReferenda = useCallback(() => {
filter.myReferenda = !filter.myReferenda;
Expand Down Expand Up @@ -130,20 +122,17 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
filtered = filtered.filter((r) => filter.status.includes(r.status));
}

if (searchKeyword) {
filtered = applySearchFilter(filtered, searchKeyword);
}

Comment on lines +125 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider optimizing Set conversion

The current implementation correctly handles search filtering, but the Set conversion for removing duplicates could be optimized.

-    // to remove duplicates
-    const uniqueFiltered = [...new Set(filtered)];
+    // to remove duplicates using post_id as unique identifier
+    const uniqueFiltered = Array.from(
+      new Map(filtered.map(item => [item.post_id, item])).values()
+    );

This approach is more efficient as it:

  1. Uses a specific unique identifier (post_id)
  2. Avoids potential reference comparison issues

Also applies to: 132-132

// to remove duplicates
const uniqueFiltered = [...new Set(filtered)];

const isAnyFilterOn = filter.myReferenda || filter.myVotes || !filter.status.includes('All');
const isAnyFilterOn = filter.myReferenda || filter.myVotes || !filter.status.includes('All') || searchKeyword;

setFilteredReferenda(isAnyFilterOn ? uniqueFiltered : referenda);
}, [filter, formatted, myVotedReferendaIndexes, referenda, setFilteredReferenda]);

useEffect(() => {
/** To re-apply search keyword on referenda loadings */
if (searchKeyword) {
onSearch(searchKeyword);
}
}, [onSearch, referenda, searchKeyword]);
}, [applySearchFilter, filter, formatted, myVotedReferendaIndexes, referenda, searchKeyword, setFilteredReferenda]);

const onChangeStatus = useCallback((s: number) => {
s = String(s) === 'All' ? 0 : s;
Expand Down Expand Up @@ -185,7 +174,7 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
}
</Grid>
</Grid>
<Grid alignItems='center' container item justifyContent='flex-start' py='10px' sx={{ color: theme.palette.mode === 'light' ? 'secondary.main' : 'text.primary', cursor: 'pointer', textDecorationLine: 'underline', width: 'fit-content' }} >
<Grid alignItems='center' container item justifyContent='flex-start' py='10px' sx={{ color: theme.palette.mode === 'light' ? 'secondary.main' : 'text.primary', cursor: 'pointer', textDecorationLine: 'underline', width: 'fit-content' }}>
<Checkbox2
checked={filter.myReferenda}
disabled={!referenda}
Expand All @@ -194,7 +183,7 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
onChange={onMyReferenda}
/>
</Grid>
<Grid alignItems='center' container item justifyContent='flex-start' py='10px' sx={{ color: theme.palette.mode === 'light' ? 'secondary.main' : 'text.primary', cursor: 'pointer', textDecorationLine: 'underline', width: 'fit-content' }} >
<Grid alignItems='center' container item justifyContent='flex-start' py='10px' sx={{ color: theme.palette.mode === 'light' ? 'secondary.main' : 'text.primary', cursor: 'pointer', textDecorationLine: 'underline', width: 'fit-content' }}>
<Checkbox2
checked={filter.myVotes}
disabled={!myVotedReferendaIndexes}
Expand Down Expand Up @@ -241,7 +230,7 @@ export default function SearchBox ({ address, myVotedReferendaIndexes, referenda
onChange={() => onFilter('methods')}
/>
</Grid>
<Grid item onClick={onReset} sx={{ color: 'secondary.light', cursor: 'pointer', textDecorationLine: 'underline', ml: '22px' }}>
<Grid item onClick={onReset} sx={{ color: 'secondary.light', cursor: 'pointer', ml: '22px', textDecorationLine: 'underline' }}>
{t('Reset')}
</Grid>
</Grid>
Expand Down
Loading