feat: add aggregate parameter input with presets #7146
Conversation
|
|
Warning Rate limit exceeded@catalyst17 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new React component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BlueprintPlayground
participant ParameterInput
participant AggregateParameterInput
User->>BlueprintPlayground: Opens playground UI
BlueprintPlayground->>ParameterInput: Render input for parameter "aggregate"
ParameterInput->>AggregateParameterInput: Render with endpointPath and form field
AggregateParameterInput->>User: Shows input and preset dropdown
User->>AggregateParameterInput: Selects preset or enters value
AggregateParameterInput->>ParameterInput: Updates form value
ParameterInput->>BlueprintPlayground: Form value updated
✨ 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 (
|
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. |
github-actions
bot
added
the
Playground
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7146 +/- ##
==========================================
- Coverage 55.63% 55.62% -0.01%
==========================================
Files 902 902
Lines 58190 58190
Branches 4098 4098
==========================================
- Hits 32373 32369 -4
- Misses 25712 25716 +4
Partials 105 105
🚀 New features to boost your workflow:
|
size-limit report 📦
|
catalyst17
force-pushed
the
feat_add_aggregate_parameter_input_with_presets
branch
from
a1e9e1a to
75d8e69
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
catalyst17
force-pushed
the
feat_add_aggregate_parameter_input_with_presets
branch
from
75d8e69 to
50f59b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's 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: 1
🧹 Nitpick comments (3)
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx (1)
525-526: Consider the impact of placeholder logic change.The placeholder logic has been simplified from a context-aware URL-based calculation to using
param.description || param.name. While this works well for the aggregate parameter, it might reduce the user experience for path parameters where the original logic provided more meaningful placeholders like{param_name}or:param_name.Consider preserving the original placeholder logic for non-aggregate parameters:
- placeholder={param.description || param.name} + placeholder={param.name === "aggregate" ? (param.description || param.name) : (url.includes(`{${param.name}}`) ? `{${param.name}}` : url.includes(`:${param.name}`) ? `:${param.name}` : "Value")}Or extract the placeholder logic into a helper function for better maintainability.
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx (2)
138-148: Consider more flexible endpoint matching.The current exact string matching for endpoint paths is brittle and won't handle dynamic path segments or variations in endpoint structure.
Consider implementing more flexible pattern matching:
-const ENDPOINT_SPECIFIC_PRESETS: Record<string, Preset[]> = { - "/v1/transactions": GENERAL_TRANSACTIONS_PRESETS, - "/v1/wallets/{wallet_address}/transactions": WALLET_TRANSACTIONS_PRESETS, - "/v1/events": EVENTS_PRESETS, - "/v1/blocks": BLOCKS_PRESETS, - // Add more endpoint paths and their specific presets here -}; +const ENDPOINT_PATTERNS: Array<{pattern: RegExp, presets: Preset[]}> = [ + { pattern: /\/v1\/transactions$/, presets: GENERAL_TRANSACTIONS_PRESETS }, + { pattern: /\/v1\/wallets\/[^/]+\/transactions/, presets: WALLET_TRANSACTIONS_PRESETS }, + { pattern: /\/v1\/events/, presets: EVENTS_PRESETS }, + { pattern: /\/v1\/blocks/, presets: BLOCKS_PRESETS }, +]; function getAggregatePresets(endpointPath: string): Preset[] { - return ENDPOINT_SPECIFIC_PRESETS[endpointPath] || DEFAULT_AGGREGATE_PRESETS; + const match = ENDPOINT_PATTERNS.find(({pattern}) => pattern.test(endpointPath)); + return match?.presets || DEFAULT_AGGREGATE_PRESETS; }
180-186: Consider edge case handling for preset selection.The current implementation only calls
onChangewhenselectedValueis truthy, but consider what happens if a user wants to clear their selection.Consider adding a "Clear" option or handling empty selection:
onValueChange={(selectedValue) => { - if (selectedValue) { - onChange({ target: { value: selectedValue } }); - } + onChange({ target: { value: selectedValue || "" } }); }}Or add a clear option to the select dropdown.
🛑 Comments failed to post (1)
apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx (1)
19-37: 🛠️ Refactor suggestion
Improve default preset usability.
The default presets contain placeholder text like
your_field_herewhich requires user modification. This might confuse users who expect working presets.Consider either:
- Removing the custom field presets from defaults and only including them in endpoint-specific collections where field names are known
- Adding clear documentation or help text indicating these are templates
const DEFAULT_AGGREGATE_PRESETS: Preset[] = [ { label: "Count All Items", value: "count() AS count_all" }, { label: "Sum (gas_used)", value: "sum(gas_used) AS total_gas_used" }, { label: "Average (gas_used)", value: "avg(gas_used) AS avg_gas_used" }, { label: "Min (gas_used)", value: "min(gas_used) AS min_gas_used" }, { label: "Max (gas_used)", value: "max(gas_used) AS max_gas_used" }, - // Presets for a user-defined field - { - label: "Count (custom field)", - value: "count(your_field_here) AS count_custom", - }, - { label: "Sum (custom field)", value: "sum(your_field_here) AS sum_custom" }, - { - label: "Average (custom field)", - value: "avg(your_field_here) AS avg_custom", - }, - { label: "Min (custom field)", value: "min(your_field_here) AS min_custom" }, - { label: "Max (custom field)", value: "max(your_field_here) AS max_custom" }, ];📝 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.const DEFAULT_AGGREGATE_PRESETS: Preset[] = [ { label: "Count All Items", value: "count() AS count_all" }, { label: "Sum (gas_used)", value: "sum(gas_used) AS total_gas_used" }, { label: "Average (gas_used)", value: "avg(gas_used) AS avg_gas_used" }, { label: "Min (gas_used)", value: "min(gas_used) AS min_gas_used" }, { label: "Max (gas_used)", value: "max(gas_used) AS max_gas_used" }, ];🤖 Prompt for AI Agents
In apps/playground-web/src/app/insight/[blueprint_slug]/aggregate-parameter-input.client.tsx around lines 19 to 37, the default aggregate presets include placeholders like "your_field_here" which can confuse users expecting functional presets. To fix this, remove these custom field presets from the default list and instead include them only in endpoint-specific preset collections where actual field names are known, or alternatively add clear inline comments or UI help text indicating these presets are templates requiring user modification.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(1 hunks)apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx(5 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
🧰 Additional context used
🪛 Biome (1.9.4)
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx
[error] 454-454: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend placeholder with an underscore.
(lint/correctness/noUnusedVariables)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx (4)
38-38: LGTM!The import statement is correctly added for the new
AggregateParameterInputcomponent.
525-526: LGTM!The placeholder change improves user experience by showing descriptive text, and the
endpointPathprop enables the new aggregate parameter functionality.
591-591: LGTM!The
endpointPathprop is correctly added to the interface to support the new aggregate parameter functionality.
628-638: LGTM!The conditional rendering for aggregate parameters is correctly implemented. All required props are passed to the
AggregateParameterInputcomponent, enabling the specialized functionality for aggregate parameter handling.
apps/playground-web/src/app/insight/[blueprint_slug]/blueprint-playground.client.tsx
Outdated
Show resolved
Hide resolved
catalyst17
force-pushed
the
feat_add_aggregate_parameter_input_with_presets
branch
from
50f59b3 to
548f0c3
Compare
catalyst17
force-pushed
the
feat_add_aggregate_parameter_input_with_presets
branch
from
548f0c3 to
72e77b2
Compare
PR-Codex overview
This PR introduces the
AggregateParameterInputcomponent to handle aggregate parameters in theBlueprintPlayground. It adds new presets for various data types and modifies theParameterSectionto utilize this new component, enhancing the functionality of the parameter input system.Detailed summary
AggregateParameterInputcomponent to handle aggregate parameters.ParameterSectionto include a check for theaggregateparameter and render the new component.ParameterSectionto useparam.descriptionorparam.name.endpointPathprop toAggregateParameterInputfor context on preset selection.Summary by CodeRabbit