feat: enhance aggregate parameter input with multi-select presets #7168
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AggregateParameterInput
participant MultiSelect
User->>AggregateParameterInput: Focuses or types in manual input
AggregateParameterInput->>AggregateParameterInput: Parses input string into array
AggregateParameterInput->>MultiSelect: Updates selected presets
User->>MultiSelect: Selects or deselects presets
MultiSelect->>AggregateParameterInput: Updates selected presets array
AggregateParameterInput->>AggregateParameterInput: Updates manual input string (comma-separated)
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
Playground
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7168 +/- ##
=======================================
Coverage 55.66% 55.66%
=======================================
Files 904 904
Lines 58400 58400
Branches 4116 4116
=======================================
Hits 32510 32510
Misses 25784 25784
Partials 106 106
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (3)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx (3)
5-11: Remove unused imports to keep bundle lean and satisfy linters
MultiSelect,useEffect, andXIconare never referenced in this file. Shipping unused symbols bloats the bundle and fails Biome/ES-lint withnoUnusedImports.-import { MultiSelect } from "@/components/blocks/multi-select"; -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useCallback, useMemo, useRef, useState } from "react"; … -import { ChevronDownIcon, SearchIcon, XIcon } from "lucide-react"; +import { ChevronDownIcon, SearchIcon } from "lucide-react";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 6-6: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
[error] 11-11: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.(lint/correctness/noUnusedImports)
190-200: Accessibility: pop-over trigger lacks an aria labelScreen-reader users will hear “button” with no description. Add
aria-label(ortitle) so the purpose is announced:-<Button +<Button + aria-label="Open aggregation presets"
238-248: Consider allowing badge removal for better UXOnce multiple presets are picked, the only way to remove one is manual text editing.
A small “×” on each badge that deletes the corresponding token would be more intuitive.(If you agree, I can draft a quick implementation.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
[error] 5-5: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 6-6: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 11-11: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 202-202: These CSS classes should be sorted.
Unsafe fix: Sort the classes.
(lint/nursery/useSortedClasses)
[error] 204-204: These CSS classes should be sorted.
Unsafe fix: Sort the classes.
(lint/nursery/useSortedClasses)
[error] 209-209: These CSS classes should be sorted.
Unsafe fix: Sort the classes.
(lint/nursery/useSortedClasses)
[error] 223-223: These CSS classes should be sorted.
Unsafe fix: Sort the classes.
(lint/nursery/useSortedClasses)
[error] 228-228: These CSS classes should be sorted.
Unsafe fix: Sort the classes.
(lint/nursery/useSortedClasses)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Size
- GitHub Check: Analyze (javascript)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
Outdated
Show resolved
Hide resolved
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
catalyst17
force-pushed
the
feat_enhance_aggregate_parameter_input_with_multi-select_presets
branch
from
a82e7b1 to
6961615
Compare
catalyst17
force-pushed
the
feat_enhance_aggregate_parameter_input_with_multi-select_presets
branch
from
6961615 to
03342b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx (2)
530-533: Consider extracting hardcoded description to a constant.The hardcoded description text for the aggregate parameter works functionally but could impact maintainability. Consider extracting this to a configuration object or constant to improve code organization.
+const PARAMETER_OVERRIDES = { + aggregate: { + description: "Aggregation(s). You can type in multiple, separated by a comma, or select from the presets", + example: "count() AS count_all, countDistinct(address) AS unique_addresses" + } +} as const; // Later in the component: - {param.name === "aggregate" - ? "Aggregation(s). You can type in multiple, separated by a comma, or select from the presets" - : description} + {PARAMETER_OVERRIDES[param.name]?.description ?? description}
554-557: Consider using CSS custom properties for magic number.The hardcoded
top-[21px]value appears to be a magic number for tooltip positioning. While functional, consider using a CSS custom property or extracting this value to improve maintainability.+const AGGREGATE_TOOLTIP_TOP_OFFSET = "top-[21px]"; className={cn( "-translate-y-1/2 absolute top-1/2 right-2 hidden h-auto w-auto p-1.5 text-muted-foreground opacity-50 hover:opacity-100 lg:flex", - param.name === "aggregate" && "top-[21px]", + param.name === "aggregate" && AGGREGATE_TOOLTIP_TOP_OFFSET, )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx(2 hunks)apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Size
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx (1)
541-544: LGTM! Example text provides clear guidance.The custom example for the aggregate parameter demonstrates typical SQL aggregation functions, which helps users understand the expected format for multiple comma-separated aggregations.
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
Show resolved
Hide resolved
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
please see check the comments - also lets modify the UI to be more like: https://v0.dev/chat/p2LWAMXAsD6
|
addressed all in the next PR |
Merge activity
|
) ## [Dashboard] Feature: Enhance Aggregate Parameter Input with Multi-Select Functionality ## Notes for the reviewer This PR replaces the simple dropdown select with a more robust multi-select component for aggregate parameter inputs. The new implementation allows users to select multiple presets and displays them as badges. ## How to test Test the new aggregate parameter input by: 1. Opening the insight blueprint page 2. Selecting multiple presets from the dropdown 3. Verifying that selected values appear as badges 4. Testing the search functionality within the preset selector 5. Confirming that manual input still works alongside preset selection <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced aggregate parameter input with combined manual entry and multi-select for presets. - Added search functionality within the multi-select component for easier preset selection. - Updated parameter descriptions and examples for "aggregate" to clarify multi-selection usage. - **Refactor** - Made input options optional and improved synchronization between manual input and multi-select. - Adjusted tooltip positioning for better UI alignment on aggregate parameters. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- start pr-codex --> --- ## PR-Codex overview This PR enhances the `ParameterSection` and `AggregateParameterInput` components to provide dynamic descriptions and examples based on the parameter type. It also refines the handling of selected values and manual input for aggregations, replacing the `Select` component with a `MultiSelect`. ### Detailed summary - Updated `description` and `exampleToShow` in `ParameterSection` based on `param.name`. - Introduced `MultiSelect` in `AggregateParameterInput` for selecting aggregation presets. - Added state management for manual input and selected values. - Removed the `Select` component in favor of a more flexible input system. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex -->
graphite-app
bot
force-pushed
the
feat_enhance_aggregate_parameter_input_with_multi-select_presets
branch
from
b1d6b81 to
f7331f1
Compare
graphite-app
bot
deleted the
feat_enhance_aggregate_parameter_input_with_multi-select_presets
branch
[Dashboard] Feature: Enhance Aggregate Parameter Input with Multi-Select Functionality
Notes for the reviewer
This PR replaces the simple dropdown select with a more robust multi-select component for aggregate parameter inputs. The new implementation allows users to select multiple presets and displays them as badges.
How to test
Test the new aggregate parameter input by:
Summary by CodeRabbit
PR-Codex overview
This PR enhances the
ParameterSectionandAggregateParameterInputcomponents by adding conditional rendering for descriptions and examples based on theparam.name. It also refines theAggregateParameterInputto use aMultiSelectfor aggregation presets, improving user interaction.Detailed summary
ParameterSectionbased onparam.name.ParameterSectionconditionally foraggregate.AggregateParameterInputto useMultiSelectinstead ofSelect.AggregateParameterInput.MultiSelect.