Allow choosing a custom background image#154
Closed
phanan wants to merge 14 commits into
Closed
Conversation
Add image_picker dependency and a 'Change background' option in the profile menu. The selected image is copied to app documents and persisted via preferences. A global ValueNotifier drives GradientDecoratedContainer to reactively switch between the custom image and the default asset. 'Reset background' option appears when a custom background is set.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInitializes a persisted custom background and highlight color at startup via new notifiers; adds UI to change/reset background; widgets and theme now use reactive highlight values; adds image-picking and palette deps and tests for notifier behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Startup
participant Storage as GetStorage
participant Init as initBackgroundPreference()
participant Prefs as preferences
participant Notifiers as background/highlight notifiers
participant Container as GradientDecoratedContainer
App->>Storage: GetStorage.init('Preferences')
App->>Storage: GetStorage.init(DownloadProvider.serializedPlayableContainer)
App->>Init: call initBackgroundPreference()
Init->>Prefs: read backgroundImagePath & highlightColor
Prefs-->>Init: return stored values (or null)
Init->>Notifiers: set backgroundImageNotifier & highlightColorNotifier
Init-->>App: complete
App->>Container: build UI
Container->>Notifiers: listen for value changes
Note over Container: Displays FileImage if path exists, else asset
sequenceDiagram
participant User as User
participant Avatar as ProfileAvatar Menu
participant Picker as image_picker
participant FS as File System
participant Prefs as preferences
participant Notifiers as background/highlight notifiers
participant Container as GradientDecoratedContainer
User->>Avatar: Select "Change background"
Avatar->>Picker: pickImage(source: gallery)
Picker-->>Avatar: return selected file path
Avatar->>FS: copy file to app documents (background_<uuid><ext>)
FS-->>Avatar: return new path
Avatar->>Prefs: set backgroundImagePath(newPath) & maybe highlightColor
Avatar->>Notifiers: update backgroundImageNotifier / highlightColorNotifier
Notifiers->>Container: notify listeners
Container->>FS: load FileImage from path
Container-->>User: display custom background
User->>Avatar: Select "Reset background"
Avatar->>Prefs: set backgroundImagePath(null) & clear highlightColor
Avatar->>Notifiers: set backgroundImageNotifier=null / default color
Notifiers->>Container: notify listeners
Container-->>User: display default asset background
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (4)
lib/ui/widgets/gradient_decorated_container.dart (1)
27-28: Minor: Reuse theFileinstance to avoid duplicate object creation.Two
Fileobjects are created for the same path.♻️ Suggested optimization
final ImageProvider image; - if (customPath != null && File(customPath).existsSync()) { - image = FileImage(File(customPath)); + final file = customPath != null ? File(customPath) : null; + if (file != null && file.existsSync()) { + image = FileImage(file); } else { image = const AssetImage('assets/images/background.webp'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/gradient_decorated_container.dart` around lines 27 - 28, Create a single File instance and reuse it instead of constructing File(customPath) twice: when customPath is non-null, instantiate a local variable (e.g., var file = File(customPath)), call file.existsSync() to check existence, and pass that same file to FileImage to set image; update the code around the customPath handling in gradient_decorated_container.dart (the image assignment and exists check) to use this single File variable.test/ui/widgets/custom_background_test.dart (1)
5-44: Add proper test isolation withsetUp/tearDownand remove listener after test.The tests mutate global state (
backgroundImageNotifier.value) and the listener test (line 34) never removes the listener. This can cause test pollution and memory leaks.♻️ Proposed fix for test isolation
void main() { group('Custom background', () { + late VoidCallback? listener; + + setUp(() { + backgroundImageNotifier.value = null; + }); + + tearDown(() { + if (listener != null) { + backgroundImageNotifier.removeListener(listener!); + listener = null; + } + backgroundImageNotifier.value = null; + }); + test('backgroundImageNotifier is a ValueNotifier', () { expect(backgroundImageNotifier, isA<ValueNotifier<String?>>()); }); test('can be set and read', () { backgroundImageNotifier.value = '/some/path/image.jpg'; expect(backgroundImageNotifier.value, '/some/path/image.jpg'); backgroundImageNotifier.value = null; expect(backgroundImageNotifier.value, isNull); }); test('null value means default asset background', () { backgroundImageNotifier.value = null; expect(backgroundImageNotifier.value, isNull); }); test('non-null value means custom background', () { backgroundImageNotifier.value = '/custom/background.png'; expect(backgroundImageNotifier.value, isNotNull); - - backgroundImageNotifier.value = null; }); test('notifier broadcasts changes to listeners', () { String? received; - backgroundImageNotifier.addListener(() { + listener = () { received = backgroundImageNotifier.value; - }); + }; + backgroundImageNotifier.addListener(listener!); backgroundImageNotifier.value = '/new/image.webp'; expect(received, '/new/image.webp'); backgroundImageNotifier.value = null; expect(received, isNull); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/widgets/custom_background_test.dart` around lines 5 - 44, The tests mutate the global backgroundImageNotifier and add a listener that is never removed; to fix, add a setUp that resets backgroundImageNotifier.value to null before each test and a tearDown that clears any listeners and resets the value after each test, and in the "notifier broadcasts changes to listeners" test capture the returned listener (or store the callback) and remove it with backgroundImageNotifier.removeListener(...) in tearDown or at the end of that test; reference backgroundImageNotifier, addListener, removeListener, setUp, and tearDown when making the changes.lib/ui/widgets/profile_avatar.dart (1)
73-76: Menu won't update after background change without widget rebuild.
hasCustomBackgroundis computed once inbuild(). SinceProfileAvataris aStatelessWidget, the "Reset background" menu item visibility won't update until the parent rebuilds. Consider usingValueListenableBuilderor converting toStatefulWidgetif dynamic updates are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/profile_avatar.dart` around lines 73 - 76, The menu visibility is computed once in ProfileAvatar.build() via hasCustomBackground so the "Reset background" item won't update; change ProfileAvatar to observe background changes instead of computing once: either convert ProfileAvatar to a StatefulWidget and subscribe to the preferences/backgroundImagePath changes (or use context.watch/Consumer if preferences is a Provider) so hasCustomBackground is recalculated on updates, or keep it Stateless but wrap the menu widget (or the relevant subtree) with a ValueListenableBuilder/Selector that listens to preferences.backgroundImagePath and recomputes hasCustomBackground to show/hide the "Reset background" menu item dynamically.pubspec.yaml (1)
56-56: Updateimage_pickerconstraint to the latest stable version.The dependency
image_picker: ^1.0.7is outdated. Version 1.2.1 is the latest stable release. While the caret syntax allows for newer versions, it's better practice to update the constraint to^1.2.1to be explicit about the supported version range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pubspec.yaml` at line 56, Update the image_picker dependency version constraint in pubspec.yaml from "image_picker: ^1.0.7" to "image_picker: ^1.2.1"; after updating, run the package manager (flutter pub get) to refresh the lockfile and verify there are no breaking API changes in any code using ImagePicker (check usages of ImagePicker.pickImage / getImage etc. and adapt if required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/widgets/profile_avatar.dart`:
- Around line 54-66: In _changeBackground, before copying the new image, check
preferences.backgroundImagePath and if it points to an existing file that is
different from the new destination (different path or extension), delete that
old file; perform the copy and only update preferences.backgroundImagePath and
backgroundImageNotifier.value after a successful copy, and wrap the file
operations (exists, delete, copy) in a try/catch to handle and log or surface IO
errors so failures don’t leave stale state. Ensure you reference the picked.path
ext and the created dest File and use File(...).exists() / delete() inside the
try block.
- Around line 68-71: The _resetBackground() method currently clears
preferences.backgroundImagePath and backgroundImageNotifier.value but leaves the
image file on disk; update _resetBackground() to check
preferences.backgroundImagePath, delete the corresponding file (using File(...)
and checking exists before delete) and handle errors (log or ignore) before
clearing the preference and notifier to avoid storage bloat; mirror the pattern
used in DownloadProvider (delete-if-exists with try/catch) and ensure any caller
(e.g., the onSelected switch case) is made async or accepts fire-and-forget so
the deletion can run safely without breaking UI flow.
---
Nitpick comments:
In `@lib/ui/widgets/gradient_decorated_container.dart`:
- Around line 27-28: Create a single File instance and reuse it instead of
constructing File(customPath) twice: when customPath is non-null, instantiate a
local variable (e.g., var file = File(customPath)), call file.existsSync() to
check existence, and pass that same file to FileImage to set image; update the
code around the customPath handling in gradient_decorated_container.dart (the
image assignment and exists check) to use this single File variable.
In `@lib/ui/widgets/profile_avatar.dart`:
- Around line 73-76: The menu visibility is computed once in
ProfileAvatar.build() via hasCustomBackground so the "Reset background" item
won't update; change ProfileAvatar to observe background changes instead of
computing once: either convert ProfileAvatar to a StatefulWidget and subscribe
to the preferences/backgroundImagePath changes (or use context.watch/Consumer if
preferences is a Provider) so hasCustomBackground is recalculated on updates, or
keep it Stateless but wrap the menu widget (or the relevant subtree) with a
ValueListenableBuilder/Selector that listens to preferences.backgroundImagePath
and recomputes hasCustomBackground to show/hide the "Reset background" menu item
dynamically.
In `@pubspec.yaml`:
- Line 56: Update the image_picker dependency version constraint in pubspec.yaml
from "image_picker: ^1.0.7" to "image_picker: ^1.2.1"; after updating, run the
package manager (flutter pub get) to refresh the lockfile and verify there are
no breaking API changes in any code using ImagePicker (check usages of
ImagePicker.pickImage / getImage etc. and adapt if required).
In `@test/ui/widgets/custom_background_test.dart`:
- Around line 5-44: The tests mutate the global backgroundImageNotifier and add
a listener that is never removed; to fix, add a setUp that resets
backgroundImageNotifier.value to null before each test and a tearDown that
clears any listeners and resets the value after each test, and in the "notifier
broadcasts changes to listeners" test capture the returned listener (or store
the callback) and remove it with backgroundImageNotifier.removeListener(...) in
tearDown or at the end of that test; reference backgroundImageNotifier,
addListener, removeListener, setUp, and tearDown when making the changes.
🪄 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
Run ID: 9c218c66-695c-432a-8b3f-968a43ea7b78
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/main.dartlib/ui/widgets/gradient_decorated_container.dartlib/ui/widgets/profile_avatar.dartlib/utils/preferences.dartpubspec.yamltest/ui/widgets/custom_background_test.dart
Comment on lines
+54
to
+66
| Future<void> _changeBackground() async { | ||
| final picker = ImagePicker(); | ||
| final picked = await picker.pickImage(source: ImageSource.gallery); | ||
| if (picked == null) return; | ||
|
|
||
| final appDir = await getApplicationDocumentsDirectory(); | ||
| final ext = path.extension(picked.path); | ||
| final dest = File('${appDir.path}/background$ext'); | ||
| await File(picked.path).copy(dest.path); | ||
|
|
||
| preferences.backgroundImagePath = dest.path; | ||
| backgroundImageNotifier.value = dest.path; | ||
| } |
There was a problem hiding this comment.
Delete the previous background file before copying a new one.
When the user changes the background, if the previous file has a different extension (e.g., .jpg → .png), the old file remains on disk. Additionally, there's no error handling for file operations.
🛡️ Proposed fix to clean up old file and add error handling
Future<void> _changeBackground() async {
+ try {
final picker = ImagePicker();
final picked = await picker.pickImage(source: ImageSource.gallery);
if (picked == null) return;
final appDir = await getApplicationDocumentsDirectory();
+
+ // Clean up any existing background files
+ final existing = appDir.listSync().whereType<File>().where(
+ (f) => path.basenameWithoutExtension(f.path) == 'background',
+ );
+ for (final file in existing) {
+ if (file.existsSync()) {
+ file.deleteSync();
+ }
+ }
+
final ext = path.extension(picked.path);
final dest = File('${appDir.path}/background$ext');
await File(picked.path).copy(dest.path);
preferences.backgroundImagePath = dest.path;
backgroundImageNotifier.value = dest.path;
+ } catch (e) {
+ debugPrint('Failed to change background: $e');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Future<void> _changeBackground() async { | |
| final picker = ImagePicker(); | |
| final picked = await picker.pickImage(source: ImageSource.gallery); | |
| if (picked == null) return; | |
| final appDir = await getApplicationDocumentsDirectory(); | |
| final ext = path.extension(picked.path); | |
| final dest = File('${appDir.path}/background$ext'); | |
| await File(picked.path).copy(dest.path); | |
| preferences.backgroundImagePath = dest.path; | |
| backgroundImageNotifier.value = dest.path; | |
| } | |
| Future<void> _changeBackground() async { | |
| try { | |
| final picker = ImagePicker(); | |
| final picked = await picker.pickImage(source: ImageSource.gallery); | |
| if (picked == null) return; | |
| final appDir = await getApplicationDocumentsDirectory(); | |
| // Clean up any existing background files | |
| final existing = appDir.listSync().whereType<File>().where( | |
| (f) => path.basenameWithoutExtension(f.path) == 'background', | |
| ); | |
| for (final file in existing) { | |
| if (file.existsSync()) { | |
| file.deleteSync(); | |
| } | |
| } | |
| final ext = path.extension(picked.path); | |
| final dest = File('${appDir.path}/background$ext'); | |
| await File(picked.path).copy(dest.path); | |
| preferences.backgroundImagePath = dest.path; | |
| backgroundImageNotifier.value = dest.path; | |
| } catch (e) { | |
| debugPrint('Failed to change background: $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/widgets/profile_avatar.dart` around lines 54 - 66, In
_changeBackground, before copying the new image, check
preferences.backgroundImagePath and if it points to an existing file that is
different from the new destination (different path or extension), delete that
old file; perform the copy and only update preferences.backgroundImagePath and
backgroundImageNotifier.value after a successful copy, and wrap the file
operations (exists, delete, copy) in a try/catch to handle and log or surface IO
errors so failures don’t leave stale state. Ensure you reference the picked.path
ext and the created dest File and use File(...).exists() / delete() inside the
try block.
Comment on lines
+68
to
+71
| void _resetBackground() { | ||
| preferences.backgroundImagePath = null; | ||
| backgroundImageNotifier.value = null; | ||
| } |
There was a problem hiding this comment.
Delete the background file from disk when resetting.
_resetBackground() clears the preference and notifier but leaves the file on disk, causing gradual storage bloat. The codebase uses a similar cleanup pattern in DownloadProvider (see lib/providers/download_provider.dart:101-108).
🐛 Proposed fix to delete the file on reset
- void _resetBackground() {
+ Future<void> _resetBackground() async {
+ final currentPath = preferences.backgroundImagePath;
+ if (currentPath != null) {
+ final file = File(currentPath);
+ if (file.existsSync()) {
+ try {
+ file.deleteSync();
+ } catch (e) {
+ debugPrint('Failed to delete background file: $e');
+ }
+ }
+ }
preferences.backgroundImagePath = null;
backgroundImageNotifier.value = null;
}Also update the switch case:
case ProfileAvatarMenuItems.resetBackground:
- _resetBackground();
+ await _resetBackground();
break;Note: The onSelected callback would need to be async, or you can call without await if you prefer fire-and-forget semantics for the delete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/widgets/profile_avatar.dart` around lines 68 - 71, The
_resetBackground() method currently clears preferences.backgroundImagePath and
backgroundImageNotifier.value but leaves the image file on disk; update
_resetBackground() to check preferences.backgroundImagePath, delete the
corresponding file (using File(...) and checking exists before delete) and
handle errors (log or ignore) before clearing the preference and notifier to
avoid storage bloat; mirror the pattern used in DownloadProvider
(delete-if-exists with try/catch) and ensure any caller (e.g., the onSelected
switch case) is made async or accepts fire-and-forget so the deletion can run
safely without breaking UI flow.
Each picked image gets a timestamped filename so the path changes, triggering both the ValueNotifier and avoiding FileImage's cache. The previous custom background file is cleaned up.
Use palette_generator to extract the vibrant/dominant color from a custom background image. The color is persisted in preferences and applied to: - Theme (buttons, tab indicators, splash, text buttons) - All direct AppColors.highlight references (radio stations, scrollbar, mini player, form cursors, shuffle button, pull-to-refresh, cards) A highlightAccentColor is derived by darkening the highlight by 10% in HSL space. Resetting the background also resets to the default pink. The App widget rebuilds when the highlight color changes via ValueListenableBuilder on highlightColorNotifier.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ui/app.dart (1)
20-30: Optional: make the color dependency explicit in the builder path.Current behavior works, but passing the builder’s
Colorvalue into theme construction would reduce hidden global coupling and improve testability.♻️ Suggested refactor
- child: ValueListenableBuilder<Color>( - valueListenable: highlightColorNotifier, - builder: (context, _, __) { + child: ValueListenableBuilder<Color>( + valueListenable: highlightColorNotifier, + builder: (context, highlight, __) { return MaterialApp( debugShowCheckedModeBanner: false, title: AppStrings.appName, - theme: themeData(context), + theme: themeData(context, highlight), initialRoute: InitialScreen.routeName, routes: AppRouter.routes, ); }, ),// lib/ui/theme_data.dart ThemeData themeData(BuildContext context, Color highlight) { final accent = HSLColor.fromColor(highlight) .withLightness((HSLColor.fromColor(highlight).lightness - 0.1).clamp(0.0, 1.0)) .toColor(); ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/app.dart` around lines 20 - 30, The builder currently ignores the Color value from highlightColorNotifier causing hidden coupling; update the ValueListenableBuilder's builder signature to accept the color (e.g., builder: (context, Color highlight, __) => ...) and pass that highlight into themeData (i.e., themeData(context, highlight)); adjust the themeData signature (function name themeData) to accept the Color parameter and use it when deriving accent/highlight colors (see HSLColor usage) so the theme depends explicitly on the notifier value.lib/ui/widgets/gradient_decorated_container.dart (1)
40-41: Consider async file existence check for strict main-thread hygiene.
existsSync()is synchronous I/O on the main thread. While this only runs when the notifier value changes (rare), an async check viacompute()or caching the existence state in the notifier itself would avoid any potential jank.This is low priority since background changes are infrequent user actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/gradient_decorated_container.dart` around lines 40 - 41, Summary: Synchronous File(customPath).existsSync() does blocking I/O on the main thread; use an async check and update the image only after confirming existence. Fix: replace the sync check inside the widget/notifier logic by performing an asynchronous existence check (File(customPath).exists() or run the check inside compute()) and only assign image = FileImage(File(customPath)) after the await returns true, or alternatively move the existence state into the notifier so the UI reacts to a cached boolean; update the code paths that reference customPath, File(customPath).existsSync(), and image = FileImage(File(customPath)) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/widgets/profile_avatar.dart`:
- Around line 67-75: The current flow deletes the old background
(preferences.backgroundImagePath via File(...).delete()) before copying the new
one (picked.path -> dest.path), risking data loss if File.copy fails; change the
order to perform File(picked.path).copy(dest.path) first, only after a
successful copy attempt to then delete the old file
(preferences.backgroundImagePath) inside a try/catch, and avoid deleting if
oldPath == dest.path; also catch and surface/report copy errors rather than
swallowing them so callers can react.
---
Nitpick comments:
In `@lib/ui/app.dart`:
- Around line 20-30: The builder currently ignores the Color value from
highlightColorNotifier causing hidden coupling; update the
ValueListenableBuilder's builder signature to accept the color (e.g., builder:
(context, Color highlight, __) => ...) and pass that highlight into themeData
(i.e., themeData(context, highlight)); adjust the themeData signature (function
name themeData) to accept the Color parameter and use it when deriving
accent/highlight colors (see HSLColor usage) so the theme depends explicitly on
the notifier value.
In `@lib/ui/widgets/gradient_decorated_container.dart`:
- Around line 40-41: Summary: Synchronous File(customPath).existsSync() does
blocking I/O on the main thread; use an async check and update the image only
after confirming existence. Fix: replace the sync check inside the
widget/notifier logic by performing an asynchronous existence check
(File(customPath).exists() or run the check inside compute()) and only assign
image = FileImage(File(customPath)) after the await returns true, or
alternatively move the existence state into the notifier so the UI reacts to a
cached boolean; update the code paths that reference customPath,
File(customPath).existsSync(), and image = FileImage(File(customPath))
accordingly.
🪄 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
Run ID: 7da508b1-9a1a-4a26-bc27-73774d2e7d52
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
lib/ui/app.dartlib/ui/screens/radio_stations.dartlib/ui/theme_data.dartlib/ui/widgets/alphabet_scrollbar.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/gradient_decorated_container.dartlib/ui/widgets/horizontal_card_scroller.dartlib/ui/widgets/mini_player.dartlib/ui/widgets/playable_list_header.dartlib/ui/widgets/profile_avatar.dartlib/ui/widgets/pull_to_refresh.dartlib/utils/preferences.dartpubspec.yaml
✅ Files skipped from review due to trivial changes (1)
- pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/preferences.dart
Comment on lines
+67
to
+75
| // Remove the old custom background file | ||
| final oldPath = preferences.backgroundImagePath; | ||
| if (oldPath != null) { | ||
| try { | ||
| await File(oldPath).delete(); | ||
| } catch (_) {} | ||
| } | ||
|
|
||
| await File(picked.path).copy(dest.path); |
There was a problem hiding this comment.
Delete old file after successful copy, not before.
If copy() fails (e.g., disk full, permission issue), the old background is already deleted and the user loses their custom background. Reorder to copy first, then delete the old file on success.
🛡️ Proposed fix for safer ordering
final dest = File('${appDir.path}/background_$id$ext');
- // Remove the old custom background file
- final oldPath = preferences.backgroundImagePath;
- if (oldPath != null) {
- try {
- await File(oldPath).delete();
- } catch (_) {}
- }
-
await File(picked.path).copy(dest.path);
+ // Remove the old custom background file after successful copy
+ final oldPath = preferences.backgroundImagePath;
+ if (oldPath != null) {
+ try {
+ await File(oldPath).delete();
+ } catch (_) {}
+ }
+
preferences.backgroundImagePath = dest.path;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Remove the old custom background file | |
| final oldPath = preferences.backgroundImagePath; | |
| if (oldPath != null) { | |
| try { | |
| await File(oldPath).delete(); | |
| } catch (_) {} | |
| } | |
| await File(picked.path).copy(dest.path); | |
| await File(picked.path).copy(dest.path); | |
| // Remove the old custom background file after successful copy | |
| final oldPath = preferences.backgroundImagePath; | |
| if (oldPath != null) { | |
| try { | |
| await File(oldPath).delete(); | |
| } catch (_) {} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/widgets/profile_avatar.dart` around lines 67 - 75, The current flow
deletes the old background (preferences.backgroundImagePath via
File(...).delete()) before copying the new one (picked.path -> dest.path),
risking data loss if File.copy fails; change the order to perform
File(picked.path).copy(dest.path) first, only after a successful copy attempt to
then delete the old file (preferences.backgroundImagePath) inside a try/catch,
and avoid deleting if oldPath == dest.path; also catch and surface/report copy
errors rather than swallowing them so callers can react.
The Expanded scroll area was consuming all space, pushing Cancel/Create buttons below the visible sheet. Use Flexible + Spacer so the form content only takes what it needs and buttons stay pinned at the bottom.
Use ConstrainedBox with maxHeight instead of fixed SizedBox height, and Flexible instead of Expanded for the scroll area. The sheet now sizes to its content (up to 85% of screen), ensuring buttons are always visible regardless of form field count.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ui/widgets/form_sheet.dart (1)
91-96: Layout may not pin buttons to bottom with small content.With both
Flexible(flex: 1)andSpacer(flex: 1), remaining space is split equally. If form content is smaller than theFlexible's allocated half, that unused portion isn't redistributed toSpacer— it becomes extra space at the Column's end (after buttons), due toMainAxisAlignment.start.This ensures buttons remain visible (addressing the off-screen issue), but they won't be strictly bottom-pinned when content is short.
If pinning to bottom is desired:
Consider using MainAxisAlignment.spaceBetween or wrapping buttons
Option 1: Use
MainAxisAlignment.spaceBetweenreturn Column( crossAxisAlignment: CrossAxisAlignment.stretch, + mainAxisAlignment: MainAxisAlignment.spaceBetween, children: [ // ... header and content - const Spacer(), const SizedBox(height: 32), Row( // ... buttons ), ], );Option 2: Wrap scroll + buttons in Expanded for bottom pinning
-Flexible( +Expanded( child: SingleChildScrollView( child: widget.builder(context, setState), ), ), -const Spacer(), const SizedBox(height: 32),Then ensure content inside the scroll view doesn't overflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 91 - 96, The Column currently uses Flexible -> SingleChildScrollView (widget.builder) plus a Spacer, which can leave buttons not pinned to the bottom when content is short; change the layout so the scrollable area and buttons are forced to opposite ends — either set the Column's mainAxisAlignment to MainAxisAlignment.spaceBetween, or replace the Flexible/Spacer pair by wrapping the SingleChildScrollView in Expanded and place the buttons after it (ensuring widget.builder content can scroll without overflowing); update the Column that contains Flexible, SingleChildScrollView, Spacer and widget.builder accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 91-96: The Column currently uses Flexible -> SingleChildScrollView
(widget.builder) plus a Spacer, which can leave buttons not pinned to the bottom
when content is short; change the layout so the scrollable area and buttons are
forced to opposite ends — either set the Column's mainAxisAlignment to
MainAxisAlignment.spaceBetween, or replace the Flexible/Spacer pair by wrapping
the SingleChildScrollView in Expanded and place the buttons after it (ensuring
widget.builder content can scroll without overflowing); update the Column that
contains Flexible, SingleChildScrollView, Spacer and widget.builder accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37411d58-29fe-4160-836d-a791e2c5c639
📒 Files selected for processing (1)
lib/ui/widgets/form_sheet.dart
Update collapsed app bar background, popup menu theme, and mini player background to use the dynamic highlight color instead of hardcoded Color.fromRGBO(25, 0, 64, ...) / 0xFF1B0047.
Set staticScreenHeaderBackground to opaque dark color instead of transparent, which eliminates the CupertinoNavigationBar blur effect.
Use a dark, desaturated variant of the highlight color for all screen headers, removing the frosted glass blur effect. Remove the unused flexibleScreenHeaderBackground constant.
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
image_pickerdependency for photo library accessGradientDecoratedContainernow uses aValueNotifierto reactively switch between custom image and default asset across all 25+ screensGetStorageand initialized at app startupTest plan
flutter testSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests