fix(FR-2526): block file browser access while a revision is being added - #9517
Draft
yomybaby wants to merge 3 commits into
Draft
fix(FR-2526): block file browser access while a revision is being added#9517yomybaby wants to merge 3 commits into
yomybaby wants to merge 3 commits into
Conversation
The Add-revision modal is the model-service update path, and while its `addModelRevision` mutation is in flight both file-browser entry points on the form stay live: the "Open Folder" icon buttons and the folder-name links in the additional-mounts table. A filebrowser session started in that window can edit files in a folder the new replicas are about to mount, so what gets served does not match what the user submitted. `confirmLoading` only spins the OK button — it does not mask the modal body. Gate both on a single `isSubmitInFlight` flag (the mutation plus the image-name resolution that precedes it, i.e. the same state the submit button already shows): - the two "Open Folder" icon buttons become disabled, and their tooltip explains why instead of the button going silently dead; - `VFolderTable` gains `isFolderLinkDisabled`, which renders the folder-name cells as disabled (non-navigating) links, and the mounts form item carries the same explanation in its `extra` slot. Access is restored automatically when the mutation settles. The create path is untouched — this modal only adds revisions to an existing deployment. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
github-actions
Bot
added
area:ux
Contributor
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Review found the new `deployment.FolderAccessDisabledWhileAddingRevision` string shipped untranslated. - Replace the raw `make i18n` `__NOT_TRANSLATED__` placeholder with a real translation in the 18 scanner locales (de, el, es, fi, fr, id, it, ja, mn, ms, pl, pt, pt-BR, ru, tr, vi, zh-CN, zh-TW). Because the key existed, i18next resolved it instead of falling back to en, so a non-English user hovering the disabled Open Folder button literally saw `__NOT_TRANSLATED__`. `origin/main` carries zero occurrences of the placeholder; this restores that. - Add the key to `resources/i18n/th.json` by hand. `th` is not in `i18n.config.js`'s `lngs`, so `make i18n` never writes it, but Thai is a user-selectable language in `UserSettingsPage` and th.json is kept in sync with the other locales by hand. Each translation keeps "Revision" as the untranslated product term, matching the surrounding `deployment.*` strings in the same file (e.g. `AddRevision`, `FailedToRollback`). `bash scripts/verify.sh` ends with `=== ALL PASS ===`. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Modal dismissal can re-enable folder access during an active request, and the submission-state wiring lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Blocks folder-browser entry points while adding a deployment revision.
Changes:
- Reuses submission state to disable folder access with explanatory feedback.
- Adds disabled-link support to
VFolderTable. - Adds link-state tests and translations across all locales.
- Requires fixes for modal dismissal during submission and missing modal-level regression coverage; also includes a prop-naming nit.
File summaries
| File | Description |
|---|---|
resources/i18n/zh-TW.json |
Adds Traditional Chinese feedback. |
resources/i18n/zh-CN.json |
Adds Simplified Chinese feedback. |
resources/i18n/vi.json |
Adds Vietnamese feedback. |
resources/i18n/tr.json |
Adds Turkish feedback. |
resources/i18n/th.json |
Adds Thai feedback. |
resources/i18n/ru.json |
Adds Russian feedback. |
resources/i18n/pt.json |
Adds Portuguese feedback. |
resources/i18n/pt-BR.json |
Adds Brazilian Portuguese feedback. |
resources/i18n/pl.json |
Adds Polish feedback. |
resources/i18n/ms.json |
Adds Malay feedback. |
resources/i18n/mn.json |
Adds Mongolian feedback. |
resources/i18n/ko.json |
Adds Korean feedback. |
resources/i18n/ja.json |
Adds Japanese feedback. |
resources/i18n/it.json |
Adds Italian feedback. |
resources/i18n/id.json |
Adds Indonesian feedback. |
resources/i18n/fr.json |
Adds French feedback. |
resources/i18n/fi.json |
Adds Finnish feedback. |
resources/i18n/es.json |
Adds Spanish feedback. |
resources/i18n/en.json |
Adds English feedback. |
resources/i18n/el.json |
Adds Greek feedback. |
resources/i18n/de.json |
Adds German feedback. |
react/src/components/VFolderTable.tsx |
Supports disabled folder links; prop naming should follow repository conventions. |
react/src/components/VFolderTable.test.tsx |
Tests enabled and disabled folder-link states. |
react/src/components/DeploymentAddRevisionModal.tsx |
Gates folder access during submission, but dismissal can bypass the gate and modal-level regression coverage is missing. |
Review details
- Files reviewed: 24/24 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.
| } | ||
| onCancel={() => onRequestClose()} | ||
| confirmLoading={isAddInFlight || isResolvingImage} | ||
| confirmLoading={isSubmitInFlight} |
Copilot asked for modal-level regression coverage of the folder-access gate: `VFolderTable.test.tsx` only proves the downstream prop, and `DeploymentAddRevisionModal.test.tsx` never held `addModelRevision` pending, so the wiring that drives the gate could regress while the table tests still passed. Add a `DeploymentAddRevisionModal folder-access gate (FR-2526)` suite that drives the preset form to a submittable state, leaves the mutation pending, and asserts the Open Folder entry point is `aria-disabled` *with* the explanatory tooltip in that window — then that the gate lifts once the mutation settles, on success and on error alike. To make the form drivable the two select mocks become form probes that write through their injected `onChange`, and the mocked ids are real global ids because the submit path runs them through `toLocalId` (atob). Custom mode is not covered: its submit path needs a resolvable image id and a populated resource block, both of which come from children this test file mocks out. `bash scripts/verify.sh` ends with `=== ALL PASS ===`. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Prevent modal dismissal while submission is in flight so reopening cannot bypass the folder-access gate.
Review details
Suppressed comments (1)
react/src/components/DeploymentAddRevisionModal.tsx:1945
confirmLoadingonly affects the OK button, so Cancel, the header close button, Escape, and backdrop clicks still callonRequestClose()while the mutation is pending. Both callers wrap this modal inBAIUnmountAfterClose; after dismissal the local Relay in-flight state is lost and the modal can be reopened with both folder entry points enabled even though the server-side add may still be running. Keep the modal mounted by disabling Cancel and ignoring dismissal untilisSubmitInFlightclears.
onCancel={() => onRequestClose()}
confirmLoading={isSubmitInFlight}
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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 #6603 (FR-2526)
The issue was filed against
ServiceLauncherPageContent, which was deleted in #7325 (FR-2852) together with themodify_endpointpath it guarded. The request itself is still undelivered: the model-service update path today isDeploymentAddRevisionModal, and it embeds the same two file-browser entry points with no in-flight gate.While
addModelRevisionis running, the modal body stays fully interactive —confirmLoadingonly spins the OK button (BAIModalpasses it to the OK button'sisLoadingand renders no mask). So a user can still open the folder explorer, start a filebrowser session, and edit files in a folder the new replicas are about to mount. A revision references a model folder, it does not snapshot its contents, so those edits land in what the new replicas serve.What changed
DeploymentAddRevisionModalgets a singleisSubmitInFlightflag (isAddInFlight || isResolvingImage— the same state the submit button already shows, so the image-name resolution that precedes the mutation is covered too) and uses it for the submit button,confirmLoading, and the new gates.VFolderTablegainsisFolderLinkDisabled, which renders the folder-name cells asBAILink type="disabled"— an aria-disabled, non-navigating link — instead of a route to the folder explorer. The additional-mounts form item carries the same explanation in itsextraslot.deployment.FolderAccessDisabledWhileAddingRevision, translated in all 21 locale files.this not ini18n.config.js'slngssomake i18nskips it; it is added by hand, the way the rest ofth.jsonis kept in sync.Access is restored automatically when the mutation settles, success or error. The create path is untouched — this modal only adds revisions to an existing deployment.
Design decisions
VFolderTable, not a tooltip per row. Astryx's disabledLinksetspointer-events: none, so aTooltipwrapping the cell would never fire. The "why" is therefore shown once, in the form item'sextra, while the cells themselves just stop navigating.Scope — what this does not close
Both limits below are deliberate; flagging them so a reviewer can confirm the narrower scope is acceptable for FR-2526 rather than discover it after merge.
FolderExplorerModalalready open via the global?folder=query param, or a filebrowser session already launched in another browser tab, keeps working when the submit starts. Revoking those means force-closing a modal the user may be mid-upload in (FileUploadManager), and a tab this modal has no handle on at all — a product call, not a mechanical one. The gate closes the routes this form owns.ServiceLauncherPageContentgated its whole<Form disabled=…>; this PR does not, becauseBAIModal'sconfirmLoadingrenders no mask and blanket-disabling every field mid-submit is a wider UX change than the issue asks for. Edits made in that window are discarded when the modal closes on success, so they do not reach the submitted revision.Tests
react/src/components/VFolderTable.test.tsx(2 cases): the folder name is a real link by default; withisFolderLinkDisabledthere is no link role and the name sits inside anaria-disabledelement.pnpm exec vitest run src/components/VFolderTable.test.tsx— 2 passed.pnpm exec vitest run src/components/DeploymentAddRevisionModal.test.tsx— 3 passed (no regression).Verification
bash scripts/verify.sh→=== ALL PASS ===Review notes
Checklist: (if applicable)
https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ