chore(release): consolidate release/1.5 into dev - #1493
Merged
Conversation
…) (#1471) * fix(ci): cap vitest worker forks at 50% of available parallelism (#1240) Vitest defaults to roughly one worker fork per core. Each fork in these two packages loads jsdom + React + the component library, so a many-core machine runs out of headroom before it runs out of cores: workers start failing to boot with "Timeout waiting for worker to respond", and the test files assigned to them are dropped. Measured on a 10-core machine, app suite, same commit: default 889s 96 failed / 3284 passed 256 files collected capped (50%) 37s 0 failed / 3480 passed 265 files collected Nine files never ran in the uncapped run. The summary still looks like a complete pass, so the missing coverage is invisible unless you diff the file count. The 96 "failures" were contention, not assertions — 28818ms on a test with a 5s budget (#1459), 15129ms on another. All green when capped. component behaves the same way: 206/206 files, 2114/2114 tests, 40s. Set as a percentage rather than a fixed number so CI runners, with far fewer vCPUs, scale down with it instead of inheriting a value tuned for a laptop. Verified that a root-level maxWorkers propagates into the `projects` blocks — both suites above ran with no CLI flag. Refs #1240, #1459, #1469 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ci): quote one consistent capped figure in the maxWorkers comment The comment cited 28s, which was a --maxWorkers=4 CLI run, while the CHANGELOG and PR cited 37s for the 50% setting this file actually configures. Both numbers were real but they were different runs, so presenting them as one measurement was misleading. Uses the 37s figure throughout, and records the CI result now that it is known: the unit job went 8m08s -> 8m20s, so the feared slowdown on 4-vCPU runners did not materialise. 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>
`count` returned the group's row count while every other aggregation
skipped nulls, so `sum / count` disagreed with `avg` on any column a
LEFT JOIN had left holes in. A group of [100, 200, null] reported
count: 3, sum: 300, avg: 150 — and count is typically the denominator
a reader divides by, so the error propagated downstream.
Counts non-null values now, matching SQL's COUNT(col). The output key
is `${column}_count`, named for the column, so the column form is the
only one it could have meant.
The existing test used null-free sample data and passed either way.
The new invariant — sum / count === avg for every group — fails on the
old code and pins the semantics.
Closes #1414
Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…#1396) (#1467) `numberFormat: "percent"` appended "%" and scaled nothing, so a KPI computing 401 / 2000 rendered "0.2%" where the true share was 20.05% — 100x too small, rendered cleanly, indistinguishable from a real value. The contract was never stated and the two option surfaces documented opposite readings: table's label said "Percent (12%)" (already 0-100) while the seeded reference tile was authored as a ratio. That is why the demo itself was wrong. percent now takes a ratio and scales it, using Intl.NumberFormat's own style: "percent" — which also brings locale grouping (12.5 -> 1,250%). With decimalPlaces unset it caps at 2 rather than Intl's default of 0, since rounding 20.05% to 20% is the same silent precision loss. BREAKING: queries that pre-multiplied by 100 now render 100x too large. Two existing tests encoded the old contract and are inverted; the boundary case (1 -> "100%") pins the convention so it cannot flip back silently. Closes #1396 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
A `category, series, value` result — what GROUP BY a, b naturally produces — rendered a chart that looked plausible and was wrong. resolveValueKeys treats every non-label column as a value series, so `series` became a series whose cells all coerced to null: duplicated x labels, a ghost legend entry with no bars, and no stacking despite stackMode being set. Line drew one value line zig-zagging across repeated x values, which reads as a spiky time series but is artifact. validateNumericValueColumns rejects a value column that has non-null cells and none that parse, naming it and showing the pivot that fixes it. An all-null column stays legal — that is a sparse series, what a LEFT JOIN produces, and the reason collectAllKeys unions keys. Resolution goes through resolveValueKeys, the same call the transform makes, so the validator cannot disagree with what is plotted. validate() now takes the column mapping, as transformWithMapping already did. Without it the validator has to assume positional defaults and would reject a result whose text columns the user had already mapped away. The 11 reference tiles using this shape are rewritten to wide format so the stackMode tiles demonstrate real stacking. page-line-v-28's rightAxisSeries: "shipped" now names a column that exists. Closes #1400 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…meric KPI column (#1397) (#1473) Both options were present in the widget editor, implemented in the component library, and silently dropped in between. Setting either did nothing, with no error. The issue's diagnosis was wrong: it blamed the Zod schema stripping unknown keys. Both schemas end in .passthrough(), which preserves them — verified against the real schema. The value died at the plugin's explicit prop mapping, which never forwarded it. Adding the key to the schema, the fix as filed, would have changed nothing on screen. trendEnabled could never have been forwarded as-is either: SingleValueChart takes a computed { direction, label }, not a boolean, and it needs the previous period — which transformToValueData had already discarded by collapsing the result to one scalar. That collapse is also the worse bug. It took the first column positionally, so following the editor's own "requires 2 rows" instruction on a `label, value` query rendered the date as the KPI: `$2026-03`. It now picks the first numeric column, falling back to a non-numeric one only when the result has none, and carries the second row's value as `previous`. Adds a ratchet asserting every option a chart type advertises is read by its plugin. The one proposed in the issue — comparing option keys against settingsSchema — would have passed while both bugs were live, since the schema was never the seam. The ratchet found 14 unforwarded options, not 2. The remaining 10 are allowlisted and tracked in #1472; the list may only shrink. The seeded reference tiles needed no changes — they were authored correctly and only the plumbing was broken. Closes #1397 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Every page a user had visited kept polling, so query load scaled with browsing history instead of with what was on screen. Measured on Chart Reference, same page and same 18 visible widgets both times: only page 1 opened 18 tiles 16 POSTs / 40s after touring 6 pages 86 tiles 77 POSTs / 40s 4.81x the database traffic for an identical view. That load is refresh-tier work against the customer's database — the exact category the scheduler exists to shed — so one user browsing a large dashboard could crowd out interactive queries for everyone on that connector. `isActive` was already computed, already used two lines above for the `hidden` class and one line below to gate `onLayoutChange`. It just never gated the refresh. Pages stay mounted, which is deliberate: tab switches remain instant and do not re-query. They simply stop polling while hidden. Scope: this is part one of the issue. The unbounded mount cache — 131 tiles and 87 canvases after 11 tabs, 146.5 MB peak heap — is a separate behaviour change with a state-loss edge case around unsaved form input, and is tracked separately. The E2E was verified in both directions. Against the unfixed build it fails with "7 queries after touring vs 2 on a fresh load", on the initial run and the retry; with the fix it passes. An earlier version of it passed either way: all three pages shared one query, and use-widget-query keys on (connection, database, query, params, staleTime) with no widget id, so TanStack collapsed them into a single fetch. The pages now query distinct data, and the reason is commented in the fixture. Refs #1419 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…) (#1478) * fix(params): apply a parameter widget's configured Default value (#1421) extractParamDefaults walked the layout correctly, had its own unit test, and had zero production callers. The editor's "Default value" field wrote into the saved layout and was read only by that test. The seeded Chart Playground carries 21 defaults across 8 pages and showed "Waiting for parameters..." on every chart until each knob was set by hand — one of the first things a new user opens. Seeded on load, filling only parameters not already set. That yields the required precedence with no ordering machinery: restore and URL are both applied before the layout finishes loading, so whatever they put in the store is already there when defaults run. URL param > restored session > widget default > unset A once-per-dashboard ref guard stops a cleared parameter snapping back; without it a knob would be impossible to clear. Adds "default" to ParameterSource rather than labelling these "selector-widget" — nothing branches on the field, but recording a value the user never picked as a user selection is false provenance. Third instance of this shape after #1234 and #1388, so a narrow guard asserts this helper has a production caller. The generalised version the issue asked for — any lib/ export reachable only from tests fails the build — measured at ~69 current matches, mostly legitimate (_reset* test hooks, Zod fragments composed in-file, Drizzle enums). That needs a real dead-export tool with a baseline and is filed as #1477. The E2E was verified in both directions: against the unfixed build it fails on both assertions ("Waiting for parameters" present, table absent); with the fix it passes. Closes #1421 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(params): make two guards actually fail on broken code (#1421) CodeRabbit review on #1478 found two tests that passed against the very things they were meant to protect. Both verified, both now proven to fail when the protection is removed. 1. "does not re-apply the default after the user clears it" passed with the ref guard deleted. `rerender` reuses the same mocked dashboard object, so `serverLayout` keeps its identity, the effect's deps never change, and the effect never re-runs — nothing exercised the guard. It now hands back a fresh object so the effect fires again. 2. The production-caller guard matched raw text, and this feature's own explanatory comment names `extractParamDefaults` in prose. Deleting the import and the call left the comment behind and the guard still passed. It now strips comments and requires both an import binding and a call expression, and handles grep's exit-1-on-no-match so the custom failure message survives. Also asserts `sourceType === "default"` on the seeded value, so seeding cannot silently record it as a user selection. Verified by neutering each protection in turn: the ref-guard test fails with the guard removed, and the caller guard fails with the import and call deleted while the prose comment remains. 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>
…#1395) (#1479) A table whose saved groupBy was an array rendered flat: no group rows, no aggregates, no error. Individual rows then read as group totals — a wrong answer with nothing on screen to suggest the grouping had been dropped. Three seeded Chart Reference tiles holding groupBy: ["region"] were pixel-identical to the ungrouped tile. parseGroupByColumns took only a string, and its `typeof groupBy === "string" ? groupBy : ""` guard silently turned an array into an empty string, which then parsed to undefined. The issue's diagnosis is inverted and worth correcting, because it points the test plan at the wrong path. The widget editor's column-multi-select emits `vals.join(",")` — a string — which the parser already accepted, so configuring grouping through the UI always worked. The array reaches storage from seeded layouts, imported dashboards and NeoDash conversions. An editor-driven E2E, which the issue asked for as the test that covers "the path that is broken", would have driven the working path and passed identically with and without this fix. The E2E here drives the array path and is verified in both directions: it fails on the pre-fix parser with the group toggle absent, on the initial run and the retry, and passes after. The seeded tiles keep their array form rather than being rewritten to strings — they work now and keep that shape exercised in the demo, while imports keep producing it regardless. Also widens the caller's cast from `as string` to `as string | string[]`, so the type reflects what actually arrives instead of hiding it. Closes #1395 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…rations (#1480) Dependabot split one migration into two PRs that can never pass, since each breaks the other's peer range (#1430, #1431). Replaced by a single combined bump, as #1461 called for. astro 5.3.0 -> 7.1.4 starlight 0.31.1 -> 0.41.5 Not a drop-in. Three breaking changes: - starlight 0.33: `social` takes an array of link items, not an object - starlight 0.39: an autogenerated sidebar group can no longer carry a `label` beside `autogenerate`; the label belongs to a group whose `items` holds the autogenerate config - astro 5+: content collections require an explicit loader, so `docsLoader()` is now passed to the docs collection The loader one is the dangerous change. Without it the collection resolves empty and `astro build` STILL EXITS 0 — it emits a lone 404 page and mentions "The collection "docs" does not exist or is empty" in passing. The first green build after this upgrade produced a one-page site. With the docs not yet deployed (#1318), that would have shipped unnoticed. So CI now fails the docs build when it emits fewer than 10 HTML pages. Verified both ways: 73 pages passes, the 1-page broken state fails. The lockfile is regenerated rather than patched — npm kept resolving against the pinned starlight 0.31.1 and refused the peer bump otherwise. Closes #1461 Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
alfredo1996
added
chore
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (47)
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 |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates
release/1.5intodev, following the same path as #1233 (release/1.3), #1366 (release/1.4) and #1464 (the earlier mid-release consolidation of this branch).origin/devhas already been merged intorelease/1.5(cc8971a9) so this PR lands conflict-free.What comes with it
Nine commits — eight correctness fixes and a docs upgrade:
fix(transforms)countcounts values, not rows —sum / countdisagreed withavgon any column a LEFT JOIN had holed (#1414)fix(charts)percentscales ratios instead of appending a sign — a KPI read0.2%where the truth was20.05%. Breaking for queries that pre-multiplied by 100 (#1396)fix(charts)fix(charts)trendEnabledandthresholdZonesactually reach their charts; the KPI picks the numeric column instead of rendering a date as$2026-03(#1397)fix(dashboard)fix(params)fix(table)groupByaccepts an array, so grouping from saved/imported layouts works instead of silently rendering flat (#1395)fix(ci)chore(docs)Conflict
Only
CHANGELOG.md, and only because both branches appended to### Changed— the docs upgrade here, the zod migration ondev. Both entries kept. No source file conflicted: this branch touched charts, plugins and dashboard code, whiledev's single commit touched API routes and manifests.Verification of the merged tree
This is the first time zod 4 and the eight chart fixes exist together, so the combination was verified before pushing rather than after:
The app count is
release/1.5's 3557 plus the two validation-branch tests that came in with the zod work.Note
Merge, not squash — consolidation should preserve the individual commits, as the previous three did.
🤖 Generated with Claude Code