refactor(FR-3430): split FairShareList into per-step self-contained components - #8516
Merged
Merged
Conversation
Contributor
…omponents Each fair-share step (resource group / domain / project / user) now owns its own query, pagination, filter, order, selection, and modals instead of sharing a single root query with @Skip flags. The selected resource group's scheduler warning moves to a name-filtered single lookup, fixing the page-window miss. URL params now store names only; the user step resolves the project name to an id via the project.name nested filter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Storing the project name in the URL broke old-format URLs (project=<uuid>) still present in browser history, and the extra name-to-id resolution round trip added no value once the step title lookup exists anyway. The user step now receives the project id directly, and the Steps title resolves the display name with a small projectV2 lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ironAiken2
force-pushed
the
refactor/FR-3430-fairshare-step-queries
branch
from
August 3, 2026 23:44
0fa6966 to
624332b
Compare
ironAiken2
marked this pull request as ready for review
August 4, 2026 00:26
The FairShare step screens churn released (query, variables) combos faster than the default 10-slot GC release buffer can hold — notably one *ResourceGroupWarningIconQuery per table row — so a step's query records were garbage-collected between visits and @required(action: THROW) fired on the GC'd reads. Doubling the buffer keeps recently released step queries retained across step round trips. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors FairShare navigation into self-contained step components with isolated Relay queries and state.
Changes:
- Splits resource-group, domain, project, and user steps.
- Adds targeted resource-group and project-name lookups.
- Updates URL cleanup, Relay artifacts, and GC retention.
Reviewed changes
Copilot reviewed 8 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
react/src/RelayEnvironment.ts |
Increases Relay GC release buffer. |
react/src/pages/SchedulerPage.tsx |
Removes obsolete user URL state. |
react/src/components/FairShareItems/FairShareList.tsx |
Orchestrates step navigation and URL state. |
react/src/components/FairShareItems/ResourceGroupFairShareStep.tsx |
Adds resource-group step implementation. |
react/src/components/FairShareItems/DomainFairShareStep.tsx |
Adds domain step implementation. |
react/src/components/FairShareItems/ProjectFairShareStep.tsx |
Adds project step implementation. |
react/src/components/FairShareItems/UserFairShareStep.tsx |
Adds user step implementation. |
react/src/components/FairShareItems/ResourceGroupSchedulerTypeAlert.tsx |
Adds targeted scheduler warning lookup. |
react/src/__generated__/FairShareListQuery.graphql.ts |
Removes obsolete combined query artifact. |
react/src/__generated__/FairShareListProjectNameQuery.graphql.ts |
Adds project-name query artifact. |
react/src/__generated__/ResourceGroupFairShareStepQuery.graphql.ts |
Adds resource-group query artifact. |
react/src/__generated__/DomainFairShareStepQuery.graphql.ts |
Adds domain query artifact. |
react/src/__generated__/ProjectFairShareStepQuery.graphql.ts |
Adds project query artifact. |
react/src/__generated__/UserFairShareStepQuery.graphql.ts |
Adds user query artifact. |
react/src/__generated__/ResourceGroupSchedulerTypeAlertQuery.graphql.ts |
Adds scheduler-alert query artifact. |
Files not reviewed (7)
- react/src/generated/DomainFairShareStepQuery.graphql.ts: Generated file
- react/src/generated/FairShareListProjectNameQuery.graphql.ts: Generated file
- react/src/generated/FairShareListQuery.graphql.ts: Generated file
- react/src/generated/ProjectFairShareStepQuery.graphql.ts: Generated file
- react/src/generated/ResourceGroupFairShareStepQuery.graphql.ts: Generated file
- react/src/generated/ResourceGroupSchedulerTypeAlertQuery.graphql.ts: Generated file
- react/src/generated/UserFairShareStepQuery.graphql.ts: Generated file
agatha197
requested changes
Aug 5, 2026
Contributor
There was a problem hiding this comment.
The refactoring direction (splitting into self-contained step components, fixing the page-window bug) is good. Please address the items below before merging. (__generated__/** was excluded from review.)
Code quality
- Selection state, modal state, pagination, toolbar JSX, and modal JSX are nearly duplicated across the 4 step components (
ResourceGroupFairShareStep/DomainFairShareStep/ProjectFairShareStep/UserFairShareStep). See the inline comment. ProjectFairShareStep'sonClickProjectNamecallback parameter name doesn't match the actual value (a project id). See the inline comment.
Potential issues / risks
- Entering the Domain/Project/User steps duplicates the
adminResourceGroupsfetch (once for the scheduler warning, once for the modal fragment). See the inline comment. - The
gcReleaseBufferSizechange affects the app-wide Relay store but has no explanatory comment in the code. See the inline comment.
Test coverage
- There are no unit/e2e tests for FairShare, before or after this refactor. Given this PR fixes regression-prone behavior (the page-window bug, resetting pagination on step transitions), I'd recommend adding at least a minimal e2e scenario (navigate steps → verify filter/pagination persists correctly).
Add a one-line note above gcReleaseBufferSize clarifying it retains step queries released during FairShare step navigation (default 10). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ogic Collapse the ~150 lines of selection state, toolbar JSX, and modal wiring that were repeated across the Domain/Project/User fair-share steps. - Add useFairShareStepSelectionState: bundles the multi-row/single-row selection state and the weight-setting/usage modal open state, plus a keyField-bound row-select handler and clear/close helpers. - Add FairShareStepToolbar: the filter + selection action buttons + AutoUpdateFetchKeyButton row. Selection actions are optional, so the resource-group step reuses it without any selection support. - Merge the duplicate adminResourceGroups lookup: the scheduler-type alert now consumes a ResourceGroupSchedulerTypeAlertFragment fed by the resource-group node each step already fetches for the weight modal, instead of firing its own identical query from FairShareList. - Rename ProjectFairShareStep's onClickProjectName parameter to projectId to match the table (which passes record.projectId) and FairShareList. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed the review feedback in 62962c1 + 7c2c697 (all inline threads replied & resolved):
Re: test coverage — agreed FairShare has no coverage today; a minimal e2e scenario (step navigation → filter/pagination persistence) is planned as a follow-up rather than in this refactor PR, to keep the diff reviewable. |
nowgnuesLee
previously requested changes
Aug 9, 2026
…e Suspense - Remove useFairShareStepSelectionState: selection/modal state goes back to plain useState + handleRowSelectionChange in each step, matching the convention used by every other list page (e.g. AdminComputeSessionListPage, VFolderNodeListPage). - Render ProjectStepNameText without its own null-fallback Suspense: step transitions run in a deferred-value background render, so suspending there makes the transition wait for the name query too — the name is guaranteed to be present when the step commits, instead of popping in after an empty gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…step-queries Resolve the FR-3482 antd->Astryx migration against the FairShare per-step split: - FairShareList.tsx: keep the per-step orchestrator structure; port main's migration decisions (antd Steps -> lab Stepper/Step with string label/description, Alert -> Banner, Skeleton -> BAISkeletonAstryx, Typography.Title -> Heading, theme -> theme-shim, drop the antd-style createStyles block). The selected project's display name moves into an `@skip`ped projectV2 query because Step.description must be a string. - New step components (ResourceGroup/Domain/Project/User) and FairShareStepToolbar / ResourceGroupSchedulerTypeAlert: convert antd imports to Astryx (Tooltip content/placement, Banner, theme-shim) and drop the unsupported pagination.style key (BAIAstryxPaginationConfig has none). - SchedulerPage / RelayEnvironment: clean auto-merge (main's Astryx page + this branch's user-param removal and gcReleaseBufferSize). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
agatha197
requested changes
Aug 19, 2026
agatha197
left a comment
agatha197
left a comment
Contributor
There was a problem hiding this comment.
Is there any reason to maintain pagination with 3 step?
If I click 1, the pagination step is maintained. But if I click 2, the pagination step goes to 10.
…ansition Each step owns its pagination via useBAIPaginationOptionStateOnSearchParam, which stores both `current` and `pageSize` in the URL. The step-transition reset in FairShareList cleared order/filter/current but left `pageSize`, so a page size chosen on one step leaked into the next step's table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UU4bgjsFizw5FriTUjnsHp
Contributor
Author
@agatha197 Good catch — that inconsistency was a bug, not intended. Each step owns its pagination via Fixed in c39e98b: |
agatha197
requested changes
Sep 1, 2026
agatha197
left a comment
agatha197
left a comment
Contributor
There was a problem hiding this comment.
I know it is unrelated with this PR, but Fair Share Factor sorting doesn't work in every tab. Can you remove it from available sort items?
Contributor
Author
I will work on this in a separate PR. @agatha197 |
This was referenced Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #8515 (FR-3430)
Problem
FairShareList.tsxfetched all four fair-share connections (adminResourceGroups,rgDomainFairShares,rgProjectFairShares,rgUserFairShares) in a single rootuseLazyLoadQuery. Because of the@skipvariables, only the current step's connection was actually fetched — the single-query structure was pure overhead:@skipflags, per-step order/filter variable switches (FairShareOrderVariables/FairShareFilterVariables), andlimit/offsetshared across 4 connections.adminResourceGroupsfetch + page-window bug — the resource group list was refetched on every pagination/filter change of lower steps, and the selected resource group was located via_.findon the current page of the unfiltered list. If it fell outside the page window, the scheduler warning Alert and the modal'sresourceGroupFrgmtsilently disappeared.Changes
ResourceGroupFairShareStep/DomainFairShareStep/ProjectFairShareStep/UserFairShareStep, each owning its ownuseLazyLoadQuery, pagination, filter, order, row selection, and modals (FairShareWeightSettingModal/UsageBucketModal).FairShareListkeeps only the step URL state, the Steps UI, and step-transition callbacks. All@skipflags, variable switches, andas Array<...>casting switches are gone; step transitions reset selection naturally via unmount.ResourceGroupSchedulerTypeAlertqueries the selected resource group by name (filter: { name: { equals } }, limit: 1), fixing the page-window miss. The weight-setting modal's resource group fragment is likewise fetched per step by name.resourceGroup/domainby name,projectby id, so pre-existing URLs and browser history stay valid. The project name in the Steps title is resolved by a smallprojectV2lookup (ProjectStepNameText) instead of being part of the root query. The unuseduserURL param is removed.current), which the previous forward-navigation handlers forgot (stale page carried into the next step).gcReleaseBufferSizefrom the default 10 to 20 (RelayEnvironment.ts) — re-entering a step could intermittently crash into theErrorBoundaryfallback (Relay: Missing @required value at path ...) with zero network requests. Leaving a step releases its queries into the store's GC release buffer, and the FairShare screens churn released(query, variables)combos fast — notably one*ResourceGroupWarningIconQueryper table row — so a step's records were garbage-collected between visits while the react-relayQueryResourcecache hit on remount skipped the refetch. A larger buffer keeps recently released step queries retained across step round trips. This supersedes the per-entry fetchKey approach explored in fix(FR-3430): force a fresh fetch on FairShare step remount with per-mount fetchKey #8565 (closed); see that PR for the full root-cause analysis.Behavioral notes
UserNotAllowedInResourceGroupalert (UserResourceGroupAlert) now renders inside the user step (below the Steps panel) instead of above the info alert.SchedulerPage'sErrorBoundaryintentionally has noresetKeys: once an error is caught, the fallback stays until the user explicitly resets via the "go back to first step" button. This is by design and unchanged here.Verification
bash scripts/verify.sh→=== ALL PASS ===(Relay / Lint / Format / TypeScript / Vite warmup / Terminology)🤖 Generated with Claude Code