enhance(ui): improve ux with a better UI#46
Merged
Conversation
…n window size is small
…cations - Introduced a native macOS menu bar for improved navigation and access to settings. - Implemented an optional feature to launch Calf automatically at user login across macOS, Linux, and Windows. - Added update notifications that check for new releases and prompt users to download the latest version.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds update-check flow, macOS native menus, launch-at-login support, a collapsible sidebar with persisted state, update and about dialogs, UI indicators, native runner sizing changes, and matching documentation updates. ChangesUpdate notifications, macOS menu, launch-at-login, and UI integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AppShell
participant UpdateChecker
participant UpdatePreferences
participant GitHubAPI
participant UpdateDialog
AppShell->>UpdateChecker: checkForUpdates(currentVersion)
UpdateChecker->>UpdatePreferences: load()
UpdatePreferences-->>UpdateChecker: cached preferences
alt cache stale or forced
UpdateChecker->>GitHubAPI: fetch latest release
GitHubAPI-->>UpdateChecker: release JSON
UpdateChecker->>UpdatePreferences: saveCheckResult(...)
end
UpdateChecker-->>AppShell: UpdateCheckResult
alt update available
AppShell->>UpdateDialog: showUpdateAvailableDialog(...)
UpdateDialog-->>AppShell: onSkip / onDownload
AppShell->>UpdatePreferences: saveSkippedVersion(version)
end
sequenceDiagram
participant User
participant MacosMenuScope
participant AppShell
participant SettingsScreen
participant LaunchAtLogin
User->>MacosMenuScope: select Preferences or Update action
MacosMenuScope->>AppShell: onOpenSettings / onCheckForUpdates
AppShell->>SettingsScreen: build(appVersion, update state)
User->>SettingsScreen: toggle Open at login
SettingsScreen->>LaunchAtLogin: setEnabled(true/false)
LaunchAtLogin-->>SettingsScreen: success or failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
ui/lib/widgets/about_dialog.dart (2)
68-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead theme from context inside
_AboutLinkinstead of passing it.
_AboutLinkreceivesShadThemeDataas a constructor parameter, but itsbuildmethod already has aBuildContext. CallingShadTheme.of(context)directly is more idiomatic and removes the coupling. This also eliminates the need to passthemeat both call sites (lines 44 and 50).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/widgets/about_dialog.dart` around lines 68 - 93, _AboutLink is taking ShadThemeData through its constructor even though build already has BuildContext; update _AboutLink to read the theme with ShadTheme.of(context) inside build instead of storing/passing theme. Remove the theme parameter from _AboutLink and adjust both call sites in about_dialog.dart to stop supplying it, keeping the label and onPressed wiring unchanged.
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid importing
macos_menu.dartfor URL constants.A generic widget file depends on a platform-specific menu module solely for
calfRepositoryUrlandcalfDocumentationUrl. This creates an inverted dependency direction — widgets should not depend on platform-specific code. Extract these URL constants into a shared location (e.g., a constants file or theopen_url.dartmodule that already deals with external links).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/widgets/about_dialog.dart` around lines 4 - 5, The about dialog widget is depending on a platform-specific menu module only to access URL constants, which creates an unwanted inverted dependency. Move `calfRepositoryUrl` and `calfDocumentationUrl` into a shared, non-platform-specific location such as `open_url.dart` or a dedicated constants file, then update `AboutDialog` to import that shared source instead of `macos_menu.dart`.ui/lib/updates/update_checker.dart (1)
179-197: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the RegExp to a static field.
RegExp(r'^\d+')is compiled on every_parseVersionPartscall. Making it a static final avoids repeated compilation on the hot version-comparison path.♻️ Proposed refactor
class UpdateChecker { + static final _leadingDigits = RegExp(r'^\d+'); + UpdateChecker({http.Client? client}) : _client = client ?? http.Client();- final match = RegExp(r'^\d+').firstMatch(parts[index]); + final match = _leadingDigits.firstMatch(parts[index]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/updates/update_checker.dart` around lines 179 - 197, Hoist the version-matching RegExp out of _parseVersionParts in UpdateChecker so it is reused instead of recompiled on every call. Add a static final RegExp field on the same class and update _parseVersionParts to use that field when extracting numeric prefixes from the normalized version parts.ui/lib/app_shell.dart (1)
228-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefer the
saveCollapsedside effect out ofbuild().Modifying
_lastWidthWasSmalland_isCollapseddirectly inbuild()works because the values are consumed in the same build cycle, but callingSidebarPreferences.saveCollapsed()is a side effect that should not run duringbuild(). Defer it withaddPostFrameCallbackto keepbuild()pure.♻️ Proposed refactor
} else if (_lastWidthWasSmall != isSmallScreen) { _lastWidthWasSmall = isSmallScreen; _isCollapsed = isSmallScreen; - SidebarPreferences.saveCollapsed(_isCollapsed); + WidgetsBinding.instance.addPostFrameCallback((_) { + SidebarPreferences.saveCollapsed(_isCollapsed); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/app_shell.dart` around lines 228 - 237, Move the SidebarPreferences.saveCollapsed side effect out of AppShell’s build path so build() stays pure. In the AppShell logic where _lastWidthWasSmall and _isCollapsed are updated, keep only the state updates during build and defer the saveCollapsed call via addPostFrameCallback. Use the existing AppShell state fields and SidebarPreferences.saveCollapsed as the main points to adjust, preserving the current collapse behavior while avoiding side effects during rendering.ui/test/update_checker_test.dart (1)
1-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd tests for
check()and cache behavior.The test suite covers static helpers (
compareVersions,normalizeTagName,parseReleaseJson,preferredAssetNames) but not the maincheck()method,_selectDownloadUrl, cache expiry logic, or error-handling paths. Injecting a mockhttp.Client(already supported via the constructor) would allow testing network success/failure/cache scenarios without real I/O.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/test/update_checker_test.dart` around lines 1 - 40, Add coverage for the main UpdateChecker behavior, not just the static helpers. Extend the tests to instantiate UpdateChecker with a mock http.Client and verify check() success, failure, and cached-result paths, including cache expiry behavior. Also add assertions around _selectDownloadUrl to ensure the preferred asset is chosen correctly, and exercise error-handling so network or parsing failures are covered without real I/O.ui/lib/platform/launch_at_login.dart (1)
67-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace generic
catch (_)with specific error handling or logging.All nine catch blocks silently swallow every exception, making it impossible to diagnose why a toggle failed. This violates the project's established practice of handling each error case individually so failures are identifiable.
At minimum, log the error before returning
false:♻️ Example for `_macEnable` (apply similarly elsewhere)
try { plist.parent.createSync(recursive: true); await plist.writeAsString(_macLaunchAgentPlist(launchPath)); return true; - } catch (_) { + } catch (e) { + // log or surface the specific failure return false; }Based on learnings: "Do not use generic catch-all error handling; handle each specific error case individually so logs, UI, and API responses clearly identify what failed and why."
Also applies to: 93-95, 107-109, 147-149, 166-168, 180-182, 204-206, 218-220, 228-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/platform/launch_at_login.dart` around lines 67 - 80, The generic catch blocks in launch_at_login.dart are swallowing all exceptions without any diagnostics. Update each affected try/catch in methods like _macIsEnabled (and the other listed helpers) to catch specific errors where possible, or at minimum log the caught error before returning false. Keep the handling consistent with the project’s pattern so failures in launch-at-login toggles can be identified from logs.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 8-24: The changelog has three repeated Unreleased headings and the
update note is written from the implementation perspective. In CHANGELOG.md,
merge the entries under a single `## [Unreleased]` section, keep the three added
items together, and rewrite the update notifications bullet in user-facing
language without mentioning GitHub Releases or other implementation details.
In `@ui/lib/app_shell.dart`:
- Around line 78-85: The loadAppVersion method is swallowing failures from
fetchStatus(), which leaves _appVersion unset and prevents checkForUpdates from
running. Update the catch block in loadAppVersion to handle the error
explicitly: log or surface the failure with enough context, and make sure the UI
state is updated so the settings screen does not stay on “Loading version...”.
Use the loadAppVersion, fetchStatus, and checkForUpdates symbols to locate and
adjust this flow.
In `@ui/lib/platform/launch_at_login.dart`:
- Around line 209-221: The Windows launch-at-login registry entry in
_windowsEnable stores launchPath unquoted, so update the reg add call to write a
properly quoted command value for the Run key. Use the _windowsEnable and
launchAtLoginPath logic to ensure the path is wrapped/escaped for Windows
startup parsing, and keep the stored value consistent with what
_windowsIsEnabled checks so paths with spaces are handled correctly.
In `@ui/lib/platform/macos_menu.dart`:
- Line 85: The `macos_menu.dart` menu builders are using `if (... case final
item?) item,` patterns that violate the `use_null_aware_elements` lint and block
CI. Update each affected element in the menu list constructors to use the
null-aware element form instead, so the item is included only when
`_platformMenu(...)` returns non-null. Focus on the repeated menu-building logic
around `_platformMenu` and `PlatformProvidedMenuItemType` occurrences, and
replace all nine matching cases consistently.
In `@ui/lib/storage/sidebar_preferences.dart`:
- Around line 7-23: The sidebar preference persistence code uses generic
catch-all handling in loadCollapsed and saveCollapsed, which hides failures and
makes diagnosis impossible. Update SidebarPreferences to handle expected
failures explicitly: in loadCollapsed, catch FileSystemException and
FormatException separately around CalfUiStorage.file, readAsStringSync, and
jsonDecode, returning defaultCollapsed only for those known cases. In
saveCollapsed, replace the empty catch with at least logging the write failure
(and preserving the exception details) so callers can tell when sidebar.json
could not be written.
In `@ui/lib/storage/update_preferences.dart`:
- Around line 49-51: The `load()` and `_save()` error handlers in
`UpdatePreferencesStore` are swallowing exceptions, which hides failures and can
silently drop persisted update state. Replace the blanket `catch (_)` blocks
with specific exception handling where possible, and make sure the caught error
is logged with enough context to diagnose the failure. For `_save()`, avoid
suppressing the failure entirely and let the error propagate or return a failure
signal so callers can react. Keep the fix scoped around the `load()` and
`_save()` methods and their existing `catch` blocks.
In `@ui/lib/updates/update_checker.dart`:
- Around line 111-117: Add a timeout to the GitHub request in
UpdateChecker.check() so a stalled call to _client.get(...) cannot leave the UI
stuck on “Checking...”. Update the logic around the existing _client.get call to
apply a deadline with .timeout(...), and handle TimeoutException in the same
failure path used for other network errors so the update state is reset and
retries remain possible without restarting the app.
In `@ui/lib/widgets/about_dialog.dart`:
- Line 46: The About dialog button handlers currently fire-and-forget the result
of openExternalUrl, so failures are silently ignored. Update the onPressed
callbacks in the About dialog widget to await openExternalUrl(calfRepositoryUrl)
and the other external link call, then handle a false result by surfacing user
feedback such as a snackbar or message. Use the existing About dialog callback
locations and openExternalUrl return value to ensure failures are not swallowed.
In `@ui/test/update_checker_test.dart`:
- Around line 36-39: The UpdateChecker.preferredAssetNames test is
platform-dependent because it asserts a macOS-only asset name unconditionally.
Update the test in update_checker_test.dart to either skip or branch on
Platform.isMacOS before expecting Calf-1.2.3.dmg, or expand the assertions so
the expected asset list matches the current platform rather than always assuming
macOS.
---
Nitpick comments:
In `@ui/lib/app_shell.dart`:
- Around line 228-237: Move the SidebarPreferences.saveCollapsed side effect out
of AppShell’s build path so build() stays pure. In the AppShell logic where
_lastWidthWasSmall and _isCollapsed are updated, keep only the state updates
during build and defer the saveCollapsed call via addPostFrameCallback. Use the
existing AppShell state fields and SidebarPreferences.saveCollapsed as the main
points to adjust, preserving the current collapse behavior while avoiding side
effects during rendering.
In `@ui/lib/platform/launch_at_login.dart`:
- Around line 67-80: The generic catch blocks in launch_at_login.dart are
swallowing all exceptions without any diagnostics. Update each affected
try/catch in methods like _macIsEnabled (and the other listed helpers) to catch
specific errors where possible, or at minimum log the caught error before
returning false. Keep the handling consistent with the project’s pattern so
failures in launch-at-login toggles can be identified from logs.
In `@ui/lib/updates/update_checker.dart`:
- Around line 179-197: Hoist the version-matching RegExp out of
_parseVersionParts in UpdateChecker so it is reused instead of recompiled on
every call. Add a static final RegExp field on the same class and update
_parseVersionParts to use that field when extracting numeric prefixes from the
normalized version parts.
In `@ui/lib/widgets/about_dialog.dart`:
- Around line 68-93: _AboutLink is taking ShadThemeData through its constructor
even though build already has BuildContext; update _AboutLink to read the theme
with ShadTheme.of(context) inside build instead of storing/passing theme. Remove
the theme parameter from _AboutLink and adjust both call sites in
about_dialog.dart to stop supplying it, keeping the label and onPressed wiring
unchanged.
- Around line 4-5: The about dialog widget is depending on a platform-specific
menu module only to access URL constants, which creates an unwanted inverted
dependency. Move `calfRepositoryUrl` and `calfDocumentationUrl` into a shared,
non-platform-specific location such as `open_url.dart` or a dedicated constants
file, then update `AboutDialog` to import that shared source instead of
`macos_menu.dart`.
In `@ui/test/update_checker_test.dart`:
- Around line 1-40: Add coverage for the main UpdateChecker behavior, not just
the static helpers. Extend the tests to instantiate UpdateChecker with a mock
http.Client and verify check() success, failure, and cached-result paths,
including cache expiry behavior. Also add assertions around _selectDownloadUrl
to ensure the preferred asset is chosen correctly, and exercise error-handling
so network or parsing failures are covered without real I/O.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 751b27ea-a682-47fa-8ce8-ba96bfc1dfb8
📒 Files selected for processing (24)
.cursor/rules/calf.mdcCHANGELOG.mdCLAUDE.mdROADMAP.mdui/lib/app_shell.dartui/lib/platform/launch_at_login.dartui/lib/platform/macos_menu.dartui/lib/storage/sidebar_preferences.dartui/lib/storage/update_preferences.dartui/lib/updates/update_checker.dartui/lib/updates/update_dialog.dartui/lib/updates/update_info.dartui/lib/widgets/about_dialog.dartui/lib/widgets/app_top_bar.dartui/lib/widgets/logs_panel.dartui/linux/runner/CMakeLists.txtui/linux/runner/calf_application.ccui/linux/runner/calf_application.hui/linux/runner/main.ccui/linux/runner/my_application.hui/macos/Runner/MainFlutterWindow.swiftui/test/launch_at_login_test.dartui/test/update_checker_test.dartui/windows/runner/win32_window.cpp
💤 Files with no reviewable changes (1)
- ui/linux/runner/my_application.h
- Updated CHANGELOG to reflect the addition of update notifications and other features in version 0.9.0. - Enhanced error handling in app version loading, sidebar preferences, and update preferences to provide more informative debug messages. - Refactored macOS menu and launch at login functionalities for better code clarity and maintainability.
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.
Summary by CodeRabbit