fix(params): parameters, grid & tables polish bundle (#1055) - #1083
Conversation
Dogfood session 5 P2 findings: - Parameter name normalization: strip a leading param_ so a name like "param_status" yields the clean token $param_status (not the doubled $param_param_status) and a STATUS label — applied at the editor hint, the param preview, and on save. - Widget-editor preview mirrors the dashboard "Waiting for parameters…" state when the query references unbound $param_x tokens, instead of running the literal token and surfacing a raw `syntax error at or near "$"` (guards handlePreview + a panel waiting state). - Data grid: rows-per-page trigger shows the active page size; the hide-columns menu humanizes snake_case headers (Total_spend → "Total Spend"); data rows get a subtle hover highlight (P3). Tests: unit for normalizeParamName + humanizeHeader; jsdom for the preview waiting state, the page-size label, and humanized column labels. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 35 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds two new utility functions — ChangesWidget Editor Parameter Fixes
Data Grid Polish
Sequence Diagram(s)sequenceDiagram
participant Editor as WidgetEditorModal
participant Ready as allReferencedParamsReady
participant Panel as WidgetPreviewPanel
participant Hook as useAutoPreview
Editor->>Ready: check(query, allParamValues)
Ready-->>Editor: false (unbound $param_x tokens)
Editor->>Panel: waitingForParams=true
Panel-->>Editor: renders "Waiting for parameters…" (no chart)
Hook->>Ready: check(query, allParamValues) during handlePreview
Ready-->>Hook: false
Hook-->>Hook: early return (preview skipped)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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/widget-editor/widget-preview-panel.tsx`:
- Around line 258-259: The error icon condition at line 331 only checks
previewQuery.isError and does not account for the waitingForParams state,
allowing both the "Waiting for parameters..." message and a stale error icon to
display simultaneously. Update the error icon condition to suppress the error
affordance when waitingForParams is active by adding a check for
!waitingForParams to the existing previewQuery.isError condition, so the error
icon is not shown while waiting for parameters to be provided.
In `@component/src/components/composed/data-grid.tsx`:
- Around line 376-381: A new hover affordance behavior has been added to
non-grouped rows in the data-grid component (the "transition-colors
hover:bg-muted/40" classes applied when isGrouped is false), but there is no
corresponding test to verify this behavior. Add a Vitest test case to the
DataGrid test file that verifies non-grouped data rows receive the correct hover
affordance styling by asserting that rows rendered with isGrouped=false include
the "transition-colors" and "hover:bg-muted/40" classes (or equivalent hover
behavior). This test should use jsdom to inspect the rendered DOM and confirm
the className contains these values.
🪄 Autofix (Beta)
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
Run ID: ba6ac706-6f12-43e6-afd5-33677be7e0b5
📒 Files selected for processing (16)
app/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/widget-preview-panel.test.tsxapp/src/components/widget-editor/parameter-config-section.tsxapp/src/components/widget-editor/parameter-preview.tsxapp/src/components/widget-editor/use-auto-preview.tsapp/src/components/widget-editor/use-widget-save.tsapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/lib/parameter/__tests__/normalize-param-name.test.tsapp/src/lib/parameter/normalize-param-name.tscomponent/src/components/composed/__tests__/data-grid-pagination.test.tsxcomponent/src/components/composed/__tests__/data-grid-view-options.test.tsxcomponent/src/components/composed/data-grid-pagination.tsxcomponent/src/components/composed/data-grid-view-options.tsxcomponent/src/components/composed/data-grid.tsxcomponent/src/lib/__tests__/humanize-header.test.tscomponent/src/lib/humanize-header.ts
| className={cn( | ||
| isGrouped | ||
| ? "bg-muted/50 font-medium" | ||
| : // Subtle hover affordance on data rows (#1055). | ||
| "transition-colors hover:bg-muted/40", | ||
| )} |
There was a problem hiding this comment.
Add test coverage for the new non-grouped row hover behavior.
This change introduces new UI behavior, but there’s no matching test in the provided DataGrid test updates. Please add a jsdom/Vitest assertion that non-grouped rows receive the hover affordance class.
As per coding guidelines, "**/*.ts: Write tests before implementation (Red → Green → Refactor). Every new behavior, bug fix, and edge case gets 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 `@component/src/components/composed/data-grid.tsx` around lines 376 - 381, A
new hover affordance behavior has been added to non-grouped rows in the
data-grid component (the "transition-colors hover:bg-muted/40" classes applied
when isGrouped is false), but there is no corresponding test to verify this
behavior. Add a Vitest test case to the DataGrid test file that verifies
non-grouped data rows receive the correct hover affordance styling by asserting
that rows rendered with isGrouped=false include the "transition-colors" and
"hover:bg-muted/40" classes (or equivalent hover behavior). This test should use
jsdom to inspect the rendered DOM and confirm the className contains these
values.
Source: Coding guidelines
…1055) The preview guard now imports allReferencedParamsReady; the test mocked use-widget-query without it, so the helper was undefined and every handlePreview-based test threw. Add a faithful mock. Full app (3018) + component (1428) unit suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#1055) Address CodeRabbit: - Preview panel: gate the 'Query failed' error icon on !waitingForParams so it can't show alongside 'Waiting for parameters…'. - Add a DataGrid jsdom test asserting non-grouped rows get the hover class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
2 participants
Closes #1055
Dogfood session 5 (#895) P2 parameters, grid & tables polish bundle.
Changes
Parameters
normalizeParamNamestrips a leadingparam_, so naming a parameterparam_statusproduces the clean token$param_status(not the doubled$param_param_status) and aSTATUSlabel. Applied at the editor reference hint, the parameter preview, and on save.$param_xtokens, instead of running the literal token and surfacing a rawsyntax error at or near "$"(guardshandlePreview+ a panel waiting render). Related to [P2] Widget error state renders raw DB driver error to all roles — no sanitization #1050.Data grid / tables
Total_spend→ "Total Spend"), consistent with "Customer"/"City" (humanizeHeader).Tests
normalizeParamName(incl. single-strip),humanizeHeader.tsc +
npm run lint(rooteslint .) clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes