Skip to content

refactor(FR-2977): unify revision-source prefill in DeploymentAddRevisionModal#7602

Open
agatha197 wants to merge 2 commits into
mainfrom
05-27-refactor_fr-2977_unify_revision-source_prefill_in_deploymentaddrevisionmodal
Open

refactor(FR-2977): unify revision-source prefill in DeploymentAddRevisionModal#7602
agatha197 wants to merge 2 commits into
mainfrom
05-27-refactor_fr-2977_unify_revision-source_prefill_in_deploymentaddrevisionmodal

Conversation

@agatha197
Copy link
Copy Markdown
Contributor

@agatha197 agatha197 commented May 27, 2026

Resolves #7601 (FR-2977)

Summary

Base refactor for DeploymentAddRevisionModal so any caller (e.g. a future "Duplicate as new revision" action — see the stacked PR #7561) can request an id-based revision prefill cleanly, replacing the prop-drilled fragment ref + dual-useEffectEvent state pump that surfaced during review of FR-2957:

  • Single source fragment — define DeploymentAddRevisionModal_revisionSource at module scope, spread on both currentRevision and the optional id-based revision(id:) field. Both are read via useFragment, sharing one $data shape so the extracted applyRevisionPrefill(rev) helper accepts either without an unsafe cast.
  • Conditional id-based fetch — extend the modal's query with $sourceRevisionId: ID! and $hasSourceRevision: Boolean! variables, plus a top-level revision(id:) @include(if:) @since(version: "25.16.0") field. When no source is requested, the field is excluded from the wire payload and the placeholder id is never evaluated server-side. The revision-history list query no longer needs to spread the prefill fragment on every row — only the id is required.
  • Render-as-you-fetch — the modal switched from useLazyLoadQuery to usePreloadedQuery with a deferred query ref, following the same pattern PR refactor(FR-2964): switch route and deployment scheduling history modals to render-as-you-fetch #7575 applied to the scheduling-history modals. The opener (DeploymentDetailPage) drives the modal via useQueryLoader: clicking "Add Revision" calls loadAddRevisionQuery({ deploymentId, sourceRevisionId, hasSourceRevision }, { fetchPolicy: 'store-and-network' }) and then flips the open flag. The page-level <Suspense> boundary around the modal is removed since the query is no longer started inside render.
  • queryRef-based source-revision control (no sourceRevisionId prop) — instead of a separate sourceRevisionId prop on the modal, the source id and hasSourceRevision flag ride on the preloaded query's variables. The modal reads queryRef.variables.hasSourceRevision to lock effectiveMode to 'custom' synchronously, and queryRef.variables.sourceRevisionId is the source id the query already resolved. Callers like feat(FR-2957): add duplicate-revision action in revision detail drawer #7561 pass the source through loadQuery variables rather than a separate prop. This consolidates "what data the modal needs" + "what mode the modal opens in" into one decision the opener makes via loadQuery, matching how RouteSchedulingHistoryModal / DeploymentSchedulingHistoryModal consume their preloaded queries.
  • Single apply effectuseEffect keyed on (open, sourceRevision), body wrapped in useEffectEvent per project convention, applying the prefill once per source id via an appliedSourceIdRef. The ref resets on close so the next session starts clean.

Notes

  • Stacked under feat(FR-2957): add duplicate-revision action in revision detail drawer #7561 (FR-2957), which lands the "Duplicate as new revision" menu/i18n/icon and the parent state that drives loadAddRevisionQuery with the source id. Land bottom-up — this PR first, then feat(FR-2957): add duplicate-revision action in revision detail drawer #7561.
  • Intentionally callsite-neutral for the source-revision path: no consumer of DeploymentAddRevisionModal calls loadAddRevisionQuery with hasSourceRevision: true yet. DeploymentDetailPage always passes sourceRevisionId: '' / hasSourceRevision: false for the existing "Add Revision" button.
  • API evolved beyond the original issue's "mode + sourceRevisionId as props" sketch — the project standardized on render-as-you-fetch for modals in PR refactor(FR-2964): switch route and deployment scheduling history modals to render-as-you-fetch #7575 (FR-2964), and this PR aligns with that convention. Net effect for callers is the same: the opener decides the mode + source in the same tick, just by constructing the preloaded query's variables instead of passing two separate props.
  • Out of scope: presetTransferPrefill / customTransferPrefill (the preset ↔ custom value transfer from FR-2862) — same anti-shape, but translates between two different form shapes and is a structurally different problem.
  • Modal UX is unchanged — alert copy, mode segmented control behavior when not locked, Apply button placement, footer all preserved. Pure data-flow refactor.

Test plan

  • Add Revision button (sourceless) — opens in persisted mode, no auto-prefill, mode toggle visible, "Load current revision" alert visible.
  • Load current revision alert — populates the custom form with the current revision's values (Model Folder, Runtime, image, 4 Core / 8 GiB, Single Node, etc.).
  • Toggle Custom ↔ Preset — the existing transfer prefill still moves the shared values across (Custom→Preset transfers Model Folder; Preset→Custom transfers Model Folder back).

Screenshots

Preset Mode (default) Advanced Mode — Load current revision alert After loading current revision
Preset Mode Advanced Mode with alert After loading current revision

@github-actions github-actions Bot added the size:XL 500~ LoC label May 27, 2026
Copy link
Copy Markdown
Contributor Author


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • flow:merge-queue - adds this PR to the back of the merge queue
  • flow:hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

Coverage Report for react-coverage (./react)

Status Category Percentage Covered / Total
🔵 Lines 6.52% 1800 / 27566
🔵 Statements 5.34% 1995 / 37291
🔵 Functions 5.27% 297 / 5626
🔵 Branches 3.72% 1300 / 34863
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
react/src/components/DeploymentAddRevisionModal.tsx 0% 0% 0% 0% 140-1851
react/src/components/DeploymentConfigurationSection.tsx 0% 0% 0% 0% 81-392
react/src/pages/DeploymentDetailPage.tsx 0% 0% 0% 0% 53-221
Generated in workflow #1210 for commit 3ba8330 by the Vitest Coverage Report Action

@agatha197 agatha197 force-pushed the 05-27-refactor_fr-2977_unify_revision-source_prefill_in_deploymentaddrevisionmodal branch from ce82933 to 034f843 Compare May 28, 2026 01:55
@agatha197 agatha197 marked this pull request as ready for review May 28, 2026 01:56
Copilot AI review requested due to automatic review settings May 28, 2026 01:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors DeploymentAddRevisionModal to a render-as-you-fetch model with a single shared DeploymentAddRevisionModal_revisionSource fragment. The modal now consumes a PreloadedQuery whose variables (sourceRevisionId, hasSourceRevision) can request an id-based revision lookup via revision(id:) @include @since("25.16.0"). Both currentRevision and the optional source revision are read through the same fragment, and a single keyed effect applies the prefill — replacing the previous useLazyLoadQuery + duplicate-state pump pattern and laying the groundwork for the stacked "Duplicate as new revision" PR (#7561).

Changes:

  • Introduce DeploymentAddRevisionModal_revisionSource fragment + exported DeploymentAddRevisionQuery; add $sourceRevisionId/$hasSourceRevision variables and a conditional revision(id:) top-level field.
  • Switch the modal from useLazyLoadQuery to usePreloadedQuery+useDeferredValue, derive effectiveMode='custom' when the queryRef carries hasSourceRevision, and reduce prefill to one keyed useEffect/useEffectEvent using appliedSourceIdRef.
  • Move query kickoff to DeploymentDetailPage via useQueryLoader, drop the local <Suspense> boundary, and gate modal render on addRevisionQueryRef != null.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
react/src/pages/DeploymentDetailPage.tsx Replaces useToggle with useState+useQueryLoader to preload the modal query on click; removes the local Suspense boundary.
react/src/components/DeploymentConfigurationSection.tsx Updates the Add Revision button comment to reflect the render-as-you-fetch flow.
react/src/components/DeploymentAddRevisionModal.tsx Adds the shared revision-source fragment, exported preloaded query, single apply-prefill effect, and locks mode to Custom when queryRef.variables.hasSourceRevision is true.
react/src/generated/DeploymentAddRevisionModalQuery.graphql.ts Regenerated query artifact for the new variables and conditional revision field.
react/src/generated/DeploymentAddRevisionModal_revisionSource.graphql.ts New generated fragment artifact for the shared revision-source selection.
Files not reviewed (2)
  • react/src/generated/DeploymentAddRevisionModalQuery.graphql.ts: Language not supported
  • react/src/generated/DeploymentAddRevisionModal_revisionSource.graphql.ts: Language not supported

Comment thread react/src/components/DeploymentAddRevisionModal.tsx
@agatha197 agatha197 force-pushed the 05-27-refactor_fr-2977_unify_revision-source_prefill_in_deploymentaddrevisionmodal branch from 034f843 to 1b84a66 Compare May 28, 2026 05:54
agatha197 pushed a commit that referenced this pull request May 28, 2026
@agatha197 agatha197 marked this pull request as draft May 28, 2026 07:57
@agatha197 agatha197 marked this pull request as ready for review May 28, 2026 09:04
Copy link
Copy Markdown
Contributor

@nowgnuesLee nowgnuesLee left a comment

Choose a reason for hiding this comment

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

Please resolve the merge conflicts.

agatha197 added 2 commits May 29, 2026 05:33
…sionModal

Lay groundwork for id-based revision-source prefill in the Add Revision
modal so a caller (e.g. a future "Duplicate as new revision" action)
can pass a `sourceRevisionId` and have the form auto-fill with that
revision's full configuration. The existing "Load current revision"
alert button continues to work unchanged as the user opt-in path for
the deployment's active revision.

What changed:
- Define a single `DeploymentAddRevisionModal_revisionSource` fragment
  at module scope. Both prefill paths (current revision + sourceRevisionId)
  read through it via `useFragment`, sharing one `$data` shape so the
  newly extracted `applyRevisionPrefill(rev)` helper accepts either
  without an unsafe cast.
- Extend the modal's `useLazyLoadQuery` with `$sourceRevisionId: ID!`
  and `$hasSourceRevision: Boolean!` variables and a conditional
  top-level `revision(id:)` field gated by `@include(if:)` and
  `@since(version: "25.16.0")`. When no source is requested, the
  field is excluded from the wire payload and the placeholder
  empty-string id is never evaluated server-side.
- Replace the inline `currentRevision` selection with a fragment
  spread.
- Add a `sourceRevisionId?: string | null` prop. When set,
  `effectiveMode` is derived synchronously as `'custom'` so the form
  mounts in custom mode immediately — no `setMode` -> mount race,
  no `pending*Prefill` state pump. The mode segmented control is
  hidden for that session; the persisted `mode` setting is untouched
  for the next normal open.
- A single `useEffect` keyed on `(open, sourceRevision)` and a
  `useEffectEvent` body apply the prefill once per source id. The
  `appliedSourceIdRef` resets on close so the next session starts
  clean.

What did not change:
- `presetTransferPrefill` / `customTransferPrefill` (the preset <->
  custom mode value transfer mechanism from FR-2862). That path
  translates between two different form shapes and is a structurally
  different problem; out of scope here.
- Modal UX (alert copy, mode segmented control behavior when not
  locked, Apply button placement, footer). Pure data-flow refactor.

Verified manually against the three flows on the local dev server:
1. Add Revision button (sourceless) -> opens in persisted mode, no
   prefill, toggle visible.
2. Load current revision alert button -> populates custom form with
   current revision values (Model Folder, Runtime, image, 4 Core /
   8 GiB, Single Node, etc.).
3. Toggle Custom <-> Preset -> existing transfer prefill still moves
   the shared values across.

This base PR is intentionally callsite-neutral: no consumer passes
`sourceRevisionId` yet. FR-2957 ("Duplicate as new revision") will
stack on top and add the menu items, i18n, icon, and the parent
state that drives `sourceRevisionId`.
…-fetch

Also adds buiTranslations to bui-language mock to fix test TypeError from
missing export (introduced by FR-2987 / #7595, backfilled here since
FR-2988 / #7620 that fixed main landed after this branch was created).
@agatha197 agatha197 force-pushed the 05-27-refactor_fr-2977_unify_revision-source_prefill_in_deploymentaddrevisionmodal branch from 1b84a66 to 3ba8330 Compare May 29, 2026 05:38
@agatha197 agatha197 requested a review from nowgnuesLee May 29, 2026 05:38
)}
</BAIFlex>
}
width={720}
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.

[Re: lines +1381 to +1394]


                <Suspense fallback={<BAISelect loading />}>
                  <Form.Item
                    name="modelFolderId"
                    noStyle
                    rules={[{ required: true }]}
                  >
                    <BAIVFolderSelect
                      ref={presetVFolderSelectRef}
                      currentProjectId={currentProjectId ?? undefined}
                      disabled={!currentProjectId}
                      excludeDeleted
                      filter='usage_mode == "model"'
                      style={{ flex: 1 }}
                    />
                  </Form.Item>
                </Suspense>

See this comment inline on Graphite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL 500~ LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor DeploymentAddRevisionModal to id-based revision source

3 participants