Merged
Conversation
…dark-light-mode-version Fix and Biome
- Refactored JSON schemas in desktop and macOS to improve structure and maintainability by reordering properties and removing redundant definitions. - Updated the version of @sentry/nextjs to ^10.38.0 in web, ui, and root package.json files to ensure compatibility and access to the latest features and fixes.
chore: update schemas and dependencies
Enhance iOS overscroll support with layout adjustments and safe area …
… 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.
Fix/510 pages not syncing
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 series of targeted refactorings and updates. Key efforts include overhauling error handling with custom pages and Sentry integration, streamlining the codebase by removing obsolete store UI components, and updating critical Sentry dependencies. Furthermore, it consolidates UI logic for core pages into a shared library, enhances chart rendering with theme support, and introduces robust page synchronization capabilities. 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 well-executed refactoring. It centralizes many UI components from the desktop and web applications into the shared packages/ui library, which greatly improves code reuse and maintainability. The introduction of new, polished components for the Library, Store, and loading screens is a major UX enhancement. The Sentry dependency updates and the addition of server-side synchronization for page state are also valuable improvements. My feedback includes a couple of suggestions to enhance maintainability and fix a potential bug related to UI updates after importing an app.
Comment on lines
+76
to
+92
| 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.
The importApp function handles importing a non-encrypted app file. After a successful import, it shows a success toast, but it doesn't seem to trigger a refetch of the app list. This will result in the UI not updating to show the newly imported app until the user manually refreshes or navigates away and back. You should invalidate the relevant queries using useQueryClient to ensure the library view updates automatically.
Suggested change
| 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 }); | |
| await queryClient.invalidateQueries(); | |
| } catch (err) { | |
| console.error(err); | |
| toast.error("Failed to import app", { id: toastId }); | |
| } | |
| }, [queryClient]); |
Comment on lines
+18
to
+70
| <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 inline styles for the error page are quite verbose, which can make the component harder to read and maintain. While using inline styles for a global error page is a reasonable fallback to avoid CSS dependency issues, extracting these styles into constant objects at the top of the file would significantly improve readability and make the JSX tree cleaner.
For example:
const containerStyle: React.CSSProperties = {
display: "flex",
flexDirection: "column",
alignItems: "center",
// ... other styles
};
// ... in component:
<div style={containerStyle}>
{/* ... */}
</div>
Comment on lines
+18
to
+70
| <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 inline styles for the error page are quite verbose, which can make the component harder to read and maintain. While using inline styles for a global error page is a reasonable fallback to avoid CSS dependency issues, extracting these styles into constant objects at the top of the file would significantly improve readability and make the JSX tree cleaner.
For example:
const containerStyle: React.CSSProperties = {
display: "flex",
flexDirection: "column",
alignItems: "center",
// ... other styles
};
// ... in component:
<div style={containerStyle}>
{/* ... */}
</div>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request primarily refactors the error handling UI and removes several unused components from the store section of the desktop app, while also updating Sentry dependencies. The most significant changes are the introduction of a custom error page, removal of unused store components, and a dependency update for Sentry.
Error Handling Improvements
apps/desktop/app/error.tsxthat captures exceptions with Sentry and provides user-friendly "Go Back" and "Reload" actions.apps/desktop/app/global-error.tsxwith a custom-styled error UI, removing the dependency onNextError.Codebase Cleanup (Store Components)
AboutSection.tsx,StoreHero.tsx,StoreInfo.tsx, andStoreRecommendations.tsx, reducing codebase complexity and potential maintenance overhead. [1] [2] [3] [4]Dependency Updates
Cargo.tomlto version 0.46.2 and enabled theanyhowfeature for improved error reporting.Minor UI/Code Quality Tweaks
<html>element inapps/desktop/app/layout.tsxto include amin-h-screenclass for improved layout consistency.apps/desktop/app/library/config/layout.tsxfor better readability and maintainability. [1] [2]