make the "Measure this model" gate a routable sidebar instead of a modal - #4805
Merged
Conversation
Which model the assessment gate has open now lives in the URL (?measureBackend / measureModel / measureTuning on /models/performance) rather than in local state, so it is shareable, bookmarkable and survives a reload — the same rule the AI-provider editor follows at /ai/edit/:providerId. Search params rather than a path segment because a model id is not one: it carries `/` and `:` (`hf.co/org/repo:Q4_K_M`), and the drawer is an overlay on an already-routed tab. The consent contract is unchanged (it still names the backend, model and generation count before any provider call, and closing still aborts a run in flight). A deep link that names a record the report no longer lists opens the drawer with a notice instead of bouncing, and the tuning form is seeded once from that record so a re-measure reproduces its configuration.
Review follow-ups, all behavior-preserving: - `openTarget` / `closeTarget` hand-rolled the merge-and-prune URLSearchParams loop `useUrlParams` owns (client/src/hooks/README.md says not to re-roll it), and re-adopted the closure-stale functional `setSearchParams` updater the hook exists to close over. - The tuning form seeded a state copy through a ref + effect to avoid a background report refresh clobbering a half-typed knob. Deriving the draft from the record — with an "edited yet?" override — does the same in four lines. - The target carried a `resolved` flag alongside a `reportLoaded` prop that the drawer recombined; the parent now computes `unknownTarget` once. - Row lookup re-spelled the composite identity `entryKey` already encodes, and copied every model in the report into one array to run a single find. - Dropped the leftover Modal heading: the Drawer header already renders the title and the model id.
The Drawer's icon-only header button aborts an in-flight assessment, but kept the primitive's default "Close settings" accessible name — a screen-reader user had no way to know that pressing it kills a multi-minute run.
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
1 participant
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.
Summary
Drawerinstead of aModal. Which model is open lives in the URL —?measureBackend=&measureModel=&measureTuning=on/models/performance— so the gate is shareable, bookmarkable and survives a reload, the same rule the AI-provider editor follows at/ai/edit/:providerId./and:(hf.co/org/repo:Q4_K_M), and the drawer is an overlay on an already-routed tab.Follow-up filed as #4803: the sibling per-model Sweep tunings gate is still a modal, so two consent gates on the same row now behave differently.
Test plan
clientsuite:LocalModelAssessments.test.jsx48/48 — the 42 existing tests now render inside aMemoryRouter, plus 6 new ones covering the routable behavior (URL written on open, deep link opens with no click, tuning seeded from the record a link names, notice for an unlisted id, URL cleared on cancel and on a completed run).client/src/components/settings/+Models.test.jsx+a11yConventions.test.js: 311/311.Catalog.test.jsx,a11yConventions.test.js) pass in isolation and are unrelated load flakes.biome lint --error-on-warnings src: clean.