add setting modal #5
Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-qa[bot]](https://avatars.githubusercontent.com/u/30079057?s=60&v=4){kind=small}
| } | ||
| return null; | ||
| }) | ||
| .filter(Boolean); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| <> | ||
| <SettingsModal /> | ||
| {/* <QuittingModal /> */} | ||
| <SubscriptionModal /> | ||
| </TooltipProvider> | ||
| </> |
There was a problem hiding this comment.
// If TooltipProvider is confirmed to be necessary for tooltips in child modals:
import { TooltipProvider } from '@onlook/ui/tooltip'; // Ensure this import is present
// ... other code ...
export const Modals = () => {
return (
<TooltipProvider>
<SettingsModal />
{/* <QuittingModal /> */}
<SubscriptionModal />
</TooltipProvider>
);
};
The TooltipProvider has been removed and replaced by a React Fragment (new lines 8 and 12). If SettingsModal or SubscriptionModal (or any components they render) rely on the context from TooltipProvider (presumably from '@onlook/ui/tooltip') for tooltips to function correctly, this change could lead to broken or missing tooltips. Please verify that TooltipProvider is no longer necessary for these components or that its functionality has been accounted for elsewhere (e.g., if tooltips are no longer used or if a different provider mechanism is in place).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -27,7 +26,7 @@ export const BaseDomain = observer(() => { | |||
| } | |||
|
|
|||
| const url = getValidUrl(baseUrl); | |||
| // invokeMainChannel(MainChannels.OPEN_EXTERNAL_WINDOW, url); | |||
| window.open(url, '_blank'); | |||
There was a problem hiding this comment.
window.open(url, '_blank', 'noopener,noreferrer');
When using window.open with target='_blank', it's recommended to add rel='noopener noreferrer' as the third argument (windowFeatures) to prevent potential security vulnerabilities like tabnabbing. noopener prevents the new page from accessing the window.opener property, and noreferrer prevents the browser from sending the Referer HTTP header to the new page.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| useEffect(() => { | ||
| if (editorEngine.state.settingsOpen && project) { | ||
| pagesManager.scanPages(); | ||
| editorEngine.image.scanImages(); | ||
| projectsManager.scanProjectMetadata(); | ||
| } | ||
| }, [editorEngine.state.settingsOpen]); |
There was a problem hiding this comment.
useEffect(() => {
if (editorEngine.state.settingsOpen && project) {
pagesManager.scanPages();
editorEngine.image.scanImages();
projectsManager.scanProjectMetadata();
}
}, [editorEngine.state.settingsOpen, project, pagesManager, editorEngine.image, projectsManager]);
The useEffect hook (lines 34-40) uses project, pagesManager, editorEngine.image, and projectsManager (via its methods like scanPages, scanImages, scanProjectMetadata) from the component scope. However, only editorEngine.state.settingsOpen is included in its dependency array. This can lead to the effect using stale values for these variables if they change while editorEngine.state.settingsOpen remains constant (e.g., modal is open). To ensure the effect operates with the correct and current data, these dependencies should be added to the array.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
This pull request introduces a settings modal to the
kodustech/onlookrepository, merging changes from thefeat/add-setting-modalbranch into themainbranch. Key updates include:Modals Integration: The
Modalscomponent is imported and rendered within theMaincomponent's layout to provide modal functionality for the project view.Project Management Enhancements: New manager classes (
DomainsManager,VersionsManager) are added with initial structures and getters. TheProjectManagernow includes functionality to scan and parse project metadata from a specified layout file, updating the project state with limited error handling.Metadata Handling: Functionality to extract metadata from files by parsing their AST is introduced. Suggestions for code refactoring, type safety improvements, and documentation enhancements are provided.
UI Component Refactoring: The
Modalscomponent is refactored by replacing theTooltipProviderwith a React Fragment, simplifying the structure. The mechanism for opening external URLs is updated to usewindow.open(url, '_blank').Settings Modal Adjustments: The settings modal sees several updates, including re-enabling a
useEffecthook for data scanning, commenting out analytics-related functionality, and renaming a property frommetadatatositeMetadatafor site-specific settings.Project Interface Updates: The
Projectinterface is enhanced with new fields forurl,siteMetadata,domains, and an optionalenvfor environment variables, along with necessary type imports.These changes collectively aim to enhance the functionality and maintainability of the project settings and metadata management within the application.