fix: Release channel ui cleanup #188
Conversation
WalkthroughThis pull request introduces significant updates across multiple components related to environment and release management. Key changes include the addition of enums for tab management in 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: 12
🧹 Outside diff range and nitpick comments (9)
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx (2)
10-15: Add JSDoc documentation for the UsageInfo type.Consider adding JSDoc documentation to explain the purpose and structure of the UsageInfo type, which would improve maintainability and help other developers understand its usage.
+/** + * Represents the usage information for environments and policies + * @property environments - List of direct environments + * @property policies - List of environment policies with their associated environments + */ type UsageInfo = { environments: SCHEMA.Environment[]; policies: (SCHEMA.EnvironmentPolicy & { environments: SCHEMA.Environment[]; })[]; };
96-98: Improve policy name fallback handling.The policy name fallback logic is duplicated and could be more consistent.
Consider extracting the policy name formatting:
+const formatPolicyName = (name: string) => name || "Unnamed policy"; + // Then use it in both places: -{e.policyInherited.name != "" ? e.policyInherited.name : "policy"} +{formatPolicyName(e.policyInherited.name)}apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1)
68-73: Add TypeScript return type for improved type safety.The hook's return type should be explicitly defined to reflect the added tab management functionality.
Consider adding this type definition:
interface UseEnvironmentDrawerReturn { environmentId: string | null; setEnvironmentId: (id: string | null) => void; removeEnvironmentId: () => void; tab: EnvironmentDrawerTab | null; setTab: (tab: EnvironmentDrawerTab | null) => void; }packages/db/src/schema/release.ts (1)
75-81: LGTM! Well-structured table relations.The
releaseChannelRelationsdefinition properly establishes the many-to-many relationships between release channels and environments, following database schema best practices.Consider documenting these relationships in a README or schema documentation to help other developers understand the data model, especially given the complexity of environment-to-release-channel mappings.
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (1)
36-63: LGTM with a minor suggestion for error handling.The enum and hook implementation look good, providing type safety and proper URL synchronization. Consider adding validation for unknown tab values in the hook to handle malformed URLs gracefully.
const useEnvironmentPolicyDrawerTab = () => { const router = useRouter(); const params = useSearchParams(); - const tab = params.get(tabParam) as EnvironmentPolicyDrawerTab | null; + const rawTab = params.get(tabParam); + const tab = rawTab && Object.values(EnvironmentPolicyDrawerTab).includes(rawTab as EnvironmentPolicyDrawerTab) + ? (rawTab as EnvironmentPolicyDrawerTab) + : null;apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
209-214: Consider adding null checks and improving variable namingWhile the logic is correct, consider these improvements for better robustness and readability:
-const blockedEnv = blockedEnvByRelease.data?.find( - (be) => - be.releaseId === release.id && - be.environmentId === env.id, -); -const isBlockedByReleaseChannel = blockedEnv != null; +const blockedEnvironment = blockedEnvByRelease.data?.find( + (blockedEntry) => + blockedEntry?.releaseId === release.id && + blockedEntry?.environmentId === env.id, +) ?? null; +const isBlockedByReleaseChannel = blockedEnvironment !== null;packages/api/src/router/release.ts (1)
Line range hint
371-394: Consider refactoring duplicate subquery logic.While the subqueries are well-structured, there's significant duplication in the field selection and join logic. Consider extracting the common parts into a reusable function.
Here's a suggested approach to reduce duplication:
const createReleaseChannelSubquery = ( sourceTable: typeof environmentReleaseChannel | typeof environmentPolicyReleaseChannel, joinField: string, alias: string ) => { return db .select({ releaseChannelId: releaseChannel.id, [joinField]: sourceTable[joinField], releaseChannelDeploymentId: releaseChannel.deploymentId, releaseChannelFilter: releaseChannel.releaseFilter, }) .from(sourceTable) .innerJoin( releaseChannel, eq(sourceTable.channelId, releaseChannel.id), ) .as(alias); }; const envRCSubquery = createReleaseChannelSubquery( environmentReleaseChannel, 'releaseChannelEnvId', 'envRCSubquery' ); const policyRCSubquery = createReleaseChannelSubquery( environmentPolicyReleaseChannel, 'releaseChannelPolicyId', 'policyRCSubquery' );apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (2)
46-47: IncludeworkspaceSlugin null checksWhile
workspaceSlugis typed as a string, adding a null or undefined check enhances robustness, ensuring that all required parameters are valid before constructing the URL.Modify the condition to include
workspaceSlug:- if (filter == null || systemSlug == null || deploymentSlug == null) + if (!workspaceSlug || !systemSlug || !deploymentSlug || !filter) return null;
37-39: Clarify the return value ofgetFinalFilterReturning
undefinedornullcan have different implications in JavaScript/TypeScript. Ensure consistency in how absence of a value is represented throughout the codebase.Consider returning
nullinstead ofundefinedif that aligns better with how null values are handled elsewhere:- const getFinalFilter = (filter: ReleaseCondition | null) => - filter && !isEmptyCondition(filter) ? filter : undefined; + const getFinalFilter = (filter: ReleaseCondition | null) => + filter && !isEmptyCondition(filter) ? filter : null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(4 hunks)packages/api/src/router/deployment.ts(1 hunks)packages/api/src/router/release.ts(4 hunks)packages/db/src/schema/environment.ts(4 hunks)packages/db/src/schema/release.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/deployment.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
🔇 Additional comments (20)
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx (2)
4-10: LGTM! Import statements are well-organized.
The new imports are properly structured and follow the project's conventions.
Also applies to: 16-16, 19-19
34-34: Verify loading state coverage.
The loading state has been simplified to only depend on releaseChannelQ.isLoading. Please ensure this doesn't miss any edge cases where we might need to show loading states for other async operations.
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try with more generic patterns and file extensions to find loading states and query hooks in the codebase.
Let's try searching in a specific directory path since the file type flags aren't working. We'll look for both query and mutation hooks that might affect loading states.
Based on the search results, I can now generate the final response. The codebase search shows that the loading state in the ReleaseChannelDrawer component only depends on releaseChannelQ.isLoading because it's the only query being used in this component, and the loading state is appropriately used to show a loading spinner while fetching the release channel data. The component's functionality is focused solely on displaying release channel details, so no other loading states are necessary.
Loading state implementation is correct
The simplified loading state that only depends on releaseChannelQ.isLoading is appropriate because:
- The component only fetches and displays release channel data
- Loading spinner is shown during the data fetch
- No other async operations are happening in this component that would require additional loading states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other loading states or queries in related components
rg -A 3 "isLoading" --type tsx
Length of output: 58
Script:
#!/bin/bash
# Search for loading states in TypeScript/React files using a more generic pattern
rg -A 3 "isLoading" --type ts --type tsx --type jsx
# Also search for any query hooks that might have loading states
rg -A 3 "useQuery" --type ts --type tsx --type jsx
# Look specifically in the release channel drawer component and related files
fd "ReleaseChannelDrawer" -e tsx -e ts | xargs cat
Length of output: 4112
Script:
#!/bin/bash
# Try with ripgrep without file type specification
rg -A 3 "isLoading|useQuery" apps/webservice/src/app/
# Also look for any mutation hooks that might affect loading states
rg -A 3 "useMutation" apps/webservice/src/app/
Length of output: 115554
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx (1)
1-8: LGTM! Imports are well-organized.
The imports are properly structured and all are being utilized in the component.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (2)
24-30: LGTM! Well-structured enum and constant declarations.
The enum values are descriptive and follow consistent naming conventions.
77-78: LGTM! Clean prop destructuring.
The destructuring is well-organized and includes all necessary props.
packages/db/src/schema/release.ts (1)
20-20: LGTM! Clean import organization.
The new imports are well-organized and properly grouped by their source modules.
Also applies to: 49-53
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (4)
70-70: Clean hook integration.
The tab management integration into useEnvironmentPolicyDrawer is well-structured and maintains good separation of concerns.
Also applies to: 89-90
192-225: Well-structured tab button implementation.
The tab button implementation is type-safe and consistent, with appropriate default tab handling and semantic icon choices.
234-236: Clean default tab handling.
The View rendering properly handles the default tab case with appropriate null coalescing.
99-99: Consider handling the ReleaseChannels edge case more explicitly.
While the tab mapping is well-structured, the ReleaseChannels case has an implicit null check that could be made more explicit to prevent potential rendering issues.
[EnvironmentPolicyDrawerTab.ReleaseChannels]: deployments != null && (
- <ReleaseChannels policy={environmentPolicy} deployments={deployments} />
+ <ReleaseChannels
+ policy={environmentPolicy}
+ deployments={deployments}
+ />
+ ) || (
+ <div>Loading deployments...</div>Also applies to: 107-122
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (2)
23-23: LGTM: Clean import addition
The new hook import is properly organized and follows the project's import structure.
46-46: LGTM: Proper hook usage
The hook is correctly integrated at the component's top level, following React hooks best practices.
packages/api/src/router/release.ts (1)
454-457: LGTM! Clean and well-structured changes.
The modifications improve the return structure by:
- Including the
releaseChannelIdin a type-safe manner - Using nullish coalescing operator appropriately
- Simplifying the return structure to a filtered array
Also applies to: 470-474, 478-478
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (3)
139-161: Ensure accurate loading states and data display
The loading spinner and release count in the form label depend on the releasesQ query. Verify that the loading state transitions correctly and that the release count updates appropriately.
Testing the component should confirm that users receive accurate feedback during data fetching.
162-177: Conditional rendering of the "View releases" button
The button to view releases is correctly conditioned on releaseFilterUrl not being null, preventing potential errors from navigating to an invalid URL.
65-69: Verify that route parameters are always available
The useParams hook assumes that workspaceSlug, systemSlug, and deploymentSlug are provided. Missing parameters could lead to runtime errors.
Run the following script to confirm that these parameters are consistently supplied in routes:
packages/db/src/schema/environment.ts (4)
59-64: Good addition of relations to environmentPolicy and environmentReleaseChannel
The new relations enhance the environment schema by establishing clear links to environmentPolicy and environmentReleaseChannel, which will improve data integrity and query capabilities.
124-130: Properly defined environmentPolicyRelations
The added relations correctly map environmentPolicy to its associated environmentPolicyReleaseChannel and environment, facilitating comprehensive access to related data.
238-250: Well-structured environmentPolicyReleaseChannelRelations
The relations accurately connect environmentPolicyReleaseChannel with environmentPolicy and releaseChannel, ensuring consistency in the relational mappings.
276-288: Correctly established environmentReleaseChannelRelations
The new relations link environmentReleaseChannel to environment and releaseChannel effectively, enhancing the schema's relational coherence.
|
|
||
| return ( | ||
| <Drawer open={isOpen} onOpenChange={setIsOpen}> | ||
| <DrawerContent | ||
| showBar={false} | ||
| className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-1/3 overflow-auto rounded-none focus-visible:outline-none" | ||
| className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider responsive design for smaller screens.
The drawer width is set to w-2/3 which might be too wide on smaller screens. Consider adding responsive width classes or a mobile-first approach.
-className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
+className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-full md:w-3/4 lg:w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"📝 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.
| className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none" | |
| className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-full md:w-3/4 lg:w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none" |
There was a problem hiding this comment.
we are setting a max width on the component
There was a problem hiding this comment.
@adityachoudhari26 Understood. Since a max width is set on the component, the current width settings are appropriate. Thank you for clarifying.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx:42-42
Timestamp: 2024-11-01T02:33:07.360Z
Learning: When a max-width is set on a component, using a fixed width class without additional responsive width classes is acceptable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| const loading = releaseChannelQ.isLoading || releasesQ.isLoading; | ||
| const [activeTab, setActiveTab] = useState("overview"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using TypeScript enum for tab values.
Using string literals for tab values could lead to maintenance issues. Consider defining an enum for better type safety and maintainability.
+enum TabType {
+ OVERVIEW = "overview",
+ USAGE = "usage"
+}
-const [activeTab, setActiveTab] = useState("overview");
+const [activeTab, setActiveTab] = useState<TabType>(TabType.OVERVIEW);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
for now since there are two tabs we can keep it simple
There was a problem hiding this comment.
@adityachoudhari26, understood. Keeping it simple for now makes sense.
...bservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx
Show resolved
Hide resolved
| const inheritedEnvironments = policies | ||
| .flatMap((p) => p.environments.map((e) => ({ ...e, policyInherited: p }))) | ||
| .filter((env) => !environments.includes(env)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize inherited environments filtering logic.
The current implementation might be inefficient for large datasets as it creates a new array with all environments before filtering.
Consider using a Set for more efficient filtering:
- const inheritedEnvironments = policies
- .flatMap((p) => p.environments.map((e) => ({ ...e, policyInherited: p })))
- .filter((env) => !environments.includes(env));
+ const directEnvironmentIds = new Set(environments.map(e => e.id));
+ const inheritedEnvironments = policies.flatMap((p) =>
+ p.environments
+ .filter(e => !directEnvironmentIds.has(e.id))
+ .map(e => ({ ...e, policyInherited: p }))
+ );Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
the array will likely never be sufficiently large enough to cause perf issues
There was a problem hiding this comment.
@adityachoudhari26, understood.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:06.728Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx
Show resolved
Hide resolved
| const useEnvironmentDrawerTab = () => { | ||
| const router = useRouter(); | ||
| const params = useSearchParams(); | ||
| const tab = params.get(tabParam); | ||
|
|
||
| const setTab = (tab: EnvironmentDrawerTab | null) => { | ||
| const url = new URL(window.location.href); | ||
| if (tab === null) { | ||
| url.searchParams.delete(tabParam); | ||
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | ||
| return; | ||
| } | ||
| url.searchParams.set(tabParam, tab); | ||
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | ||
| }; | ||
|
|
||
| return { tab, setTab }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for tab parameter values.
The hook handles tab state well, but it might benefit from validating that the tab value from URL params matches the EnvironmentDrawerTab enum values.
Consider this improvement:
const useEnvironmentDrawerTab = () => {
const router = useRouter();
const params = useSearchParams();
const tab = params.get(tabParam);
+ const isValidTab = (value: string | null): value is EnvironmentDrawerTab => {
+ return value != null && Object.values(EnvironmentDrawerTab).includes(value as EnvironmentDrawerTab);
+ };
+ const validTab = isValidTab(tab) ? tab : null;
const setTab = (tab: EnvironmentDrawerTab | null) => {
// ... existing code ...
};
- return { tab, setTab };
+ return { tab: validTab, setTab };
};📝 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 useEnvironmentDrawerTab = () => { | |
| const router = useRouter(); | |
| const params = useSearchParams(); | |
| const tab = params.get(tabParam); | |
| const setTab = (tab: EnvironmentDrawerTab | null) => { | |
| const url = new URL(window.location.href); | |
| if (tab === null) { | |
| url.searchParams.delete(tabParam); | |
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | |
| return; | |
| } | |
| url.searchParams.set(tabParam, tab); | |
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | |
| }; | |
| return { tab, setTab }; | |
| }; | |
| const useEnvironmentDrawerTab = () => { | |
| const router = useRouter(); | |
| const params = useSearchParams(); | |
| const tab = params.get(tabParam); | |
| const isValidTab = (value: string | null): value is EnvironmentDrawerTab => { | |
| return value != null && Object.values(EnvironmentDrawerTab).includes(value as EnvironmentDrawerTab); | |
| }; | |
| const validTab = isValidTab(tab) ? tab : null; | |
| const setTab = (tab: EnvironmentDrawerTab | null) => { | |
| const url = new URL(window.location.href); | |
| if (tab === null) { | |
| url.searchParams.delete(tabParam); | |
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | |
| return; | |
| } | |
| url.searchParams.set(tabParam, tab); | |
| router.replace(`${url.pathname}?${url.searchParams.toString()}`); | |
| }; | |
| return { tab: validTab, setTab }; | |
| }; |
| {isBlockedByReleaseChannel ? ( | ||
| <span> | ||
| Blocked by{" "} | ||
| <Button | ||
| variant="link" | ||
| size="sm" | ||
| onClick={() => | ||
| setReleaseChannelId( | ||
| blockedEnv.releaseChannelId ?? null, | ||
| ) | ||
| } | ||
| className="px-0 text-muted-foreground/70" | ||
| > | ||
| release channel | ||
| </Button> | ||
| </span> | ||
| ) : ( | ||
| "No job agent" | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the release channel interaction UX
The interactive button is a good addition, but consider these UX improvements:
- Add loading state while the release channel drawer is opening
- Handle potential errors if the release channel data is unavailable
- Add a tooltip explaining what happens when clicking the button
<Button
variant="link"
size="sm"
+ disabled={isLoading}
+ title="Click to view release channel details"
onClick={() =>
- setReleaseChannelId(blockedEnv.releaseChannelId ?? null)
+ setReleaseChannelId(blockedEnv?.releaseChannelId ?? null).catch((error) => {
+ console.error('Failed to open release channel:', error);
+ // Add error handling UI feedback here
+ })
}
className="px-0 text-muted-foreground/70"
>
- release channel
+ {isLoading ? 'Opening...' : 'release channel'}
</Button>Committable suggestion skipped: line range outside the PR's diff.
| const onSubmit = form.handleSubmit((data) => { | ||
| const releaseFilter = getFinalFilter(data.releaseFilter); | ||
| updateReleaseChannel | ||
| .mutateAsync({ id: releaseChannel.id, data }) | ||
| .then(() => form.reset(data)) | ||
| .mutateAsync({ id: releaseChannel.id, data: { ...data, releaseFilter } }) | ||
| .then(() => form.reset({ ...data, releaseFilter })) | ||
| .then(() => | ||
| utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id), | ||
| ) | ||
| .then(() => router.refresh()), | ||
| .then(() => router.refresh()); | ||
| }); |
There was a problem hiding this comment.
Add error handling to the onSubmit function
Currently, the onSubmit function lacks error handling for the mutation. It's important to handle potential errors to provide feedback to the user in case the update fails.
Consider updating the function to include error handling:
- const onSubmit = form.handleSubmit((data) => {
+ const onSubmit = form.handleSubmit(async (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(() => router.refresh());
+ try {
+ await updateReleaseChannel.mutateAsync({
+ id: releaseChannel.id,
+ data: { ...data, releaseFilter },
+ });
+ form.reset({ ...data, releaseFilter });
+ await utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id);
+ router.refresh();
+ } catch (error) {
+ // Handle the error, e.g., display a notification to the user
+ console.error("Failed to update the release channel:", error);
+ }
});📝 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 onSubmit = form.handleSubmit((data) => { | |
| const releaseFilter = getFinalFilter(data.releaseFilter); | |
| updateReleaseChannel | |
| .mutateAsync({ id: releaseChannel.id, data }) | |
| .then(() => form.reset(data)) | |
| .mutateAsync({ id: releaseChannel.id, data: { ...data, releaseFilter } }) | |
| .then(() => form.reset({ ...data, releaseFilter })) | |
| .then(() => | |
| utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id), | |
| ) | |
| .then(() => router.refresh()), | |
| .then(() => router.refresh()); | |
| }); | |
| const onSubmit = form.handleSubmit(async (data) => { | |
| const releaseFilter = getFinalFilter(data.releaseFilter); | |
| try { | |
| await updateReleaseChannel.mutateAsync({ | |
| id: releaseChannel.id, | |
| data: { ...data, releaseFilter }, | |
| }); | |
| form.reset({ ...data, releaseFilter }); | |
| await utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id); | |
| router.refresh(); | |
| } catch (error) { | |
| // Handle the error, e.g., display a notification to the user | |
| console.error("Failed to update the release channel:", error); | |
| } | |
| }); |
| const rc = await ctx.db.query.releaseChannel.findFirst({ | ||
| where: eq(releaseChannel.id, input), | ||
| with: { | ||
| environmentReleaseChannels: { with: { environment: true } }, | ||
| environmentPolicyReleaseChannels: { | ||
| with: { environmentPolicy: true }, | ||
| }, | ||
| }, | ||
| }); | ||
| if (rc == null) return null; | ||
| const policyIds = rc.environmentPolicyReleaseChannels.map( | ||
| (eprc) => eprc.environmentPolicy.id, | ||
| ); | ||
|
|
||
| const envs = await ctx.db | ||
| .select() | ||
| .from(releaseChannel) | ||
| .where(eq(releaseChannel.id, input)) | ||
| .orderBy(releaseChannel.name) | ||
| .then(takeFirst), | ||
| ), | ||
| .from(environment) | ||
| .where(inArray(environment.policyId, policyIds)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider combining database queries to optimize performance
The current implementation makes two separate database queries: one to retrieve the releaseChannel and its related data (rc), and another to fetch environments (envs) based on policy IDs. You can optimize performance by combining these queries into a single query using eager loading of nested relationships.
Apply this diff to combine the queries:
const rc = await ctx.db.query.releaseChannel.findFirst({
where: eq(releaseChannel.id, input),
with: {
environmentReleaseChannels: { with: { environment: true } },
environmentPolicyReleaseChannels: {
with: {
environmentPolicy: {
+ with: { environments: true },
},
},
},
},
});
if (rc == null) return null;
- const policyIds = rc.environmentPolicyReleaseChannels.map(
- (eprc) => eprc.environmentPolicy.id,
- );
- const envs = await ctx.db
- .select()
- .from(environment)
- .where(inArray(environment.policyId, policyIds));
return {
...rc,
usage: {
environments: rc.environmentReleaseChannels.map(
(erc) => erc.environment,
),
policies: rc.environmentPolicyReleaseChannels.map((eprc) => ({
...eprc.environmentPolicy,
- environments: envs.filter(
- (e) => e.policyId === eprc.environmentPolicy.id,
- ),
+ environments: eprc.environmentPolicy.environments,
})),
},
};📝 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 rc = await ctx.db.query.releaseChannel.findFirst({ | |
| where: eq(releaseChannel.id, input), | |
| with: { | |
| environmentReleaseChannels: { with: { environment: true } }, | |
| environmentPolicyReleaseChannels: { | |
| with: { environmentPolicy: true }, | |
| }, | |
| }, | |
| }); | |
| if (rc == null) return null; | |
| const policyIds = rc.environmentPolicyReleaseChannels.map( | |
| (eprc) => eprc.environmentPolicy.id, | |
| ); | |
| const envs = await ctx.db | |
| .select() | |
| .from(releaseChannel) | |
| .where(eq(releaseChannel.id, input)) | |
| .orderBy(releaseChannel.name) | |
| .then(takeFirst), | |
| ), | |
| .from(environment) | |
| .where(inArray(environment.policyId, policyIds)); | |
| const rc = await ctx.db.query.releaseChannel.findFirst({ | |
| where: eq(releaseChannel.id, input), | |
| with: { | |
| environmentReleaseChannels: { with: { environment: true } }, | |
| environmentPolicyReleaseChannels: { | |
| with: { | |
| environmentPolicy: { | |
| with: { environments: true }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| if (rc == null) return null; | |
| return { | |
| ...rc, | |
| usage: { | |
| environments: rc.environmentReleaseChannels.map( | |
| (erc) => erc.environment, | |
| ), | |
| policies: rc.environmentPolicyReleaseChannels.map((eprc) => ({ | |
| ...eprc.environmentPolicy, | |
| environments: eprc.environmentPolicy.environments, | |
| })), | |
| }, | |
| }; |
There was a problem hiding this comment.
due to a limitation with drizzle (where I reference the same table twice in a rbq) I need to pull it out. but the majority of the work is done by the main query, so this shouldn't be too bad perf-wise
There was a problem hiding this comment.
@adityachoudhari26, understood. Thank you for explaining the limitation with Drizzle when referencing the same table twice in an rbq. I've noted this information for future reviews.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: packages/api/src/router/deployment.ts:144-161
Timestamp: 2024-11-01T02:37:25.340Z
Learning: In `packages/api/src/router/deployment.ts`, when using Drizzle ORM, there is a limitation when referencing the same table twice in a relational builder query (rbq), requiring separate queries to avoid issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores