chore(release): consolidate release/1.5 into dev - #1464
Merged
Conversation
…ed 1 container (#1379) * fix(cli): parse both shapes of docker compose ps --format json Current Compose emits a single line holding a JSON array; composePs only handled the older one-object-per-line shape. It failed silently rather than throwing: an array is valid JSON, so JSON.parse of the whole "line" succeeded, .map returned exactly ONE entry, and Name/State/Status were undefined on an array. So `neoboard status` reported "running (1 containers)" no matter how many were up, with empty name/state/status on the typed ContainerInfo that any future caller would trust. Parse the whole output and use it directly when it is an array, falling back to newline-delimited parsing otherwise — works across Compose versions instead of trading one version-specific bug for another. Closes #1369 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): probe /api/health in status, not the auth-gated / getAppHealth curled `/` and treated only 200 as healthy. But `/` is auth-gated, so the sessionless request curl makes is *supposed* to 307 to /login — a correctly working instance reported "unhealthy (HTTP 307)" every single time. There was no state in which that line was right, and it invited operators to go debugging a non-problem or to stop trusting the one command that answers "is it working?". Probe /api/health instead: no session needed, and it is already what docker-compose.full.yml's healthcheck targets, so the CLI and Docker now agree. Because the probe now keeps the body as well as the status code, a non-empty `errors` array reports unhealthy WITH the reason — an app that is up but degraded used to read identically to a healthy one. An unparseable 200 body stays healthy: the app answered, and inventing a failure from a payload we merely failed to read would reintroduce the same false negative. Closes #1368 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…yout (#1375) (#1378) * fix(component): a save on a narrow window permanently squashed the layout (#1375) One layout is stored per dashboard page, but `ResponsiveGridLayout` was handed that single layout with four different column counts (lg:12, md:10, sm:6, xs:4). Below `lg` the grid clamps every item into the narrower count — and `onDragStop` hands the clamped result back, which is then persisted as THE layout. The authored 12-column arrangement got overwritten by its own squashed projection, and since nothing ever widened it again, each save on a narrow window ratcheted it further toward a single column. Irreversibly: the stored layout is the only one there is. The container is the viewport minus the sidebar, so a 1280px window already measures below the lg:1200 breakpoint. This fired on ordinary laptops, which is why it read as intermittent — "sometimes when saving". Fix: one column count at every breakpoint. If you store one layout, you have to author at one column count; the grid now scales instead of reflowing, so a drag can never return fewer columns than it was given. Responsive stacking, if it is ever wanted, needs somewhere to store per-breakpoint layouts — it cannot be a side effect that destroys the only layout we keep. The existing `handleUserLayoutChange` guard is untouched and was never sufficient here: it correctly stops incidental reflow from persisting, but a real drag at `md` is a real user drag and still returned a 10-column layout. It fixed the trigger, not the projection. Deliberately NOT added: a store-level "a save must never reduce the layout's column span" guard, which the issue proposed. At the store layer a user legitimately resizing widgets narrower is indistinguishable from a breakpoint projection, so that rule would reject real edits. The breakpoint context only exists in the grid, which is where this belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(component): assert the exact breakpoint→cols mapping, not just that values agree CodeRabbit was right: `new Set(Object.values(cols)).size === 1` is satisfied by `{ lg: 12 }` alone, so the test would have passed a fix that mapped only `lg` and left md/sm/xs unmapped — which is the bug itself. Exact object assertions close that hole, and the narrow-container case now pins the measured width too, so it cannot silently stop exercising the sub-lg path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…browser (#1380) DialogContent centres with translate(-50%,-50%), but tailwindcss-animate's `enter` keyframe is `from`-only and `exit` is `to`-only, and both build a single `transform` from --tw-enter/exit-translate-x/y, which default to 0. With no slide-* utility setting them, every dialog interpolated between translate3d(0,0,0) — its top-left corner on the centre anchor — and the resting translate(-50%,-50%): flying in from the bottom-right on open, back out to it on close. zoom-in-95 did not remove a slide, it introduced one. Add the four slide-*-1/2 compensation classes, matching the sibling AlertDialog which still had them and was never broken. `1/2` on both axes rather than upstream shadcn's top-[48%]: 48% is a deliberate 2%-of-height rise, whereas -50% on both axes keeps the visual centre mathematically invariant for the whole animation. The comment block says plainly that these are centring compensation and not motion, since they have now been deleted twice (d723a12, PR #1173). jsdom cannot catch this: no layout engine, no getAnimations(). That is why the old class-presence test — asserting the *absence* of slide-in-from-bottom — passed both before and after the bug. It now positively asserts the four classes, and three new stories scrub the paused animation in real chromium via the previously unused `storybook` Vitest browser project, asserting the box centre never leaves the viewport centre. Worst-sample drift, size=full: 584.0px before, 0.00006px after. Wired into CI as a `test:visual` step; deliberately not into `npm run verify`, where a chromium download does not belong. Closes #1373 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…nt (#1381) Scrolling a graph tile out of view and back re-ran NVL's force layout from scratch, so the nodes landed in a different arrangement each time. LazyVisible unmounted its children unconditionally once they left the viewport, even on a page holding 8 graphs — half the browser's ~16 WebGL contexts — where nothing needed releasing. Mount stays gated on intersection, so a graph-dense dashboard still doesn't build every context on initial load (#1052). Unmount is now gated on a page-wide budget: an off-screen slot only gives up its context once more than WEBGL_WIDGET_BUDGET slots are live, and then only the oldest off-screen ones, one per slot over budget. Closes #1367 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes #1374. The issue's stated mechanism is wrong, and so was the follow-up analysis. Measured in Chromium at 1280x720: empty / 3 lines 173px editing surface, no internal scroll 30 lines 627px, no internal scroll 120 lines 2391px, no internal scroll The editor is neither capped nor starved. It has no definite height at all: `.cm-editor { height: 100% }` resolves against an indefinite-height flex parent, so it computes to `auto` and the column grows without bound with the document. Nothing scrolls internally — what scrolls is the whole left settings column, dragging the Run toolbar, tabs and chart selectors out of view with it. The preview was never the cause either. The modal body is `display: grid` with `minmax(0,1fr) minmax(0,1fr)`, so the panes sit side by side and never compete for height. `flex-shrink-0` on the preview was inert (its flex parent has no definite height, so nothing ever applies shrink pressure) — removed. `h-[500px]` stays: chart and graph renderers measure that container. The toggle collapses the grid to one column, unmounts the preview and swaps the editor to `h-[70vh]`. Measured effect: short query 173px -> 457px height (2.64x), 553px -> 1140px width 120 lines width doubles; scrolling moves into the editor (2391/2391 -> 2391/457) with the toolbar pinned Height cannot grow for a long query: at 720px the modal body is capped at calc(90vh - 180px) = 468px, so ~457px is the ceiling any toggle can offer. The modal itself is the remaining constraint — issue option (c). Notes on the implementation: - The preview is UNMOUNTED, not hidden. ECharts recovers via ResizeObserver and TableRenderer guards on containerHeight <= 0, but NVL's WebGL canvas is fragile at a 0-height mount; `display: none` would keep every renderer alive at 0x0. Unmounting makes the first measurement on the way back correct. - Classes change, the tree does not. QueryEditorPanel is not reparented, so CodeMirror never remounts and the cursor position and undo history survive the toggle. The E2E asserts the live CM6 doc length across both directions. - The toggle lives in the editor header, not the preview header — maximizing hides the preview, so a control there would leave no way back (and #1372 adds its own toggle to that header). - The maximize request is ignored for parameter-select and content-only widgets: they render no query editor, so honouring it would strand the user in a one-column layout with no preview and no control to escape. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…s remounting it (#1370, #1371) (#1387) * chore: ignore the E2E server pid file Every `playwright test` run writes app/e2e/.server-pid for globalTeardown to kill the Next server with. Its sibling .containers-state.json is already ignored; this one was not, so it showed up as untracked after every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(dashboard): clamp a negative ?page= and retire the dead editMode flag (#1371) setLayout clamped only the upper bound, so `?page=-1` reached the store as -1 and rendered a blank dashboard — no page matched the active index. Clamp the lower bound too; `?page=99` was already covered. editMode/setEditMode had zero consumers outside the store and its own tests (#1370 called this out). The URL segment is the mode now, so a store mirror would be a second source of truth that can desync. Removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(dashboard): hoist the dashboard UI into [id]/layout so Cmd-E stops remounting it (#1370, #1371) View and edit were two Next page segments, so Cmd-E unmounted the entire dashboard tree and mounted the other one: every widget re-ran its query, and while the outgoing tree was gone the document collapsed to near-zero height and the browser clamped the scroll offset — the reported "screen flicks, trying to scroll up". The local activePageIndex re-initialised to 0 on the way back, so the page you were editing was discarded (#1371). Both URLs stay. The UI moves up into a new `[id]/layout.tsx` and both `page.tsx` files become `return null`. Next preserves a layout across navigation into a child segment, and `edit` IS a child of `[id]`, so the toggle re-renders only the empty page slot: same DOM nodes, same chart instances, same in-flight queries. `editable` was already a pure prop down to card-container, so flipping the mode remounts and re-queries nothing. Also here: - The active page moves from the view route's local useState to dashboard-store, which the layout outlives — so it survives the toggle in both directions. `?page=` is still read once on first load for existing /[id]/edit?page=N links, and is deliberately NOT appended to the exit navigation: ~8 specs match the view URL with /\/[\w-]+$/. - The setLayout effect is keyed on `id:version` instead of the `dashboard` object. useUpdateDashboard invalidates ["dashboards", id] on every save, so saving while on page 3 already threw you back to page 1 and clobbered savedLayout/_dirty; it now bails when the store is dirty and keeps the current index on reloads. - View mode renders the server layout, edit mode the store's working copy, so discarded edits do not leak into the view. While clean they are the same object (migrateLayout returns a v2 layout verbatim), so nothing remounts. - The auto-refresh countdown moves into the view toolbar, confining its 1Hz re-render to the toolbar instead of the widget tree. - `enabled: editMode` on useConnections/useWidgetTemplates keeps view mode from fetching the editor's data (#913 removed that fetch; the merge would have reintroduced it). useConnections takes an options bag now. - Missing `{ scroll: false }` added to the empty-page CTA and the reader redirect. Tests: a jsdom probe counts DashboardContainer unmounts in its effect CLEANUP and asserts the same instance survives the toggle — the only test that can regress-proof #1370. Two E2E specs tag a widget card with a data attribute that a remount would destroy, and assert scroll equality within 2px and that page 3 is still selected after a round trip. Both fail on the parent commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The CHANGELOG stopped at [1.0.0] — 2026-05-17. Four releases and roughly 200 closed issues had shipped since with no entry, so anyone reading the file concluded the project went quiet in May. That is the opposite of the truth, and it is the first thing a visitor checks to decide whether a project is alive. Reconstructed from the GitHub milestones, not from memory: every bullet traces to a closed issue and cites it. All 152 distinct issue references were verified to exist and be closed. Release dates are the dates each release branch was consolidated into `dev`, because no 1.x version has ever been git-tagged — the repo has two tags and neither belongs to this line. Rather than imply four tagged releases exist, a note at the top states that plainly and points at #1216, which tracks cutting the first one. Two things deliberately not smoothed over: - 1.1.0 moved the interaction accent to indigo (#1104) and 1.3.0 moved it back to citrine amber (#1125). Both entries say so. A changelog that hides a reversal is less useful than one that shows it. - #1155 is described as replacing the modal slide-up with a scale-and-fade, NOT as centring the entrance. It did not centre it: dialogs still animated in 212.5px off-centre until #1373. Claiming otherwise would have put a false statement in a public document for the third time. Also adds an [Unreleased] section for the v1.5 work already on release/1.5, so the file is current rather than ending at the last consolidation. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…import count (#1376, #1377) (#1392) * fix(dashboard): bump version/updatedAt on reassign so the optimistic lock sees it reassignConnectionWidgets rewrote layoutJson and nothing else, while PUT /api/dashboards/[id] bumps version, updatedAt and updated_by and clients send expectedVersion as an optimistic lock. A reassign was therefore invisible to that lock: a browser with the editor open still held a matching version, so its next save silently REVERTED the reassign instead of conflicting. Bumping version turns that lost write into the 409 it should always have been. This changes the existing global reassign's behaviour too — desirably. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(dashboard): scope connection reassignment to one dashboard (#1376, #1377) Reassignment was connection-scoped and global: repointing one imported dashboard silently repointed every other dashboard using that connection. Lib - reassignConnectionWidgets takes an options object instead of five positional strings. Adding a sixth positional meant transposing userId and tenantId type-checked cleanly and silently wrote across tenants. - optional dashboardId adds an ADDITIVE `AND d.id = $x` to both the count and the UPDATE. It never replaces editableDashboardsScope — `d.id = $x` is a filter, not an authorization check, so the owner/editor-share test still gates the raw UPDATE even though the route checks too. - fromConnectionId "" now means "unassigned and needs a connector" (#1377). Two traps handled: `widget->>'connectionId' = ''` is NULL-blind for a widget with no connectionId key, so COALESCE folds missing and empty together; and "" is OVERLOADED because dashboard-export rewrites markdown/iframe widgets to connectionId:"" as well, so content-only types are excluded or a bulk assign stamps a connector onto text widgets. `NOT IN` is NULL when chartType is absent, so an unclassifiable widget is skipped rather than stamped. - the match predicate is extracted once and shared by all three sites (count LATERAL, UPDATE CASE, UPDATE EXISTS) so the reported count cannot drift from what was actually rewritten. - CONTENT_ONLY_CHART_TYPES is exported so the SQL mirrors one source of truth. Route - POST /api/dashboards/{id}/reassign-connection. The source lives in the body, not a path segment, because a path segment cannot express "no connection" — that is the whole reason this sits alongside the connection-scoped endpoint. - guards: canWrite, editor on the dashboard, and a target the caller can actually QUERY (in-tenant AND owned/shared/admin), mirroring /api/query rather than the weaker "exists in tenant". Source ownership is deliberately not required; the write target is the dashboard. - type check kept for a real source, skipped when the source is empty since the original connector type is unrecoverable after an import. - audited as connection.reassign against resourceType "dashboard". Unit tests assert the SHAPE of the emitted SQL via PgDialect.sqlToQuery, since a dropped tenant predicate or ORDER BY is invisible to a result-count assertion. Documented in openapi-spec.ts; the existing route and the delete-dialog flow are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(dashboard): add the Change connection dialog and fix the import count (#1376, #1377) UI - "Change connection…" joins the dashboard card's options menu between Duplicate and Export, gated by canEdit. The bulk reassign is no longer reachable only by starting to delete a connection. - new DashboardConnectionDialog, in its own file so jsdom can render it. It derives {connectionId → widgetCount} plus an "Unassigned (N)" row client-side from useDashboard's layoutJson — no new GET endpoint — and takes names/types from useConnections. ONE SOURCE AT A TIME: pick source, pick target, apply, repeat. A mapping table would need every source resolved before anything could be applied, which is worse for the common case of one wrong connection. - the target picker copies the delete-dialog's same-type filter, and drops it for the Unassigned source where there is no source type to match. - the same dialog serves #1377's post-import prompt and #1376's menu item, mounted once on the page both entry points already live on. Import count - the response now carries unassignedWidgetCount, so the offered count and the count named in the note are the same number by construction. - that count now EXCLUDES content-only widgets, which fixes a pre-existing false alarm: dashboard-export writes markdown/iframe widgets with connectionId "", so importing a correctly-mapped dashboard containing 3 text widgets already reported "3 widgets imported without a connection". - when the skipped connections have different connector types the shortcut is withheld and today's per-widget note stands, since one target cannot be right for both. That decision is a pure helper (importFollowUp) rather than inline JSX so it is covered by unit tests. E2E proves the isolation invariant a mocked driver cannot: the same export is imported twice against connection A, dashboard #1 is reassigned to B through the menu, and #2 is asserted to still be on A and still loading. Also covers the skipped-import bulk fix without opening a widget editor, that a markdown widget is neither counted nor reassigned, and that a viewer-share user gets neither the menu item nor the API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…window save (#1375) (#1385) #1378 fixed the ratchet (one column count at every breakpoint) with component tests only. Three E2E attempts were removed with it because none could be shown to fail against the bug. This is the one that can, and it was verified red with `dashboard-grid.tsx` reverted and green with it restored. The two things that made the earlier attempts blind: Width cannot detect this. Clamping a 12-column layout into 10 keeps `w:6` and moves `x:6` → `x:4`, so the items overlap and the vertical compactor stacks them. Position is what changes. The reverted run reports it exactly: left = {x:274, y:120, width:641} right = {x:274, y:480, width:641} Same width, 360px apart vertically. Any width assertion passes there — and the stored layout and the breakpoint rendering it shrink together anyway, so a `6-of-12` item saved as `6-of-10` measures identically at a fixed viewport. And the seeded dashboards are effectively single-column, so they cannot exhibit the symptom regardless of what is asserted. Hence a purpose-built fixture: two markdown widgets at x:0,w:6 and x:6,w:6 on one row, seeded over the API. The drag and save happen at 1280px (container ~1040, below lg); the assertions happen at 1600px (container ~1360, at lg) where the authored 12 columns are actually rendered as 12. Both container widths are asserted rather than assumed, so a future sidebar change fails the test loudly instead of quietly making it vacuous — as does the pre-save check that the fixture really is two-up. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…wn (#1384) (#1386) Leaflet's `_animateZoom` arms a bare `setTimeout(_onZoomTransitionEnd, 250)` that `Map.remove()` never cancels — `remove()` runs `_stop()` (pan animation only) and then `delete this._mapPane`. Unmounting a map mid-zoom let that callback fire against the destroyed map: `_onZoomTransitionEnd` -> `_move` -> `_getMapPanePos` -> `getPosition(undefined)` -> `undefined._leaflet_pos`. `_onZoomTransitionEnd` early-returns on a falsy `_animatingZoom`, so clearing the flag in the init-effect cleanup disarms both the timer and the `transitionend` path. MapChart is the only Leaflet map in the repo, so the one cleanup covers every caller. With teardown clean, `test:visual` now runs the whole `storybook` project (via the existing `component` workspace script) instead of the single dialog story file, so all 393 story smoke tests gate CI. Before: 393 passed, 4 unhandled errors, exit 1. After: 393 passed, 0 errors, exit 0. Closes #1384 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`review:local` still diffed against release/1.4, so anything branched from release/1.5 was reviewed against the previous release — every commit already consolidated into 1.5 showed up as "changed". Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…1393) The repo root held 13KB of internal agent instructions, visible to every GitHub visitor alongside the README. `.claude/` is where the rest of that machinery already lives — hooks, skills, agents, settings — so the doc belongs with them. Verified before moving, because a wrong answer here would silently disable every project rule with no error: Claude Code loads project memory from BOTH `./CLAUDE.md` and `./.claude/CLAUDE.md` (per the official memory docs, which name the two as equivalent). So the TDD, package-boundary, query-safety and credential rules keep loading. Two references were load-bearing rather than prose and would have broken: - `app/src/lib/__tests__/docs-accuracy.test.ts` reads the doc by path and asserts its claims — the path-existence check, the MIGRATE_ON_START claim and the tenant-guard path. It would have failed on a missing file. - `package.json`'s `review:local` passes `-c CLAUDE.md` to the CodeRabbit CLI. The remaining mentions were comments and skill/agent prose; those now point at the new path so nobody goes looking for a root file that no longer exists. Also adds #1376 and #1377 to the [Unreleased] section, including the two bugs that surfaced during that work: the reassignment being invisible to the optimistic lock, and the import's unassigned-widget count counting markdown and iframe widgets. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…1373) (#1389) #1373 fixed DialogContent to compensate the centring translate on both axes with `slide-*-1/2`. Its sibling AlertDialogContent still carried upstream shadcn's `top-[48%]`, a deliberate 2%-of-height lift, so the two modal components animated differently for no design reason: the confirmation dialog rose into place while the editor dialog scaled in place. Measured in chromium, the drift is 3.24px on a 162px-tall confirmation — small, but it is travel, and travel is what got these classes deleted from Dialog twice (d723a12, PR #1173) by someone reading them as motion. Both axes are `1/2` now, and AlertDialogContent carries the same DO-NOT-DELETE comment: these classes are centring compensation, not motion. The comment also records the `1/2`-over-`[48%]` decision so the next person does not "restore upstream". Tests, both layers, Red verified by putting `[48%]` back: - `stories/ui/alert-dialog.stories.tsx` — CentredOnEnter/CentredOnExit scrub the paused animation in real chromium and assert the box centre stays within 1.5px of the viewport centre. With `[48%]`: dy=3.2px at t=0 on enter, t=200ms on exit. With `1/2`: passes. - `src/components/ui/__tests__/alert-dialog.test.tsx` — jsdom class-presence guard, including `not.toContain("48%")`. Fails fast if the classes go; cannot see the geometry, which is why the story exists. The scrub helper moved to `stories/ui/animation-centring.ts` rather than being copied — the two modals are held to one rule, so they share one measurement and cannot drift apart. `test:visual` was pinned to dialog.stories.tsx and now runs both files. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…1388) * fix(dashboard): make parameter URL sync opt-in and honour the toggle The dashboard viewer called buildUrlParams() with no exclude set, so every parameter landed in the address bar whatever the widget's "Sync to URL" option said — extractNoSyncParams() had zero production callers. Wiring it up was not enough on its own. The chart option defaults to false and is never persisted until an author flips it, so a widget nobody touched showed the toggle off while still publishing its value. Sync is now opt-in: only syncToUrl === true reaches the URL, and the allow-list argument is required rather than optional, so the compiler catches the omission that caused this. Also covers the companion keys range widgets write (_from/_to/_min/_max), and strips a non-syncing param from an inbound URL instead of merely omitting it from later updates. Note: existing deep-links stop populating until the author enables "Sync to URL" on the widget and saves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(dashboard): port the opt-in URL sync onto the hoisted workspace #1387 moved the dashboard body out of [id]/page.tsx into dashboard-workspace.tsx while this branch was open, so the parameter->URL sync effect this PR rewrites no longer lived where it patched. page.tsx is now a return-null routing stub. Ported the opt-in effect onto dashboard-workspace.tsx. This was not optional housekeeping: buildUrlParams' `syncable` argument became REQUIRED, and the workspace still called it with one argument, so merging as-is would not have type-checked. The effect had to move down the component body — it needs `dashboard`, which is fetched after the point where the old effect sat. Guarded the replace on the URL actually changing, tracked against what we last wrote rather than window.location: - Without any guard, the up-front strip replaces the URL we are already on. That is indistinguishable from a redirect, which made the existing "a reader is not redirected in view mode" test unfalsifiable — it asserts on exactly that call. - Reading window.location instead would look correct and silently break the clear-parameters test, because a mocked router never updates the location. Three tests in dashboard-workspace.test.tsx encoded the old opt-out contract and were rewritten to the new one rather than deleted: - "mirrors an opted-in parameter into the URL" — now uses a parameter-select widget with syncToUrl: true, since that is what makes a param syncable. - "does NOT mirror a parameter whose widget never opted in" — new. Without it an implementation that synced everything would still pass the positive case. - "drops the query string once every opted-in parameter is cleared" — now puts the param in the URL first. The drop is a transition; asserting the bare pathname from a standing start passes even if sync is broken entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Six PRs landed after the CHANGELOG was reconstructed and were not represented: #1384, #1373's AlertDialog follow-up, #1388, plus the CLAUDE.md move and the review:local retarget. The #1388 entry leads with the actual defect rather than the design change, because the defect is the part a user felt: `extractNoSyncParams()` had ZERO production callers, so the "Sync to URL" toggle on a parameter-select widget did nothing at all — switching it off still published the value to the address bar. The opt-in inversion is the fix, and it is flagged as breaking for shared links, since a URL carrying `?param_…` for a widget that never opted in will no longer reproduce those values. The #1384 entry records the knock-on that matters more than the crash: four unhandled `_leaflet_pos` errors made the whole Storybook browser project exit 1 even though every story passed, which is why the visual gate could only be aimed at two files. With the timer disarmed, `test:visual` covers the whole project — which is what #1389 then relied on. All 13 issue references in the section verified to exist and be closed. #1384 itself was still open despite its PR merging; closed with the same note. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) (#1462) * fix(component): honour prefers-reduced-motion across all overlays (#1458) Ten Radix primitives animated unconditionally while spinner, progress and skeleton had honoured the preference since the component audit. The obvious per-component fix does not work: the overlays animate through data-attribute variants such as `data-[state=open]:animate-in`, which compile to `.class[data-state=open]` — specificity (0,2,0) — whereas a `motion-reduce:animate-none` utility is (0,1,0), and a media query contributes no specificity. The guard loses the cascade at every one of the fourteen animated surfaces. The reset therefore lives once in `design-tokens.css`, the only stylesheet that both `component/src/index.css` and `app/src/app/globals.css` import. It uses `animation: none` rather than the usual `animation-duration: 0.01ms` because Radix's presence machinery reads `getComputedStyle().animationName` and unmounts synchronously when it is `none`, but waits for an `animationend` when it is not — and that wait is what made `dev` red. Radix keeps an overlay mounted until the exit animation reports back; when an NVL/WebGL widget mounts alongside the close, the 150ms animation stalls and the dialog stays in the DOM well past a 5s budget. Every "flaky graph chart" failure was that same `expect(dialog).not.toBeVisible()` assertion — never a graph assertion — which is why the failing test rotated across the block and retries never helped. Consequences of `animation: none` are handled rather than assumed: `scrollAndHighlight` removed its highlight class only on `animationend`, so under reduced motion the class would have stuck forever. It now also listens for `animationcancel` (an interrupted pulse dispatches that instead, a latent leak independent of this change) and keeps a timer as the backstop. Playwright runs the suite as a reduced-motion user, so the accessibility branch is exercised in CI instead of merely declared. Tests: the mock-based `scrollAndHighlight` cases asserted only that a stubbed `addEventListener` had invoked its own callback, staying green whether or not the highlight was ever cleared. They are replaced by jsdom tests against a real DOM; the node file keeps the parts that genuinely need no DOM. Verified: full E2E 359 passed / 0 failed, and the graph block 30/30 across three repeats with no flakes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(component): scope highlight cleanup to the widget's own animation Addresses two CodeRabbit findings on #1462. `animationend` and `animationcancel` bubble, and a widget card is full of things that animate independently (skeletons, spinners, ECharts). The first descendant animation to finish would have invoked the cleanup and stripped the highlight early. The handler now runs only when `event.target` is the widget itself. Also moves the node-environment test next to its source, per the repo rule that tests live in `__tests__/` beside the file under test, and stops its fake element from invoking listener callbacks with no argument — no real listener does that, and nothing left in that file needs the event to fire. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): wait for the save to land, not for a button that was never disabled `await expect(Save).toBeEnabled()` is not a wait. The button is enabled both before and after the mutation, so the assertion passes on its first poll and the test races on while the dashboard store is still dirty. Clicking "Back" then trips the unsaved-changes guard, and its AlertDialog overlay swallows the pointer events the test is about to send. Animation timing was hiding this: the incidental delays gave the save time to land. With animations disabled (#1458) the race is exposed, and `charts.spec.ts:872` failed 1 run in 5. `parameters.spec.ts` and `form-widget.spec.ts` had each already met this and worked around it locally by clicking "Leave" if the dialog happened to appear — one of them even names it "grid compaction race". The dialog is not the problem; not waiting for the save is. `dashboard-workspace.tsx` calls `markSaved()` and only then raises the "Dashboard saved" toast, so the toast is the first observable moment at which the store is genuinely clean. `saveDashboard()` in fixtures waits for it, so the dialog never appears rather than being dismissed after the fact. Applied to the three Save-then-Back sites that had no workaround (charts.spec.ts ×2, styling-rules.spec.ts). The already-guarded sites are left alone. Verified: charts.spec.ts:872 6/6 with retries disabled (was 1 failure in 5), and the full suite 360 passed / 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(component): scope the reduced-motion assertions to the media block Addresses the third CodeRabbit finding on #1462. The property assertions searched the whole stylesheet, so they would have passed on an `animation: none !important` sitting anywhere else in the file — including one applied unconditionally, which is the opposite of the intent. They now run against the extracted media block only (179 chars of 9,240), plus a check that the extraction has not silently swallowed the whole file and made every assertion vacuous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
alfredo1996
added
chore
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (85)
WalkthroughThe pull request centralizes dashboard view and edit rendering in ChangesDashboard workspace and routing
Dashboard connection reassignment
Widget editor and rendering
Component and CLI updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (20)
app/src/lib/db/connection-reassign.ts (1)
94-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the composite predicate in parentheses.
For an empty source,
widgetMatchesSourcereturns two predicates joined byANDwithout an enclosing group. All three current call sites place the fragment last in aWHEREorWHEN, so the result is correct today. If a future edit combines the fragment withOR, or appends another predicate, theANDbinds unexpectedly and the content-only exclusion is silently lost. Parentheses make the fragment self-contained.♻️ Proposed change
return sql` - COALESCE(widget->>'connectionId', '') = '' - AND widget->>'chartType' NOT IN (${sql.join( - CONTENT_ONLY_CHART_TYPES.map((t) => sql`${t}`), - sql`, `, - )}) + ( + COALESCE(widget->>'connectionId', '') = '' + AND widget->>'chartType' NOT IN (${sql.join( + CONTENT_ONLY_CHART_TYPES.map((t) => sql`${t}`), + sql`, `, + )}) + ) `;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/db/connection-reassign.ts` around lines 94 - 105, Update widgetMatchesSource so the empty fromConnectionId branch wraps its combined COALESCE and chart-type exclusion predicates in an enclosing parenthesized SQL expression, keeping both conditions grouped as one self-contained fragment while leaving the non-empty branch unchanged.app/e2e/dashboard-connection-reassign.spec.ts (1)
85-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
anywith the PlaywrightPagetype.The coding guidelines require a comment that explains every
any. Theeslint-disableline suppresses the rule but does not state a reason.Pageis already available from@playwright/test, so the annotation is not needed.♻️ Proposed change
-const openCardMenu = async ( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - page: any, - name: string, -) => { +const openCardMenu = async (page: Page, name: string) => {Add the type import:
+import type { Page } from "`@playwright/test`";As per coding guidelines: "Use TypeScript strict mode and do not use
anywithout a comment explaining why."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/dashboard-connection-reassign.spec.ts` around lines 85 - 96, Update the openCardMenu helper to use Playwright’s Page type for its page parameter, importing Page from `@playwright/test`. Remove the unnecessary eslint-disable directive and any annotation while preserving the existing menu interaction.Source: Coding guidelines
app/src/app/api/dashboards/import/__tests__/route.test.ts (1)
403-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a status assertion to the mixed-case test.
The other two new tests assert the response status. This one reads
body.data.unassignedWidgetCountwithout checking that the import succeeded, so a validation regression surfaces as a confusing property-access failure rather than a status mismatch.💚 Proposed change
const body = await res.json(); + expect(res.status).toBe(201); // w1 was skipped; w2/w3 are markdown+iframe and never wanted a connection. expect(body.data.unassignedWidgetCount).toBe(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/api/dashboards/import/__tests__/route.test.ts` around lines 403 - 420, Add a response status assertion in the mixed-case test before reading body.data.unassignedWidgetCount, matching the success-status assertions in the neighboring import tests. Keep the existing unassignedWidgetCount assertion unchanged.app/src/app/api/dashboards/[id]/reassign-connection/__tests__/route.test.ts (1)
173-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test cannot prove the visibility guarantee it names.
The
whereclause is mocked away, somakeSelectChain([])only re-tests the same "target lookup returned no row" path as the test on lines 159-168. It does not exercise the owner/visibility='shared'/admin predicate. Consider asserting on the built predicate instead, or cover the visibility rule in the DB-level test suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/api/dashboards/`[id]/reassign-connection/__tests__/route.test.ts around lines 173 - 181, The private-connection test currently mocks away the visibility predicate and duplicates the missing-target case. Update the test around POST and makeSelectChain to verify the owner/shared/admin filtering predicate, or move this visibility guarantee to the database-level test suite, while keeping the 404 response and no-widget-reassignment assertions.app/src/components/dashboard-edit-toolbar.tsx (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared Filters toggle button. Both new toolbars contain the same parameter-toggle button with the same
aria-labellogic and the same arbitrarytext-[10px]count badge. One shared component removes the copy and keeps the badge styling consistent.
app/src/components/dashboard-edit-toolbar.tsx#L89-L104: replace this block with the sharedParameterToggleButtonand passhasParameters,parameterCount,showParameterBar, andonToggleParameterBar.app/src/components/dashboard-view-toolbar.tsx#L130-L145: replace this identical block with the same shared component, and move the count badge styling into it so both toolbars use one token set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-edit-toolbar.tsx` around lines 89 - 104, Extract a shared ParameterToggleButton component containing the Filters toggle, aria-label logic, and count badge styling. In app/src/components/dashboard-edit-toolbar.tsx lines 89-104 and app/src/components/dashboard-view-toolbar.tsx lines 130-145, replace the duplicated blocks with ParameterToggleButton, passing hasParameters, parameterCount, showParameterBar, and onToggleParameterBar; move the badge styling into the shared component.app/e2e/edit-page-preservation.spec.ts (2)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the layout PUT response.
If this request fails, the test fails later at an unrelated visibility assertion. The
createTestDashboardfixture inapp/e2e/fixtures.tsalready throws on a non-OK response. Apply the same check here for a clear failure message.♻️ Proposed fix
- await page.request.put(`/api/dashboards/${id}`, { + const res = await page.request.put(`/api/dashboards/${id}`, { data: { layoutJson: { version: 2, pages: [1, 2, 3, 4].map(pageWith), }, }, }); + expect(res.ok(), `layout PUT failed: ${res.status()}`).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/edit-page-preservation.spec.ts` around lines 42 - 49, Check the response returned by the layout PUT request in the edit-page preservation test and fail immediately with the same non-OK response handling used by createTestDashboard in the fixture. Preserve the existing request payload and make the response validation occur before subsequent visibility assertions.
110-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the upper clamp too.
The comment states that
99clamps to the last page. The loop only proves some tab is selected. Add the name-level assertion forPage 4so a regression that clamps99to the first page still fails.♻️ Proposed addition
await page.goto(`/${id}/edit?page=abc`); await expect( page.getByRole("tab", { name: "Page 1", selected: true }), ).toBeVisible({ timeout: 15_000 }); + await page.goto(`/${id}/edit?page=99`); + await expect( + page.getByRole("tab", { name: "Page 4", selected: true }), + ).toBeVisible({ timeout: 15_000 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/edit-page-preservation.spec.ts` around lines 110 - 118, Update the pagination assertions in the edit-page preservation test to verify that navigating with page=99 selects the last page, specifically the “Page 4” tab. Keep the existing assertions for -1 and abc resolving to “Page 1” unchanged, and ensure the upper-clamp case checks the tab name rather than only selected-tab visibility.Source: Coding guidelines
app/src/components/__tests__/dashboard-connection-dialog.test.tsx (2)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain each
any, or drop it.The coding guidelines require a comment that explains why
anyis used. Theeslint-disablelines suppress the rule but do not state a reason. Either add the reason, or type the fixtures asPartial<DashboardDetail>(already the declared type ofmockDashboard) and the override map asPartial<typeof props>, which removes the need forany.Also applies to: 150-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx` around lines 137 - 139, Update the renderDialog test helper and the related override fixture at the additional location to avoid unexplained any usage: type dashboard fixtures as Partial<DashboardDetail> and override maps as Partial<typeof props>, removing the eslint-disable comments while preserving the existing render behavior.Source: Coding guidelines
203-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion does not prove the test name.
The name claims the dashboard query is not fired. The body only asserts that the title is absent. Add a spy on the mocked
useDashboardand assert it received an empty id, or rename the test to describe what it checks.♻️ Proposed change
+const useDashboardSpy = vi.fn(); vi.mock("`@/hooks/use-dashboards`", () => ({ - useDashboard: (id: string) => ({ + useDashboard: (id: string) => (useDashboardSpy(id), { data: id ? mockDashboard : undefined, isLoading: false, }),it("stays inert while closed so the dashboard query is not fired", () => { renderDialog({ open: false, dashboardId: "" }); expect(screen.queryByText("Change connection")).not.toBeInTheDocument(); + expect(useDashboardSpy).toHaveBeenCalledWith(""); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx` around lines 203 - 206, Update the test “stays inert while closed so the dashboard query is not fired” to verify its stated behavior by spying on the mocked useDashboard call and asserting it receives an empty dashboard id when rendered closed; retain the existing visibility assertion if useful, or rename the test only if that query assertion cannot be added.app/src/components/dashboard-view-toolbar.tsx (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive feedback when the custom interval is rejected.
handleCustomApplyreturns silently for a non-numeric value or for a value below 5. The user sees no change and no message. Show a short inline hint, or disable the Set button while the value is invalid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-view-toolbar.tsx` around lines 90 - 96, Update handleCustomApply to provide user feedback when customSeconds is non-numeric or below the 5-second minimum, using a short inline hint or disabling the Set button while invalid. Preserve the existing onApplyInterval, reset, and dropdown-closing behavior for valid values.app/src/hooks/__tests__/use-connections.test.ts (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
enabledflag.The call site is correctly migrated to the options object, but no test asserts the new
enabledbehavior. Add two cases against the mockeduseQueryconfig:enableddefaults totruewhen no options are passed, anduseConnections({ enabled: false })forwardsenabled: false. This is the flagDashboardWorkspacerelies on to skip the connections request in view mode.As per coding guidelines: "Every new behavior, bug fix, and edge case must have a test, written before implementation, following Red → Green → Refactor."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/hooks/__tests__/use-connections.test.ts` at line 67, Add coverage in the useConnections tests for the new enabled option: assert the mocked useQuery configuration defaults enabled to true when no options are provided, and assert useConnections({ enabled: false }) forwards enabled: false. Keep the assertions focused on the generated query config and follow the existing test structure.Source: Coding guidelines
component/src/components/composed/dashboard-grid.tsx (1)
132-136: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGive the
colsprop a stable identity.
colsForEveryBreakpoint(cols)builds a fresh object on every render, while the siblinglayoutsprop is memoized anddefaultBreakpointsis a module constant. Memoize it for consistency, and to avoid handingResponsiveGridLayouta new config reference on each render.♻️ Memoize the column map
const layouts = React.useMemo( () => ({ lg: layout, md: layout, sm: layout, xs: layout }), [layout], ); + + const gridCols = React.useMemo(() => colsForEveryBreakpoint(cols), [cols]);- cols={colsForEveryBreakpoint(cols)} + cols={gridCols}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@component/src/components/composed/dashboard-grid.tsx` around lines 132 - 136, Memoize the result of colsForEveryBreakpoint(cols) before rendering ResponsiveGridLayout, using the existing component memoization pattern and cols as its dependency. Pass the memoized column map to the cols prop so its object identity remains stable when cols is unchanged.app/src/app/(dashboard)/[id]/layout.tsx (1)
20-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive
editModefrom the exact segment.
pathname.endsWith("/edit")also matches the view route of a dashboard whose id is literallyedit, because that pathname is/edit. IDs are server-generated, so this is unlikely, but an exact comparison removes the ambiguity and expresses the intent directly.♻️ Compare against the concrete segment
const { id } = useParams<{ id: string }>(); const pathname = usePathname(); - const editMode = pathname.endsWith("/edit"); + const editMode = pathname === `/${id}/edit`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/`(dashboard)/[id]/layout.tsx around lines 20 - 22, Update the editMode derivation in the dashboard layout to use an exact pathname comparison with the edit route, rather than pathname.endsWith("/edit"). Preserve editMode as true only for the concrete edit pathname and false when the dashboard id itself is "edit".app/src/components/__tests__/dashboard-workspace.test.tsx (1)
897-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the positive Cmd+S case.
The test name says "saves in edit mode only", but the body only pins the view-mode negative. No test fires Cmd+S in edit mode, so a regression that disables the shortcut wiring (for example a wrong
disabledexpression atdashboard-workspace.tsxLine 581) still passes. The Save button test at Line 663 does not exercise the shortcut path.💚 Proposed addition
it("Cmd+S saves in edit mode only", () => { render(<DashboardWorkspace id="d1" editMode={false} />); fireEvent.keyDown(document, { key: "s", metaKey: true }); expect(mockMutateAsync).not.toHaveBeenCalled(); }); + + it("Cmd+S saves in edit mode", async () => { + pathname = "/d1/edit"; + render(<DashboardWorkspace id="d1" editMode={true} />); + fireEvent.keyDown(document, { key: "s", metaKey: true }); + await vi.waitFor(() => + expect(mockMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ id: "d1", expectedVersion: 1 }), + ), + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-workspace.test.tsx` around lines 897 - 901, Extend the “Cmd+S saves in edit mode only” test to render DashboardWorkspace with editMode enabled, fire the same Cmd+S key event, and assert mockMutateAsync is called. Keep the existing view-mode assertion so both enabled and disabled shortcut behavior are covered.app/src/lib/shared/url-params.ts (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stray
ponytail:marker from the comment.The explanation itself is useful. The
ponytail:prefix reads as an internal scratch marker and has no meaning to a future reader.♻️ Proposed tweak
- // ponytail: add every companion key regardless of parameterType — + // Add every companion key regardless of parameterType — // a `select` simply never writes them, so the extra entries are inert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/shared/url-params.ts` around lines 93 - 94, Update the comment near the companion-key handling to remove the stray “ponytail:” prefix while preserving the explanation about adding every companion key regardless of parameterType.app/src/stores/__tests__/dashboard-store.test.ts (1)
63-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the upper clamp and the
NaNbranch.
setLayoutclamps both ends and has an explicitisNaNguard (dashboard-store.tsLine 104). The new tests only pin the lower bound and the zero case. Add an above-range index and aNaNindex so the other two branches cannot regress silently.💚 Proposed addition
+ it("setLayout clamps an initialPageIndex past the last page", () => { + const newLayout = { + version: 2 as const, + pages: [ + { id: "p1", title: "A", widgets: [], gridLayout: [] }, + { id: "p2", title: "B", widgets: [], gridLayout: [] }, + ], + }; + useDashboardStore.getState().setLayout(newLayout, 99); + expect(useDashboardStore.getState().activePageIndex).toBe(1); + }); + + it("setLayout treats a NaN initialPageIndex as 0", () => { + const newLayout = { + version: 2 as const, + pages: [{ id: "p1", title: "A", widgets: [], gridLayout: [] }], + }; + useDashboardStore.getState().setLayout(newLayout, Number.NaN); + expect(useDashboardStore.getState().activePageIndex).toBe(0); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/stores/__tests__/dashboard-store.test.ts` around lines 63 - 86, Add tests for setLayout covering an initialPageIndex above the available page range, asserting activePageIndex clamps to the last page, and a NaN initialPageIndex, asserting it follows the explicit NaN fallback behavior. Keep the existing negative and zero cases unchanged.app/e2e/parameter-url-sync.spec.ts (1)
11-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the seeded connection id into
fixtures.ts.
fixtures.tsalready exports shared seed constants such asALICE,TEST_NEO4J_BOLT_URL, andTEST_PG_PORT. Add the Neo4jconn-neo4j-001constant there and reuse it fromparamWidgetso seed-ID changes stay central.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/parameter-url-sync.spec.ts` around lines 11 - 26, Move the seeded Neo4j connection ID into the shared constants in fixtures.ts, alongside ALICE, TEST_NEO4J_BOLT_URL, and TEST_PG_PORT, then import and reuse that constant in paramWidget instead of the inline "conn-neo4j-001" value.app/src/components/dashboard-workspace.tsx (1)
317-353: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSend
expectedVersionwith the auto-refresh settings write.
applyIntervalsends{...serverLayout, settings: newSettings}withoutexpectedVersion, so the API does not apply the optimistic-lock guard. If another save changes the dashboard afterserverLayoutwas loaded, this write overwrites that layout.Forward
expectedVersion: dashboard?.versionand handle the resulting conflict through the existing save-error toast path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-workspace.tsx` around lines 317 - 353, Update applyInterval to include expectedVersion: dashboard?.version in the updateDashboard payload, using the current dashboard version for optimistic locking. Preserve the existing persist queue and route mutation failures, including version conflicts, through the existing classifySaveError toast path.app/e2e/editor-maximize.spec.ts (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the test-only CodeMirror property.
The lint suppression identifies the rule but does not explain why
HTMLElementrequiresany. Model__cmViewwith a local structural type, or document a specific reason if that type cannot represent the runtime shape. Verify the property shape against the active CodeMirror wrapper.As per coding guidelines, do not use
anywithout a comment explaining why.Proposed type-safe replacement
+type CodeMirrorHost = HTMLElement & { + __cmView?: { state: { doc: { length: number } } }; +}; + function docLength(editor: import("`@playwright/test`").Locator) { return editor.evaluate( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (el: HTMLElement) => (el as any).__cmView?.state.doc.length ?? -1, + (el: HTMLElement) => + (el as CodeMirrorHost).__cmView?.state.doc.length ?? -1, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/editor-maximize.spec.ts` around lines 31 - 36, Update docLength to replace the eslint-suppressed any cast with a local structural type matching the active CodeMirror wrapper’s __cmView property and its state.doc.length shape. Access the typed property from the HTMLElement while preserving the existing -1 fallback, and verify the modeled runtime shape against the wrapper.Source: Coding guidelines
app/src/components/__tests__/widget-editor-modal-maximize.test.tsx (1)
252-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
parameter-selectexit branch.
isParamSelectis an independent maximize guard fromisContentOnly. This test switches only to"markdown". Add a test that maximizes the editor, switches to"parameter-select", and verifies that the preview mounts and the grid returns to two columns.As per coding guidelines, every new behavior, bug fix, and edge case must have a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/widget-editor-modal-maximize.test.tsx` around lines 252 - 266, Add a dedicated test alongside the existing maximize behavior test that expands the editor, switches the chart type to "parameter-select" via useWidgetEditorStore, and verifies widget-preview is rendered and gridColumns() returns the two-column layout. Keep the setup and assertions consistent with the existing markdown test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 22-24: Update the review:local description in the command list to
reference release/1.5, matching the script configuration in package.json instead
of release/1.4.
In `@app/e2e/grid.spec.ts`:
- Around line 187-204: The drag in the test must produce a real grid layout
change before clicking Save. Update the drag coordinates in the flow around
item, boundingBox, and page.mouse.move so the movement exceeds half a column and
changes gridLayout, preserving the existing drag-and-save assertions.
In `@app/src/components/dashboard-view-toolbar.tsx`:
- Line 123: Update the Badge rendering in the dashboard toolbar so it is
conditionally rendered only when the optional role value is set; preserve the
existing secondary variant and role text when a value is available.
In `@app/src/components/dashboard-workspace.tsx`:
- Around line 796-806: Update the DashboardContainer props in the page-rendering
block so refetchInterval is enabled only when isActive; preserve editMode’s
false interval behavior and pass viewRefetchInterval only for the active page,
matching the existing onLayoutChange gating.
In `@app/src/components/lazy-visible.tsx`:
- Around line 84-88: Update the useEffect claim flow in
app/src/components/lazy-visible.tsx: after claimSlot(slot), immediately invoke
the existing eviction path or enforce the budget within claimSlot, while
preserving cleanup via dropSlot(slot). Add the regression in
app/e2e/heavy-widgets.spec.ts to leave at-budget slots off-screen, claim a
replacement above the budget, and verify that the over-budget eviction occurs.
In `@app/src/lib/api/openapi-spec.ts`:
- Around line 365-384: Update the reassign-connection endpoint response schema
to wrap dashboardsUpdated and widgetsReassigned under a data object, matching
apiSuccess’s { data, error, meta } envelope and the API-key endpoint style. Add
the missing 500 response using R.serverError alongside the existing 400–404
responses.
---
Nitpick comments:
In `@app/e2e/dashboard-connection-reassign.spec.ts`:
- Around line 85-96: Update the openCardMenu helper to use Playwright’s Page
type for its page parameter, importing Page from `@playwright/test`. Remove the
unnecessary eslint-disable directive and any annotation while preserving the
existing menu interaction.
In `@app/e2e/edit-page-preservation.spec.ts`:
- Around line 42-49: Check the response returned by the layout PUT request in
the edit-page preservation test and fail immediately with the same non-OK
response handling used by createTestDashboard in the fixture. Preserve the
existing request payload and make the response validation occur before
subsequent visibility assertions.
- Around line 110-118: Update the pagination assertions in the edit-page
preservation test to verify that navigating with page=99 selects the last page,
specifically the “Page 4” tab. Keep the existing assertions for -1 and abc
resolving to “Page 1” unchanged, and ensure the upper-clamp case checks the tab
name rather than only selected-tab visibility.
In `@app/e2e/editor-maximize.spec.ts`:
- Around line 31-36: Update docLength to replace the eslint-suppressed any cast
with a local structural type matching the active CodeMirror wrapper’s __cmView
property and its state.doc.length shape. Access the typed property from the
HTMLElement while preserving the existing -1 fallback, and verify the modeled
runtime shape against the wrapper.
In `@app/e2e/parameter-url-sync.spec.ts`:
- Around line 11-26: Move the seeded Neo4j connection ID into the shared
constants in fixtures.ts, alongside ALICE, TEST_NEO4J_BOLT_URL, and
TEST_PG_PORT, then import and reuse that constant in paramWidget instead of the
inline "conn-neo4j-001" value.
In `@app/src/app/`(dashboard)/[id]/layout.tsx:
- Around line 20-22: Update the editMode derivation in the dashboard layout to
use an exact pathname comparison with the edit route, rather than
pathname.endsWith("/edit"). Preserve editMode as true only for the concrete edit
pathname and false when the dashboard id itself is "edit".
In `@app/src/app/api/dashboards/`[id]/reassign-connection/__tests__/route.test.ts:
- Around line 173-181: The private-connection test currently mocks away the
visibility predicate and duplicates the missing-target case. Update the test
around POST and makeSelectChain to verify the owner/shared/admin filtering
predicate, or move this visibility guarantee to the database-level test suite,
while keeping the 404 response and no-widget-reassignment assertions.
In `@app/src/app/api/dashboards/import/__tests__/route.test.ts`:
- Around line 403-420: Add a response status assertion in the mixed-case test
before reading body.data.unassignedWidgetCount, matching the success-status
assertions in the neighboring import tests. Keep the existing
unassignedWidgetCount assertion unchanged.
In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx`:
- Around line 137-139: Update the renderDialog test helper and the related
override fixture at the additional location to avoid unexplained any usage: type
dashboard fixtures as Partial<DashboardDetail> and override maps as
Partial<typeof props>, removing the eslint-disable comments while preserving the
existing render behavior.
- Around line 203-206: Update the test “stays inert while closed so the
dashboard query is not fired” to verify its stated behavior by spying on the
mocked useDashboard call and asserting it receives an empty dashboard id when
rendered closed; retain the existing visibility assertion if useful, or rename
the test only if that query assertion cannot be added.
In `@app/src/components/__tests__/dashboard-workspace.test.tsx`:
- Around line 897-901: Extend the “Cmd+S saves in edit mode only” test to render
DashboardWorkspace with editMode enabled, fire the same Cmd+S key event, and
assert mockMutateAsync is called. Keep the existing view-mode assertion so both
enabled and disabled shortcut behavior are covered.
In `@app/src/components/__tests__/widget-editor-modal-maximize.test.tsx`:
- Around line 252-266: Add a dedicated test alongside the existing maximize
behavior test that expands the editor, switches the chart type to
"parameter-select" via useWidgetEditorStore, and verifies widget-preview is
rendered and gridColumns() returns the two-column layout. Keep the setup and
assertions consistent with the existing markdown test.
In `@app/src/components/dashboard-edit-toolbar.tsx`:
- Around line 89-104: Extract a shared ParameterToggleButton component
containing the Filters toggle, aria-label logic, and count badge styling. In
app/src/components/dashboard-edit-toolbar.tsx lines 89-104 and
app/src/components/dashboard-view-toolbar.tsx lines 130-145, replace the
duplicated blocks with ParameterToggleButton, passing hasParameters,
parameterCount, showParameterBar, and onToggleParameterBar; move the badge
styling into the shared component.
In `@app/src/components/dashboard-view-toolbar.tsx`:
- Around line 90-96: Update handleCustomApply to provide user feedback when
customSeconds is non-numeric or below the 5-second minimum, using a short inline
hint or disabling the Set button while invalid. Preserve the existing
onApplyInterval, reset, and dropdown-closing behavior for valid values.
In `@app/src/components/dashboard-workspace.tsx`:
- Around line 317-353: Update applyInterval to include expectedVersion:
dashboard?.version in the updateDashboard payload, using the current dashboard
version for optimistic locking. Preserve the existing persist queue and route
mutation failures, including version conflicts, through the existing
classifySaveError toast path.
In `@app/src/hooks/__tests__/use-connections.test.ts`:
- Line 67: Add coverage in the useConnections tests for the new enabled option:
assert the mocked useQuery configuration defaults enabled to true when no
options are provided, and assert useConnections({ enabled: false }) forwards
enabled: false. Keep the assertions focused on the generated query config and
follow the existing test structure.
In `@app/src/lib/db/connection-reassign.ts`:
- Around line 94-105: Update widgetMatchesSource so the empty fromConnectionId
branch wraps its combined COALESCE and chart-type exclusion predicates in an
enclosing parenthesized SQL expression, keeping both conditions grouped as one
self-contained fragment while leaving the non-empty branch unchanged.
In `@app/src/lib/shared/url-params.ts`:
- Around line 93-94: Update the comment near the companion-key handling to
remove the stray “ponytail:” prefix while preserving the explanation about
adding every companion key regardless of parameterType.
In `@app/src/stores/__tests__/dashboard-store.test.ts`:
- Around line 63-86: Add tests for setLayout covering an initialPageIndex above
the available page range, asserting activePageIndex clamps to the last page, and
a NaN initialPageIndex, asserting it follows the explicit NaN fallback behavior.
Keep the existing negative and zero cases unchanged.
In `@component/src/components/composed/dashboard-grid.tsx`:
- Around line 132-136: Memoize the result of colsForEveryBreakpoint(cols) before
rendering ResponsiveGridLayout, using the existing component memoization pattern
and cols as its dependency. Pass the memoized column map to the cols prop so its
object identity remains stable when cols is unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cb95f87-119e-4372-8629-33d121918670
📒 Files selected for processing (85)
.claude/CLAUDE.md.claude/agents/project-architect.md.claude/hooks/check-boundaries.sh.claude/hooks/check-migration-guard.sh.claude/skills/next/SKILL.md.github/workflows/ci.yml.gitignoreCHANGELOG.mdapp/e2e/charts.spec.tsapp/e2e/dashboard-connection-reassign.spec.tsapp/e2e/edit-page-preservation.spec.tsapp/e2e/edit-scroll-position.spec.tsapp/e2e/editor-maximize.spec.tsapp/e2e/fixtures.tsapp/e2e/grid.spec.tsapp/e2e/heavy-widgets.spec.tsapp/e2e/parameter-url-sync.spec.tsapp/e2e/styling-rules.spec.tsapp/playwright.config.tsapp/src/__tests__/test-environment-boundary.test.tsapp/src/app/(dashboard)/[id]/__tests__/layout.test.tsxapp/src/app/(dashboard)/[id]/edit/page.tsxapp/src/app/(dashboard)/[id]/layout.tsxapp/src/app/(dashboard)/[id]/page.tsxapp/src/app/(dashboard)/page.tsxapp/src/app/api/connections/[id]/reassign/__tests__/route.test.tsapp/src/app/api/connections/[id]/reassign/route.tsapp/src/app/api/dashboards/[id]/reassign-connection/__tests__/route.test.tsapp/src/app/api/dashboards/[id]/reassign-connection/route.tsapp/src/app/api/dashboards/import/__tests__/route.test.tsapp/src/app/api/dashboards/import/route.tsapp/src/components/__tests__/dashboard-connection-dialog.test.tsxapp/src/components/__tests__/dashboard-workspace.test.tsxapp/src/components/__tests__/lazy-visible.test.tsxapp/src/components/__tests__/widget-editor-modal-maximize.test.tsxapp/src/components/dashboard-connection-dialog.tsxapp/src/components/dashboard-edit-toolbar.tsxapp/src/components/dashboard-view-toolbar.tsxapp/src/components/dashboard-workspace.tsxapp/src/components/lazy-visible.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/query-editor-panel.test.tsxapp/src/components/widget-editor/__tests__/widget-preview-panel.test.tsxapp/src/components/widget-editor/query-editor-panel.tsxapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/hooks/__tests__/use-connections.test.tsapp/src/hooks/use-connections.tsapp/src/hooks/use-dashboards.tsapp/src/hooks/use-widget-templates.tsapp/src/lib/__tests__/db/connection-reassign.test.tsapp/src/lib/__tests__/docs-accuracy.test.tsapp/src/lib/__tests__/shared/url-params.test.tsapp/src/lib/api/openapi-spec.tsapp/src/lib/dashboard/__tests__/import-follow-up.test.tsapp/src/lib/dashboard/import-follow-up.tsapp/src/lib/db/__tests__/tenant-scope.test.tsapp/src/lib/db/connection-reassign.tsapp/src/lib/shared/url-params.tsapp/src/lib/widget/__tests__/scroll-to-widget.test.tsapp/src/lib/widget/__tests__/scroll-to-widget.test.tsxapp/src/lib/widget/__tests__/webgl-budget.test.tsapp/src/lib/widget/content-only-chart.tsapp/src/lib/widget/scroll-to-widget.tsapp/src/lib/widget/webgl-budget.tsapp/src/plugins/graph/component.tsxapp/src/stores/__tests__/dashboard-store.test.tsapp/src/stores/dashboard-store.tscli/src/__tests__/commands/status.test.tscli/src/__tests__/lib/docker.test.tscli/src/commands/status.tscli/src/lib/docker.tscomponent/design-tokens.csscomponent/src/__tests__/reduced-motion.test.tscomponent/src/charts/__tests__/map-chart.test.tsxcomponent/src/charts/map-chart.tsxcomponent/src/components/composed/__tests__/dashboard-grid.test.tsxcomponent/src/components/composed/dashboard-grid.tsxcomponent/src/components/ui/__tests__/alert-dialog.test.tsxcomponent/src/components/ui/__tests__/dialog.test.tsxcomponent/src/components/ui/alert-dialog.tsxcomponent/src/components/ui/dialog.tsxcomponent/stories/ui/alert-dialog.stories.tsxcomponent/stories/ui/animation-centring.tscomponent/stories/ui/dialog.stories.tsxpackage.json
Comment on lines
+187
to
+204
| // Nudge the left widget by less than half a column so the drag registers | ||
| // (threshold is 3px) without intentionally moving anything: any layout | ||
| // change from here is the grid's own doing, not the user's. | ||
| const handle = item(page, "left").locator(".drag-handle"); | ||
| const grip = (await handle.boundingBox())!; | ||
| const [cx, cy] = [grip.x + grip.width / 2, grip.y + grip.height / 2]; | ||
| await page.mouse.move(cx, cy); | ||
| await page.mouse.down(); | ||
| await page.mouse.move(cx + 12, cy, { steps: 6 }); | ||
| await page.mouse.up(); | ||
|
|
||
| const saved = page.waitForResponse( | ||
| (r) => | ||
| r.url().includes(`/api/dashboards/${id}`) && | ||
| r.request().method() === "PUT", | ||
| ); | ||
| await page.getByRole("button", { name: "Save" }).click(); | ||
| expect((await saved).ok(), "the save request failed").toBe(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is the store dirtied by an identical grid layout, and is Save gated on dirty state?
ast-grep outline app/src/stores/dashboard-store.ts --items all
rg -nP -C 8 'updateGridLayout|hasUnsavedChanges' app/src/stores/dashboard-store.ts
rg -nP -C 6 'name: "Save"|>Save<|disabled=' app/src/components/dashboard-edit-toolbar.tsxRepository: alfredo1996/neoboard
Length of output: 3491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dashboard store relevant lines =="
sed -n '1,150p' app/src/stores/dashboard-store.ts
sed -n '210,270p' app/src/stores/dashboard-store.ts
echo "== grid spec relevant lines =="
sed -n '1,240p' app/e2e/grid.spec.ts
echo "== dashboard edit toolbar Save search =="
rg -n -C 8 'Save|hasUnsavedChanges|onClick|disabled|form' app/src/components dashboard-edit-toolbar.tsx app/src 2>/dev/null || trueRepository: alfredo1996/neoboard
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact dashboard store relevant lines =="
sed -n '1,140p' app/src/stores/dashboard-store.ts
sed -n '230,260p' app/src/stores/dashboard-store.ts
echo "== exact grid spec relevant lines =="
sed -n '160,220p' app/e2e/grid.spec.ts
echo "== files containing dashboard-edit-toolbar or grid workspace components =="
rg -n "dashboard-edit-toolbar|dashboard-workspace|ReactGridLayout|onLayoutChange|onDragStop|updateGridLayout|hasUnsavedChanges" app/src -g '*.{tsx,ts}' | head -n 120Repository: alfredo1996/neoboard
Length of output: 16317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dashboard workspace save/unsaved and grid handlers =="
sed -n '120,175p' app/src/components/dashboard-workspace.tsx
sed -n '160,170p' app/src/components/dashboard-workspace.tsx
sed -n '788,830p' app/src/components/dashboard-workspace.tsx
echo "== dashboard edit toolbar save button =="
rg -n -C 12 'Save|hasUnsavedChanges|onClick|disabled|type="button"' app/src/components/dashboard-edit-toolbar.tsx
echo "== dashboard edit toolbar file =="
sed -n '1,180p' app/src/components/dashboard-edit-toolbar.tsxRepository: alfredo1996/neoboard
Length of output: 12726
Drag far enough to register a real grid change before saving.
The small nudge still leaves the active gridLayout equal to the previous layout, so updateGridLayout returns the store unchanged. The edit toolbar is not gated on hasUnsavedChanges, but waitForResponse can still time out because the save handler is no-op when the store layout did not change. Move the mouse more than half a column, or mark the grid drag as an edit even when snap/normalize produces the same positions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/e2e/grid.spec.ts` around lines 187 - 204, The drag in the test must
produce a real grid layout change before clicking Save. Update the drag
coordinates in the flow around item, boundingBox, and page.mouse.move so the
movement exceeds half a column and changes gridLayout, preserving the existing
drag-and-save assertions.
| </ToolbarSection> | ||
| <ToolbarSection className="flex-1"> | ||
| <h1 className="text-lg font-bold">{name}</h1> | ||
| <Badge variant="secondary">{role}</Badge> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not render an empty badge.
role is optional. When it is undefined, this renders a badge with no text. Render the badge only when role is set.
🐛 Proposed fix
- <Badge variant="secondary">{role}</Badge>
+ {role && <Badge variant="secondary">{role}</Badge>}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Badge variant="secondary">{role}</Badge> | |
| {role && <Badge variant="secondary">{role}</Badge>} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/components/dashboard-view-toolbar.tsx` at line 123, Update the Badge
rendering in the dashboard toolbar so it is conditionally rendered only when the
optional role value is set; preserve the existing secondary variant and role
text when a value is available.
Comment on lines
+796
to
+806
| if (!isActive && !visitedPages.has(index)) return null; | ||
| return ( | ||
| <div | ||
| key={page.id} | ||
| className={isActive ? undefined : "hidden"} | ||
| aria-hidden={!isActive} | ||
| > | ||
| <DashboardContainer | ||
| page={page} | ||
| editable={editMode} | ||
| refetchInterval={editMode ? false : viewRefetchInterval} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Stop auto-refresh polling on hidden pages.
Visited pages stay mounted and are only visually hidden (Line 796 and Line 800). Line 806 passes viewRefetchInterval to every mounted page, so each visited page keeps refetching its widget queries in the background. After a user visits several pages, auto-refresh multiplies concurrent queries for content nobody can see.
Gate the interval on isActive, the same way onLayoutChange is gated at Line 813.
⚡ Proposed fix
<DashboardContainer
page={page}
editable={editMode}
- refetchInterval={editMode ? false : viewRefetchInterval}
+ refetchInterval={
+ editMode || !isActive ? false : viewRefetchInterval
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!isActive && !visitedPages.has(index)) return null; | |
| return ( | |
| <div | |
| key={page.id} | |
| className={isActive ? undefined : "hidden"} | |
| aria-hidden={!isActive} | |
| > | |
| <DashboardContainer | |
| page={page} | |
| editable={editMode} | |
| refetchInterval={editMode ? false : viewRefetchInterval} | |
| if (!isActive && !visitedPages.has(index)) return null; | |
| return ( | |
| <div | |
| key={page.id} | |
| className={isActive ? undefined : "hidden"} | |
| aria-hidden={!isActive} | |
| > | |
| <DashboardContainer | |
| page={page} | |
| editable={editMode} | |
| refetchInterval={ | |
| editMode || !isActive ? false : viewRefetchInterval | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/components/dashboard-workspace.tsx` around lines 796 - 806, Update
the DashboardContainer props in the page-rendering block so refetchInterval is
enabled only when isActive; preserve editMode’s false interval behavior and pass
viewRefetchInterval only for the active page, matching the existing
onLayoutChange gating.
Comment on lines
+84
to
+88
| useEffect(() => { | ||
| if (!visible) return; | ||
| claimSlot(slot); | ||
| return () => dropSlot(slot); | ||
| }, [visible, slot]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files"
fd -a 'lazy-visible\.tsx|heavy-widgets\.spec\.ts|webgl-budget\.ts|.*budget.*|.*visible.*' . | sed 's#^\./##' | sort | head -200
echo
echo "Inspect lazy-visible outline and relevant lines"
file=$(fd 'lazy-visible\.tsx$' . | head -1)
echo "FILE=$file"
wc -l "$file"
ast-grep outline "$file" --view compact || true
sed -n '1,140p' "$file" | cat -n
echo
echo "Inspect budget implementation"
budget_file=$(fd 'webgl-budget\.ts$' . | head -1)
echo "BUDGET_FILE=$budget_file"
wc -l "$budget_file"
sed -n '1,140p' "$budget_file" | cat -n
echo
echo "Inspect e2e relevant section"
e2e_file=$(fd 'heavy-widgets\.spec\.ts$' . | head -1)
echo "E2E_FILE=$e2e_file"
wc -l "$e2e_file"
sed -n '300,355p' "$e2e_file" | cat -nRepository: alfredo1996/neoboard
Length of output: 11584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for liveSlotCount usages/tests"
rg -n "liveSlotCount|claimSlot|dropSlot|evictOverBudget|WEBGL_WIDGET_BUDGET|resetSlotRegistry" app/src app/e2e -S
echo
echo "LazyVisible tests"
fd 'lazy-visible\.test\.tsx$' . | while read -r f; do
echo "FILE=$f"
wc -l "$f"
sed -n '1,260p' "$f" | cat -n
done
echo
echo "Budget tests relevant sections"
fd 'webgl-budget\.test\.ts$' . | while read -r f; do
echo "FILE=$f"
wc -l "$f"
sed -n '1,260p' "$f" | cat -n
done
echo
echo "Read-only behavioral model verifier"
python3 - <<'PY'
WEBGL_WIDGET_BUDGET = 8
class Slot:
def __init__(self, name):
self.name = name
self.on_screen = True
self.ejected = False
def evict(self):
self.ejected = True
live = [] # preserves insertion order like Set iteration
def claim_slot(slot):
if slot not in live:
live.append(slot)
def drop_slot(slot):
live.remove(slot)
def evict_over_budget():
over = len(live) - WEBGL_WIDGET_BUDGET
idx = 0
while idx < len(live) and over > 0:
slot = live[idx]
if slot.on_screen:
idx += 1
continue
slot_on_screen = slot.on_screen
live.remove(slot)
# slot.evict() would happen outside live
slot.ejected = True
over -= 1
return idx
# 2 visible slots at budget
for i in range(WEBGL_WIDGET_BUDGET):
claim_slot(Slot(f"old-{i}"))
live[i].on_screen = False
# Simulate existing slots leaving viewport and then replacement visible slot claiming.
for s in live:
s.on_screen = False
claim_slot(Slot("replacement"))
# In current LazyVisible behavior, claimSlot is not followed by evictOverBudget here.
print({
"budget": WEBGL_WIDGET_BUDGET,
"live_before_after_leave_and_claim": len(live),
"old_offscreen_mounted": sum(not s.on_screen and not s.ejected for s in live),
"replacement_mounted": any(s.name == "replacement" and not s.ejected for s in live) and not live[-1].ejected,
# The observed invariant violation: replacement claim happens at or above budget with no eviction.
"current_claim_enforces_budget": len(live) - WEBGL_WIDGET_BUDGET <= 0 and not any(not s.ejected and not s.on_screen for s in live),
})
# Apply eviction after claim to demonstrate proposed invariant.
replacement = next(s for s in live if s.name == "replacement")
replacement.on_screen = False
claim_slot(Slot("replacement2"))
evicted_count = evict_over_budget()
dead_offscreen_after_proposed = sum(not s.on_screen and s.ejected for s in live)
print({
"evicted_count_after_claim_and_evict": evicted_count,
"replacement2_ejected": any(s.name == "replacement2" and s.ejected for s in live),
"live_size_after_proposed": len(live),
"dead_offscreen_after_proposed": dead_offscreen_after_proposed,
})
PYRepository: alfredo1996/neoboard
Length of output: 17412
Enforce the WebGL budget when a slot is claimed.
Current claim-time behavior can hold at or above the budget and keep old off-screen widgets mounted until another observer event calls evictOverBudget(). Run eviction immediately after claimSlot(slot), or enforce the budget inside claimSlot.
Add a regression that first leaves existing at-budget slots off-screen, then claims a replacement slot above the budget and verifies the eviction path.
📍 Affects 2 files
app/src/components/lazy-visible.tsx#L84-L88(this comment)app/e2e/heavy-widgets.spec.ts#L330-L332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/components/lazy-visible.tsx` around lines 84 - 88, Update the
useEffect claim flow in app/src/components/lazy-visible.tsx: after
claimSlot(slot), immediately invoke the existing eviction path or enforce the
budget within claimSlot, while preserving cleanup via dropSlot(slot). Add the
regression in app/e2e/heavy-widgets.spec.ts to leave at-budget slots off-screen,
claim a replacement above the budget, and verify that the over-budget eviction
occurs.
Source: Coding guidelines
Comment on lines
+365
to
+384
| responses: { | ||
| 200: { | ||
| description: "Widgets re-assigned", | ||
| content: { | ||
| "application/json": { | ||
| schema: { | ||
| type: "object", | ||
| properties: { | ||
| dashboardsUpdated: { type: "integer" }, | ||
| widgetsReassigned: { type: "integer" }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| 400: R.badRequest, | ||
| 401: R.unauthorized, | ||
| 403: R.forbidden, | ||
| 404: R.notFound, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the 500 response and the envelope wrapper.
Two gaps against the route behavior:
- The route wraps failures in
handleRouteError, which returns 500. The route test atapp/src/app/api/dashboards/[id]/reassign-connection/__tests__/route.test.tslines 284-293 asserts 500. Add500: R.serverError, as/api/querydoes on line 472. - The route returns
apiSuccess(result), which emits{ data, error, meta }. The documented 200 schema places the counts at the top level. Nest them underdata, matching the "Envelope containing …" style used for the API-key endpoints.
📘 Proposed change
responses: {
description: "Widgets re-assigned",
content: {
"application/json": {
schema: {
type: "object",
properties: {
- dashboardsUpdated: { type: "integer" },
- widgetsReassigned: { type: "integer" },
+ data: {
+ type: "object",
+ properties: {
+ dashboardsUpdated: { type: "integer" },
+ widgetsReassigned: { type: "integer" },
+ },
+ },
+ error: { $ref: "`#/components/schemas/EnvelopeError`" },
+ meta: { type: "object", nullable: true },
},
},
},
},
},
+ 500: R.serverError,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| responses: { | |
| 200: { | |
| description: "Widgets re-assigned", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| dashboardsUpdated: { type: "integer" }, | |
| widgetsReassigned: { type: "integer" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 400: R.badRequest, | |
| 401: R.unauthorized, | |
| 403: R.forbidden, | |
| 404: R.notFound, | |
| }, | |
| responses: { | |
| 200: { | |
| description: "Widgets re-assigned", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| data: { | |
| type: "object", | |
| properties: { | |
| dashboardsUpdated: { type: "integer" }, | |
| widgetsReassigned: { type: "integer" }, | |
| }, | |
| }, | |
| error: { $ref: "`#/components/schemas/EnvelopeError`" }, | |
| meta: { type: "object", nullable: true }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 400: R.badRequest, | |
| 401: R.unauthorized, | |
| 403: R.forbidden, | |
| 404: R.notFound, | |
| 500: R.serverError, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/api/openapi-spec.ts` around lines 365 - 384, Update the
reassign-connection endpoint response schema to wrap dashboardsUpdated and
widgetsReassigned under a data object, matching apiSuccess’s { data, error, meta
} envelope and the API-key endpoint style. Add the missing 500 response using
R.serverError alongside the existing 400–404 responses.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (20)
app/src/lib/db/connection-reassign.ts (1)
94-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the composite predicate in parentheses.
For an empty source,
widgetMatchesSourcereturns two predicates joined byANDwithout an enclosing group. All three current call sites place the fragment last in aWHEREorWHEN, so the result is correct today. If a future edit combines the fragment withOR, or appends another predicate, theANDbinds unexpectedly and the content-only exclusion is silently lost. Parentheses make the fragment self-contained.♻️ Proposed change
return sql` - COALESCE(widget->>'connectionId', '') = '' - AND widget->>'chartType' NOT IN (${sql.join( - CONTENT_ONLY_CHART_TYPES.map((t) => sql`${t}`), - sql`, `, - )}) + ( + COALESCE(widget->>'connectionId', '') = '' + AND widget->>'chartType' NOT IN (${sql.join( + CONTENT_ONLY_CHART_TYPES.map((t) => sql`${t}`), + sql`, `, + )}) + ) `;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/db/connection-reassign.ts` around lines 94 - 105, Update widgetMatchesSource so the empty fromConnectionId branch wraps its combined COALESCE and chart-type exclusion predicates in an enclosing parenthesized SQL expression, keeping both conditions grouped as one self-contained fragment while leaving the non-empty branch unchanged.app/e2e/dashboard-connection-reassign.spec.ts (1)
85-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
anywith the PlaywrightPagetype.The coding guidelines require a comment that explains every
any. Theeslint-disableline suppresses the rule but does not state a reason.Pageis already available from@playwright/test, so the annotation is not needed.♻️ Proposed change
-const openCardMenu = async ( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - page: any, - name: string, -) => { +const openCardMenu = async (page: Page, name: string) => {Add the type import:
+import type { Page } from "`@playwright/test`";As per coding guidelines: "Use TypeScript strict mode and do not use
anywithout a comment explaining why."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/dashboard-connection-reassign.spec.ts` around lines 85 - 96, Update the openCardMenu helper to use Playwright’s Page type for its page parameter, importing Page from `@playwright/test`. Remove the unnecessary eslint-disable directive and any annotation while preserving the existing menu interaction.Source: Coding guidelines
app/src/app/api/dashboards/import/__tests__/route.test.ts (1)
403-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a status assertion to the mixed-case test.
The other two new tests assert the response status. This one reads
body.data.unassignedWidgetCountwithout checking that the import succeeded, so a validation regression surfaces as a confusing property-access failure rather than a status mismatch.💚 Proposed change
const body = await res.json(); + expect(res.status).toBe(201); // w1 was skipped; w2/w3 are markdown+iframe and never wanted a connection. expect(body.data.unassignedWidgetCount).toBe(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/api/dashboards/import/__tests__/route.test.ts` around lines 403 - 420, Add a response status assertion in the mixed-case test before reading body.data.unassignedWidgetCount, matching the success-status assertions in the neighboring import tests. Keep the existing unassignedWidgetCount assertion unchanged.app/src/app/api/dashboards/[id]/reassign-connection/__tests__/route.test.ts (1)
173-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test cannot prove the visibility guarantee it names.
The
whereclause is mocked away, somakeSelectChain([])only re-tests the same "target lookup returned no row" path as the test on lines 159-168. It does not exercise the owner/visibility='shared'/admin predicate. Consider asserting on the built predicate instead, or cover the visibility rule in the DB-level test suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/api/dashboards/`[id]/reassign-connection/__tests__/route.test.ts around lines 173 - 181, The private-connection test currently mocks away the visibility predicate and duplicates the missing-target case. Update the test around POST and makeSelectChain to verify the owner/shared/admin filtering predicate, or move this visibility guarantee to the database-level test suite, while keeping the 404 response and no-widget-reassignment assertions.app/src/components/dashboard-edit-toolbar.tsx (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared Filters toggle button. Both new toolbars contain the same parameter-toggle button with the same
aria-labellogic and the same arbitrarytext-[10px]count badge. One shared component removes the copy and keeps the badge styling consistent.
app/src/components/dashboard-edit-toolbar.tsx#L89-L104: replace this block with the sharedParameterToggleButtonand passhasParameters,parameterCount,showParameterBar, andonToggleParameterBar.app/src/components/dashboard-view-toolbar.tsx#L130-L145: replace this identical block with the same shared component, and move the count badge styling into it so both toolbars use one token set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-edit-toolbar.tsx` around lines 89 - 104, Extract a shared ParameterToggleButton component containing the Filters toggle, aria-label logic, and count badge styling. In app/src/components/dashboard-edit-toolbar.tsx lines 89-104 and app/src/components/dashboard-view-toolbar.tsx lines 130-145, replace the duplicated blocks with ParameterToggleButton, passing hasParameters, parameterCount, showParameterBar, and onToggleParameterBar; move the badge styling into the shared component.app/e2e/edit-page-preservation.spec.ts (2)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the layout PUT response.
If this request fails, the test fails later at an unrelated visibility assertion. The
createTestDashboardfixture inapp/e2e/fixtures.tsalready throws on a non-OK response. Apply the same check here for a clear failure message.♻️ Proposed fix
- await page.request.put(`/api/dashboards/${id}`, { + const res = await page.request.put(`/api/dashboards/${id}`, { data: { layoutJson: { version: 2, pages: [1, 2, 3, 4].map(pageWith), }, }, }); + expect(res.ok(), `layout PUT failed: ${res.status()}`).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/edit-page-preservation.spec.ts` around lines 42 - 49, Check the response returned by the layout PUT request in the edit-page preservation test and fail immediately with the same non-OK response handling used by createTestDashboard in the fixture. Preserve the existing request payload and make the response validation occur before subsequent visibility assertions.
110-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the upper clamp too.
The comment states that
99clamps to the last page. The loop only proves some tab is selected. Add the name-level assertion forPage 4so a regression that clamps99to the first page still fails.♻️ Proposed addition
await page.goto(`/${id}/edit?page=abc`); await expect( page.getByRole("tab", { name: "Page 1", selected: true }), ).toBeVisible({ timeout: 15_000 }); + await page.goto(`/${id}/edit?page=99`); + await expect( + page.getByRole("tab", { name: "Page 4", selected: true }), + ).toBeVisible({ timeout: 15_000 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/edit-page-preservation.spec.ts` around lines 110 - 118, Update the pagination assertions in the edit-page preservation test to verify that navigating with page=99 selects the last page, specifically the “Page 4” tab. Keep the existing assertions for -1 and abc resolving to “Page 1” unchanged, and ensure the upper-clamp case checks the tab name rather than only selected-tab visibility.Source: Coding guidelines
app/src/components/__tests__/dashboard-connection-dialog.test.tsx (2)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain each
any, or drop it.The coding guidelines require a comment that explains why
anyis used. Theeslint-disablelines suppress the rule but do not state a reason. Either add the reason, or type the fixtures asPartial<DashboardDetail>(already the declared type ofmockDashboard) and the override map asPartial<typeof props>, which removes the need forany.Also applies to: 150-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx` around lines 137 - 139, Update the renderDialog test helper and the related override fixture at the additional location to avoid unexplained any usage: type dashboard fixtures as Partial<DashboardDetail> and override maps as Partial<typeof props>, removing the eslint-disable comments while preserving the existing render behavior.Source: Coding guidelines
203-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion does not prove the test name.
The name claims the dashboard query is not fired. The body only asserts that the title is absent. Add a spy on the mocked
useDashboardand assert it received an empty id, or rename the test to describe what it checks.♻️ Proposed change
+const useDashboardSpy = vi.fn(); vi.mock("`@/hooks/use-dashboards`", () => ({ - useDashboard: (id: string) => ({ + useDashboard: (id: string) => (useDashboardSpy(id), { data: id ? mockDashboard : undefined, isLoading: false, }),it("stays inert while closed so the dashboard query is not fired", () => { renderDialog({ open: false, dashboardId: "" }); expect(screen.queryByText("Change connection")).not.toBeInTheDocument(); + expect(useDashboardSpy).toHaveBeenCalledWith(""); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx` around lines 203 - 206, Update the test “stays inert while closed so the dashboard query is not fired” to verify its stated behavior by spying on the mocked useDashboard call and asserting it receives an empty dashboard id when rendered closed; retain the existing visibility assertion if useful, or rename the test only if that query assertion cannot be added.app/src/components/dashboard-view-toolbar.tsx (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive feedback when the custom interval is rejected.
handleCustomApplyreturns silently for a non-numeric value or for a value below 5. The user sees no change and no message. Show a short inline hint, or disable the Set button while the value is invalid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-view-toolbar.tsx` around lines 90 - 96, Update handleCustomApply to provide user feedback when customSeconds is non-numeric or below the 5-second minimum, using a short inline hint or disabling the Set button while invalid. Preserve the existing onApplyInterval, reset, and dropdown-closing behavior for valid values.app/src/hooks/__tests__/use-connections.test.ts (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
enabledflag.The call site is correctly migrated to the options object, but no test asserts the new
enabledbehavior. Add two cases against the mockeduseQueryconfig:enableddefaults totruewhen no options are passed, anduseConnections({ enabled: false })forwardsenabled: false. This is the flagDashboardWorkspacerelies on to skip the connections request in view mode.As per coding guidelines: "Every new behavior, bug fix, and edge case must have a test, written before implementation, following Red → Green → Refactor."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/hooks/__tests__/use-connections.test.ts` at line 67, Add coverage in the useConnections tests for the new enabled option: assert the mocked useQuery configuration defaults enabled to true when no options are provided, and assert useConnections({ enabled: false }) forwards enabled: false. Keep the assertions focused on the generated query config and follow the existing test structure.Source: Coding guidelines
component/src/components/composed/dashboard-grid.tsx (1)
132-136: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGive the
colsprop a stable identity.
colsForEveryBreakpoint(cols)builds a fresh object on every render, while the siblinglayoutsprop is memoized anddefaultBreakpointsis a module constant. Memoize it for consistency, and to avoid handingResponsiveGridLayouta new config reference on each render.♻️ Memoize the column map
const layouts = React.useMemo( () => ({ lg: layout, md: layout, sm: layout, xs: layout }), [layout], ); + + const gridCols = React.useMemo(() => colsForEveryBreakpoint(cols), [cols]);- cols={colsForEveryBreakpoint(cols)} + cols={gridCols}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@component/src/components/composed/dashboard-grid.tsx` around lines 132 - 136, Memoize the result of colsForEveryBreakpoint(cols) before rendering ResponsiveGridLayout, using the existing component memoization pattern and cols as its dependency. Pass the memoized column map to the cols prop so its object identity remains stable when cols is unchanged.app/src/app/(dashboard)/[id]/layout.tsx (1)
20-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive
editModefrom the exact segment.
pathname.endsWith("/edit")also matches the view route of a dashboard whose id is literallyedit, because that pathname is/edit. IDs are server-generated, so this is unlikely, but an exact comparison removes the ambiguity and expresses the intent directly.♻️ Compare against the concrete segment
const { id } = useParams<{ id: string }>(); const pathname = usePathname(); - const editMode = pathname.endsWith("/edit"); + const editMode = pathname === `/${id}/edit`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/`(dashboard)/[id]/layout.tsx around lines 20 - 22, Update the editMode derivation in the dashboard layout to use an exact pathname comparison with the edit route, rather than pathname.endsWith("/edit"). Preserve editMode as true only for the concrete edit pathname and false when the dashboard id itself is "edit".app/src/components/__tests__/dashboard-workspace.test.tsx (1)
897-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the positive Cmd+S case.
The test name says "saves in edit mode only", but the body only pins the view-mode negative. No test fires Cmd+S in edit mode, so a regression that disables the shortcut wiring (for example a wrong
disabledexpression atdashboard-workspace.tsxLine 581) still passes. The Save button test at Line 663 does not exercise the shortcut path.💚 Proposed addition
it("Cmd+S saves in edit mode only", () => { render(<DashboardWorkspace id="d1" editMode={false} />); fireEvent.keyDown(document, { key: "s", metaKey: true }); expect(mockMutateAsync).not.toHaveBeenCalled(); }); + + it("Cmd+S saves in edit mode", async () => { + pathname = "/d1/edit"; + render(<DashboardWorkspace id="d1" editMode={true} />); + fireEvent.keyDown(document, { key: "s", metaKey: true }); + await vi.waitFor(() => + expect(mockMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ id: "d1", expectedVersion: 1 }), + ), + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/dashboard-workspace.test.tsx` around lines 897 - 901, Extend the “Cmd+S saves in edit mode only” test to render DashboardWorkspace with editMode enabled, fire the same Cmd+S key event, and assert mockMutateAsync is called. Keep the existing view-mode assertion so both enabled and disabled shortcut behavior are covered.app/src/lib/shared/url-params.ts (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stray
ponytail:marker from the comment.The explanation itself is useful. The
ponytail:prefix reads as an internal scratch marker and has no meaning to a future reader.♻️ Proposed tweak
- // ponytail: add every companion key regardless of parameterType — + // Add every companion key regardless of parameterType — // a `select` simply never writes them, so the extra entries are inert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/shared/url-params.ts` around lines 93 - 94, Update the comment near the companion-key handling to remove the stray “ponytail:” prefix while preserving the explanation about adding every companion key regardless of parameterType.app/src/stores/__tests__/dashboard-store.test.ts (1)
63-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the upper clamp and the
NaNbranch.
setLayoutclamps both ends and has an explicitisNaNguard (dashboard-store.tsLine 104). The new tests only pin the lower bound and the zero case. Add an above-range index and aNaNindex so the other two branches cannot regress silently.💚 Proposed addition
+ it("setLayout clamps an initialPageIndex past the last page", () => { + const newLayout = { + version: 2 as const, + pages: [ + { id: "p1", title: "A", widgets: [], gridLayout: [] }, + { id: "p2", title: "B", widgets: [], gridLayout: [] }, + ], + }; + useDashboardStore.getState().setLayout(newLayout, 99); + expect(useDashboardStore.getState().activePageIndex).toBe(1); + }); + + it("setLayout treats a NaN initialPageIndex as 0", () => { + const newLayout = { + version: 2 as const, + pages: [{ id: "p1", title: "A", widgets: [], gridLayout: [] }], + }; + useDashboardStore.getState().setLayout(newLayout, Number.NaN); + expect(useDashboardStore.getState().activePageIndex).toBe(0); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/stores/__tests__/dashboard-store.test.ts` around lines 63 - 86, Add tests for setLayout covering an initialPageIndex above the available page range, asserting activePageIndex clamps to the last page, and a NaN initialPageIndex, asserting it follows the explicit NaN fallback behavior. Keep the existing negative and zero cases unchanged.app/e2e/parameter-url-sync.spec.ts (1)
11-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the seeded connection id into
fixtures.ts.
fixtures.tsalready exports shared seed constants such asALICE,TEST_NEO4J_BOLT_URL, andTEST_PG_PORT. Add the Neo4jconn-neo4j-001constant there and reuse it fromparamWidgetso seed-ID changes stay central.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/parameter-url-sync.spec.ts` around lines 11 - 26, Move the seeded Neo4j connection ID into the shared constants in fixtures.ts, alongside ALICE, TEST_NEO4J_BOLT_URL, and TEST_PG_PORT, then import and reuse that constant in paramWidget instead of the inline "conn-neo4j-001" value.app/src/components/dashboard-workspace.tsx (1)
317-353: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSend
expectedVersionwith the auto-refresh settings write.
applyIntervalsends{...serverLayout, settings: newSettings}withoutexpectedVersion, so the API does not apply the optimistic-lock guard. If another save changes the dashboard afterserverLayoutwas loaded, this write overwrites that layout.Forward
expectedVersion: dashboard?.versionand handle the resulting conflict through the existing save-error toast path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-workspace.tsx` around lines 317 - 353, Update applyInterval to include expectedVersion: dashboard?.version in the updateDashboard payload, using the current dashboard version for optimistic locking. Preserve the existing persist queue and route mutation failures, including version conflicts, through the existing classifySaveError toast path.app/e2e/editor-maximize.spec.ts (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the test-only CodeMirror property.
The lint suppression identifies the rule but does not explain why
HTMLElementrequiresany. Model__cmViewwith a local structural type, or document a specific reason if that type cannot represent the runtime shape. Verify the property shape against the active CodeMirror wrapper.As per coding guidelines, do not use
anywithout a comment explaining why.Proposed type-safe replacement
+type CodeMirrorHost = HTMLElement & { + __cmView?: { state: { doc: { length: number } } }; +}; + function docLength(editor: import("`@playwright/test`").Locator) { return editor.evaluate( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (el: HTMLElement) => (el as any).__cmView?.state.doc.length ?? -1, + (el: HTMLElement) => + (el as CodeMirrorHost).__cmView?.state.doc.length ?? -1, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/editor-maximize.spec.ts` around lines 31 - 36, Update docLength to replace the eslint-suppressed any cast with a local structural type matching the active CodeMirror wrapper’s __cmView property and its state.doc.length shape. Access the typed property from the HTMLElement while preserving the existing -1 fallback, and verify the modeled runtime shape against the wrapper.Source: Coding guidelines
app/src/components/__tests__/widget-editor-modal-maximize.test.tsx (1)
252-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
parameter-selectexit branch.
isParamSelectis an independent maximize guard fromisContentOnly. This test switches only to"markdown". Add a test that maximizes the editor, switches to"parameter-select", and verifies that the preview mounts and the grid returns to two columns.As per coding guidelines, every new behavior, bug fix, and edge case must have a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/__tests__/widget-editor-modal-maximize.test.tsx` around lines 252 - 266, Add a dedicated test alongside the existing maximize behavior test that expands the editor, switches the chart type to "parameter-select" via useWidgetEditorStore, and verifies widget-preview is rendered and gridColumns() returns the two-column layout. Keep the setup and assertions consistent with the existing markdown test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 22-24: Update the review:local description in the command list to
reference release/1.5, matching the script configuration in package.json instead
of release/1.4.
In `@app/e2e/grid.spec.ts`:
- Around line 187-204: The drag in the test must produce a real grid layout
change before clicking Save. Update the drag coordinates in the flow around
item, boundingBox, and page.mouse.move so the movement exceeds half a column and
changes gridLayout, preserving the existing drag-and-save assertions.
In `@app/src/components/dashboard-view-toolbar.tsx`:
- Line 123: Update the Badge rendering in the dashboard toolbar so it is
conditionally rendered only when the optional role value is set; preserve the
existing secondary variant and role text when a value is available.
In `@app/src/components/dashboard-workspace.tsx`:
- Around line 796-806: Update the DashboardContainer props in the page-rendering
block so refetchInterval is enabled only when isActive; preserve editMode’s
false interval behavior and pass viewRefetchInterval only for the active page,
matching the existing onLayoutChange gating.
In `@app/src/components/lazy-visible.tsx`:
- Around line 84-88: Update the useEffect claim flow in
app/src/components/lazy-visible.tsx: after claimSlot(slot), immediately invoke
the existing eviction path or enforce the budget within claimSlot, while
preserving cleanup via dropSlot(slot). Add the regression in
app/e2e/heavy-widgets.spec.ts to leave at-budget slots off-screen, claim a
replacement above the budget, and verify that the over-budget eviction occurs.
In `@app/src/lib/api/openapi-spec.ts`:
- Around line 365-384: Update the reassign-connection endpoint response schema
to wrap dashboardsUpdated and widgetsReassigned under a data object, matching
apiSuccess’s { data, error, meta } envelope and the API-key endpoint style. Add
the missing 500 response using R.serverError alongside the existing 400–404
responses.
---
Nitpick comments:
In `@app/e2e/dashboard-connection-reassign.spec.ts`:
- Around line 85-96: Update the openCardMenu helper to use Playwright’s Page
type for its page parameter, importing Page from `@playwright/test`. Remove the
unnecessary eslint-disable directive and any annotation while preserving the
existing menu interaction.
In `@app/e2e/edit-page-preservation.spec.ts`:
- Around line 42-49: Check the response returned by the layout PUT request in
the edit-page preservation test and fail immediately with the same non-OK
response handling used by createTestDashboard in the fixture. Preserve the
existing request payload and make the response validation occur before
subsequent visibility assertions.
- Around line 110-118: Update the pagination assertions in the edit-page
preservation test to verify that navigating with page=99 selects the last page,
specifically the “Page 4” tab. Keep the existing assertions for -1 and abc
resolving to “Page 1” unchanged, and ensure the upper-clamp case checks the tab
name rather than only selected-tab visibility.
In `@app/e2e/editor-maximize.spec.ts`:
- Around line 31-36: Update docLength to replace the eslint-suppressed any cast
with a local structural type matching the active CodeMirror wrapper’s __cmView
property and its state.doc.length shape. Access the typed property from the
HTMLElement while preserving the existing -1 fallback, and verify the modeled
runtime shape against the wrapper.
In `@app/e2e/parameter-url-sync.spec.ts`:
- Around line 11-26: Move the seeded Neo4j connection ID into the shared
constants in fixtures.ts, alongside ALICE, TEST_NEO4J_BOLT_URL, and
TEST_PG_PORT, then import and reuse that constant in paramWidget instead of the
inline "conn-neo4j-001" value.
In `@app/src/app/`(dashboard)/[id]/layout.tsx:
- Around line 20-22: Update the editMode derivation in the dashboard layout to
use an exact pathname comparison with the edit route, rather than
pathname.endsWith("/edit"). Preserve editMode as true only for the concrete edit
pathname and false when the dashboard id itself is "edit".
In `@app/src/app/api/dashboards/`[id]/reassign-connection/__tests__/route.test.ts:
- Around line 173-181: The private-connection test currently mocks away the
visibility predicate and duplicates the missing-target case. Update the test
around POST and makeSelectChain to verify the owner/shared/admin filtering
predicate, or move this visibility guarantee to the database-level test suite,
while keeping the 404 response and no-widget-reassignment assertions.
In `@app/src/app/api/dashboards/import/__tests__/route.test.ts`:
- Around line 403-420: Add a response status assertion in the mixed-case test
before reading body.data.unassignedWidgetCount, matching the success-status
assertions in the neighboring import tests. Keep the existing
unassignedWidgetCount assertion unchanged.
In `@app/src/components/__tests__/dashboard-connection-dialog.test.tsx`:
- Around line 137-139: Update the renderDialog test helper and the related
override fixture at the additional location to avoid unexplained any usage: type
dashboard fixtures as Partial<DashboardDetail> and override maps as
Partial<typeof props>, removing the eslint-disable comments while preserving the
existing render behavior.
- Around line 203-206: Update the test “stays inert while closed so the
dashboard query is not fired” to verify its stated behavior by spying on the
mocked useDashboard call and asserting it receives an empty dashboard id when
rendered closed; retain the existing visibility assertion if useful, or rename
the test only if that query assertion cannot be added.
In `@app/src/components/__tests__/dashboard-workspace.test.tsx`:
- Around line 897-901: Extend the “Cmd+S saves in edit mode only” test to render
DashboardWorkspace with editMode enabled, fire the same Cmd+S key event, and
assert mockMutateAsync is called. Keep the existing view-mode assertion so both
enabled and disabled shortcut behavior are covered.
In `@app/src/components/__tests__/widget-editor-modal-maximize.test.tsx`:
- Around line 252-266: Add a dedicated test alongside the existing maximize
behavior test that expands the editor, switches the chart type to
"parameter-select" via useWidgetEditorStore, and verifies widget-preview is
rendered and gridColumns() returns the two-column layout. Keep the setup and
assertions consistent with the existing markdown test.
In `@app/src/components/dashboard-edit-toolbar.tsx`:
- Around line 89-104: Extract a shared ParameterToggleButton component
containing the Filters toggle, aria-label logic, and count badge styling. In
app/src/components/dashboard-edit-toolbar.tsx lines 89-104 and
app/src/components/dashboard-view-toolbar.tsx lines 130-145, replace the
duplicated blocks with ParameterToggleButton, passing hasParameters,
parameterCount, showParameterBar, and onToggleParameterBar; move the badge
styling into the shared component.
In `@app/src/components/dashboard-view-toolbar.tsx`:
- Around line 90-96: Update handleCustomApply to provide user feedback when
customSeconds is non-numeric or below the 5-second minimum, using a short inline
hint or disabling the Set button while invalid. Preserve the existing
onApplyInterval, reset, and dropdown-closing behavior for valid values.
In `@app/src/components/dashboard-workspace.tsx`:
- Around line 317-353: Update applyInterval to include expectedVersion:
dashboard?.version in the updateDashboard payload, using the current dashboard
version for optimistic locking. Preserve the existing persist queue and route
mutation failures, including version conflicts, through the existing
classifySaveError toast path.
In `@app/src/hooks/__tests__/use-connections.test.ts`:
- Line 67: Add coverage in the useConnections tests for the new enabled option:
assert the mocked useQuery configuration defaults enabled to true when no
options are provided, and assert useConnections({ enabled: false }) forwards
enabled: false. Keep the assertions focused on the generated query config and
follow the existing test structure.
In `@app/src/lib/db/connection-reassign.ts`:
- Around line 94-105: Update widgetMatchesSource so the empty fromConnectionId
branch wraps its combined COALESCE and chart-type exclusion predicates in an
enclosing parenthesized SQL expression, keeping both conditions grouped as one
self-contained fragment while leaving the non-empty branch unchanged.
In `@app/src/lib/shared/url-params.ts`:
- Around line 93-94: Update the comment near the companion-key handling to
remove the stray “ponytail:” prefix while preserving the explanation about
adding every companion key regardless of parameterType.
In `@app/src/stores/__tests__/dashboard-store.test.ts`:
- Around line 63-86: Add tests for setLayout covering an initialPageIndex above
the available page range, asserting activePageIndex clamps to the last page, and
a NaN initialPageIndex, asserting it follows the explicit NaN fallback behavior.
Keep the existing negative and zero cases unchanged.
In `@component/src/components/composed/dashboard-grid.tsx`:
- Around line 132-136: Memoize the result of colsForEveryBreakpoint(cols) before
rendering ResponsiveGridLayout, using the existing component memoization pattern
and cols as its dependency. Pass the memoized column map to the cols prop so its
object identity remains stable when cols is unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cb95f87-119e-4372-8629-33d121918670
📒 Files selected for processing (85)
.claude/CLAUDE.md.claude/agents/project-architect.md.claude/hooks/check-boundaries.sh.claude/hooks/check-migration-guard.sh.claude/skills/next/SKILL.md.github/workflows/ci.yml.gitignoreCHANGELOG.mdapp/e2e/charts.spec.tsapp/e2e/dashboard-connection-reassign.spec.tsapp/e2e/edit-page-preservation.spec.tsapp/e2e/edit-scroll-position.spec.tsapp/e2e/editor-maximize.spec.tsapp/e2e/fixtures.tsapp/e2e/grid.spec.tsapp/e2e/heavy-widgets.spec.tsapp/e2e/parameter-url-sync.spec.tsapp/e2e/styling-rules.spec.tsapp/playwright.config.tsapp/src/__tests__/test-environment-boundary.test.tsapp/src/app/(dashboard)/[id]/__tests__/layout.test.tsxapp/src/app/(dashboard)/[id]/edit/page.tsxapp/src/app/(dashboard)/[id]/layout.tsxapp/src/app/(dashboard)/[id]/page.tsxapp/src/app/(dashboard)/page.tsxapp/src/app/api/connections/[id]/reassign/__tests__/route.test.tsapp/src/app/api/connections/[id]/reassign/route.tsapp/src/app/api/dashboards/[id]/reassign-connection/__tests__/route.test.tsapp/src/app/api/dashboards/[id]/reassign-connection/route.tsapp/src/app/api/dashboards/import/__tests__/route.test.tsapp/src/app/api/dashboards/import/route.tsapp/src/components/__tests__/dashboard-connection-dialog.test.tsxapp/src/components/__tests__/dashboard-workspace.test.tsxapp/src/components/__tests__/lazy-visible.test.tsxapp/src/components/__tests__/widget-editor-modal-maximize.test.tsxapp/src/components/dashboard-connection-dialog.tsxapp/src/components/dashboard-edit-toolbar.tsxapp/src/components/dashboard-view-toolbar.tsxapp/src/components/dashboard-workspace.tsxapp/src/components/lazy-visible.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/query-editor-panel.test.tsxapp/src/components/widget-editor/__tests__/widget-preview-panel.test.tsxapp/src/components/widget-editor/query-editor-panel.tsxapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/hooks/__tests__/use-connections.test.tsapp/src/hooks/use-connections.tsapp/src/hooks/use-dashboards.tsapp/src/hooks/use-widget-templates.tsapp/src/lib/__tests__/db/connection-reassign.test.tsapp/src/lib/__tests__/docs-accuracy.test.tsapp/src/lib/__tests__/shared/url-params.test.tsapp/src/lib/api/openapi-spec.tsapp/src/lib/dashboard/__tests__/import-follow-up.test.tsapp/src/lib/dashboard/import-follow-up.tsapp/src/lib/db/__tests__/tenant-scope.test.tsapp/src/lib/db/connection-reassign.tsapp/src/lib/shared/url-params.tsapp/src/lib/widget/__tests__/scroll-to-widget.test.tsapp/src/lib/widget/__tests__/scroll-to-widget.test.tsxapp/src/lib/widget/__tests__/webgl-budget.test.tsapp/src/lib/widget/content-only-chart.tsapp/src/lib/widget/scroll-to-widget.tsapp/src/lib/widget/webgl-budget.tsapp/src/plugins/graph/component.tsxapp/src/stores/__tests__/dashboard-store.test.tsapp/src/stores/dashboard-store.tscli/src/__tests__/commands/status.test.tscli/src/__tests__/lib/docker.test.tscli/src/commands/status.tscli/src/lib/docker.tscomponent/design-tokens.csscomponent/src/__tests__/reduced-motion.test.tscomponent/src/charts/__tests__/map-chart.test.tsxcomponent/src/charts/map-chart.tsxcomponent/src/components/composed/__tests__/dashboard-grid.test.tsxcomponent/src/components/composed/dashboard-grid.tsxcomponent/src/components/ui/__tests__/alert-dialog.test.tsxcomponent/src/components/ui/__tests__/dialog.test.tsxcomponent/src/components/ui/alert-dialog.tsxcomponent/src/components/ui/dialog.tsxcomponent/stories/ui/alert-dialog.stories.tsxcomponent/stories/ui/animation-centring.tscomponent/stories/ui/dialog.stories.tsxpackage.json
🛑 Comments failed to post (1)
.claude/CLAUDE.md (1)
22-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the documented review base with the script.
package.jsonnow setsreview:localtorelease/1.5. Line 24 still describesrelease/1.4. Update the description to the same base.Proposed fix
- npm run review:local # CodeRabbit review of committed changes vs release/1.4 + npm run review:local # CodeRabbit review of committed changes vs release/1.5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.npm run verify # Local CI mirror: typecheck + lint + all unit suites npm run sonar:local # Scan the current branch against SonarCloud (real gate) npm run review:local # CodeRabbit review of committed changes vs release/1.5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/CLAUDE.md around lines 22 - 24, Update the review:local description in the command list to reference release/1.5, matching the script configuration in package.json instead of release/1.4.
|
This was referenced Aug 5, 2026
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.
Consolidates
release/1.5intodev, following the same path as #1233 (release/1.3) and #1366 (release/1.4).This is a release-timing decision, not just a flake fix — v1.5 is still in progress (milestone #31 has 40 open issues). It brings 16 commits of in-flight v1.5 work into
devearlier than a normal end-of-release consolidation would.Why now
devfails E2E shard 1 roughly one run in three, oncharts.spec.tsgraph tests, withexpect(locator).not.toBeVisible(). The cause was diagnosed and fixed in #1462 (#1458): the ten Radix overlay primitives never honouredprefers-reduced-motion, and Radix keeps an overlay mounted until its exit animation reportsanimationend— which stalls when an NVL/WebGL widget mounts alongside the close.That fix landed on
release/1.5, sodevkept the flake. It blocked #1463, a dependency PR with nothing to do with overlays, and a re-run of the failed shard failed again. Every PR targetingdevwas paying this toll, and each failure needed a human to confirm it was the known flake rather than a real regression.What comes with it
fix(component)fix(dashboard)feat(dashboard)feat(widget-editor)fix(dashboard)fix(dashboard)fix(charts)fix(charts)fix(component)fix(cli)neoboard statushealth probe and container count (#1379)choreCLAUDE.mdmoved into.claude/(#1393)docsNote #1388 is breaking for existing shared links: a URL carrying
?param_…for a widget that never opted into URL sync will no longer reproduce those values. That is documented in the CHANGELOG entry it shipped with.Merge check
Merges cleanly into
dev— verified locally withgit merge --no-commit --no-ff, zero conflicts..github/workflows/ci.ymlandpackage.jsonauto-merged.Follow-on
#1463 (five validated dependency bumps) currently fails E2E shard 1 on this exact flake. Once this lands it should go green on a re-run.
Summary by CodeRabbit
New Features
Bug Fixes