Add success Dialog and UI improvment #465
Conversation
|
|
@mit-27 is attempting to deploy a commit to the Panora Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates span multiple components and applications, focusing on enhancing UI/UX and functionality. Key changes include layout adjustments in login pages, theme management improvements, new logout and profile functionalities, and OAuth flow enhancements with success notifications. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant App
participant OAuthService
participant UI
User->>App: Initiates OAuth Flow
App->>OAuthService: Redirect to OAuth Provider
OAuthService-->>User: OAuth Authentication
User-->>OAuthService: Grant Access
OAuthService-->>App: OAuth Token
App->>UI: Display Success Notification
UI-->>User: OAuth Success Message
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Outside diff range and nitpick comments (10)
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (3)
Line range hint
72-72: AddonWindowCloseto the dependency array ofuseEffectto ensure it captures the latest state.- }, [startFlow, isReady, open]); + }, [startFlow, isReady, open, onWindowClose]);
Line range hint
137-137: Provide an alternative text for the image for accessibility.- <img src={item.logoPath} className="w-8 h-8 rounded-lg" /> + <img src={item.logoPath} alt={`Logo of ${item.name}`} className="w-8 h-8 rounded-lg" />
Line range hint
128-154: Consider using optional chaining to safely access nested properties.- {item.description!.substring(0, 300)} + {item.description?.substring(0, 300)}apps/magic-link/src/lib/ProviderModal.tsx (3)
Line range hint
24-24: Avoid non-null assertions. Consider adding proper null checks or default values.- providerName: selectedProvider?.provider!, - vertical: selectedProvider?.category!, + providerName: selectedProvider?.provider ?? 'defaultProvider', + vertical: selectedProvider?.category ?? 'defaultCategory',Also applies to: 27-27, 95-96
Line range hint
75-75: Use strict equality for comparisons.- if (selectedCategory == "All") { + if (selectedCategory === "All") {
Line range hint
199-199: Add a uniquekeyproperty for elements in a list.- <SelectItem key="All" value="All"> + <SelectItem key="category-all" value="All">apps/client-ts/src/components/shared/team-switcher.tsx (4)
Line range hint
112-112: Avoid non-null assertions. Consider adding proper null checks or default values.- id_user: profile!.id_user, + id_user: profile?.id_user ?? 'defaultUserId',
Line range hint
116-117: Specify a more specific type thanany.- success: (data: any) => { + success: (data: ProjectData) => {
Line range hint
247-247: Use strict equality for comparisons.- {showNewDialog.status == 0 ? + {showNewDialog.status === 0 ?
Line range hint
212-212: Avoid using the index of an array as a key property in React elements.- key={index} + key={`skeleton-${index}`}
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/embedded-catalog/react/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!pnpm-lock.yaml,!**/*.yaml
Files selected for processing (16)
- apps/client-ts/src/app/b2c/login/page.tsx (1 hunks)
- apps/client-ts/src/app/layout.tsx (1 hunks)
- apps/client-ts/src/components/Nav/main-nav-sm.tsx (5 hunks)
- apps/client-ts/src/components/Nav/main-nav.tsx (2 hunks)
- apps/client-ts/src/components/Nav/user-nav.tsx (2 hunks)
- apps/client-ts/src/components/RootLayout/index.tsx (3 hunks)
- apps/client-ts/src/components/shared/team-switcher.tsx (1 hunks)
- apps/embedded-catalog/react/src/components/Modal.tsx (1 hunks)
- apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (4 hunks)
- apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (3 hunks)
- apps/embedded-catalog/react/src/hooks/queries/useProjectConnectors.tsx (1 hunks)
- apps/embedded-catalog/react/src/hooks/useOAuth.tsx (2 hunks)
- apps/magic-link/src/App.tsx (1 hunks)
- apps/magic-link/src/components/Modal.tsx (1 hunks)
- apps/magic-link/src/hooks/useOAuth.ts (1 hunks)
- apps/magic-link/src/lib/ProviderModal.tsx (7 hunks)
Files skipped from review due to trivial changes (1)
- apps/magic-link/src/App.tsx
Additional Context Used
Biome (100)
apps/client-ts/src/app/b2c/login/page.tsx (5)
59-59: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
75-75: Avoid the words "image", "picture", or "photo" in img element alt text.
22-22: This hook does not specify all of its dependencies: router.replace
30-30: This hook does not specify all of its dependencies: mutate
30-30: This hook does not specify all of its dependencies: profile
apps/client-ts/src/components/Nav/main-nav-sm.tsx (12)
65-65: Use === instead of ==.
== is only allowed when comparing againstnull
65-65: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
65-65: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
71-71: Do not use template literals if interpolation and special-character handling are not needed.
87-87: Use a button element instead of an a element.
93-93: Use a button element instead of an a element.
100-100: Use a button element instead of an a element.
106-106: Use a button element instead of an a element.
117-117: Alternative text title element cannot be empty
117-117: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
121-121: Use a button element instead of an a element.
128-128: Use a button element instead of an a element.
apps/client-ts/src/components/Nav/main-nav.tsx (7)
38-38: Use a button element instead of an a element.
44-44: Use a button element instead of an a element.
50-50: Use a button element instead of an a element.
56-56: Use a button element instead of an a element.
67-67: Alternative text title element cannot be empty
67-67: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
3-4: Some named imports are only used as types.
apps/client-ts/src/components/RootLayout/index.tsx (9)
86-86: Do not use template literals if interpolation and special-character handling are not needed.
91-91: Use === instead of ==.
== is only allowed when comparing againstnull
91-91: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
91-91: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
103-106: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
111-111: Alternative text title element cannot be empty
116-116: Alternative text title element cannot be empty
117-117: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
30-30: This hook specifies more dependencies than necessary: refreshAccessToken
apps/client-ts/src/components/shared/team-switcher.tsx (9)
112-112: Forbidden non-null assertion.
116-116: Unexpected any. Specify a different type.
117-117: Unexpected any. Specify a different type.
122-122: Alternative text title element cannot be empty
122-122: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
247-247: Use === instead of ==.
== is only allowed when comparing againstnull
2-3: All these imports are only used as types.
57-58: All these imports are only used as types.
212-212: Avoid using the index of an array as key property in an element.
apps/embedded-catalog/react/src/components/Modal.tsx (3)
6-12: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
14-20: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
1-1: The default import is only used as a type.
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (12)
38-38: Template literals are preferred over string concatenation.
38-38: Template literals are preferred over string concatenation.
45-45: Forbidden non-null assertion.
46-46: Forbidden non-null assertion.
58-58: Forbidden non-null assertion.
106-106: Forbidden non-null assertion.
128-154: Change to an optional chain.
137-137: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
141-141: Forbidden non-null assertion.
144-144: Forbidden non-null assertion.
1-2: Some named imports are only used as types.
72-72: This hook does not specify all of its dependencies: onWindowClose
apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (7)
24-24: Template literals are preferred over string concatenation.
24-24: Template literals are preferred over string concatenation.
63-63: Forbidden non-null assertion.
77-77: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
81-81: Forbidden non-null assertion.
2-3: Some named imports are only used as types.
49-49: This hook does not specify all of its dependencies: onWindowClose
apps/embedded-catalog/react/src/hooks/queries/useProjectConnectors.tsx (1)
7-7: Unexpected any. Specify a different type.
apps/embedded-catalog/react/src/hooks/useOAuth.tsx (10)
45-45: Forbidden non-null assertion.
51-51: Template literals are preferred over string concatenation.
54-54: Declare variables separately
64-64: Template literals are preferred over string concatenation.
64-64: Forbidden non-null assertion.
64-64: Forbidden non-null assertion.
64-64: Forbidden non-null assertion.
64-64: Template literals are preferred over string concatenation.
64-64: Forbidden non-null assertion.
2-3: Some named imports are only used as types.
apps/magic-link/src/components/Modal.tsx (3)
7-13: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
15-21: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
1-2: The default import is only used as a type.
apps/magic-link/src/hooks/useOAuth.ts (9)
45-45: Forbidden non-null assertion.
51-51: Template literals are preferred over string concatenation.
54-54: Declare variables separately
64-64: Template literals are preferred over string concatenation.
64-64: Forbidden non-null assertion.
64-64: Forbidden non-null assertion.
64-64: Forbidden non-null assertion.
64-64: Template literals are preferred over string concatenation.
64-64: Forbidden non-null assertion.
apps/magic-link/src/lib/ProviderModal.tsx (13)
24-24: Forbidden non-null assertion.
24-24: Forbidden non-null assertion.
27-27: Forbidden non-null assertion.
75-75: Use === instead of ==.
== is only allowed when comparing againstnull
95-95: Forbidden non-null assertion.
96-96: Forbidden non-null assertion.
98-98: Template literals are preferred over string concatenation.
98-98: Template literals are preferred over string concatenation.
141-141: Forbidden non-null assertion.
205-205: Forbidden non-null assertion.
2-3: Some named imports are only used as types.
120-120: This hook does not specify all of its dependencies: onWindowClose
199-199: Missing key property for this element in iterable.
Additional comments not posted (6)
apps/embedded-catalog/react/src/hooks/queries/useProjectConnectors.tsx (1)
8-8: Good error handling in the API call.apps/client-ts/src/app/layout.tsx (1)
25-25: Good update to use system settings for theme management.apps/client-ts/src/components/Nav/main-nav.tsx (1)
22-22: Good UI improvements with reduced padding and concise naming.Also applies to: 66-66
apps/client-ts/src/app/b2c/login/page.tsx (1)
57-61: Good layout improvements for the login and account creation page.apps/client-ts/src/components/Nav/user-nav.tsx (2)
27-27: Ensure proper import usage and check for potential unused imports.Also applies to: 29-29
9-9: Ensure proper import usage and check for potential unused imports.Also applies to: 14-14, 16-16, 17-17, 18-18
| return useQuery({ | ||
| queryKey: ['project-connectors', id], | ||
| queryFn: async (): Promise<any> => { | ||
| const response = await fetch(`${config.API_URL}/project-connectors?projectId=${id}`); | ||
| const response = await fetch(`${API_URL}/project-connectors?projectId=${id}`); |
There was a problem hiding this comment.
Specify a more detailed type instead of any for better type safety.
- queryFn: async (): Promise<any> => {
+ queryFn: async (): Promise<ProjectConnector[]> => {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.
| const response = await fetch(`${API_URL}/project-connectors?projectId=${id}`); | |
| const response = await fetch(`${API_URL}/project-connectors?projectId=${id}`); | |
| queryFn: async (): Promise<ProjectConnector[]> => { |
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | ||
| : ''; |
There was a problem hiding this comment.
Use template literals for URL construction to improve readability.
- const returnUrlWithWindow = (typeof window !== 'undefined')
- ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '')
- : '';
+ const returnUrlWithWindow = (typeof window !== 'undefined')
+ ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}`
+ : '';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.
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | |
| : ''; | |
| ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}` | |
| : ''; |
| providerName: selectedProvider?.provider!, | ||
| vertical: selectedProvider?.category! as ConnectorCategory, |
There was a problem hiding this comment.
Avoid non-null assertions. Consider adding proper null checks or default values.
- providerName: selectedProvider?.provider!,
- vertical: selectedProvider?.category! as ConnectorCategory,
+ providerName: selectedProvider?.provider ?? 'defaultProvider',
+ vertical: (selectedProvider?.category ?? 'defaultCategory') as ConnectorCategory,Also applies to: 58-58
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.
| providerName: selectedProvider?.provider!, | |
| vertical: selectedProvider?.category! as ConnectorCategory, | |
| providerName: selectedProvider?.provider ?? 'defaultProvider', | |
| vertical: (selectedProvider?.category ?? 'defaultCategory') as ConnectorCategory, |
| } | ||
|
|
||
| const width = 600, height = 600; |
There was a problem hiding this comment.
Declare variables separately for clarity.
- const width = 600, height = 600;
+ const width = 600;
+ const height = 600;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.
| const width = 600, height = 600; | |
| const width = 600; | |
| const height = 600; |
| const openModal = async (onWindowClose: () => void) => { | ||
| const apiUrl = optionalApiUrl? optionalApiUrl : config.API_URL!; | ||
| const apiUrl = optionalApiUrl ? optionalApiUrl : config.API_URL!; |
There was a problem hiding this comment.
Avoid using non-null assertions.
- const apiUrl = optionalApiUrl ? optionalApiUrl : config.API_URL!;
+ const apiUrl = optionalApiUrl || config.API_URL;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.
| const apiUrl = optionalApiUrl ? optionalApiUrl : config.API_URL!; | |
| const apiUrl = optionalApiUrl || config.API_URL; |
| }); | ||
|
|
||
| if (!authUrl) { | ||
| throw new Error("Auth Url is Invalid " + authUrl); |
There was a problem hiding this comment.
Use template literals for string concatenation.
- throw new Error("Auth Url is Invalid " + authUrl);
+ throw new Error(`Auth Url is Invalid ${authUrl}`);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.
| throw new Error("Auth Url is Invalid " + authUrl); | |
| throw new Error(`Auth Url is Invalid ${authUrl}`); |
| <div | ||
| onClick={() => setOpen(false)} | ||
| className={` | ||
| fixed inset-0 flex justify-center items-center transition-colors | ||
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | ||
| `} | ||
| > |
There was a problem hiding this comment.
Add keyboard event handlers for accessibility.
+ onKeyUp={(e) => { if (e.key === 'Escape') setOpen(false); }}Also applies to: 15-21
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.
| <div | |
| onClick={() => setOpen(false)} | |
| className={` | |
| fixed inset-0 flex justify-center items-center transition-colors | |
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | |
| `} | |
| > | |
| <div | |
| onClick={() => setOpen(false)} | |
| onKeyUp={(e) => { if (e.key === 'Escape') setOpen(false); }} | |
| className={` | |
| fixed inset-0 flex justify-center items-center transition-colors | |
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | |
| `} | |
| > |
| <img src="/logo.png" className='w-14' /> | ||
| <Tabs defaultValue="login" className="w-[400px]"> | ||
| </div> |
There was a problem hiding this comment.
Add alt text to the logo image for accessibility.
- <img src="/logo.png" className='w-14' />
+ <img src="/logo.png" className='w-14' alt="Panora Logo" />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.
| </div> | |
| <img src="/logo.png" className='w-14' alt="Panora Logo" /> |
| <img src="./../../../../public/logo.png" className="w-10 mr-1"/><span className="font-bold">Panora.</span> | ||
| <Link href="/" className="flex flex-row" onClick={() => setOpen(false)}> | ||
| {/* <img src="./../../../../public/logo.png" className="w-10 mr-1"/><span className="font-bold">Panora.</span> */} | ||
| {theme == "light" ? <img src="/logo-panora-black.png" className='w-12' /> : <img src="/logo-panora-white-hq.png" className='w-12' />} |
There was a problem hiding this comment.
Provide alt text for images and use strict equality.
- {theme == "light" ? <img src="/logo-panora-black.png" className='w-12' /> : <img src="/logo-panora-white-hq.png" className='w-12' />}
+ {theme === "light" ? <img src="/logo-panora-black.png" alt="Panora Logo Black" className='w-12' /> : <img src="/logo-panora-white-hq.png" alt="Panora Logo White" className='w-12' />}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.
| {theme == "light" ? <img src="/logo-panora-black.png" className='w-12' /> : <img src="/logo-panora-white-hq.png" className='w-12' />} | |
| {theme === "light" ? <img src="/logo-panora-black.png" alt="Panora Logo Black" className='w-12' /> : <img src="/logo-panora-white-hq.png" alt="Panora Logo White" className='w-12' />} |
| <div className='flex items-center justify-center min-w-full'> | ||
| <div className="hidden lg:block pt-5"> | ||
| <Link href='/'> | ||
| {theme == "light" ? <img src="/logo-panora-black.png" className='w-14' /> : <img src="/logo-panora-white-hq.png" className='w-14' />} |
There was a problem hiding this comment.
Provide alt text for images and use strict equality.
- {theme == "light" ? <img src="/logo-panora-black.png" className='w-14' /> : <img src="/logo-panora-white-hq.png" className='w-14' />}
+ {theme === "light" ? <img src="/logo-panora-black.png" alt="Panora Logo Black" className='w-14' /> : <img src="/logo-panora-white-hq.png" alt="Panora Logo White" className='w-14' />}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.
| {theme == "light" ? <img src="/logo-panora-black.png" className='w-14' /> : <img src="/logo-panora-white-hq.png" className='w-14' />} | |
| {theme === "light" ? <img src="/logo-panora-black.png" alt="Panora Logo Black" className='w-14' /> : <img src="/logo-panora-white-hq.png" alt="Panora Logo White" className='w-14' />} |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (1)
Line range hint
62-62: Ensure accessibility by providing text alternatives for images.- <img src={img} className="w-8 h-8 rounded-lg" /> + <img src={img} className="w-8 h-8 rounded-lg" alt="Provider logo" /> - <img src={currentProviderLogoURL} alt={selectedProvider?.provider} /> + <img src={currentProviderLogoURL} alt={`${selectedProvider?.provider} logo`} />Also applies to: 76-76, 80-80
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (2)
Line range hint
71-71: Specify all dependencies in theuseEffecthook to avoid potential bugs.- }, [startFlow, isReady, open]); + }, [startFlow, isReady, open, onWindowClose]);
Line range hint
136-136: Ensure accessibility by providing text alternatives for images.- <img src={item.logoPath} className="w-8 h-8 rounded-lg" /> + <img src={item.logoPath} className="w-8 h-8 rounded-lg" alt="Provider logo" /> - <img src={currentProviderLogoURL} alt={selectedProvider?.provider} /> + <img src={currentProviderLogoURL} alt={`${selectedProvider?.provider} logo`} />Also applies to: 140-140, 143-143
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (4 hunks)
- apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (2 hunks)
- apps/embedded-catalog/react/src/index.tsx (1 hunks)
Additional Context Used
Biome (21)
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (12)
37-37: Template literals are preferred over string concatenation.
37-37: Template literals are preferred over string concatenation.
44-44: Forbidden non-null assertion.
45-45: Forbidden non-null assertion.
57-57: Forbidden non-null assertion.
105-105: Forbidden non-null assertion.
127-153: Change to an optional chain.
136-136: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
140-140: Forbidden non-null assertion.
143-143: Forbidden non-null assertion.
1-2: Some named imports are only used as types.
71-71: This hook does not specify all of its dependencies: onWindowClose
apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (7)
23-23: Template literals are preferred over string concatenation.
23-23: Template literals are preferred over string concatenation.
62-62: Forbidden non-null assertion.
76-76: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
80-80: Forbidden non-null assertion.
2-3: Some named imports are only used as types.
48-48: This hook does not specify all of its dependencies: onWindowClose
apps/embedded-catalog/react/src/index.tsx (2)
1-2: Some named imports are only used as types.
3-4: Some named imports are only used as types.
Additional comments not posted (4)
apps/embedded-catalog/react/src/index.tsx (2)
7-11: LGTM! Consider separating type-only imports for clarity.- import PanoraIntegrationCard, { ProviderCardProp } from "./components/PanoraIntegrationCard"; + import PanoraIntegrationCard from "./components/PanoraIntegrationCard"; + import type { ProviderCardProp } from "./components/PanoraIntegrationCard";
16-20: LGTM! Consider separating type-only imports for clarity.- import PanoraDynamicCatalog, { DynamicCardProp } from "./components/PanoraDynamicCatalog"; + import PanoraDynamicCatalog from "./components/PanoraDynamicCatalog"; + import type { DynamicCardProp } from "./components/PanoraDynamicCatalog";apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx (1)
7-8: Consider separating type-only imports for clarity.- import { CONNECTORS_METADATA, ConnectorCategory } from '@panora/shared'; + import { CONNECTORS_METADATA } from '@panora/shared'; + import type { ConnectorCategory } from '@panora/shared';apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (1)
2-2: Consider separating type-only imports for clarity.- import {providersArray, ConnectorCategory, categoryFromSlug, Provider,CONNECTORS_METADATA} from '@panora/shared'; + import {providersArray, categoryFromSlug, CONNECTORS_METADATA} from '@panora/shared'; + import type { ConnectorCategory, Provider } from '@panora/shared';
| }); | ||
|
|
||
| const onWindowClose = () => { | ||
| setLoading(false); | ||
| return; | ||
| setStartFlow(false); | ||
| // return; | ||
| } | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Specify all dependencies in the useEffect hook to avoid potential bugs.
- }, [startFlow, isReady, open]);
+ }, [startFlow, isReady, open, onWindowClose]);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.
| useEffect(() => { | |
| useEffect(() => { | |
| // effect logic here | |
| }, [startFlow, isReady, open, onWindowClose]); |
| const [startFlow, setStartFlow] = useState(false); | ||
| const returnUrlWithWindow = (typeof window !== 'undefined') | ||
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') |
There was a problem hiding this comment.
Use template literals for string concatenation to improve readability.
- ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '')
+ ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}`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.
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | |
| ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}` |
| const [openSuccessDialog,setOpenSuccessDialog] = useState<boolean>(false); | ||
| const [currentProviderLogoURL,setCurrentProviderLogoURL] = useState<string>('') | ||
| const returnUrlWithWindow = (typeof window !== 'undefined') | ||
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') |
There was a problem hiding this comment.
Use template literals for URL construction to improve readability.
- ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '')
+ ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}`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.
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | |
| ? `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}` |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Summary: Add Success Dialog and UI Improvement
- OAuth Success Dialog: Introduced a success dialog that appears after successful OAuth authentication, enhancing user feedback and simplifying the interface by managing the return URL internally.
- UI Enhancements: Made various UI improvements across multiple components, including layout adjustments, theme-based logo rendering, and updated navigation items for better user experience.
- Theme Management: Modified the default theme setting from 'light' to 'system', allowing the application to adapt to the user's system theme settings.
- New Modal Component: Added a new modal component with smooth transition effects and backdrop, improving the overall user interface.
Notes:
- Potential Pitfall: The change in theme management might affect the application's appearance based on users' system settings, which could lead to unexpected visual discrepancies.
- Code Reuse Opportunity: The new modal component and success dialog can be reused in other parts of the application to maintain consistency in user feedback mechanisms.
Comments
apps/client-ts/src/app/layout.tsx
- Line 22: Changing the default theme to 'system' will now respect the user's system theme settings. Ensure this aligns with the intended user experience.
apps/client-ts/src/components/Nav/main-nav-sm.tsx
- Line 66: Remove commented-out code if it is no longer needed to keep the codebase clean.
apps/client-ts/src/components/Nav/user-nav.tsx
- Line 50: Remove the commented-out Button component if it is no longer required.
- Line 57: Remove the commented-out DropdownMenuLabel if it is no longer needed.
- Line 60: Remove the commented-out DropdownMenuGroup if it is no longer needed.
apps/embedded-catalog/react/package.json
- Line 22: Added
lucide-reactdependency. Ensure that all components using this library are correctly updated and tested.
apps/embedded-catalog/react/src/components/Modal.tsx
- Line 5: The 'setOpen' function should be typed more explicitly, e.g., 'setOpen: React.Dispatch<React.SetStateAction>'.
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx
- Line 18: Remove the commented-out
returnUrlprop if it is no longer needed. - Line 153: Check if
selectedProvider?.provideris correctly set before using it insetLoading.
apps/embedded-catalog/react/src/components/PanoraIntegrationCard.tsx
- Line 28: The
returnUrlWithWindowvariable should be defined outside the component to avoid redefinition on each render. - Line 50: The
onWindowClosefunction should be defined outside the component to avoid redefinition on each render.
apps/embedded-catalog/react/src/hooks/useOAuth.tsx
- Line 33: The
clearExistingIntervalfunction should be defined before its first use in theuseEffectcleanup function for better readability. - Line 55: The error message 'Auth Url is Invalid' should not concatenate the
authUrlvariable directly. Consider using template literals for better readability:throw new Error(Auth Url is Invalid: ${authUrl}); - Line 68: The
clearExistingIntervalfunction is called twice withfalseas an argument. Consider refactoring to avoid redundancy. - Line 77: The
try...catchblock should have a more specific error handling mechanism rather than just logging the error to the console.
apps/magic-link/src/App.tsx
- Line 7: Good use of Tailwind CSS classes to center the content.
apps/magic-link/src/components/Modal.tsx
- Line 14: The
bg-[#1d1d1d]class could be replaced with a more descriptive class name or a variable to improve readability and maintainability. - Line 18: The commented-out button code can be removed if it is no longer needed to keep the codebase clean.
apps/magic-link/src/hooks/useOAuth.ts
- Line 41: Remove the redundant
!afterconfig.API_URLsince TypeScript already ensuresconfig.API_URLis defined. - Line 72: Remove the redundant
as stringcast forauthUrlsinceconstructAuthUrlshould return a string.
apps/magic-link/src/lib/ProviderModal.tsx
- Line 94: The
returnUrlshould be encoded to ensure it handles special characters correctly. Consider usingencodeURIComponent(window.location.href). - Line 168: Add a null check for
CONNECTORS_METADATA[category.toLowerCase()]to avoid potential runtime errors if the category is not found.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (3)
Line range hint
126-152: Change to an optional chain to safely access nested properties.- selectedProvider?.provider! + selectedProvider?.provider ?? 'defaultProvider'
Line range hint
135-135: Provide a text alternative through the alt attribute for images.- <img src={item.logoPath} className="w-8 h-8 rounded-lg" /> + <img src={item.logoPath} alt={`Logo of ${item.name}`} className="w-8 h-8 rounded-lg" />
Line range hint
70-70: Specify all dependencies in the useEffect hook to ensure correct re-rendering.- }, [startFlow, isReady, open]); + }, [startFlow, isReady, open, onWindowClose]);
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/client-ts/src/components/Configuration/LinkedUsers/columns.tsx (1 hunks)
- apps/client-ts/src/components/Nav/main-nav-sm.tsx (5 hunks)
- apps/embedded-catalog/react/src/components/Modal.tsx (1 hunks)
- apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (4 hunks)
- apps/embedded-catalog/react/src/hooks/useOAuth.tsx (2 hunks)
Additional Context Used
Biome (41)
apps/client-ts/src/components/Configuration/LinkedUsers/columns.tsx (4)
9-9: Unexpected any. Specify a different type.
22-22: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
1-1: All these imports are only used as types.
1-2: All these imports are only used as types.
apps/client-ts/src/components/Nav/main-nav-sm.tsx (12)
64-64: Use === instead of ==.
== is only allowed when comparing againstnull
64-64: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
64-64: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
70-70: Do not use template literals if interpolation and special-character handling are not needed.
74-74: Use a button element instead of an a element.
80-80: Use a button element instead of an a element.
87-87: Use a button element instead of an a element.
93-93: Use a button element instead of an a element.
104-104: Alternative text title element cannot be empty
104-104: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
108-108: Use a button element instead of an a element.
115-115: Use a button element instead of an a element.
apps/embedded-catalog/react/src/components/Modal.tsx (3)
5-11: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
13-19: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
1-1: All these imports are only used as types.
apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (12)
37-37: Template literals are preferred over string concatenation.
37-37: Template literals are preferred over string concatenation.
44-44: Forbidden non-null assertion.
45-45: Forbidden non-null assertion.
56-56: Forbidden non-null assertion.
104-104: Forbidden non-null assertion.
126-152: Change to an optional chain.
135-135: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
139-139: Forbidden non-null assertion.
142-142: Forbidden non-null assertion.
1-2: Some named imports are only used as types.
70-70: This hook does not specify all of its dependencies: onWindowClose
apps/embedded-catalog/react/src/hooks/useOAuth.tsx (10)
47-47: Forbidden non-null assertion.
56-56: Declare variables separately
66-66: Template literals are preferred over string concatenation.
66-66: Forbidden non-null assertion.
66-66: Forbidden non-null assertion.
66-66: Forbidden non-null assertion.
66-66: Template literals are preferred over string concatenation.
66-66: Forbidden non-null assertion.
2-3: Some named imports are only used as types.
31-31: This hook does not specify all of its dependencies: clearExistingInterval
Additional comments not posted (10)
apps/embedded-catalog/react/src/components/Modal.tsx (2)
13-19: Ensure keyboard accessibility by adding keyboard event handlers.
1-1: Optimize imports by removing unnecessary React import.apps/client-ts/src/components/Configuration/LinkedUsers/columns.tsx (1)
Line range hint
1-1: Optimize imports by removing unnecessary imports used only as types.Also applies to: 1-2
apps/embedded-catalog/react/src/hooks/useOAuth.tsx (4)
47-47: Avoid using non-null assertions. Use logical OR for default values.
56-56: Declare variables separately for clarity.
66-66: Use template literals for URL construction to improve readability and avoid non-null assertions.
2-3: Optimize imports by removing unnecessary imports used only as types.apps/client-ts/src/components/Nav/main-nav-sm.tsx (1)
64-64: Use strict equality for comparisons and provide alt text for images.apps/embedded-catalog/react/src/components/PanoraDynamicCatalog.tsx (2)
37-37: Use template literals for URL construction to improve readability.
44-44: Avoid non-null assertions. Consider adding proper null checks or default values.Also applies to: 45-45, 56-56, 104-104
| <div | ||
| onClick={() => setOpen(false)} | ||
| className={` | ||
| fixed inset-0 flex justify-center items-center transition-colors | ||
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | ||
| `} | ||
| > |
There was a problem hiding this comment.
Ensure keyboard accessibility by adding keyboard event handlers.
+ onKeyUp={(e) => { if (e.key === 'Escape') setOpen(false); }}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.
| <div | |
| onClick={() => setOpen(false)} | |
| className={` | |
| fixed inset-0 flex justify-center items-center transition-colors | |
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | |
| `} | |
| > | |
| <div | |
| onClick={() => setOpen(false)} | |
| onKeyUp={(e) => { if (e.key === 'Escape') setOpen(false); }} | |
| className={` | |
| fixed inset-0 flex justify-center items-center transition-colors | |
| ${open ? "visible bg-black/20 backdrop-blur" : "invisible"} | |
| `} | |
| > |
| }; | ||
|
|
||
| return ( | ||
| <div className="cursor-pointer" onClick={handleCopyLinkedUserId}> |
There was a problem hiding this comment.
Ensure keyboard accessibility by adding keyboard event handlers.
+ onKeyUp={(e) => { if (e.key === 'Enter') handleCopyLinkedUserId(); }}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.
| <div className="cursor-pointer" onClick={handleCopyLinkedUserId}> | |
| <div className="cursor-pointer" onClick={handleCopyLinkedUserId} onKeyUp={(e) => { if (e.key === 'Enter') handleCopyLinkedUserId(); }}> |
| import { toast } from "sonner"; | ||
|
|
||
|
|
||
| const LinkedUserIdComponent = ({ row } : {row:any}) => { |
There was a problem hiding this comment.
Specify a more precise type instead of any for the row parameter.
- const LinkedUserIdComponent = ({ row } : {row:any}) => {
+ const LinkedUserIdComponent = ({ row } : {row: { getValue: (key: string) => string }}) => { 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.
| const LinkedUserIdComponent = ({ row } : {row:any}) => { | |
| const LinkedUserIdComponent = ({ row } : {row: { getValue: (key: string) => string }}) => { |
| clearInterval(intervalRef.current); | ||
| intervalRef.current = null; | ||
| } | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Specify all dependencies in the useEffect hook to ensure correct re-rendering.
- }, []);
+ }, [clearExistingInterval]);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.
| useEffect(() => { | |
| useEffect(() => { | |
| }, [clearExistingInterval]); |
| <img src="./../../../../public/logo.png" className="w-10 mr-1"/><span className="font-bold">Panora.</span> | ||
| <Link href="/" className="flex flex-row" onClick={() => setOpen(false)}> | ||
| {theme == "light" ? <img src="/logo-panora-black.png" className='w-12' /> : <img src="/logo-panora-white-hq.png" className='w-12' />} | ||
|
|
||
| </Link> | ||
| </SheetTitle> | ||
| </SheetHeader> | ||
| <nav | ||
| className={`flex flex-col items-start mt-6`} |
There was a problem hiding this comment.
Use button elements instead of anchor tags for actions that don't navigate to a URL.
- <a onClick={...}>
+ <button onClick={...}>Also applies to: 74-74, 80-80, 87-87, 93-93, 104-104, 108-108, 115-115
Committable suggestion was skipped due low confidence.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9763120 | Triggered | Generic High Entropy Secret | 0130c73 | docs/mint.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Handle OAuth success status, display success dialog
- Prevent re-authentication with same provider, linked user, and Project_ID
- Add new environment variables, remove deprecated ones
- Update Docker Compose files with health checks, new environment variables
- Improve Linked Users UI with new component for ID display and copying
|
|
||
|
|
||
| const LinkedUserIdComponent = ({ row } : {row:any}) => { | ||
|
|
There was a problem hiding this comment.
Remove the console.log statement or replace it with a more appropriate logging mechanism.
| const returnUrlWithWindow = (typeof window !== 'undefined') | ||
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | ||
| : ''; | ||
|
|
||
|
|
||
| const [data, setData] = useState<Provider[]>([]); | ||
|
|
||
| const { open, isReady } = useOAuth({ | ||
| providerName: selectedProvider?.provider!, |
There was a problem hiding this comment.
Ensure returnUrlWithWindow is correctly defined and used.
| const returnUrlWithWindow = (typeof window !== 'undefined') | ||
| ? window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '') | ||
| : ''; | ||
|
|
||
|
|
||
| const [data, setData] = useState<Provider[]>([]); | ||
|
|
||
| const { open, isReady } = useOAuth({ | ||
| providerName: selectedProvider?.provider!, |
There was a problem hiding this comment.
Remove commented-out returnUrl line if no longer needed.
| @@ -15,36 +15,78 @@ type UseOAuthProps = { | |||
|
|
|||
| const useOAuth = ({ providerName, vertical, returnUrl, projectId, linkedUserId, optionalApiUrl, onSuccess }: UseOAuthProps) => { | |||
| const [isReady, setIsReady] = useState(false); | |||
There was a problem hiding this comment.
Consider adding a comment explaining the purpose of intervalRef and authWindowRef for better code clarity.
| const openModal = async (onWindowClose: () => void) => { | ||
| const apiUrl = optionalApiUrl? optionalApiUrl : config.API_URL!; | ||
| const apiUrl = optionalApiUrl ? optionalApiUrl : config.API_URL!; | ||
| const authUrl = await constructAuthUrl({ | ||
| projectId, linkedUserId, providerName, returnUrl, apiUrl, vertical | ||
| }); |
There was a problem hiding this comment.
Use template literals for error messages for consistency and readability.
Tasks
Summary