feat(web): chip-bar device filter UI (#968 PR 2)#1013
Conversation
985fbfd to
6a56a5c
Compare
6a56a5c to
567da46
Compare
|
Rebased onto current main now that #1012 (the #968 PR 1 filter engine) is merged. Dropped this branch's three superseded PR-1 commits (their filterEngine work is now in main via #1012) and kept only the two PR-2 web-UI commits. Web preflight green on node 22: astro check 0 errors, vitest 730/730. Ready for review. |
ToddHebebrand
left a comment
There was a problem hiding this comment.
The chip-bar UX itself works — preview contract matches the merged #1012 backend, the add/remove/edit chip state machine is correct, no XSS, and runAction isn't required (the only network call is the read-only POST /filters/preview count). Bouncing on coherence, not correctness.
Must-fix 1 — this reinvents the existing filter component library. apps/web/src/components/filters/ already ships a full set: FilterBuilder.tsx, FieldSelector.tsx (searchable field dropdown), ValueInput.tsx (per-type value editor), OperatorSelector.tsx, FilterPreview.tsx, ConditionGroup/Row.tsx, SavedFilterList.tsx. This PR adds a parallel stack under devices/ — FilterSentenceBuilder.tsx, FilterAddDropdown.tsx, FilterValueEditor.tsx, FilterPreviewFooter.tsx — that re-solves the same problems, plus a hand-rolled click-outside mousedown close handler in three places (FilterAddDropdown.tsx, FilterChipBar.tsx, the help popover) that FieldSelector.tsx already standardizes. The chip-bar can be a distinct presentation, but the field dropdown / value editor / preview pieces should wrap the existing filters/ components rather than duplicate them.
Must-fix 2 — filterFields.ts:14 V2_FILTER_FIELDS is a third copy of the field catalog. The source of truth is the backend filterEngine.ts field set; web already has a second copy in filters/FilterBuilder.tsx (DEFAULT_FILTER_FIELDS); this adds a third. Three hand-maintained catalogs will drift — a field added backend-side silently won't appear, or will appear with the wrong operators (already visible: patches.pending / alerts.critical / system.rebootRequired are modeled here as enum {yes} but are boolean {equals,notEquals} in filterEngine.ts, so the UI can't express the negative case). Derive the catalog from one place.
Should-fix — flag uses a query param. The filter value correctly serializes to the #filtersV2= hash, but the on/off override is ?filtersV2=0/1 (filterUrl.ts:60), against CLAUDE.md's "use window.location.hash for transient UI state, never query params." If the override needs to be URL-addressable, put it in the hash too; the localStorage['breeze.filtersV2'] sticky form is fine as-is.
Happy to re-review once the new builder/value/dropdown pieces compose the filters/ library and the field catalog has a single owner.
…s#968 PR 2) The NinjaOne-style chip bar for /devices, on top of the now-complete filterEngine (LanternOps#968 PR 1). Builds a FilterConditionGroup and feeds it through the existing DeviceList serverFilter → /filters/preview → matchingIds path (the plumbing already on main); no new device-list query. - FilterChipBar + FilterAddDropdown + FilterValueEditor + FilterSentenceBuilder (advanced/nested mode) + FilterHelpPopover + FilterPreviewFooter (live match count via /filters/preview) + QuickAddChips (one-click common filters). - filterFields.ts: web mirror of the API FILTER_FIELDS catalog. - filterUrl.ts: shareable `#filtersV2=<b64>` hash + the filtersV2 flag — default ON, opt out via `?filtersV2=0` or localStorage (one release window, then remove the flag + legacy DeviceFilterBar per LanternOps#968). - DevicesPage renders the chip bar under the flag, legacy DeviceFilterBar otherwise, both feeding the same advancedFilter state. Software multi-select uses the editor's text entry; the name-autocomplete picker needs a distinct-software endpoint (follow-up). Saved-filter sharing is deferred to PR 3 (SavedFiltersPanel intentionally not included). astro check clean; full web suite green (node 22); eslint clean.
…talog (LanternOps#968 PR 2) Web V2_FILTER_FIELDS mirror of the server FILTER_FIELDS additions: lastUser, isHeadless, uptimeSeconds, watchdogStatus, quarantinedAt, lastSeenIp, and the full 7-value status enum. Keeps the picker in sync with the engine.
…review) Addresses the LanternOps#1013 review (Must-fix 2 + should-fix): - Collapse the field catalog to ONE source: new filters/filterFields.ts mirrors the backend filterEngine (39 fields, correct orgId/siteId/groupId keys, full status enum). filters/FilterBuilder DEFAULT_FILTER_FIELDS and the chip bar's V2_FILTER_FIELDS both re-export it — no more third hand-copy. This also fixes the stale DEFAULT catalog (wrong dotted hierarchy keys, phantom patchCompliance, missing 11 fields) for its 7 consumers. - Model patches.pending / alerts.critical / system.rebootRequired as boolean (equals/notEquals) so the negative case is expressible, matching the backend (was enum{yes}). - Move the v2 on/off flag off the ?filtersV2= query param to #filtersV2Flag (distinct hash key so it never collides with the filter value), per CLAUDE.md. - Operator lists mirror the backend minus numeric between (ValueInput renders between as a date-range only). Add filterUrl tests.
…review Must-fix 1) - Value editor: FilterValueEditor now delegates standard type rendering (string/number/enum/boolean/date/multi) to the shared filters/ValueInput instead of re-implementing it (~125 lines removed). Keeps only the genuinely chip-bar-specific editors (org/site name pickers, software multi-select with counts). This also gains correct boolean (Yes/No) + between handling the chip-bar editor lacked. Added stable data-testids to ValueInput so it's testable; updated the chip-bar test to assert them. - Click-outside: extracted the mousedown handler FieldSelector, FilterAddDropdown and the chip popover each hand-rolled into one shared hooks/useClickOutside; the chip popover's Esc handler now uses the existing useEscapeClose. Not folded together (distinct responsibilities, not duplication): FilterAddDropdown is an add-a-filter affordance vs FieldSelector's edit-this-field control; FilterPreviewFooter fetches a live match count vs FilterPreview rendering a provided result list.
567da46 to
bcc718a
Compare
|
Thanks — all three addressed. Net diff is −62 lines (the changes remove duplication rather than add). Must-fix 2 — single field catalog. New Must-fix 2 — the boolean predicates. Must-fix 1 — compose the filters/ library. Should-fix — flag off query param. The on/off override moved from Two I deliberately did NOT fold together (distinct responsibilities, not duplication): |
The chip-bar preview footer (this PR) calls POST /filters/preview, which fetchWithAuth tags with the current org-scope selection (?orgId=). The handler ignored it and summed totalCount across every accessible org, so a partner user scoped to one org saw an inflated "~N devices match" that disagreed with the org-scoped device list (e.g. 7 vs 6 when a sibling org had a matching device). Honor the pinned orgId: scope to it when present (403 if the caller can't access it), span all accessible orgs only when absent (the "All orgs" scope, which omits orgId). Pre-existing in the LanternOps#1012-era endpoint; surfaced by the chip bar. Regression tests cover the scoped count + the foreign-org 403. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed a small API fix to the branch ( Symptom: the preview footer's "~N devices match" over-counted vs the filtered list — e.g. ~7 vs 6 for Root cause: pre-existing in the Fix: honor the pinned Verified locally: API now returns 6 with Re-reviewing for merge once CI re-runs. |
# Conflicts: # apps/api/src/routes/filters.ts
… filters (#1195) **Root cause:** Verification of the v0.69.0..main Codex review confirmed two bugs in the advanced device filters (#1013): 1. `DeviceList` resolved the advanced filter via `POST /filters/preview` with `limit: 100` (the API schema also caps `limit` at 100) and used the returned ids as the authoritative client-side filter for the **main device table** — not a preview count. Any filter matching >100 devices (trivial at the 10k-agent target) silently showed at most 100 rows, and which 100 depended on per-org query order. The API's correct `totalCount` was ignored. 2. Grid view mapped the raw `devices` array (`DevicesPage.tsx:793`) while the active filter chip bar rendered above it — the UI displayed active filters over an unfiltered fleet. **Fix:** - `POST /filters/preview` gains an `idsOnly: true` mode returning the complete uncapped matching id set (ids only — no per-device enrichment; `evaluateFilter` applies no row limit so the per-org `previewLimit` never truncates). The capped enriched preview is unchanged for its existing caller (filter-builder footer). - New shared `useAdvancedFilterIds` hook resolves the filter once at page level; the list view receives the id set via props (replacing its internal capped fetch) and the grid maps the same filtered set. Quick-filters (search/status/os/…) deliberately stay list-only. - `runActionAllowlist`: DeviceList removed from the migration backlog (its only fetch moved into the hook). **Tests:** API — idsOnly returns 250/250 ids uncapped, partner multi-org aggregation, capped path still trims 150→100 with true `totalCount`, `limit > 100` still 400 (17 pass, plus filters_crud + filterEngine 48 pass). Web — hook (5), DeviceList prop filtering, DevicesPage grid/list parity (16 pass), `no-silent-mutations` 24 pass. Web `tsc --noEmit` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
PR 2 of 3 for the chip-bar device filter (#968). The web UI, on top of #968 PR 1's completed
filterEngine.What it does
A NinjaOne-style chip bar on
/devices. It builds aFilterConditionGroupand feeds it through the existingDeviceListserverFilter→POST /filters/preview→matchingIdsclient-intersection path that's already onmain— no new device-list query, no change to the keyset cursor./filters/preview.#filtersV2=<base64>hash + thefiltersV2flag: default ON, opt out via?filtersV2=0orlocalStorage['breeze.filtersV2']='off'. Per your Proposal: chip-bar filter UI for /devices — working prototype in production #968 note, the flag + legacyDeviceFilterBarstay one release window, then get removed.advancedFilterstate.Deferred (by your #968 split)
SavedFiltersPanelis intentionally not in this PR.Verification (local, node 22)
astro check0 errors (775 files); full web suite 653 passing; eslint clean.🤖 Generated with Claude Code