Frontend: Monitor Grouping Implementation #3021
Conversation
Add complete frontend support for monitor grouping: - Add group field to uptime and infrastructure monitor forms - Implement validation for group field (max 50 chars) - Create reusable hooks for monitor grouping logic - Add grouped display with collapsible sections - Integrate with backend API for group fetching - Support both grouped and ungrouped monitor views Features: - Group input fields in monitor create/edit forms - Automatic grouping and sorting of monitors - Collapsible group sections with monitor counts - Fallback to standard table when no groups exist - Consistent design using Checkmate UI components 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
gorkem-bwl
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
WalkthroughAdds monitor grouping: new GroupedDataTable UI, hooks to compute and fetch groups, inclusion of an optional monitor "group" field across forms, payloads, and validation, NetworkService support, and extensive tests and test tooling for the feature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Page as Monitors Page
participant Hook as useMonitorGrouping
participant GDT as GroupedDataTable
participant DT as DataTable
Page->>Hook: supply monitors[]
Hook-->>Page: { grouped, ungrouped }, hasGroups
alt hasGroups == true
Page->>GDT: headers, groupedMonitors, config
GDT->>GDT: render GroupSection per group
GDT->>DT: render DataTable for each group's monitors
else
Page->>DT: headers, monitors, config
DT-->>Page: render flat table
end
sequenceDiagram
autonumber
participant Comp as Component
participant UseHook as useGetGroupsByTeamId
participant Net as NetworkService
participant API as /monitors/team/groups
Comp->>UseHook: mount
UseHook->>UseHook: set isLoading=true
UseHook->>Net: getGroupsByTeamId()
Net->>API: GET /monitors/team/groups
API-->>Net: 200 JSON [...]
Net-->>UseHook: data
UseHook->>UseHook: set groups, isLoading=false
UseHook-->>Comp: [groups, isLoading, error]
note over UseHook: on error -> set error, isLoading=false, toast shown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Critical fix: The useUpdateMonitor hook was missing the group field in the updatedFields object, which meant group assignments would not be saved when editing existing monitors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
client/src/Components/v1/GroupedDataTable/index.jsx(1 hunks)client/src/Hooks/v1/useGroups.js(1 hunks)client/src/Hooks/v1/useMonitorGrouping.js(1 hunks)client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx(3 hunks)client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx(1 hunks)client/src/Pages/v1/Infrastructure/Create/index.jsx(1 hunks)client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx(4 hunks)client/src/Pages/v1/Uptime/Create/index.jsx(3 hunks)client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx(4 hunks)client/src/Utils/NetworkService.js(1 hunks)client/src/Validation/validation.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx (1)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-21)
client/src/Pages/v1/Uptime/Create/index.jsx (1)
client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
onChange(113-117)
client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (3)
client/src/Hooks/v1/useMonitorGrouping.js (4)
useMonitorGrouping(8-57)useMonitorGrouping(8-57)hasGroups(54-54)groupedMonitors(9-52)client/src/Components/v1/GroupedDataTable/index.jsx (5)
theme(27-27)theme(127-127)hasGroups(136-136)GroupedDataTable(121-190)groupedMonitors(134-134)client/src/Pages/v1/Uptime/Monitors/Components/LoadingSpinner/index.jsx (1)
LoadingSpinner(4-40)
client/src/Hooks/v1/useMonitorGrouping.js (3)
client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (1)
useMonitorGrouping(133-133)client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (1)
useMonitorGrouping(64-64)client/src/Components/v1/GroupedDataTable/index.jsx (2)
groupedMonitors(134-134)hasGroups(136-136)
client/src/Pages/v1/Infrastructure/Create/index.jsx (2)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-21)client/src/Pages/v1/Uptime/Create/index.jsx (2)
onChange(250-284)errors(68-68)
client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (3)
client/src/Hooks/v1/useMonitorGrouping.js (4)
useMonitorGrouping(8-57)useMonitorGrouping(8-57)hasGroups(54-54)groupedMonitors(9-52)client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (4)
useMonitorGrouping(133-133)tableConfig(135-146)theme(30-30)headers(39-103)client/src/Components/v1/GroupedDataTable/index.jsx (5)
theme(27-27)theme(127-127)hasGroups(136-136)GroupedDataTable(121-190)groupedMonitors(134-134)
client/src/Hooks/v1/useGroups.js (1)
client/src/Utils/NetworkService.js (1)
networkService(1163-1163)
client/src/Components/v1/GroupedDataTable/index.jsx (1)
client/src/Hooks/v1/useMonitorGrouping.js (2)
groupedMonitors(9-52)hasGroups(54-54)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (2)
client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
monitor(42-45)client/src/Pages/v1/Uptime/Create/index.jsx (1)
monitor(56-67)
🪛 GitHub Actions: Format Check (Client & Server)
client/src/Hooks/v1/useMonitorGrouping.js
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
client/src/Hooks/v1/useGroups.js
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
client/src/Components/v1/GroupedDataTable/index.jsx
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
🔇 Additional comments (30)
client/src/Validation/validation.js (2)
234-236: LGTM! Group validation is well-defined.The validation appropriately defines
groupas an optional field with a 50-character limit, trimming, and support for null/empty values.
454-456: LGTM! Consistent validation across schemas.The
groupvalidation ininfrastructureMonitorValidationmatches the one inmonitorValidation, ensuring consistency across monitor types.client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx (1)
17-17: LGTM! Appropriate handling of optional group field.Using
|| undefinedensures that falsy values (including empty strings) are sent asundefinedin the payload, which is appropriate for optional fields.client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (3)
6-6: LGTM! Group field properly initialized.The
groupfield is correctly added to the initial form state with an empty string default, consistent with other optional text fields.
41-41: LGTM! Create path initializes group correctly.The group field is properly reset to an empty string when initializing for creation.
63-63: LGTM! Update path handles group appropriately.Using
monitor.group || ""ensures the field defaults to an empty string if the monitor doesn't have a group set.client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
263-273: LGTM! Group input field is well-integrated.The Group field is properly added to the form with appropriate bindings, validation, and helpful placeholder text. The placement is logical and consistent with other optional fields.
client/src/Pages/v1/Uptime/Create/index.jsx (3)
194-194: LGTM! Group field added to create payload.The group field is appropriately included in the create monitor payload using the
|| undefinedpattern for optional fields.
214-214: LGTM! Group field added to update payload.The group field is consistently included in the update monitor payload.
612-622: LGTM! Group input field properly integrated.The Group TextInput is well-integrated into the uptime monitor form with proper state binding, validation, and helpful placeholder text.
client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (4)
4-4: LGTM! Appropriate imports for grouping functionality.The imports for
GroupedDataTableanduseMonitorGroupingare correctly added to support the new grouping feature.Also applies to: 18-18
64-64: LGTM! Grouping hook properly integrated.The
useMonitorGroupinghook is correctly called withfilteredMonitorsto derive grouping information.
191-203: LGTM! Excellent refactor with shared table configuration.Extracting the common table configuration eliminates duplication between the grouped and ungrouped rendering paths. This improves maintainability.
208-220: LGTM! Clean conditional rendering logic.The conditional rendering appropriately switches between
GroupedDataTableandDataTablebased on whether groups exist, while maintaining consistent configuration.client/src/Hooks/v1/useGroups.js (1)
14-34: Verify fetch-on-mount is the intended behavior.The empty dependency array
[]means groups are fetched only once when the component mounts. If groups can change during the component lifecycle (e.g., when a user creates/deletes a group), you may need to expose a refetch mechanism or add dependencies.Is fetch-on-mount the intended behavior, or should there be a way to refetch groups when they change?
client/src/Utils/NetworkService.js (2)
1035-1040: LGTM! Method implementation is correct.The
getGroupsByTeamId()method correctly makes a GET request to/monitors/team/groupswith appropriate headers.
1032-1041: No duplicate getGroupsByTeamId declarations. Only oneasync getGroupsByTeamId()exists in NetworkService.js; no further action needed.client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (4)
4-4: LGTM: Imports are correct.The new imports for
GroupedDataTableanduseMonitorGroupingare properly added and follow the existing import patterns.Also applies to: 17-17
123-123: LGTM: Group field correctly included.The
groupfield is properly included in the mapped monitor data, which enables the grouping functionality.
133-146: LGTM: Good refactor to extract common configuration.The extraction of
tableConfigreduces duplication between the grouped and non-grouped rendering paths. TheuseMonitorGroupinghook is correctly invoked with the transformed data.Note: The TODO comment on Line 136 suggests that similar row styling behavior exists elsewhere. Consider consolidating this pattern across tables in a future refactor.
151-165: LGTM: Conditional rendering correctly implemented.The conditional rendering between
GroupedDataTableandDataTablebased onhasGroupsis correct. Both branches properly pass the required props and share the commontableConfigfor consistent behavior.client/src/Hooks/v1/useMonitorGrouping.js (4)
8-12: LGTM: Clean hook signature with proper early return.The hook properly handles edge cases with a default empty array parameter and early return for null/empty input, avoiding unnecessary computation.
17-30: LGTM: Robust grouping logic.The grouping logic correctly handles:
- Different data structures via the dual check
monitor.group || monitor.monitor?.group- Whitespace with
trim()before validation and usage- Edge cases like empty/null groups
32-49: LGTM: Consistent and locale-aware sorting.The sorting implementation:
- Uses case-insensitive comparison (
sensitivity: 'base')- Provides fallback from
nametourlfor sorting keys- Applies consistent sorting to groups, monitors within groups, and ungrouped monitors
- Uses
localeComparefor proper internationalization support
9-9: LGTM: Proper memoization and return values.The hook correctly:
- Memoizes the grouping computation with
useMemo- Specifies the correct dependency (
monitors)- Computes
hasGroupsfor conditional rendering- Returns the expected shape for consumers
Also applies to: 51-56
client/src/Components/v1/GroupedDataTable/index.jsx (5)
19-108: LGTM: Well-structured collapsible group component.
GroupSectionprovides a clean, user-friendly interface with:
- Customizable default expanded state
- Clear group name and monitor count display with proper i18n pluralization
- Intuitive click-to-toggle interaction
- Reuse of existing
DataTablecomponent for consistency
121-132: LGTM: Clean component signature with conditional rendering.The component signature and early return pattern are consistent with the codebase's conventions for conditional rendering.
134-162: LGTM: Proper fallback handling for edge cases.The component correctly handles:
- Empty state when no monitors exist (grouped or ungrouped)
- Graceful degradation to regular
DataTablewhen grouping is not applicable- Uses fallback empty view message when config doesn't provide one
164-189: LGTM: Thoughtful grouped rendering with smart defaults.The rendering logic shows good UX consideration:
- Groups are rendered first, each with a unique key
- Ungrouped section uses i18n-friendly label
- Smart
defaultExpandedfor ungrouped: expanded when no groups exist, ensuring visibility when ungrouped monitors are the only content
192-208: LGTM: Comprehensive PropTypes definitions.The PropTypes properly validate:
- Required vs optional props
- Shape of
groupedMonitorsobject withgroupedandungroupedfields- All component props for both
GroupSectionandGroupedDataTable
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
client/src/Utils/NetworkService.js (1)
1032-1041: Consider adding JSDoc for consistency.All other methods in this file have detailed JSDoc comments. Adding documentation for
getGroupsByTeamId()would maintain codebase consistency and improve developer experience.Example JSDoc:
/** * ************************************ * Fetch all monitor groups for a team * ************************************ * * @async * @returns {Promise<AxiosResponse>} The response from the axios GET request containing the groups data. */ async getGroupsByTeamId() { return this.axiosInstance.get(`/monitors/team/groups`, { headers: { "Content-Type": "application/json", }, }); }client/src/Hooks/v1/useGroups.js (1)
14-34: Consider adding a refetch mechanism.The hook currently fetches groups only on mount (empty dependency array). If you need to refetch groups after certain actions (e.g., after creating a monitor with a new group), consider returning a refetch function.
Example implementation:
export const useGetGroupsByTeamId = () => { const [groups, setGroups] = useState([]); const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState(null); const [refetchTrigger, setRefetchTrigger] = useState(0); const refetch = useCallback(() => { setRefetchTrigger(prev => prev + 1); }, []); useEffect(() => { const fetchGroups = async () => { // ... existing fetch logic ... }; fetchGroups(); }, [refetchTrigger]); // Add refetchTrigger to dependencies return [groups, isLoading, error, refetch]; };This allows consumers to trigger a refetch when needed:
const [groups, isLoading, error, refetch] = useGetGroupsByTeamId();client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (1)
135-146: Address the TODO: consolidate repeated table configuration.The inline TODO correctly identifies that the
tableConfigpattern (row hover styles, cursor pointer, and onRowClick) is duplicated across monitor tables. This configuration appears in both Infrastructure and Uptime monitor tables.Consider extracting this configuration into a shared utility or higher-order component. Would you like me to open an issue to track this refactoring task, or generate a proposal for consolidating this pattern?
client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (1)
191-203: Duplicate table configuration pattern.This
tableConfigis nearly identical to the one inMonitorsTable/index.jsx(lines 135-146). Both define the same row hover styling, cursor pointer, and click navigation pattern.Consider consolidating this pattern as noted in the TODO comment in
MonitorsTable/index.jsx. A shared utility or configuration factory would reduce duplication and improve maintainability.client/src/Hooks/v1/useMonitorGrouping.js (1)
17-30: Remove unused nested group fallback
No references tomonitor.monitor.groupexist elsewhere—either drop the|| monitor.monitor?.groupfallback to simplify or add documentation linking it to the API’s nested monitor shape.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
client/src/Components/v1/GroupedDataTable/index.jsx(1 hunks)client/src/Hooks/v1/monitorHooks.js(1 hunks)client/src/Hooks/v1/useGroups.js(1 hunks)client/src/Hooks/v1/useMonitorGrouping.js(1 hunks)client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx(3 hunks)client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx(1 hunks)client/src/Pages/v1/Infrastructure/Create/index.jsx(1 hunks)client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx(4 hunks)client/src/Pages/v1/Uptime/Create/index.jsx(3 hunks)client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx(4 hunks)client/src/Utils/NetworkService.js(1 hunks)client/src/Validation/validation.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx (1)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-21)
client/src/Hooks/v1/useMonitorGrouping.js (3)
client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (1)
useMonitorGrouping(133-133)client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (1)
useMonitorGrouping(64-64)client/src/Components/v1/GroupedDataTable/index.jsx (2)
groupedMonitors(134-134)hasGroups(136-136)
client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (3)
client/src/Hooks/v1/useMonitorGrouping.js (4)
useMonitorGrouping(8-57)useMonitorGrouping(8-57)hasGroups(54-54)groupedMonitors(9-52)client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (4)
useMonitorGrouping(133-133)tableConfig(135-146)theme(30-30)headers(39-103)client/src/Components/v1/GroupedDataTable/index.jsx (5)
theme(27-27)theme(127-127)hasGroups(136-136)GroupedDataTable(121-190)groupedMonitors(134-134)
client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (2)
client/src/Hooks/v1/useMonitorGrouping.js (4)
useMonitorGrouping(8-57)useMonitorGrouping(8-57)hasGroups(54-54)groupedMonitors(9-52)client/src/Components/v1/GroupedDataTable/index.jsx (5)
theme(27-27)theme(127-127)hasGroups(136-136)GroupedDataTable(121-190)groupedMonitors(134-134)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (2)
client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
monitor(42-45)client/src/Pages/v1/Uptime/Create/index.jsx (1)
monitor(56-67)
client/src/Pages/v1/Uptime/Create/index.jsx (1)
client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
onChange(113-117)
client/src/Components/v1/GroupedDataTable/index.jsx (1)
client/src/Hooks/v1/useMonitorGrouping.js (1)
groupedMonitors(9-52)
client/src/Pages/v1/Infrastructure/Create/index.jsx (2)
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-21)client/src/Pages/v1/Uptime/Create/index.jsx (2)
onChange(250-284)errors(68-68)
client/src/Hooks/v1/useGroups.js (2)
client/src/Pages/v1/Uptime/Create/index.jsx (1)
isLoading(96-96)client/src/Utils/NetworkService.js (1)
networkService(1163-1163)
🪛 GitHub Actions: Format Check (Client & Server)
client/src/Hooks/v1/useMonitorGrouping.js
Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
client/src/Components/v1/GroupedDataTable/index.jsx
Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
client/src/Hooks/v1/useGroups.js
Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🔇 Additional comments (20)
client/src/Hooks/v1/monitorHooks.js (1)
372-372: LGTM!The group field is correctly added to the update payload with the
|| undefinedpattern, ensuring explicit undefined values are sent when the field is empty, which aligns with the backend's optional field handling.client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
6-6: LGTM!The group field is correctly initialized in all three state management scenarios: initial state, create initialization, and update initialization. The consistent use of empty string
""aligns with the form's text input expectations.Also applies to: 41-41, 63-63
client/src/Pages/v1/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx (1)
17-17: LGTM!The group field is correctly added to the infrastructure monitor payload using the
|| undefinedpattern, ensuring consistent optional field handling.client/src/Pages/v1/Uptime/Create/index.jsx (1)
194-194: LGTM!The group field is correctly included in both create and update payloads with the
|| undefinedpattern.Also applies to: 214-214
client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx (4)
4-4: LGTM!The imports for
GroupedDataTableanduseMonitorGroupingare correctly added to support the new grouping feature.Also applies to: 17-17
123-123: LGTM!The group field is correctly added to the data mapping, enabling the grouping functionality.
133-133: LGTM!The
useMonitorGroupinghook is correctly applied to compute grouped monitors and determine if groups exist.
151-165: LGTM!The conditional rendering correctly switches between
GroupedDataTable(when groups exist) and standardDataTable(when no groups exist), maintaining consistent configuration and loading state handling.client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx (3)
4-4: LGTM!The imports for
GroupedDataTableanduseMonitorGroupingare correctly added.Also applies to: 18-18
64-64: LGTM!The
useMonitorGroupinghook correctly receivesfilteredMonitorsas input, ensuring grouping respects search and filter operations.
208-220: LGTM!The conditional rendering correctly switches between grouped and standard table views, maintaining consistent header and configuration handling.
client/src/Hooks/v1/useMonitorGrouping.js (3)
8-12: LGTM!The hook structure is well-designed with proper use of
useMemofor performance optimization and an early return for the empty array edge case.
32-49: LGTM!The sorting logic is well-implemented with:
- Case-insensitive alphabetical sorting for both groups and monitors
- Proper fallback to empty string when
nameorurlis missing- Consistent sorting applied to both grouped and ungrouped monitors
54-56: LGTM!The return value correctly provides both
groupedMonitors(the sorted data structure) andhasGroups(a convenient boolean flag) for consumers.client/src/Components/v1/GroupedDataTable/index.jsx (6)
26-32: LGTM!The state management for the collapsible group is straightforward and correctly implements expand/collapse functionality.
46-91: LGTM!The group header is well-designed with:
- Accessible click target (entire header)
- Proper i18n pluralization for monitor count
- Theme-consistent styling
- Clear visual feedback with expand/collapse icons
94-106: LGTM!The collapsible content correctly wraps the
DataTableand provides a group-specific empty state message.
130-162: LGTM!The guard clauses are well-structured, progressively handling edge cases:
- Early return when
shouldRenderis false- Empty state when no monitors exist
- Standard table when only ungrouped monitors exist
- Grouped view for all other cases
This provides a clear fallback strategy and maintains backward compatibility.
164-189: LGTM!The rendering logic is well-designed:
- Proper React key for mapped groups
- Smart default expansion (ungrouped section only expands when it's the only section)
- Consistent i18n usage with fallback text
192-208: LGTM!The PropTypes definitions are comprehensive and correctly validate the component APIs, including the shape of
groupedMonitorswith itsgroupedandungroupedproperties.
| createToast({ | ||
| body: "Failed to load monitor groups", | ||
| }); |
There was a problem hiding this comment.
Hard-coded error message breaks i18n/l10n support.
The error message should use the t() function for internationalization, consistent with other toasts in the codebase.
Apply this diff:
+import { useTranslation } from "react-i18next";
+
export const useGetGroupsByTeamId = () => {
+ const { t } = useTranslation();
const [groups, setGroups] = useState([]);
// ... other state ...
// ... in the catch block:
createToast({
- body: "Failed to load monitor groups",
+ body: t("failedToLoadGroups"),
});Note: Add the translation key failedToLoadGroups to your i18n translation files.
🧰 Tools
🪛 GitHub Actions: Format Check (Client & Server)
Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🤖 Prompt for AI Agents
In client/src/Hooks/v1/useGroups.js around lines 25-27, replace the hard-coded
toast message with a localized string by passing t('failedToLoadGroups') to
createToast; if t isn't in scope import or obtain it via the project i18n hook
(e.g., useTranslation()) and use that t function, and add the new key
failedToLoadGroups to the i18n translation files as requested.
| <TextInput | ||
| type="text" | ||
| id="group" | ||
| name="group" | ||
| label="Group" | ||
| placeholder="e.g., Production, Development, Network" | ||
| isOptional={true} | ||
| value={infrastructureMonitor.group} | ||
| onChange={onChange} | ||
| error={errors["group"]} | ||
| /> |
There was a problem hiding this comment.
Hard-coded label breaks i18n/l10n support.
The label "Group" should use the t() function for internationalization, consistent with other labels in this form (e.g., t("infrastructureDisplayNameLabel")).
Apply this diff to use a translation key:
<TextInput
type="text"
id="group"
name="group"
- label="Group"
+ label={t("infrastructureGroupLabel")}
placeholder="e.g., Production, Development, Network"
isOptional={true}
value={infrastructureMonitor.group}
onChange={onChange}
error={errors["group"]}
/>Note: Ensure the translation key infrastructureGroupLabel is added to your i18n translation files.
📝 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.
| <TextInput | |
| type="text" | |
| id="group" | |
| name="group" | |
| label="Group" | |
| placeholder="e.g., Production, Development, Network" | |
| isOptional={true} | |
| value={infrastructureMonitor.group} | |
| onChange={onChange} | |
| error={errors["group"]} | |
| /> | |
| <TextInput | |
| type="text" | |
| id="group" | |
| name="group" | |
| label={t("infrastructureGroupLabel")} | |
| placeholder="e.g., Production, Development, Network" | |
| isOptional={true} | |
| value={infrastructureMonitor.group} | |
| onChange={onChange} | |
| error={errors["group"]} | |
| /> |
🤖 Prompt for AI Agents
In client/src/Pages/v1/Infrastructure/Create/index.jsx around lines 263 to 273
the TextInput label is hard-coded as "Group", breaking i18n; replace the literal
with a call to the t() translation function (e.g.,
t("infrastructureGroupLabel")) and pass that result as the label prop, and
ensure the key infrastructureGroupLabel is added to the i18n translation files
for all supported locales.
| <TextInput | ||
| name="group" | ||
| type="text" | ||
| label="Group" | ||
| isOptional={true} | ||
| placeholder="e.g., Production, Development, Network" | ||
| value={monitor.group || ""} | ||
| onChange={onChange} | ||
| error={errors["group"] ? true : false} | ||
| helperText={errors["group"]} | ||
| /> |
There was a problem hiding this comment.
Hard-coded label breaks i18n/l10n support.
The label "Group" should use the t() function for internationalization, consistent with other labels in this form (e.g., t("displayName"), t("checkFrequency")).
Apply this diff to use a translation key:
<TextInput
name="group"
type="text"
- label="Group"
+ label={t("groupLabel")}
isOptional={true}
placeholder="e.g., Production, Development, Network"
value={monitor.group || ""}
onChange={onChange}
error={errors["group"] ? true : false}
helperText={errors["group"]}
/>Note: Ensure the translation key groupLabel is added to your i18n translation files.
📝 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.
| <TextInput | |
| name="group" | |
| type="text" | |
| label="Group" | |
| isOptional={true} | |
| placeholder="e.g., Production, Development, Network" | |
| value={monitor.group || ""} | |
| onChange={onChange} | |
| error={errors["group"] ? true : false} | |
| helperText={errors["group"]} | |
| /> | |
| <TextInput | |
| name="group" | |
| type="text" | |
| label={t("groupLabel")} | |
| isOptional={true} | |
| placeholder="e.g., Production, Development, Network" | |
| value={monitor.group || ""} | |
| onChange={onChange} | |
| error={errors["group"] ? true : false} | |
| helperText={errors["group"]} | |
| /> |
🤖 Prompt for AI Agents
In client/src/Pages/v1/Uptime/Create/index.jsx around lines 612 to 622, the
TextInput label is hard-coded as "Group" which breaks i18n; replace the literal
with the translation call t("groupLabel") (and ensure the translation key
groupLabel exists in i18n files), i.e., import/use the existing t function in
scope and pass t("groupLabel") to the label prop to match other localized labels
in the form.
- Fixed formatting issues for CI/CD compliance - Added test infrastructure with Vitest and React Testing Library - Created comprehensive test suite: - Unit tests for useMonitorGrouping hook - Unit tests for useGetGroupsByTeamId hook - Component tests for GroupedDataTable - Integration tests for form submission with group field - Added test utilities and mocks for consistent testing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR implements monitor grouping with solid architecture but has minor issues affecting user experience, performance, and maintainability that should be addressed.
📄 Documentation Diagram
This diagram documents the new monitor grouping workflow implemented in the frontend.
sequenceDiagram
participant U as User
participant F as Frontend
participant B as Backend
participant G as Grouping Logic
participant T as Grouped Data Table
U->>F: View monitor list
F->>B: Fetch monitors
B-->>F: Return monitors
F->>G: Group monitors by group field
G-->>F: Return grouped data
F->>T: Render grouped table with collapsible sections
T-->>U: Display organized monitors
note over T: PR #35;3021 adds grouping display
🌟 Strengths
- Well-designed reusable components and hooks for consistent grouping logic.
- Good integration with existing UI components and validation patterns.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | Components/v1/.../index.jsx | Maintainability | Collapse state not persisted, poor UX on re-renders | |
| P2 | Hooks/v1/useMonitorGrouping.js | Performance | Expensive sorting could slow performance with many monitors | MonitorsTable, UptimeDataTable |
| P2 | Hooks/v1/useGroups.js | Architecture | No caching or team scope, unnecessary API calls | |
| P2 | Pages/v1/.../index.jsx | Architecture | Duplicated validation schema risks inconsistencies | validation.js |
| P2 | Components/v1/.../index.jsx | Documentation | Missing translation keys hinders internationalization | |
| P2 | Hooks/v1/useMonitorGrouping.js | Bug | Inconsistent data access could cause grouping errors | MonitorsTable, UptimeDataTable |
🔍 Notable Themes
- Internationalization Debt: Multiple components use hardcoded strings without translation keys, hindering multi-language support.
- Code Consistency: Duplicated validation logic and inconsistent data access patterns increase maintenance burden and risk of errors.
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: client/src/Pages/v1/Uptime/Create/index.jsx
The group validation schema is duplicated across multiple files (Uptime Create and validation.js). This violates DRY principles and could lead to validation inconsistencies. Changes to group validation rules would need to be made in multiple places.
Suggestion:
// In validation.js - export the group validation
export const groupValidation = joi.string().max(50).trim().allow(null, "").optional().messages({
"string.max": "Group name must be 50 characters or less.",
});
// In Uptime Create - import and use
import { groupValidation } from "@/Validation/validation.js";
const validationSchema = {
group: groupValidation,
// ... other validations
};
Related Code:
const validationSchema = {
group: joi.string().max(50).trim().allow(null, "").optional().messages({
"string.max": "Group name must be 50 characters or less.",
}),
};
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| const GroupSection = ({ | ||
| groupName, | ||
| monitors, | ||
| headers, | ||
| config, | ||
| defaultExpanded = true, | ||
| }) => { | ||
| const [expanded, setExpanded] = useState(defaultExpanded); | ||
| const theme = useTheme(); | ||
| const { t } = useTranslation(); |
There was a problem hiding this comment.
P2 | Confidence: High
The defaultExpanded prop defaults to true but there's no mechanism to persist collapse state across component re-renders. When users collapse groups and then perform actions that trigger re-renders (like refreshing data), all groups will expand again. This creates a poor user experience where users lose their preferred view state.
Code Suggestion:
const GroupSection = ({
groupName,
monitors,
headers,
config,
defaultExpanded = true
}) => {
// Use groupName as part of the key for persistence
const storageKey = `group-${groupName}-expanded`;
const [expanded, setExpanded] = useState(() => {
const saved = localStorage.getItem(storageKey);
return saved !== null ? JSON.parse(saved) : defaultExpanded;
});
const toggleExpanded = () => {
const newExpanded = !expanded;
setExpanded(newExpanded);
localStorage.setItem(storageKey, JSON.stringify(newExpanded));
};
P2 | Confidence: High
The component uses translation function t() but doesn't provide translation keys for hardcoded strings like "No monitors in ${groupName}". This creates internationalization debt and inconsistent user experience across languages.
Code Suggestion:
// In the DataTable usage within GroupSection
<DataTable
headers={headers}
data={monitors}
config={{
...config,
emptyView: t('groupedDataTable.emptyGroup', { group: groupName }),
}}
/>
// And ensure translation keys are defined:
// groupedDataTable.emptyGroup: "No monitors in {{group}}"
| export const useGetGroupsByTeamId = () => { | ||
| const [groups, setGroups] = useState([]); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const [error, setError] = useState(null); | ||
|
|
||
| useEffect(() => { | ||
| const fetchGroups = async () => { | ||
| setIsLoading(true); |
There was a problem hiding this comment.
P2 | Confidence: High
The hook fetches groups on every mount without any caching strategy or dependency injection for teamId. This could lead to unnecessary API calls when components re-mount. In a multi-team environment, this would fail to scope groups to the current team.
Code Suggestion:
export const useGetGroupsByTeamId = (teamId) => {
const [groups, setGroups] = useState([]);
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState(null);
useEffect(() => {
if (!teamId) return;
const fetchGroups = async () => {
setIsLoading(true);
setError(null);
try {
const response = await networkService.getGroupsByTeamId(teamId);
setGroups(response?.data?.data || []);
} catch (err) {
// error handling
} finally {
setIsLoading(false);
}
};
fetchGroups();
}, [teamId]);| export const useMonitorGrouping = (monitors = []) => { | ||
| const groupedMonitors = useMemo(() => { | ||
| if (!monitors || monitors.length === 0) { | ||
| return { grouped: {}, ungrouped: [] }; | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
The hook performs expensive sorting operations on every render when monitors change. In large deployments with hundreds of monitors, the localeCompare operations with sensitivity settings could cause performance issues. Both MonitorsTable and UptimeDataTable use this hook, potentially doubling the impact during data refreshes.
Code Suggestion:
export const useMonitorGrouping = (monitors = []) => {
const groupedMonitors = useMemo(() => {
if (!monitors || monitors.length === 0) {
return { grouped: {}, ungrouped: [] };
}
// Use simpler comparison for better performance
const caseInsensitiveCompare = (a, b) => a.localeCompare(b);
// ... rest of sorting logic with caseInsensitiveCompareEvidence: path:client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx, path:client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx
|
|
||
| monitors.forEach((monitor) => { | ||
| // Check if monitor has a group (either from the monitor object or monitor.monitor object) | ||
| const group = monitor.group || monitor.monitor?.group; |
There was a problem hiding this comment.
P2 | Confidence: Medium
The grouping logic handles both monitor.group and monitor.monitor.group but this creates inconsistent data access patterns. The infrastructure and uptime tables pass different monitor object structures, which could lead to grouping inconsistencies. This dual access pattern suggests an architectural inconsistency in how monitor data is structured across the application.
Code Suggestion:
// Standardize on a single approach - either always use monitor.group
// or create a helper function to extract group consistently
const getMonitorGroup = (monitor) => {
return monitor.group || monitor.monitor?.group || '';
};
// Use throughout the grouping logic
const group = getMonitorGroup(monitor);Evidence: path:client/src/Pages/v1/Infrastructure/Monitors/Components/MonitorsTable/index.jsx, path:client/src/Pages/v1/Uptime/Monitors/Components/UptimeDataTable/index.jsx
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
client/src/Components/v1/GroupedDataTable/index.jsx (1)
1-210: Run Prettier to fix formatting issues.Previous reviews identified Prettier formatting inconsistencies in this file. Please run:
prettier --write client/src/Components/v1/GroupedDataTable/index.jsxThen verify the formatting passes CI checks before merging.
🧹 Nitpick comments (2)
client/src/Hooks/v1/__tests__/useMonitorGrouping.test.js (1)
178-200: Consider clarifying the test name.The test name "should be case-sensitive for group names" is slightly misleading. Group names are indeed case-sensitive (creating distinct groups), but the sorting uses case-insensitive comparison (
{ sensitivity: "base" }). Consider renaming to something like "should treat different casing as distinct groups" to better reflect what's being tested.Example:
- it("should be case-sensitive for group names", () => { + it("should treat different casing as distinct groups", () => {client/src/Components/v1/GroupedDataTable/__tests__/index.test.jsx (1)
88-116: Complete the expand/collapse test.The test logic for expand/collapse is incomplete. After clicking to collapse, the test has a comment "This test might need adjustment based on actual implementation" and doesn't verify the collapsed state.
Consider completing the test:
// Click to collapse if (expandButton) { fireEvent.click(expandButton); } else { // If no specific button, click the whole header fireEvent.click(infrastructureHeader); } - // After collapse, there should be fewer data tables - // Note: This test might need adjustment based on actual implementation + // After collapse, the Infrastructure group's table should not be visible + // but other groups should still be visible + await waitFor(() => { + const tables = screen.getAllByTestId("data-table"); + expect(tables.length).toBeLessThan(3); + });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
client/package.json(3 hunks)client/src/Components/v1/GroupedDataTable/__tests__/index.test.jsx(1 hunks)client/src/Components/v1/GroupedDataTable/index.jsx(1 hunks)client/src/Hooks/v1/__tests__/useGroups.test.js(1 hunks)client/src/Hooks/v1/__tests__/useMonitorGrouping.test.js(1 hunks)client/src/Hooks/v1/useGroups.js(1 hunks)client/src/Hooks/v1/useMonitorGrouping.js(1 hunks)client/src/Pages/v1/Infrastructure/Create/__tests__/form-integration.test.jsx(1 hunks)client/src/Pages/v1/Uptime/Create/__tests__/form-integration.test.jsx(1 hunks)client/src/test/i18n-mock.js(1 hunks)client/src/test/setup.js(1 hunks)client/src/test/utils.jsx(1 hunks)client/vitest.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/src/test/i18n-mock.js
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/Hooks/v1/useGroups.js
- client/src/Hooks/v1/useMonitorGrouping.js
🧰 Additional context used
🧬 Code graph analysis (8)
client/src/Hooks/v1/__tests__/useMonitorGrouping.test.js (2)
client/src/Hooks/v1/useMonitorGrouping.js (3)
useMonitorGrouping(8-57)useMonitorGrouping(8-57)groupedMonitors(9-52)client/src/test/utils.jsx (2)
mockMonitors(47-168)mockMonitors(47-168)
client/src/test/setup.js (1)
server/src/middleware/v1/sanitization.js (1)
window(5-5)
client/src/Pages/v1/Infrastructure/Create/__tests__/form-integration.test.jsx (2)
client/src/test/utils.jsx (1)
renderWithProviders(34-44)client/src/Pages/v1/Infrastructure/Create/index.jsx (1)
CreateInfrastructureMonitor(30-377)
client/src/Hooks/v1/__tests__/useGroups.test.js (2)
client/src/test/utils.jsx (2)
mockGroups(171-175)mockGroups(171-175)client/src/Hooks/v1/useGroups.js (2)
useGetGroupsByTeamId(9-37)useGetGroupsByTeamId(9-37)
client/src/Components/v1/GroupedDataTable/index.jsx (2)
client/src/test/utils.jsx (1)
theme(8-31)client/src/Hooks/v1/useMonitorGrouping.js (1)
groupedMonitors(9-52)
client/src/Pages/v1/Uptime/Create/__tests__/form-integration.test.jsx (2)
client/src/test/utils.jsx (1)
renderWithProviders(34-44)client/src/Pages/v1/Uptime/Create/index.jsx (1)
UptimeCreate(51-819)
client/src/test/utils.jsx (1)
client/src/Components/v1/GroupedDataTable/index.jsx (2)
theme(28-28)theme(127-127)
client/src/Components/v1/GroupedDataTable/__tests__/index.test.jsx (2)
client/src/test/utils.jsx (1)
renderWithProviders(34-44)client/src/Components/v1/GroupedDataTable/index.jsx (1)
GroupedDataTable(126-190)
🔇 Additional comments (20)
client/src/Hooks/v1/__tests__/useMonitorGrouping.test.js (9)
7-26: LGTM!The test correctly verifies basic grouping functionality, including the
hasGroupsflag, group structure, monitor counts, and alphabetical ordering within groups.
28-35: LGTM!The test correctly validates that monitors with
nullor empty string groups are treated as ungrouped and sorted alphabetically.
37-42: LGTM!The test correctly verifies alphabetical sorting of group names.
44-50: LGTM!The test correctly handles the edge case of an empty monitors array.
52-58: LGTM!The test correctly handles the edge case of undefined monitors input.
60-84: LGTM!The test thoroughly covers all variations of ungrouped monitors (null, empty string, missing property).
86-113: LGTM!The test correctly validates behavior when all monitors have groups assigned.
115-136: LGTM!The test correctly validates the fallback behavior for nested
monitor.monitor.groupstructure.
138-156: LGTM!The test correctly validates that direct
groupproperty takes precedence over nestedmonitor.group.client/src/Pages/v1/Infrastructure/Create/__tests__/form-integration.test.jsx (3)
1-96: LGTM!The mock setup is comprehensive and properly isolated. The beforeEach/afterEach hooks ensure clean test state between runs.
98-227: LGTM!The tests comprehensively cover Group field rendering, user input, validation, and state management. The assertions correctly verify form behavior and integration with the submission flow.
229-334: LGTM!The tests thoroughly verify that the Group field integrates correctly with all other form sections (HTTPS protocol, notifications, incident config, custom alerts, frequency selection, and validation flow).
client/src/Components/v1/GroupedDataTable/__tests__/index.test.jsx (3)
1-66: LGTM!The mock setup effectively isolates the GroupedDataTable component for testing. The mock DataTable renders data as JSON for easy verification, and the test data structure aligns with expected input format.
131-213: LGTM!The tests thoroughly cover edge cases including empty data, ungrouped-only scenarios, grouped-only scenarios, and the
shouldRenderflag.
215-337: LGTM!The tests correctly verify config propagation, custom and default empty views, and handling of multiple groups with varying monitor counts.
client/src/Pages/v1/Uptime/Create/__tests__/form-integration.test.jsx (2)
1-57: LGTM!The mock setup is comprehensive and follows good testing practices with proper cleanup between tests.
219-345: LGTM!The tests comprehensively cover Group field behavior across different monitor types (ping, port) and protocol switching (HTTP/HTTPS). The error clearing test is particularly valuable for ensuring proper form state management.
client/src/Components/v1/GroupedDataTable/index.jsx (3)
1-113: LGTM!The GroupSection component is well-implemented with proper state management, theme integration, and accessibility features (clickable header, expand/collapse icons). The monitor count badge correctly handles singular/plural forms.
115-190: LGTM!The GroupedDataTable component correctly handles all rendering scenarios:
- Early return for shouldRender=false
- Empty state with fallback message
- Ungrouped-only as regular table
- Grouped sections with optional ungrouped section
- Smart defaultExpanded logic for ungrouped section
192-210: LGTM!The PropTypes definitions are comprehensive and correctly mark required vs optional props. The shape validation for
groupedMonitorsensures proper data structure.
| it("should reset error state on new fetch attempt", async () => { | ||
| const mockError = new Error("Network error"); | ||
| mockNetworkService.getGroupsByTeamId.mockRejectedValueOnce(mockError); | ||
|
|
||
| const { result, rerender } = renderHook(() => useGetGroupsByTeamId()); | ||
|
|
||
| // Wait for the error to be handled | ||
| await waitFor(() => { | ||
| expect(result.current[2]).toBe("Network error"); | ||
| }); | ||
|
|
||
| // Mock successful response for retry | ||
| const mockGroups = [{ name: "Infrastructure" }]; | ||
| mockNetworkService.getGroupsByTeamId.mockResolvedValueOnce({ | ||
| data: { | ||
| data: mockGroups, | ||
| }, | ||
| }); | ||
|
|
||
| // Trigger re-render (simulating component re-mount or dependency change) | ||
| rerender(); | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current[1]).toBe(false); // isLoading should be false | ||
| }); | ||
|
|
||
| expect(result.current[0]).toEqual(mockGroups); // groups should be set | ||
| expect(result.current[2]).toBe(null); // error should be reset | ||
| }); |
There was a problem hiding this comment.
rerender() won't trigger a refetch here.
useGetGroupsByTeamId runs its effect only on mount ([] deps). Calling rerender() never re-executes the fetch, so this test will keep error === "Network error" and fail. Remount the hook (e.g., via renderHook with a changing key or unmount + fresh render) or expose an explicit refetch API instead of assuming rerender resets state.
🤖 Prompt for AI Agents
In client/src/Hooks/v1/__tests__/useGroups.test.js around lines 121 to 149, the
test incorrectly uses rerender() to trigger a new fetch even though
useGetGroupsByTeamId only runs its effect on mount; update the test to remount
the hook so the effect runs again — either call unmount() then renderHook()
again (or renderHook with a changing key prop) to simulate a fresh mount before
mocking the successful response, then assert isLoading, groups and error as
currently written.
| it("should handle whitespace-only group names", () => { | ||
| const monitorsWithWhitespaceGroups = [ | ||
| { | ||
| _id: "1", | ||
| name: "Monitor 1", | ||
| group: " ", | ||
| }, | ||
| { | ||
| _id: "2", | ||
| name: "Monitor 2", | ||
| group: "\\t\\n", | ||
| }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook(() => useMonitorGrouping(monitorsWithWhitespaceGroups)); | ||
|
|
||
| expect(result.current.hasGroups).toBe(false); | ||
| expect(result.current.groupedMonitors.ungrouped).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
Fix escaped characters in whitespace test.
Line 168 uses the literal string "\\t\\n" instead of actual whitespace characters. This doesn't test the intended scenario of tab and newline characters.
Apply this diff to use actual whitespace characters:
{
_id: "2",
name: "Monitor 2",
- group: "\\t\\n",
+ group: "\t\n",
},📝 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.
| it("should handle whitespace-only group names", () => { | |
| const monitorsWithWhitespaceGroups = [ | |
| { | |
| _id: "1", | |
| name: "Monitor 1", | |
| group: " ", | |
| }, | |
| { | |
| _id: "2", | |
| name: "Monitor 2", | |
| group: "\\t\\n", | |
| }, | |
| ]; | |
| const { result } = renderHook(() => useMonitorGrouping(monitorsWithWhitespaceGroups)); | |
| expect(result.current.hasGroups).toBe(false); | |
| expect(result.current.groupedMonitors.ungrouped).toHaveLength(2); | |
| }); | |
| it("should handle whitespace-only group names", () => { | |
| const monitorsWithWhitespaceGroups = [ | |
| { | |
| _id: "1", | |
| name: "Monitor 1", | |
| group: " ", | |
| }, | |
| { | |
| _id: "2", | |
| name: "Monitor 2", | |
| group: "\t\n", | |
| }, | |
| ]; | |
| const { result } = renderHook(() => useMonitorGrouping(monitorsWithWhitespaceGroups)); | |
| expect(result.current.hasGroups).toBe(false); | |
| expect(result.current.groupedMonitors.ungrouped).toHaveLength(2); | |
| }); |
🤖 Prompt for AI Agents
In client/src/Hooks/v1/__tests__/useMonitorGrouping.test.js around lines 158 to
176, the test uses the literal string "\\t\\n" rather than actual tab/newline
characters; replace the double-escaped string with the proper escaped characters
so the group contains a tab and newline (i.e., use "\t\n" instead of "\\t\\n"),
then re-run the test to ensure whitespace-only group names are handled and the
two monitors are classified as ungrouped.
| it("should trim whitespace from group name", async () => { | ||
| const user = userEvent.setup(); | ||
| renderUptimeCreateForm(); | ||
|
|
||
| const groupInput = screen.getByLabelText(/group/i); | ||
| await user.type(groupInput, " Production "); | ||
|
|
||
| const urlInput = screen.getByLabelText(/website url/i); | ||
| await user.type(urlInput, "example.com"); | ||
|
|
||
| const nameInput = screen.getByLabelText(/display name/i); | ||
| await user.type(nameInput, "Test Monitor"); | ||
|
|
||
| const submitButton = screen.getByRole("button", { name: /save/i }); | ||
| await user.click(submitButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockCreateMonitor).toHaveBeenCalledWith({ | ||
| monitor: expect.objectContaining({ | ||
| group: " Production ", // Frontend validation should handle trimming | ||
| }), | ||
| redirect: "/uptime", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test name is misleading.
The test is named "should trim whitespace from group name" but actually verifies that whitespace is NOT trimmed (line 212 expects " Production "). Either the test name should be updated to reflect actual behavior, or trimming logic should be implemented if desired.
Consider one of these options:
- Update the test name to reflect current behavior:
- it("should trim whitespace from group name", async () => {
+ it("should preserve whitespace in group name", async () => {- Or implement trimming in the form (if that's the intended behavior):
group: monitor.group?.trim() || undefined🤖 Prompt for AI Agents
In client/src/Pages/v1/Uptime/Create/__tests__/form-integration.test.jsx around
lines 193-217 the test is misnamed — it claims "should trim whitespace from
group name" but asserts that the submitted group contains leading/trailing
spaces; either rename the test to reflect current behavior (e.g., "should
preserve whitespace in group name" and keep the expectation), or change the code
that builds the monitor payload to trim the group value before submission (call
trim() on monitor.group or set group to monitor.group?.trim() || undefined) and
update the test expectation to expect "Production" without surrounding spaces.
| global.process = { | ||
| ...global.process, | ||
| env: { | ||
| NODE_ENV: "test", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Do not replace global.process.
Reassigning global.process to a plain object drops Node’s process prototype and wipes existing environment variables, breaking any code (or Vitest itself) that expects standard process APIs/config. Set process.env.NODE_ENV instead of replacing the whole object.
-global.process = {
- ...global.process,
- env: {
- NODE_ENV: "test",
- },
-};
+process.env = {
+ ...process.env,
+ NODE_ENV: "test",
+};📝 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.
| global.process = { | |
| ...global.process, | |
| env: { | |
| NODE_ENV: "test", | |
| }, | |
| }; | |
| process.env = { | |
| ...process.env, | |
| NODE_ENV: "test", | |
| }; |
🤖 Prompt for AI Agents
In client/src/test/setup.js around lines 4 to 9, do not replace global.process
with a plain object because that removes Node's process prototype and existing
env vars; instead set the environment variable directly by assigning
process.env.NODE_ENV = "test" (or process.env.NODE_ENV ||= "test" if you want to
preserve an existing value) and remove the global.process reassignment so the
native process object and its APIs remain intact.
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
@gorkem-bwl can we remove the tests and related dependencies from this PR? We haven't set up a front end testing regime set up yet so anything added now is probably going to be pulled out eventually anyway. Thanks!
Summary
Frontend implementation for monitor grouping functionality. Depends on backend PR #3019.
What's Done
GroupedDataTablecomponent anduseMonitorGroupinghookKey Files
src/Components/v1/GroupedDataTable/index.jsx- Main grouped display componentsrc/Hooks/v1/useMonitorGrouping.js- Grouping logic hooksrc/Hooks/v1/useGroups.js- API hook for groupsTest Cases Added
useMonitorGroupinghook (edge cases, sorting, validation)useGetGroupsByTeamIdhook (API integration, error handling)GroupedDataTable(rendering, collapsing, empty states)🤖 Generated with Claude Code
Summary by CodeRabbit