FEAT: Pick an operation from existing values in the labels bar - #2396
Open
varunj-msft wants to merge 24 commits into
Open
FEAT: Pick an operation from existing values in the labels bar#2396varunj-msft wants to merge 24 commits into
varunj-msft wants to merge 24 commits into
Conversation
Setting the operation label meant retyping the name from memory. The suggestion list that was supposed to help never appeared, because it filtered the known values against the value being replaced -- with the shipped default op_trash_panda that matches nothing. Editing the operation label now opens a combobox listing the operations already in memory, sourced from the labels request the bar already makes. Typing filters the list and offers to create a name that doesn't exist yet. - Existing values are selectable as-is; only new names are validated, so operations created before the current naming rules stay usable. - The editor now renders in the labels popover too. Clicking a label there previously set edit state but rendered no editor, which showed nothing at all when the chip was too narrow to fit inline. - Label rows are reachable by keyboard, and focus moves into the picker. - Escape, clicking away, and Tab all leave without writing a value. The selected operation applies to attacks started afterwards; it does not relabel existing ones.
Contributor
|
Screenshots please! Or video. |
The dropdown took its width from the input it hangs off, so longer operation names were cut off mid-name with no ellipsis -- op_2026_05_mai_image_2.5 rendered as op_2026_05_mai_image. Names never wrap out of it either: values may only contain letters, digits and underscores, none of which are line break opportunities. Widening the input instead would push it past the labels bar and clip the control itself, so leave the input alone and let the dropdown size to its own content.
Rejecting a bad name after the fact put the reason in a line of text beside the picker, and the labels bar clips anything that overflows it. On a narrow ribbon none of it survived; at full width it read "Only l". The message also stayed on screen while the name was corrected. The rules are now stated in the dropdown as the name is typed, and a name that breaks them is not offered for creation at all. The dropdown sizes to its contents, so the whole message is always readable. Fluent dims disabled options to roughly 1.9:1 against their background, which is too faint for text that has to be read rather than chosen, so the notes carry their own colour.
With more operations than fit under the input -- and there are already around sixty in use -- the list stopped hanging off the picker and became a full height column pinned to the top of the window, covering the page. Giving it a ceiling lets it stay where it belongs and scroll instead. A name typed into the picker also disappeared from it. Operations are read once from the labels API, and a name only reaches that API after an attack has been stored under it, so a name created moments earlier was absent when the picker was reopened and was offered for creation again. Newly created names now join the list they came from.
A failed labels request left the picker looking like a working picker with nothing in it, so someone whose backend had hiccuped was told their operations did not exist and invited to type a name that already existed somewhere else. It now says it could not load them, and keeps the "none recorded yet" wording for the case where that is actually true. Also fold the operation naming rules back into the single validator they were copied from, and stop the newly created name from being appended to a list captured before the request that fills it had returned.
The picker lets a name be typed and created before the request that fills it has come back, and the response then replaced everything that had been collected in the meantime, so a name created during those first moments disappeared again as soon as the list arrived. The response is now merged with what is already there rather than replacing it.
The 240px cap never took effect. Fluent's combobox defaults to autoSize: true, which writes its own max-height inline once positioned, and an inline style beats the class the cap lives in. Overriding only matchTargetSize left autoSize in place, so the list stretched to whatever room it had: 501px below the input at an 800px viewport, and above the input it ran to the top of the window. Asking Fluent to auto-size width alone leaves the height to the class. Measured in Chromium with 60 options: 240px and anchored under the input at 800px, 240px and anchored above it at 500px and 420px, still scrolling internally, and the dropdown width is unchanged. It shrinks below the cap when there are fewer options. No test: jsdom does not position the popup, so the inline max-height that caused this is never written there and a unit assertion would pass either way.
Sizing the dropdown to width alone hands the height back to the class, which is what makes the 240px cap work, but it also gives up Fluent's vertical fitting. The cap was a flat 240px, so in a window shorter than about 250px the list ran past the viewport edge and the options there were unreachable. Yielding to the viewport keeps both: measured with 60 options, the list is still the full 240px and anchored at every height from 300px up, and at 200px it now renders 168px and stays on screen instead of overflowing by 40px.
Every sizing bug in this feature shipped past a green unit run, because jsdom has no layout engine: reverting the fix that caps the list height leaves all 53 LabelsBar unit tests passing. These run in the existing mock Playwright project, which CI already runs on every PR and which needs only Vite. They assert what jsdom cannot -- the list is capped and anchored to the input, it stays on screen when it opens upwards, and a long operation name is not cut off. Reverting the cap fails the first one with a 501px list.
Contributor
Yes, would be great if it persisted! Same for operator name. I suppose that one persists in the deployed version due to auth but in the local one it doesn't. |
The picker is 180px where the plain input it replaced was 120px, and the row it sits in has never been allowed to shrink. On Home that row starts further right once the grid splits into two columns, so the control ran past the edge the bar clips at and took its dropdown chevron with it. Measured on Home with the editor open, against main: main clips 0px at every width, this branch clipped 27px at 520, 1000 and 1024 — and a hit test at the chevron's centre returned the card behind it rather than the icon. 1024 is an ordinary laptop width. Letting the operation row give way fixes it: 0px clipped and the chevron hit-testable at 520/560/600/1000/1024/1090/1280/1920. The control keeps its full 180px wherever there is room and shrinks to about 140 where there is not. The dropdown is sized separately, so it still measures 466px and shows a 58-character name in full.
Each labels bar fetches its own list of known operations, and the one on Home and the one in the chat ribbon are separate mounts — Home is a route, so it is thrown away when you navigate. Pick an operation on Home, go to Chat to run the attack, and the picker there does not list the value the chip is showing: typing it offers to Create the name already in use, which is exactly the confusion the picker was built to remove. It is also gone from Home on the way back. The value in use is now listed wherever it came from, and shows as selected. The placeholder stays off the list — it is not a real operation, and it would otherwise contradict "No operations yet". The empty and could-not-load notes now key off the fetched list rather than what is on screen, so listing the current value cannot suppress them. Without that, a failed load with an operation already set would have shown that operation and said nothing about the failure.
Picking an operation only changed React state, so a refresh put the run back on the op_trash_panda placeholder — and brought its warning icon back with it. Anyone who reloads mid-session and does not notice files their next attacks under the placeholder, on the very field this feature exists to set. The labels are now kept in localStorage, the same way the theme and the chat markdown preference already are, with the same guards for storage being unavailable. Only string values are read back, so a hand-edited entry cannot reach the rest of the app. Precedence is built-in defaults, then the backend's default_labels, then what you last picked, and a signed-in account still decides the operator — so a deployment that sets labels in its config file still wins on first run, and auth still names the operator. The stored labels are read once at mount so the version request landing later cannot undo them.
Four gaps, each proven by reverting the fix and watching the suite stay green: - the short-window height yield: a flat 240px cap passed every existing assertion because they run at 800 and 420, both tall enough to hide it - the editor overhanging the labels bar at 1024 - the operation in use missing from the picker - the labels not surviving a refresh Reverting each fix now fails exactly one test, with the number from its commit message: 240px where it should shrink, 27px of overhang, the value in use absent, the operation back on the placeholder after reload. The spec also mocks the endpoints the app calls while booting, so it no longer waits on a dev-server proxy with no backend behind it. That cut the run from about 30s a test to under 10.
The merge that decides this had no test: swapping it so the backend's default_labels beat what the user picked left all 116 tests green. The plan called for these and they were missed. Two cases, both mutation-checked: - what you last picked beats the backend defaults, while backend keys you never chose still come through - a signed-in account still names the operator, even with one stored Each asserts after the backend response has landed rather than inside waitFor, so it cannot pass by matching the state from before the merge — which is how my first attempt at these passed against the mutation.
The labels bar is usable before the version request comes back, and that response was overwriting whatever had been picked in the meantime: the merge re-applied the labels read from storage at mount, which are stale by then. Picking an operation two seconds after load and waiting silently put the old one back. Measured in Chromium with the version request held for six seconds: the chip went op_from_storage, op_i_pick_now, then back to op_from_storage. The backend's defaults now only fill in labels you have not chosen — neither stored earlier nor picked since — instead of the whole object being rebuilt around a stale snapshot. A deployment that sets labels in its config file still seeds a first run, and a signed-in account still names the operator. This is the same mistake as the one fixed for the operation list in 8b4c049, one level up.
Listing the operation in use made it selectable in appearance only. The check that waives the naming rules looks at the names the labels API returned, and the one in use is not necessarily among them — it can come from a config file, or from a session where no attack has been stored under it yet. Clicking it rejected it with "Only lowercase letters, numbers, underscores" and left the editor open. Reproduced with legacy-op-name.2024 in use and absent from the API: the option appeared, and clicking it produced the error instead of selecting it. That breaks the rule that a name already in memory is always selectable, which is the whole reason legacy names are exempt. The name in use is now exempt too. Added in the same commit that started listing it, c89954d.
Persisting the whole label map wrote the operator placeholder to storage
the first time an operation was picked, even though nobody chose it. From
then on it beat whatever the backend was configured to hand out, on every
visit, and the placeholder warning stayed lit.
Measured in Chromium across two visits: pick an operation on day one,
have the deployment configure operator=varunj on day two, and the bar
still read roakey with the warning showing. Storage held
{"operator":"roakey","operation":"op_x"}.
Only labels that differ from the placeholders are stored now, so the
above ends up as {"operation":"op_x"} and the configured operator comes
through. What you actually picked still wins.
Listing the operation in use meant the picker could show it and, directly underneath, "No operations yet — type a name to create one". The note was keyed off the fetched list, which is empty in that case. The empty note now appears only when there is genuinely nothing on the list. The could-not-load note still keys off the fetch, so a failed request says so even when an operation is already set — that separation is why the note was keyed off the fetch in the first place.
Persistence kept every label that differed from the built-in placeholder,
which swept up whatever the backend's default_labels had supplied. Those
were never anyone's choice, and once stored they outranked that same
config on every later visit — so a deployment that changed its configured
operator was silently ignored. Any label interaction was enough to
trigger it, including a click that changed nothing.
Measured in Chromium: config says operator=configured_day1, pick an
operation, storage becomes {"operator":"configured_day1","operation":
"op_beta"}; change the config to configured_day2 and reload, and the bar
still reads configured_day1.
Now only labels that differ from what the app would have shown anyway —
placeholders, then whatever the backend hands out, then the signed-in
account — are kept. The same run now stores {"operation":"op_beta"} and
picks up configured_day2.
The failure note was only shown when the list was empty, and a name created while the request was still in flight is added to that list. So creating one and then having the request fail left the picker showing a single local name and no sign that anything had gone wrong — the list looked complete when it was empty. The note now follows the request, not the list length, and sits above whatever local names there are. The empty note is unchanged: it still means the list really is empty, and it no longer has to share a condition with the failure case.
Four conditions in this PR could be deleted without a single test failing, including the one added to fix a defect a reviewer had already hit. - The late `/version` response only fills in labels you have not touched. The test named for that case seeded storage and sent no default labels, so the clause it was meant to guard never ran. Drop `&& untouched` and the whole suite still passes, while the chip silently reverts to the backend's value. Covered now by a case that stores nothing and does send an `operation` default. - The "could not load" note has to stay while a name is being typed; it answers why the list is empty, and typing does not answer that. - The notes share the list with real values, so they have to stay unselectable. - "No operations yet" must not appear next to an offer to create one. Each of the four now fails exactly one named test when removed. No production code changed.
Contributor
Author
Roman Lutz (@romanlutz) screenshots below! First is the before/after — on Second one is your persistence ask - that's a real page refresh, and the operator name comes 
|
| No operations yet — type a name to create one | ||
| </Option> | ||
| )} | ||
| {matches.map(option => ( |
Contributor
There was a problem hiding this comment.
Have you tested this against a production-sized database? This renders every unique operation from the full attack history and rescans the list on each keystroke. With production data, opening or searching the picker may be extremely slow or freeze the UI. I’m concerned this approach may not be viable at our production scale.
Fluent renders every option as a real component and does not virtualize, so the list the picker builds is as long as the number of operations memory has ever seen. Measured in Chromium on a production build: 58 operations opens in 120ms, 5,000 takes 867ms with half-second keystrokes, and 50,000 blocks the tab for 27 seconds on a single keystroke. Render the first 200 and say how many are left, the same way the label filter in the history bar already does. Opening and typing are then flat: 50,000 operations open in 131ms with 79ms keystrokes and no blocking at all. The operation in use is added to whatever the labels request returned, so it now goes to the front of that list — appended, it was the first thing a cap would drop, and nothing would have failed.
Two ways the capped list could drop the very operation you wanted, both only reachable once memory holds more names than the cap shows. The operation in use was only moved to the front of the list when the labels request had not returned it. Once an attack has been stored under it, it usually is in that response, and it stays wherever it sorted — so the cap dropped it and you could no longer see or re-pick the operation you were working in. Typing a name in full had the same problem from the other end. If two hundred other operations contain what you typed, the exact one sorts wherever it sorts, the list never shows it, and no offer to create it appears either, because it does exist. Pressing Enter then committed whichever operation happened to be listed first: a different operation from the one you typed, with nothing on screen to say so. Both are now pinned ahead of the cap. Also says in the GUI docs that long lists show the first two hundred.
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
2 participants
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.
Description
Setting the
operationlabel meant retyping the name from memory. There was a suggestionlist meant to help, but it never appeared: it filtered the known values against the value you
were replacing, and with the shipped default
op_trash_pandathat matches nothing.Clicking the
operationlabel now opens a combobox listing the operations already in memory,using the labels request the bar was already making. Typing filters the list, and offers to
create a name that doesn't exist yet.
Your labels also survive a refresh now — the operator name too,
which is the part that only mattered locally, since a signed-in deployment gets that from auth
either way. Only labels you actually chose are kept, so a value that merely came from the
backend config doesn't get frozen into your browser and outrank that same config later. The
order is: built-in placeholders, then the backend's
default_labels, then what you stored,then whatever you pick this session — and a signed-in account always names the operator.
A few other details worth calling out:
the current naming rules (like
some-op-name) stay usable, including the one already in use.state but render no editor at all, so on a narrow window — where the chip doesn't fit inline
— there was no way to edit it.
notes aren't selectable, and the failure one stays put while you type, because typing doesn't
answer why the list is empty.
and Tab all leave without writing a value.
The operation you pick applies to attacks you start afterwards; it doesn't relabel existing
ones. Two known limits I left alone deliberately: picking a value that happens to equal the
backend's configured default isn't recorded as a choice (that's what stops config from getting
frozen), and two tabs open at once are last-write-wins, same as the existing theme and markdown
preferences.
No backend or API changes.
Tests and Documentation
LabelsBar.test.tsxgoes from 27 tests to 61, covering the picker: options listed on openwithout clearing the field, filtering, creating a name, rejecting an invalid one, selecting a
legacy value that predates the naming rules, re-selecting the value already in use, the
loading/empty/failed notes and their disabled state, keyboard select/dismiss, and editing from
the popover.
labelStorage.test.tsadds 6 for the storage helper, andApp.test.tsxcoverswhich label wins when stored, backend and account values disagree.
There's also a new Playwright spec,
e2e/labels-operation-picker.spec.ts(11 tests). Thedropdown's size and placement are decided by Fluent's floating positioning at runtime, and
jsdom has no layout engine — several sizing regressions shipped past a green Jest run. These
measure the rendered box in a real browser. It runs in the existing
mockproject, which ishow the other specs in
frontend/e2e/work.npx jest src/components/Labels src/components/Home src/App.test.tsx→ 137 passingnpx playwright test --project=mock→ 96 passingnpm run lintandnpm run type-check→ cleanDocs: added a short paragraph to
doc/gui/0_gui.mddescribing the picker and the "applies tonew attacks only" behavior. It's a plain markdown page with no paired notebook, so JupyText
doesn't apply here.