fix: persist custom themes across updates & auto-reload on file changes#8664
Open
alexrosepizant wants to merge 2 commits intoAppFlowy-IO:mainfrom
Open
fix: persist custom themes across updates & auto-reload on file changes#8664alexrosepizant wants to merge 2 commits intoAppFlowy-IO:mainfrom
alexrosepizant wants to merge 2 commits intoAppFlowy-IO:mainfrom
Conversation
Store plugins under the platform app-support directory instead of Documents so they survive application updates. The legacy Documents folder is kept as a read-only fallback. PluginLocationService now supports multiple scan locations with deduplication.
…#5873) Watch the active theme's source directory for file-system changes and automatically re-decode and emit the updated theme with a 500ms debounce. The watcher is restarted whenever the user switches theme and stopped when the cubit is closed.
Contributor
Reviewer's GuideUpdates plugin theme storage to use an app-support directory with a legacy Documents fallback, adds multi-location plugin discovery, and introduces filesystem-based hot-reload for custom themes plus tests covering plugin locations and theme file watching. Sequence diagram for theme hot-reload on filesystem changessequenceDiagram
actor User
participant AppearanceSettingsCubit
participant FlowyPluginService
participant PluginDirectory as Plugin_source_directory
participant FileSystem
User->>AppearanceSettingsCubit: setTheme(themeName)
AppearanceSettingsCubit->>AppearanceSettingsCubit: _saveAppearanceSettings()
AppearanceSettingsCubit->>AppTheme: fromName(themeName)
AppTheme-->>AppearanceSettingsCubit: AppTheme instance
AppearanceSettingsCubit->>AppearanceSettingsCubit: _startWatchingTheme(appTheme)
AppearanceSettingsCubit->>AppearanceSettingsCubit: _stopWatchingTheme()
AppearanceSettingsCubit->>FlowyPluginService: lookup(name: themeName)
FlowyPluginService-->>AppearanceSettingsCubit: FlowyDynamicPlugin or null
AppearanceSettingsCubit->>PluginDirectory: plugin.source.watch(recursive: true)
PluginDirectory-->>AppearanceSettingsCubit: FileSystemEvent stream
loop On file change
FileSystem-->>AppearanceSettingsCubit: FileSystemEvent
AppearanceSettingsCubit->>AppearanceSettingsCubit: cancel existing _themeReloadDebounce
AppearanceSettingsCubit->>AppearanceSettingsCubit: start 500ms _themeReloadDebounce
AppearanceSettingsCubit->>AppearanceSettingsCubit: _reloadCurrentTheme()
AppearanceSettingsCubit->>AppTheme: fromName(currentName)
AppTheme-->>AppearanceSettingsCubit: AppTheme reloaded
AppearanceSettingsCubit->>AppearanceSettingsCubit: emit(state.copyWith(appTheme: theme))
end
AppearanceSettingsCubit->>AppearanceSettingsCubit: close()
AppearanceSettingsCubit->>AppearanceSettingsCubit: _stopWatchingTheme()
Updated class diagram for plugin and theme managementclassDiagram
class PluginLocationService {
-Future~Directory~ _fallback
-List~Future~Directory~~ _additionalLocations
+PluginLocationService(fallback, additionalLocations)
+Future~Directory~ get fallback
+Future~Directory~ get location
+Future~List~Directory~~ get allLocations
}
class FlowyPluginService {
-static FlowyPluginService _instance
+static FlowyPluginService get instance
-PluginLocationService _locationService
+FlowyPluginService._()
+setLocation(PluginLocationService locationService)
+Future~Iterable~Directory~~ get _targets
+Future~FlowyDynamicPlugin?~ lookup(name)
}
class AppearanceSettingsCubit {
-AppearanceSettingsPB _appearanceSettings
-DateTimeSettingsPB _dateTimeSettings
-StreamSubscription~FileSystemEvent~? _themeWatcher
-Timer? _themeReloadDebounce
-int _watchGeneration
+AppearanceSettingsCubit(AppTheme appTheme, AppearanceSettingsPB appearanceSettings, DateTimeSettingsPB dateTimeSettings)
+Future~void~ setTextScaleFactor(textScaleFactor)
+Future~void~ setTheme(themeName)
-void _startWatchingTheme(AppTheme theme)
-Future~void~ _watchThemeByName(themeName, generation)
-Future~void~ _reloadCurrentTheme()
-void _stopWatchingTheme()
+Future~void~ close()
}
class AppTheme {
+String themeName
+bool builtIn
+static Future~AppTheme~ fromName(themeName)
}
class FlowyDynamicPlugin {
+Directory source
+static bool isPlugin(FileSystemEntity entity)
}
FlowyPluginService --> PluginLocationService : uses
FlowyPluginService --> FlowyDynamicPlugin : discovers
AppearanceSettingsCubit --> FlowyPluginService : calls lookup
AppearanceSettingsCubit --> AppTheme : loads and reloads
FlowyDynamicPlugin --> Directory : wraps
PluginLocationService --> Directory : resolves locations
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
AppearanceSettingsCubit._watchThemeByNametheonError: (_) {}handler silently swallows file watching errors; consider at least logging them so failures in the watcher are visible during debugging and in production logs. - The plugin discovery and initialization paths (
_initPluginLocation,FlowyPluginService._targets, theme watcher) use multiple synchronous filesystem calls (existsSync,createSync,listSync) on the main isolate; consider switching these to async equivalents to avoid potential UI jank on slower disks or large plugin directories.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AppearanceSettingsCubit._watchThemeByName` the `onError: (_) {}` handler silently swallows file watching errors; consider at least logging them so failures in the watcher are visible during debugging and in production logs.
- The plugin discovery and initialization paths (`_initPluginLocation`, `FlowyPluginService._targets`, theme watcher) use multiple synchronous filesystem calls (`existsSync`, `createSync`, `listSync`) on the main isolate; consider switching these to async equivalents to avoid potential UI jank on slower disks or large plugin directories.
## Individual Comments
### Comment 1
<location path="frontend/appflowy_flutter/lib/workspace/application/settings/appearance/appearance_cubit.dart" line_range="337" />
<code_context>
+ _reloadCurrentTheme,
+ );
+ },
+ onError: (_) {},
+ );
+ }
</code_context>
<issue_to_address>
**suggestion:** Swallowing watcher errors entirely makes diagnosing theme-related issues harder.
The onError handler currently discards all exceptions. Please at least log the error (e.g., via Log.debug/Log.error) so we can diagnose cases where the theme directory becomes unreadable or the watcher fails. If log volume is a concern, consider restricting to unexpected error types or guarding behind a verbose flag.
Suggested implementation:
```
_themeWatcher = plugin.source.watch(recursive: true).listen(
(_) {
_themeReloadDebounce?.cancel();
_themeReloadDebounce = Timer(
const Duration(milliseconds: 500),
_reloadCurrentTheme,
);
},
onError: (Object error, StackTrace stackTrace) {
Log.error(
'Theme watcher encountered an error while watching the theme directory.',
error,
stackTrace,
);
},
```
```
);
```
1. Ensure the appropriate logging utility is imported at the top of this file. For example, if your project uses a `Log` utility, add:
`import 'package:appflowy/core/log.dart';`
or the correct path used elsewhere in the codebase.
2. If your `Log` API differs (e.g., `Log.debug` or different parameter order), adjust the call in the `onError` handler to match your existing logging conventions.
3. If your codebase prefers conditional/verbose logging for high-frequency errors, you may want to wrap `Log.error` in a verbosity check or filter specific error types before logging.
</issue_to_address>
### Comment 2
<location path="frontend/appflowy_flutter/packages/flowy_infra/lib/plugins/service/plugin_service.dart" line_range="25-40" />
<code_context>
+ final locations = await _locationService.allLocations;
+ final seen = <String>{};
+ final targets = <Directory>[];
+ for (final location in locations) {
+ if (!location.existsSync()) continue;
+ for (final entity in location.listSync()) {
+ if (FlowyDynamicPlugin.isPlugin(entity)) {
+ final dir = entity as Directory;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Listing plugin directories without per-location error handling can cause all plugin discovery to fail if a single location is problematic.
Because we now scan multiple directories, a single unreadable/permission-denied location will cause `listSync` to throw and stop discovery entirely. Please wrap the `listSync` call for each location in a try/catch, log the failure, and continue so that other valid plugin locations are still processed, especially for user-controlled `additionalLocations`.
```suggestion
final locations = await _locationService.allLocations;
final seen = <String>{};
final targets = <Directory>[];
for (final location in locations) {
if (!location.existsSync()) continue;
List<FileSystemEntity> entities;
try {
entities = location.listSync();
} on FileSystemException catch (e, stackTrace) {
// Log and continue so that a single problematic location
// doesn't prevent discovery in other locations.
print(
'Failed to list plugin location at ${location.path}: $e\n$stackTrace',
);
continue;
} catch (e, stackTrace) {
// Defensive catch-all for unexpected errors.
print(
'Unexpected error while listing plugin location at ${location.path}: $e\n$stackTrace',
);
continue;
}
for (final entity in entities) {
if (FlowyDynamicPlugin.isPlugin(entity)) {
final dir = entity as Directory;
final name = p.basename(dir.path);
if (seen.add(name)) {
targets.add(dir);
}
}
}
}
return targets;
```
</issue_to_address>
### Comment 3
<location path="frontend/appflowy_flutter/packages/flowy_infra/lib/plugins/service/location_service.dart" line_range="20" />
<code_context>
+ /// Returns the primary and all additional scan locations.
+ Future<List<Directory>> get allLocations async {
+ final primary = await fallback;
+ final extras = await Future.wait(_additionalLocations);
+ return [primary, ...extras];
+ }
</code_context>
<issue_to_address>
**suggestion:** A failure in one additional location prevents using any of the others.
Because `Future.wait` fails if any future throws (e.g. a location provider not supported on some platforms), one bad additional location will break `allLocations` entirely. If you want best-effort behavior, consider `Future.wait(_additionalLocations, eagerError: false)` and filtering out failures, or awaiting each future in a try/catch so failures in extras don’t prevent returning the primary and other valid locations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| _reloadCurrentTheme, | ||
| ); | ||
| }, | ||
| onError: (_) {}, |
Contributor
There was a problem hiding this comment.
suggestion: Swallowing watcher errors entirely makes diagnosing theme-related issues harder.
The onError handler currently discards all exceptions. Please at least log the error (e.g., via Log.debug/Log.error) so we can diagnose cases where the theme directory becomes unreadable or the watcher fails. If log volume is a concern, consider restricting to unexpected error types or guarding behind a verbose flag.
Suggested implementation:
_themeWatcher = plugin.source.watch(recursive: true).listen(
(_) {
_themeReloadDebounce?.cancel();
_themeReloadDebounce = Timer(
const Duration(milliseconds: 500),
_reloadCurrentTheme,
);
},
onError: (Object error, StackTrace stackTrace) {
Log.error(
'Theme watcher encountered an error while watching the theme directory.',
error,
stackTrace,
);
},
);
- Ensure the appropriate logging utility is imported at the top of this file. For example, if your project uses a
Logutility, add:
import 'package:appflowy/core/log.dart';
or the correct path used elsewhere in the codebase. - If your
LogAPI differs (e.g.,Log.debugor different parameter order), adjust the call in theonErrorhandler to match your existing logging conventions. - If your codebase prefers conditional/verbose logging for high-frequency errors, you may want to wrap
Log.errorin a verbosity check or filter specific error types before logging.
Comment on lines
+25
to
+40
| final locations = await _locationService.allLocations; | ||
| final seen = <String>{}; | ||
| final targets = <Directory>[]; | ||
| for (final location in locations) { | ||
| if (!location.existsSync()) continue; | ||
| for (final entity in location.listSync()) { | ||
| if (FlowyDynamicPlugin.isPlugin(entity)) { | ||
| final dir = entity as Directory; | ||
| final name = p.basename(dir.path); | ||
| if (seen.add(name)) { | ||
| targets.add(dir); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return targets; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Listing plugin directories without per-location error handling can cause all plugin discovery to fail if a single location is problematic.
Because we now scan multiple directories, a single unreadable/permission-denied location will cause listSync to throw and stop discovery entirely. Please wrap the listSync call for each location in a try/catch, log the failure, and continue so that other valid plugin locations are still processed, especially for user-controlled additionalLocations.
Suggested change
| final locations = await _locationService.allLocations; | |
| final seen = <String>{}; | |
| final targets = <Directory>[]; | |
| for (final location in locations) { | |
| if (!location.existsSync()) continue; | |
| for (final entity in location.listSync()) { | |
| if (FlowyDynamicPlugin.isPlugin(entity)) { | |
| final dir = entity as Directory; | |
| final name = p.basename(dir.path); | |
| if (seen.add(name)) { | |
| targets.add(dir); | |
| } | |
| } | |
| } | |
| } | |
| return targets; | |
| final locations = await _locationService.allLocations; | |
| final seen = <String>{}; | |
| final targets = <Directory>[]; | |
| for (final location in locations) { | |
| if (!location.existsSync()) continue; | |
| List<FileSystemEntity> entities; | |
| try { | |
| entities = location.listSync(); | |
| } on FileSystemException catch (e, stackTrace) { | |
| // Log and continue so that a single problematic location | |
| // doesn't prevent discovery in other locations. | |
| print( | |
| 'Failed to list plugin location at ${location.path}: $e\n$stackTrace', | |
| ); | |
| continue; | |
| } catch (e, stackTrace) { | |
| // Defensive catch-all for unexpected errors. | |
| print( | |
| 'Unexpected error while listing plugin location at ${location.path}: $e\n$stackTrace', | |
| ); | |
| continue; | |
| } | |
| for (final entity in entities) { | |
| if (FlowyDynamicPlugin.isPlugin(entity)) { | |
| final dir = entity as Directory; | |
| final name = p.basename(dir.path); | |
| if (seen.add(name)) { | |
| targets.add(dir); | |
| } | |
| } | |
| } | |
| } | |
| return targets; |
| /// Returns the primary and all additional scan locations. | ||
| Future<List<Directory>> get allLocations async { | ||
| final primary = await fallback; | ||
| final extras = await Future.wait(_additionalLocations); |
Contributor
There was a problem hiding this comment.
suggestion: A failure in one additional location prevents using any of the others.
Because Future.wait fails if any future throws (e.g. a location provider not supported on some platforms), one bad additional location will break allLocations entirely. If you want best-effort behavior, consider Future.wait(_additionalLocations, eagerError: false) and filtering out failures, or awaiting each future in a try/catch so failures in extras don’t prevent returning the primary and other valid locations.
|
|
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
2 participants
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.
Feature Preview
Custom themes now survive app updates and automatically reload when their JSON files are modified on disk — no re-import needed. This enables seamless integration with external theming tools (e.g. matugen, wallpaper scripts).
Theme persistence: Plugins are stored under the platform's app-support directory (Application Support on macOS, AppData\Roaming on Windows, ~/.local/share on Linux) instead of Documents. The legacy Documents folder is kept as a read-only fallback.
Hot-reload: The app watches the active theme's source directory for file-system changes and re-applies the theme automatically with a 500ms debounce.
fixes #8663
fixes #5873
PR Checklist