fix(FR-3316): carry the model card's folder into Custom mode - #9489
Draft
yomybaby wants to merge 1 commit into
Draft
fix(FR-3316): carry the model card's folder into Custom mode#9489yomybaby wants to merge 1 commit into
yomybaby wants to merge 1 commit into
Conversation
In the Add Revision modal's Preset mode the "Model Card" source resolves the
picked card to its backing vfolder, kept in `selectedCardVfolderId` as a raw
UUID rather than in the form's `modelFolderId`. The Preset -> Custom transfer
only carried `presetValues.modelFolderId`, so switching to Custom with a card
selected dropped the folder and left the Custom form's required Model Folder
field empty.
Make the carry source-aware: in card mode encode `selectedCardVfolderId` with
`toGlobalId('VirtualFolderNode', ...)` — the Custom form's `modelFolderId` is
global-id valued (its submit path runs `toLocalId`), so the raw UUID cannot be
carried across as-is. The folder-source branch is unchanged. The reverse
Custom -> Preset direction was already handled.
The unit test drives the real transition (source radio -> card pick -> mode
toggle) and asserts the Custom folder selector receives the re-encoded id; it
fails without the fix. The E2E half of the issue is not included — see the PR.
Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Update the closing reference or separately track the omitted E2E coverage before merge.
Pull request overview
Fixes Preset → Custom mode transfer so a model card’s backing folder remains selected.
Changes:
- Converts the card’s raw vfolder UUID to a global ID.
- Adds a unit test covering the mode transition.
- Updates the mode-state test mock.
File summaries
| File | Description |
|---|---|
react/src/components/DeploymentAddRevisionModal.tsx |
Carries the model card’s backing folder into Custom mode. |
react/src/components/DeploymentAddRevisionModal.test.tsx |
Tests global-ID folder transfer across modes. |
Review details
Suppressed comments (2)
react/src/components/DeploymentAddRevisionModal.test.tsx:332
- This test covers only acceptance item 1, but the PR uses
Resolves #8255, so merging it will automatically close the issue while item 2's required Playwright scenarios remain incomplete. Please either add that E2E coverage or move it to a separately tracked issue and update the closing reference before merge.
it("carries the model card's backing folder into Custom mode as a global id", async () => {
react/src/components/DeploymentAddRevisionModal.tsx:985
- The linked issue includes the Model Card E2E scenarios as item 2, but this PR explicitly omits them while its description still says
Resolves #8255. Merging will auto-close the issue with that acceptance criterion incomplete, contrary to the description's request to keep it open. Please change the closing reference to a non-closing one, or move the E2E work to a separate tracked issue and resolve that follow-up explicitly.
if (presetValues.presetModelSource === 'card') {
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
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 #8255 (FR-3316)
What changed
DeploymentAddRevisionModal's Preset → Custom transfer now carries the modelcard's backing folder.
In Preset mode's Model Card source the picked card resolves to a backing
vfolder that lives in
selectedCardVfolderId(a raw UUID), not in the form'smodelFolderId.handleModeChangeonly carriedpresetValues.modelFolderId,so switching to Custom mode with a card selected silently dropped the folder and
left the Custom form's required Model Folder field empty. The carry is now
source-aware: in card mode it re-encodes
selectedCardVfolderIdwithtoGlobalId('VirtualFolderNode', …), because the Custom form'smodelFolderIdis global-id valued (its submit path runs
toLocalId), so the raw UUID cannot behanded over as-is. The folder-source branch is untouched, and the reverse
Custom → Preset direction was already handled in #8246.
Not included: the E2E half of the issue
Item 2 of the issue (Playwright coverage for the Model Card branch: preset
scoping to the card's
availablePresets, source-reset on toggle back, andsubmission of the card's vfolder) is not in this PR. Meaningful coverage
needs either a cluster carrying a model card whose backing folder is mountable,
or a new set of GraphQL mocks for
projectModelCardsV2/modelCardAvailablePresetsine2e/serving/mocking/; neither locators nor mockshapes can be validated from this environment, and an unrunnable spec on a suite
that executes against real clusters is worse than none. Please keep #8255 open
for that half (or split it out) rather than letting the auto-close take it.
Tests
pnpm exec vitest run src/components/DeploymentAddRevisionModal.test.tsx(from
react/) — 4 passed. The new case drives the real transition (sourceradio → card pick → mode toggle) and asserts the Custom folder selector
receives the re-encoded global id. Verified it fails with the source
change reverted, so it genuinely guards the fix.
Verification
bash scripts/verify.sh→=== ALL PASS ===Review notes
Model Card → pick a card → switch to Advanced Mode: the Model Folder
selector should now be pre-filled with the card's backing folder.
presetModelSource: 'folder', clears the card, and nullsselectedCardVfolderId.useBAISettingUserStatestand-in now holds the mode in realstate (it used to return a
vi.fn()setter), which is what lets the modetoggle actually switch forms; the three pre-existing contract tests still
pin their mode via
mockModeand pass unchanged.Checklist: (if applicable)
https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ