Conversation
… for better structure - Updated LoadingScreen component to include dynamic tips and hints during loading. - Refactored FlowLogo and TipCard components for improved readability and maintainability. - Introduced useThemeGradient hook for theme management and gradient generation. - Added size prop to ShareButton for better customization. - Cleaned up unused code and optimized existing functionality.
Summary of ChangesHello @felix-schultz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the application's stability, maintainability, and user experience through a significant refactor of core UI components and the introduction of robust error handling. By centralizing store and library functionalities into a shared UI library, the codebase is cleaner and more modular. Additionally, new page synchronization capabilities enhance data consistency, and a redesigned loading screen provides a more engaging user experience. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring. It successfully cleans up the codebase by moving shared UI components and logic for the store and library pages into the @tm9657/flow-like-ui package. The introduction of new error boundary components and the redesign of the library and store pages are great improvements for both maintainability and user experience.
My review includes a few suggestions to enhance data synchronization logic and improve code maintainability in the new error pages. Specifically, I've pointed out a missing data refetch after importing an app and an opportunity to make the page synchronization logic more robust. I've also commented on the use of inline styles in the global error handlers.
| const importApp = useCallback(async (path: string) => { | ||
| if (path.toLowerCase().endsWith(".enc.flow-app")) { | ||
| setEncryptedImportPath(path); | ||
| setImportDialogOpen(true); | ||
| return; | ||
| } | ||
| const toastId = toast.loading("Importing app...", { | ||
| description: "Please wait.", | ||
| }); | ||
| try { | ||
| await invoke("import_app_from_file", { path }); | ||
| toast.success("App imported successfully!", { id: toastId }); | ||
| } catch (err) { | ||
| console.error(err); | ||
| toast.error("Failed to import app", { id: toastId }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
After a successful app import, the list of apps in the library is not being updated. The previous implementation called apps.refetch(), but the new version is missing a mechanism to refresh the app list. This will lead to a stale UI until the user manually refreshes or navigates away and back.
You should invalidate the queries to trigger a refetch and update the UI. Since queryClient is already available in the component, you can use it to invalidate all queries.
| const importApp = useCallback(async (path: string) => { | |
| if (path.toLowerCase().endsWith(".enc.flow-app")) { | |
| setEncryptedImportPath(path); | |
| setImportDialogOpen(true); | |
| return; | |
| } | |
| const toastId = toast.loading("Importing app...", { | |
| description: "Please wait.", | |
| }); | |
| try { | |
| await invoke("import_app_from_file", { path }); | |
| toast.success("App imported successfully!", { id: toastId }); | |
| } catch (err) { | |
| console.error(err); | |
| toast.error("Failed to import app", { id: toastId }); | |
| } | |
| }, []); | |
| const importApp = useCallback(async (path: string) => { | |
| if (path.toLowerCase().endsWith(".enc.flow-app")) { | |
| setEncryptedImportPath(path); | |
| setImportDialogOpen(true); | |
| return; | |
| } | |
| const toastId = toast.loading("Importing app...", { | |
| description: "Please wait.", | |
| }); | |
| try { | |
| await invoke("import_app_from_file", { path }); | |
| toast.success("App imported successfully!", { id: toastId }); | |
| queryClient.invalidateQueries(); | |
| } catch (err) { | |
| console.error(err); | |
| toast.error("Failed to import app", { id: toastId }); | |
| } | |
| }, [queryClient]); |
| if (remoteUpdated > localUpdated) { | ||
| const merged = { ...remotePage, boardId: remotePage.boardId || localPage.boardId }; | ||
| await invoke("update_page", { appId, page: merged }); | ||
| } |
There was a problem hiding this comment.
The background sync logic in getPage only handles synchronizing from remote to local when the remote page is newer. It doesn't handle the case where the local page is newer than the remote one (e.g., after an offline edit). This can lead to data loss or stale data on the server.
To ensure data consistency, you should also push local changes to the server if the local version is more recent.
if (remoteUpdated > localUpdated) {
const merged = { ...remotePage, boardId: remotePage.boardId || localPage.boardId };
await invoke("update_page", { appId, page: merged });
} else if (localUpdated > remoteUpdated) {
await this.pushPageToServer(appId, localPage);
}| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| minHeight: "100vh", | ||
| gap: "24px", | ||
| padding: "24px", | ||
| fontFamily: "system-ui, sans-serif", | ||
| background: "#0a0a0a", | ||
| color: "#fafafa", | ||
| }} | ||
| > | ||
| <h2 style={{ fontSize: "1.5rem", fontWeight: 600 }}> | ||
| Something went wrong | ||
| </h2> | ||
| <p style={{ fontSize: "0.875rem", color: "#a1a1aa" }}> | ||
| An unexpected error occurred. | ||
| </p> | ||
| <div style={{ display: "flex", gap: "12px" }}> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.history.back()} | ||
| style={{ | ||
| padding: "8px 16px", | ||
| fontSize: "0.875rem", | ||
| borderRadius: "6px", | ||
| border: "1px solid #27272a", | ||
| background: "transparent", | ||
| color: "#fafafa", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| Go Back | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.location.reload()} | ||
| style={{ | ||
| padding: "8px 16px", | ||
| fontSize: "0.875rem", | ||
| borderRadius: "6px", | ||
| border: "none", | ||
| background: "#fafafa", | ||
| color: "#0a0a0a", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| Reload | ||
| </button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The error page uses inline styles with hardcoded color values (e.g., #0a0a0a, #fafafa). This can lead to inconsistencies if the application's theme changes and makes maintenance harder.
While global-error.tsx has limitations and using theme providers might not be safe, it would be better to define these colors as constants at the top of the file, or ideally use CSS variables if they are available at this stage. This would improve maintainability and consistency.
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| minHeight: "100vh", | ||
| gap: "24px", | ||
| padding: "24px", | ||
| fontFamily: "system-ui, sans-serif", | ||
| background: "#0a0a0a", | ||
| color: "#fafafa", | ||
| }} | ||
| > | ||
| <h2 style={{ fontSize: "1.5rem", fontWeight: 600 }}> | ||
| Something went wrong | ||
| </h2> | ||
| <p style={{ fontSize: "0.875rem", color: "#a1a1aa" }}> | ||
| An unexpected error occurred. | ||
| </p> | ||
| <div style={{ display: "flex", gap: "12px" }}> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.history.back()} | ||
| style={{ | ||
| padding: "8px 16px", | ||
| fontSize: "0.875rem", | ||
| borderRadius: "6px", | ||
| border: "1px solid #27272a", | ||
| background: "transparent", | ||
| color: "#fafafa", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| Go Back | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.location.reload()} | ||
| style={{ | ||
| padding: "8px 16px", | ||
| fontSize: "0.875rem", | ||
| borderRadius: "6px", | ||
| border: "none", | ||
| background: "#fafafa", | ||
| color: "#0a0a0a", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| Reload | ||
| </button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The error page uses inline styles with hardcoded color values (e.g., #0a0a0a, #fafafa). This can lead to inconsistencies if the application's theme changes and makes maintenance harder.
While global-error.tsx has limitations and using theme providers might not be safe, it would be better to define these colors as constants at the top of the file, or ideally use CSS variables if they are available at this stage. This would improve maintainability and consistency.
This pull request primarily removes several unused React components related to the store feature in the desktop app, and introduces a new error boundary component for improved error handling and reporting. The changes help to clean up the codebase and enhance user experience when errors occur.
Error handling improvements:
error.tsxthat captures exceptions with Sentry and provides users with options to reload or go back, improving error reporting and user experience.global-error.tsxto use a custom-styled error message instead of the default Next.js error component, offering a more consistent and user-friendly appearance.Codebase cleanup:
AboutSection,StoreHero,StoreInfo(includingInfoGrid,MediaGallery,DetailsCard,LinkRow,ExternalLinkRow),StoreRecommendations, andStoreSkeletons. This reduces code bloat and maintenance overhead. [1] [2] [3] [4] [5]