fix: Select release channel filter on deployment page #214
Conversation
WalkthroughThis pull request introduces several enhancements across multiple components related to release channels and filters. Key changes include the integration of a new hook for managing release filters, modifications to existing components to utilize this hook, and the addition of a tabbed interface in the Changes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Outside diff range and nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx (1)
33-35: Consider adding error handling for release channel fetchingThe current implementation might fail silently if an invalid release channel ID is provided. Consider adding error handling and user feedback.
- const releaseChannel = searchParams["release-channel"] - ? await api.deployment.releaseChannel.byId(searchParams["release-channel"]) - : null; + let releaseChannel = null; + if (searchParams["release-channel"]) { + try { + releaseChannel = await api.deployment.releaseChannel.byId( + searchParams["release-channel"] + ); + } catch (error) { + console.error("Failed to fetch release channel:", error); + // Consider redirecting or showing an error message + } + }apps/webservice/src/app/[workspaceSlug]/_components/release-condition/useReleaseFilter.ts (1)
62-68: Consider preserving other URL parametersThe current implementation might unintentionally affect other URL parameters.
Consider this more robust implementation:
const removeReleaseChannel = useCallback(() => { const query = new URLSearchParams(window.location.search); query.delete("release-channel"); query.delete("filter"); router.replace(`?${query.toString()}`); - router.refresh(); + // Consider if refresh is necessary based on your use case }, [router]);apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
41-51: Consider enhancing error handling and user feedback.While the deletion logic is functionally correct, consider these improvements:
- Add error handling to the promise chain
- Show appropriate feedback during the deletion process
Consider applying this enhancement:
const onDelete = () => deleteReleaseChannel .mutateAsync(releaseChannelId) .then(() => removeReleaseChannelId()) .then(() => removeReleaseChannel()) .then(() => router.refresh()) - .then(() => setOpen(false)); + .then(() => setOpen(false)) + .catch((error) => { + console.error('Failed to delete release channel:', error); + // Show error toast/notification to user + });Also, consider showing a loading state in the Delete button while the operation is in progress:
<AlertDialogAction onClick={onDelete} disabled={deleteReleaseChannel.isPending} className={buttonVariants({ variant: "destructive" })} > - Delete + {deleteReleaseChannel.isPending ? "Deleting..." : "Delete"} </AlertDialogAction>apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (2)
111-113: Fix indentation while maintaining correct logicThe conditional filter update logic is correct, but the indentation doesn't match the surrounding promise chain.
Apply this formatting fix:
.then(() => utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id), ) - .then(() => { - if (paramFilter != null) setFilter(releaseFilter); - }) + .then(() => { + if (paramFilter != null) setFilter(releaseFilter); + }) .then(() => router.refresh());
Line range hint
103-114: Consider adding error handling to the promise chainWhile the mutation and state updates are implemented correctly, consider adding error handling to gracefully handle failures.
Consider updating the promise chain with error handling:
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(() => { if (paramFilter != null) setFilter(releaseFilter); }) - .then(() => router.refresh()); + .then(() => router.refresh()) + .catch((error) => { + console.error('Failed to update release channel:', error); + // Consider adding user feedback here + }); });packages/api/src/router/deployment.ts (2)
333-353: LGTM! Consider adding type safety for the release channels array.The left join and result transformation look correct. The
isPresentfilter ensures we don't include null values in the releaseChannels array.Consider adding a type assertion or runtime validation for the releaseChannels array to ensure type safety:
- releaseChannels: r - .map((r) => r.release_channel) - .filter(isPresent), + releaseChannels: r + .map((r) => r.release_channel) + .filter(isPresent) + .map((channel): ReleaseChannel => ({ + id: channel.id, + // ... other required fields + })),
Line range hint
380-392: LGTM! Consider enhancing the deduplication comment.The left join and deduplication logic using
uniqByis correct. The comment explains why deduplication is needed, but could be more specific.Consider updating the comment to be more specific about the data structure:
- // latest active release subquery can return multiple active releases - // and multiple release channels, which means we need to dedupe them - // since there will be a row per combination of active release and release channel + // The query returns a cartesian product where each row contains a combination of: + // 1. Deployment + // 2. Active release (one per environment) + // 3. Release channel + // We need to dedupe both active releases and release channels to avoid duplicates
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-condition/useReleaseFilter.ts(3 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx(2 hunks)packages/api/src/router/deployment.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/_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 (14)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/page.tsx (3)
11-11: LGTM: Type definition follows Next.js conventions
The searchParams type definition with optional release-channel parameter is correctly implemented.
25-28: LGTM: Function signature follows Next.js conventions
The updated page component signature correctly implements the searchParams parameter.
40-40: Verify releaseChannel prop type in DeploymentPageContent
Ensure that the releaseChannel prop type in DeploymentPageContent matches the expected schema.ReleaseChannel | null type.
✅ Verification successful
ReleaseChannel prop type in DeploymentPageContent is correct
The releaseChannel prop type matches the expected schema.ReleaseChannel | null type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DeploymentPageContent type definition
ast-grep --pattern 'type DeploymentPageContentProps = {
$$$
releaseChannel: $_
$$$
}'
Length of output: 1502
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/useReleaseFilter.ts (3)
21-24: LGTM: Clean implementation of release channel ID memoization
The memoization is implemented correctly with proper dependency tracking and null handling.
70-76: LGTM: Well-structured return object
The return object cleanly exposes all necessary functionality and state.
Line range hint 1-76: Verify usage patterns across the codebase
Since this hook manages critical URL state for release channels, let's verify its usage patterns.
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
25-25: LGTM: Import statement is correctly placed and necessary.
The import for useReleaseFilter hook is appropriately placed with other hook imports and is required for the new release channel filtering functionality.
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (2)
37-37: LGTM: Import statement is properly placed
The import of useReleaseFilter hook is correctly positioned and follows the project's import organization pattern.
90-90: LGTM: Hook usage follows React best practices
The useReleaseFilter hook is properly implemented at the component's top level with clear naming conventions.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (4)
5-5: LGTM!
The IconSettings import is properly grouped with other icon imports and is used appropriately in the new settings button UI.
32-32: LGTM!
The type definitions are well-structured and maintain type safety:
- Deployment type properly extends the base schema type with releaseChannels
- ReleaseChannel type is correctly nullable to handle cases where no channel is selected
Also applies to: 38-38
44-44: LGTM!
The releaseChannel prop is correctly destructured and matches its type definition.
71-76: LGTM!
The ReleaseConditionDialog integration is clean and properly receives the releaseChannels data.
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx (1)
80-100: Tabbed interface implementation enhances user experience.
The addition of the tabbed interface for managing release conditions and release channels is well-designed and improves the usability of the component.
| const setReleaseChannel = useCallback( | ||
| (releaseChannel: SCHEMA.ReleaseChannel) => { | ||
| const query = new URLSearchParams(window.location.search); | ||
| query.set("release-channel", releaseChannel.id); | ||
| if (releaseChannel.releaseFilter != null) { | ||
| const filterJson = LZString.compressToEncodedURIComponent( | ||
| JSON.stringify(releaseChannel.releaseFilter), | ||
| ); | ||
| query.set("filter", filterJson); | ||
| } | ||
|
|
||
| router.replace(`?${query.toString()}`); | ||
| router.refresh(); | ||
| }, | ||
| [router], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation and error handling
The implementation has several potential improvements:
- Add validation for releaseChannel.id
- Handle potential errors during JSON stringification and compression
- Consider if router.refresh() is necessary for all cases
- Preserve other URL parameters when updating
Consider this improved implementation:
const setReleaseChannel = useCallback(
(releaseChannel: SCHEMA.ReleaseChannel) => {
+ if (!releaseChannel?.id) {
+ console.error('Invalid release channel');
+ return;
+ }
const query = new URLSearchParams(window.location.search);
query.set("release-channel", releaseChannel.id);
if (releaseChannel.releaseFilter != null) {
+ try {
const filterJson = LZString.compressToEncodedURIComponent(
JSON.stringify(releaseChannel.releaseFilter),
);
query.set("filter", filterJson);
+ } catch (error) {
+ console.error('Failed to compress release filter:', error);
+ // Optionally handle the error case
+ }
}
router.replace(`?${query.toString()}`);
- router.refresh();
+ // Consider if refresh is necessary based on your use case
},
[router],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setReleaseChannel = useCallback( | |
| (releaseChannel: SCHEMA.ReleaseChannel) => { | |
| const query = new URLSearchParams(window.location.search); | |
| query.set("release-channel", releaseChannel.id); | |
| if (releaseChannel.releaseFilter != null) { | |
| const filterJson = LZString.compressToEncodedURIComponent( | |
| JSON.stringify(releaseChannel.releaseFilter), | |
| ); | |
| query.set("filter", filterJson); | |
| } | |
| router.replace(`?${query.toString()}`); | |
| router.refresh(); | |
| }, | |
| [router], | |
| ); | |
| const setReleaseChannel = useCallback( | |
| (releaseChannel: SCHEMA.ReleaseChannel) => { | |
| if (!releaseChannel?.id) { | |
| console.error('Invalid release channel'); | |
| return; | |
| } | |
| const query = new URLSearchParams(window.location.search); | |
| query.set("release-channel", releaseChannel.id); | |
| if (releaseChannel.releaseFilter != null) { | |
| try { | |
| const filterJson = LZString.compressToEncodedURIComponent( | |
| JSON.stringify(releaseChannel.releaseFilter), | |
| ); | |
| query.set("filter", filterJson); | |
| } catch (error) { | |
| console.error('Failed to compress release filter:', error); | |
| // Optionally handle the error case | |
| } | |
| } | |
| router.replace(`?${query.toString()}`); | |
| // Consider if refresh is necessary based on your use case | |
| }, | |
| [router], | |
| ); |
| const { removeReleaseChannel } = useReleaseFilter(); | ||
| const router = useRouter(); | ||
| const deleteReleaseChannel = | ||
| api.deployment.releaseChannel.delete.useMutation(); | ||
| const onDelete = () => | ||
| deleteReleaseChannel | ||
| .mutateAsync(releaseChannelId) | ||
| .then(() => removeReleaseChannelId()) | ||
| .then(() => removeReleaseChannel()) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)); |
There was a problem hiding this comment.
💡 Codebase verification
Ensure all releaseChannelId references are properly handled upon deletion.
Some instances of releaseChannelId are not associated with removal functions, which may lead to inconsistent state.
- packages/api/src/router/release.ts
- packages/api/src/router/environment.ts
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/useReleaseChannelDrawer.ts
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/useReleaseFilter.ts
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx
🔗 Analysis chain
Verify proper cleanup of release channel references.
Let's ensure that all references to the deleted release channel are properly cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any potential references to release channels that might need cleanup
# Look for other components using release channels
echo "Checking for components using release channels..."
rg -l "useReleaseChannel|releaseChannelId" --type typescript
# Look for filter-related components that might need updates
echo "Checking for filter-related components..."
rg -l "useReleaseFilter|release-channel.*filter" --type typescript
# Check for any hardcoded release channel IDs
echo "Checking for hardcoded release channel references..."
rg -l "releaseChannel.*Id.*=.*['\"]" --type typescript
Length of output: 612
Script:
#!/bin/bash
# Description: Check for any potential references to release channels that might need cleanup
# Look for other components using release channels
echo "Checking for components using release channels..."
rg "useReleaseChannel|releaseChannelId" --glob "*.ts" --glob "*.tsx"
# Look for filter-related components that might need updates
echo "Checking for filter-related components..."
rg "useReleaseFilter|release-channel.*filter" --glob "*.ts" --glob "*.tsx"
# Check for any hardcoded release channel IDs
echo "Checking for hardcoded release channel references..."
rg "releaseChannel.*Id.*=['\"]" --glob "*.ts" --glob "*.tsx"
Length of output: 9498
Script:
#!/bin/bash
# Description: Verify that all references to releaseChannelId properly handle deletion
# Check if removal functions are called wherever releaseChannelId is used
echo "Verifying removal function calls for releaseChannelId..."
rg "removeReleaseChannelId\(\)|removeReleaseChannel\(\)" --glob "*.ts" --glob "*.tsx"
# Ensure that after deletion, there are no lingering references to releaseChannelId
echo "Checking for lingering releaseChannelId references after deletion..."
rg "releaseChannelId" --glob "*.ts" --glob "*.tsx" | grep -v "removeReleaseChannelId\(\)|removeReleaseChannel\(\)"
Length of output: 5223
| {filter != null && releaseChannel == null && ( | ||
| <ReleaseConditionBadge condition={filter} /> | ||
| )} | ||
| </div> | ||
| </ReleaseConditionDialog> | ||
| {filter != null && releaseChannel != null && ( | ||
| <div className="flex items-center gap-2"> | ||
| <span>{releaseChannel.name}</span> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-6 w-6" | ||
| onClick={() => setReleaseChannelId(releaseChannel.id)} | ||
| > | ||
| <IconSettings className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and simplify conditional rendering
The conditional rendering logic could be improved for better maintainability and accessibility.
Consider these improvements:
- {filter != null && releaseChannel == null && (
- <ReleaseConditionBadge condition={filter} />
- )}
- {filter != null && releaseChannel != null && (
+ {filter != null && (
+ releaseChannel == null ? (
+ <ReleaseConditionBadge condition={filter} />
+ ) : (
<div className="flex items-center gap-2">
<span>{releaseChannel.name}</span>
<Button
variant="ghost"
size="icon"
className="h-6 w-6"
+ aria-label={`Configure ${releaseChannel.name} settings`}
onClick={() => setReleaseChannelId(releaseChannel.id)}
>
<IconSettings className="h-4 w-4" />
</Button>
</div>
+ )
)}Changes:
- Simplified nested conditions for better readability
- Added aria-label to the settings button for improved accessibility
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {filter != null && releaseChannel == null && ( | |
| <ReleaseConditionBadge condition={filter} /> | |
| )} | |
| </div> | |
| </ReleaseConditionDialog> | |
| {filter != null && releaseChannel != null && ( | |
| <div className="flex items-center gap-2"> | |
| <span>{releaseChannel.name}</span> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-6 w-6" | |
| onClick={() => setReleaseChannelId(releaseChannel.id)} | |
| > | |
| <IconSettings className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| )} | |
| {filter != null && ( | |
| releaseChannel == null ? ( | |
| <ReleaseConditionBadge condition={filter} /> | |
| ) : ( | |
| <div className="flex items-center gap-2"> | |
| <span>{releaseChannel.name}</span> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-6 w-6" | |
| aria-label={`Configure ${releaseChannel.name} settings`} | |
| onClick={() => setReleaseChannelId(releaseChannel.id)} | |
| > | |
| <IconSettings className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| ) | |
| )} |
| <div className="flex-grow" /> | ||
| <Button | ||
| onClick={() => { | ||
| console.log(">>> localCondition", localCondition); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debugging console.log statement.
The console.log statement on line 178 appears to be for debugging purposes and should be removed to prevent unnecessary console output in production.
Apply this diff to remove the debug statement:
- console.log(">>> localCondition", localCondition);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(">>> localCondition", localCondition); |
| onClick={() => { | ||
| const releaseChannel = releaseChannels.find( | ||
| (rc) => rc.id === localReleaseChannelId, | ||
| ); | ||
| if (releaseChannel == null) return; | ||
| setReleaseChannel(releaseChannel); | ||
| setOpen(false); | ||
| setError(null); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent state updates by invoking onChange when selecting a release channel.
In the "Release Channels" tab, the onChange handler is not called when the Save button is clicked. To maintain consistency and notify the parent component of the condition change, consider invoking onChange with the selected release channel's release filter.
Apply this diff to fix the issue:
onClick={() => {
const releaseChannel = releaseChannels.find(
(rc) => rc.id === localReleaseChannelId,
);
if (releaseChannel == null) return;
setReleaseChannel(releaseChannel);
+ onChange(releaseChannel.releaseFilter ?? undefined);
setOpen(false);
setError(null);
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={() => { | |
| const releaseChannel = releaseChannels.find( | |
| (rc) => rc.id === localReleaseChannelId, | |
| ); | |
| if (releaseChannel == null) return; | |
| setReleaseChannel(releaseChannel); | |
| setOpen(false); | |
| setError(null); | |
| }} | |
| onClick={() => { | |
| const releaseChannel = releaseChannels.find( | |
| (rc) => rc.id === localReleaseChannelId, | |
| ); | |
| if (releaseChannel == null) return; | |
| setReleaseChannel(releaseChannel); | |
| onChange(releaseChannel.releaseFilter ?? undefined); | |
| setOpen(false); | |
| setError(null); | |
| }} |
| const releaseChannelId = useMemo<string | undefined>( | ||
| () => urlParams.get("release-channel") ?? undefined, | ||
| [urlParams], | ||
| ); |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes