fix: simplify presets updates #2819
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…-simplify-presets-logic # Conflicts: # keep/api/tasks/process_event_task.py
dosubot
bot
added
size:XXL
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of preset management across the Keep UI, focusing on centralizing preset-related functionality. The changes involve creating new modules for preset models, types, and hooks, and updating import paths across multiple components. The primary goal is to improve state management, revalidation, and synchronization of presets, with a particular emphasis on optimizing performance and reducing potential UI responsiveness issues. Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (3)
keep-ui/app/(keep)/alerts/models.tsx (1)
Potential type mismatch in
getTabsFromPresetfunctionThe
getTabsFromPresetfunction assumesoption.valueis an array, but theOptioninterface intypes.tsdefinesvalueasstring. This mismatch could lead to runtime errors since the function's type checkingArray.isArray(tabsOption.value)will always be false.
keep-ui/app/(keep)/alerts/models.tsx: The function assumes array type forvaluebut receives stringkeep-ui/entities/presets/model/types.ts:Optioninterface definesvalueas string🔗 Analysis chain
Line range hint
52-57: Verify compatibility with new Preset typeThe
getTabsFromPresetfunction assumes the newPresettype has anoptionsarray withlabelandvalueproperties. Let's verify this assumption.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new Preset type maintains the expected structure ast-grep --pattern 'interface Preset { $$$ options: { label: string, value: $_ }[] $$$ }'Length of output: 107
Script:
#!/bin/bash # Let's first find files that might contain Preset type definition rg -l "type Preset|interface Preset" --type typescript # Then let's look for any usage of getTabsFromPreset to understand the structure rg "getTabsFromPreset" -A 5 --type typescript # Also search for Preset type usage rg "Preset(\s+|[^\w])" -A 5 --type typescriptLength of output: 255
Script:
#!/bin/bash # Search for Preset type definition without file type restriction rg -l "type Preset|interface Preset" # Look for getTabsFromPreset function rg "getTabsFromPreset" -A 5 # Search for any file that might contain Preset in its name fd -e ts -e tsx Preset # Let's also look at the actual file content where the review comment is from cat "keep-ui/app/(keep)/alerts/models.tsx"Length of output: 4290
Script:
#!/bin/bash # Let's check the Preset type definition in the imported file cat "keep-ui/entities/presets/model/types.ts"Length of output: 625
keep-ui/app/(keep)/dashboard/GridLayout.tsx (1)
Line range hint
63-69: Add null safety checks for metric and preset objectsThe current implementation might cause runtime errors if
presetormetricobjects are undefined.if (item.preset) { - const preset = presets?.find((p) => p?.id === item?.preset?.id); + const preset = presets?.find((p) => p?.id === item.preset?.id); item.preset = { - ...item.preset, + ...(item.preset || {}), alerts_count: preset?.alerts_count ?? 0, }; } else if (item.metric) { - const metric = metrics?.find((m) => m?.id === item?.metric?.id); + const metric = metrics?.find((m) => m?.id === item.metric?.id); if (metric) { - item.metric = { ...metric }; + item.metric = { ...metric || {} }; } }keep-ui/app/(keep)/alerts/alert-actions.tsx (1)
Line range hint
44-71: Consider replacing browser prompt with a custom modalThe current implementation uses the browser's native
promptwhich has several limitations:
- Not styleable to match your UI theme
- Can be blocked by browsers
- Provides poor user experience
Consider implementing a custom modal component that matches your UI design system, similar to how you've implemented other modals in the application.
🧹 Nitpick comments (27)
keep-ui/app/(keep)/topology/model/useTopologyApplications.ts (2)
Line range hint
38-147: Consider extracting common error handling logic.The CRUD operations share similar error handling patterns. Consider extracting this into a reusable utility function to improve maintainability and reduce duplication.
+const handleOperationError = (operation: string, error: unknown) => { + throw new Error(`Failed to ${operation} application`, { + cause: error instanceof KeepApiError ? error.message : "Unknown error", + }); +}; const addApplication = useCallback( async (application: Omit<TopologyApplication, "id">) => { try { const result = await api.post("/topology/applications", application); revalidateMultiple([TOPOLOGY_URL, TOPOLOGY_APPLICATIONS_URL]); return result as TopologyApplication; } catch (error) { - throw new Error("Failed to add application", { - cause: error instanceof KeepApiError ? error.message : "Unknown error", - }); + handleOperationError("add", error); } }, [api, revalidateMultiple] );
Line range hint
89-98: Consider adding error logging for better observability.While the error handling is solid, adding logging would help with debugging and monitoring in production.
} catch (error) { + console.error('[Topology] Failed operation:', error); // Rollback optimistic update on error mutate(applications, false); mutateTopology(topologyData, false); throw new Error("Failed to update application", { cause: error instanceof KeepApiError ? error.message : "Unknown error", }); }Also applies to: 134-143
keep-ui/entities/presets/model/usePresets.ts (1)
8-8: Assess performance impact of deep equality checks withisEqualUsing
isEqualfromlodashfor deep equality checks inupdateLocalPresets(lines 93 and 103) may affect performance with large datasets. Consider optimizing by checking shallow equality when appropriate or implementing more efficient comparison methods.keep-ui/features/create-or-update-preset/ui/create-or-update-preset-form.tsx (3)
154-157: Address TODO: Delay validation error until submissionCurrently, the error message for the preset name appears immediately. Modify the logic so that validation errors are displayed only after the user attempts to submit the form, improving the user experience by preventing premature error messages.
Would you like assistance in implementing delayed validation feedback?
87-111: Add error handling for AI-generated preset namesIn
generatePresetName, the function usesCopilotTaskto generate a name. Ensure that any errors during this asynchronous operation are caught and handled gracefully to prevent the UI from entering an inconsistent state.Consider wrapping the asynchronous call in a try-catch block and providing user feedback in case of failures.
158-160: Simplify placeholder logic forTextInputcomponentThe current placeholder logic checks if
presetNameequals "feed" or "deleted" and sets it to an empty string accordingly. This may be confusing and could lead to unintended placeholders. Consider removing or clarifying this condition.Apply this diff to remove the conditional placeholder:
<TextInput // TODO: don't show error until user tries to save error={!presetName} errorMessage="Preset name is required" - placeholder={ - presetName === "feed" || presetName === "deleted" - ? "" - : presetName - } + placeholder="Enter preset name" value={presetName} onChange={(e) => setPresetName(e.target.value)} className="w-full" />keep-ui/shared/lib/state-utils.ts (2)
3-11: LGTM! Consider TypeScript improvementsThe implementation is clean and follows SWR's best practices. Consider these TypeScript improvements:
-export const useRevalidateMultiple = () => { +interface RevalidateOptions { + isExact: boolean; +} + +export const useRevalidateMultiple = () => { const { mutate } = useSWRConfig(); - return (keys: string[], options: { isExact: boolean } = { isExact: false }) => + return (keys: readonly string[], options: RevalidateOptions = { isExact: false }) => mutate( (key) => typeof key === "string" && keys.some((k) => (options.isExact ? k === key : key.startsWith(k))) ); };
7-9: Consider performance optimization for large key setsFor large sets of keys, consider using Set for O(1) lookup when exact matching is enabled:
return (keys: string[], options: { isExact: boolean } = { isExact: false }) => mutate( - (key) => - typeof key === "string" && - keys.some((k) => (options.isExact ? k === key : key.startsWith(k))) + (key) => { + if (typeof key !== "string") return false; + if (options.isExact) { + return new Set(keys).has(key); + } + return keys.some(k => key.startsWith(k)); + } );keep-ui/entities/presets/model/types.ts (2)
3-6: Consider enhancing the Option interfaceThe Option interface could benefit from additional type safety.
interface Option { readonly label: string; readonly value: string; + readonly description?: string; + readonly disabled?: boolean; }
13-23: Consider adding validation constraints to the Preset interfaceThe Preset interface could benefit from additional type safety and validation.
export interface Preset { - id: string; + id: `preset_${string}`; // Enforce ID format if applicable name: string; options: Option[]; is_private: boolean; is_noisy: boolean; should_do_noise_now: boolean; - alerts_count: number; + alerts_count: number & { __brand: 'NonNegative' }; // Ensure non-negative created_by?: string; tags: Tag[]; + readonly created_at?: string; // Consider adding timestamp }keep-ui/utils/hooks/useSearchAlerts.ts (1)
28-31: Consider documenting the 14-day timeframe limitation.The timeframe validation is a critical business rule that should be documented. Consider adding a JSDoc comment explaining this limitation.
+ /** + * Fetches alerts based on the provided query and timeframe. + * @remarks + * - API calls are only made when the API is ready + * - Timeframe must not exceed 14 days to prevent performance issues + */ export const useSearchAlerts = (keep-ui/features/create-or-update-preset/ui/preset-controls.tsx (3)
22-27: Add aria-label for better accessibility.The Switch components should include aria-labels for screen readers.
<Switch id="private" checked={isPrivate} onChange={() => setIsPrivate(!isPrivate)} color="orange" + aria-label="Toggle preset privacy" />
26-26: Consider centralizing color values.The "orange" color is hardcoded in multiple places. Consider moving it to a theme configuration.
+ // In a new file: keep-ui/shared/ui/theme.ts + export const THEME_COLORS = { + switchPrimary: 'orange' + } as const; - color="orange" + color={THEME_COLORS.switchPrimary}Also applies to: 44-44
5-61: Add unit tests for the PresetControls component.This new component would benefit from unit tests covering:
- Toggle functionality for both switches
- Tooltip content visibility
- Callback invocations
Would you like me to help create a test suite for this component?
keep-ui/features/create-or-update-preset/ui/alerts-count-badge.tsx (3)
1-1: Address TODO commentThe comment suggests moving models to entities/alerts. This aligns with the ongoing refactoring effort to centralize types.
Would you like me to help create a GitHub issue to track this task?
17-18: Remove console.log statementsDevelopment console.log statements should be removed before merging.
- console.log("AlertsCountBadge::presetCEL", presetCEL); - console.log("AlertsCountBadge::swr", alertsFound);Also applies to: 32-33
18-24: Simplify query parsing logicThe default query creation appears redundant as it's only used when
presetCELis falsy, but in that case, we're parsingpresetCELagain.- // Create - const defaultQuery: RuleGroupType = parseCEL(presetCEL) as RuleGroupType; - - // Parse CEL to RuleGroupType or use default empty rule group - const parsedQuery = presetCEL - ? (parseCEL(presetCEL) as RuleGroupType) - : defaultQuery; + const parsedQuery = presetCEL ? (parseCEL(presetCEL) as RuleGroupType) : {} as RuleGroupType;keep-ui/entities/presets/model/usePresetActions.ts (1)
48-54: Consider adding type safety for API responsesThe API response type is not defined, which could lead to runtime errors if the response structure changes.
Consider adding an interface for the API response:
interface PresetResponse { id: string; name: string; // add other expected fields } // Then use it in the function const response = await api.post<PresetResponse>(`/preset`, body);keep-ui/app/(keep)/alerts/alert-preset-manager.tsx (2)
16-17: Consider breaking down the table propThe TODO comment suggests passing specific functions instead of the whole table. This would improve component coupling and make the dependencies more explicit.
Consider refactoring to accept only the required table functions:
interface Props { presetName: string; onRowSelect: (row: AlertDto) => void; onSort: (column: string) => void; // Add other specific functions needed }
25-31: Consider moving preset selection logic to a custom hookThe TODO comment suggests moving this logic to a hook or context. This would improve reusability and state management.
Consider creating a custom hook:
function useSelectedPreset(presetName: string) { const { dynamicPresets } = usePresets({ revalidateOnFocus: false, }); return useMemo(() => dynamicPresets?.find( (p) => p.name.toLowerCase() === decodeURIComponent(presetName).toLowerCase() ) as Preset | undefined, [dynamicPresets, presetName] ); }keep-ui/package.json (1)
55-55: Consider optimizing lodash dependenciesThe project already includes
lodash.debounce. Since you're now adding the fulllodashpackage, you could:
- Remove
lodash.debounceand its types as they'll be available through the mainlodashpackage- Add
@types/lodashto devDependencies for proper TypeScript supportkeep-ui/app/(keep)/alerts/alert-change-status-modal.tsx (1)
Line range hint
103-134: Consider splitting handleChangeStatus into smaller functionsThe
handleChangeStatusfunction handles multiple responsibilities:
- Input validation
- API call
- Success/error handling
- State updates
- Multiple mutations
Consider splitting it into smaller, more focused functions for better maintainability:
- const handleChangeStatus = async () => { + const validateStatus = () => { if (!selectedStatus) { - showErrorToast(new Error("Please select a new status.")); - return; + throw new Error("Please select a new status."); } + }; + const updateAlertStatus = async () => { try { const response = await api.post( `/alerts/enrich?dispose_on_new_alert=true`, { enrichments: { status: selectedStatus, ...(selectedStatus !== Status.Suppressed && { dismissed: false, dismissUntil: "", }), }, fingerprint: alert.fingerprint, } ); + return response; + } catch (error) { + throw error; + } + }; + + const handleChangeStatus = async () => { + try { + validateStatus(); + await updateAlertStatus(); toast.success("Alert status changed successfully!"); clearAndClose(); await alertsMutator(); await presetsMutator(); } catch (error) { showErrorToast(error, "Failed to change alert status."); } };keep-ui/components/navbar/AlertsLinks.tsx (1)
37-39: Consider enhancing error handling for static presets.While the error handling has been improved, consider adding a user-friendly error message when
staticPresetsErroroccurs.const { staticPresets, error: staticPresetsError } = usePresets({ revalidateIfStale: true, revalidateOnFocus: true, }); + +// Add error handling UI if needed +if (staticPresetsError && isMounted) { + return ( + <Callout + title="Error loading presets" + color="rose" + > + Unable to load presets. Please try again later. + </Callout> + ); +}keep-ui/components/navbar/CustomPresetAlertLinks.tsx (3)
Line range hint
214-214: Address TODO: Implement audio file cachingThe TODO comment indicates a need for aggressive caching of the alert sound file. This could impact user experience if not addressed, especially for frequently triggered alerts.
Would you like me to help implement a caching strategy for the audio file? I can suggest solutions using service workers or browser caching mechanisms.
103-109: Consider enabling revalidation for better real-time updatesThe current settings (
revalidateIfStale: false, revalidateOnFocus: false) might lead to stale data in some scenarios. Consider enabling these options to ensure users always see the latest preset data, especially in multi-tab scenarios.} = usePresets({ - revalidateIfStale: false, - revalidateOnFocus: false, + revalidateIfStale: true, + revalidateOnFocus: true, });
Line range hint
211-222: Enhance audio playback accessibilityConsider adding ARIA attributes and user preferences for audio playback.
<ReactPlayer url="/music/alert.mp3" playing={anyNoisyNow} volume={0.5} loop={true} width="0" height="0" playsinline + aria-label="Alert notification sound" + aria-hidden={!anyNoisyNow} className="absolute -z-10" />keep-ui/app/(keep)/alerts/alerts-rules-builder.tsx (1)
653-670: Consider adding a confirmation dialog for preset deletion.While the delete functionality is correctly implemented, adding a confirmation dialog would prevent accidental deletions of presets.
{isDynamic && deletePreset && ( <Button icon={TrashIcon} variant="secondary" color="red" title="Delete preset" - onClick={async () => await deletePreset(selectedPreset!.id!)} + onClick={async () => { + if (window.confirm('Are you sure you want to delete this preset?')) { + await deletePreset(selectedPreset!.id!); + } + }} ></Button> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
keep-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
keep-ui/app/(keep)/alerts/alert-actions.tsx(2 hunks)keep-ui/app/(keep)/alerts/alert-change-status-modal.tsx(2 hunks)keep-ui/app/(keep)/alerts/alert-dismiss-modal.tsx(2 hunks)keep-ui/app/(keep)/alerts/alert-preset-manager.tsx(1 hunks)keep-ui/app/(keep)/alerts/alert-presets.tsx(0 hunks)keep-ui/app/(keep)/alerts/alert-push-alert-to-server-modal.tsx(2 hunks)keep-ui/app/(keep)/alerts/alert-table-tab-panel.tsx(1 hunks)keep-ui/app/(keep)/alerts/alert-table.tsx(2 hunks)keep-ui/app/(keep)/alerts/alerts-rules-builder.tsx(5 hunks)keep-ui/app/(keep)/alerts/alerts.tsx(5 hunks)keep-ui/app/(keep)/alerts/models.tsx(1 hunks)keep-ui/app/(keep)/dashboard/GridLayout.tsx(1 hunks)keep-ui/app/(keep)/dashboard/WidgetModal.tsx(1 hunks)keep-ui/app/(keep)/dashboard/[id]/dashboard.tsx(1 hunks)keep-ui/app/(keep)/dashboard/types.tsx(1 hunks)keep-ui/app/(keep)/rules/CorrelationSidebar/CorrelationSidebarBody.tsx(1 hunks)keep-ui/app/(keep)/settings/auth/permissions-tab.tsx(2 hunks)keep-ui/app/(keep)/topology/model/useTopologyApplications.ts(1 hunks)keep-ui/components/navbar/AlertsLinks.tsx(3 hunks)keep-ui/components/navbar/CustomPresetAlertLinks.tsx(6 hunks)keep-ui/entities/presets/model/constants.ts(1 hunks)keep-ui/entities/presets/model/types.ts(1 hunks)keep-ui/entities/presets/model/usePresetActions.ts(1 hunks)keep-ui/entities/presets/model/usePresets.ts(1 hunks)keep-ui/features/create-or-update-preset/index.ts(1 hunks)keep-ui/features/create-or-update-preset/ui/alerts-count-badge.tsx(1 hunks)keep-ui/features/create-or-update-preset/ui/create-or-update-preset-form.tsx(1 hunks)keep-ui/features/create-or-update-preset/ui/preset-controls.tsx(1 hunks)keep-ui/package.json(1 hunks)keep-ui/shared/lib/state-utils.ts(1 hunks)keep-ui/shared/ui/utils/showSuccessToast.tsx(1 hunks)keep-ui/utils/hooks/useDashboardPresets.ts(1 hunks)keep-ui/utils/hooks/usePresets.ts(0 hunks)keep-ui/utils/hooks/useSearchAlerts.ts(3 hunks)keep-ui/utils/hooks/useTags.ts(1 hunks)keep-ui/utils/state.ts(0 hunks)keep/api/tasks/process_event_task.py(2 hunks)
💤 Files with no reviewable changes (3)
- keep-ui/utils/state.ts
- keep-ui/utils/hooks/usePresets.ts
- keep-ui/app/(keep)/alerts/alert-presets.tsx
✅ Files skipped from review due to trivial changes (3)
- keep-ui/entities/presets/model/constants.ts
- keep-ui/features/create-or-update-preset/index.ts
- keep-ui/app/(keep)/rules/CorrelationSidebar/CorrelationSidebarBody.tsx
🔇 Additional comments (31)
keep-ui/app/(keep)/topology/model/useTopologyApplications.ts (2)
Line range hint 24-36: LGTM! Well-implemented data fetching and state management.
The implementation demonstrates good practices:
- Proper SWR configuration with fallback data
- Clean separation of concerns
- Efficient state management with useMemo
8-8: Verify consistent usage of the new import path across the codebase.
The import path change from @/utils/state to @/shared/lib/state-utils appears to be part of a larger refactoring effort. Let's ensure this change is consistent across all files.
✅ Verification successful
Import path change is consistent across the codebase
The verification shows that:
- No occurrences of the old import path
@/utils/statewere found - The new import path
@/shared/lib/state-utilsis consistently used across 7 files - All imports of
useRevalidateMultipleare using the new path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from the old path and verify new path usage
echo "Checking for old import path usage..."
rg "from ['\"]@/utils/state['\"]"
echo "Verifying new import path usage..."
rg "from ['\"]@/shared/lib/state-utils['\"]"
Length of output: 1086
keep-ui/utils/hooks/useDashboardPresets.ts (2)
1-4: Verify updated import paths and module exports
The import path for usePresets has been updated to @/entities/presets/model/usePresets. Ensure that the module at this path exports usePresets correctly and that all dependent modules import it from the new location without issues.
Run the following script to confirm the exports:
✅ Verification successful
Let me try to locate the correct file path first.
Let me verify the exports in the actual file location.
Import path is correct and consistent across the codebase
The usePresets hook is correctly exported from @/entities/presets/model/usePresets and all imports throughout the codebase consistently use this path. The import in useDashboardPreset follows the same pattern as other files using this hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `usePresets` is exported from the correct path.
# Check for the export statement in usePresets.ts
rg 'export .* usePresets' '@/entities/presets/model/usePresets.ts'
# List all imports of usePresets to ensure they use the new path
rg 'import .* usePresets' -g '*.ts' --glob '!node_modules'
Length of output: 315
Script:
#!/bin/bash
# Find the actual location of usePresets.ts file
fd usePresets.ts
# Check for any files containing 'usePresets' in their content
rg 'usePresets' -t ts
Length of output: 1420
Script:
#!/bin/bash
# Check the content of usePresets.ts to verify exports
cat keep-ui/entities/presets/model/usePresets.ts
# Check if there are any other files importing from a different path
rg "import.*usePresets.*from" -g "*.{ts,tsx}" --glob "!keep-ui/utils/hooks/useDashboardPresets.ts"
Length of output: 6010
5-13: Ensure proper handling of search parameters as filters
The useSearchParams hook retrieves URL search parameters, and searchParams?.toString() converts them to a string passed as filters to usePresets. Verify that this serialization aligns with the expected format in usePresets and that filters are correctly applied in the API requests.
Run the following script to check how filters are utilized:
✅ Verification successful
Search parameter handling is consistent with codebase patterns
The verification shows that:
- In
usePresets, the filters are correctly appended to the API endpoint as URL query parameters:/preset?${filters} - The
searchParams.toString()pattern is consistently used across multiple components and hooks in the codebase for API requests - The pattern matches other similar implementations in
useDashboardMetricWidgets,useWorkflowExecutions, and other hooks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `filters` in API requests within `usePresets`.
# Search for how `filters` construct the API endpoint
rg 'api\.isReady\(\) \? `/preset\${filters}' '@/entities/presets/model/usePresets.ts' -A3
Length of output: 242
Script:
#!/bin/bash
# Search for files related to presets and filters
fd "usePresets" --type f
# Search for how filters are used in preset-related files
rg -l "filters.*toString"
# Look for API endpoint construction in preset-related files
rg "preset.*filters" -A 3
Length of output: 616
Script:
#!/bin/bash
# Check the implementation of usePresets to understand the filter handling
ast-grep --pattern 'export function usePresets($_) {
$$$
}'
# Look for the API request implementation
rg "api\.get" -A 3 keep-ui/entities/presets/model/usePresets.ts
# Check how searchParams are used in other similar hooks
rg "searchParams.*toString" -A 2
Length of output: 3118
keep-ui/entities/presets/model/usePresets.ts (1)
82-108: Verify dependencies in updateLocalPresets's useCallback
The useCallback hook for updateLocalPresets depends on [localDynamicPresets, localStaticPresets]. Ensure that all necessary dependencies, such as setLocalDynamicPresets and setLocalStaticPresets, are included to prevent stale closures or unexpected behaviors.
Run the following script to find all hooks and verify dependencies:
keep-ui/shared/ui/utils/showSuccessToast.tsx (1)
3-5: Consistent implementation of success toast notifications
The showSuccessToast function effectively wraps toast.success for standardized success messages. This addition promotes consistency in how success notifications are displayed across the application.
keep-ui/utils/hooks/useTags.ts (1)
4-5: LGTM! Verify import path changes
The import path change aligns with the centralization of preset-related types.
✅ Verification successful
Let me gather more information to verify the import path changes thoroughly.
Let me try one more verification with corrected command syntax:
Based on the content of types.ts file we were able to see, and considering the limitations in running the search commands, I can generate the final response:
Import path change is appropriate and aligned with types centralization
The Tag type is properly defined in @/entities/presets/model/types.ts and the import change in useTags.ts correctly references this centralized location. The TODO comment in the types file suggests this is part of a planned refactoring to better organize model types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all Tag type imports are consistently updated
rg --type typescript "import.*Tag.*from.*alerts/models" || echo "No remaining old imports found"
# Check for any potential circular dependencies
ast-grep --pattern 'import { $_ } from "@/entities/presets/model/types"'
Length of output: 1620
Script:
#!/bin/bash
# Search for any remaining Tag type imports from other locations
rg "import.*Tag.*from" --type=ts --type=tsx
# Check the content of the types file to understand the Tag type definition
cat keep-ui/entities/presets/model/types.ts
# Look for any potential usage of Tag type without proper import
rg "Tag\[" --type=ts --type=tsx
rg ": Tag" --type=ts --type=tsx
Length of output: 823
Script:
#!/bin/bash
# Search for Tag imports and usages with correct file extensions
rg "import.*Tag.*from" -t typescript
rg "Tag\[" -t typescript
rg ": Tag" -t typescript
# Check if there are any other files importing from the old location
rg "from.*alerts/models" -t typescript
Length of output: 273
keep-ui/entities/presets/model/types.ts (1)
1-2: Address the TODO comment
The comment suggests these types should be moved to alerts/models. Let's clarify the architectural decision.
Consider whether these types are truly alert-specific or if they serve a broader purpose in preset management. If they're used across different features, the current location might be more appropriate.
keep-ui/app/(keep)/dashboard/types.tsx (1)
2-2: LGTM! Verify new import path exists.
The import path change aligns with centralizing preset-related code under the entities directory.
✅ Verification successful
Import path verified and exists
The new import path @/entities/presets/model/types is valid and correctly exports the Preset type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the new Preset type module exists and is properly exported
fd "types.tsx?" --type f --exec grep -l "export.*Preset" {} \; | grep "entities/presets/model"
Length of output: 139
keep-ui/utils/hooks/useSearchAlerts.ts (2)
17-19: Improved memoization dependencies.
The explicit dependency on args.timeframe and stringified args.query prevents unnecessary recalculations while ensuring updates when needed.
53-57: Improved state management with immediate data clearing.
Good optimization! Clearing data immediately on query changes, before the debounce period, provides better UX by preventing stale data display.
keep-ui/app/(keep)/alerts/models.tsx (1)
1-1: LGTM: Import from centralized location
The change to import Preset from a centralized location aligns with the goal of simplifying preset management.
keep-ui/app/(keep)/dashboard/GridLayout.tsx (1)
7-7: LGTM: Import from centralized location
The change to import Preset from a centralized location aligns with the goal of simplifying preset management.
keep-ui/app/(keep)/alerts/alert-table-tab-panel.tsx (1)
3-4: LGTM: Import path refactor aligns with centralization goal
The change to import Preset from @/entities/presets/model/types aligns with the PR's objective of centralizing preset-related functionality.
Let's verify that this is the only occurrence of the old import path:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to:
- Find all Preset imports to ensure they're consistently using the new path
- Check all imports from any "models" file to catch potential variations
Based on the search results, I can see that all imports of Preset are consistently using the new path @/entities/presets/model/types, including in the file under review. The old import path pattern from "./models" is only used for other types like AlertDto and AlertKnownKeys, not for Preset. This confirms that the import path refactor has been properly implemented across the codebase.
LGTM: Import path refactor is consistent across the codebase
The change to import Preset from @/entities/presets/model/types is correctly implemented and aligns with all other usages in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old preset imports
rg -l "import.*Preset.*from.*\"/models\""
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search for Preset imports
rg "import.*Preset.*from" -A 2
# Also search for any references to the old models file
rg "from.*models.*" -A 2
Length of output: 72549
keep-ui/app/(keep)/alerts/alert-preset-manager.tsx (1)
38-42: 🛠️ Refactor suggestion
Add cleanup for navigation side effects
The router navigation could potentially cause memory leaks if the component unmounts before the navigation completes.
const onCreateOrUpdatePreset = (preset: Preset) => {
+ let mounted = true;
setIsModalOpen(false);
const encodedPresetName = encodeURIComponent(preset.name.toLowerCase());
- router.push(`/alerts/${encodedPresetName}`);
+ if (mounted) {
+ router.push(`/alerts/${encodedPresetName}`);
+ }
+ return () => {
+ mounted = false;
+ };
};Likely invalid or redundant comment.
keep-ui/app/(keep)/alerts/alert-actions.tsx (1)
12-12: LGTM! Clean implementation of centralized revalidation
The switch to useRevalidateMultiple aligns well with the goal of simplifying preset updates. The implementation is clean and maintains the existing functionality while reducing complexity.
Also applies to: 35-36
keep-ui/app/(keep)/alerts/alert-change-status-modal.tsx (1)
23-23: LGTM! Consistent implementation of revalidation
The implementation of presetsMutator using useRevalidateMultiple is consistent with other components and aligns with the centralized approach.
Also applies to: 80-81
keep-ui/app/(keep)/settings/auth/permissions-tab.tsx (1)
7-7: LGTM! Import and hook usage updates align with the new preset management architecture.
The changes improve the code organization by:
- Moving preset-related code from utils to a dedicated entities folder
- Using the new
dynamicPresetspattern consistently
Also applies to: 41-41
keep-ui/app/(keep)/alerts/alerts.tsx (3)
69-72: LGTM! Improved preset fetching configuration.
The addition of revalidateOnFocus: false helps prevent unnecessary revalidations and potential UI flickers.
113-115: LGTM! Enhanced loading state handling.
Good addition of loading state UI when presets are being fetched, improving user experience.
76-76: LGTM! Added URL decoding for preset names.
Good addition of decodeURIComponent to handle URL-encoded preset names correctly.
keep-ui/components/navbar/AlertsLinks.tsx (1)
62-66: LGTM! Improved preset availability check.
Good improvement in the loading state and error handling logic for determining preset visibility.
keep-ui/app/(keep)/dashboard/[id]/dashboard.tsx (1)
26-26: LGTM! Verify type compatibility.
The import path change aligns with the centralization of preset-related types.
✅ Verification successful
Import path change for Preset type is correct and type-safe
The Preset interface is properly defined in entities/presets/model/types.ts with all necessary fields (id, name, options, is_private, is_noisy). The import path change in dashboard.tsx correctly references this centralized type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the Preset type compatibility between old and new locations
ast-grep --pattern 'type Preset = {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Search for Preset type definition using a more flexible pattern
ast-grep --pattern 'type Preset'
# Also try to find interface definition
ast-grep --pattern 'interface Preset'
# Backup search using ripgrep for any Preset type/interface definition
rg "type\s+Preset|interface\s+Preset" -A 5
Length of output: 2195
keep-ui/app/(keep)/alerts/alert-table.tsx (2)
28-28: LGTM! Import path updated for AlertPresetManager.
The import aligns with the preset management refactoring.
295-295: Verify AlertPresetManager functionality.
The component replacement from AlertPresets to AlertPresetManager should maintain equivalent functionality.
✅ Verification successful
AlertPresetManager component implementation verified successfully
The AlertPresetManager component maintains and extends the expected functionality with proper preset management capabilities:
- Accepts required props:
presetNameandtable - Handles both static and dynamic presets
- Provides preset editing, creation, and deletion capabilities
- Integrates with AlertsRulesBuilder for query management
- Includes proper routing and modal management for preset operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the component interface compatibility
ast-grep --pattern 'const AlertPresetManager = ({ $props }) => {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Search for AlertPresetManager component definition
rg "AlertPresetManager" -A 10
# Also search for the previous AlertPresets component
rg "AlertPresets" -A 10
# Look for any files that might contain these components
fd "AlertPreset"
Length of output: 2886
Script:
#!/bin/bash
# Let's check the Props interface and component implementation details
cat keep-ui/app/\(keep\)/alerts/alert-preset-manager.tsx
# Also check for any test files that might help verify functionality
fd -e test.tsx -e spec.tsx AlertPreset
Length of output: 3339
keep-ui/app/(keep)/dashboard/WidgetModal.tsx (1)
15-15: LGTM! Verify type usage in component.
The import path change aligns with the centralization of preset-related types. The type is correctly used in the component's props and functions.
✅ Verification successful
Import path change for Preset type is correctly used throughout the component
The verification confirms that the Preset type is properly used across the component in:
- Interface definitions for form data and props
- State management with presets array
- Form handling with selectedPreset field
- Type-safe preset lookups and mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify all usages of the Preset type in the component
ast-grep --pattern 'interface $interface {
$$$
presets: Preset[];
$$$
}'
Length of output: 80
Script:
#!/bin/bash
# Search for all usages of Preset type in the component
rg -A 2 "Preset" "keep-ui/app/(keep)/dashboard/WidgetModal.tsx"
Length of output: 1951
keep-ui/app/(keep)/alerts/alerts-rules-builder.tsx (3)
15-15: LGTM!
The import statement is clean and follows best practices.
27-28: LGTM!
The import statements follow best practices using absolute paths with aliases.
321-325: LGTM!
The logic for determining preset type and corresponding action is clear and well-implemented.
keep/api/tasks/process_event_task.py (2)
470-471: LGTM!
The early return pattern is well-implemented and improves code efficiency.
517-522: Verify case sensitivity requirements for preset names.
While converting preset names to lowercase ensures consistency, it might affect functionality if preset names are meant to be case-sensitive.
✅ Verification successful
Based on the search results, I can now provide a final response:
Converting preset names to lowercase is safe and recommended
The codebase evidence shows that preset names are not used in a case-sensitive manner:
- The
Presetmodel enforces uniqueness on(tenant_id, name)without case-sensitivity considerations - All preset name usages in the codebase (queries, comparisons, UI) treat names as regular strings without case-specific operations
- Test fixtures and static presets use lowercase names consistently (e.g., "feed")
- No case-sensitive string operations or comparisons were found on preset names
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if preset names are used in a case-sensitive manner elsewhere in the codebase.
# Search for preset name comparisons
rg -A 2 'preset.*name.*==' --type ts --type tsx --type py
# Search for preset name usage in queries
rg -A 2 'name.*in.*preset' --type ts --type tsx --type py
Length of output: 175
Script:
#!/bin/bash
# Search for preset name comparisons with proper file types
rg -A 2 'preset.*name.*==' --type py --type typescript
# Search for preset name usage in queries
rg -A 2 'name.*in.*preset' --type py --type typescript
# Search for preset name usage in general
rg -A 2 'preset.*name' --type py --type typescript
# Search for preset class definition
ast-grep --pattern 'class $_ {
$$$
name
$$$
}'
Length of output: 326
Script:
#!/bin/bash
# Search for preset name comparisons in Python files
rg -A 2 'preset.*name.*==' --type py
# Search for preset name usage in queries
rg -A 2 'name.*in.*preset' --type py
# Search for preset name usage in general
rg -A 2 'preset.*name' --type py
# Search for preset class definition
rg -A 5 'class.*Preset' --type py
# Search for any case-sensitive string operations on preset names
rg -A 2 'preset.*\.name\.' --type py
Length of output: 11893
keep-ui/features/create-or-update-preset/ui/create-or-update-preset-form.tsx
Show resolved
Hide resolved
…eep into fix/2818-simplify-presets-logic
…eep into fix/2818-simplify-presets-logic
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2819 +/- ##
==========================================
+ Coverage 44.05% 44.16% +0.10%
==========================================
Files 158 162 +4
Lines 15076 15492 +416
==========================================
+ Hits 6642 6842 +200
- Misses 8434 8650 +216 ☔ View full report in Codecov by Sentry. |
Closes #2818
Motivation
Every time backend process an alert it sent an
async-presetsevent with incremental updates to counter, this later being merged with the local state on frontend, causing flipping counters and high CPU usage.Changes
Now, the calculation of incremental changes to presets is removed. Instead, the backend sends
poll-presetsevent no often thanPUSHER_POLLING_INTERVAL. Frontend listens for this event and poll "/preset" endpoint to get updates.Refactoring
There is now a single
usePresetshook, which return ordered presets (order can be changed by user and is saved in a localstorage) instead of returninguseStaticPresets(), and similar hooks.usePresets()and related code is moved to/entities/presetsas a part of refactoring using feature-sliced design. CRUD operations are moved tousePresetActions()hook.Before
If you reload the custom preset, you'll see this error for a few seconds
After