feat(fds): replace the left navigation with the design-system Navbar (#17539) - #17514
Conversation
Replaces LeftBar.jsx, LeftBarItem.tsx and LeftBarHeader.tsx with the Navbar, NavbarItem, NavbarSubmenu and ProductSwitcher components from @filigran/design-system, pinned at the head of the library's main. The menu tree, which was an untyped inline JSX tree mixing three permission mechanisms with rendering, becomes a typed structure built by useNavMenu, with a pure filterNavGroups that is unit-tested on its own. Navigation rows are now real anchors, so Ctrl-click and "open in a new tab" work on every entry. The rail widths, the collapsed-state storage key and its notification channel move to navBarConstants. Seven floating toolbars and the end-to-end page object are retargeted onto it; the collapsed width goes from 55px to 48px, the design-system rail's real width. Installing a private library inside a container needs the credential to cross the container boundary, so the three Dockerfiles take it as a BuildKit build secret and the workflows that install outside an image inject it explicitly. No token is written to any image layer. The design-system theme class is now written on the document root: the library portals its flyouts and tooltips to document.body, so a subtree-scoped class would leave them unthemed. Custom themes keep their background and their accent colour; both compensations are documented in fds-migration/LIBRARY-FEEDBACK.md with a testable removal condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Filigran Automation (Filigran-Automation)
changed the title
Filigran Automation (Filigran-Automation)
added
the
filigran team
|
🤖 [AI-generated] Hey Sandy Ghs (@sandyghs)! 👋 Thanks so much for opening PR #17514 — this is an impressively thorough writeup of a big change, really appreciate the detail on the compensations, validation and pinned library version! 🙏 I just had a quick look and there's one thing that could help reviewers and the release process a little. I haven't changed anything in your description — just a gentle suggestion:
No rush at all — thanks again for this contribution, the navigation migration looks like solid work! 🚀 |
…flow The main continuous-integration workflow builds the platform image with the same composite action as the delivery workflows, so it needs the same credential. It was the one caller left unwired, and a composite action cannot read the secrets context itself. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sandy Ghs (sandyghs)
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## design-system/current #17514 +/- ##
=========================================================
+ Coverage 34.07% 34.15% +0.07%
=========================================================
Files 3379 3380 +1
Lines 137610 137528 -82
Branches 37229 37180 -49
=========================================================
+ Hits 46895 46966 +71
+ Misses 90715 90562 -153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Running the product surfaced three defects that no type-check, lint or unit test could see, because none of them computes layout: - the rail shrank from 48px to 22px: the MUI Drawer it replaces carried `flex: 0 0 auto`, the library's <nav> carries only its width utility, and the private layout is a flex row that overflows; - top-level labels overflowed the collapsed rail: `asChild` replaces the library's row body, including the `sr-only` switch it applies to the label while collapsed. Reproduced verbatim so the accessible name survives; - the selected-row tint ignored a custom `theme_primary`: it comes from a token the stylesheet DERIVES from the brand colour on `:root`, where the substitution happens, so the override on the <nav> never reached it. Both derived tokens are now re-derived alongside the base one. The accent guard is reworked accordingly: it now extracts every custom property the installed library resolves for `aria-current` rows and asserts the rail overrides each one. The previous substring form passed while the tint was visibly wrong. Also names the logo link after the platform title: it pointed at /dashboard under the name "Home", colliding with the "Home" row and making an exact accessible-name lookup ambiguous for screen readers and for the e2e model. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Both survived the page-object re-anchoring because neither goes through it:
- a report spec clicked `getByTestId('ChevronRightIcon')`, the MUI icon inside
the old rail's expand button, to expand the rail. It now calls the page
object's `open()`, like every other rail interaction in the suite;
- the widget page object located perspective cards page-wide by label. The
library's submenu is an accordion, which keeps a closed panel mounted as a
hidden region labelled by its parent, so `getByLabel('Entities')` matched the
navigation as well as the card. Both lookups are now scoped to the dialog the
cards actually live in.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
count() does not auto-wait, so probing the parent row's role right after a dialog closes read a page still marked aria-hidden, answered zero, and fell into the collapsed branch on an expanded rail, where it waited 200 seconds for a link that cannot exist. Wait for either role to be visible first, then discriminate on the role: the parent is a button when the rail is expanded and an anchor when it is collapsed, and both carry aria-expanded. In the collapsed flyout the library's Radix menu re-roles the submenu anchors to menuitem, so look them up there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The replaced rail exposed its logo as a link named "logo", from the image's alt text, and two settings specs read the logo source through that name. The library names the logo link after its destination, which is /dashboard, so it came out as "Home" and collided with the Home row; naming it after the platform title fixed the collision but broke the two specs. Name it "logo", which is both unambiguous inside the navigation and identical to what the product exposed before. The platform title is no longer read. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Three findings from a hands-on pass on the running platform, each solved where it belongs. The rail was neither full height nor fixed: the library lays its <nav> out in flow and sizes it with a percentage height, which does not resolve against OpenCTI's shell. It now sticks to the viewport below the banners with a definite height, the same geometry the OpenAEV pilot uses. The offsets are derived from the numeric banner height: the string form is '0' without a unit, which makes any calc() containing it invalid, and an invalid declaration is dropped in silence. The "Made by Filigran" signature is rewritten as its own component, ported from the OpenAEV pilot: inline geometry, since the product has no Tailwind build, and the wordmark cropped to the Filigran emblem alone when the rail is collapsed, from the same asset. Collapsed, the hover flyout is left uncontrolled. The library drives it from the same props as the expanded accordion, so binding them to the persisted menu state made every flyout after the first unusable and rewrote that state on hover. Four unit tests cover the geometry, the uncontrolled collapsed flyout and the signature in both rail states. Library entries 10 and 11 filed, with their removal tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
70d4d07 to
d4688bb
Compare
Sandy Ghs (sandyghs)
changed the title
ci-docker-build.yml builds the whole platform Dockerfile, so it reaches builder-front and its RUN --mount=type=secret,id=fds_git_token, but it passed no secrets: to docker/build-push-action. An unprovided BuildKit secret is no file at all, so the step dies on "can't open /run/secrets/fds_git_token: No such file or directory" — which reads like an absent repository secret. Its three callers never run on a pull request, one of them on push to design-system/current, so no check here could have caught it. A test now does the enumeration instead of a human: every docker/build-push-action step whose Dockerfile requires the secret, and which does not stop at an earlier target:, must declare it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
filterNavGroups removed a parent once every one of its submenu rows had been filtered out. The rail this replaces did the opposite: LeftBarItem's "No Subitems" branch rendered such a parent as a plain navigable row. Two reachable cases. canSeeData is granted by INGESTION alone, which grants none of the eight Data sub-items, so an ingestion operator lost /dashboard/data entirely. Hiding the Dashboard entity type empties the Dashboards submenu the same way. NavBar.renderItem already renders an empty subItems as a leaf, so the fix is to stop dropping the row. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The library's main has moved four commits on: sub-menu primitives for Menu with ProductSwitcher adopting them, a docs correction, the pointer-cursor fix on rail rows, and a new Header component. The cursor fix is the one this rail needed. Every button-rendered row resolved to `cursor: default` -- 14 of them expanded, 1 collapsed -- because a browser gives `<a href>` the hand cursor for free and gives `<button>` nothing, and the library restated neither. Measured in the running platform, the bump takes that to 13/14 and 1/1. This is a regression against the MUI navigation being replaced, which had the hand cursor on every row; it was reported by the OpenAEV pilot, not by this one, and it survived a full structural parity pass here because no structural diff can see a UA default. The remaining row is the ProductSwitcher trigger, a sibling component and so not a NavbarItem. It is filed as entry 12 in LIBRARY-FEEDBACK.md and deliberately not compensated product-side: a host rule would hide the gap from every other consumer. No compensation could be retired. Navbar.tsx and NavbarSubmenu.tsx are byte-identical between the two pins, `NavbarProps` still has no accent prop, and `NavbarSubmenuProps` still has a single open/onOpenChange pair. Nothing this rail depends on moved: set-diffing the compiled dist/index.css between the pins gives 641 -> 689 class selectors and 414 -> 420 custom properties with zero removals, so the accent guard that reads the installed stylesheet still holds. One intentional visual change lands inside the product dropdown, where ProductSwitcher now uses Menu's own metrics. The install grows 3 384 759 -> 3 637 106 bytes. No reduction was available from dropping the documentation: the package has shipped `files: ["dist"]` since the initial scaffold, an ancestor of both pins. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Update: I've bumped the design system pin on this PR, following feedback the OpenAEV team gave on the equivalent migration on their side. Since both products consume the same library, their review benefits this PR directly. What the feedback found. Rows in the rail that render as buttons — the collapse toggle and the submenu triggers — didn't show the pointer cursor on hover, while rows rendering as links did. The browser gives links that cursor for free and gives buttons nothing, and the library ships no global reset, so nothing caught it. The old navigation, built on MUI, had it everywhere. Small, but a real iso-functionality regression: measured at 0 of 14 button rows before the bump, 13 of 14 after. It was fixed in the library rather than patched here, so both products get it from the same place. Also in this bump, since the pin moved forward: the ProductSwitcher now renders through the library's shared Menu. That changes its dropdown slightly — right padding 8→16px, minimum width 192→200px, separator 2→4px, group header 24→32px — with row height and label size unchanged. Nothing else moves: a set-diff of the compiled stylesheet between the two pins shows 641→689 class selectors and 414→420 custom properties, with zero removals on either side. |
Natalie (natalie-natsu)
left a comment
Natalie (natalie-natsu)
left a comment
There was a problem hiding this comment.
Lots of changes. It's nice to have tests. It will require another review on the components because for me most parts were ok, it is a lot of noise with these comments.
However, I can't approve anything before getting clear with the "FDS_GIT_TOKEN". How does it works with OpenAEV? I don't understand how we aren't soft locking projects with that. A. Jard (@aHenryJard) have you seen this part?
| secrets: | | ||
| fds_git_token=${{ inputs.fds_git_token }} |
There was a problem hiding this comment.
What happens if you don't have the FDS_GIT_TOKEN? If a contributor wants to fork the project could he build it?
There was a problem hiding this comment.
You're right to flag it — it's a real constraint today: the library is a private git dependency, so a fork can't build the front (in CI or locally). Two things frame it: no external contributor is affected yet, because fork PRs target master while this work lives on design-system/current; and the token was always an interim consumption mode. The structural fix is already planned — publishing the library to npm (workstream s5-release-machinery), which removes both the token and the private clone.
Review feedback on #17514 was that the explanatory comments around the workarounds are noisy enough to obstruct a component review. Each of the eight workaround sites now carries a single marker naming its removal condition and pointing at its numbered entry: // FDS-WORKAROUND #N: <summary> — remove when <condition> — see fds-migration/LIBRARY-FEEDBACK.md #N The rationale is not dropped, it moves to the matching entry in fds-migration/LIBRARY-FEEDBACK.md, which becomes the single source of truth: full why, the code shape, and the testable removal condition. That also ends the duplication, where the same reasoning lived in two places and only one of them was checked by anything. Entry 5 (portalled surfaces) gains the removal test it never had, plus the Radix inline-z-index and MUI zIndex.modal rationale. Entries 1, 2, 6, 7, 8, 10 and 11 gain the code shapes, measured figures and the OpenAEV reference implementation path that were only in the source before. The MUI icon-sizing rule and the conditional custom-theme background are deliberately left with their inline rationale: neither is a workaround, and marking them removable would misrepresent them. Comment-only. Verified mechanically by stripping all comments from both files before and after: the remaining code is byte-identical, so there is no behaviour change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fair point, and you were right to raise it. I've done a cleanup pass: each workaround is now a single line pointing at its numbered entry in fds-migration/LIBRARY-FEEDBACK.md, where the full context and the removal condition now live. These markers are temporary by design — each one gets deleted along with its workaround once the library ships the corresponding fix, and some of those fixes already landed this week. Nothing essential was lost, just the noise. |
Natalie (natalie-natsu)
left a comment
Natalie (natalie-natsu)
left a comment
There was a problem hiding this comment.
I accept only to unlock the PR, because this is the same action than a local merge commit. In this current state, we can't approve any request other than POC.
3 participants
What changed, in plain terms
The left navigation is now the design system's
Navbar, not a hand-built MUIDrawer. Same entries, same permissions, same destinations — but the rows arereal links, so Ctrl-click and "open in a new tab" work everywhere, and the rail
is painted by the design system instead of by ~870 lines of product CSS.
Four things a user can notice:
system's real rail width; the seven floating toolbars that align themselves
against it read the constant, so they shift 7px with it.
theme_backgroundstill paints the rail, a
theme_primarystill colours the selected row, anda draft still turns the accent orange.
than a reimplementation of it.
rail is expanded, the emblem alone when it is collapsed — cropped from the
same asset, so there is no second file to keep in sync with the brand.
Proposed changes
LeftBar.jsxand its ~870 lines of styling)with
NavBar.tsx, built on the design system'sNavbar,NavbarItem,NavbarSubmenuandProductSwitcher, keeping the menu structure, the threepermission mechanisms, the feature flags and the
submenu_show_iconsuserpreference.
secrets for the three Dockerfiles, explicit per-run injection for the
workflows that install outside an image.
emits, and keep the rail's width contract for the eight files that read it.
Related issues
Pinned library version
486cec92c3abf006997ac269d34ff0fcc23f178f— head ofmain. Verifiedinstallable from a clean external project and from inside a container
before being pinned.
Bumped from
56f7e59823cae7d815a451206e3cb4cb1d31022dto pick up libraryPR #84, which gives button-rendered rail rows the pointer cursor. Before it,
every button-rendered row of this rail resolved to
cursor: default— 14expanded, 1 collapsed — while link rows got the hand cursor for free from the
user-agent sheet. That was a regression against the MUI navigation being
replaced, and no structural diff can see it. After the bump: 13/14 and 1/1.
The one row left out is the
ProductSwitchertrigger, which is a siblingcomponent rather than a
NavbarItem. Filed as observation 12 and deliberatelynot compensated here — a host rule would hide the gap from every other
consumer.
Re-checked at the new pin, and recorded in
fds-migration/IMPLEMENTATION-LOG.md:Navbar.tsxandNavbarSubmenu.tsxarebyte-identical between the two pins;
NavbarPropsstill exposes no accentprop and
NavbarSubmenuPropsstill exposes a singleopen/onOpenChangepair.
dist/index.cssbetween the pins: 641 → 689 class selectors, 414 → 420custom properties, zero removals on either — so the accent guard that
reads the installed stylesheet still holds.
PR Integrate with multiple MISP Instances #81 moved
ProductSwitcher's panel ontoMenu, so row padding-right goes8px → 16px, panel min-width 192px → 200px with a new cap and bounded height,
divider margin 2px → 4px, group header band 24px → 32px, and disabled rows
are painted with the disabled tokens instead of
opacity-60. Row height(36px) and label size (14px) are unchanged — checked in the running platform.
(+252 347, +7.5%), 60% of it source maps and typings for the new
HeaderandMenuprimitives. There was no documentation to drop: the package hasshipped
files: ["dist"]since the initial monorepo scaffold, an ancestor ofboth pins.
The point of this pull request
Beyond the navigation itself, this is the first time OpenCTI's continuous
integration installs the private design system. OpenCTI installs inside
containers — three Dockerfiles and four workflows — and a credential placed
on the host does not cross the container boundary. So:
only for the duration of the
yarn installlayer and never written to animage layer (verified: no match in image history, in any gitconfig, or
anywhere on the filesystem of the built image);
container run, with tracing disabled around the credential;
gitis installed where it was missing (node:22-alpineships without it),including in the shared
builderstage, because that image is exported as anartifact and reused by the rolling build.
Compensations
Eight, each carrying a comment explaining why, an entry in
fds-migration/LIBRARY-FEEDBACK.md, and a testable removal condition.<nav>(entries 1, 6)NavbarItempaints its selected state from a fixed brand token, and the derived tint token is frozen against:root; OpenCTI'stheme_primaryis administrator-configurable and drafts switch the accentNavbargains an accent prop — pass the same value, delete the block; the token-name guard test passes unchangedasChild— required for real links — makes the library'sicon/showIconinert, sosubmenu_show_iconshad to be re-implementedicon/showIconagainsr-only(entry 7)asChildreplaces the library's own row content, including the hidden label the collapsed rail is navigated byasChildcomposes the row body instead of replacing itopen/onOpenChangebound only while expanded (entry 10)selectedMenuis unchanged<nav>is laid out in flow and sized with a percentage height, which does not resolve against OpenCTI's shell: the rail was 776px tall in an 800px viewport and scrolled away with the page<nav>takes the height it is given — delete thenavStylegeometry block, the rail still measures the full viewport and stays put while an inner container scrolls.app-navbar { flex: 0 0 auto }(entry 8)<nav>carriesshrink-0itself — delete the rule, the rail still measures 48/180pxdocument.bodybelow OpenCTI's app baruseFdsThemeScopewrites the theme classes on the document rootdocument.body; a subtree class leaves them unthemed. The hook's guard against targeting the root was amended, not bypassedConditional, not a compensation: the legacy gradient is restored inline
only when an administrator configured a custom
theme_background. Standardinstallations get the real design-system rendering — the point of this pilot —
and custom-theme deployments lose nothing. The branch is commented as a
deliberate choice, not a workaround, and both cases were checked in the running
platform.
Library observations filed
Twelve entries in
fds-migration/LIBRARY-FEEDBACK.md, each with its removaltest. Three were added after hands-on passes on the running platform:
Collapsed,
open/onOpenChangedrive the hover flyout; the 150ms delayedclose of the row being left resolves against the same state snapshot as the
next row's open, so the last one wins.
to write the same sticky, definite-height block in their host. Filed as an
observation rather than a defect: it is arguably the host's responsibility,
but two consumers out of two hit it.
NavbarItem. Library PR INTERNAL_SERVER_ERROR - "Int cannot represent non-integer value:" #84 fixedboth of that component's render paths, which is 13 of this rail's 14
button-rendered rows. The
ProductSwitchertrigger declares no cursor of itsown and still resolves to
default. The ask is to generalise PR INTERNAL_SERVER_ERROR - "Int cannot represent non-integer value:" #84's ownreasoning to every button the library renders.
Nothing in the library was modified by this pull request.
Validation
Run locally on this exact commit: type-check clean, lint clean, 1337 unit
tests passing. Two failures are pre-existing and reproduced identically
with this branch stashed:
src/utils/Time.test.ts— timezone-dependent assertion, fails outside UTC.src/buildConfig.test.ts— requiresdist/index.html, i.e. a prior build.41 unit tests cover the new code and the amended theme hook, including the
behaviours most at risk of a silent regression: a submenu parent navigates
only while the rail is collapsed (as before); the selected-row accent is
still driven by the library token this code overrides — that guard reads the
installed stylesheet, because a rename upstream would otherwise lose the accent
with nothing turning red; and the rail's sticky, full-height geometry.
Continuous integration is green on this commit: 17/17 jobs of
OpenCTI CI— frontend quality, backend, the three end-to-end groups, the client-python
matrix, the license check and the three container image builds, which is the
central proof here — plus
PR conventions checksandPR add labels.PR check linked issueis skipped by design on this base branch(
pr-check-linked-issue.ymlonly arms formasterandlts/*). One red wasdated and re-run:
pycti test - python 3.12failed once on a runner portcollision (
failed to bind host port … address already in use), unrelated to afront-end change, green on re-run.
Note for reviewers:
codeql-analysis.ymlandcd-check-images-on-pr.ymlrestrict their trigger to
master/lts, so they do not run here — apre-existing structural gap of this base branch, already recorded on the
OpenAEV side. Independent review is the only safety net for what they cover.
How to test this PR
destinations identical to
master; Ctrl-click and middle-click open a newtab from any row; collapsed, hover moves from one submenu to the next and
each flyout opens.
theme_background+theme_primaryin Settings →Customization): the rail keeps the configured background and the selected row
the configured accent. Then switch back to a standard theme: the rail is
painted by the design system.
yarn test src/private/components/nav src/utils/hooks/useFdsThemeScope.test.tsx— 41/41 passing.
Not fixed here (mono-subject), dated in
fds-migration/IMPLEMENTATION-LOG.mdchannel, so their offset goes stale. Pre-dates this change.
yarn i18n-checkeris declared inpackage.jsonbut the script file does notexist and no job runs it.
bannerHeightis built as the unitless string'0'when no banner isdisplayed, which makes any
calc()containing it invalid — including the appshell's own
marginTop. This rail derives its offsets from the numeric forminstead; the shell's declaration is left alone.
Checklist