Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors the organization general settings UI by introducing a new GeneralPage component, simplifying the main index, removing the delete route, converting DeleteOrganizationDialog to a controlled component, and introducing navigation callback patterns through new type definitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/sdk/react/views/general/delete-organization-dialog.tsx (1)
38-57:⚠️ Potential issue | 🟠 MajorForm and checkbox state are not reset when the dialog reopens.
Previously, the delete dialog was route-mounted, so it remounted (and reset) on every navigation. Now that it's a controlled component that stays mounted, opening → typing → closing → reopening will show stale input and a pre-checked acknowledgment checkbox.
Reset the form and acknowledgment state when
openchanges:Proposed fix — reset state on open
+import { useState, useEffect } from 'react'; ... export const DeleteOrganizationDialog = ({ open, onOpenChange, onDeleteSuccess }: DeleteOrganizationDialogProps) => { const { watch, handleSubmit, setError, formState: { errors, isSubmitting }, - register + register, + reset } = useForm({ resolver: yupResolver(orgSchema) }); ... const [isAcknowledged, setIsAcknowledged] = useState(false); + + useEffect(() => { + if (open) { + reset(); + setIsAcknowledged(false); + } + }, [open, reset]);
🧹 Nitpick comments (2)
web/sdk/react/components/organization/general/index.tsx (1)
6-9: Usewindow.location.hrefinstead ofwindow.locationto avoid@ts-ignore.Assigning a string to
window.location.hrefis type-safe and has the same effect as assigning towindow.location.♻️ Proposed fix
return <GeneralPage onDeleteSuccess={() => { - // `@ts-ignore` - window.location = window.location.origin; + window.location.href = window.location.origin; }}/>;web/sdk/react/views/general/delete-organization-dialog.tsx (1)
59-59: Consider typing theonSubmitparameter instead ofany.The
dataparameter can use the inferred schema type from yup (yup.InferType<typeof orgSchema>) to avoidanyand get type safety ondata.title.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
web/sdk/react/components/organization/general/index.tsxweb/sdk/react/components/organization/routes.tsxweb/sdk/react/components/organization/shared/types.tsweb/sdk/react/views/general/delete-organization-dialog.tsxweb/sdk/react/views/general/general-organization.tsxweb/sdk/react/views/general/general-page.tsxweb/sdk/react/views/general/general.module.cssweb/sdk/react/views/general/index.ts
| <Button | ||
| variant="solid" | ||
| color="danger" | ||
| type="submit" | ||
| onClick={() => setShowDeleteDialog(true)} | ||
| disabled={!canDeleteWorkspace} | ||
| data-test-id="frontier-sdk-delete-organization-btn" | ||
| > | ||
| Delete {t.organization({ case: 'lower' })} | ||
| </Button> |
There was a problem hiding this comment.
Delete button should use type="button", not type="submit".
This button is not inside a <form> — it opens a dialog via onClick. Using type="submit" can cause unintended form submission if this component is ever rendered inside an ancestor <form> element.
Proposed fix
<Button
variant="solid"
color="danger"
- type="submit"
+ type="button"
onClick={() => setShowDeleteDialog(true)}
disabled={!canDeleteWorkspace}
data-test-id="frontier-sdk-delete-organization-btn"
>📝 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.
| <Button | |
| variant="solid" | |
| color="danger" | |
| type="submit" | |
| onClick={() => setShowDeleteDialog(true)} | |
| disabled={!canDeleteWorkspace} | |
| data-test-id="frontier-sdk-delete-organization-btn" | |
| > | |
| Delete {t.organization({ case: 'lower' })} | |
| </Button> | |
| <Button | |
| variant="solid" | |
| color="danger" | |
| type="button" | |
| onClick={() => setShowDeleteDialog(true)} | |
| disabled={!canDeleteWorkspace} | |
| data-test-id="frontier-sdk-delete-organization-btn" | |
| > | |
| Delete {t.organization({ case: 'lower' })} | |
| </Button> |
Pull Request Test Coverage Report for Build 22369338658Details
💛 - Coveralls |
2 participants
Summary
Decouple the General page components from TanStack Router, making them standalone and exportable. This is the first step in migrating organization page components to a router-agnostic architecture.
Changes
react/views/general/:general-page.tsx— Self-containedGeneralPagewith state-based delete dialog (replaces route-based modal)general-organization.tsx— Organization settings form, moved fromcomponents/organization/general/general.workspace.tsxdelete-organization-dialog.tsx— Prop-controlledDeleteOrganizationDialog(open/onClose) replacing the route-mounted versiongeneral.module.css— Styles co-located with view componentsindex.ts— Barrel exportcomponents/organization/shared/types.ts(OnNavigate,BasePageProps)components/organization/general/index.tsxto a thin wrapper importingGeneralPagefrom viewsdeleteOrgRoutechild route fromroutes.tsxgeneral/delete.tsxandgeneral/general.workspace.tsx(no longer needed)Technical Details
<Outlet />on/delete) to state-based (useState+ prop-controlled dialog)useNavigate,Outlet) removed from view componentsGeneralPageaccepts an optionalonDeleteSuccesscallback; defaults to redirecting to originOrganizationProfilecontinues working via the thin wrapper ingeneral/index.tsx