Skip to content

refactor(FR-3351): extract idle-checker classification into a helper module - #9492

Closed
yomybaby wants to merge 2 commits into
mainfrom
refactor/FR-3351-extract-idle-checker-color-classificatio
Closed

refactor(FR-3351): extract idle-checker classification into a helper module#9492
yomybaby wants to merge 2 commits into
mainfrom
refactor/FR-3351-extract-idle-checker-color-classificatio

Conversation

@yomybaby

@yomybaby yomybaby commented Sep 7, 2026

Copy link
Copy Markdown
Member

Resolves #8350 (FR-3351)

Follow-up refactor deferred from PR #8260 (FR-3318) review feedback. No behaviour change.

What changed

1. One home for the classification logic. The reclamation color logic lived in
SessionReclamationStatusPopover.tsx while the idle_checks payload types lived in
SessionIdleChecks.tsx, so the two sibling components imported each other (a type-only
cycle). Both now live in react/src/components/ComputeSessionNodeItems/idleChecks.ts:
IdleChecks / IdleCheckItem / UtilizationExtra, getUtilizationCheckerColor,
getOverallReclamation, the severity ordering, RECLAMATION_LEGENDS,
getIdleChecksTagColor, and the useReclamationColorMap hook (it reads t, so it stays
a hook). Neither component imports the other any more.

2. idle_checks is parsed once per boundary instead of three times for the same
session. SessionReclamationStatusCell remains the Relay boundary for the session table
and hands the parsed utilization check to a new presentational SessionReclamationStatus;
SessionIdleChecks renders that component directly from the payload it already parsed, so
it no longer spreads SessionReclamationStatusCellFragment. The popover takes the parsed
UtilizationExtra as a prop, so SessionReclamationStatusPopoverFragment is deleted.

3. The popover body is built lazily. The content JSX moved into
ReclamationPopoverContent. Astryx HoverCard mounts its content lazily (useLayer
renders only a <template> marker while closed), so useResourceSlotsDetails(), the
per-resource rows and the legend rows now run when the card opens rather than for every
rendered session row.

Design decisions

  • UtilizationExtra.resources is typed as Record<string, number[]> rather than the
    previous fixed six keys (cpu_util/mem/cuda_*/ipu_*): the manager builds the map
    from the configured resource slots (UtilizationResourceReport in idle.py), so the
    fixed shape did not describe the payload. Both consumers already treated it as an open
    map.
  • getIdleChecksTagColor moved along with the rest — it is the same color classification,
    and moving it makes it unit-testable without importing a component.
  • SessionReclamationStatus stays in SessionReclamationStatusCell.tsx next to its Relay
    wrapper rather than getting a file of its own.

Tests

  • New react/src/components/ComputeSessionNodeItems/idleChecks.test.ts (15 cases) covering
    the color thresholds, the and/or severity pick, the "no measurement yet" filtering,
    and both branches of getIdleChecksTagColor.
    pnpm --filter backend-ai-webui-react exec vitest run src/components/ComputeSessionNodeItems/idleChecks.test.ts → 15 passed.
  • pnpm --filter backend-ai-webui-react exec vitest run src/components/SessionDetailContent.test.tsx → 3 passed.
  • node scripts/migration-gates/astryx-token-gate.mjs --strict → no new findings (only the
    pre-existing BUI ones).

Verification

bash scripts/verify.sh →

=== ALL PASS ===

Review notes

  • Session list → the "Utilization Idle Timeout" column: the status dot/label and the info
    popover should be unchanged, and the popover should still open instantly (delay={0}).
  • Session detail → Idle Checks panel: the utilization row now renders
    SessionReclamationStatus from the already-parsed payload instead of the Relay cell; the
    row, tag colors and countdown should look identical.
  • Relay: the only __generated__ churn is the removed SessionReclamationStatusPopoverFragment
    and the two fragments that stopped spreading it/being spread.

Checklist: (if applicable)

  • Documentation
  • Minium required manager version
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ

@yomybaby
refactor(FR-3351): extract idle-checker classification into a helper …
43f62bb 
…module

The reclamation color classification lived in SessionReclamationStatusPopover
while the idle-check payload types lived in SessionIdleChecks, so the two
sibling components imported each other. Move both into a dedicated
`idleChecks.ts` module: the payload types, `getUtilizationCheckerColor`,
`getOverallReclamation`, the severity ordering, the legend constants,
`getIdleChecksTagColor` and the `useReclamationColorMap` hook. Neither
component imports the other any more.

`idle_checks` was JSON-parsed three times for the same session (once per
component). It is now parsed once per boundary: `SessionReclamationStatusCell`
stays the Relay boundary for the session table and hands the parsed
utilization check to the new presentational `SessionReclamationStatus`, which
`SessionIdleChecks` renders directly from the payload it already parsed. The
popover takes the parsed `UtilizationExtra` as a prop, so its own fragment is
gone.

The popover body moves into `ReclamationPopoverContent`. Astryx `HoverCard`
mounts its content lazily (`useLayer` renders only a `<template>` marker while
closed), so the resource-slot lookup and the per-resource/legend rows now run
when the card opens instead of for every rendered session row.

`UtilizationExtra.resources` is typed as an open map: the manager builds it
from the configured resource slots (idle.py `UtilizationResourceReport`), so
the previous fixed six-key shape did not describe the payload.

Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
@github-actions

github-actions Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage Report for react-coverage (./react)

Status Category Percentage Covered / Total
🔵 Lines 15.7% 5715 / 36396
🔵 Statements 13.15% 6958 / 52889
🔵 Functions 13.77% 893 / 6484
🔵 Branches 9.52% 4816 / 50573
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
react/src/components/SessionDetailContent.tsx 62.29% 36.17% 16.66% 64.21% 96-97, 125, 148, 152-158, 168, 170, 171, 181, 268, 277, 294, 298, 301, 320-327, 338-341, 435-734, 113-675
react/src/components/ComputeSessionNodeItems/SessionIdleChecks.tsx 0% 0% 0% 0% 21-98
react/src/components/ComputeSessionNodeItems/SessionReclamationStatusCell.tsx 4.44% 0% 0% 9.09% 30-51, 65-81
react/src/components/ComputeSessionNodeItems/SessionReclamationStatusPopover.tsx 3.63% 0% 0% 7.14% 33-42, 99-105
react/src/components/ComputeSessionNodeItems/idleChecks.ts 52% 55.26% 85.71% 67.56% 128-134
Generated in workflow #736 for commit aa64351 by the Vitest Coverage Report Action

@yomybaby
yomybaby requested a balanced review from Copilot September 7, 2026 16:30
Copilot AI reviewed Sep 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The extracted custom hook is missing the required React Compiler annotation directive.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Refactors idle-check classification to remove component coupling, reduce repeated parsing, and lazily render popover content.

Changes:

  • Centralizes idle-check types and classification helpers.
  • Passes parsed utilization data through presentational components.
  • Adds 15 classification tests and updates Relay artifacts.
File summaries
File Description
react/src/components/SessionDetailContent.tsx Updates the shared type import.
react/src/components/ComputeSessionNodeItems/SessionReclamationStatusPopover.tsx Accepts parsed data and lazily renders content.
react/src/components/ComputeSessionNodeItems/SessionReclamationStatusCell.tsx Separates Relay parsing from presentation.
react/src/components/ComputeSessionNodeItems/SessionIdleChecks.tsx Reuses parsed utilization data.
react/src/components/ComputeSessionNodeItems/idleChecks.ts Centralizes types and classification logic.
react/src/components/ComputeSessionNodeItems/idleChecks.test.ts Tests classification behavior and boundaries.
react/src/__generated__/SessionReclamationStatusPopoverFragment.graphql.ts Removes the obsolete generated fragment.
react/src/__generated__/SessionReclamationStatusCellFragment.graphql.ts Removes the popover fragment spread.
react/src/__generated__/SessionIdleChecksNodeFragment.graphql.ts Removes the status-cell fragment spread.
react/src/__generated__/SessionDetailContentQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/resourceRegistrySessionQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/RecentlyCreatedSessionRefetchQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/PendingSessionNodeListQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/DashboardPageQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/ComputeSessionListPageQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/BAIComputeSessionNodeNotificationItemSubscription.graphql.ts Regenerates the updated subscription.
react/src/__generated__/AstryxSessionProbeCasesQuery.graphql.ts Regenerates the updated probe query.
react/src/__generated__/AdminDashboardPageQuery.graphql.ts Regenerates the updated query.
react/src/__generated__/AdminComputeSessionListPageQuery.graphql.ts Regenerates the updated query.
Review details

Files not reviewed (13)

  • react/src/generated/AdminComputeSessionListPageQuery.graphql.ts: Generated file
  • react/src/generated/AdminDashboardPageQuery.graphql.ts: Generated file
  • react/src/generated/AstryxSessionProbeCasesQuery.graphql.ts: Generated file
  • react/src/generated/BAIComputeSessionNodeNotificationItemSubscription.graphql.ts: Generated file
  • react/src/generated/ComputeSessionListPageQuery.graphql.ts: Generated file
  • react/src/generated/DashboardPageQuery.graphql.ts: Generated file
  • react/src/generated/PendingSessionNodeListQuery.graphql.ts: Generated file
  • react/src/generated/RecentlyCreatedSessionRefetchQuery.graphql.ts: Generated file
  • react/src/generated/SessionDetailContentQuery.graphql.ts: Generated file
  • react/src/generated/SessionIdleChecksNodeFragment.graphql.ts: Generated file
  • react/src/generated/SessionReclamationStatusCellFragment.graphql.ts: Generated file
  • react/src/generated/SessionReclamationStatusPopoverFragment.graphql.ts: Generated file
  • react/src/generated/resourceRegistrySessionQuery.graphql.ts: Generated file
  • Files reviewed: 6/19 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread react/src/components/ComputeSessionNodeItems/idleChecks.ts
@yomybaby
fix(FR-3351): address Copilot review
aa64351 
Add the required 'use memo' React Compiler directive as the first
statement of useReclamationColorMap. The repository runs React Compiler
in annotation mode (.github/instructions/react.instructions.md), so the
newly extracted hook was excluded from compiler memoization without it.

Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
@yomybaby
yomybaby requested a balanced review from Copilot September 7, 2026 16:37
Copilot AI reviewed Sep 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The refactor preserves classification semantics, removes redundant parsing and fragments, and includes focused regression tests.

Review details

Files not reviewed (13)

  • react/src/generated/AdminComputeSessionListPageQuery.graphql.ts: Generated file
  • react/src/generated/AdminDashboardPageQuery.graphql.ts: Generated file
  • react/src/generated/AstryxSessionProbeCasesQuery.graphql.ts: Generated file
  • react/src/generated/BAIComputeSessionNodeNotificationItemSubscription.graphql.ts: Generated file
  • react/src/generated/ComputeSessionListPageQuery.graphql.ts: Generated file
  • react/src/generated/DashboardPageQuery.graphql.ts: Generated file
  • react/src/generated/PendingSessionNodeListQuery.graphql.ts: Generated file
  • react/src/generated/RecentlyCreatedSessionRefetchQuery.graphql.ts: Generated file
  • react/src/generated/SessionDetailContentQuery.graphql.ts: Generated file
  • react/src/generated/SessionIdleChecksNodeFragment.graphql.ts: Generated file
  • react/src/generated/SessionReclamationStatusCellFragment.graphql.ts: Generated file
  • react/src/generated/SessionReclamationStatusPopoverFragment.graphql.ts: Generated file
  • react/src/generated/resourceRegistrySessionQuery.graphql.ts: Generated file
  • Files reviewed: 6/19 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@yomybaby
yomybaby marked this pull request as ready for review September 7, 2026 16:40
@yomybaby
yomybaby requested a review from agatha197 as a code owner September 7, 2026 16:40
@yomybaby
yomybaby requested review from nowgnuesLee and removed request for agatha197 September 7, 2026 23:50
@yomybaby yomybaby closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@nowgnuesLee nowgnuesLee Awaiting requested review from nowgnuesLee

Assignees

@yomybaby yomybaby

Labels

quick-capture size:XL 500~ LoC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extract idle-checker color classification logic into a dedicated helper module

2 participants

@yomybaby