EDM-3777: Filter repositories via the API#630
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (6)
Summary by CodeRabbitRelease Notes
WalkthroughThis pull request removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/ResourceSync/RepositoryResourceSyncList.tsx (1)
276-286:⚠️ Potential issue | 🟡 MinorGuard
ResourceSyncEmptyStatewhen a search filter is active.Now that
nameSearchdrives a server-side query, an active search with zero matches will renderResourceSyncEmptyState("No resource syncs here!") even though the repository may actually contain resource syncs that just don't match the filter. This is also inconsistent withRepositoryList.tsx(line 231), which gates its empty state with!nameSearch. It may additionally collide with theTable's own "no results for the applied filters" state driven byhasFilters/emptyData/clearFilters.🛠️ Proposed fix
- {resourceSyncs.length === 0 && ( + {resourceSyncs.length === 0 && !nameSearch && ( <ResourceSyncEmptyState addResourceSync={ canCreate ? () => { setIsAddRsModalOpen(true); } : undefined } /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/ResourceSync/RepositoryResourceSyncList.tsx` around lines 276 - 286, The empty-state component ResourceSyncEmptyState is rendered even when a nameSearch filter is active; change the render guard so the empty state only appears when there are zero resourceSyncs AND no active nameSearch (mirror RepositoryList behavior), i.e. update the condition around ResourceSyncEmptyState to include !nameSearch while keeping the existing canCreate -> setIsAddRsModalOpen logic intact so the addResourceSync prop remains unchanged.
🧹 Nitpick comments (1)
libs/ui-components/src/utils/query.ts (1)
68-88: Object-parameter refactor looks good; minor note on rsName interpolation.The new signature is clean and mirrors the
repo + optional name containspattern elsewhere. One small thing to keep in mind:rsNameis spliced into a comma-joinedfieldSelectorwithout escaping, so a literal comma inrsNamewould be interpreted as a selector separator by the API.TableTextSearchonly strips whitespace, not commas. Not blocking given resource-sync names shouldn't contain commas, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/utils/query.ts` around lines 68 - 88, getResourceSyncsByRepo currently injects rsName directly into the fieldSelector which treats commas as selector separators; modify the function so you escape commas in rsName before building the selector (e.g., compute const safeRsName = rsName.replaceAll(',', '\\,') and use safeRsName in the selectors.push call), keeping the rest of the URLSearchParams logic unchanged; this ensures commas in the provided rsName are treated literally when constructing the fieldSelector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@libs/ui-components/src/components/ResourceSync/RepositoryResourceSyncList.tsx`:
- Around line 276-286: The empty-state component ResourceSyncEmptyState is
rendered even when a nameSearch filter is active; change the render guard so the
empty state only appears when there are zero resourceSyncs AND no active
nameSearch (mirror RepositoryList behavior), i.e. update the condition around
ResourceSyncEmptyState to include !nameSearch while keeping the existing
canCreate -> setIsAddRsModalOpen logic intact so the addResourceSync prop
remains unchanged.
---
Nitpick comments:
In `@libs/ui-components/src/utils/query.ts`:
- Around line 68-88: getResourceSyncsByRepo currently injects rsName directly
into the fieldSelector which treats commas as selector separators; modify the
function so you escape commas in rsName before building the selector (e.g.,
compute const safeRsName = rsName.replaceAll(',', '\\,') and use safeRsName in
the selectors.push call), keeping the rest of the URLSearchParams logic
unchanged; this ensures commas in the provided rsName are treated literally when
constructing the fieldSelector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bacd6e19-bcdc-4a1b-9cbf-d8ba19934f76
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/ocp-plugin/package.jsonapps/standalone/package.jsonlibs/ui-components/package.jsonlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/RepositoryDetails/DeleteRepositoryModal.tsxlibs/ui-components/src/components/Repository/RepositoryList.tsxlibs/ui-components/src/components/Repository/useRepositories.tslibs/ui-components/src/components/ResourceSync/RepositoryResourceSyncList.tsxlibs/ui-components/src/components/modals/massModals/MassDeleteRepositoryModal/MassDeleteRepositoryModal.tsxlibs/ui-components/src/hooks/useTableTextSearch.tslibs/ui-components/src/utils/query.tslibs/ui-components/src/utils/search.ts
💤 Files with no reviewable changes (5)
- libs/ui-components/package.json
- apps/standalone/package.json
- apps/ocp-plugin/package.json
- libs/ui-components/src/utils/search.ts
- libs/ui-components/src/hooks/useTableTextSearch.ts
b651fb8 to
f70f339
Compare
| const prevNameRef = React.useRef(name); | ||
| React.useEffect(() => { | ||
| if (prevNameRef.current !== name) { | ||
| prevNameRef.current = name; | ||
| setCurrentPage(1); | ||
| } | ||
| }, [name, setCurrentPage]); |
There was a problem hiding this comment.
why are you using name Ref ? Would this be sufficient ?
| const prevNameRef = React.useRef(name); | |
| React.useEffect(() => { | |
| if (prevNameRef.current !== name) { | |
| prevNameRef.current = name; | |
| setCurrentPage(1); | |
| } | |
| }, [name, setCurrentPage]); | |
| React.useEffect(() => { | |
| setCurrentPage(1); | |
| }, [name, setCurrentPage]); |
There was a problem hiding this comment.
The only difference is that with the ref we avoid a re-render in the first mount.
I can remove it, since it's not very clear in any case.
f70f339 to
f550b18
Compare
Filtering by name in Repositories page worked differently to the rest of the pages, since it was done client-side and used fuzzy search.
Now the filtering in Repositories / ResourceSyncs will be done via the API with "contains"