fix(params): apply a parameter widget's configured Default value (#1421) - #1478
Merged
Merged
Conversation
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>
alfredo1996
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughDashboard loads now seed parameter values from widget defaults. URL and restored session values retain precedence, and cleared parameters are not re-seeded. Unit, ratchet, end-to-end, and changelog coverage were added. ChangesParameter default loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServerLayout
participant DashboardWorkspace
participant ParameterStore
participant DashboardWidgets
ServerLayout->>DashboardWorkspace: load dashboard layout
DashboardWorkspace->>DashboardWorkspace: extractParamDefaults
DashboardWorkspace->>ParameterStore: initialize missing parameters as default
ParameterStore->>DashboardWidgets: provide resolved parameter values
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🤖 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 `@app/src/components/__tests__/dashboard-workspace.test.tsx`:
- Around line 1204-1213: The test around renderWithDefaults and
DashboardWorkspace must simulate a refetch before rerendering by returning a new
layout object for the same dashboard id. Update the mocked
dashboard/serverLayout data, then rerender and assert the cleared dimension
remains undefined, ensuring the effect is re-exercised rather than relying on
unchanged mocked data.
- Around line 1161-1166: Update the test “seeds the store from a widget's
configured default” to also assert that parameters.dimension.sourceType equals
"default", while retaining the existing value assertion.
In `@app/src/lib/__tests__/parameter/apply-param-defaults.test.ts`:
- Around line 2-3: Update the production-caller guard in the parameter-defaults
test to parse source syntax rather than rely on raw grep counts, ensuring
comments and strings do not satisfy the check and requiring both a real import
and call expression for extractParamDefaults. Handle zero matches before the
assertion so the custom failure message is emitted, and add or update tests
covering comment/string false positives and the zero-match case.
🪄 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: b484ab3c-79d6-4a00-b12c-2a195a0e4b69
📒 Files selected for processing (6)
CHANGELOG.mdapp/e2e/param-defaults.spec.tsapp/src/components/__tests__/dashboard-workspace.test.tsxapp/src/components/dashboard-workspace.tsxapp/src/lib/__tests__/parameter/apply-param-defaults.test.tsapp/src/stores/parameter-store.ts
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>
|
6 tasks
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.
Closes #1421 · follow-up #1477
The bug
extractParamDefaultswalks the layout and returnsparameterName → defaultValue. It is correct, and it has its own unit test file. It also had zero production callers — every reference inapp/srcwas inside its own test.So the editor's Default value field wrote into the saved layout, round-tripped through export, and was read only by a test. The seeded Chart Playground — 8 pages, 21 configured defaults, one of the first things a new user opens — showed
Waiting for parameters… $param_cat_dimension $param_cat_metric $param_cat_limit_maxon every chart until each knob was set by hand.The fix
Call it, on load, filling only parameters that are not already set.
That single rule produces the required precedence with no ordering machinery:
restoreFromDashboardand the URL effect both run on mount, before the layout has loaded; the defaults effect can only run onceserverLayoutexists, by which point anything they set is already in the store. A once-per-dashboard ref guard stops a cleared parameter snapping straight back — without it, clearing a knob would be impossible."default"is added toParameterSource. Nothing branches on that field, but it exists to record where a value came from, and filing a never-selected default under"selector-widget"is false provenance — the same class of small lie this PR exists to remove.Tests, and two that were wrong first
Unit (5 new) — seeds from the default; URL wins; restored session wins; a cleared parameter is not re-defaulted; a widget with no default is left alone.
E2E — built as a fixture rather than driving the Playground, which is a demo showcase absent from the E2E database. Verified in both directions: against the unfixed build it fails on both assertions (
Waiting for parameterspresent and the table absent); with the fix it passes.Caller guard — asserts
extractParamDefaultshas a non-test importer. Proven both ways withgit grepagainstrelease/1.5: zero production references before, one now.Why not the ratchet the issue asked for
Measured: 69 of 253
lib/exports currently match. The overwhelming majority are legitimate —_resetSchedulerRegistry-style test hooks, Zod fragments composed in-file, Drizzle enums used insideschema.ts. Shipping it would mean shipping an allowlist larger than the signal, and every legitimate test-only helper added later would need an entry.It is a good idea that needs a real dead-export tool (
knip/ts-prune) with a committed baseline, so it is filed as #1477 with the measurements. This PR ships the narrow guard instead: it protects this instance without pretending to close the class.Verification
app3550/3550 · typecheck 0 · lint 0 · targeted E2E green, and red against the unfixed buildNote
E2E runs here needed
rm app/.next/BUILD_IDfirst —global-setupreuses a cached build from whatever branch produced it (#1476).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests