Feature/alerts#257
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughIntroduces a complete alerts management feature with client pages for listing, creating, viewing, and editing alerts. Includes reusable form and detail components, API hooks for CRUD operations, type definitions, and feature flag integration to conditionally show the alerts navigation item. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Client as Client Page
participant Form as AlertForm Component
participant API as API Hooks
participant Server as Backend API
participant Toast as Toast Notification
rect rgb(100, 150, 200, 0.5)
Note over User,Server: Create Alert Flow
User->>Client: Navigate to /alerts/new
Client->>Form: Render with onSave callback
User->>Form: Fill form (query config, schedule, recipients)
User->>Form: Click Save
Form->>API: createAlert(data)
API->>Server: POST /api/alerts/
Server-->>API: Alert created response
API-->>Form: Return Alert object
Form->>Toast: Show success notification
Form->>Client: Navigate to /alerts
end
rect rgb(150, 200, 100, 0.5)
Note over User,Server: View & Edit Alert Flow
User->>Client: Navigate to /alerts/[id]
Client->>API: useAlert(alertId)
API->>Server: GET /api/alerts/[id]/
Server-->>API: Alert details response
API-->>Client: Return alert data + evaluations
Client->>Form: Render AlertDetail with edit button
User->>Form: Click Edit button
User->>Client: Navigate to /alerts/[id]/edit
Client->>Form: Render with existing alert data
User->>Form: Modify form fields
User->>Form: Click Save
Form->>API: updateAlert(id, data)
API->>Server: PUT /api/alerts/[id]/
Server-->>API: Updated alert response
API-->>Form: Return Alert object
Form->>Toast: Show success notification
Form->>Client: Navigate to /alerts/[id]
end
rect rgb(200, 150, 100, 0.5)
Note over User,Server: Delete Alert Flow
User->>Client: Click delete in AlertList
Client->>Client: Show confirmation dialog
User->>Client: Confirm deletion
Client->>API: deleteAlert(id)
API->>Server: DELETE /api/alerts/[id]/
Server-->>API: Deletion confirmed
API-->>Client: Operation complete
Client->>Toast: Show success notification
Client->>Client: Refresh alert list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
- Coverage 23.12% 22.67% -0.46%
==========================================
Files 322 332 +10
Lines 20952 21371 +419
Branches 7046 7167 +121
==========================================
Hits 4845 4845
- Misses 16096 16515 +419
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
types/alert.ts (1)
125-157: Consider substring matching order for data type detection.The current substring matching could have unintended matches. For example, a column type like
"TINYINT"would match both"int"and"smallint"checks. While this works correctly (both map toNUMERIC_FILTER_OPERATORS), the check for"time"in the date array could incorrectly match"DATETIME"before the explicit"datetime"check runs.Since the checks are in order (boolean → date → numeric → text), this is actually correct for the current cases. However, using word boundaries or exact matching would be more robust for future data type additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/alert.ts` around lines 125 - 157, The substring checks in getFilterOperatorsForDataType can misfire (e.g., "time" matching "datetime" or "int" matching "tinyint"); update the detection to use exact token matching or word-boundary checks instead of simple .includes on the lowercased string: tokenize the input type (split on non-alphanumeric/underscore or use a /\b...\\b/ regex) and check equality against the token lists (BOOLEAN tokens, DATE tokens like 'timestamp','datetime','date','time', NUMERIC tokens like 'int','smallint', etc.) so each type is matched as a whole word and future additions won't cause unintended overlaps.app/alerts/new/page.tsx (1)
12-28: Keep the route file thin and share the form payload type.
handleSaveowns the mutation, toast, and navigation flow here, and the payload shape is duplicated in the edit page. Moving that into a feature component or hook with a sharedAlertFormpayload type will keep both routes in sync and leavepage.tsxas a thin wrapper.As per coding guidelines, "Page components should be thin wrappers that delegate logic to feature-specific components - keep business logic and state management in separate component files, not in page.tsx" and "Define TypeScript interfaces in hook files or import from
types/directory, never useanytype - define proper interfaces instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/alerts/new/page.tsx` around lines 12 - 28, The page currently implements the save flow (handleSave) and duplicates the payload shape; move the mutation, toast and navigation logic into a reusable hook or feature component (e.g., useAlertForm or AlertFormContainer) and export a shared payload type (e.g., AlertFormPayload or AlertFormProps using AlertQueryConfig) from a central place (types/ or the hook file). Replace handleSave in page.tsx with a thin wrapper that simply renders <AlertForm onSave={hook.onSave} onCancel={hook.onCancel}> and import the shared payload type into both the new and edit routes so they use the same interface and the page remains a thin wrapper. Ensure createAlert, toastSuccess/toastError calls and router.push are performed inside the hook/container, and update both pages to consume the hook and shared types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/alerts/`[id]/edit/page.tsx:
- Around line 23-26: After updateAlert resolves, invalidate the SWR cache for
this alert before navigating: call useSWRConfig().mutate(alert cache key,
undefined, { revalidate: false }) (or the appropriate key used to fetch the
alert) right after updateAlert(...) and before router.push(...). Also add a key
prop to the edit form component (e.g., pass
key={`${alertId}-${data.someCriticalField || updatedAt}`} or similar using the
alertId and a critical field) so the form remounts when alert data changes;
reference updateAlert, toastSuccess.updated, router.push, and
useSWRConfig().mutate when making these edits.
- Around line 10-15: The EditAlertPage currently parses id and immediately calls
useAlert with alertId; validate the parsed id first (using parseInt and
Number.isFinite/Number.isInteger and ensuring >0) and only call useAlert when
the id is valid—if invalid, short-circuit by returning a 404/redirect or calling
next/router's replace or Next.js notFound() before invoking useAlert; move the
useAlert(id) invocation below this validation and reference EditAlertPage,
parseInt, alertId and useAlert when locating the code to change.
In `@app/alerts/`[id]/page.tsx:
- Around line 6-8: The AlertDetailPage currently passes an unvalidated parsed id
into AlertDetail; update AlertDetailPage (the function receiving { params } and
calling use(params)) to parse the id with parseInt, check that it's a finite
number (e.g., Number.isInteger or !isNaN(parseInt(...)) and > 0 as appropriate),
and short-circuit when invalid by returning/throwing the proper "not found" or
error response instead of rendering <AlertDetail alertId={...}>; ensure you
reference the same params usage and AlertDetail component so invalid non-numeric
route params never reach the data hooks.
In `@components/alerts/AlertDetail.tsx`:
- Around line 147-148: The list rendered by qc.filters.map uses a non-stable key
`key={`filter-${i}`}` which can cause incorrect reuse when filters are
inserted/removed; change the key on the <p> element to use a stable identifier
from the filter object (e.g., use `f.id` or compose a unique string like
`${f.type}-${f.field}-${f.value}`) in the qc.filters.map callback, and if
filters lack a stable id add one on the filter objects when they are created so
the key is always deterministic.
- Around line 63-70: The back icon button lacks an aria-label so screen readers
announce it generically; update the Button component instance (the one rendering
ArrowLeft and calling router.push('/alerts') with data-testid="back-to-alerts")
to include an appropriate aria-label such as "Back to alerts" (preserve all
existing props like variant, size, onClick, and data-testid) so the icon-only
button is accessible and keyboard focus/activation behavior remains unchanged.
In `@components/alerts/AlertForm.tsx`:
- Around line 179-205: The code uses parseFloat(conditionValue) when
constructing currentQueryConfig which can produce NaN for invalid/empty strings;
update the useMemo so you validate the parsed number (e.g., const parsed =
parseFloat(conditionValue)) and if Number.isNaN(parsed) either return null from
currentQueryConfig or set condition_value to null instead of NaN; modify the
construction around currentQueryConfig / condition_value / conditionValue to
ensure the API never receives NaN.
- Around line 461-468: The current map uses the array index as the React key for
FilterRow which can break state when filters are reordered/removed; modify the
code that creates new filters (e.g., the add/filter-creation logic) to assign a
stable unique id (UUID or incrementing id) onto the filter object, add an
optional id field to the AlertFilter type (types/alert.ts), and change the map
to use filter.id as the key; update updateFilter and removeFilter to
accept/find-by id (or translate id to index) so operations remain stable. If you
cannot change the type right now, as a temporary fallback use a stable composite
key like `${filter.column}-${index}` until you add the id.
In `@components/alerts/AlertList.tsx`:
- Around line 281-323: Add descriptive data-testid attributes to the interactive
footer controls and add aria-labels for the icon-only pagination buttons: add a
data-testid like "alerts-page-size-select" to the Select (or SelectTrigger)
and/or each SelectItem, add data-testid "alerts-page-size-10|20|50|100" as
appropriate, and add data-testid "alerts-prev-page-button" and
"alerts-next-page-button" to the two Button components; also add
aria-label="Previous page" to the Button that renders ChevronLeft and
aria-label="Next page" to the Button that renders ChevronRight. Keep the
existing handlers (setPageSize, setCurrentPage) and state (currentPage,
totalPages) unchanged and ensure attributes are added on the
Select/SelectTrigger and Button elements referenced above.
- Around line 181-187: The TableRow in AlertList.tsx is clickable via onClick
but not keyboard-accessible; update the alerts.map rendering so each TableRow
(or a semantic wrapper inside it) is keyboard-focusable and operable: add
tabIndex={0}, role="button" (or preferably wrap the row content in a semantic
<a> or Link element) and handle onKeyDown to trigger the same navigation as
onClick for Enter and Space keys, and provide an accessible label (aria-label)
if the row contains icon-only controls; ensure you keep the existing onClick
that calls router.push(`/alerts/${alert.id}`) and mirror that call in the
keyboard handler.
- Around line 55-76: When deleting an alert in handleDelete, clamp currentPage
against the updated totalPages so it cannot remain past the last page: compute
newTotal = total - 1 and newTotalPages = Math.max(1, Math.ceil(newTotal /
pageSize)), and if currentPage > newTotalPages call the page setter (e.g.,
setCurrentPage(newTotalPages)) before/after calling mutate to ensure the next
fetch isn't for an empty page; update handleDelete to reference total, pageSize,
currentPage and the page setter so the UI stays on a valid page after deletions.
In `@components/alerts/FilterRow.tsx`:
- Around line 90-98: The icon-only remove Button (component Button rendering <X
/>) lacks an accessible label; update the Button where onClick is onRemove and
data-testid="filter-remove-btn" to include an aria-label (e.g.,
aria-label="Remove filter" or a localized string) so screen readers can announce
its purpose, ensuring the change is applied to the Button element that contains
the <X className="h-4 w-4" /> icon.
- Around line 69-70: customDateValue is created by parsing filter.value and can
become an Invalid Date; wrap the computation in a useMemo and defensively check
the parsed result with isValid (or Number.isNaN / isNaN(parsed.getTime()))
before returning it so you pass either a real Date or undefined to the
DatePicker. Specifically, update the customDateValue computation (the expression
using showDatePicker, filter.value and parse) to run inside useMemo, call
parse(filter.value, 'yyyy-MM-dd', new Date()), validate the parsed Date, and
return the Date only if valid—otherwise return undefined; import useMemo from
React and keep referencing customDateValue, showDatePicker, filter.value and
parse to locate the change.
---
Nitpick comments:
In `@app/alerts/new/page.tsx`:
- Around line 12-28: The page currently implements the save flow (handleSave)
and duplicates the payload shape; move the mutation, toast and navigation logic
into a reusable hook or feature component (e.g., useAlertForm or
AlertFormContainer) and export a shared payload type (e.g., AlertFormPayload or
AlertFormProps using AlertQueryConfig) from a central place (types/ or the hook
file). Replace handleSave in page.tsx with a thin wrapper that simply renders
<AlertForm onSave={hook.onSave} onCancel={hook.onCancel}> and import the shared
payload type into both the new and edit routes so they use the same interface
and the page remains a thin wrapper. Ensure createAlert, toastSuccess/toastError
calls and router.push are performed inside the hook/container, and update both
pages to consume the hook and shared types.
In `@types/alert.ts`:
- Around line 125-157: The substring checks in getFilterOperatorsForDataType can
misfire (e.g., "time" matching "datetime" or "int" matching "tinyint"); update
the detection to use exact token matching or word-boundary checks instead of
simple .includes on the lowercased string: tokenize the input type (split on
non-alphanumeric/underscore or use a /\b...\\b/ regex) and check equality
against the token lists (BOOLEAN tokens, DATE tokens like
'timestamp','datetime','date','time', NUMERIC tokens like 'int','smallint',
etc.) so each type is matched as a whole word and future additions won't cause
unintended overlaps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72b88f79-a0dc-4405-8d15-3c829d07f929
📒 Files selected for processing (13)
app/alerts/[id]/edit/page.tsxapp/alerts/[id]/page.tsxapp/alerts/new/page.tsxapp/alerts/page.tsxcomponents/alerts/AlertDetail.tsxcomponents/alerts/AlertForm.tsxcomponents/alerts/AlertList.tsxcomponents/alerts/AlertTestPreview.tsxcomponents/alerts/FilterRow.tsxcomponents/main-layout.tsxhooks/api/useAlerts.tshooks/api/useFeatureFlags.tstypes/alert.ts
| export default function EditAlertPage({ params }: { params: Promise<{ id: string }> }) { | ||
| const { id } = use(params); | ||
| const alertId = parseInt(id, 10); | ||
| const router = useRouter(); | ||
| const { alert, isLoading } = useAlert(alertId); | ||
|
|
There was a problem hiding this comment.
Reject invalid alert IDs before calling useAlert.
This route parses id and immediately fetches with it. Invalid or malformed IDs should be filtered out first, otherwise the edit screen can issue requests with a bad identifier and get stuck in the loading path.
🔎 Suggested fix
export default function EditAlertPage({ params }: { params: Promise<{ id: string }> }) {
const { id } = use(params);
- const alertId = parseInt(id, 10);
+ const alertId = Number(id);
+
+ if (!Number.isInteger(alertId) || alertId <= 0) {
+ return <div className="p-6 text-sm text-muted-foreground">Invalid alert ID.</div>;
+ }
+
const router = useRouter();
const { alert, isLoading } = useAlert(alertId);📝 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.
| export default function EditAlertPage({ params }: { params: Promise<{ id: string }> }) { | |
| const { id } = use(params); | |
| const alertId = parseInt(id, 10); | |
| const router = useRouter(); | |
| const { alert, isLoading } = useAlert(alertId); | |
| export default function EditAlertPage({ params }: { params: Promise<{ id: string }> }) { | |
| const { id } = use(params); | |
| const alertId = Number(id); | |
| if (!Number.isInteger(alertId) || alertId <= 0) { | |
| return <div className="p-6 text-sm text-muted-foreground">Invalid alert ID.</div>; | |
| } | |
| const router = useRouter(); | |
| const { alert, isLoading } = useAlert(alertId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/alerts/`[id]/edit/page.tsx around lines 10 - 15, The EditAlertPage
currently parses id and immediately calls useAlert with alertId; validate the
parsed id first (using parseInt and Number.isFinite/Number.isInteger and
ensuring >0) and only call useAlert when the id is valid—if invalid,
short-circuit by returning a 404/redirect or calling next/router's replace or
Next.js notFound() before invoking useAlert; move the useAlert(id) invocation
below this validation and reference EditAlertPage, parseInt, alertId and
useAlert when locating the code to change.
| try { | ||
| await updateAlert(alertId, data); | ||
| toastSuccess.updated('Alert'); | ||
| router.push(`/alerts/${alertId}`); |
There was a problem hiding this comment.
Invalidate the cached alert data before navigating back.
After updateAlert, this page still holds the pre-edit alert in cache. Routing straight to /alerts/${alertId} can briefly render stale details unless the alert cache is cleared or revalidated first.
As per coding guidelines, "For edit forms with SWR stale cache issues on navigation: (1) invalidate cache with useSWRConfig().mutate(key, undefined, { revalidate: false }) after mutations, (2) add key prop to form components including critical fields to force remount when data changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/alerts/`[id]/edit/page.tsx around lines 23 - 26, After updateAlert
resolves, invalidate the SWR cache for this alert before navigating: call
useSWRConfig().mutate(alert cache key, undefined, { revalidate: false }) (or the
appropriate key used to fetch the alert) right after updateAlert(...) and before
router.push(...). Also add a key prop to the edit form component (e.g., pass
key={`${alertId}-${data.someCriticalField || updatedAt}`} or similar using the
alertId and a critical field) so the form remounts when alert data changes;
reference updateAlert, toastSuccess.updated, router.push, and
useSWRConfig().mutate when making these edits.
| export default function AlertDetailPage({ params }: { params: Promise<{ id: string }> }) { | ||
| const { id } = use(params); | ||
| return <AlertDetail alertId={parseInt(id, 10)} />; |
There was a problem hiding this comment.
Validate the route param before using it as an alert ID.
A non-numeric /alerts/[id] URL will flow straight into AlertDetail, which can send an invalid alertId into the data hooks. Guard the parsed value first and short-circuit invalid routes.
🔎 Suggested fix
export default function AlertDetailPage({ params }: { params: Promise<{ id: string }> }) {
const { id } = use(params);
- return <AlertDetail alertId={parseInt(id, 10)} />;
+ const alertId = Number(id);
+
+ if (!Number.isInteger(alertId) || alertId <= 0) {
+ return <div className="p-6 text-sm text-muted-foreground">Invalid alert ID.</div>;
+ }
+
+ return <AlertDetail alertId={alertId} />;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/alerts/`[id]/page.tsx around lines 6 - 8, The AlertDetailPage currently
passes an unvalidated parsed id into AlertDetail; update AlertDetailPage (the
function receiving { params } and calling use(params)) to parse the id with
parseInt, check that it's a finite number (e.g., Number.isInteger or
!isNaN(parseInt(...)) and > 0 as appropriate), and short-circuit when invalid by
returning/throwing the proper "not found" or error response instead of rendering
<AlertDetail alertId={...}>; ensure you reference the same params usage and
AlertDetail component so invalid non-numeric route params never reach the data
hooks.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => router.push('/alerts')} | ||
| data-testid="back-to-alerts" | ||
| > | ||
| <ArrowLeft className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Label the back icon button.
Without an aria-label, assistive tech will announce a generic button here.
As per coding guidelines, "Preserve Radix UI's built-in accessibility props, add aria-label for icon-only buttons, ensure keyboard navigation (Tab, Enter, Escape), use semantic HTML elements, ensure sufficient color contrast".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/AlertDetail.tsx` around lines 63 - 70, The back icon button
lacks an aria-label so screen readers announce it generically; update the Button
component instance (the one rendering ArrowLeft and calling
router.push('/alerts') with data-testid="back-to-alerts") to include an
appropriate aria-label such as "Back to alerts" (preserve all existing props
like variant, size, onClick, and data-testid) so the icon-only button is
accessible and keyboard focus/activation behavior remains unchanged.
| {qc.filters.map((f, i) => ( | ||
| <p key={`filter-${i}`} className="text-sm font-mono"> |
There was a problem hiding this comment.
Use a stable key for filters.
Keying by array index will reuse the wrong row when filters are inserted, removed, or reordered. Compose the key from stable filter fields instead.
As per coding guidelines, "Always add key prop to all items in lists/arrays using unique identifiers, never array indices".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/AlertDetail.tsx` around lines 147 - 148, The list rendered
by qc.filters.map uses a non-stable key `key={`filter-${i}`}` which can cause
incorrect reuse when filters are inserted/removed; change the key on the <p>
element to use a stable identifier from the filter object (e.g., use `f.id` or
compose a unique string like `${f.type}-${f.field}-${f.value}`) in the
qc.filters.map callback, and if filters lack a stable id add one on the filter
objects when they are created so the key is always deterministic.
| const totalPages = useMemo(() => Math.max(1, Math.ceil(total / pageSize)), [total, pageSize]); | ||
| const startIndex = (currentPage - 1) * pageSize; | ||
|
|
||
| const handleDelete = useCallback( | ||
| async (alert: Alert) => { | ||
| const confirmed = await confirm({ | ||
| title: 'Delete alert?', | ||
| description: `This will permanently delete "${alert.name}" and all its evaluation history. This action cannot be undone.`, | ||
| confirmText: 'Delete', | ||
| type: 'warning', | ||
| }); | ||
| if (!confirmed) return; | ||
| try { | ||
| await deleteAlert(alert.id); | ||
| toastSuccess.deleted('Alert'); | ||
| await mutate(); | ||
| } catch (error: unknown) { | ||
| toastError.delete(error, 'Alert'); | ||
| } | ||
| }, | ||
| [mutate, confirm] | ||
| ); |
There was a problem hiding this comment.
Clamp currentPage when the result set shrinks.
If the user deletes the last row on the last page, totalPages drops but currentPage stays stale. The next fetch can come back empty and trigger the global empty state even though earlier pages still contain alerts.
📌 Suggested fix
-import { useState, useCallback, useMemo } from 'react';
+import { useState, useCallback, useEffect, useMemo } from 'react';
…
const totalPages = useMemo(() => Math.max(1, Math.ceil(total / pageSize)), [total, pageSize]);
const startIndex = (currentPage - 1) * pageSize;
+
+ useEffect(() => {
+ if (currentPage > totalPages) {
+ setCurrentPage(totalPages);
+ }
+ }, [currentPage, totalPages]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalPages = useMemo(() => Math.max(1, Math.ceil(total / pageSize)), [total, pageSize]); | |
| const startIndex = (currentPage - 1) * pageSize; | |
| const handleDelete = useCallback( | |
| async (alert: Alert) => { | |
| const confirmed = await confirm({ | |
| title: 'Delete alert?', | |
| description: `This will permanently delete "${alert.name}" and all its evaluation history. This action cannot be undone.`, | |
| confirmText: 'Delete', | |
| type: 'warning', | |
| }); | |
| if (!confirmed) return; | |
| try { | |
| await deleteAlert(alert.id); | |
| toastSuccess.deleted('Alert'); | |
| await mutate(); | |
| } catch (error: unknown) { | |
| toastError.delete(error, 'Alert'); | |
| } | |
| }, | |
| [mutate, confirm] | |
| ); | |
| import { useState, useCallback, useEffect, useMemo } from 'react'; | |
| // ... other imports | |
| const totalPages = useMemo(() => Math.max(1, Math.ceil(total / pageSize)), [total, pageSize]); | |
| const startIndex = (currentPage - 1) * pageSize; | |
| useEffect(() => { | |
| if (currentPage > totalPages) { | |
| setCurrentPage(totalPages); | |
| } | |
| }, [currentPage, totalPages]); | |
| const handleDelete = useCallback( | |
| async (alert: Alert) => { | |
| const confirmed = await confirm({ | |
| title: 'Delete alert?', | |
| description: `This will permanently delete "${alert.name}" and all its evaluation history. This action cannot be undone.`, | |
| confirmText: 'Delete', | |
| type: 'warning', | |
| }); | |
| if (!confirmed) return; | |
| try { | |
| await deleteAlert(alert.id); | |
| toastSuccess.deleted('Alert'); | |
| await mutate(); | |
| } catch (error: unknown) { | |
| toastError.delete(error, 'Alert'); | |
| } | |
| }, | |
| [mutate, confirm] | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/AlertList.tsx` around lines 55 - 76, When deleting an alert
in handleDelete, clamp currentPage against the updated totalPages so it cannot
remain past the last page: compute newTotal = total - 1 and newTotalPages =
Math.max(1, Math.ceil(newTotal / pageSize)), and if currentPage > newTotalPages
call the page setter (e.g., setCurrentPage(newTotalPages)) before/after calling
mutate to ensure the next fetch isn't for an empty page; update handleDelete to
reference total, pageSize, currentPage and the page setter so the UI stays on a
valid page after deletions.
| {alerts.map((alert) => ( | ||
| <TableRow | ||
| key={alert.id} | ||
| data-testid={`alert-row-${alert.id}`} | ||
| className="hover:bg-gray-50 cursor-pointer" | ||
| onClick={() => router.push(`/alerts/${alert.id}`)} | ||
| > |
There was a problem hiding this comment.
Don't make row navigation mouse-only.
This TableRow is clickable but not keyboard-focusable, so keyboard users cannot open alert details from the list. Prefer a real link/button in the row, or add focus handling plus Enter/Space activation.
As per coding guidelines, "Preserve Radix UI's built-in accessibility props, add aria-label for icon-only buttons, ensure keyboard navigation (Tab, Enter, Escape), use semantic HTML elements, ensure sufficient color contrast".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/AlertList.tsx` around lines 181 - 187, The TableRow in
AlertList.tsx is clickable via onClick but not keyboard-accessible; update the
alerts.map rendering so each TableRow (or a semantic wrapper inside it) is
keyboard-focusable and operable: add tabIndex={0}, role="button" (or preferably
wrap the row content in a semantic <a> or Link element) and handle onKeyDown to
trigger the same navigation as onClick for Enter and Space keys, and provide an
accessible label (aria-label) if the row contains icon-only controls; ensure you
keep the existing onClick that calls router.push(`/alerts/${alert.id}`) and
mirror that call in the keyboard handler.
| <Select | ||
| value={pageSize.toString()} | ||
| onValueChange={(value) => { | ||
| setPageSize(parseInt(value)); | ||
| setCurrentPage(1); | ||
| }} | ||
| > | ||
| <SelectTrigger | ||
| className="h-7 text-sm border-gray-200 bg-white" | ||
| style={{ width: '70px' }} | ||
| > | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="10">10</SelectItem> | ||
| <SelectItem value="20">20</SelectItem> | ||
| <SelectItem value="50">50</SelectItem> | ||
| <SelectItem value="100">100</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <div className="flex items-center gap-1"> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => setCurrentPage(currentPage - 1)} | ||
| disabled={currentPage === 1} | ||
| className="h-7 px-2 hover:bg-gray-100 disabled:opacity-50" | ||
| > | ||
| <ChevronLeft className="h-4 w-4" /> | ||
| </Button> | ||
| <span className="text-sm text-gray-600 px-3 py-1"> | ||
| {currentPage} of {totalPages} | ||
| </span> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => setCurrentPage(currentPage + 1)} | ||
| disabled={currentPage >= totalPages} | ||
| className="h-7 px-2 hover:bg-gray-100 disabled:opacity-50" | ||
| > | ||
| <ChevronRight className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Add test IDs and labels to the footer controls.
The page-size selector and the pagination buttons are interactive controls, but they are missing data-testids, and the icon-only prev/next buttons also need aria-labels.
As per coding guidelines, "Always add data-testid attribute to all interactive elements (buttons, inputs, rows, etc.) using descriptive, kebab-case names" and "Preserve Radix UI's built-in accessibility props, add aria-label for icon-only buttons, ensure keyboard navigation (Tab, Enter, Escape), use semantic HTML elements, ensure sufficient color contrast".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/AlertList.tsx` around lines 281 - 323, Add descriptive
data-testid attributes to the interactive footer controls and add aria-labels
for the icon-only pagination buttons: add a data-testid like
"alerts-page-size-select" to the Select (or SelectTrigger) and/or each
SelectItem, add data-testid "alerts-page-size-10|20|50|100" as appropriate, and
add data-testid "alerts-prev-page-button" and "alerts-next-page-button" to the
two Button components; also add aria-label="Previous page" to the Button that
renders ChevronLeft and aria-label="Next page" to the Button that renders
ChevronRight. Keep the existing handlers (setPageSize, setCurrentPage) and state
(currentPage, totalPages) unchanged and ensure attributes are added on the
Select/SelectTrigger and Button elements referenced above.
| const customDateValue = | ||
| showDatePicker && filter.value ? parse(filter.value, 'yyyy-MM-dd', new Date()) : undefined; |
There was a problem hiding this comment.
Handle invalid date string parsing defensively.
If filter.value contains an invalid date string, parse() will return an Invalid Date. This could cause unexpected behavior when passed to the DatePicker.
🛡️ Proposed defensive fix
- const customDateValue =
- showDatePicker && filter.value ? parse(filter.value, 'yyyy-MM-dd', new Date()) : undefined;
+ const customDateValue = useMemo(() => {
+ if (!showDatePicker || !filter.value) return undefined;
+ const parsed = parse(filter.value, 'yyyy-MM-dd', new Date());
+ return isNaN(parsed.getTime()) ? undefined : parsed;
+ }, [showDatePicker, filter.value]);Note: You'll need to import useMemo from React.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const customDateValue = | |
| showDatePicker && filter.value ? parse(filter.value, 'yyyy-MM-dd', new Date()) : undefined; | |
| const customDateValue = useMemo(() => { | |
| if (!showDatePicker || !filter.value) return undefined; | |
| const parsed = parse(filter.value, 'yyyy-MM-dd', new Date()); | |
| return isNaN(parsed.getTime()) ? undefined : parsed; | |
| }, [showDatePicker, filter.value]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/FilterRow.tsx` around lines 69 - 70, customDateValue is
created by parsing filter.value and can become an Invalid Date; wrap the
computation in a useMemo and defensively check the parsed result with isValid
(or Number.isNaN / isNaN(parsed.getTime())) before returning it so you pass
either a real Date or undefined to the DatePicker. Specifically, update the
customDateValue computation (the expression using showDatePicker, filter.value
and parse) to run inside useMemo, call parse(filter.value, 'yyyy-MM-dd', new
Date()), validate the parsed Date, and return the Date only if valid—otherwise
return undefined; import useMemo from React and keep referencing
customDateValue, showDatePicker, filter.value and parse to locate the change.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={onRemove} | ||
| className="shrink-0" | ||
| data-testid="filter-remove-btn" | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Add aria-label to icon-only remove button.
The remove button only contains an icon and lacks an accessible label for screen readers. As per coding guidelines, icon-only buttons should have aria-label.
♿ Proposed fix
<Button
variant="ghost"
size="icon"
onClick={onRemove}
className="shrink-0"
data-testid="filter-remove-btn"
+ aria-label="Remove filter"
>
<X className="h-4 w-4" />
</Button>📝 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.
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={onRemove} | |
| className="shrink-0" | |
| data-testid="filter-remove-btn" | |
| > | |
| <X className="h-4 w-4" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={onRemove} | |
| className="shrink-0" | |
| data-testid="filter-remove-btn" | |
| aria-label="Remove filter" | |
| > | |
| <X className="h-4 w-4" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/alerts/FilterRow.tsx` around lines 90 - 98, The icon-only remove
Button (component Button rendering <X />) lacks an accessible label; update the
Button where onClick is onRemove and data-testid="filter-remove-btn" to include
an aria-label (e.g., aria-label="Remove filter" or a localized string) so screen
readers can announce its purpose, ensuring the change is applied to the Button
element that contains the <X className="h-4 w-4" /> icon.
1 participant
Summary by CodeRabbit
Release Notes