-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for adding required integrations for each platform #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This pull request adds support for platform-specific integration allowances, introducing a new 'allowed_integrations' property to the platform table and corresponding functionality to fetch and display allowed integrations.
- Added 'allowed_integrations' field to platform table in
src/database/database.types.ts - Implemented
getAllowedIntegrationsByPlatformNamefunction insrc/database/supabase.tsto fetch allowed integrations - Updated
src/entrypoints/popup/App.tsxto fetch, filter, and display allowed integrations based on platform name - Introduced state management for allowed integrations and platform name in the App component
- Modified UI to show platform name and filter displayed integrations based on the allowed list
3 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| ): Promise<string[]> { | ||
| const { data, error } = await supabase | ||
| .from("platform") | ||
| .select("allowed_integrations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Using .single() assumes only one result. Ensure this is always true for platform names
| const [allowedIntegrations, setAllowedIntegrations] = useState<string[]>([]); | ||
| const [platformName, setPlatformName] = useState<string>(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more specific type for allowedIntegrations, such as Array
| useEffect(() => { | ||
| const fetchAllowedIntegrations = async () => { | ||
| try { | ||
| const platformDetails: PlatformDetails = await getPlatformDetails(); | ||
| setPlatformName(platformDetails.platform); | ||
| const allowed = await getAllowedIntegrationsByPlatformName( | ||
| platformDetails.platform, | ||
| ); | ||
| setAllowedIntegrations(allowed); | ||
| } catch (error) { | ||
| console.error("Error fetching allowed integrations:", error); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Add a cleanup function to the useEffect to cancel the async operation if the component unmounts before it completes
| .filter( | ||
| (integration) => | ||
| integration.values?.some((value) => | ||
| value.toLowerCase().includes(searchTerm.toLowerCase()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This type assertion might be unsafe. Consider using a type guard or refactoring to ensure type safety
| integration.values[0] as keyof typeof integrationEnum.Values, | ||
| ), | ||
| ) | ||
| .sort((a, b) => a.values[0].localeCompare(b.values[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Ensure platformName is properly sanitized before rendering to prevent XSS attacks
We can delay merging this as this might complicate the testing process.