Draft
Conversation
This is a more performant version of the upstream component. It will not be necessary as of Mantine v9. Signed-off-by: Joe Adams <github@joeadams.io>
The LabelBadges component is a pill shaped box that represents a label and value formatted label="value" and has an optional add callback to render a plus icon The LabelPill component is just a wrapper around the Mantine Pill component with some styles and label formatting in the form label="value" Signed-off-by: Joe Adams <github@joeadams.io>
Add AlertGroup component to show the list of Alerts in groups provided by the API. The Alert page includes options for showing silenced or inhibited alerts as well as label filters. Adds types around the useGroups data component. Signed-off-by: Joe Adams <github@joeadams.io>
Signed-off-by: Joe Adams <github@joeadams.io>
Signed-off-by: Joe Adams <github@joeadams.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new Accordion component system with contexts and styles, Day.js-based time utilities and tests, label UI primitives (badges/pills), an alerts grouping UI wired to a refactored groups data hook, test react-query support, and two new runtime dependencies. Changes
sequenceDiagram
participant Page as AlertsPage
participant Hook as useGroups
participant UI as AlertGroupList
participant Accordion as Accordion (Context + Items)
participant Time as time.ts
Page->>Hook: call useGroups(filterParams)
Hook-->>Page: returns groups data
Page->>UI: render AlertGroupList(groups)
UI->>Accordion: render Accordion with items
Accordion->>Accordion: manage active state (context)
Accordion->>Time: format endsAt with parseISO8601/fromNow
Note over Page,Accordion: user interacts (add/remove filters, toggle switches) -> re-call useGroups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
🧹 Nitpick comments (8)
ui/mantine-ui/src/components/Accordion/README-PROMETHEUS.md (1)
5-7: Use a durable public reference for the fork rationale.Line 6 points to a Discord thread, which is often inaccessible to contributors and can rot. Please prefer a public Mantine issue/changelog/release note URL so future cleanup is verifiable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/Accordion/README-PROMETHEUS.md` around lines 5 - 7, Replace the ephemeral Discord thread reference in the sentence starting "According to Mantine author Vitaly..." with a durable public Mantine URL (issue, changelog, or release notes) that documents the feature backport to Mantine 9.0.0; update the README-PROMETHEUS.md text to quote or link that public URL (or an archived permalink) so the rationale is verifiable and won’t rot.ui/mantine-ui/src/components/AlertGroupList.tsx (2)
44-46: Avoid duplicateparseISO8601calls.
parseISO8601(alert.endsAt)is called twice—once for the tooltip label and once for the relative time. This parses the same ISO string twice unnecessarily.Proposed fix
+ <Table.Tr> + <Table.Td w="15%">Ends</Table.Td> + <Table.Td> + {(() => { + const endsAt = parseISO8601(alert.endsAt); + return ( + <Tooltip label={endsAt.format()}> + <Box>{endsAt.fromNow()}</Box> + </Tooltip> + ); + })()} + </Table.Td> + </Table.Tr> - <Table.Tr> - <Table.Td w="15%">Ends</Table.Td> - <Table.Td> - <Tooltip label={parseISO8601(alert.endsAt).format()}> - <Box>{parseISO8601(alert.endsAt).fromNow()}</Box> - </Tooltip> - </Table.Td> - </Table.Tr>Alternatively, extract it into a small inner component or compute it once per alert in the map callback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/AlertGroupList.tsx` around lines 44 - 46, Compute parseISO8601(alert.endsAt) once and reuse it instead of calling it twice; inside the map callback in AlertGroupList (where Tooltip and Box are rendered) assign the result to a local const (e.g., endsAtDate) and pass endsAtDate.format() to Tooltip and endsAtDate.fromNow() to Box (or extract into a small inner component that accepts the parsed date) so you avoid duplicate parsing and improve performance.
13-14: Consider using stable keys instead of array indices.Using array indices as keys (
key={i},key={j}) can cause React reconciliation issues if items are reordered, inserted, or removed. If the API provides unique identifiers for groups or alerts (e.g.,alert.fingerprintor similar), prefer using those.Also applies to: 29-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/AlertGroupList.tsx` around lines 13 - 14, Replace the unstable array-index keys in AlertGroupList.tsx (the groups?.map rendering Card with key={i} and the nested alerts.map with key={j}) with stable unique identifiers from the data model (e.g., use group.id, group.groupId, or another unique group property for the Card key, and use alert.fingerprint or alert.id for each alert key); update the JSX in the AlertGroupList component to reference those unique fields instead of i/j so React can reconcile items correctly when reordering or mutating the lists.ui/mantine-ui/src/components/LabelPill.tsx (1)
11-23: Consider making the remove button conditional ononRemoveprop.The remove button is always rendered (
withRemoveButtonis hardcoded totrue), butonRemoveis optional. IfonRemoveis not provided, the button appears but does nothing when clicked.Proposed fix
<Pill className={classes.label} classNames={{ remove: classes.remove }} size="lg" - withRemoveButton + withRemoveButton={!!onRemove} onRemove={onRemove} {...others} >🤖 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 11 - 23, The LabelPill component currently always shows the remove button by passing withRemoveButton unconditionally; make the remove button conditional by only setting withRemoveButton when onRemove is provided. Update the LabelPill function (component) to compute or pass withRemoveButton={Boolean(onRemove)} (or omit the prop when onRemove is undefined) and ensure onRemove is still forwarded to the underlying Pill so clicks work when present; reference LabelPill, the onRemove prop, and the Pill component/classNames props when making the change.ui/mantine-ui/src/pages/Alerts.page.tsx (1)
157-157: Consider adding loading and error state feedback.Currently, when
isLoadingis true orerrorexists, nothing is displayed. Users won't know if data is loading or if an error occurred.Suggested improvement
+import { ActionIcon, Alert, Group, Loader, Stack, Switch, TextInput } from '@mantine/core'; -import { ActionIcon, Group, Stack, Switch, TextInput } from '@mantine/core'; ... - {!isLoading && !error && groupList} + {isLoading && <Loader />} + {error && <Alert color="red" title="Error">{error.message || 'Failed to load alerts'}</Alert>} + {!isLoading && !error && groupList}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/pages/Alerts.page.tsx` at line 157, The UI currently renders nothing when isLoading is true or error is present for the Alerts page; update the render logic around the existing {!isLoading && !error && groupList} to show explicit feedback: when isLoading is true render a loading indicator (e.g., Mantine Loader or "Loading..." text) and when error exists render a user-friendly error message with optional retry action (e.g., a Retry button that calls the same fetch or refresh function). Locate the conditional around groupList in Alerts.page.tsx and replace the empty branches with appropriate loading and error UI, reusing existing state variables isLoading, error and the fetch/retry handler function.ui/mantine-ui/src/components/Accordion/AccordionPanel/AccordionPanel.tsx (2)
46-50: UnusedonTransitionEndprop declared in interface.The
onTransitionEndprop is defined in the interface but never used in the component implementation. Either remove it from the interface or wire it to theCollapsecomponent'sonTransitionEndhandler.🔧 Option 1: Remove unused prop
export interface AccordionPanelProps extends BoxProps, CompoundStylesApiProps<AccordionPanelFactory>, ElementProps<'div'> { - /** Called when the panel animation completes */ - onTransitionEnd?: () => void; }🔧 Option 2: Wire the prop to Collapse
const { classNames, className, style, styles, vars, children, ...others } = useProps( + const { classNames, className, style, styles, vars, children, onTransitionEnd, ...others } = useProps( 'AccordionPanel', null, props );Then pass it to
<Collapse onTransitionEnd={onTransitionEnd} ...>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/Accordion/AccordionPanel/AccordionPanel.tsx` around lines 46 - 50, The interface AccordionPanelProps declares an unused onTransitionEnd prop; fix by wiring it through the AccordionPanel component to the Collapse component (pass onTransitionEnd to <Collapse onTransitionEnd={onTransitionEnd} ...>) so the handler is invoked when the panel animation completes, or if you prefer to remove the unused API, delete onTransitionEnd from AccordionPanelProps and any related exports; locate AccordionPanelProps and the AccordionPanel component (and the nested Collapse usage) to apply the change.
75-85: Hardcoded 200ms timeout may desync from customtransitionDuration.The timeout for unmounting children is hardcoded to 200ms, but
ctx.transitionDurationcan be customized. If a longer transition duration is configured, children will unmount before the animation completes.Consider using the context's transition duration:
♻️ Proposed fix
useEffect(() => { let timeout: ReturnType<typeof setTimeout>; + const duration = ctx.transitionDuration ?? 200; if (isActive) { setShowChildren(true); } else { - timeout = setTimeout(() => setShowChildren(false), 200); + timeout = setTimeout(() => setShowChildren(false), duration); } return () => clearTimeout(timeout); - }, [isActive]); + }, [isActive, ctx.transitionDuration]);Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/components/Accordion/AccordionPanel/AccordionPanel.tsx` around lines 75 - 85, The unmount delay is hardcoded to 200ms and can desync from customized durations; update the unmount timeout to use the context transition duration (ctx.transitionDuration) wherever the 200ms literal is used (inside the useEffect controlling setShowChildren when isActive changes and the similar occurrence around the other block at ~93), e.g., derive the timeout value from ctx.transitionDuration (fallback to a sensible default if undefined), store it in the same timeout variable, and ensure clearTimeout(timeout) still correctly clears that timer in the cleanup.ui/mantine-ui/src/lib/time.ts (1)
20-26:parseISO8601andparseTimeare functionally identical.Both functions call
dayjs.utc(value). Consider removing one or documenting the semantic difference if intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/lib/time.ts` around lines 20 - 26, parseISO8601 and parseTime are duplicates; either remove the redundant function or implement/document a distinct semantic for parseTime. Fix by choosing one approach: 1) Remove parseTime and update all callers to use parseISO8601 (keep dayjs.utc(value) in parseISO8601), or 2) Give parseTime a clear different behavior (e.g., parse time-only strings with a specific format/assume local date or attach today's date) and update its implementation and JSDoc accordingly. Reference the functions parseISO8601 and parseTime when making the change so callers and tests can be updated consistently.
🤖 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/Accordion/Accordion.tsx`:
- Around line 89-90: The JSDoc says the Accordion prop "loop" defaults to true
but Accordion.defaultProps does not set it, so add loop: true to the Accordion
component's defaultProps (update the defaultProps object defined for Accordion)
to ensure runtime behavior matches the documentation; also scan the nearby
documented props block (the JSDoc block around lines 125-132) and add any other
props that declare `@default` to Accordion.defaultProps so documented defaults and
runtime defaults stay in sync.
In `@ui/mantine-ui/src/data/groups.ts`:
- Around line 38-40: The current construction of filterEntries uses raw
interpolation of params.filter values which can break matcher strings if values
contain quotes or backslashes; update the map for filterEntries to escape
problematic characters (at minimum backslashes and both single/double quotes)
before embedding value into the `${key}="${value}"` string so the resulting
matcher is syntactically valid. Locate the filterEntries creation (uses
params?.filter and Object.entries(params.filter)) and replace the direct
interpolation with a sanitizedValue computed from value by escaping backslashes
and quotes (or use a safe serializer) and then build the matcher as
`${key}="${sanitizedValue}"`. Ensure this change handles undefined/null values
consistently and preserves existing behavior when values contain no special
characters.
In `@ui/mantine-ui/src/lib/time.ts`:
- Around line 33-39: The duration regex (durationRE) fails for millisecond-only
strings like "100ms" because the minute group (?<min>[0-9]+)m appears before
(?<ms>[0-9]+)ms and greedily consumes the "m"; update the durationRE declaration
in the time parsing code to either place the ms pattern before the min pattern
(move ((?<ms>[0-9]+)ms)? to appear before ((?<min>[0-9]+)m)?), or change the
minutes group to use a negative lookahead like (?<min>[0-9]+)m(?!s) so "ms" is
correctly parsed, ensuring the rest of the matching logic using matches and
matches.groups remains unchanged.
In `@ui/mantine-ui/src/pages/Alerts.page.tsx`:
- Around line 28-33: The memoized groupList currently has a stale closure and
inconsistent return types: wrap handleSubAdd in useCallback (e.g., const
handleSubAdd = useCallback(..., [...relevant deps...])) so it is stable, then
update the useMemo to consistently return a JSX element or null (not an array)
and include handleSubAdd and data in the dependency array (e.g., useMemo(() =>
data ? <AlertGroupList groups={data} addCallback={handleSubAdd} /> : null,
[data, handleSubAdd])). Ensure the referenced symbols are handleSubAdd,
useMemo/groupList, and AlertGroupList.
In `@ui/mantine-ui/test-utils/render.tsx`:
- Around line 6-12: Move the module-level QueryClient creation into a factory
function (e.g., createTestQueryClient) so each test gets a fresh instance;
replace the existing const queryClient = new QueryClient(...) with a function
that returns new QueryClient({ defaultOptions: { queries: { retry: false } } })
and update any callers in this file (such as the render wrapper or
renderWithProviders) to call createTestQueryClient() per-render instead of using
the shared queryClient variable to avoid shared cache/observers between tests.
---
Nitpick comments:
In `@ui/mantine-ui/src/components/Accordion/AccordionPanel/AccordionPanel.tsx`:
- Around line 46-50: The interface AccordionPanelProps declares an unused
onTransitionEnd prop; fix by wiring it through the AccordionPanel component to
the Collapse component (pass onTransitionEnd to <Collapse
onTransitionEnd={onTransitionEnd} ...>) so the handler is invoked when the panel
animation completes, or if you prefer to remove the unused API, delete
onTransitionEnd from AccordionPanelProps and any related exports; locate
AccordionPanelProps and the AccordionPanel component (and the nested Collapse
usage) to apply the change.
- Around line 75-85: The unmount delay is hardcoded to 200ms and can desync from
customized durations; update the unmount timeout to use the context transition
duration (ctx.transitionDuration) wherever the 200ms literal is used (inside the
useEffect controlling setShowChildren when isActive changes and the similar
occurrence around the other block at ~93), e.g., derive the timeout value from
ctx.transitionDuration (fallback to a sensible default if undefined), store it
in the same timeout variable, and ensure clearTimeout(timeout) still correctly
clears that timer in the cleanup.
In `@ui/mantine-ui/src/components/Accordion/README-PROMETHEUS.md`:
- Around line 5-7: Replace the ephemeral Discord thread reference in the
sentence starting "According to Mantine author Vitaly..." with a durable public
Mantine URL (issue, changelog, or release notes) that documents the feature
backport to Mantine 9.0.0; update the README-PROMETHEUS.md text to quote or link
that public URL (or an archived permalink) so the rationale is verifiable and
won’t rot.
In `@ui/mantine-ui/src/components/AlertGroupList.tsx`:
- Around line 44-46: Compute parseISO8601(alert.endsAt) once and reuse it
instead of calling it twice; inside the map callback in AlertGroupList (where
Tooltip and Box are rendered) assign the result to a local const (e.g.,
endsAtDate) and pass endsAtDate.format() to Tooltip and endsAtDate.fromNow() to
Box (or extract into a small inner component that accepts the parsed date) so
you avoid duplicate parsing and improve performance.
- Around line 13-14: Replace the unstable array-index keys in AlertGroupList.tsx
(the groups?.map rendering Card with key={i} and the nested alerts.map with
key={j}) with stable unique identifiers from the data model (e.g., use group.id,
group.groupId, or another unique group property for the Card key, and use
alert.fingerprint or alert.id for each alert key); update the JSX in the
AlertGroupList component to reference those unique fields instead of i/j so
React can reconcile items correctly when reordering or mutating the lists.
In `@ui/mantine-ui/src/components/LabelPill.tsx`:
- Around line 11-23: The LabelPill component currently always shows the remove
button by passing withRemoveButton unconditionally; make the remove button
conditional by only setting withRemoveButton when onRemove is provided. Update
the LabelPill function (component) to compute or pass
withRemoveButton={Boolean(onRemove)} (or omit the prop when onRemove is
undefined) and ensure onRemove is still forwarded to the underlying Pill so
clicks work when present; reference LabelPill, the onRemove prop, and the Pill
component/classNames props when making the change.
In `@ui/mantine-ui/src/lib/time.ts`:
- Around line 20-26: parseISO8601 and parseTime are duplicates; either remove
the redundant function or implement/document a distinct semantic for parseTime.
Fix by choosing one approach: 1) Remove parseTime and update all callers to use
parseISO8601 (keep dayjs.utc(value) in parseISO8601), or 2) Give parseTime a
clear different behavior (e.g., parse time-only strings with a specific
format/assume local date or attach today's date) and update its implementation
and JSDoc accordingly. Reference the functions parseISO8601 and parseTime when
making the change so callers and tests can be updated consistently.
In `@ui/mantine-ui/src/pages/Alerts.page.tsx`:
- Line 157: The UI currently renders nothing when isLoading is true or error is
present for the Alerts page; update the render logic around the existing
{!isLoading && !error && groupList} to show explicit feedback: when isLoading is
true render a loading indicator (e.g., Mantine Loader or "Loading..." text) and
when error exists render a user-friendly error message with optional retry
action (e.g., a Retry button that calls the same fetch or refresh function).
Locate the conditional around groupList in Alerts.page.tsx and replace the empty
branches with appropriate loading and error UI, reusing existing state variables
isLoading, error and the fetch/retry handler function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd3f7372-3c16-4288-acf9-e8f708686ca2
⛔ Files ignored due to path filters (1)
ui/mantine-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
ui/mantine-ui/package.jsonui/mantine-ui/src/components/Accordion/Accordion.context.tsui/mantine-ui/src/components/Accordion/Accordion.module.cssui/mantine-ui/src/components/Accordion/Accordion.tsxui/mantine-ui/src/components/Accordion/Accordion.types.tsui/mantine-ui/src/components/Accordion/AccordionChevron.tsxui/mantine-ui/src/components/Accordion/AccordionControl/AccordionControl.tsxui/mantine-ui/src/components/Accordion/AccordionItem.context.tsui/mantine-ui/src/components/Accordion/AccordionItem/AccordionItem.tsxui/mantine-ui/src/components/Accordion/AccordionPanel/AccordionPanel.tsxui/mantine-ui/src/components/Accordion/README-PROMETHEUS.mdui/mantine-ui/src/components/Accordion/index.tsui/mantine-ui/src/components/AlertGroupList.tsxui/mantine-ui/src/components/LabelBadges.module.cssui/mantine-ui/src/components/LabelBadges.tsxui/mantine-ui/src/components/LabelPill.module.cssui/mantine-ui/src/components/LabelPill.tsxui/mantine-ui/src/data/groups.tsui/mantine-ui/src/lib/time.test.tsui/mantine-ui/src/lib/time.tsui/mantine-ui/src/pages/Alerts.page.tsxui/mantine-ui/test-utils/render.tsx
Signed-off-by: Joe Adams <github@joeadams.io>
Contributor
Author
|
Moving to draft. Mantine v9 was released, so we can pull the Accordion out. |
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
1 participant
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.
Builds out the new alerts page in the mantine-ui.
Includes some additional components to support rendering the alerts and groups. This should make the new UI actually useful for something.
Reviewer note: I tried to break up commits for this PR. The Accordion is just a copy of the Prometheus Accordion which will be unnecessary in Mantine v9
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?
Summary by CodeRabbit
New Features
Tests
Documentation
Dependencies