Conversation
|
WalkthroughAdds a Metrics page section to Shortcuts and a fullscreen-chart keyboard shortcut (key "v") gated by hover for QueryWidget; introduces a new ChartBlankState component and replaces several empty-state UIs to use it (QueryResultsChart, BigNumberCard, TSQLResultsTable when configured). Replaces custom chart-type and sort-direction controls with a SegmentedControl, updates SegmentedControl to accept ReactNode labels, extends ShortcutKey variants, adds showHeaderOnEmpty prop to TSQLResultsTable and threads it through QueryWidget, and applies multiple styling/accessibility adjustments (ClientTabs, QueryEditor, chart components). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/primitives/charts/ChartBlankState.tsx`:
- Around line 1-12: The ChartBlankState props reference React.ComponentType but
React is not imported; add a React type import and/or switch to a named type
import to fix type checking: either add "import React from 'react';" at the top
so React.ComponentType<{ className?: string }> resolves, or replace
React.ComponentType with a named import like "import type { ComponentType } from
'react';" and update the prop type to ComponentType<{ className?: string }>;
update the ChartBlankState prop signature accordingly.
In `@apps/webapp/app/components/primitives/ClientTabs.tsx`:
- Line 100: The Tab triggers in ClientTabs are forcing tabIndex={0} on every
trigger and breaking Radix's roving tabindex behavior; remove the explicit
tabIndex prop (or change it to tabIndex={isActive ? 0 : -1}) on the trigger
element(s) so Radix can manage focus, updating the trigger variants at the
locations using tabIndex (references: ClientTabs component and the trigger
element/variant where isActive is available) to restore proper Tab/arrow
keyboard navigation.
🧹 Nitpick comments (4)
apps/webapp/app/components/primitives/SegmentedControl.tsx (1)
81-81: Convert to named export to align with coding guidelines.This file uses a default export, but the coding guidelines specify:
**/*.{ts,tsx,js,jsx}: Use function declarations instead of default exports. Since the file is being touched, converting to a named export is a good opportunity.This requires updating 4 import statements across the codebase:
apps/webapp/app/routes/storybook.charts/route.tsx:18apps/webapp/app/routes/storybook.segmented-control/route.tsx:3apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx:24apps/webapp/app/components/code/ChartConfigPanel.tsx:10♻️ Suggested change
-export default function SegmentedControl({ +export function SegmentedControl({apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
60-63:startsWith("small")is fragile for variant matching.If a future variant like
"smaller"is added, it would incorrectly match the small branch. Consider a set-based check instead.💡 Suggested approach
- const className = variant.startsWith("small") ? "w-2.5 h-4" : "w-2.5 h-4.5"; + const isSmall = variant === "small" || variant === "small/bright"; + const className = isSmall ? "w-2.5 h-4" : "w-2.5 h-4.5";Same pattern for
modifierString.Also applies to: 89-94
apps/webapp/app/components/metrics/QueryWidget.tsx (1)
184-192: Shortcut implementation is functional but could avoid re-registering the hotkey on every fullscreen toggle.The
isFullscreendependency causes the callback (and thus the hotkey registration) to be recreated every time fullscreen toggles. You can move the guard into the functional updater to drop that dependency:💡 Suggested optimization
useShortcutKeys({ shortcut: { key: "v" }, - action: useCallback(() => { - const isHovered = containerRef.current?.matches(":hover"); - if (!isFullscreen && !isHovered) return; - setIsFullscreen((prev) => !prev); - }, [isFullscreen]), + action: useCallback(() => { + const isHovered = containerRef.current?.matches(":hover"); + setIsFullscreen((prev) => { + if (!prev && !isHovered) return prev; + return !prev; + }); + }, []), });apps/webapp/app/components/code/ChartConfigPanel.tsx (1)
10-10: ConvertSegmentedControlto a named export.The coding guidelines require function declarations instead of default exports.
SegmentedControlis currently exported as a default export and should be converted to a named export to align with the project standards. This is a pre-existing pattern, so not blocking.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/primitives/charts/Card.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
🧠 Learnings (7)
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/SegmentedControl.tsxapps/webapp/app/components/primitives/charts/ChartBlankState.tsxapps/webapp/app/components/primitives/charts/BigNumberCard.tsxapps/webapp/app/components/code/ChartConfigPanel.tsxapps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/components/code/QueryResultsChart.tsxapps/webapp/app/components/primitives/ShortcutKey.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/ClientTabs.tsxapps/webapp/app/components/primitives/charts/Card.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{ts,tsx} : Avoid using enums; prefer string unions or const objects instead
Applied to files:
apps/webapp/app/components/primitives/SegmentedControl.tsx
📚 Learning: 2026-02-11T16:50:07.201Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:07.201Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/components/metrics/SaveToDashboardDialog.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Use `useRun`, `useRealtimeRun` and other SWR/realtime hooks from `trigger.dev/react-hooks` for data fetching
Applied to files:
apps/webapp/app/components/metrics/SaveToDashboardDialog.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
📚 Learning: 2026-01-28T16:57:47.620Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 2964
File: apps/webapp/app/components/AskAI.tsx:121-141
Timestamp: 2026-01-28T16:57:47.620Z
Learning: In the trigger.dev webapp codebase, the Button component (apps/webapp/app/components/primitives/Buttons) does not spread unknown props to the underlying <button> element—it only passes specific props (type, disabled, onClick, name, value, ref, form, autoFocus). When using Radix UI's TooltipTrigger with asChild, a span wrapper around the Button is necessary to receive Radix props (aria-describedby, onPointerEnter, onPointerLeave, data-state) while the Button handles its own behavior. Directly making the Button the child of TooltipTrigger with asChild will break accessibility and tooltip functionality.
Applied to files:
apps/webapp/app/components/primitives/ClientTabs.tsx
🧬 Code graph analysis (9)
apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
apps/webapp/app/components/primitives/charts/ChartBlankState.tsx (2)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)
apps/webapp/app/components/primitives/charts/BigNumberCard.tsx (1)
apps/webapp/app/components/primitives/charts/ChartBlankState.tsx (1)
ChartBlankState(4-23)
apps/webapp/app/components/code/ChartConfigPanel.tsx (2)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/SegmentedControl.tsx (1)
SegmentedControl(81-153)
apps/webapp/app/components/code/QueryResultsChart.tsx (1)
apps/webapp/app/components/primitives/charts/ChartBlankState.tsx (1)
ChartBlankState(4-23)
apps/webapp/app/components/code/TSQLResultsTable.tsx (1)
apps/webapp/app/components/primitives/charts/ChartBlankState.tsx (1)
ChartBlankState(4-23)
apps/webapp/app/components/metrics/QueryWidget.tsx (4)
apps/webapp/app/hooks/useShortcutKeys.tsx (1)
useShortcutKeys(28-59)apps/webapp/app/components/primitives/charts/Card.tsx (1)
Card(5-16)apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
ShortcutKey(41-58)
apps/webapp/app/components/primitives/ClientTabs.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
apps/webapp/app/components/primitives/charts/Card.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
🔇 Additional comments (21)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx (1)
58-58: LGTM — focus-visible styling for keyboard accessibility.Clean addition of
focus-customandfocus-visible:!rounded-fullto preserve the pill shape on keyboard focus. Consistent with the broader accessibility improvements in this PR.apps/webapp/app/components/primitives/charts/Card.tsx (1)
18-29: LGTM!The asymmetric padding (
pl-4 pr-3) and inline props type annotation look good.apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsx (1)
170-170: LGTM!Horizontal padding addition aligns with the broader padding consistency updates across chart components.
apps/webapp/app/components/primitives/SegmentedControl.tsx (1)
66-69: WideninglabeltoReactNodeis a nice flexibility improvement.One thing to keep in mind:
RadioGroup.Labelrenders as<p>by default, so passing block-level elements (e.g.,<div>) as the label would produce invalid HTML. This should be fine as long as callers stick to inline content (icons,<span>, text).apps/webapp/app/components/code/QueryResultsChart.tsx (1)
951-967: LGTM!Clean adoption of
ChartBlankStatewith a dynamic icon based onchartType. All hooks remain above the early returns, preserving correct hook ordering.apps/webapp/app/components/primitives/charts/BigNumberCard.tsx (1)
141-143: LGTM!Clean replacement with the new unified
ChartBlankStatecomponent.apps/webapp/app/components/metrics/SaveToDashboardDialog.tsx (1)
141-145: LGTM!Good UX improvement — the green checkmark provides clear visual feedback for the selected dashboard, and the spacer preserves alignment for unselected items.
apps/webapp/app/components/primitives/ClientTabs.tsx (2)
54-54:activationMode="manual"— good for preventing accidental tab switches.This ensures tabs only activate on Enter/Space, not on arrow-key focus. Works well with keyboard-heavy UIs.
193-203: LGTM!Setting
tabIndex={-1}andoutline-noneon tab content correctly removes it from the tab order and prevents a focus ring flash on click.apps/webapp/app/components/code/ChartConfigPanel.tsx (2)
243-266: Clean upgrade to SegmentedControl for chart type.The replacement of the two-button toggle with
SegmentedControlusing icon-enriched labels is clean and consistent with the broader PR direction. LGTM.
518-541: Sort direction SegmentedControl looks good.Nice use of
IconSortAscending/IconSortDescendingwithin the labels for visual clarity.apps/webapp/app/components/primitives/charts/ChartBar.tsx (1)
1-20: Import cleanup looks good.Removing unused imports (
useMemo,ChartConfig,ChartState,cn,ZoomRange) and reorganizing the remaining imports is a clean maintenance change.apps/webapp/app/components/code/TSQLResultsTable.tsx (3)
980-983: Nice adoption ofChartBlankStatefor the empty state.Consistent with the broader PR's move to unified empty-state rendering.
1084-1087: Explicit row height for virtual items is a good fix.Setting
height: ${virtualRow.size}pxensures the row container dimensions match what the virtualizer expects, preventing layout drift.
720-731: Accessibility and layout improvements in CopyableCell.The
focus-visiblestyles for anchors and the flex-based layout changes improve keyboard navigation and cell content alignment.apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
11-24: Good refactor: extractingsmallbase style and addingShortcutKeyVarianttype.Clean DRY improvement and the exported type gives consumers proper type safety.
apps/webapp/app/components/Shortcuts.tsx (1)
196-201: New Metrics page shortcut section looks good.The "v" shortcut for fullscreen toggle is properly documented. Note that "v" is also used on the Logs page for "View full run" — this is fine since they're page-specific contexts.
apps/webapp/app/components/metrics/QueryWidget.tsx (2)
213-214: Container ref and className prop wired correctly.The
groupclass on the container div enables thegroup-hover:opacity-100pattern for the maximize button, andclassNameis properly merged viacn.
230-235: Nice touch showing the shortcut key in the tooltip.Displaying
ShortcutKeyinside the tooltip is good for discoverability.apps/webapp/app/components/query/QueryEditor.tsx (2)
774-787: Background and border updates for embedded QueryWidgets look good.Switching to
bg-background-bright, removing inner padding, and applyingborder-0via the newclassNameprop creates a cleaner embedded appearance within the query editor.
1371-1371: Brighter config panel labels improve contrast.Changing from
text-text-dimmedtotext-text-brighton the BigNumberConfigPanel labels is a consistent improvement matching theConfigFieldlabel update in ChartConfigPanel (line 552).Also applies to: 1393-1393, 1421-1421, 1455-1455, 1466-1466
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import { cn } from "~/utils/cn"; | ||
| import { Paragraph } from "../Paragraph"; | ||
|
|
||
| export function ChartBlankState({ | ||
| icon: Icon, | ||
| message, | ||
| className, | ||
| }: { | ||
| icon?: React.ComponentType<{ className?: string }>; | ||
| message: string; | ||
| className?: string; | ||
| }) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if the file exists and read its full content
echo "=== Reading ChartBlankState.tsx ==="
cat -n apps/webapp/app/components/primitives/charts/ChartBlankState.tsx
echo ""
echo "=== Checking tsconfig.json in apps/webapp ==="
find apps/webapp -name "tsconfig.json" -exec cat {} \;
echo ""
echo "=== Checking for jsxImportSource in tsconfig ==="
rg "jsxImportSource" apps/webapp --type jsonRepository: triggerdotdev/trigger.dev
Length of output: 1780
🏁 Script executed:
#!/bin/bash
# Check global.d.ts
echo "=== Checking global.d.ts ==="
find apps/webapp -name "global.d.ts" -exec cat {} \;
echo ""
echo "=== Check if TypeScript compilation would pass with this file ==="
# Look for any other patterns of React.ComponentType usage in the webapp
echo "Searching for other React.ComponentType usages in apps/webapp:"
rg "React\.ComponentType" apps/webapp/app --type tsx --type ts -B 2 | head -50Repository: triggerdotdev/trigger.dev
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Fix the search for React.ComponentType
echo "=== Searching for React.ComponentType in apps/webapp ==="
rg "React\.ComponentType" apps/webapp/app --type ts -A 1 -B 1 | head -100
echo ""
echo "=== Checking how other files handle ComponentType imports ==="
rg "import.*ComponentType" apps/webapp/app --type ts | head -20Repository: triggerdotdev/trigger.dev
Length of output: 1574
🏁 Script executed:
#!/bin/bash
# Check the first few lines of these files to see if React is imported
echo "=== Checking EnabledStatus.tsx imports ==="
head -15 apps/webapp/app/components/runs/v3/EnabledStatus.tsx
echo ""
echo "=== Checking InfoPanel.tsx imports ==="
head -15 apps/webapp/app/components/primitives/InfoPanel.tsx
echo ""
echo "=== Checking Avatar.tsx imports ==="
head -15 apps/webapp/app/components/primitives/Avatar.tsx
echo ""
echo "=== Check tsconfig types setting to see if React is globally available ==="
rg "\"types\"" apps/webapp/tsconfig.jsonRepository: triggerdotdev/trigger.dev
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Check remix.env.d.ts which is included in tsconfig
echo "=== Checking remix.env.d.ts ==="
cat apps/webapp/remix.env.d.ts
echo ""
echo "=== Search for any auto-import or ambient module declarations ==="
rg "declare.*React" apps/webapp --type ts -B 2 -A 2
echo ""
echo "=== Check if React is included via node types ==="
rg "React" apps/webapp/node_modules/@types/node -l 2>/dev/null | head -5Repository: triggerdotdev/trigger.dev
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Check if `@remix-run` packages declare React globally
echo "=== Checking `@remix-run/dev` for React declarations ==="
find . -path "*/node_modules/@remix-run/*/index.d.ts" -o -path "*/node_modules/@remix-run/*/global.d.ts" | head -5
echo ""
echo "=== Look for global React type declaration in `@remix-run` ==="
rg "declare.*React" . --type ts --type tsx 2>/dev/null | grep -i remix | head -10
echo ""
echo "=== Check the actual type definitions that `@remix-run` provides ==="
find . -path "*/node_modules/@remix-run/react/dist/*" -name "*.d.ts" | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'
echo ""
echo "=== Search for React in `@remix-run` globals ==="
find . -path "*/@remix-run/node/globals*" -name "*.d.ts" | head -1 | xargs cat 2>/dev/null | head -50Repository: triggerdotdev/trigger.dev
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Check package.json for react and typescript versions
echo "=== Checking package.json ==="
grep -A 5 -B 5 '"react"' apps/webapp/package.json | head -30
echo ""
echo "=== Check TypeScript version ==="
grep '"typescript"' apps/webapp/package.json
echo ""
echo "=== Check if there's a .tsx or .ts strict check in the project ==="
rg "noImplicitAny|noImplicitThis" apps/webapp/tsconfig.json
echo ""
echo "=== Let's examine if InfoPanel actually imports React properly despite the grep result ==="
cat -n apps/webapp/app/components/primitives/InfoPanel.tsx | head -30Repository: triggerdotdev/trigger.dev
Length of output: 1574
🏁 Script executed:
#!/bin/bash
# Final verification: Check if TypeScript would error on this
# Let's see if there's a build script and if it runs tsc
echo "=== Checking if build includes type checking ==="
cat apps/webapp/package.json | grep -A 20 '"scripts"' | head -30Repository: triggerdotdev/trigger.dev
Length of output: 1264
🏁 Script executed:
#!/bin/bash
# Check the type checking tsconfig
echo "=== Checking tsconfig.check.json ==="
cat apps/webapp/tsconfig.check.json
echo ""
echo "=== Verify ChartBlankState is included in the type check scope ==="
rg "ChartBlankState|primitives/charts" apps/webapp/tsconfig.check.jsonRepository: triggerdotdev/trigger.dev
Length of output: 491
Add missing React import — React.ComponentType will fail during type checking.
React is referenced on line 9 as React.ComponentType<{ className?: string }> but is never imported. With "jsx": "react-jsx" you don't need React for JSX, but you do need it (or a named import) for type references. TypeScript strict mode will flag React as undefined.
Fix
+import type React from "react";
import { cn } from "~/utils/cn";
import { Paragraph } from "../Paragraph";Or:
+import type { ComponentType } from "react";
import { cn } from "~/utils/cn";
import { Paragraph } from "../Paragraph";Then use ComponentType<{ className?: string }> instead of React.ComponentType<{ className?: string }>.
📝 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.
| import { cn } from "~/utils/cn"; | |
| import { Paragraph } from "../Paragraph"; | |
| export function ChartBlankState({ | |
| icon: Icon, | |
| message, | |
| className, | |
| }: { | |
| icon?: React.ComponentType<{ className?: string }>; | |
| message: string; | |
| className?: string; | |
| }) { | |
| import type React from "react"; | |
| import { cn } from "~/utils/cn"; | |
| import { Paragraph } from "../Paragraph"; | |
| export function ChartBlankState({ | |
| icon: Icon, | |
| message, | |
| className, | |
| }: { | |
| icon?: React.ComponentType<{ className?: string }>; | |
| message: string; | |
| className?: string; | |
| }) { |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/charts/ChartBlankState.tsx` around
lines 1 - 12, The ChartBlankState props reference React.ComponentType but React
is not imported; add a React type import and/or switch to a named type import to
fix type checking: either add "import React from 'react';" at the top so
React.ComponentType<{ className?: string }> resolves, or replace
React.ComponentType with a named import like "import type { ComponentType } from
'react';" and update the prop type to ComponentType<{ className?: string }>;
update the ChartBlankState prop signature accordingly.
| return ( | ||
| <TabsPrimitive.Trigger | ||
| ref={ref} | ||
| tabIndex={0} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the ClientTabs.tsx file to understand the implementation
cat -n apps/webapp/app/components/primitives/ClientTabs.tsx | head -200Repository: triggerdotdev/trigger.dev
Length of output: 7798
🏁 Script executed:
# Let's also search for any documentation or comments about tabIndex in the file
rg -n "tabIndex" apps/webapp/app/components/primitives/ClientTabs.tsx -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 1107
🏁 Script executed:
# Check if there are other Tabs implementations or similar patterns in the codebase
fd -e tsx -e ts "Tabs" apps/webapp/app/components/ | head -20Repository: triggerdotdev/trigger.dev
Length of output: 168
🏁 Script executed:
# Check the server-side Tabs.tsx component
cat -n apps/webapp/app/components/primitives/Tabs.tsx | head -100Repository: triggerdotdev/trigger.dev
Length of output: 3490
🏁 Script executed:
# Search for any TODO, FIXME, or comments about tabIndex in the webapp
rg -i "todo|fixme|note.*tab" apps/webapp/app/components/primitives/ -B 1 -A 1Repository: triggerdotdev/trigger.dev
Length of output: 50383
🏁 Script executed:
# Search git history for when tabIndex was added to ClientTabs (if available)
git log --oneline --all -- apps/webapp/app/components/primitives/ClientTabs.tsx | head -20Repository: triggerdotdev/trigger.dev
Length of output: 360
🌐 Web query:
Radix UI Tabs roving tabindex pattern accessibility
💡 Result:
Radix UI Tabs implements the roving tabindex (roving focus) pattern for the tab triggers (the “tab” elements): only one tab is in the page Tab order at a time (tabindex="0"), and the rest are removed from the Tab order (tabindex="-1"). Arrow keys move focus between tabs within the tablist, keeping the tablist as a single Tab stop in the page’s overall tab sequence. (developer.mozilla.org)
What “roving tabindex” means for accessibility (Tabs)
- Tablist/Tab/Tabpanel semantics: tabs should use
role="tablist"containingrole="tab"elements; the active tab usesaria-selected="true"and references its panel viaaria-controls; panels userole="tabpanel"and are referenced back viaaria-labelledby. (developer.mozilla.org) - Keyboard model (APG):
Radix-specific notes
- Radix Tabs is built on Radix’s internal
@radix-ui/react-roving-focusutility (i.e., it’s explicitly using a roving-focus group + items under the hood). (deepwiki.com) - It supports automatic vs manual activation (whether focusing a tab also selects it, vs requiring
Enter/Space). In Radix’s type definitions this is exposed asactivationMode?: 'automatic' | 'manual'. (app.unpkg.com) - There’s a long-running reported issue where screen reader “virtual cursor” focus (VoiceOver) can cause tab selection changes (i.e., selection following SR focus), which matters if you’re trying to strictly separate “focus” from “selection” for SR browsing. (github.com)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/tab_role?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/tab_role?utm_source=openai
- 3: https://www.w3.org/TR/2019/WD-wai-aria-practices-1.2-20191218/?utm_source=openai
- 4: https://www.w3.org/TR/2019/WD-wai-aria-practices-1.2-20191218/?utm_source=openai
- 5: https://deepwiki.com/radix-ui/primitives/4.3-focus-management?utm_source=openai
- 6: https://app.unpkg.com/%40radix-ui/react-tabs%400.0.16/files/dist/index.d.ts?utm_source=openai
- 7: [Tabs] Screen reader focus shouldn't select the tab radix-ui/primitives#1047
🌐 Web query:
WAI-ARIA Tabs pattern keyboard navigation tabIndex
💡 Result:
WAI-ARIA Tabs pattern — keyboard navigation + tabindex
Keyboard interaction (from APG Tabs pattern)
When focus is in the tablist:
- Tab:
- When entering the tablist, focus lands on the active tab.
- When leaving, focus moves to the next element in page tab order (typically the tabpanel, or the first meaningful focusable element inside it). [1]
- Arrow keys (move focus among tabs):
- Horizontal tablist: Left Arrow = previous tab (wraps), Right Arrow = next tab (wraps). [1]
- Vertical tablist (
aria-orientation="vertical"): Up Arrow acts like Left; Down Arrow acts like Right. [1]
- Enter / Space: activates the focused tab when using manual activation (i.e., selection does not follow focus). [1]
- Home / End (optional): move focus to first / last tab (and may optionally activate). [1]
APG recommends automatic activation (activate on focus) when switching panels has no noticeable latency; otherwise consider manual activation. [1]
tabindex (roving tabindex approach used for tabs)
Use a roving tabindex so only one tab is in the page Tab sequence:
- Set
tabindex="0"on the active tab (the one that should be reachable via Tab). - Set
tabindex="-1"on all other tabs (still focusable via arrow-key handling +element.focus(), but not tabbable). [2] - On arrow-key navigation: move
tabindex="0"to the newly focused tab, set the old one to-1, then call.focus()on the new one. [2]
Avoid positive tabindex values (1, 2, 3, …) for ordering. (This is general APG guidance for keyboard interfaces.) [2]
Sources: WAI-ARIA Authoring Practices Guide (APG) Tabs pattern + roving tabindex guidance. [1] [2]
Remove explicit tabIndex={0} from all trigger variants to restore Radix's roving tabindex pattern.
Radix Tabs implements the WAI-ARIA roving tabindex pattern: only the active trigger should be tabbable (tabIndex={0}) while inactive ones should have tabIndex={-1}. By forcing tabIndex={0} on every trigger, all become individually focusable via Tab, breaking the intended keyboard model where Tab enters the tablist once (to the active trigger), and arrow keys navigate between triggers.
The isActive state is already tracked but unused for tabIndex control. Either remove the explicit prop to let Radix manage it, or conditionally set tabIndex={isActive ? 0 : -1} to maintain manual activation semantics.
Applies to: lines 100, 139, 176
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/ClientTabs.tsx` at line 100, The Tab
triggers in ClientTabs are forcing tabIndex={0} on every trigger and breaking
Radix's roving tabindex behavior; remove the explicit tabIndex prop (or change
it to tabIndex={isActive ? 0 : -1}) on the trigger element(s) so Radix can
manage focus, updating the trigger variants at the locations using tabIndex
(references: ClientTabs component and the trigger element/variant where isActive
is available) to restore proper Tab/arrow keyboard navigation.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| <ClientTabsContext.Provider value={contextValue}> | ||
| <TabsPrimitive.Root | ||
| ref={ref} | ||
| activationMode="manual" |
There was a problem hiding this comment.
🚩 activationMode="manual" changes keyboard tab-switching behavior
Adding activationMode="manual" to the Radix TabsPrimitive.Root changes the keyboard interaction model: previously (default "automatic" mode), arrow-key focus on a tab trigger would immediately activate that tab and show its content. Now, users must press Enter or Space after focusing a tab to activate it.
This is used in the query editor's results tabs (Table/Graph/Big number) and potentially other ClientTabs consumers throughout the app. The change is likely intentional (prevents accidental tab switches when navigating with keyboard, especially important when tab content is expensive to render like charts), but it's a global behavioral change affecting all ClientTabs usages, not just the metrics UI.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
activationMode="manual" is the WAI-ARIA recommended pattern for tabs with non-trivial panel content — it matches what users expect from tabbed UIs with heavy content.
| @@ -977,97 +979,7 @@ export const TSQLResultsTable = memo(function TSQLResultsTable({ | |||
|
|
|||
| // Empty state | |||
| if (rows.length === 0) { | |||
There was a problem hiding this comment.
🚩 TSQLResultsTable empty state loses table headers
The old empty state for rows.length === 0 rendered full table headers (with sorting, filtering, and column resize capabilities) plus a "No results" row. The new code replaces this entire block with <ChartBlankState icon={IconTable} message="No data to display" />.
This means when a query returns zero rows, users no longer see column headers showing what columns were returned. The old behavior was useful for confirming the query structure was correct even when results were empty. This is likely an intentional simplification, but it's worth noting the information loss — especially for the query editor where understanding the result schema matters.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/metrics/QueryWidget.tsx`:
- Around line 186-194: Multiple QueryWidget instances each call
useShortcutKeys({ key: "v" }) causing N handlers and a race where pressing "v"
toggles both the currently-fullscreen widget and the hovered widget; fix by
ensuring only one global handler or preventing per-widget registration when any
widget is fullscreen. Modify QueryWidget so its useShortcutKeys registration is
conditional on a shared fullscreen flag (e.g., a context or parent prop like
globalIsFullscreen) and only registers when !globalIsFullscreen, or lift the "v"
shortcut out of QueryWidget into a parent/dashboard component that registers a
single useShortcutKeys handler which finds the hovered widget (using
containerRef refs map or document.querySelector(':hover') and a widget
identifier) and calls that widget's setIsFullscreen; reference symbols:
QueryWidget, useShortcutKeys, containerRef, isFullscreen, setIsFullscreen.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/query/QueryEditor.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
🧠 Learnings (1)
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/query/QueryEditor.tsxapps/webapp/app/components/metrics/QueryWidget.tsx
🧬 Code graph analysis (3)
apps/webapp/app/components/code/TSQLResultsTable.tsx (4)
internal-packages/clickhouse/src/index.ts (1)
OutputColumnMetadata(59-59)internal-packages/tsql/src/query/schema.ts (1)
OutputColumnMetadata(278-293)apps/webapp/app/components/primitives/charts/ChartBlankState.tsx (1)
ChartBlankState(4-23)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)
apps/webapp/app/components/query/QueryEditor.tsx (2)
apps/webapp/app/components/metrics/QueryWidget.tsx (1)
QueryWidget(161-398)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)
apps/webapp/app/components/metrics/QueryWidget.tsx (4)
apps/webapp/app/hooks/useShortcutKeys.tsx (1)
useShortcutKeys(28-59)apps/webapp/app/components/primitives/charts/Card.tsx (1)
Card(5-16)apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
ShortcutKey(41-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Bun Runtime
🔇 Additional comments (7)
apps/webapp/app/components/query/QueryEditor.tsx (2)
774-788: LGTM — styling and prop additions look consistent.The background class migration to
bg-background-bright,border-0onQueryWidget, and threading ofshowTableHeaderOnEmptyare all consistent with the broader UI improvements in this PR.
1372-1372: LGTM — label color updates totext-text-brightare consistent.All five config panel labels (Column, Sort order, Aggregation, Prefix, Suffix) are uniformly updated.
Also applies to: 1394-1394, 1422-1422, 1456-1456, 1467-1467
apps/webapp/app/components/code/TSQLResultsTable.tsx (3)
904-917: LGTM —showHeaderOnEmptyprop cleanly controls the two empty-state paths.The prop is well-documented with a JSDoc comment and the default (
false) preserves backward compatibility for existing callers that don't pass it.
985-1041: LGTM — empty-state branching is clear and consistent with other chart components.The
ChartBlankStatewithIconTablefor the default path and the header +AlertCircle"no results" message forshowHeaderOnEmptyare both well-implemented.
1143-1146: Good fix — explicitly setting row height from virtualizer.Setting
height: virtualRow.sizeensures the rendered row matches the virtualizer's size expectation, preventing layout drift when estimated and actual sizes diverge.apps/webapp/app/components/metrics/QueryWidget.tsx (2)
146-158: LGTM —classNameandshowTableHeaderOnEmptyprops are cleanly threaded.
classNamemerges into the Card viacn(...), andshowTableHeaderOnEmptyflows throughQueryWidgetBodyto both the inline and fullscreenTSQLResultsTableinstances.Also applies to: 214-216
232-237: LGTM — tooltip now shows the keyboard shortcut indicator.Good UX improvement showing the
"v"shortcut alongside the "Maximize" label.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // "v" to toggle fullscreen on hovered widget | ||
| useShortcutKeys({ | ||
| shortcut: { key: "v" }, | ||
| action: useCallback(() => { | ||
| const isHovered = containerRef.current?.matches(":hover"); | ||
| if (!isFullscreen && !isHovered) return; | ||
| setIsFullscreen((prev) => !prev); | ||
| }, [isFullscreen]), | ||
| }); |
There was a problem hiding this comment.
Multiple widget instances each register the same "v" hotkey — verify behavior on dashboards.
On a dashboard with N widgets, every mounted QueryWidget registers its own "v" listener. When the user presses "v" while hovering one widget, all N handlers fire; the :hover guard ensures only the hovered one opens fullscreen. This works, but:
- Performance: N event listeners for the same key on pages with many widgets.
- Edge case: If the user is fullscreen on widget A and hovers widget B, pressing
"v"will close A (isFullscreenis true → toggle off) and open B (hover check passes → toggle on) simultaneously.
Consider lifting the shortcut to a single parent-level handler that identifies the hovered widget, or gating the shortcut registration with disabled when another widget is already fullscreen.
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/metrics/QueryWidget.tsx` around lines 186 - 194,
Multiple QueryWidget instances each call useShortcutKeys({ key: "v" }) causing N
handlers and a race where pressing "v" toggles both the currently-fullscreen
widget and the hovered widget; fix by ensuring only one global handler or
preventing per-widget registration when any widget is fullscreen. Modify
QueryWidget so its useShortcutKeys registration is conditional on a shared
fullscreen flag (e.g., a context or parent prop like globalIsFullscreen) and
only registers when !globalIsFullscreen, or lift the "v" shortcut out of
QueryWidget into a parent/dashboard component that registers a single
useShortcutKeys handler which finds the hovered widget (using containerRef refs
map or document.querySelector(':hover') and a widget identifier) and calls that
widget's setIsFullscreen; reference symbols: QueryWidget, useShortcutKeys,
containerRef, isFullscreen, setIsFullscreen.
There was a problem hiding this comment.
"Performance concern is a non-issue. N keydown listeners for the same key is trivially cheap — these are just function calls that check :hover and bail early. Even 50 widgets wouldn't be measurable."
There was a problem hiding this comment.
@samejr, you're right—the performance concern is overblown. Multiple listeners with early bail-out are indeed cheap, and the :hover guard ensures minimal wasted work.
The edge case (pressing v while hovering widget B when widget A is already fullscreen toggles both) is still technically possible, but if it hasn't caused issues in testing, it's likely fine in practice.
Thanks for the clarification!
3 participants
Uh oh!
There was an error while loading. Please reload this page.