Repository navigation
fix(ui): limit incident Assignee filter to users/teams with incidents - #36402
shah-harshit wants to merge 5 commits into
Conversation
The AI-mode Incident Manager Assignee filter listed every user and team. Read its options from a terms aggregation over the incident index instead, scoped to the active domain, so only assignees of some incident show. The aggregate request sends deleted: null because incident documents have no top-level deleted field and the default deleted: false term matches none.
shah-harshit
added
UI
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
|
| Count | Rule |
|---|---|
| 2 | openmetadata-imports/no-lower-layer-page-imports |
| 1 | react-hooks/exhaustive-deps |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/IncidentManager/IncidentManager.test.tsx:18:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/IncidentManager/useIncidentFilterOptions.ts:27:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/common/UserTeamSelectableList/UserTeamSelectableList.component.tsx:359:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'init'. Either include it or remove the dependency array. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed…ignees Restore the tabbed UserTeamSelectableList in the AI-mode incident filter and give it an allowedIds prop: the user and team lists, their searches and the tab counts only cover those ids. Both incident filters (AI mode and the classic list) pass the ids of the incident assignees, read once per domain from a terms aggregation on assignee.id.
| }) | ||
| ), | ||
| }; | ||
| }, [allowedIds]); |
There was a problem hiding this comment.
Opening the Assignee picker before the aggregation finishes leaves the open list empty after the IDs arrive. Updating allowedIds rebuilds the query filters, but SelectableList fetches its first page only on mount. The picker also keeps that empty page until it closes, and its tab counts do not refresh.
Users must close and reopen the picker to see the loaded choices. Clear the cached pages and refresh the list and counts when allowedIds changes, or wait to mount the list until the IDs are ready.
Knowledge Base Used: UI foundations
There was a problem hiding this comment.
Fixed in b42b824, in UserTeamSelectableList itself so both incident filters get it. When allowedIds changes while the picker is open, both lists remount (their key is the ids) and fetch again, the cached first pages are dropped, and both tab counts are read again. Ids that arrive after opening now fill the open picker; no close and reopen needed. A new test checks this and fails without the fix. Both comments come from this one root cause, so there were no other occurrences to fix.
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| ), | ||
| }; | ||
| }, [allowedIds]); | ||
| const allowedIdsKey = allowedIds?.join(','); |
There was a problem hiding this comment.
⚠️ Bug: Remounted lists still read the old cache, so new ids show no one
Changing allowedIdsKey remounts both SelectableLists. Each one's mount useEffect calls fetchOptions('') (SelectableList.component.tsx, fetchListOptions). In a commit React runs child passive effects before parent ones, so this call happens before the parent effect at lines 384-395 clears initialUserOptionsRef/initialTeamOptionsRef. fetchUserOptions/fetchTeamOptions check the ref synchronously at the top (lines 162, 205), so they return the empty first page cached while the ids were []. The tab counts update to the new totals, but the lists stay empty until the picker is closed and reopened: the same bug this commit is meant to fix. The test misses this because it mocks SelectableList and calls fetchOptions by hand after the parent effect has run. Fix: clear the caches before any child effect runs, using useLayoutEffect (all layout effects in a commit run before passive effects), or store the cache together with the ids key it was built for and ignore it when the key differs.
Fix 1: Store the ids key with each cached page so a cache built for other ids is never returned (apply the same change to the team cache and fetchTeamOptions).
const initialUserOptionsRef = useRef<{
key?: string;
data: EntityReference[];
paging: { total: number; after?: string };
} | null>(null);
// in fetchUserOptions:
if (isInitialPage && initialUserOptionsRef.current?.key === allowedIdsKey) {
return initialUserOptionsRef.current;
}
// when caching:
initialUserOptionsRef.current = { ...result, key: allowedIdsKey };
- Apply fix
Fix 2: Clear the caches in a layout effect so it runs before the remounted children's mount effects.
useLayoutEffect(() => {
initialUserOptionsRef.current = null;
initialTeamOptionsRef.current = null;
}, [allowedIdsKey]);
- Apply fix
Check a box to apply a fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
Describe your changes:
Fixes open-metadata/openmetadata-collate#7693
The Incident Manager Assignee filter listed every user and team in the org, so picking someone with no incidents just returned an empty list.
The filter keeps the shared Teams / Users picker (
UserTeamSelectableList): two tabs, counts on both, search and paging. It now only offers the users and teams assigned to at least one incident, in both the AI-mode grouped Incident Manager and the classic incident list.UserTeamSelectableListtakes an optionalallowedIds. When set, every user/team search it runs (the lists, their searches, both tab counts) adds atermsclause onid.keyword. Unset leaves every other caller unchanged.useIncidentAssigneeIds()reads those ids once per domain from a terms aggregation ontestCaseResolutionStatusDetails.assignee.idover thetestCaseResolutionStatusindex (POST /search/aggregate). The options never depend on the page of incidents on screen.domains.fullyQualifiedNameterm/prefix the rest of the app uses.deleted: null. Incident documents have no top-leveldeletedfield, and the schema defaultdeleted: falseadds a term that matches none of them (verified against a local stack:deleted:false→ 0 buckets,deleted:null→ the real assignees).AGGREGATE_PAGE_SIZE_LARGE(1000) assignees; a terms aggregation cannot page.Known limits (left as is on purpose):
Aaronfinds Aaron Johnson;aadoes not).Type of change:
High-level design:
N/A — small change (3 UI files).
Tests:
Use cases covered
Unit tests
UserTeamSelectableList.test.tsx:allowedIdsscopes the user/team searches and counts; no ids leaves them unscoped; ids that arrive while the picker is open re-read both lists and both counts.IncidentGroupsFilters.test.tsx(AI mode): aggregation request, ids reach the picker, domain scope, failure → nobody.IncidentManager.test.tsx(classic): opening the picker searches users and teams scoped to the incident assignee ids.npx jest src/components/observability/IncidentManager src/components/IncidentManager src/components/common/UserTeamSelectableList→ 31 suites, 391 tests passed onb42b82497c6.npx tsc --noEmitclean;make ui-checkstyle-changedclean.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
select-assigneekeep working: the picker and its test ids are unchanged.Manual testing performed
assignee.idreturns 6 ids; the user index filtered by them returns those 6 users, the team index 0.Aaron→ Aaron Johnson; pick → 7 groups become 1./incident-manager→ Assignee: same tabs and counts; pick Aaron Johnson → his one incident.deleted: false→ no buckets (why the request sendsnull).UI screen recording:
This branch at
6ccc83f929b, Vite dev server proxied to a local server on :8585 with sample data, 1600×1000. Before this PR, the same picker listed every team and user (Teams 222, Users 320 on this data).AI mode (Observability → Incidents)
AaronClassic (
/incident-manager, Classic interface)AaronRecordings (H.264, about 16 s each): AI mode · classic
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>(title carries no ticket id by team convention; the ticket is linked above)My PR is linked to a GitHub issue via
Fixesabove.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: not applicable.
For a UI feature or feature task: not applicable (bug fix).
I have added tests (unit / integration / Playwright as applicable) and listed them above.
Self-review: I reviewed the final diff against
REVIEW.mdand fixed every 🔴 finding, or answered it on the line below.b42b82497c6. No 🔴. Answered: historical assignees, filters that don't narrow the options and table-tab scope are listed above as known limits.Decision check: no durable decision; nothing added under
docs/decisions/.I have added a test that covers the exact scenario we are fixing.
🤖 Generated with Claude Code
Refresh the open Assignee picker when its IDs arrive before merging.
Findings
Summary
The PR limits Assignee choices using a domain-scoped aggregation of incident assignee IDs.
Diagram
Reviews (3) · Last reviewed commit: "fix(ui): keep the Teams/Users assignee p..." · Reviewed by Greptile