fix: Clean up filter hooks #234
Conversation
WalkthroughThe pull request introduces significant changes across multiple components and hooks in the application, primarily focusing on enhancing type safety and null handling for filter parameters. Key modifications include the introduction of a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (30)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/useQueryParams.ts (2)
6-8: Consider adding explicit return type for better type safety.Adding an explicit return type would improve type safety and documentation.
-export const useQueryParams = () => { +export const useQueryParams = (): { + getParam: (key: string) => string | null; + setParams: (params: Record<string, string | null>) => void; +} => {
10-13: Consider adding parameter validation.The
getParamfunction should validate the input key to prevent potential issues with invalid parameter names.const getParam = useCallback( - (key: string) => urlParams.get(key), + (key: string) => { + if (!key || typeof key !== 'string') { + console.warn('Invalid parameter key provided to getParam'); + return null; + } + return urlParams.get(key); + }, [urlParams], );apps/webservice/src/app/[workspaceSlug]/(app)/_components/useFilter.ts (3)
9-18: Consider enhancing error handling and validation.While the current implementation handles errors gracefully, consider these improvements:
- Add type validation for the parsed data
- Log parsing errors in development environment
- Use specific error handling for LZString vs JSON parse errors
const filter = useMemo<T | null>(() => { if (filterHash == null) return null; try { - return JSON.parse(LZString.decompressFromEncodedURIComponent(filterHash)); + const decompressed = LZString.decompressFromEncodedURIComponent(filterHash); + if (!decompressed) return null; + const parsed = JSON.parse(decompressed); + // Basic validation that parsed data is an object + if (typeof parsed !== 'object' || parsed === null) return null; + return parsed as T; } catch (error) { + if (process.env.NODE_ENV === 'development') { + console.error('Filter parsing error:', error); + } return null; } }, [filterHash]);
19-32: Consider optimizing the setFilter implementation.The current implementation is functional but could be improved:
- Consider early return for null filter case
- Add error logging in development
const setFilter = useCallback( (filter: T | null) => { + if (filter === null) { + setParams({ filter: null }); + return; + } try { - const filterJsonHash = - filter != null - ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) - : null; + const filterJsonHash = LZString.compressToEncodedURIComponent( + JSON.stringify(filter) + ); setParams({ filter: filterJsonHash }); } catch (error) { + if (process.env.NODE_ENV === 'development') { + console.error('Filter compression error:', error); + } setParams({ filter: null }); } }, [setParams], );
34-35: Consider adding explicit return type.The hook's return type could be more explicit for better type inference and documentation.
-export const useFilter = <T extends object>() => { +export const useFilter = <T extends object>(): { + filter: T | null; + setFilter: (filter: T | null) => void; +} => {apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/useTargetFilter.ts (3)
5-8: Good architectural decision usinguseQueryParams!The switch to a custom
useQueryParamshook provides better abstraction and centralized URL parameter management.
11-18: Consider adding runtime type validation for parsed filter data.While the error handling is good, the JSON.parse result should be validated to ensure it matches the
ResourceConditiontype structure.Consider adding type validation:
const filter = useMemo<ResourceCondition | null>(() => { if (filterHash == null) return null; try { - return JSON.parse(LZString.decompressFromEncodedURIComponent(filterHash)); + const parsed = JSON.parse(LZString.decompressFromEncodedURIComponent(filterHash)); + // Add validation to ensure parsed data matches ResourceCondition structure + if (!isValidResourceCondition(parsed)) { + console.warn('Invalid filter structure detected'); + return null; + } + return parsed; } catch { return null; } }, [filterHash]);
23-34: Consider optimizing the filter update logic.The implementation is good but could be improved for better performance and readability.
Consider these optimizations:
const setFilter = useCallback( (filter: ResourceCondition | null, viewId?: string | null) => { const filterJsonHash = - filter != null - ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) - : null; + filter && LZString.compressToEncodedURIComponent(JSON.stringify(filter)); - if (viewId === undefined) { - setParams({ filter: filterJsonHash }); - return; - } - setParams({ filter: filterJsonHash, view: viewId }); + setParams({ + filter: filterJsonHash, + ...(viewId !== undefined && { view: viewId }) + }); }, [setParams], );apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx (1)
33-36: Consider explicit error handling for invalid channel IDsThe release channel fetching logic is well-structured and handles the optional case correctly. However, it might be worth adding explicit error handling for cases where the channel ID exists but is invalid.
Consider this enhancement:
const releaseChannel = searchParams["release-channel-id"] ? await api.deployment.releaseChannel.byId( searchParams["release-channel-id"], ).catch(() => null) : null;apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/useReleaseFilter.ts (3)
11-18: Consider extracting compression/decompression logicThe filter parsing logic is robust, but the compression/decompression operations could be extracted into a utility function for reusability and better testing.
Consider applying this refactor:
+const parseCompressedFilter = <T>(hash: string | null): T | null => { + if (hash == null) return null; + try { + return JSON.parse(LZString.decompressFromEncodedURIComponent(hash)); + } catch { + return null; + } +}; const filter = useMemo<ReleaseCondition | null>(() => { - if (filterHash == null) return null; - try { - return JSON.parse(LZString.decompressFromEncodedURIComponent(filterHash)); - } catch { - return null; - } + return parseCompressedFilter<ReleaseCondition>(filterHash); }, [filterHash]);
20-20: Add type annotation for releaseChannelIdThe type of
releaseChannelIdshould be explicitly declared for better type safety.-const releaseChannelId = getParam("release-channel-id"); +const releaseChannelId: string | null = getParam("release-channel-id");
23-37: LGTM! Well-structured filter setting logicThe implementation handles both filter and release channel updates efficiently. The null handling is consistent, and the types are well-defined. The use of
useCallbackis appropriate for performance optimization.Consider extracting the compression logic to match the earlier suggestion:
+const compressFilter = (filter: ReleaseCondition | null): string | null => { + return filter != null + ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) + : null; +}; const setFilter = useCallback( (filter: ReleaseCondition | null, releaseChannelId?: string | null) => { - const filterJsonHash = - filter != null - ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) - : null; + const filterJsonHash = compressFilter(filter); if (releaseChannelId === undefined) { setParams({ filter: filterJsonHash }); return; } setParams({ filter: filterJsonHash, "release-channel-id": releaseChannelId, }); }, [setParams], );apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobConditionDialog.tsx (2)
79-83: Consider clearing error state when handling empty conditionsThe null handling logic is good, but consider clearing the error state when handling empty conditions to ensure consistent state management.
if (isEmptyCondition(localCondition)) { + setError(null); onChange(null); return; }
79-83: Add documentation for empty condition handlingConsider adding a comment explaining the empty condition handling logic for better maintainability.
+ // Convert empty conditions to null to maintain consistent filter state if (isEmptyCondition(localCondition)) { onChange(null); return; } onChange(localCondition);apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (2)
25-25: LGTM! Good architectural improvementThe replacement of specialized hooks with a generic
useQueryParamshook improves code maintainability and reduces duplication.
47-48: Consider using a more formal comment styleWhile the comment is helpful, consider rephrasing it in a more formal style:
-// setting params one at a time does not work and only makes one change -// so we directly set all params to null at once +// Setting parameters individually can lead to race conditions. +// Update all parameters atomically to ensure consistency.apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetViewActionsDropdown.tsx (1)
85-86: Consider memoizing the setView functionWhile the implementation is correct, consider memoizing the
setViewfunction to prevent unnecessary recreations on re-renders.- const setView = (v: schema.ResourceView) => setFilter(v.filter, v.id); + const setView = React.useCallback( + (v: schema.ResourceView) => setFilter(v.filter, v.id), + [setFilter] + );apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx (1)
50-50: Consider using strict equality for filter badge rendering.While the changes look good, there's a minor improvement opportunity in the filter badge rendering condition.
Consider this change for more explicit null checking:
- {filter != null && <JobConditionBadge condition={filter} />} + {filter !== null && <JobConditionBadge condition={filter} />}The strict equality operator (
!==) is preferred in TypeScript/React codebases for null checks as it's more explicit and prevents potential issues with loose equality.Also applies to: 95-95
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetConditionDialog.tsx (2)
Line range hint
193-195: Fix incorrect dialog title in EditTargetViewDialogThe dialog title says "Create Target View" in the EditTargetViewDialog component, which should be "Edit Target View" instead.
Apply this fix:
<DialogHeader> - <DialogTitle>Create Target View</DialogTitle> + <DialogTitle>Edit Target View</DialogTitle> <DialogDescription>
Line range hint
97-199: Consider refactoring Create and Edit dialogsThe
CreateTargetViewDialogandEditTargetViewDialogcomponents share significant code duplication in their structure and form handling. Consider extracting the common logic into a base component or custom hook.Example approach:
// Create a shared hook for form handling function useTargetViewForm({ initialValues, onSubmit, onClose, }: { initialValues: TargetViewFormSchema; onSubmit: (view: schema.ResourceView) => void; onClose?: () => void; }) { const form = useForm({ schema: targetViewFormSchema, defaultValues: initialValues, }); const router = useRouter(); const handleSubmit = (data: TargetViewFormSchema) => { // ... shared submit logic }; return { form, handleSubmit }; }apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Overview.tsx (1)
112-112: Consider adding loading state handlingWhile the simplified filter setting logic is good, consider protecting against potential race conditions during rapid updates by adding loading state handling.
- .then(() => paramFilter != null && setFilter(releaseFilter ?? null)) + .then(() => { + if (!updateReleaseChannel.isPending && paramFilter != null) { + setFilter(releaseFilter ?? null); + } + })apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/ReleaseConditionDialog.tsx (3)
62-67: Consider extracting state management logic into a custom hookThe local state management for condition and channel ID could be simplified by extracting it into a custom hook. This would improve reusability and reduce the component's complexity.
Example implementation:
function useReleaseConditionState( initialCondition: ReleaseCondition | null, initialChannelId: string | null ) { const [localCondition, setLocalCondition] = useState<ReleaseCondition | null>( initialCondition ?? defaultCondition ); const [localChannelId, setLocalChannelId] = useState<string | null>( initialChannelId ); return { localCondition, setLocalCondition, localChannelId, setLocalChannelId, filter: localCondition ?? undefined }; }Also applies to: 70-70
112-119: Simplify the Select callback with optional chainingThe onValueChange callback could be more concise using optional chaining and early returns.
-value={localReleaseChannelId ?? undefined} -onValueChange={(value) => { - const releaseChannel = releaseChannels.find( - (rc) => rc.id === value, - ); - if (releaseChannel == null) return; - setLocalReleaseChannelId(value); - setLocalCondition(releaseChannel.releaseFilter); -}} +value={localReleaseChannelId ?? undefined} +onValueChange={(value) => { + const releaseChannel = releaseChannels.find(rc => rc.id === value); + setLocalReleaseChannelId(value); + setLocalCondition(releaseChannel?.releaseFilter ?? null); +}}
196-204: Simplify the save handler logicThe save handler's nested conditions could be more readable with early returns and clearer condition grouping.
-if ( - localCondition != null && - isEmptyCondition(localCondition) -) { - onChange(null, null); - return; -} -onChange(localCondition, null); +if (!localCondition || isEmptyCondition(localCondition)) { + onChange(null, null); + return; +} +onChange(localCondition, null);apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/layout.tsx (1)
Line range hint
41-52: Consider removing or updating the unrelated album comment.The comment about Drake's album "Views" appears to be a playful note but doesn't provide meaningful documentation about the code. Consider replacing it with technical documentation about the views functionality in your application.
- /** - * Views is a 2016 album by Drake that showcases his blend of rap and R&B - * with influences from his hometown, Toronto. It features hit singles like - * "Hotline Bling" and "One Dance," reflecting Drake's introspective lyrics - * on fame, relationships, and his rise to the top. The album explores - * themes of loyalty and love, all while solidifying his position as one of - * the most influential artists in contemporary hip-hop. - * - * This is NOT a reference to that album. - */ + /** + * Fetches the list of target views configured for the workspace. + * These views represent saved filter configurations that users can + * quickly apply to their target list. + */apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/TargetPageContent.tsx (1)
120-124: Simplify the filter change handlerWhile the logic is correct, it can be more concise by combining the conditions.
-const onFilterChange = (condition: ResourceCondition | null) => { - const cond = condition ?? defaultCondition; - if (isEmptyCondition(cond)) setFilter(null); - if (!isEmptyCondition(cond)) setFilter(cond); -}; +const onFilterChange = (condition: ResourceCondition | null) => { + const cond = condition ?? defaultCondition; + setFilter(isEmptyCondition(cond) ? null : cond); +};apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/variables/ConfigFields.tsx (1)
Line range hint
278-282: Simplify the filter update logic.The current implementation can be made more concise and type-safe while maintaining the same functionality.
Consider this refactoring:
- const onFilterChange = (condition: ResourceCondition | null) => { - const cond = condition ?? defaultCondition; - if (isEmptyCondition(cond)) updateConfig({ ...config, filter: undefined }); - if (!isEmptyCondition(cond)) updateConfig({ ...config, filter: cond }); - }; + const onFilterChange = (condition: ResourceCondition | null) => { + const cond = condition ?? defaultCondition; + updateConfig({ + ...config, + filter: isEmptyCondition(cond) ? undefined : cond + }); + };apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
Line range hint
41-41: Consider adding type safety for the filter prop in ReleaseConditionDialog.While the filter parameter handling in the query is improved, consider enhancing type safety for the
conditionprop passed toReleaseConditionDialog. This would ensure consistent null handling throughout the component.- condition={filter} + condition={filter ?? null}Also applies to: 77-82
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2)
37-37: Consider using path aliases to improve maintainabilityThe import path
"../../../../../../_components/useFilter"is quite deep and could be fragile during refactoring. Consider using path aliases (e.g.,@/components/useFilter) to make imports more maintainable.
415-418: Consider optimizing the polling interval configurationThe
refetchInterval: 5_000is hardcoded. Consider:
- Making this configurable through environment variables or constants
- Implementing exponential backoff for failed requests
- Using WebSocket for real-time updates if available
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/layout.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/TargetPageContent.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobConditionDialog.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/useJobFilter.ts(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Overview.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/ReleaseChannelDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/ReleaseConditionDialog.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/useReleaseFilter.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetConditionDialog.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetViewActionsDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/useTargetFilter.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/useFilter.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/useQueryParams.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/variables/ConfigFields.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/useJobFilter.ts
🧰 Additional context used
📓 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
🔇 Additional comments (36)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/useQueryParams.ts (3)
1-4: LGTM! Imports and client directive are correctly configured.
29-30: LGTM! Hook return value follows React patterns.
1-30: Verify the migration from useJobFilter to useQueryParams.
Since this hook replaces useJobFilter, we should verify that all instances have been properly migrated.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence that:
- There are no remaining instances of
useJobFilterin the codebase - The new
useQueryParamshook is properly imported and used across multiple components
Migration from useJobFilter to useQueryParams is complete
The codebase shows no remaining instances of useJobFilter, and useQueryParams is now properly integrated in multiple components including release channel, target condition, and release condition filters. The migration appears to be successful.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of useJobFilter that might need migration
echo "Checking for remaining useJobFilter usage:"
rg "useJobFilter"
# Search for new useQueryParams usage to verify migration
echo -e "\nVerifying useQueryParams implementation:"
rg "useQueryParams"
Length of output: 1486
apps/webservice/src/app/[workspaceSlug]/(app)/_components/useFilter.ts (3)
6-8: LGTM! Clean hook declaration with proper generic type constraints.
The generic type parameter extending object ensures type safety while maintaining flexibility.
1-35: Verify complete migration from useJobFilter.
Based on the AI summary, this hook replaces useJobFilter. Let's verify the migration is complete.
✅ Verification successful
Migration from useJobFilter is complete
The verification shows that useJobFilter has been completely removed from the codebase, and the new useFilter hook is being properly used in relevant components including the job-related views (JobTable.tsx and TargetReleaseTable.tsx). The migration appears to be successful.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining usage of useJobFilter
rg "useJobFilter" --type ts --type tsx
# Check for components mentioned in the summary using the new hook
rg "useFilter.*JobCondition|ResourceCondition" --type ts --type tsx
Length of output: 164
Script:
#!/bin/bash
# Check for any remaining usage of useJobFilter without file type flags
rg "useJobFilter"
# Check for components using the new hook
rg "useFilter"
# Find all TypeScript/TSX files that might be using these hooks
fd "\.(ts|tsx)$" | grep -i "filter"
Length of output: 2612
1-4: Consider pinning the LZString package version.
To ensure consistent behavior across installations, consider specifying the LZString package version in package.json.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/useTargetFilter.ts (3)
20-20: LGTM!
Clean and straightforward view ID extraction.
37-37: LGTM!
Clean and focused hook interface providing only the necessary values.
1-37: Verify null handling in components using this hook.
Since the filter type has changed from undefined to null, we should verify that all components using this hook handle the null case correctly.
✅ Verification successful
Let me gather more context about the usage patterns by checking the full component code.
After analyzing the code, I can now generate the final response:
Null handling is consistent and properly implemented
The component handles the null case correctly in all scenarios:
- Query parameter:
filter: filter ?? undefinedproperly converts null to undefined for the API call - Filter display:
filter != nullchecks are used consistently for conditional rendering - Filter manipulation:
onFilterChangeproperly handles both null and non-null cases - Search functionality: Correctly handles existing filter conditions when filter is null
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using useTargetFilter to verify null handling
rg -l "useTargetFilter" | xargs rg "filter\s*[?]{2}|filter\s*!=\s*null|filter\s*!==\s*null"
Length of output: 614
Script:
#!/bin/bash
# Get the full content of TargetPageContent.tsx to understand the usage pattern
cat "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/TargetPageContent.tsx"
Length of output: 7626
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx (1)
11-11: Verify handling of existing URLs with old parameter name
The change from release-channel to release-channel-id in the URL parameters is a breaking change that could affect existing bookmarked URLs or external links.
Let's check for any transition handling or other usages of the old parameter name:
✅ Verification successful
Based on the search results, I can now generate the final response:
Parameter rename appears to be part of a coordinated update
The change from release-channel to release-channel-id appears to be consistent with the codebase:
- The new parameter name
release-channel-idis already being used in URL construction (found in Overview.tsx) - No instances of the old parameter name being used in URL parameters or search params were found
- This change appears to be part of a broader cleanup effort mentioned in the PR title "fix: Clean up filter hooks"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old parameter name
rg -l "release-channel[^-]" --type ts --type tsx
# Search for any URL transition or compatibility handling
rg -l "searchParams.*release-channel" --type ts --type tsx
Length of output: 167
Script:
#!/bin/bash
# Search for any remaining usage of the old parameter name, using ripgrep without file type filters
rg -l "release-channel[^-]"
# Search for any URL transition or compatibility handling
rg -l "searchParams.*release-channel"
# Search for any usage of either parameter name in the codebase
rg -l "release-channel"
Length of output: 2810
Script:
#!/bin/bash
# Let's check the actual content of the releases page to understand the parameter usage
rg -A 5 -B 5 "searchParams.*release-channel" apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx
# Check if there are any URL constructions or links using this parameter
rg "release-channel.*=.*" apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]
# Check for any router or navigation related code using this parameter
rg "router.*release-channel|useSearchParams.*release-channel" apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]
Length of output: 341
Script:
#!/bin/bash
# Escape the special characters in the path
# Let's check the actual content of the releases page
rg -A 5 -B 5 "searchParams.*release-channel" "apps/webservice/src/app/\[workspaceSlug\]/\(app\)/systems/\[systemSlug\]/deployments/\[deploymentSlug\]/releases/page.tsx"
# Check for any URL constructions or links using this parameter in the deployment directory
find apps/webservice/src/app -type f -name "*.tsx" -o -name "*.ts" | xargs rg "release-channel.*=.*"
# Check for any router or navigation related code using this parameter
find apps/webservice/src/app -type f -name "*.tsx" -o -name "*.ts" | xargs rg "useSearchParams.*release-channel"
Length of output: 910
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/useReleaseFilter.ts (1)
5-5: Verify the implementation of useQueryParams hook
The switch to a custom useQueryParams hook is a good abstraction. However, we should verify its implementation to ensure it handles all edge cases properly.
✅ Verification successful
Based on the implementation we saw earlier and the usage patterns across multiple components, I can now generate the final response:
The useQueryParams hook implementation is sound and well-utilized
The custom hook provides a clean abstraction over URL parameter operations and is consistently used across several components including filters, release channels, and target conditions. The implementation:
- Properly manages URL parameters using Next.js's native hooks (useSearchParams, useRouter)
- Handles parameter updates safely with null checks
- Uses proper dependency arrays in useCallback hooks
- Provides a consistent interface (getParam/setParams) used consistently across all consuming components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of useQueryParams hook
# Expected: The hook should handle all URL parameter operations safely
# Look for the useQueryParams implementation
ast-grep --pattern 'export const useQueryParams = () => {
$$$
}'
# Check for any error handling patterns in the implementation
rg -A 5 "try.*catch" "../useQueryParams.ts"
Length of output: 2770
Script:
#!/bin/bash
# Let's verify the imports and dependencies of useQueryParams hook
rg -A 2 "^import|^export" apps/webservice/src/app/[workspaceSlug]/(app)/_components/useQueryParams.ts
# Check if there are any other files using this hook to understand its usage pattern
rg "useQueryParams" --type ts
Length of output: 284
Script:
#!/bin/bash
# Let's verify the imports and dependencies of useQueryParams hook by escaping the brackets
rg -A 2 "^import|^export" "apps/webservice/src/app/\[workspaceSlug\]/(app)/_components/useQueryParams.ts"
# Check if there are any other files using this hook to understand its usage pattern
# Using a simpler pattern that doesn't include the path
rg "useQueryParams" --type ts
Length of output: 1512
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobConditionDialog.tsx (2)
17-17: LGTM: Import addition aligns with new null handling strategy
The addition of isEmptyCondition import is well-placed and supports the new explicit null handling approach.
24-25: Verify all parent components are updated for breaking changes
The prop type changes from optional to nullable are breaking changes that require updates in all parent components using this dialog.
✅ Verification successful
Based on the search results, I can now generate the final response as all necessary information has been gathered. The script revealed all usages of JobConditionDialog, showing that parent components are already handling the nullable condition correctly:
All parent components are properly handling nullable condition prop
The search results show that both parent components are already using the nullable pattern:
TargetReleaseTable.tsx:condition={filter ?? null}JobTable.tsx:condition={filter ?? null}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all components using JobConditionDialog to verify they handle the new prop types correctly
# Search for JobConditionDialog usage
rg -l "JobConditionDialog" | while read -r file; do
echo "=== $file ==="
# Show the context of how JobConditionDialog is used
rg -A 5 -B 5 "JobConditionDialog" "$file"
done
Length of output: 4666
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (2)
39-39: LGTM! Clean state management
The use of useQueryParams hook provides a centralized way to manage URL parameters.
49-53: Verify the necessity of duplicate parameter names
The code sets both kebab-case (release-channel-id) and snake_case (release_channel_id) parameters to null. This might indicate inconsistent parameter naming across the codebase.
✅ Verification successful
Both parameter names are used in different parts of the codebase
The search results reveal that both naming conventions serve different purposes:
release-channel-idis used in frontend URL parameters and search paramsrelease_channel_idis used in database-related code and some internal state management
The duplicate parameter names in ReleaseChannelDropdown.tsx are intentional to maintain compatibility with both the frontend routing and backend database conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent parameter naming patterns
# Search for both naming patterns in the codebase
echo "Searching for kebab-case usage:"
rg -A 2 "release-channel-id"
echo -e "\nSearching for snake_case usage:"
rg -A 2 "release_channel_id"
# Search for any parameter setting patterns
echo -e "\nSearching for setParams usage patterns:"
ast-grep --pattern 'setParams({$$$})'
Length of output: 22382
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetViewActionsDropdown.tsx (2)
37-37: LGTM: Clean filter state management after view deletion
The changes properly handle the cleanup of filter state after view deletion. Setting both filter and view ID to null ensures a clean state reset.
Also applies to: 66-66
37-37: Verify consistent usage of setFilter across components
The changes align well with the filter hooks cleanup objective. Let's verify that this pattern is consistently applied across other components.
Also applies to: 85-86
✅ Verification successful
Filter hook usage is consistent across the codebase
The verification shows that the setFilter pattern from useTargetFilter is consistently implemented:
- Used correctly in
TargetViewActionsDropdown.tsxandTargetPageContent.tsx - Similar patterns are followed in related components using
useReleaseFilteranduseFilter - No legacy view management code remains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of removeView or direct view state management
rg -A 2 'removeView|setView(?!.*setFilter)' --type typescript
# Verify the adoption of setFilter pattern
rg -A 2 'setFilter.*filter.*id' --type typescript
Length of output: 405
Script:
#!/bin/bash
# Search for any remaining uses of removeView or direct view state management
rg -A 2 'removeView|setView' --type ts
# Search for setFilter usage patterns
rg -A 2 'setFilter' --type ts
# Look for filter hook implementations
ast-grep --pattern 'const $_ = useTargetFilter()'
Length of output: 13862
apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx (3)
3-3: LGTM! Import changes align with the new filter implementation.
The new imports support the transition to a more type-safe filter implementation.
Also applies to: 26-26
42-42: LGTM! Improved null handling in query parameters.
The filter ?? undefined pattern ensures proper handling of null values in the API query, preventing potential issues with null being sent as a query parameter.
34-34: LGTM! Verify the generic hook implementation.
The transition to a generic useFilter hook with proper type parameters improves code reusability while maintaining type safety.
✅ Verification successful
Let me gather more information about the useFilter hook implementation.
Let me search for the JobCondition type and check if there are any other files that were using the previous useJobFilter.
Based on the gathered information, I can now provide the final response:
✅ Generic hook implementation verified and properly typed
The transition from useJobFilter to useFilter<JobCondition> is correctly implemented:
- The generic
useFilterhook is properly implemented in_components/useFilter.tswith type safety JobConditiontype is well-defined in the validators package- The hook is consistently used across the codebase with proper typing
- No remaining references to the old
useJobFilterwere found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useFilter hook implementation and its usage across the codebase
# Check the implementation of useFilter
ast-grep --pattern 'export function useFilter<$_>() {
$$$
}'
# Check other usages of useFilter to ensure consistent typing
rg -A 2 'useFilter<'
Length of output: 1083
Script:
#!/bin/bash
# Search for the useFilter hook implementation
rg -g '*.{ts,tsx}' -A 10 'export .*useFilter'
# Search for JobCondition type definition
ast-grep --pattern 'type JobCondition = {
$$$
}'
# Find any other filter-related hooks that might have been replaced
rg -g '*.{ts,tsx}' 'use.*Filter'
Length of output: 6652
Script:
#!/bin/bash
# Search for JobCondition type definition
rg -g '*.{ts,tsx}' -B 2 -A 5 'type JobCondition'
# Search for previous useJobFilter usage
rg -g '*.{ts,tsx}' 'useJobFilter'
# Check git history for the specific file to see the previous implementation
git log -p apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx | grep -A 10 'useJobFilter'
Length of output: 2702
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-condition/TargetConditionDialog.tsx (2)
Line range hint 42-43: LGTM: Proper null handling implementation
The implementation correctly handles null values using the nullish coalescing operator and maintains proper state initialization.
29-30: Breaking change: Verify all component usage sites
The prop types have been changed from optional to required nullable, which is a breaking change. While this improves type safety by forcing explicit null handling, it requires updates to all usage sites.
Let's verify all usage sites of this component:
✅ Verification successful
Props change is safe: All usage sites handle null correctly
The verification shows that all usage sites of TargetConditionDialog already handle the null case correctly:
- In
ConfigFields.tsx: Uses nullish coalescingconfig.filter ?? defaultCondition - In
TargetPageContent.tsx: Directly passesfilterwhich is already nullable
The change from optional to required nullable props is non-breaking as existing code already handles null values appropriately. No updates are needed at the usage sites.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all instances where TargetConditionDialog is used
ast-grep --pattern 'const $_ = <TargetConditionDialog $$$>'
Length of output: 61
Script:
#!/bin/bash
# Search for TargetConditionDialog usage with a more flexible pattern
rg -l "TargetConditionDialog" --type tsx
# For each file found, show the content around TargetConditionDialog usage
rg -A 5 -B 5 "<TargetConditionDialog" --type tsx
Length of output: 147
Script:
#!/bin/bash
# Search for TargetConditionDialog usage without file type restriction
rg -l "TargetConditionDialog"
# For each file found, show the content around TargetConditionDialog usage
rg -A 5 -B 5 "<TargetConditionDialog"
Length of output: 4894
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Overview.tsx (2)
49-49: LGTM: Clean URL parameter handling
The addition of the release channel ID to the URL parameters is well implemented, maintaining proper parameter validation and URL encoding.
Also applies to: 60-60
127-127: LGTM: Proper parameter passing
The addition of releaseChannel.id as a parameter is correctly implemented and maintains type safety.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-condition/ReleaseConditionDialog.tsx (2)
29-29: LGTM: Improved type safety with explicit null handling
The changes to prop types and callback signatures show a consistent approach to handling optional values using null instead of undefined. This makes the component's contract clearer and more predictable.
Also applies to: 40-40, 43-45
72-72: LGTM: Safe query parameter handling
The null coalescing operator ensures type-safe conversion of the filter parameter for the API query.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/layout.tsx (1)
174-177: LGTM! The filter prop addition aligns with the PR objectives.
The addition of the filter={null} prop to CreateTargetViewDialog is consistent with the broader changes to enhance type safety and null handling for filter parameters across the application.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/TargetPageContent.tsx (4)
83-83: LGTM: Simplified filter hook usage
The removal of setView and consolidation to just filter and setFilter improves the clarity of state management.
176-176: LGTM: Improved view submission handling
The change to use setFilter with both filter and view ID parameters properly consolidates the state management.
210-210: LGTM: Consistent filter clearing
The change to use null when clearing filters maintains consistency with the new filter state management approach.
116-116: Verify API handling of undefined filter parameter
The use of filter ?? undefined ensures type safety and consistent API behavior. However, let's verify that the API endpoint properly handles undefined filters.
✅ Verification successful
These commands will help us locate the API endpoint implementation and understand how filters are handled. Let's analyze the results before making a conclusion.
These commands will help us locate the API endpoint implementation and understand how the filter parameter is validated. Let's analyze the results to verify the API's handling of undefined filters.
Based on the search results, I can now generate the final response as we have sufficient evidence about the API's filter handling:
API properly handles undefined filter parameter
The codebase shows consistent handling of optional filters across multiple API endpoints. The filter parameters are properly validated using Zod schemas with .optional() modifier in the API routers:
packages/api/src/router/resources.ts: Resource endpointspackages/api/src/router/job.ts: Job endpointspackages/api/src/router/release.ts: Release endpoints
The use of filter ?? undefined in the component is consistent with the API's validation, ensuring type safety and proper handling of undefined values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the API endpoint implementation to verify filter handling
ast-grep --pattern 'export async function list($_, $_) {
$$$
filter
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# Let's try to find API endpoint files and filter-related code
rg -l "filter.*workspaceId" --type ts
Length of output: 39
Script:
#!/bin/bash
# Search for target-related API endpoints
rg -l "targets.*api" --type ts
Length of output: 968
Script:
#!/bin/bash
# Search for filter parameter usage in target-related files
rg "filter\s*[?:]" --type ts -A 3
Length of output: 32625
Script:
#!/bin/bash
# Let's search for the API endpoint implementation in the packages directory
rg -A 5 "export.*list.*workspaceId.*filter" --type ts packages/
Length of output: 65
Script:
#!/bin/bash
# Search for filter type definitions and validations
rg -A 3 "filter.*optional\(\)" --type ts packages/
Length of output: 1630
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/variables/ConfigFields.tsx (2)
Line range hint 285-291: LGTM! Proper handling of null and undefined cases.
The component correctly:
- Uses the nullish coalescing operator to handle null values
- Passes the filter condition to child components
- Maintains type safety with the new
ResourceCondition | nulltype
Line range hint 278-291: Verify consistent null handling across filter components.
Let's ensure this change aligns with the filter handling patterns in other components.
✅ Verification successful
Let me gather more information about the filter handling patterns, particularly focusing on the implementation details.
Based on the gathered information, I can now provide the final response:
Filter handling is consistent with codebase patterns
The implementation aligns with the established patterns across the codebase:
isEmptyConditionis consistently used to determine null/undefined filter states- Other components like
TargetPageContentand release-related components follow the same pattern of setting filter to null/undefined for empty conditions - The handling in
ConfigFields.tsxmatches the pattern where empty conditions result inundefinedfilter values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using TargetConditionDialog to verify consistent null handling
ast-grep --pattern 'condition={$_} onChange={$_}' | grep -B 2 -A 2 'TargetConditionDialog'
# Search for similar filter change patterns
rg -A 3 'filter: ResourceCondition \| null'
Length of output: 1232
Script:
#!/bin/bash
# Search for isEmptyCondition usage patterns
rg -A 3 'isEmptyCondition'
# Search for filter-related state updates
ast-grep --pattern 'updateConfig({ $$$, filter: $_$$ })'
# Look for TargetConditionDialog implementations
rg -A 5 'export.*TargetConditionDialog'
Length of output: 9891
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
54-54: LGTM! Consistent null handling for the filter parameter.
The change improves type safety by explicitly converting null to undefined for the API query parameter, which aligns with the PR objective of cleaning up filter hooks.
Let's verify that this pattern is consistently applied across other components:
✅ Verification successful
LGTM! The filter null handling pattern is consistently applied across the codebase
The search results confirm that the filter ?? undefined and filter ?? null patterns are consistently used across multiple components in similar contexts:
- JobTable.tsx
- TargetPageContent.tsx
- TargetReleaseTable.tsx
- ConfigFields.tsx
The change in DeploymentPageContent.tsx follows the established pattern in the codebase for handling nullable filter parameters in query hooks and dialog components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar filter parameter usage patterns in query hooks
rg -A 2 'useQuery.*filter:' --type typescript
Length of output: 82
Script:
#!/bin/bash
# Search for similar filter parameter usage patterns in query hooks
# Using more generic pattern without file type restriction
rg -A 2 'useQuery.*filter:'
# Also search for specific filter handling patterns
rg -A 2 'filter\s*\?\?'
Length of output: 3552
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2)
Line range hint 434-441: LGTM! Consistent null handling for dialog condition
The explicit null handling with filter ?? null aligns well with the component's prop types and maintains consistency in null value handling throughout the application.
414-417: LGTM! Verify the filter hook usage across the codebase
The migration from useJobFilter to useFilter<JobCondition> looks good and improves type safety. The null coalescing operator ensures proper handling of undefined values.
Let's verify the consistent usage of this hook across the codebase:
✅ Verification successful
Migration from useJobFilter to useFilter is consistent
The verification shows that:
- No instances of
useJobFilterremain in the codebase useFilter<JobCondition>is used consistently in both locations where job filtering is needed:TargetReleaseTable.tsxJobTable.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining useJobFilter usage and verify consistent useFilter typing
# Check for any remaining useJobFilter usage
echo "Checking for remaining useJobFilter usage:"
rg "useJobFilter"
# Check for useFilter usage patterns
echo "Checking useFilter usage patterns:"
rg "useFilter<.*?>"
Length of output: 541
| const setParams = useCallback( | ||
| (params: Record<string, string | null>) => { | ||
| const query = new URLSearchParams(window.location.search); | ||
|
|
||
| Object.entries(params).forEach(([key, value]) => { | ||
| if (value == null) query.delete(key); | ||
| if (value != null) query.set(key, value); | ||
| }); | ||
|
|
||
| router.replace(`?${query.toString()}`); | ||
| }, | ||
| [router], | ||
| ); |
There was a problem hiding this comment.
Fix logical flaw in parameter handling and add validation.
There are several issues in the current implementation:
- The conditions for handling null values and setting values are not mutually exclusive.
- Direct usage of
window.locationmight cause issues in SSR or testing environments. - Missing validation for parameter values.
const setParams = useCallback(
(params: Record<string, string | null>) => {
- const query = new URLSearchParams(window.location.search);
+ const query = new URLSearchParams(urlParams.toString());
Object.entries(params).forEach(([key, value]) => {
+ if (!key) {
+ console.warn('Invalid parameter key provided to setParams');
+ return;
+ }
+
if (value == null) query.delete(key);
- if (value != null) query.set(key, value);
+ else query.set(key, value);
});
router.replace(`?${query.toString()}`);
},
- [router],
+ [router, urlParams],
);Committable suggestion skipped: line range outside the PR's diff.
| .then(() => | ||
| utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id), | ||
| ) | ||
| .then(() => { | ||
| if (paramFilter != null) setFilter(releaseFilter); | ||
| }) | ||
| .then(() => paramFilter != null && setFilter(releaseFilter ?? null)) | ||
| .then(() => router.refresh()); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the API mutation
The mutation chain should include error handling to provide feedback to users when updates fail.
const onSubmit = form.handleSubmit((data) => {
const releaseFilter = getFinalFilter(data.releaseFilter);
updateReleaseChannel
.mutateAsync({ id: releaseChannel.id, data: { ...data, releaseFilter } })
.then(() => form.reset({ ...data, releaseFilter }))
.then(() =>
utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id),
)
.then(() => paramFilter != null && setFilter(releaseFilter ?? null))
- .then(() => router.refresh());
+ .then(() => router.refresh())
+ .catch((error) => {
+ console.error('Failed to update release channel:', error);
+ // Consider adding a toast notification or other user feedback here
+ });
});Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
useFilterfor managing filter parameters.useQueryParamsfor handling URL query parameters.Improvements
TargetViewActionsDropdownandReleaseConditionDialog.Bug Fixes
Refactor
useJobFilteranduseReleaseFilterhooks with more generic implementations.