Open
Conversation
Add LabelPill ui component to handle labels in a consistent way. This uses the Mantine Pill component under the hood, but adds classes to handle styles when a remove button is used. This is similar to how the current UI handles matchers/labels and very closely matches how they are handled visually in the Mantine version of the Prometheus ui. Signed-off-by: Joe Adams <github@joeadams.io>
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (4)
ui/mantine-ui/src/components/LabelPill.test.tsx (3)
41-46: Consider also testing emptyvaluestring.The test covers an empty
name, but it would be good to also test an emptyvaluefor completeness (e.g.,<LabelPill name="key" value="" />should renderkey="").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/LabelPill.test.tsx` around lines 41 - 46, Add a new test in LabelPill.test.tsx that mirrors the existing "renders with empty name or value strings" case but for an empty value: render the LabelPill component with name="key" and value="" (e.g., <LabelPill name="key" value="" />) and assert that the output contains the text 'key=""'; update or add the it(...) block alongside the existing test for LabelPill to cover the empty value scenario.
25-27: Consider usingfireEvent.click()for more idiomatic RTL testing.Calling
.click()directly on the DOM element works, but usingfireEvent.click()from@testing-library/reactis more idiomatic and consistent with RTL patterns.♻️ Suggested refactor
+import { render, screen, fireEvent } from '@test-utils'; -import { render, screen } from '@test-utils';// Find and click the remove button const removeButton = screen.getByRole('button', { hidden: true }); - removeButton.click(); + fireEvent.click(removeButton);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/LabelPill.test.tsx` around lines 25 - 27, Replace the direct DOM click call with RTL's fireEvent: locate the test using screen.getByRole(...) to assign removeButton and change removeButton.click() to fireEvent.click(removeButton); also ensure you import fireEvent from "@testing-library/react" at the top of LabelPill.test.tsx if it's not already imported so the test follows idiomatic React Testing Library patterns.
33-39: Consider usingscreen.getByTestId()for consistency.Using
screen.getByTestId('label-pill-component')is more idiomatic for RTL and avoids needing to destructurecontainer.♻️ Suggested refactor
// Test that the component accepts and applies additional props it('applies additional Pill props correctly', () => { - const { container } = render( + render( <LabelPill name="env" value="staging" data-testid="label-pill-component" /> ); - expect(container.querySelector('[data-testid="label-pill-component"]')).toBeInTheDocument(); + expect(screen.getByTestId('label-pill-component')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/LabelPill.test.tsx` around lines 33 - 39, The test in LabelPill.test.tsx uses container.querySelector('[data-testid="label-pill-component"]') which is less idiomatic; update the test for the LabelPill render to use screen.getByTestId('label-pill-component') instead and assert it's in the document, and ensure you import { screen } from '@testing-library/react' (or reference the existing testing-library import) so the test uses RTL's screen API with the LabelPill component and its data-testid.ui/mantine-ui/src/components/LabelPill.tsx (1)
1-1: UnusedReactimport.With React 17+ and the new JSX transform (which is enabled per the PR objectives), the explicit
Reactimport is no longer required.♻️ Suggested refactor
-import React from 'react'; import { Pill, PillProps } from '@mantine/core';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/LabelPill.tsx` at line 1, The file contains an unused top-level import "React" in LabelPill.tsx; remove the import statement (the explicit React import) since the project uses the new JSX transform and no other symbols from React are referenced, ensuring the component still exports/works (check the LabelPill component and any named exports) and run a quick lint/type check to confirm no remaining references to React remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/mantine-ui/src/components/LabelPill.tsx`:
- Around line 10-15: The LabelPill component currently spreads {...others} after
className={classes.label} so a consumer-supplied className will overwrite the
default; update LabelPill to extract className from the rest props (e.g., const
{ className, ...rest } = others) and merge it with classes.label (e.g.,
`${classes.label} ${className || ''}`) when passing className to Pill, keep
classNames={{ remove: classes.remove }} and pass the remaining props via
{...rest}; this avoids adding clsx while ensuring default label styling is
preserved and consumer classes are appended.
---
Nitpick comments:
In `@ui/mantine-ui/src/components/LabelPill.test.tsx`:
- Around line 41-46: Add a new test in LabelPill.test.tsx that mirrors the
existing "renders with empty name or value strings" case but for an empty value:
render the LabelPill component with name="key" and value="" (e.g., <LabelPill
name="key" value="" />) and assert that the output contains the text 'key=""';
update or add the it(...) block alongside the existing test for LabelPill to
cover the empty value scenario.
- Around line 25-27: Replace the direct DOM click call with RTL's fireEvent:
locate the test using screen.getByRole(...) to assign removeButton and change
removeButton.click() to fireEvent.click(removeButton); also ensure you import
fireEvent from "@testing-library/react" at the top of LabelPill.test.tsx if it's
not already imported so the test follows idiomatic React Testing Library
patterns.
- Around line 33-39: The test in LabelPill.test.tsx uses
container.querySelector('[data-testid="label-pill-component"]') which is less
idiomatic; update the test for the LabelPill render to use
screen.getByTestId('label-pill-component') instead and assert it's in the
document, and ensure you import { screen } from '@testing-library/react' (or
reference the existing testing-library import) so the test uses RTL's screen API
with the LabelPill component and its data-testid.
In `@ui/mantine-ui/src/components/LabelPill.tsx`:
- Line 1: The file contains an unused top-level import "React" in LabelPill.tsx;
remove the import statement (the explicit React import) since the project uses
the new JSX transform and no other symbols from React are referenced, ensuring
the component still exports/works (check the LabelPill component and any named
exports) and run a quick lint/type check to confirm no remaining references to
React remain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecd15621-ee11-403d-8081-762307a1eaf2
📒 Files selected for processing (3)
ui/mantine-ui/src/components/LabelPill.module.cssui/mantine-ui/src/components/LabelPill.test.tsxui/mantine-ui/src/components/LabelPill.tsx
Comment on lines
+10
to
+15
| export const LabelPill = ({ name, value, ...others }: LabelPillProps) => { | ||
| return ( | ||
| <Pill className={classes.label} classNames={{ remove: classes.remove }} size="lg" {...others}> | ||
| {name}="{value}" | ||
| </Pill> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if clsx is available as a dependency
rg '"clsx"' package.json ui/mantine-ui/package.json 2>/dev/null || echo "clsx not found in package.json files"
# Check existing usage patterns
rg -l 'clsx' ui/mantine-ui/src/ 2>/dev/null | head -5Repository: prometheus/alertmanager
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check the actual package.json to see available dependencies
cat ui/mantine-ui/package.json | jq -r '.dependencies, .devDependencies' 2>/dev/null | head -40
# Look for other components that handle className to see patterns
fd -t f -e tsx "\.tsx$" ui/mantine-ui/src/components/ | head -5 | xargs grep -l "className" 2>/dev/null
# Check the specific component file
cat ui/mantine-ui/src/components/LabelPill.tsxRepository: prometheus/alertmanager
Length of output: 1683
🏁 Script executed:
#!/bin/bash
# Find other components that have similar patterns with className
fd -t f -e tsx "\.tsx$" ui/mantine-ui/src/components/ | xargs grep -l "className=" 2>/dev/null | head -10
# Check a few other components to see their patterns
fd -t f -e tsx "\.tsx$" ui/mantine-ui/src/components/ | head -5 | while read file; do
echo "=== $file ==="
head -20 "$file"
doneRepository: prometheus/alertmanager
Length of output: 3203
🏁 Script executed:
#!/bin/bash
# Check if Mantine provides any class merging utilities
rg -i "merge" ui/mantine-ui/src/components/ -t tsx --max-count 5
# Look for how Mantine components themselves handle classNames in the codebase
rg "classNames.*\{" ui/mantine-ui/src/components/ -A 2 -B 1 | head -30
# Check if there are any imports or utilities in the codebase for class merging
rg "cx\(" ui/mantine-ui/src/ -t tsx | head -5
# Check the Mantine version and if there's documentation about classNames merging
cat ui/mantine-ui/package.json | grep -A 2 -B 2 "@mantine/core"Repository: prometheus/alertmanager
Length of output: 624
🏁 Script executed:
#!/bin/bash
# Check if Mantine's Pill component has any documentation about classNames merging
# Look at how the component is typically used in the Header.tsx
cat ui/mantine-ui/src/components/Header.tsx | grep -A 10 -B 2 "Pill\|className"
# Check if there's a simple inline approach without external dependencies
# Look for examples of className merging without external libraries
rg "className.*\$\|className.*\+" ui/mantine-ui/src/ --max-count 10
# Try checking if Mantine Pill itself accepts both className and classNames
rg "Pill" ui/mantine-ui/src/components/ -A 3 -B 1 | head -30Repository: prometheus/alertmanager
Length of output: 4452
🏁 Script executed:
#!/bin/bash
# Check how Mantine Pill component actually handles className vs classNames
# Look at the Mantine core types to understand the behavior
rg "PillProps" ui/mantine-ui/src/ -A 5
# Check if there's any pattern in the codebase for merging classes without external libs
rg "className.*\?" ui/mantine-ui/src/ | head -10
# Look for template literals used for className merging
rg "className=\`" ui/mantine-ui/src/ | head -5Repository: prometheus/alertmanager
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Let's test whether the className override actually happens with Mantine
# by checking if there are any test cases that demonstrate this behavior
cat ui/mantine-ui/src/components/LabelPill.test.tsxRepository: prometheus/alertmanager
Length of output: 1801
🏁 Script executed:
#!/bin/bash
# Check if there's any Mantine-specific guidance for handling classNames
# Look at how Mantine's classNames prop actually works - is it designed for merging?
rg "classNames" ui/mantine-ui/src/components/ -B 2 -A 2 | head -30
# Verify the actual behavior: check if spreading ...others AFTER explicit props causes override
# by looking at how React/JSX evaluates prop order
cat ui/mantine-ui/src/components/LabelPill.tsx | head -20Repository: prometheus/alertmanager
Length of output: 940
Consumer-provided className will override label styling; clsx is not a project dependency.
The code spreads {...others} after className={classes.label}, so if a consumer passes className, it overrides the label styling entirely. The suggested fix uses clsx, which is not currently available as a dependency.
To resolve this, either:
- Add
clsxto dependencies and apply the class merging approach, or - Use an alternative without external dependencies (e.g., extract
classNamefromothersand merge manually)
Clarify the intended behavior: should consumers be able to override label styling, or should custom classes be merged with the default label class?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/mantine-ui/src/components/LabelPill.tsx` around lines 10 - 15, The
LabelPill component currently spreads {...others} after
className={classes.label} so a consumer-supplied className will overwrite the
default; update LabelPill to extract className from the rest props (e.g., const
{ className, ...rest } = others) and merge it with classes.label (e.g.,
`${classes.label} ${className || ''}`) when passing className to Pill, keep
classNames={{ remove: classes.remove }} and pass the remaining props via
{...rest}; this avoids adding clsx while ensuring default label styling is
preserved and consumer classes are appended.
TheMeier
approved these changes
Mar 29, 2026
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
3 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.
Add LabelPill ui component to handle labels in a consistent way. This uses the Mantine Pill component under the hood, but adds classes to handle styles when a remove button is used. This is similar to how the current UI handles matchers/labels and very closely matches how they are handled visually in the Mantine version of the Prometheus ui.
Example render of the LabelPill:
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?
Summary by CodeRabbit
New Features
Tests