feat(FR-454): paginate and filter the session detail kernel list - #9521
feat(FR-454): paginate and filter the session detail kernel list#9521yomybaby wants to merge 2 commits into
Conversation
The kernel table in the session detail panel rendered every kernel of a cluster session at once, sorted client-side, with the filter UI commented out — the two TODOs said to wait for a query that supports filtering and pagination. Manager 26.2.0 added `sessionKernelsV2(scope: SessionScope!, filter: KernelV2Filter, orderBy: [KernelV2OrderBy!], limit, offset)`, so that query now backs the tab. `ConnectedKernelListV2` runs its own `useLazyLoadQuery` against it with offset-mode pagination (`limit` + `offset` — a Strawberry V2 connection rejects a mixed mode at runtime), server-side ordering driven by the table header (cluster hostname / cluster index / status) and a `BAIGraphQLPropertyFilter` over the properties `KernelV2Filter` actually exposes: kernel id and status. `KernelV2` rows are addressed by their Strawberry global id, so the kernel id column and the container-log button decode it with `safeDecodeUuid`. The legacy `kernel_nodes` path is kept for managers below 26.2.0 behind the new `session-kernels-v2` client capability, and its two obsolete TODOs are dropped. The container-log modal keeps its own complete kernel list, so its kernel picker is unaffected by the table's paging. `KernelV2` has no `status_info` counterpart, so the V2 status cell shows the status badge alone rather than the legacy double tag. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate runtime and state-management issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds server-side pagination, filtering, and ordering to session kernel details while retaining legacy-manager compatibility.
Changes:
- Adds the
sessionKernelsV2kernel table and tests. - Selects the V2 path through a new client capability.
- Removes obsolete legacy-table TODOs.
Unresolved issues:
- Moderate: Combined filters fail on Managers 26.2–26.6 because sub-filters require 26.7.
- Moderate: Clearing sorting removes the deterministic
CLUSTER_IDX ASCfallback. - Moderate: Kernel-list state persists when navigating between sessions, potentially showing stale or empty results.
File summaries
| File | Description |
|---|---|
react/src/components/SessionDetailContent.tsx |
Selects the legacy or V2 kernel list. |
react/src/components/ComputeSessionNodeItems/ConnectedKernelListV2.tsx |
Implements the paginated, filtered V2 table. |
react/src/components/ComputeSessionNodeItems/ConnectedKernelListV2.test.tsx |
Tests query variables and row rendering. |
react/src/components/ComputeSessionNodeItems/ConnectedKernelList.tsx |
Removes obsolete commented code. |
react/src/__generated__/ConnectedKernelListV2Query.graphql.ts |
Adds the generated Relay query artifact. |
packages/backend.ai-client/src/client.ts |
Enables V2 kernels for Manager 26.2+. |
Review details
Files not reviewed (1)
- react/src/generated/ConnectedKernelListV2Query.graphql.ts: Generated file
- Files reviewed: 5/6 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Gate combined filter conditions on the `sub-filter` capability. Managers 26.2-26.6 serve `sessionKernelsV2` but reject the AND/OR/NOT sub-filters `BAIGraphQLPropertyFilter` emits once two tokens are entered, so the filter runs in `singleCondition` mode there (same pattern as AdminRuntimeVariantPreset / ResourcePolicyPage). - Restore `CLUSTER_IDX ASC` when sorting is cleared. `BAITable.onChangeOrder` emits `undefined` for the unsorted state; storing `null` sent no `orderBy` at all, leaving offset-paginated pages without a deterministic order. - Remount `ConnectedKernelListV2` per session. The dependent/dependency links swap only the `sessionDetail` search param, so `SessionDetailContent` stays mounted with a new `row_id` and the list carried its filter, page and sort over to the next session. Test updated with a `useSuspendedBackendaiClient` mock; 3/3 pass. verify.sh: === ALL PASS === Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
There was a problem hiding this comment.
🟡 Changes recommended
The unsupported combined-filter path and unstable pagination ordering must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- react/src/generated/ConnectedKernelListV2Query.graphql.ts: Generated file
Suppressed comments (1)
react/src/components/ComputeSessionNodeItems/ConnectedKernelListV2.tsx:201
singleConditiononly keeps one token per property; it does not limit the filter to one overall token. With both Kernel ID and Status selected,powerSearchFiltersToGraphQLFilter()still reachesconvertConditionsToGraphQLFilter()with two conditions and emits{ AND: [...] }(BAIGraphQLPropertyFilter.tsx:655-683, 369-375), so managers 26.2–26.6 still reject this path. Please either make the filter emit one flat object for distinct properties when sub-filters are unavailable, or prevent a second property token entirely, and cover the ID+Status case withsupports('sub-filter') === false.
singleCondition={!supportsSubFilter}
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| { | ||
| scope: { sessionId }, | ||
| filter: deferredFilter, | ||
| orderBy: convertToOrderBy<KernelV2OrderBy>(deferredOrder), |
|
Ready-gate status: Copilot pass 2 (#pullrequestreview-5134172287) left two findings that I am not deciding on my own. The PR stays a draft.
Fixed and resolved in |
Resolves #3084 (FR-454)
The kernel table in the session detail panel rendered every kernel of a cluster session at once, ordered client-side, with its filter UI commented out — two TODOs said to wait for a query that supports filtering and pagination. Manager 26.2.0 added
sessionKernelsV2(scope: SessionScope!, filter: KernelV2Filter, orderBy: [KernelV2OrderBy!], limit, offset), so that query now backs the tab.What changed
ConnectedKernelListV2(new) — runs its ownuseLazyLoadQueryagainstsessionKernelsV2with:limit+offset). A Strawberry V2 connection rejects a mixed pagination mode at runtime, sofirstis deliberately not read offuseBAIPaginationOptionState(.claude/rules/graphql-pagination.md).CLUSTER_IDX ASCso the main kernel stays first, as the old client-sideorderBy(['cluster_role', 'cluster_idx'])did.BAIGraphQLPropertyFilterover the propertiesKernelV2Filteractually exposes: kernelidandstatus. It has noagentIdfield, so the agent filter the old TODO asked for is not part of this PR.ConnectedKernelList(legacy) — kept unchanged as the path for managers below 26.2.0; only its two now-obsolete TODOs and the commented-out filter block are removed.SessionDetailContent— picks between the two behind the newsession-kernels-v2client capability (added to the existing 26.2.0 block inbackend.ai-client), and forwards the panel'sfetchKeyso a session refresh refetches the kernel page.Design decisions
KernelV2rows are addressed by their Strawberry global id, so the kernel-id column and the container-log button decode it withsafeDecodeUuidrather than arow_idfield (there is none).kernel_nodeslist, so its kernel picker still offers every kernel regardless of which table page is shown.KernelV2has nostatus_infocounterpart, so the V2 status cell shows the status badge alone instead of the legacyBAIDoubleTag. This is the one behavioural difference between the two paths.kernel.KernelId/kernel.Statuscolumn labels.Tests
react/src/components/ComputeSessionNodeItems/ConnectedKernelListV2.test.tsx(3 cases) — asserts the query sendslimit/offsetand nofirst/after/last/before, that the defaultorderByisCLUSTER_IDX ASC, and that a row renders from the nestedKernelV2shape.cd react && pnpm exec vitest run src/components/ComputeSessionNodeItems/ConnectedKernelListV2.test.tsx→Test Files 1 passed (1) / Tests 3 passed (3)pnpm run relay— generated artifact committed.Verification
bash scripts/verify.sh→Review notes
countrather than the fetched array length.Status = RUNNING, or a kernel id) and click a sortable header (Hostname / Status) — both should round-trip to the server and reset to page 1.session-kernels-v2capability to simulate).Checklist: (if applicable)
ConnectedKernelListV2.test.tsxhttps://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ