[WIP] Plugin support #4001
[WIP] Plugin support #4001
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
.eslintrc.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a comprehensive set of changes focused on enhancing theme management, plugin support, and configuration across multiple packages in the desktop client application. The modifications span several key areas:
The changes aim to create a more modular and extensible theming and plugin system for the desktop client, allowing for easier customization and future expansion of the application's visual and functional capabilities. Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Move file settings to the file management area![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
packages/plugins-shared/vite.config.mts (2)
8-8: Consider adding more DTS plugin options for better type generationWhile the basic DTS plugin configuration works, consider adding these options for better TypeScript support:
rollupTypes: truefor better type bundlingskipDiagnostics: falsefor stricter type checking- plugins: [dts({ insertTypesEntry: true, include: ['src'] })], + plugins: [dts({ + insertTypesEntry: true, + include: ['src'], + rollupTypes: true, + skipDiagnostics: false + })],
16-17: Consider adding source maps for better debuggingFor a shared library package, source maps would be valuable for debugging in development environments.
formats: ['es', 'cjs'], fileName: format => `index.${format}.js`, + }, + sourcemap: true,packages/desktop-client/src/style/theme.tsx (2)
59-71: Performance note: Spread syntax used in reduce accumulatorsWhile collecting plugin themes, repeated usage of spread syntax in “acc = { ...acc, ... }” inside a reduce can have O(n^2) complexity. If plugin lists become large, consider push or assignment-based merges to improve performance.
Below is an example of how you might refactor:
-acc = { - ...acc, - [theme]: { - name: theme, - colors: plugin.getThemeSchema(theme, false), - } -}; +acc[theme] = { + name: theme, + colors: plugin.getThemeSchema(theme, false), +};
72-84: Same performance note appliesThe same pattern of object accumulator spread is repeated for the “themesDark”. Consider using property assignment to avoid repeated object copies if performance becomes a concern.
packages/desktop-client/src/components/settings/Themes.tsx (2)
60-78: Performance note on spread with reduce accumulatorsLike in other files, spread syntax on each iteration can be inefficient at scale. Consider direct property assignment to “acc” object to avoid potential performance issues with large plugin arrays.
🧰 Tools
🪛 Biome (1.9.4)
[error] 69-69: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
80-98: Reusing accumulation pattern for 'themesDark'Again, to maintain performance, avoid repeated full object copying for each iteration. Consider a direct assignment pattern.
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
packages/desktop-client/src/components/ThemeSelector.tsx (4)
64-77: Accumulating plugin icons with repeated spread usageReusing spread syntax in a reduce accumulator can have performance downsides at scale. Consider direct property assignments to avoid O(n^2) complexity.
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
79-92: Same pattern for 'pluginIconsDark'Again, prefer direct assignments if performance concerns arise. Current approach is correct functionally, but not optimal in large plugin scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
93-111: Collecting plugin-provided themes (light, dark) via reduceSimilar performance reference: repeated object spreads in a loop can degrade performance. Library usage or direct assignments might be more efficient.
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
133-134: Merging extended plugin themesSame merge pattern as in other components. It’s functionally appropriate while having the same performance caution.
packages/plugins-shared/src/interfaces/actualPluginEntry.ts (1)
1-5: Well-designed plugin entry point type!The
ActualPluginEntrytype follows a good pattern by injecting React as a parameter, which helps avoid potential React version conflicts between the host application and plugins. This approach ensures that plugins use the same React instance as the host application.Consider documenting this pattern in the plugin development guide to help plugin authors understand why React is passed as a parameter and how to properly structure their plugins.
packages/plugins-shared/src/globals.d.ts (1)
1-6: Good approach for extending React's CSS types!The extension of React's
CSSPropertieswith Emotion'sCSSObjectis a clean way to enhance TypeScript's CSS type checking for your theming system. The ESLint disable comment is appropriately used and documented.Consider adding a comment explaining why this extension is necessary for the theming system, to help future maintainers understand its purpose.
packages/plugins-shared/src/interfaces/actualPlugin.ts (3)
5-8: Add validation and documentation for plugin metadataThe required
nameandversionfields lack format validation and documentation. Consider:
- Adding JSDoc comments describing expected formats
- Adding version format validation (e.g., semver)
Example documentation:
/** * Interface defining the contract for Actual Budget plugins */ export interface ActualPlugin { /** Unique identifier for the plugin. Should be kebab-case */ name: string; /** Plugin version following semver (e.g., "1.0.0") */ version: string; // ... }
8-14: Document theme-related method contractsThe theme-related methods lack documentation about their expected behavior and return values. Add JSDoc comments describing:
- Expected theme name formats
- Icon requirements/constraints
- Schema structure expectations
15-22: Clean up or document future hooksThe commented-out hooks section suggests incomplete functionality. Either:
- Remove the commented code until ready for implementation, or
- Add TODO comments explaining the planned functionality and timeline
packages/loot-core/src/types/prefs.d.ts (1)
89-89: Document customTheme relationship with Theme typeThe
customThemeproperty lacks documentation explaining:
- Its relationship with the
themeproperty- When it's used vs. built-in themes
- Format expectations
packages/desktop-client/src/components/ActualPluginsProvider.tsx (2)
124-151: Improve plugin loading architectureThe current implementation has several architectural concerns:
- No plugin validation/sandboxing
- Basic error handling
- No retry logic
- No version management
Consider:
- Implementing a plugin sandbox using Web Workers
- Adding proper validation of plugin exports
- Implementing retry logic with backoff
- Adding version compatibility checks
83-122: Clean up or implement commented GitHub integrationThe commented GitHub integration code should either be:
- Implemented with proper error handling and security measures
- Removed until ready for implementation
- Documented with TODO explaining the implementation plan
packages/plugins-shared/src/interfaces/themeDefinition.ts (1)
1-196: Add JSDoc documentation and consider type safety improvements.While the interface is well-structured, consider these enhancements:
- Add JSDoc documentation to describe:
- Expected color formats (hex, rgb, etc.)
- Usage guidelines for different theme properties
- Examples of valid values
- Consider using more specific types for color values:
+/** Theme interface for customizing the application's appearance. + * @property pageBackground - Main background color (hex: #RRGGBB or rgb(r,g,b)) + * @property pageText - Main text color + * ... + */ export interface ThemeDefinition { - pageBackground?: string; + pageBackground?: `#${string}` | `rgb(${number},${number},${number})`; // Apply similar type constraints to other color propertiespackages/loot-core/src/server/main.ts (2)
1335-1337: Add validation for customTheme value.Consider adding validation for the
customThemevalue before saving it to ensure it contains valid theme data.if ('customTheme' in prefs) { + if (typeof prefs.customTheme !== 'string') { + throw new Error('customTheme must be a string'); + } await asyncStorage.setItem('customTheme', prefs.customTheme); }
1383-1384: Consider extracting theme validation logic.The theme validation logic could be more maintainable if extracted into a separate function. This would make it easier to add new themes and maintain the validation logic in one place.
+ const isValidTheme = (theme: string) => { + const validThemes = ['light', 'dark', 'auto', 'development', 'midnight']; + return validThemes.includes(theme) || Boolean(customTheme); + }; + return { floatingSidebar: floatingSidebar === 'true' ? true : false, maxMonths: stringToInteger(maxMonths || ''), documentDir: documentDir || getDefaultDocumentDir(), keyId: encryptKey && JSON.parse(encryptKey).id, - theme: - theme === 'light' || - theme === 'dark' || - theme === 'auto' || - theme === 'development' || - theme === 'midnight' || - customTheme - ? theme - : 'auto', + theme: isValidTheme(theme) ? theme : 'auto', customTheme, preferredDarkTheme: preferredDarkTheme === 'dark' || preferredDarkTheme === 'midnight'Also applies to: 1387-1387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/plugins-shared/README.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.eslintrc.js(1 hunks)packages/desktop-client/public/index.es.js(1 hunks)packages/desktop-client/src/components/ActualPluginsProvider.tsx(1 hunks)packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/ThemeSelector.tsx(2 hunks)packages/desktop-client/src/components/settings/Themes.tsx(3 hunks)packages/desktop-client/src/style/theme.tsx(5 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/types/prefs.d.ts(2 hunks)packages/plugins-shared/.gitignore(1 hunks)packages/plugins-shared/package.json(1 hunks)packages/plugins-shared/src/globals.d.ts(1 hunks)packages/plugins-shared/src/index.ts(1 hunks)packages/plugins-shared/src/interfaces/actualPlugin.ts(1 hunks)packages/plugins-shared/src/interfaces/actualPluginEntry.ts(1 hunks)packages/plugins-shared/src/interfaces/themeDefinition.ts(1 hunks)packages/plugins-shared/tsconfig.json(1 hunks)packages/plugins-shared/vite.config.mts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/plugins-shared/tsconfig.json
- packages/plugins-shared/.gitignore
- packages/plugins-shared/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/plugins-shared/src/globals.d.ts
[error] 7-8: This empty export is useless because there's another export or import.
This import makes useless the empty export.
Safe fix: Remove this useless empty export.
(lint/complexity/noUselessEmptyExport)
packages/desktop-client/src/components/settings/Themes.tsx
[error] 69-69: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 88-88: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/desktop-client/src/components/ThemeSelector.tsx
[error] 70-70: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 84-84: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 101-101: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 120-120: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (41)
packages/plugins-shared/vite.config.mts (2)
1-6: LGTM: Imports and basic setup look good
The imports and basic configuration setup follow best practices, using proper ES module imports and the recommended Vite configuration pattern.
10-18: Verify the library name usage across the codebase
The library name 'Shared' is used as a global name. Ensure this doesn't conflict with other globals in the application.
✅ Verification successful
No conflict with the 'Shared' library name
The search results show that 'Shared' is only used as a library name in the Vite configuration and there are no conflicts with global variables. Other occurrences of "Shared" in the codebase are related to the browser's SharedArrayBuffer API, which is a different context and doesn't conflict with the library name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any potential naming conflicts with 'Shared'
rg -l "(?:var|let|const|window\.)Shared\b" --type ts --type tsx
Length of output: 93
Script:
#!/bin/bash
# Search for any potential naming conflicts with 'Shared' in all files
rg -l "(?:var|let|const|window\.)Shared\b"
# Also check for any usage of 'Shared' in general to understand the context
rg "Shared" -C 2
Length of output: 6915
packages/desktop-client/src/style/theme.tsx (12)
7-8: Exporting ThemeDefinition is appropriate
Importing and using “ThemeDefinition” clarifies theme typing across multiple files, promoting type consistency. No issues found here.
16-16: Good practice: Exporting 'themes' for external access
Exporting “themes” fosters modularity and meaningful cross-component usage. The structure is clean, and the environment-based addition of “development” is well-encapsulated with a condition.
37-38: Neat exposure of customTheme
Exposing “[customTheme, setCustomTheme]” ensures that custom themes can be loaded and saved dynamically. Looks good.
48-50: Use of useState with 'themesExtended'
“themesExtended” merges base themes with plugin themes. Straightforward approach, no issues noted here.
52-54: Maintaining themeColors in state
Storing “themeColors” locally with useState is a sensible approach, ensuring updates propagate correctly. No concerns here.
56-56: Proper destructuring from useActualPlugins
Neatly extracts “plugins” from “useActualPlugins”, facilitating plugin-driven theme logic. No issues found.
85-86: Merging plugin-provided themes with base themes
By calling “setThemesExtended({ ...themes, ...themesLight, ...themesDark }),” you incorporate plugin-based themes seamlessly. This approach is correct from a functionality standpoint. Watch out for potential naming collisions if a plugin-based theme’s key matches an existing base theme name.
88-92: Loading customTheme from global prefs
Properly prioritizes “customTheme” if it exists, returning early after setting theme colors. This is a valid approach for user customizations.
94-101: Responsive theme switching for 'auto' preference
Effectively handles system dark-mode preference changes at runtime by listening to the media query. Good approach for real-time theme updates.
116-116: Fallback to the 'light' theme
When the system preference is set to “light,” the code sets “themesExtended[‘light’].colors.” This fallback ensures consistent theming. No issues identified.
126-126: Applying user-specified theme
When not using “auto,” it directly sets the chosen theme’s colors. Straightforward mechanism, no issues detected.
128-128: Overall thorough logic
The final effect returns a unified <style> element reflecting the currently active or custom theme. This is logically correct, clean, and modular.
packages/desktop-client/src/components/settings/Themes.tsx (5)
42-47: Defining ThemesExtendedType
This custom type helps unify plugin-based and base themes under a single shape, improving readability and type safety.
52-52: Introducing setThemeObject in useTheme array destructuring
• “setThemeObject” usage hints at storing a full theme object into global prefs.
• “themesExtended” and “themeOptionsExtended” facilitate dynamic plugin-based theme insertion.
All changes look consistent with the new plugin-driven theming approach.
Also applies to: 54-59
99-100: Merging plugin themes with base 'themes'
Appropriately merges plugin-based light/dark themes into a single extended structure. Watch for name collisions if a plugin uses a name that is already defined in “themes.”
102-108: Updating themeOptionsExtended from themesExtended
Efficient approach to keep the UI’s selection options in sync with loaded plugin themes. Looks good.
131-136: Conditional call to setThemeObject
When a user selects a theme not originally in “themes,” it attempts to store that as a custom theme via JSON serialization. The logic is straightforward. Ensure robust JSON error handling if malformed data is encountered.
packages/desktop-client/src/components/ThemeSelector.tsx (9)
1-8: Importing new hooks and types
Imports for “useEffect,” “useMemo,” “SVGProps,” etc., reflect the expanded dynamic theming. No issues noted on these lines.
13-13: Adding ThemeDefinition, themes, useTheme, useActualPlugins
These additions are consistent with the plugin-based theming approach across the codebase. Implementation aligns with usage in other modules.
Also applies to: 15-15, 17-17
21-21: Setup for ThemesExtendedType, ThemesIconsType, and local states
• “themesExtended” ensures that plugin-provided themes can be appended.
• “themeIcons” allows distinct icons per plugin-based theme.
Well-structured approach.
Also applies to: 28-37
43-46: useState initialization with base 'themes' and 'themeOptions'
Establishes default states before layering custom plugin logic. This is consistent with the broader theming mechanism.
51-60: Memoization of baseIcons
The “useMemo” approach for base theme icons prevents unnecessary re-renders. Implementation is correct.
61-62: Stateful 'themeIcons'
Retains plugin-injected icons for new themes. No immediate concerns, usage is consistent with the final merges.
131-132: Combining baseIcons with pluginIcons
Merging “baseIcons” with “pluginIconsLight” and “pluginIconsDark” keeps the logic straightforward. Again, watch out for key collisions.
136-142: Keep themeOptionsExtended in sync
Syncing “themeOptionsExtended” to newly extended themes is consistent with the dynamic plugin approach across the codebase. No issues detected.
175-175: Rendering theme menu items with extended options
Now referencing the merged “themeOptionsExtended” ensures plugin-based themes are fully integrated. Implementation is correct.
packages/desktop-client/public/index.es.js (7)
1-17: Dynamic SVG creation via 'c' and usage in 'u'
• Using a factory function to create SVG with inherited style is a neat approach.
• Proper usage of “...t” spread for props is acceptable here since it’s not in a tight loop.
No issues found.
18-26: Return different elements based on themeName
The switch-case toggles between “dracula” and a fallback
28-229: Dracula theme definitions
A thorough, consistent set of color keys for the “Dracula” theme. This approach is consistent with the code’s theming pattern, no functional concerns.
230-412: Sepia theme definitions
Again, a large mapping of color values for a “Sepia” style theme. The naming convention and consistency appear good.
413-637: Rose Pine theme definitions
Thorough coloring for each UI element. Similar approach with clear naming. No correctness issues noted.
638-808: Tokyo Night theme definitions
Completes the theme set with a large set of color properties. Methodical structure, no logical flaws detected.
809-834: Default export function 'i' integrating plugin interface
• Exposes “availableThemes,” “getThemeIcon,” and “getThemeSchema” for custom usage.
• The “switch” logic in “d” reads cleanly, returning the correct theme object.
No concerns found.
packages/plugins-shared/src/index.ts (1)
1-3: Clean type exports forming a well-structured plugin API!
The type-only exports create a clear public API for the plugins package. The separation of interfaces into distinct files while aggregating exports in index.ts follows good TypeScript practices.
packages/plugins-shared/src/globals.d.ts (1)
8-8: Verify if the empty export is required
While static analysis suggests removing this empty export, it might be necessary to maintain the file as a module. Let's verify its usage across the codebase.
✅ Verification successful
Let me check the actual content of the file that imports from globals to understand its usage.
Empty export is required - it marks the file as a module
The empty export is necessary in this case as the file contains a module augmentation for React's CSSProperties interface using the declare module syntax. The empty export ensures the file is treated as a module rather than an ambient declaration file, which is required for module augmentations to work correctly in TypeScript.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any direct imports of this module
rg -l "from ['\"].*globals['\"]"
# Check if there are any references to the CSS properties extension
ast-grep --pattern 'interface $_ extends CSSProperties'
Length of output: 141
Script:
#!/bin/bash
# Check the content of the file importing globals
cat packages/loot-core/src/server/budget/util.ts
# Also check the content of globals.d.ts itself to understand what it declares
cat packages/plugins-shared/src/globals.d.ts
Length of output: 901
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: This empty export is useless because there's another export or import.
This import makes useless the empty export.
Safe fix: Remove this useless empty export.
(lint/complexity/noUselessEmptyExport)
packages/desktop-client/src/components/App.tsx (2)
176-218: LGTM: Provider hierarchy is well-structured.
The ActualPluginsProvider is correctly positioned as the outermost provider (after BrowserRouter), ensuring plugin functionality is available throughout the component tree. The existing providers are properly nested within it, maintaining their original order and relationships.
184-189: LGTM: Theme integration is properly implemented.
The View component's data-theme attribute and style properties are correctly configured for theme support:
- data-theme binding to the theme value
- Proper flex layout properties
- Consistent style structure
.eslintrc.js (1)
66-66: Verify the scope of ignored files.
The pattern **/public/*.js will exclude all JavaScript files in any public directory. Let's verify this won't exclude files that should be linted.
✅ Verification successful
ESLint ignore pattern is appropriate for generated files
The ignore pattern **/public/*.js is correctly scoped:
- Only affects one public directory:
packages/desktop-client/public/ - The JS file in this directory (
index.es.js) is clearly a generated file containing bundled SVG components - This is supported by the webpack configuration presence, indicating this is a build output directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check what files would be ignored by the new pattern
# List all .js files in public directories
fd -g '**/public/*.js'
# Show the content of these files to verify they are indeed generated/build files
fd -g '**/public/*.js' -x head -n 5
Length of output: 62
Script:
#!/bin/bash
# Let's try a different approach to understand the codebase structure
# and potential impact of the ignore pattern
# First, let's see if there are any public directories
fd -t d 'public'
# Let's also check for any JavaScript files that might be affected
# by looking at the overall JS file structure
fd -e js
# Additionally, let's check if there's any documentation about
# the public directory usage in README or similar files
fd -g 'README*' -X grep -l 'public'
Length of output: 3023
Script:
#!/bin/bash
# Let's examine the content of the only JS file found in public directory
# to verify if it's a generated/build file
head -n 10 "packages/desktop-client/public/index.es.js"
# Let's also check if there are any build configuration files
# that might indicate the purpose of the public directory
fd -g '*webpack*' -X grep -l 'public'
Length of output: 433
packages/loot-core/src/server/main.ts (1)
1360-1360: LGTM!
The addition of customTheme to the preferences loading logic is implemented correctly and follows the established pattern.
Also applies to: 1369-1369
| external: ['react', 'react-dom', '@emotion/css'], | ||
| output: { | ||
| globals: { | ||
| react: 'React', | ||
| 'react-dom': 'ReactDOM', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
@emotion/css should be added to globals configuration
Based on the extensive usage of @emotion/css throughout the codebase (over 50 files importing it) and its current inclusion in the external array, it should also be configured in the globals configuration to ensure proper bundling.
external: ['react', 'react-dom', '@emotion/css'],
output: {
globals: {
react: 'React',
'react-dom': 'ReactDOM',
+ '@emotion/css': 'emotionCss',
},
},🔗 Analysis chain
Consider expanding external dependencies and globals configuration
The current configuration might be incomplete:
@emotion/cssis marked as external but missing in globals- Other potential shared dependencies might need to be added
Consider updating the configuration:
external: ['react', 'react-dom', '@emotion/css'],
output: {
globals: {
react: 'React',
'react-dom': 'ReactDOM',
+ '@emotion/css': 'emotionCss',
},
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other potential shared dependencies in the project
echo "Checking package.json files for common dependencies..."
fd package.json$ --type f --exec jq -r '.dependencies // {} | keys[]' {} \; | sort | uniq -c | sort -nr
# Check for @emotion/css usage
echo "Checking @emotion/css usage patterns..."
rg --type ts --type tsx "from ['|\"]@emotion/css['|\"]" -A 3
Length of output: 4850
Script:
#!/bin/bash
# Let's fix the previous commands and try again
# Check package dependencies
echo "Checking package.json files for dependencies..."
fd package.json --type f --exec cat {} \; | jq -r '.dependencies | keys[]' 2>/dev/null | sort | uniq -c | sort -nr
# Check for @emotion/css imports
echo -e "\nChecking @emotion/css usage patterns..."
rg "from ['\"]@emotion/css['\"]"
# Check for any other emotion-related imports
echo -e "\nChecking other emotion-related imports..."
rg "from ['\"]@emotion/"
Length of output: 13786
| export type Theme = | ||
| | 'light' | ||
| | 'dark' | ||
| | 'auto' | ||
| | 'midnight' | ||
| | 'development' | ||
| | string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a branded type for Theme safety
Adding string to the Theme union type reduces type safety. Consider:
- Using a branded type to ensure theme names are valid
- Creating a theme registry type for plugin-provided themes
Example:
type BuiltInTheme = 'light' | 'dark' | 'auto' | 'midnight' | 'development';
type BrandedPluginTheme = string & { __brand: 'PluginTheme' };
export type Theme = BuiltInTheme | BrandedPluginTheme;| const loaded = await Promise.all([ | ||
| loadPluginFromRepo('https://github.com/actual-plugins/example'), | ||
| ]); |
There was a problem hiding this comment.
Security: Implement plugin source validation
Loading plugins directly from GitHub without validation poses security risks. Consider:
- Implementing a plugin registry/marketplace
- Adding plugin signature verification
- Using a whitelist of approved plugin sources
| window.React = React; | ||
|
|
There was a problem hiding this comment.
Remove global React assignment
Setting window.React is an anti-pattern that:
- Creates global namespace pollution
- May cause version conflicts
- Makes testing more difficult
Consider passing React as a parameter or using proper module bundling.
| // ); | ||
|
|
||
| // console.log(`Downloading plugin “${repo}” v${latestVersion}...`); | ||
| const response = await fetch('http://localhost:3001/index.es.js'); |
There was a problem hiding this comment.
Security: Remove hardcoded localhost URL
Using a hardcoded localhost URL is not production-ready:
- Won't work in production
- No HTTPS enforcement
- No error handling for different environments
Consider:
const PLUGIN_REGISTRY_URL = process.env.PLUGIN_REGISTRY_URL || 'https://plugins.actualbudget.com';
// Add proper error handling for different environments
const response = await fetch(`${PLUGIN_REGISTRY_URL}/plugins/${pluginId}/latest`);
Second implementation of plugin support. This is not ready for nothing