Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR upgrades React and TypeScript types from v18 to v19 in the client-demo app, introduces new organization settings UI components (ImageUpload, ViewContainer, ViewHeader, GeneralView, DeleteOrganizationDialog) to the SDK, adds a Settings page with nested routing to the app, updates package dependencies, and modifies SheetFooter styling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4a8fab5-aa99-40ce-bd99-938852a3637f
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
web/apps/client-demo/package.jsonweb/apps/client-demo/src/App.tsxweb/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Home.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/General.tsxweb/apps/client-demo/src/styles.cssweb/sdk/admin/components/SheetFooter.tsxweb/sdk/package.jsonweb/sdk/react/components/image-upload/image-upload.module.cssweb/sdk/react/components/image-upload/image-upload.tsxweb/sdk/react/components/image-upload/index.tsweb/sdk/react/components/view-container/index.tsweb/sdk/react/components/view-container/view-container.module.cssweb/sdk/react/components/view-container/view-container.tsxweb/sdk/react/components/view-header/index.tsweb/sdk/react/components/view-header/view-header.tsxweb/sdk/react/index.tsweb/sdk/react/views-new/general/components/delete-organization-dialog.module.cssweb/sdk/react/views-new/general/components/delete-organization-dialog.tsxweb/sdk/react/views-new/general/general-view.module.cssweb/sdk/react/views-new/general/general-view.tsxweb/sdk/react/views-new/general/index.tsweb/sdk/tsup.config.ts
| <Link | ||
| to={`/${org.name}/settings`} | ||
| data-test-id={`[organization-new-ui-link-${org.name}]`} | ||
| > |
There was a problem hiding this comment.
Use a stable, URL-safe identifier in the settings link path.
Line 64 currently builds the route from org.name, which is not guaranteed unique or path-safe. This can produce broken or ambiguous navigation.
💡 Suggested fix
- <Link
- to={`/${org.name}/settings`}
- data-test-id={`[organization-new-ui-link-${org.name}]`}
- >
+ <Link
+ to={`/${encodeURIComponent(org.id)}/settings`}
+ data-test-id={`[organization-new-ui-link-${org.id}]`}
+ >📝 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.
| <Link | |
| to={`/${org.name}/settings`} | |
| data-test-id={`[organization-new-ui-link-${org.name}]`} | |
| > | |
| <Link | |
| to={`/${encodeURIComponent(org.id)}/settings`} | |
| data-test-id={`[organization-new-ui-link-${org.id}]`} | |
| > |
| useEffect(() => { | ||
| if (!orgId || organizations.length === 0) return; | ||
| const org = organizations.find(_org => _org.id === orgId || _org.name === orgId); | ||
| if (org && activeOrganization?.id !== org.id) { | ||
| setActiveOrganization(org); | ||
| } | ||
| }, [orgId, organizations, activeOrganization?.id, setActiveOrganization]); |
There was a problem hiding this comment.
Handle unmatched orgId explicitly to avoid stale organization context.
On Line 18–Line 21, when no org matches orgId, activeOrganization remains whatever was previously selected. That can render/update the wrong org in nested settings.
Proposed fix
+ const matchedOrg = organizations.find(
+ _org => _org.id === orgId || _org.name === orgId
+ );
+
useEffect(() => {
if (!orgId || organizations.length === 0) return;
- const org = organizations.find(_org => _org.id === orgId || _org.name === orgId);
- if (org && activeOrganization?.id !== org.id) {
- setActiveOrganization(org);
+ if (matchedOrg && activeOrganization?.id !== matchedOrg.id) {
+ setActiveOrganization(matchedOrg);
}
- }, [orgId, organizations, activeOrganization?.id, setActiveOrganization]);
+ }, [orgId, organizations.length, matchedOrg, activeOrganization?.id, setActiveOrganization]);
+
+ if (orgId && organizations.length > 0 && !matchedOrg) {
+ return <Navigate to="/" replace />;
+ }| export function SheetFooter({ children }: SheetFooterProps) { | ||
| // @ts-ignore | ||
| return <Flex style={{ ...styles.footer, ...css }}>{children}</Flex>; | ||
| return <Flex style={styles.footer}>{children}</Flex>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect current implementation
sed -n '1,120p' web/sdk/admin/components/SheetFooter.tsx
# Check whether this pattern appears elsewhere (ts-ignore + Flex style usage)
rg -n -C2 '@ts-ignore|<Flex[^>]*\bstyle=' web/sdk/admin/components web/sdk/admin/views
# Compare with existing Flex css-prop usage in nearby code
rg -n -C1 '<Flex[^>]*\bcss=' web/sdk/admin/components web/sdk/admin/viewsRepository: raystack/frontier
Length of output: 15370
🏁 Script executed:
# Inspect SheetHeader.tsx to see how it handles styles without `@ts-ignore`
cat -n web/sdk/admin/components/SheetHeader.tsx
# Check the type definitions for Flex component from apsara
rg -A 10 "export.*Flex.*Props|interface.*FlexProps|type.*FlexProps" --type tsx --type tsRepository: raystack/frontier
Length of output: 1126
Remove @ts-ignore and type the styles object with CSSProperties.
The companion component SheetHeader.tsx in the same directory uses the exact same pattern without type suppression by importing CSSProperties and explicitly typing the styles object. Apply the same approach here to eliminate the suppression.
Proposed fix
-import React from "react";
+import React, { type CSSProperties } from "react";
import { Flex } from "@raystack/apsara";
type SheetFooterProps = {
children?: React.ReactNode;
};
export function SheetFooter({ children }: SheetFooterProps) {
- // `@ts-ignore`
return <Flex style={styles.footer}>{children}</Flex>;
}
-const styles = {
+const styles: { footer: CSSProperties } = {
footer: {
bottom: 0,
left: 0,| const height = ((crop?.height || 0) * image.height) / 100; | ||
| const width = ((crop?.width || 0) * image.width) / 100; | ||
| const x = ((crop?.x || 0) * image.width) / 100; | ||
| const y = ((crop?.y || 0) * image.width) / 100; | ||
|
|
There was a problem hiding this comment.
Fix Y-axis crop calculation (uses width instead of height).
Line 62 computes y using image.width; this skews vertical crop positioning.
Proposed fix
- const y = ((crop?.y || 0) * image.width) / 100;
+ const y = ((crop?.y || 0) * image.height) / 100;📝 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 height = ((crop?.height || 0) * image.height) / 100; | |
| const width = ((crop?.width || 0) * image.width) / 100; | |
| const x = ((crop?.x || 0) * image.width) / 100; | |
| const y = ((crop?.y || 0) * image.width) / 100; | |
| const height = ((crop?.height || 0) * image.height) / 100; | |
| const width = ((crop?.width || 0) * image.width) / 100; | |
| const x = ((crop?.x || 0) * image.width) / 100; | |
| const y = ((crop?.y || 0) * image.height) / 100; |
| function onFileChange(e: ChangeEvent<HTMLInputElement>) { | ||
| const files = e.target.files || []; | ||
| if (files.length > 0) { | ||
| const file = files[0]; | ||
| const imageUrl = URL.createObjectURL(file); | ||
| setImgSrc(imageUrl); | ||
| setShowCropDialog(true); | ||
| e.target.files = null; | ||
| } |
There was a problem hiding this comment.
Revoke object URLs after upload flow to prevent memory leaks.
Line 180 creates a blob URL, but it’s never revoked on subsequent uploads or unmount.
Proposed fix
-import { useRef, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
...
const [imgSrc, setImgSrc] = useState('');
+
+ useEffect(() => {
+ return () => {
+ if (imgSrc.startsWith('blob:')) URL.revokeObjectURL(imgSrc);
+ };
+ }, [imgSrc]);
...
function onFileChange(e: ChangeEvent<HTMLInputElement>) {
const files = e.target.files || [];
if (files.length > 0) {
const file = files[0];
const imageUrl = URL.createObjectURL(file);
- setImgSrc(imageUrl);
+ setImgSrc(prev => {
+ if (prev.startsWith('blob:')) URL.revokeObjectURL(prev);
+ return imageUrl;
+ });
setShowCropDialog(true);
e.target.files = 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.
| function onFileChange(e: ChangeEvent<HTMLInputElement>) { | |
| const files = e.target.files || []; | |
| if (files.length > 0) { | |
| const file = files[0]; | |
| const imageUrl = URL.createObjectURL(file); | |
| setImgSrc(imageUrl); | |
| setShowCropDialog(true); | |
| e.target.files = null; | |
| } | |
| function onFileChange(e: ChangeEvent<HTMLInputElement>) { | |
| const files = e.target.files || []; | |
| if (files.length > 0) { | |
| const file = files[0]; | |
| const imageUrl = URL.createObjectURL(file); | |
| setImgSrc(prev => { | |
| if (prev.startsWith('blob:')) URL.revokeObjectURL(prev); | |
| return imageUrl; | |
| }); | |
| setShowCropDialog(true); | |
| e.target.files = null; | |
| } | |
| } |
| const [isAcknowledged, setIsAcknowledged] = useState(false); | ||
|
|
||
| const { mutateAsync: deleteOrganization } = useMutation( | ||
| FrontierServiceQueries.deleteOrganization | ||
| ); | ||
|
|
||
| const { | ||
| register, | ||
| handleSubmit, | ||
| watch, | ||
| setError, | ||
| formState: { errors, isSubmitting } | ||
| } = useForm({ | ||
| resolver: yupResolver(deleteOrgSchema) | ||
| }); | ||
|
|
||
| const deleteTitle = watch('title') ?? ''; |
There was a problem hiding this comment.
Reset confirmation state when dialog is reopened.
isAcknowledged and typed title persist across open/close cycles, which is risky for a destructive flow.
Proposed fix
-import { useState } from 'react';
+import { useEffect, useState } from 'react';
...
const {
register,
handleSubmit,
watch,
+ reset,
setError,
formState: { errors, isSubmitting }
} = useForm({
resolver: yupResolver(deleteOrgSchema)
});
+
+ useEffect(() => {
+ if (open) {
+ reset({ title: '' });
+ setIsAcknowledged(false);
+ }
+ }, [open, reset]);📝 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 [isAcknowledged, setIsAcknowledged] = useState(false); | |
| const { mutateAsync: deleteOrganization } = useMutation( | |
| FrontierServiceQueries.deleteOrganization | |
| ); | |
| const { | |
| register, | |
| handleSubmit, | |
| watch, | |
| setError, | |
| formState: { errors, isSubmitting } | |
| } = useForm({ | |
| resolver: yupResolver(deleteOrgSchema) | |
| }); | |
| const deleteTitle = watch('title') ?? ''; | |
| const [isAcknowledged, setIsAcknowledged] = useState(false); | |
| const { mutateAsync: deleteOrganization } = useMutation( | |
| FrontierServiceQueries.deleteOrganization | |
| ); | |
| const { | |
| register, | |
| handleSubmit, | |
| watch, | |
| reset, | |
| setError, | |
| formState: { errors, isSubmitting } | |
| } = useForm({ | |
| resolver: yupResolver(deleteOrgSchema) | |
| }); | |
| useEffect(() => { | |
| if (open) { | |
| reset({ title: '' }); | |
| setIsAcknowledged(false); | |
| } | |
| }, [open, reset]); | |
| const deleteTitle = watch('title') ?? ''; |
| onError: (error: Error) => { | ||
| toastManager.add({ | ||
| title: 'Something went wrong', | ||
| description: error?.message || 'Failed to update', | ||
| type: 'error' | ||
| }); |
There was a problem hiding this comment.
Avoid double error toasts on update failure.
Update failures are handled in both mutation onError and the catch block, producing duplicate error notifications.
Proposed fix
- async function onUpdateSubmit(data: FormData) {
- if (!organization?.id) return;
-
- try {
- const req = create(UpdateOrganizationRequestSchema, {
- id: organization.id,
- body: {
- title: data.title,
- name: data.name,
- avatar: data.avatar
- }
- });
- await updateOrganization(req);
- } catch (error: unknown) {
- toastManager.add({
- title: 'Something went wrong',
- description:
- error instanceof Error
- ? error.message
- : `Failed to update ${t.organization({ case: 'lower' })}`,
- type: 'error'
- });
- }
- }
+ async function onUpdateSubmit(data: FormData) {
+ if (!organization?.id) return;
+ const req = create(UpdateOrganizationRequestSchema, {
+ id: organization.id,
+ body: {
+ title: data.title,
+ name: data.name,
+ avatar: data.avatar
+ }
+ });
+ await updateOrganization(req);
+ }Also applies to: 159-167
rohanchkrabrty
changed the title
1 participant
Summary
views-new/general/using@raystack/apsara-v1components@raystack/apsara-v1is just an alias for the@raystack/apsara@v1.0.0.rc.1. This is needed temporarily to not break existing pages.ViewContainer(page layout wrapper),ViewHeader(page title/description),ImageUpload(avatar/image upload with crop dialog)DeleteOrganizationDialogwith self-contained delete logic (form, mutation, validation)/:orgId/settings) with Apsara Sidebar and nested routes