build: second supply-chain dependency reduction pass#804
Conversation
…rser firebase-admin already bundles @google-cloud/firestore and re-exports its entire type surface (plus an app-aware getFirestore()) from firebase-admin/firestore. The server also pinned @google-cloud/firestore directly at ^8.3.0, which resolved to 8.5.0 alongside the 7.11.6 that firebase-admin ships -- and the two diverge below google-gax (4.x vs 5.x), dragging parallel copies of google-gax, the grpc proto loader, node-fetch, and friends into the tree purely because of the separate pin. Retarget the two import sites (db-firestore.ts, table-firestore.ts) at firebase-admin/firestore and swap `new Firestore()` for `getFirestore()` (the admin app is already initialized in app.ts before the database is constructed). The runtime API used here -- collection/doc/where/ runTransaction, FieldPath.documentId() -- is stable across 7.x/8.x. cookieParser() was registered as middleware but nothing ever reads req.cookies or req.signedCookies: the seshcookie session library parses the Cookie header itself. Remove the dead middleware (and its mirror in the session-auth test) along with cookie-parser + @types/cookie-parser; that also sheds the duplicate cookie-signature@1.0.6 copy.
criterion was pulled with its default features on plus html_reports, which enables the plotters HTML-report backend. We only ever read the terminal summary from benchmark runs (the regenerable harnesses report in-chat), so switch to default-features = false with just cargo_bench_support. That drops plotters, plotters-backend, plotters-svg, and web-sys from the lockfile and stops compiling the plotting stack for `cargo test`/bench builds. simlin-mcp-core declared tracing as a direct dependency but never logged or spanned anything; remove it. tracing stays in the tree via simlin-serve and rmcp, so this only corrects a misleading direct edge.
…deps Replace five third-party UI dependencies with small owned implementations, keeping each component's public API and existing tests as the contract. This evicts the @floating-ui/popper positioning stack and downshift entirely. - Accordion: a context-backed single-item disclosure. The open/close height animation is a JS-free grid-template-rows 0fr<->1fr transition, replacing the Radix keyframes that depended on --radix-accordion-content-height. Adds an accordion test (there was none). - Checkbox: a native <input type="checkbox"> styled with appearance:none and a CheckIcon overlay, preserving the data-state and primary/secondary class contract the test pins. - Autocomplete: a new owned useCombobox hook (filter-as-you-type listbox with arrow/enter/escape keys and WAI-ARIA combobox attributes) replaces downshift's useCombobox; the component body is otherwise unchanged. Opens on click/type/arrow to match the wiring-editor selects. - Menu: an owned fixed-position portal positioned from the live anchor rect (the useAnchorRect issue-#710 logic is unchanged), with Escape and click-outside dismissal and close-on-select via context. Drops Radix dropdown-menu (and with it the @floating-ui stack). - SpeedDial action tooltips: a CSS-only hover/focus tooltip (left-pointing arrow, 300ms show delay) replaces Radix tooltip. Removed deps: @radix-ui/react-accordion, @radix-ui/react-checkbox, @radix-ui/react-dropdown-menu, @radix-ui/react-tooltip, downshift. The Radix dialog/tabs/toast components stay (accessibility-sensitive, still used).
The grid-rows height animation keeps the panel content mounted while closed, which left it keyboard-focusable and exposed to assistive tech -- a regression from Radix's `hidden`. Mark the collapsed content subtree `inert` so it is removed from the tab order and a11y tree while still laid out for the transition.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c313ca9327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aria-labelledby={triggerId} | ||
| className={clsx(styles.details, className)} | ||
| style={style} | ||
| data-state={open ? 'open' : 'closed'} |
There was a problem hiding this comment.
When open is false this panel is only collapsed visually via data-state, but the region and all of its children remain mounted without hidden, inert, or aria-hidden. In the New Project form the collapsed Advanced section contains a checkbox, so keyboard users can tab into an invisible control before reaching Create; keep the closed content out of the accessibility tree/tab order while preserving the animation.
Useful? React with 👍 / 👎.
[P2] Re-clicking the trigger no longer dismisses the menu (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
=======================================
Coverage 90.83% 90.84%
=======================================
Files 224 224
Lines 142627 142627
=======================================
+ Hits 129562 129563 +1
+ Misses 13065 13064 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review SummaryI reviewed the diff for code-correctness bugs and didn't find any blocking issues that would warrant changes. The PR is a careful supply-chain reduction with the existing component tests acting as the contract for the reimplementations, plus a new Accordion test where coverage was missing. A few things I specifically verified:
Overall correctness verdictCorrect. I see no behavioral regressions, no broken contracts, and the new tests look adequate for the components introduced. |
- useCombobox: set the field text to the chosen item on selection. Without it an uncontrolled Autocomplete (one that does not feed `value` back in) kept showing the partial text the user typed instead of the selection -- a behavioral regression from downshift. - Menu: restore the keyboard/focus affordances Radix provided. Move focus to the first item on open (the menu portals to end-of-body, so otherwise a keyboard user must tab the whole page to reach it); Up/Down/Home/End rove between enabled items; Escape closes and returns focus to the anchor; focus leaving the menu (tab-out) dismisses it. Also exclude the anchor from outside-press dismissal so a trigger press no longer triggers a redundant close->reopen and a toggling trigger can work. - Accordion: mark the collapsed region itself (not just its inner wrapper) inert + aria-hidden, so a closed panel is not announced as an empty region. Adds Menu dismissal/keyboard tests, an uncontrolled-select Autocomplete test, and updates the Accordion tests for the region-level hiding.
|
Reviewed the PR end-to-end. No blocking issues found. Verified correctness of the reimplemented components:
Server changes are safe:
Build changes:
Overall correctness: correct. |
- db-firestore: getFirestore() resolves against the default Firebase app and throws app/no-app if none is initialized. App bootstrap calls admin.initializeApp() first, but standalone callers (scripts/debug-import.mjs) reach createDatabase without it -- the prior new Firestore() needed no app. Initialize a default app on demand when none exists; reuse the existing one otherwise (no double init). Adds a db-firestore unit test for both paths. - useCombobox: scroll the keyboard-highlighted option into view. Focus stays on the input (combobox pattern), so arrowing past the fold of a list taller than the 300px popup moved the highlight off-screen with nothing scrolling, which downshift handled. `block: 'nearest'` only scrolls when the row is off-screen; the call is optional-chained since jsdom lacks scrollIntoView.
The blur handler exempted the anchor so a mouse press on the trigger would not flicker-close the menu, but that also wrongly kept it open when a keyboard user moved focus back to the trigger (Shift+Tab) -- focus had genuinely left the menu yet it stayed open. Distinguish the two with a transient skip-blur-close flag set during pointer interactions (owned by the mousedown listener) and around Escape's focus restore, so the blur handler closes on a genuine keyboard focus-out -- including Shift+Tab to the trigger -- while mouse/Escape focus moves still do not double-close. Adds a Shift+Tab-to-trigger regression test.
Two related blur-close bugs in the owned Menu: - The skip-blur-close flag was stranded true after an outside-press dismissal: that path sets the flag and calls onClose synchronously inside the mousedown listener, which tears down the mouseup reset before it fires. The flag then survived to the next open and suppressed a genuine keyboard focus-out. Clear it on open so every session starts clean. - A null relatedTarget was treated as "stay open", but Tab from the only/last item blurs to browser chrome with relatedTarget null -- focus has left the menu, so it must close (Radix did). Treat null as outside unless the skip-blur-close flag is set. Adds regression tests for focus-out to browser chrome and for keyboard focus-out after a prior outside-click dismissal.
|
Reviewed the dependency-trim changes. No blocking issues found. The firestore retarget is well-handled: The four reimplemented diagram components preserve their public APIs and their existing tests as the contract:
The criterion trim correctly keeps Verdict: correct. |
The proxy span is rendered by our own Menu now, not anchored by Radix; correct the getProxySpan helper comment accordingly. No behavior change.
|
Reviewed the diff end-to-end (server firestore retarget, cookie-parser removal, criterion trim, tracing removal, and the four reimplemented diagram components). The reimplementations carefully preserve the prior public API shapes and the tests give strong contractual coverage: Menu's positioning + dismissal + keyboard edge cases are all pinned; Accordion's inert/aria-hidden + controlled/uncontrolled toggle is covered; Autocomplete's external-value sync, portal repositioning, and clearOnEscape are tested. The The server retarget (admin's bundled firestore via No correctness issues to flag. Overall correctness: correct. |
When a keyboard user typed, arrowed to highlight an option, then tabbed away, the blur handler closed the list and cleared the highlight without selecting -- so they lost the choice they navigated to unless they pressed Enter. downshift accepted the highlighted item on a keyboard-driven blur. Restore that: on blur, if an option is highlighted and the blur is not caused by a pointer press, commit it. A mouse-driven blur (clicking elsewhere to dismiss) still just closes, so clicking away never accidentally selects. A small always-on pointer tracker distinguishes the two without the teardown-stranding race.
Code ReviewI reviewed this PR end-to-end with focus on the diagram component reimplementations, the firestore migration, and the criterion trim. The work is thorough, the tests are comprehensive, and patches 4-9 already iterated through multiple rounds of feedback on the owned components. I didn't find any blocking bugs. Notes (non-blocking)
OverallCorrect. The PR achieves its stated goal (4 Cargo + 26 pnpm packages dropped, plus the firestore/gax tree dedupe), preserves the public API contract of each replaced component, and ships matching tests for new code paths. The Menu blur/focus state machine (the trickiest piece) is rigorously covered by tests that pin the edge cases the patches discovered. 🤖 Generated with Claude Code |
ReviewI focused on the four reimplemented [P2]
|
…omment The keyboard-blur commit relies on a flag that says "this blur was caused by a mouse press." The mousedown/mouseup pair could strand the flag true if the mouseup was never delivered (release outside the window), which would then make the next genuine keyboard tab-away fail to commit. Replace it with a single mousedown listener that defers clearing to the next task: the click-induced blur fires synchronously in the mousedown's task, so it still observes the flag, but the flag can never linger regardless of where the release lands. Also correct a stale "downshift's getInputProps()" comment in TextField (it is the owned combobox getInputProps() now).
ReviewI reviewed this dependency-trim PR (criterion plotters stack, The reimplementations are unusually well-tested: the iterative follow-up commits ( The Accordion's grid 0fr-to-1fr height trick plus No blocking findings. Overall correctnessCorrect. |
Option presses only prevented the focus-stealing blur via onMouseDown. On touch, focus can shift on pointerdown (before the synthesized mousedown), so the input blurred and the listbox closed/unmounted before the tap's click landed -- mobile users couldn't select autocomplete options. Also prevent default on the option's onPointerDown, which fires first for both touch and mouse, before any focus shift. onMouseDown is kept for the mouse compatibility-blur path.
|
Reviewed the diff focused on correctness/maintainability. No blocking bugs found. The reimplementations carefully address the Radix → owned-component migration:
Overall correctness: correct. |
The "is this blur caused by a press?" guard that suppresses auto-committing the highlighted option was set only from mousedown. On touch, tapping outside a focused autocomplete moves focus and fires the input blur BEFORE the synthesized mousedown, so the flag was still false and the blur was treated as a keyboard tab-away -- committing the highlighted row when the user meant to dismiss. Listen on pointerdown instead: it fires for both mouse and touch and, on touch, before the focus shift, so the flag is set when the blur fires. The deferred self-reset still applies, so it can never strand true.
Every MenuItem had tabIndex=0, so in a multi-item menu Tab moved between items (and the blur handler ignores within-menu focus moves), trapping keyboard users in the menu instead of leaving it -- a regression from Radix's roving focus. Give items tabIndex=-1: the menu already focuses the first item on open and moves focus with the arrow keys programmatically, and with nothing tabbable Tab/Shift+Tab now leave the menu and the resulting focus-out dismisses it.
ReviewI traced through the Firestore migration, the Criterion/tracing trims, all five reimplemented diagram components, and the new Highlights of what I verified:
Overall correctness verdictCorrect. The patch should not break existing code or tests. The reimplementations preserve the public API and the existing component tests as the contract, and the new tests cover the trickier corners (uncontrolled-select fill, scroll-into-view, keyboard-vs-pointer blur, Escape focus restore, Shift+Tab to trigger, null- |
|
Reviewed the dep-reduction re-implementations against their preserved behaviors. The drops ( One latent edge case worth noting: [P3] Menu loses live re-positioning on window resize when the anchor rect doesn't changeFile: The proxy/content position math for Overall correctness verdictCorrect — the PR is safe to merge. No bugs found that block release, and the re-implementations preserve all existing tests (1195 diagram tests cited in the description). The Menu edge case above is latent and worth tracking but does not affect the current call site. |
The outside-press dismissal listened on mousedown. Surfaces like the diagram canvas call preventDefault() on pointerdown for pan/drag, which suppresses the compatibility mousedown -- so pressing such a surface while a menu was open could leave it stuck open. Listen on pointerdown in the capture phase (matching Radix's outside-interaction handling): pointerdown fires for mouse and touch and, in capture, runs before the target's own handlers regardless of what they do. The skip-blur-close pairing moves to pointerup accordingly.
Follow-up to the pointerdown/pointerup dismissal switch: the open-effect strand-reset comment and two menu-test comments still described the old mousedown/mouseup listeners. Align the wording with the code. No behavior change.
|
Reviewed the diff focusing on the reimplemented components and the server-side surgery. Overall correctness: looks correct. A few low-priority observations, none rising to a bug worth flagging as a fix:
The reimplemented No blocking findings. |
Review: PR 804 - second supply-chain dependency reduction passI reviewed the substantial set of changes: removal of the FindingsI did not find any blocking bugs introduced in this commit. The work appears carefully executed:
Overall correctness verdictCorrect. The changes preserve existing public API contracts, are accompanied by new tests where behavior was reimplemented (Accordion, additions to Autocomplete and Menu, db-firestore on-demand init), and the dependency removals are appropriately justified. No actionable bugs to flag. |
The Autocomplete list only closed via input blur. A press on a surface that prevents the focus change -- e.g. the diagram canvas, which preventDefault()s pointerdown for pan/drag -- never blurs the input, so the portaled listbox could stay open over the canvas. Add a capture-phase outside-pointerdown handler that closes the list when the press lands outside the field and the listbox (matched by id, since the listbox is portaled), independent of blur -- the dismissal behavior downshift provided.
Code review for PR #804I reviewed the supply-chain reduction changes across the server (firestore retarget, cookie-parser removal), the diagram component reimplementations (Accordion, Autocomplete + The retargeting work is careful and well-scoped:
One minor behavior change worth noting (not a bug, but a subtle UX divergence): in the new Overall correctness: correctThe patch should not break existing tests, and I did not identify any blocking bugs. The behavior change above is the only thing worth a second look. |
Review cycle completeBoth automated reviewers (codex and the Claude The bulk of the work hardened the owned reimplementations to true downshift/Radix parity. The Menu gained real focus management — focus moves into the menu on open, arrow/Home/End roving with |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
A follow-up to PR #719: a fresh attribution audit of both lockfiles (Rust workspace crates + all JS packages), verifying actual call sites before cutting. Everything removed is dead weight, a redundant pin, or replaceable by a small amount of tested code we own.
The headline counts undersell it: the biggest wins are deduplicating parallel transitive trees (firestore/gax) and evicting the
@floating-ui/popper positioning stack, both of which trade many duplicated/heavy packages for owned code.server: redundant firestore pin + dead cookie-parser
firebase-adminalready bundles@google-cloud/firestoreand re-exports its full type surface (plus an app-awaregetFirestore()) fromfirebase-admin/firestore. The server also pinned@google-cloud/firestore@^8.3.0, which resolved to8.5.0next to firebase-admin's7.11.6; the two diverge belowgoogle-gax(4.x vs 5.x), dragging parallel copies ofgoogle-gax,google-auth-library, the grpc proto loader, andnode-fetchinto the tree. Retargeted the two import sites atfirebase-admin/firestoreand swappednew Firestore()forgetFirestore()(the admin app is already initialized before the DB is built). The runtime API used (collection/doc/where/runTransaction,FieldPath.documentId()) is stable across 7.x/8.x.cookieParser()was registered as middleware but nothing readsreq.cookies/req.signedCookies(seshcookie parses the Cookie header itself). Removed it +@types/cookie-parserand the duplicatecookie-signaturecopy it pulled.build: criterion plotters trim + unused tracing
criterionis nowdefault-features = false, features = ["cargo_bench_support"], dropping theplotters/plotters-backend/plotters-svg/web-sysstack (we only read the terminal summary; the bench harnesses report in-chat). Removed the declared-but-unusedtracingdependency fromsimlin-mcp-core(Closes #803).diagram: reimplement 4 UI components, drop 5 deps
Replaced five third-party UI dependencies with small owned implementations, keeping each component's public API and its existing tests as the contract. This fully evicts the
@floating-ui/*+react-popperpositioning stack anddownshift.grid-template-rows0fr<->1frtransition (no more Radix--radix-accordion-content-height). Adds an Accordion test where none existed.<input type="checkbox">(appearance:none) with a CheckIcon overlay, preserving thedata-state+ primary/secondary class contract.useComboboxhook (filter-as-you-type listbox, arrow/enter/escape keys, WAI-ARIA combobox attributes) replaces downshift'suseCombobox; the component body is otherwise unchanged.useAnchorRectissue-diagram: Menu anchors Radix to a position:fixed proxy holding a stale getBoundingClientRect snapshot #710 logic is unchanged), with Escape + click-outside dismissal and close-on-select via context. Drops Radix dropdown-menu (and with it@floating-ui).Radix
dialog/tabs/toastdeliberately stay -- they carry focus-trap/ARIA correctness that is not worth re-owning. All 1195 diagram tests pass.Deliberately kept (audited, not worth touching)
ignore,calamine,notify-debouncer-full,jsonschema-- big headline closures evaporate under inverse-dependency analysis (shared withtracing-subscriber/salsa/loro) or are already correctly feature-gated.twox-hash/smallvec/rustc-hash/indexmapadd zero net crates.helmet/@iarna/toml(zero deps),cors(one tiny pkg),katex(one call site, but deps only oncommander-- weight is fonts not tree),slate,@firebase/*,clsx,wouter-- load-bearing or zero-closure.Verification
Every commit passed the full pre-commit battery (rustfmt, clippy, cargo test, eslint, build, tsc, JS tests, pysimlin). New/reimplemented components keep their existing component tests green; the firestore retarget and cookie-parser removal were validated against the server suite (111 tests) and tsc.
Closes #803
🤖 Generated with Claude Code