Skip to content

fix(models): use a native modal for artifact inspection - #4356

Closed
tang-vu wants to merge 2 commits into
Osmantic:public-betafrom
tang-vu:fix/beta-hub-modal-keyboard
Closed

tang-vu wants to merge 2 commits into
Osmantic:public-betafrom
tang-vu:fix/beta-hub-modal-keyboard

Conversation

@tang-vu

@tang-vu tang-vu commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

The Hub artifact picker now opens as a native modal, focuses Close, handles Escape through the existing import guard, and restores focus to the repository trigger when closed.

Why this matters

Model Library -> Hugging Face -> Choose file previously left keyboard focus on the background repository list. Its ARIA modal attributes did not contain focus or implement Escape, making artifact inspection difficult with a keyboard.

Overlap check

Searched open/closed PRs by Hugging Face, dialog, keyboard, focus and HuggingFaceModelBrowser.jsx. #2755 labels the close button; #3050 cancels details requests; #3203 covers formatting. #4339 addresses search response races in a separate effect. None establishes this modal lifecycle. #4341 fixes a different dependency-confirmation workflow.

Validation

A rendered-browser-component regression failed on the base because Close did not receive focus; it now passes (1 test). It asserts cancel performs no import and restores focus. Focused ESLint: 0 errors. Native Tab containment is provided by showModal; JSDOM stubs do not validate browser inertness. Native Chromium validation is recorded below.

Tradeoffs and rollback

The existing guard continues to prevent closing during import. Import authorization, API calls and artifact verification are unchanged. Reverting only restores the previous overlay. Uses the modern native dialog API supported by the dashboard's target browsers.

Batch integration receipt

Validated with the other 19 public-beta PRs on integration commit ce0d0132, based on upstream f5f49b7. All 20 patches compose without conflicts in creation order; no new PR requires another to fix its own behavior. For shared files, use #4325 before #4327 and #4339 before #4356 as the tested order.

  • Final combined dashboard: 118 test files / 932 tests pass; ESLint 0 errors (575 warnings); production build passes.
  • Affected API suites: 292 pass. APE full suite: 52 pass. Scoped pre-commit secret/private-key/large-file checks and git diff --check pass.
  • Native Linux make gate reaches an existing CLI-update failure also reproduced on the untouched base; fix(update): keep host-agent refresh non-fatal #3159 supplies its separate fix. BATS: 419/421 pass, with the same two baseline failures (non-GNU OSTYPE fixture detects the real WSL host; skip-Docker fixture lacks production helpers). Smoke tests and installer simulation pass. These are explicit validation gaps, not a claim that the whole release gate is green.

Chromium on Windows, using the real component and CSS with mocked API data: initial focus, Tab/Shift+Tab, inert background, Escape, and trigger focus restoration all pass. Screenshots were visually inspected. No services were enabled or artifacts imported in this browser check.

Follow-up 2dfa8cc updates the existing Models page test's native-dialog stubs; all 44 page and dialog tests pass. This fixes the CI integration test failure without weakening the production modal API.

Latest candidate CI: 32 successful checks, 4 skipped. Draft status on filesystem/authorization changes is retained for independent human review despite green CI.

@tang-vu
tang-vu marked this pull request as ready for review September 11, 2026 16:47
@Lightheartdevs

Copy link
Copy Markdown
Collaborator

Thanks for this contribution. public-beta was promoted into main on 2026-09-24 and no longer receives changes, so we're closing pull requests that target it. This isn't a judgment on the change itself. If it's still needed, please rebase onto main and open a focused PR. See #7253 for details and the contribution policy.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants