chore: clean up edit policy ui #546
Conversation
WalkthroughThis update refactors several policy editing components to centralize form state management into a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PolicyFormContextProvider
participant EditComponent
participant API
User->>EditComponent: Interact with form fields
EditComponent->>PolicyFormContextProvider: Update form state
User->>EditComponent: Submit form
EditComponent->>PolicyFormContextProvider: Trigger submit handler
PolicyFormContextProvider->>API: Validate and send update mutation
API-->>PolicyFormContextProvider: Return success or error
PolicyFormContextProvider-->>EditComponent: Update form state and UI accordingly
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🔭 Outside diff range comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (1)
178-186: 🛠️ Refactor suggestionGuard query with a meaningful key instead of empty string
Passing
""as a placeholder workspace id makes the query key ambiguous and may pollute the cache:-const deploymentsQ = api.deployment.byWorkspaceId.useQuery( - workspace?.id ?? "", - { enabled: workspace != null }, -); +const deploymentsQ = api.deployment.byWorkspaceId.useQuery( + workspace?.id as string, + { enabled: !!workspace }, +);Using
as string(or a non-nullable assertion) together withenabledkeeps the key stable and avoids accidental sharing between workspaces.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (2)
95-101: 🛠️ Refactor suggestionTighten typing of
fieldsprop
fieldsis currently typed asRecord<"id", string>[], which loses the rest of the information React-Hook-Form attaches (e.g. field values, internal meta). This weakens type-safety and forces downstreamanycasts.-interface UserSectionProps { - control: Control<SCHEMA.UpdatePolicy>; - fields: Record<"id", string>[]; +import type { FieldArrayWithId } from "react-hook-form"; + +interface UserSectionProps { + control: Control<SCHEMA.UpdatePolicy>; + fields: FieldArrayWithId<SCHEMA.UpdatePolicy, "versionUserApprovals", "id">[];Apply the same change to
RoleSectionProps.
206-214:⚠️ Potential issue
parseIntmay yieldNaN; add safe-guardIf the input is cleared,
parseInt("")returnsNaN, which violates the schema and can make the form value non-serialisable. A small guard keeps the value valid:- onChange={(e) => field.onChange(parseInt(e.target.value))} + onChange={(e) => { + const num = parseInt(e.target.value, 10); + field.onChange(Number.isNaN(num) ? 1 : num); + }}Apply the same pattern for the general-approval count field below.
Also applies to: 313-321
🧹 Nitpick comments (13)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionTagConditionRender.tsx (2)
12-12: Extended component to support both VersionCondition and TagCondition types.Broadened the generic type parameter to handle both condition types, which aligns with the updated conditional logic in DeploymentVersionConditionRender.tsx.
However, the component name "DeploymentVersionTagConditionRender" might be slightly misleading since it now handles both version and tag conditions. Consider renaming it to better reflect its dual purpose in a future refactoring.
25-25: Consider dynamic title based on condition type.The title is hardcoded as "Tag" even when the component may be rendering a version condition. For better clarity, consider making this dynamic:
- title="Tag" + title={condition.type === "tag" ? "Tag" : "Version"}apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx (1)
30-37: Reduce repetition with a mapping table.Both
getInitialTabandonTabChangerepeat the sameifcascade, which is error-prone. A map improves readability and maintains a single source of truth.const routes = { overview: baseUrl, configuration: configurationUrl, "time-windows": timeWindowsUrl, "deployment-flow": deploymentFlowUrl, "quality-security": qualitySecurityUrl, }; const getInitialTab = () => Object.entries(routes).find(([, url]) => url === pathname)?.[0] ?? "overview"; ... const onTabChange = (value: keyof typeof routes) => { router.push(routes[value]); setActiveTab(value); };Also applies to: 45-51
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (1)
45-54: Duplicate policy fetch may be unnecessary.The parent
PolicyFormContextProvideralready provides the fullpolicy; fetching it again (api.policy.byId.useQuery) duplicates network traffic and may lead to inconsistent data if the two sources diverge. Consider:
- Passing
policythrough context and readingpolicy.denyWindowsdirectly, or- Converting this query to
enabled: falseand refetching only when a manual “refresh” is requested.Reducing duplicate requests will improve performance on slow networks.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (2)
62-67: Avoid repeated.find()calls inside the button label
deployments.find(...)is executed up to three times on every render.
For large deployment lists this becomes unnecessarily expensive.-? deployments.find((d) => d.id === selectedId) - ? `${deployments.find((d) => d.id === selectedId)?.system.name} / ${deployments.find((d) => d.id === selectedId)?.name}` +const selected = deployments.find((d) => d.id === selectedId); +selected ? `${selected.system.name} / ${selected.name}`By computing
selectedonce you gain O(n) → O(1) performance and improve readability.
254-260: Multiple “Save” buttons within the same form can cause UX confusionAll edit sub-pages render their own
<Button type="submit">. When two tabs are visible concurrently (e.g. in split-screen) users may not realise the buttons are identical. Consider:• Rendering a single save action in a shared toolbar/layout, or
• Giving contextual labels (“Save Deployment Flow”, “Save Targets”, …).This also prevents accidental double-submissions triggered by
Enterkey presses in nested sections.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (2)
48-56: Remove strayconsole.logstatementsDebug logs left in production code clutter the console and may leak PII.
- console.log(policy); ... - console.log(form.getValues());Please delete or wrap them behind a dev-only flag.
60-61: Mutation hook is created on every render
const updatePolicy = api.policy.update.useMutation();Because the provider re-renders whenever
childrenchange, the mutation instance is recreated.
Wrapping it inuseMemo(or moving it outside the component if possible) avoids resetting internal mutation state.-const updatePolicy = api.policy.update.useMutation(); +const updatePolicy = useMemo(() => api.policy.update.useMutation(), []);apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx (3)
72-76: Debug statement should be removed
console.log(form.getValues());leaks form data and clutters the console.
217-238: Inefficient detection of selected target scope
TARGET_SCOPE_OPTIONS.findis executed on every render for every target row.
Caching the matched option or storing an explicittargetScopefield (enum) in the form state would be more performant and simpler to reason about.
360-369: Removing a target without confirmation can lead to data lossA single click instantly deletes the target. Consider adding:
• A confirmation dialog, or
• An undo snackbar.This aligns with common UX patterns and prevents accidental loss.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (2)
38-42: Handle loading / error states for members query
api.workspace.members.list.useQuerycan returnisLoading/error. While the component gracefully handlesundefineddata, the combobox is still rendered and may appear empty while loading or permanently blank on network failure.
Consider disabling the trigger or showing a spinner / error message until data is available so the UX is clearer.
360-374: Remove stale commented-out codeThe
appendRolebutton & handler are commented out but theappendRolehook is no longer declared. Keeping dead code around adds noise and confuses future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionConditionRender.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionTagConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/layout.tsx(1 hunks)apps/webservice/src/app/urls.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionConditionRender.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/layout.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionTagConditionRender.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsxapps/webservice/src/app/urls.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx
🧬 Code Graph Analysis (5)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionConditionRender.tsx (1)
packages/validators/src/releases/conditions/release-condition.ts (2)
isVersionCondition(91-94)isTagCondition(101-104)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionTagConditionRender.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/deployment-version-condition-props.ts (1)
DeploymentVersionConditionRenderProps(3-11)packages/validators/src/releases/conditions/tag-condition.ts (1)
TagCondition(11-11)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx (2)
packages/db/src/schema/workspace.ts (1)
workspace(18-27)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (1)
EditQualitySecurity(234-388)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx (2)
packages/db/src/schema/policy.ts (1)
policy(42-62)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
PolicyFormContextProvider(44-103)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (6)
packages/db/src/schema/policy.ts (5)
PolicyTarget(186-186)PolicyDeploymentVersionSelector(187-189)UpdatePolicy(176-176)policy(42-62)updatePolicy(154-175)packages/db/src/schema/rules/deny-window.ts (1)
PolicyRuleDenyWindow(142-144)packages/db/src/schema/rules/approval-any.ts (1)
PolicyRuleAnyApproval(77-79)packages/db/src/schema/rules/approval-user.ts (1)
PolicyRuleUserApproval(82-84)packages/db/src/schema/rules/approval-role.ts (1)
PolicyRuleRoleApproval(88-90)apps/webservice/src/app/[workspaceSlug]/(app)/policies/_utils/policy-targets.ts (2)
convertEmptySelectorsToNull(24-49)isValidTarget(77-101)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (9)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionConditionRender.tsx (2)
8-8: Added support for tag conditions.Added import for the
isTagConditionvalidator function to enable tag condition type checking alongside version conditions.
58-58: Logic enhancement to handle both version and tag conditions.Extending the conditional check to support both version and tag conditions is a good improvement that unifies the handling of similar condition types.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionTagConditionRender.tsx (1)
5-5: Added TagCondition type import.Added import for the TagCondition type to support the component's type extension.
apps/webservice/src/app/urls.ts (1)
59-59: Added URL builder function for approval gates appropriately.The addition of the
approvalGates()URL builder method to theworkspacePolicyEditfunction is consistent with the existing URL pattern in the codebase. This enhances the API by making the URL structure for approval gates accessible at the policy edit level.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/layout.tsx (1)
64-64: Improved container with proper scrolling behavior.The enhanced container div now includes custom scrollbar styling with explicit height calculation and overflow control. This improvement provides a better user experience by ensuring consistent scrolling behavior.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsx (1)
3-4: Simplified page component correctly.The ConfigurationPage component has been appropriately simplified to a synchronous function that renders the EditConfiguration component without props. This aligns with the broader refactoring effort to centralize form state management through context.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx (1)
7-12: Simplified page component to follow context pattern.The QualitySecurityPage has been refactored to only fetch workspace data and no longer handle policy data directly. This change properly aligns with using the PolicyFormContext for form state management.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (1)
49-53: Potentially destructiveresetduring form editing.
useEffectunconditionally callsform.reset({ …denyWindows })whenpolicyarrives or refreshes.
If the user has already modified other fields in the same form, those unsaved edits are silently preserved via...currentValues, but local edits todenyWindowsare overwritten. This can surprise users if the policy query refetches (e.g., window focus).Add a guard:
if (!form.formState.isDirty) { form.reset({ ...currentValues, denyWindows: policy.denyWindows }); }or debounce refetches.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (1)
169-176:useParamsgeneric omitspolicyId– risk of silentundefined
useParams<{ workspaceSlug: string }>()ignorespolicyId, yet this page is nested under.../policies/[policyId]/edit/....
AccessingpolicyIdelsewhere would yieldundefined, leading to hard-to-trace bugs.-const { workspaceSlug } = useParams<{ workspaceSlug: string }>(); +const { workspaceSlug, policyId } = + useParams<{ workspaceSlug: string; policyId: string }>();Even if
policyIdis not currently used, declaring it prevents accidental omissions later.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx
Outdated
Show resolved
Hide resolved
| const { params, children } = props; | ||
| const { workspaceSlug, policyId } = await params; | ||
| const { policyId } = await params; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider accepting params synchronously instead of as a Promise.
nextjs’s Layout/Page server components receive params synchronously ({ params: { … } }).
Treating it as a Promise adds an unnecessary await, complicates the type signature, and may confuse future maintainers.
-export default async function Layout(props: {
- children: React.ReactNode;
- params: Promise<{
- workspaceSlug: string;
- policyId: string;
- }>;
-}) {
- const { params, children } = props;
- const { policyId } = await params;
+export default async function Layout({
+ children,
+ params,
+}: {
+ children: React.ReactNode;
+ params: {
+ workspaceSlug: string;
+ policyId: string;
+ };
+}) {
+ const { policyId } = params;This also removes the accidental shadowing of the local params variable.
There was a problem hiding this comment.
this is not true as of the latest next js update
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right! I apologize for the incorrect suggestion. In Next.js 15, dynamic route parameters (params) do return Promises in async contexts, so the current implementation awaiting params is correct:
const { policyId } = await params;Thank you for the correction. This aligns with our existing knowledge about Next.js 15's behavior where params should be awaited before accessing their properties in async contexts.
🧠 Learnings used
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx
Outdated
Show resolved
Hide resolved
| const [activeTab, setActiveTab] = useState(getInitialTab()); | ||
|
|
||
| const router = useRouter(); | ||
|
|
||
| const onTabChange = (value: string) => { | ||
| if (value === "overview") router.push(baseUrl); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep tab state in sync with external navigation.
activeTab is initialised only once, so a browser back/forward action that changes pathname will desynchronise the UI. Add an effect to update state whenever the URL changes.
- const [activeTab, setActiveTab] = useState(getInitialTab());
+ const [activeTab, setActiveTab] = useState(() => getInitialTab());
+
+ // sync tab state when user navigates with browser controls
+ useEffect(() => {
+ setActiveTab(getInitialTab());
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [pathname]);Remember to import useEffect.
Committable suggestion skipped: line range outside the PR's diff.
| const onSubmit = form.handleSubmit((data) => { | ||
| const targets = | ||
| data.targets === undefined | ||
| ? undefined | ||
| : data.targets.map(convertEmptySelectorsToNull); | ||
|
|
||
| const isTargetsValid = | ||
| targets === undefined || targets.every(isValidTarget); | ||
| if (!isTargetsValid) { | ||
| const errorStr = "One or more of your targets are invalid"; | ||
| form.setError("targets", { message: errorStr }); | ||
| toast.error("Error creating policy", { description: errorStr }); | ||
| return; | ||
| } | ||
|
|
||
| return updatePolicy | ||
| .mutateAsync({ | ||
| id: policy.id, | ||
| data: { ...data, targets }, | ||
| }) | ||
| .then(() => { | ||
| toast.success("Policy updated successfully"); | ||
| form.reset(data); | ||
| router.refresh(); | ||
| utils.policy.byId.invalidate(); | ||
| utils.policy.list.invalidate(); | ||
| }) | ||
| .catch((error) => { | ||
| toast.error("Failed to update policy", { | ||
| description: error.message, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer async/await and invalidate queries with parameters
form.handleSubmitalready supports async handlers – usingawaitimproves readability and ensures thrown errors are caught by the wrapper.utils.policy.byId.invalidate()without{ id }may not hit the correct cache entry.
-const onSubmit = form.handleSubmit((data) => {
+const onSubmit = form.handleSubmit(async (data) => {
...
- return updatePolicy
- .mutateAsync({ id: policy.id, data: { ...data, targets } })
- .then(() => {
+ try {
+ await updatePolicy.mutateAsync({ id: policy.id, data: { ...data, targets } });
toast.success("Policy updated successfully");
form.reset(data);
router.refresh();
- utils.policy.byId.invalidate();
+ utils.policy.byId.invalidate({ id: policy.id });
utils.policy.list.invalidate();
- })
- .catch((error) => {
+ } catch (error: any) {
toast.error("Failed to update policy", { description: error.message });
- });
+ }
});This yields clearer flow control and guarantees the right cache key is invalidated.
📝 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 targets = | |
| data.targets === undefined | |
| ? undefined | |
| : data.targets.map(convertEmptySelectorsToNull); | |
| const isTargetsValid = | |
| targets === undefined || targets.every(isValidTarget); | |
| if (!isTargetsValid) { | |
| const errorStr = "One or more of your targets are invalid"; | |
| form.setError("targets", { message: errorStr }); | |
| toast.error("Error creating policy", { description: errorStr }); | |
| return; | |
| } | |
| return updatePolicy | |
| .mutateAsync({ | |
| id: policy.id, | |
| data: { ...data, targets }, | |
| }) | |
| .then(() => { | |
| toast.success("Policy updated successfully"); | |
| form.reset(data); | |
| router.refresh(); | |
| utils.policy.byId.invalidate(); | |
| utils.policy.list.invalidate(); | |
| }) | |
| .catch((error) => { | |
| toast.error("Failed to update policy", { | |
| description: error.message, | |
| }); | |
| }); | |
| }); | |
| const onSubmit = form.handleSubmit(async (data) => { | |
| const targets = | |
| data.targets === undefined | |
| ? undefined | |
| : data.targets.map(convertEmptySelectorsToNull); | |
| const isTargetsValid = | |
| targets === undefined || targets.every(isValidTarget); | |
| if (!isTargetsValid) { | |
| const errorStr = "One or more of your targets are invalid"; | |
| form.setError("targets", { message: errorStr }); | |
| toast.error("Error creating policy", { description: errorStr }); | |
| return; | |
| } | |
| try { | |
| await updatePolicy.mutateAsync({ | |
| id: policy.id, | |
| data: { ...data, targets }, | |
| }); | |
| toast.success("Policy updated successfully"); | |
| form.reset(data); | |
| router.refresh(); | |
| utils.policy.byId.invalidate({ id: policy.id }); | |
| utils.policy.list.invalidate(); | |
| } catch (error: any) { | |
| toast.error("Failed to update policy", { | |
| description: error.message, | |
| }); | |
| } | |
| }); |
| const value = parseInt(e.target.value); | ||
| field.onChange(value); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Handle NaN when parsing priority
parseInt("") or non-numeric input yields NaN, which the schema probably rejects.
-const value = parseInt(e.target.value);
-field.onChange(value);
+const parsed = Number.isFinite(+e.target.value) ? parseInt(e.target.value, 10) : undefined;
+field.onChange(parsed);Alternatively, rely on e.target.valueAsNumber which returns NaN for empty input and can be handled explicitly.
Committable suggestion skipped: line range outside the PR's diff.
| <div className="flex gap-2"> | ||
| <Button | ||
| type="button" | ||
| variant="outline" | ||
| size="sm" | ||
| className="flex-1" | ||
| onClick={() => appendUser({ userId: "" })} | ||
| > | ||
| <IconPlus className="mr-2 h-4 w-4" /> |
There was a problem hiding this comment.
Default object passed to appendUser is incomplete
appendUser({ userId: "" }) omits required fields such as requiredApprovalsCount (and possibly the RHF-generated id). This will immediately put the form in an invalid state and can break validation.
- onClick={() => appendUser({ userId: "" })}
+ onClick={() =>
+ appendUser({
+ userId: "",
+ requiredApprovalsCount: 1,
+ })
+ }Ensure the structure matches SCHEMA.UpdatePolicy["versionUserApprovals"][number].
📝 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.
| <div className="flex gap-2"> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| size="sm" | |
| className="flex-1" | |
| onClick={() => appendUser({ userId: "" })} | |
| > | |
| <IconPlus className="mr-2 h-4 w-4" /> | |
| <div className="flex gap-2"> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| size="sm" | |
| className="flex-1" | |
| - onClick={() => appendUser({ userId: "" })} | |
| + onClick={() => | |
| + appendUser({ | |
| + userId: "", | |
| + requiredApprovalsCount: 1, | |
| + }) | |
| + } | |
| > | |
| <IconPlus className="mr-2 h-4 w-4" /> |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx (1)
39-39: Keep tab state in sync with external navigation.
activeTabis initialised only once, so a browser back/forward action that changespathnamewill desynchronise the UI. Consider adding an effect to update state whenever the URL changes.const [activeTab, setActiveTab] = useState(getInitialTab()); + + // sync tab state when user navigates with browser controls + useEffect(() => { + setActiveTab(getInitialTab()); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [pathname]);Note: Don't forget to import
useEffectfrom "react".
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx (4)
30-37: Consider usingswitchstatement for better readability and maintenance.The multiple consecutive
ifstatements could be replaced with a more structuredswitchstatement to improve readability, especially as more tabs might be added in the future.const getInitialTab = () => { - if (pathname === baseUrl) return "overview"; - if (pathname === configurationUrl) return "configuration"; - if (pathname === timeWindowsUrl) return "time-windows"; - if (pathname === deploymentFlowUrl) return "deployment-flow"; - if (pathname === qualitySecurityUrl) return "quality-security"; - return "overview"; + switch (pathname) { + case baseUrl: + return "overview"; + case configurationUrl: + return "configuration"; + case timeWindowsUrl: + return "time-windows"; + case deploymentFlowUrl: + return "deployment-flow"; + case qualitySecurityUrl: + return "quality-security"; + default: + return "overview"; + } };
43-50: Consider usingswitchstatement in the tab change handler for consistency.Similar to the initial tab detection, using a switch statement here would improve readability and maintenance.
const onTabChange = (value: string) => { - if (value === "overview") router.push(baseUrl); - if (value === "configuration") router.push(configurationUrl); - if (value === "time-windows") router.push(timeWindowsUrl); - if (value === "deployment-flow") router.push(deploymentFlowUrl); - if (value === "quality-security") router.push(qualitySecurityUrl); + switch (value) { + case "overview": + router.push(baseUrl); + break; + case "configuration": + router.push(configurationUrl); + break; + case "time-windows": + router.push(timeWindowsUrl); + break; + case "deployment-flow": + router.push(deploymentFlowUrl); + break; + case "quality-security": + router.push(qualitySecurityUrl); + break; + } setActiveTab(value); };
56-56: Consider updating tab label for clarity.The tab labeled "Edit" has a value of "configuration", which might be confusing. Consider updating the label to "Configuration" to match its functionality.
- <TabsTrigger value="configuration">Edit</TabsTrigger> + <TabsTrigger value="configuration">Configuration</TabsTrigger>
62-67: Label mismatch with value for deployment-flow tab.The tab has a value of "deployment-flow" but is labeled "Version Conditions", which might cause confusion. Consider updating the label to match the value for better clarity and consistency.
<TabsTrigger value="deployment-flow" className="flex items-center gap-1" > <IconTag className="h-4 w-4" /> - Version Conditions + Deployment Flow </TabsTrigger>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsx (1)
3-4: LGTM - Clean component refactoring.The refactoring simplifies this component by leveraging the context-based state management pattern, which is a good practice for reducing prop drilling and centralizing form state.
Summary by CodeRabbit
New Features
Refactor
Style
Chores