Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Plugin support #4001

Closed
wants to merge 16 commits into from
Closed

[WIP] Plugin support #4001

wants to merge 16 commits into from

Conversation

lelemm
Copy link
Contributor

@lelemm lelemm commented Dec 18, 2024

Second implementation of plugin support. This is not ready

@actual-github-bot actual-github-bot bot changed the title Plugin support [WIP] Plugin support Dec 18, 2024
@lelemm lelemm changed the title [WIP] Plugin support [Plugin support Dec 18, 2024
Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit f079b61
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/6772c215afe8d50008b0dd00
😎 Deploy Preview https://deploy-preview-4001.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lelemm lelemm changed the title [Plugin support [WIP] Plugin support Dec 18, 2024
Copy link
Contributor

github-actions bot commented Dec 18, 2024

Bundle Stats — desktop-client

Hey 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

Files count Total bundle size % Changed
10 5.64 MB → 5.67 MB (+27.3 kB) +0.47%
Changeset
File Δ Size
src/components/modals/SelectNewPluginModal.tsx 🆕 +9.08 kB 0 B → 9.08 kB
src/components/ActualPluginsProvider.tsx 🆕 +7.49 kB 0 B → 7.49 kB
src/components/plugins/ManagePlugins.tsx 🆕 +5.82 kB 0 B → 5.82 kB
src/components/plugins/PluginsHeader.tsx 🆕 +827 B 0 B → 827 B
src/icons/v1/Plugin.tsx 🆕 +523 B 0 B → 523 B
src/components/plugins/Plugins.tsx 🆕 +214 B 0 B → 214 B
src/components/settings/Themes.tsx 📈 +1.28 kB (+54.55%) 2.35 kB → 3.63 kB
src/style/theme.tsx 📈 +895 B (+38.28%) 2.28 kB → 3.16 kB
src/components/sidebar/PrimaryButtons.tsx 📈 +262 B (+12.93%) 1.98 kB → 2.24 kB
home/runner/work/actual/actual/packages/loot-core/src/platform/server/indexeddb/index.web.ts 📈 +141 B (+8.72%) 1.58 kB → 1.72 kB
src/components/settings/Experimental.tsx 📈 +281 B (+6.60%) 4.16 kB → 4.43 kB
src/hooks/useFeatureFlag.ts 📈 +18 B (+5.04%) 357 B → 375 B
src/components/App.tsx 📈 +176 B (+2.94%) 5.84 kB → 6.01 kB
src/components/FinancesApp.tsx 📈 +229 B (+1.93%) 11.59 kB → 11.81 kB
src/components/Modals.tsx 📈 +160 B (+0.93%) 16.76 kB → 16.92 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/index.js 3.71 MB → 3.74 MB (+27.9 kB) +0.73%

Smaller

Asset File Size % Changed
static/js/narrow.js 83.36 kB → 82.76 kB (-608 B) -0.71%

Unchanged

Asset File Size % Changed
static/js/useAccountPreviewTransactions.js 1.63 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/AppliedFilters.js 10.24 kB 0%
static/js/workbox-window.prod.es5.js 5.69 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/wide.js 104.11 kB 0%
static/js/ReportRouter.js 1.58 MB 0%

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Bundle Stats — loot-core

Hey 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

Files count Total bundle size % Changed
1 1.33 MB → 1.33 MB (+180 B) +0.01%
Changeset
File Δ Size
packages/loot-core/src/platform/server/indexeddb/index.web.ts 📈 +179 B (+3.39%) 5.16 kB → 5.34 kB
packages/loot-core/src/server/main.ts 📈 +186 B (+0.26%) 69.47 kB → 69.65 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
kcab.worker.js 1.33 MB → 1.33 MB (+180 B) +0.01%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

Copy link
Contributor

coderabbitai bot commented Dec 18, 2024

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/desktop-client/src/components/ActualPluginsProvider.tsx

Oops! 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:

npm install eslint-plugin-eslint-plugin@latest --save-dev

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.

Walkthrough

This 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:

  1. ESLint Configuration: The .eslintrc.js file now includes an ignorePatterns property to exclude specific JavaScript files from linting.

  2. Theme Management: A new system for dynamic theme handling has been implemented, allowing for plugin-based theme extensions. This includes:

    • New context for managing actual plugins
    • Enhanced theme selection components
    • Updated theme preference handling in global preferences
    • Extended theme type definitions
  3. Plugin Infrastructure: A new plugins-shared package has been created to support plugin development, including:

    • Shared interfaces for plugins
    • TypeScript configuration
    • Build and development setup
  4. Type System Enhancements: The type definitions have been expanded to support more flexible theme configurations and plugin integrations.

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

  • :electron: Move file settings to the file management area #3584: Introduces a new ignorePatterns property in the ESLint configuration, which may relate to the changes in file management and settings in this PR, particularly regarding how files are handled and ignored during linting processes.
  • Duplicate Budget #3847: The addition of the DuplicateFileModal and related functionalities in this PR could be relevant as it involves file management, which aligns with the changes made in the main PR regarding file handling and exclusions in the ESLint configuration.

Suggested reviewers

  • youngcw
  • MikesGlitch

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 generation

While the basic DTS plugin configuration works, consider adding these options for better TypeScript support:

  • rollupTypes: true for better type bundling
  • skipDiagnostics: false for 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 debugging

For 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 accumulators

While 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 applies

The 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 accumulators

Like 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 of O(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 of O(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 usage

Reusing 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 of O(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 of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)


93-111: Collecting plugin-provided themes (light, dark) via reduce

Similar 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 of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)


133-134: Merging extended plugin themes

Same 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 ActualPluginEntry type 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 CSSProperties with Emotion's CSSObject is 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 metadata

The required name and version fields 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 contracts

The 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 hooks

The commented-out hooks section suggests incomplete functionality. Either:

  1. Remove the commented code until ready for implementation, or
  2. 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 type

The customTheme property lacks documentation explaining:

  • Its relationship with the theme property
  • When it's used vs. built-in themes
  • Format expectations
packages/desktop-client/src/components/ActualPluginsProvider.tsx (2)

124-151: Improve plugin loading architecture

The current implementation has several architectural concerns:

  1. No plugin validation/sandboxing
  2. Basic error handling
  3. No retry logic
  4. No version management

Consider:

  1. Implementing a plugin sandbox using Web Workers
  2. Adding proper validation of plugin exports
  3. Implementing retry logic with backoff
  4. Adding version compatibility checks

83-122: Clean up or implement commented GitHub integration

The commented GitHub integration code should either be:

  1. Implemented with proper error handling and security measures
  2. Removed until ready for implementation
  3. 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:

  1. Add JSDoc documentation to describe:
    • Expected color formats (hex, rgb, etc.)
    • Usage guidelines for different theme properties
    • Examples of valid values
  2. 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 properties
packages/loot-core/src/server/main.ts (2)

1335-1337: Add validation for customTheme value.

Consider adding validation for the customTheme value 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

📥 Commits

Reviewing files that changed from the base of the PR and between 94666a2 and ebc794a.

⛔ Files ignored due to path filters (2)
  • packages/plugins-shared/README.md is excluded by !**/*.md
  • yarn.lock is 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

. This logic is valid for initial theme icon resolution.


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

Comment on lines +20 to +26
external: ['react', 'react-dom', '@emotion/css'],
output: {
globals: {
react: 'React',
'react-dom': 'ReactDOM',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. @emotion/css is marked as external but missing in globals
  2. 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

Comment on lines +73 to +79
export type Theme =
| 'light'
| 'dark'
| 'auto'
| 'midnight'
| 'development'
| string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using a branded type for Theme safety

Adding string to the Theme union type reduces type safety. Consider:

  1. Using a branded type to ensure theme names are valid
  2. 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;

Comment on lines 34 to 36
const loaded = await Promise.all([
loadPluginFromRepo('https://github.com/actual-plugins/example'),
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Implement plugin source validation

Loading plugins directly from GitHub without validation poses security risks. Consider:

  1. Implementing a plugin registry/marketplace
  2. Adding plugin signature verification
  3. Using a whitelist of approved plugin sources

Copy link
Contributor

👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review.

@github-actions github-actions bot added the Stale label Dec 26, 2024
@lelemm
Copy link
Contributor Author

lelemm commented Dec 27, 2024

@coderabbitai review

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

♻️ Duplicate comments (2)
packages/desktop-client/src/components/ActualPluginsProvider.tsx (2)

34-36: ⚠️ Potential issue

Security: Implement plugin source validation and configuration

Loading plugins from a hardcoded GitHub repository URL is not production-ready and poses security risks.

Consider:

  1. Moving the plugin source URL to a configuration file
  2. Implementing plugin source validation
  3. Adding support for multiple plugin sources
  4. Using HTTPS endpoints only
-      const loaded = await Promise.all([
-        loadPluginFromRepo('https://github.com/actual-plugins/example'),
-      ]);
+      const pluginSources = await getConfiguredPluginSources();
+      const loaded = await Promise.all(
+        pluginSources.map(source => loadPluginFromRepo(source))
+      );

67-67: ⚠️ Potential issue

Remove global React assignment

Setting window.React is an anti-pattern that creates global namespace pollution and may cause version conflicts.

Consider passing React as a parameter through the plugin API instead.

🧹 Nitpick comments (10)
packages/plugins-shared/src/interfaces/actualPluginManifest.ts (1)

1-8: Consider using enums or documented constant types for pluginType.
While this interface is clear and concise, using an enum or well-documented constant union type for pluginType could enhance its extensibility and maintainability if more plugin types are introduced later.

packages/plugins-shared/package.json (1)

1-21: Consider adding missing metadata fields and aligning dependency versioning strategies.
Adding fields such as license, repository, or author can make the package more informative. Also, it might be beneficial to standardize how versions are pinned (e.g., caret vs. exact) across dependencies for consistency.

packages/desktop-client/src/style/theme.tsx (3)

16-16: Consider documenting exported themes.
The themes object is instrumental to theming. Adding a short JSDoc or comment explaining each built-in theme’s purpose can improve clarity for future maintainers.


58-73: Consider prefixing plugin-provided themes to avoid name collisions.
If different plugins define a “dark” or “light” theme, merging them here might cause a collision. Prefixing with the plugin’s slug or name could provide uniqueness.


Line range hint 82-115: Refactor repetitive assignment of theme colors.
Multiple lines assign themeColors from either darkTheme.colors or themesExtended['light'].colors. Extracting a small helper function can make it more readable and reduce duplication.

packages/desktop-client/src/components/ActualPluginsProvider.tsx (1)

83-122: Remove or implement commented code

The commented-out utility functions for GitHub integration appear to be well-structured but are currently unused.

Either:

  1. Remove the commented code if it's not needed
  2. Implement the functionality if it's required
  3. Add a TODO comment explaining why it's preserved
packages/desktop-client/src/components/ThemeSelector.tsx (1)

64-71: Simplify plugin theme aggregation

The plugin theme aggregation logic is duplicated and could be simplified.

Consider extracting the common validation logic into a utility function:

const validatePlugin = (plugin: ActualPlugin) => 
  plugin?.availableThemes?.() ?? [];

const getValidThemes = (plugin: ActualPlugin) =>
  validatePlugin(plugin).filter(theme => theme !== undefined);

Also applies to: 83-93

🧰 Tools
🪛 Biome (1.9.4)

[error] 68-69: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/desktop-client/src/components/settings/Themes.tsx (1)

116-123: Improve theme object handling

The theme object handling could be more robust.

Consider:

  1. Using type guards for theme validation
  2. Adding error handling for invalid theme objects
  3. Implementing a more explicit state management approach
-                if (
-                  !Object.keys(themes).some(t => t === value) &&
-                  typeof themesExtended === 'object'
-                ) {
-                  setThemeObject(JSON.stringify(themesExtended?.[value] ?? ''));
-                } else {
-                  setThemeObject(null);
-                }
+                const themeObject = getThemeObject(value, themes, themesExtended);
+                setThemeObject(themeObject);
packages/desktop-client/public/index.es.js (2)

1-19: Enhance SVG icon accessibility and flexibility.

The SVG icon implementation could be improved in the following areas:

  1. Add ARIA attributes for accessibility
  2. Make the icon path configurable instead of hardcoded
  3. Make the viewBox dimensions configurable

Consider applying these changes:

 const c = (e) => (a) => /* @__PURE__ */ e.createElement(
   "svg",
   {
     ...a,
     xmlns: "http://www.w3.org/2000/svg",
-    viewBox: "0 0 20 20",
+    viewBox: a.viewBox || "0 0 20 20",
+    role: "img",
+    "aria-hidden": "true",
     style: {
       color: "inherit",
       ...a.style
     }
   },
   /* @__PURE__ */ e.createElement(
     "path",
     {
-      d: "M11 9.27V0l6 11-4 6H7l-4-6L9 0v9.27a2 2 0 1 0 2 0zM6 18h8v2H6v-2z",
+      d: a.path || "M11 9.27V0l6 11-4 6H7l-4-6L9 0v9.27a2 2 0 1 0 2 0zM6 18h8v2H6v-2z",
       fill: "currentColor"
     }
   )
 )

28-804: Consider enhancing theme configuration maintainability.

The theme configurations are well-structured and thoroughly documented. However, consider these improvements:

  1. Extract common color values into shared variables
  2. Use CSS variables for runtime theme customization
  3. Add color contrast validation for accessibility

Would you like me to help generate:

  1. A color palette extraction utility
  2. CSS variable integration
  3. A color contrast validation script?
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebc794a and af2c85e.

⛔ Files ignored due to path filters (2)
  • upcoming-release-notes/4001.md is excluded by !**/*.md
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (10)
  • packages/desktop-client/public/index.es.js (1 hunks)
  • packages/desktop-client/src/components/ActualPluginsProvider.tsx (1 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/types/prefs.d.ts (2 hunks)
  • packages/plugins-shared/package.json (1 hunks)
  • packages/plugins-shared/src/index.ts (1 hunks)
  • packages/plugins-shared/src/interfaces/actualPlugin.ts (1 hunks)
  • packages/plugins-shared/src/interfaces/actualPluginManifest.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/loot-core/src/types/prefs.d.ts
  • packages/plugins-shared/src/interfaces/actualPlugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/components/ThemeSelector.tsx

[error] 68-69: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 86-87: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 74-74: 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] 95-95: 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/settings/Themes.tsx

[error] 64-65: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 73-73: 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 (8)
packages/plugins-shared/src/index.ts (1)

1-5: Export statements look good.
Re-exporting each type individually keeps the index file clean and explicit.

packages/desktop-client/src/style/theme.tsx (6)

7-8: Imports from plugins-shared and ActualPluginsProvider look correct.
These imports properly integrate plugin-related types and logic into the theme system.


37-38: Double-check the structure of customTheme.
If customTheme is expected to be valid JSON, handling parse failures (e.g., via a try/catch in the consumer) could prevent runtime errors in case the stored preference is corrupted.


48-50: State setup is straightforward.
Initializing themesExtended from the default themes simplifies the subsequent merging logic.


52-55: Good approach using a union type for themeColors.
Managing themeColors as ThemeDefinition | undefined is a clean way to handle the initialization cycle until themes are resolved.


56-57: Properly obtaining loadedPlugins.
Retrieving plugins from the context ensures the plugin-based themes can be integrated cleanly.


80-80: Blank line is fine.
No special concerns.

packages/desktop-client/src/components/ActualPluginsProvider.tsx (1)

137-137: ⚠️ Potential issue

Security: Remove hardcoded plugin path

Using a hardcoded path '/index.es.js' is not production-ready and poses security risks.

Consider:

  1. Implementing a proper plugin registry
  2. Using versioned plugin URLs
  3. Adding integrity checks for downloaded content
-    const response = await fetch('/index.es.js');
+    const pluginMetadata = await fetchPluginMetadata(repo);
+    const response = await fetch(pluginMetadata.url, {
+      headers: { 'Accept': 'application/javascript' }
+    });

Likely invalid or redundant comment.

Comment on lines +76 to +79
if (customTheme) {
setThemeColors(JSON.parse(customTheme).colors);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle malformed customTheme parsing.
JSON parsing errors can break the component. Consider a safeguard like try/catch or a fallback if parsing fails.

- setThemeColors(JSON.parse(customTheme).colors);
+ try {
+   const parsed = JSON.parse(customTheme);
+   setThemeColors(parsed.colors);
+ } catch (e) {
+   console.error('Invalid customTheme JSON', e);
+   setThemeColors(undefined);
+ }
📝 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
if (customTheme) {
setThemeColors(JSON.parse(customTheme).colors);
return;
}
if (customTheme) {
try {
const parsed = JSON.parse(customTheme);
setThemeColors(parsed.colors);
} catch (e) {
console.error('Invalid customTheme JSON', e);
setThemeColors(undefined);
}
return;
}

Comment on lines 143 to 146
const indexJsBlob = await response.blob();

console.log(`Plugin “${repo}” loaded successfully.`);
return await loadPluginScript(indexJsBlob);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance plugin loading error handling

The current implementation logs success before actually loading the plugin and provides minimal error context.

Consider:

  1. Moving the success log after successful plugin initialization
  2. Adding detailed error information
  3. Implementing plugin validation before loading
-    console.log(`Plugin "${repo}" loaded successfully.`);
-    return await loadPluginScript(indexJsBlob);
+    const plugin = await loadPluginScript(indexJsBlob);
+    await validatePlugin(plugin);
+    console.log(`Plugin "${repo}" v${plugin.version} loaded successfully.`);
+    return plugin;

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +74 to +77
...acc,
[theme]: (props: SVGProps<SVGSVGElement>) =>
plugin?.getThemeIcon?.(theme, props.style) ?? <View />,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize reducer performance

Using spread operator in reducers can lead to O(n²) time complexity.

Consider using array methods or object manipulation:

-            acc = {
-              ...acc,
-              [theme]: (props: SVGProps<SVGSVGElement>) =>
-                plugin?.getThemeIcon?.(theme, props.style) ?? <View />,
-            };
+            acc[theme] = (props: SVGProps<SVGSVGElement>) =>
+              plugin?.getThemeIcon?.(theme, props.style) ?? <View />;
📝 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
...acc,
[theme]: (props: SVGProps<SVGSVGElement>) =>
plugin?.getThemeIcon?.(theme, props.style) ?? <View />,
};
acc[theme] = (props: SVGProps<SVGSVGElement>) =>
plugin?.getThemeIcon?.(theme, props.style) ?? <View />;
🧰 Tools
🪛 Biome (1.9.4)

[error] 74-74: 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)

Comment on lines +72 to +79
acc = {
...acc,
[theme]: {
name: theme,
colors: plugin?.getThemeSchema?.(theme) ?? {},
},
};
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize reducer performance and extract shared logic

This component has similar performance issues and duplicated logic as ThemeSelector.

  1. Extract shared theme management logic into a custom hook
  2. Optimize reducer performance by avoiding spread operator
  3. Consider implementing a theme registry service
// themeRegistry.ts
export const useThemeRegistry = () => {
  const [themes, setThemes] = useState({});
  
  const registerTheme = (theme: string, definition: ThemeDefinition) => {
    setThemes(current => {
      current[theme] = definition;
      return current;
    });
  };
  
  return { themes, registerTheme };
};
🧰 Tools
🪛 Biome (1.9.4)

[error] 73-73: 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)

Comment on lines 19 to 28
), u = (e) => {
const t = c(e);
return ({ themeName: d, darkMode: r, style: o }) => {
switch (d) {
case "dracula":
return /* @__PURE__ */ e.createElement(t, { style: o });
}
return /* @__PURE__ */ e.createElement("div", { style: o });
};
}, b = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix theme name consistency and improve fallback handling.

The theme icon component has several issues:

  1. Theme name case mismatch: "dracula" vs "Dracula" in availableThemes
  2. Empty div fallback provides poor UX for unsupported themes
  3. No error handling for invalid theme names

Consider applying these changes:

 const u = (e) => {
   const t = c(e);
   return ({ themeName: d, darkMode: r, style: o }) => {
+    const normalizedTheme = d?.toLowerCase();
     switch (d) {
-      case "dracula":
+      case "dracula": // TODO: Remove after migration
+      case "Dracula":
         return /* @__PURE__ */ e.createElement(t, { style: o });
+      default:
+        console.warn(`Theme icon not found for theme: ${d}`);
+        return /* @__PURE__ */ e.createElement(t, { 
+          style: { ...o, opacity: 0.5 }, 
+          title: `${d} theme` 
+        });
     }
-    return /* @__PURE__ */ e.createElement("div", { style: o });
   };
 };
📝 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
), u = (e) => {
const t = c(e);
return ({ themeName: d, darkMode: r, style: o }) => {
switch (d) {
case "dracula":
return /* @__PURE__ */ e.createElement(t, { style: o });
}
return /* @__PURE__ */ e.createElement("div", { style: o });
};
}, b = {
), u = (e) => {
const t = c(e);
return ({ themeName: d, darkMode: r, style: o }) => {
const normalizedTheme = d?.toLowerCase();
switch (d) {
case "dracula": // TODO: Remove after migration
case "Dracula":
return /* @__PURE__ */ e.createElement(t, { style: o });
default:
console.warn(`Theme icon not found for theme: ${d}`);
return /* @__PURE__ */ e.createElement(t, {
style: { ...o, opacity: 0.5 },
title: `${d} theme`
});
}
};
}, b = {

Comment on lines 805 to 830
}, i = (e) => {
const t = u(e), a = {
name: "Example",
version: "0.0.1",
availableThemes: () => ["Dracula", "Sepia", "Rose Pine", "Tokyo Night"],
getThemeIcon: (r, o, n) => /* @__PURE__ */ e.createElement(t, { themeName: r, darkMode: o, style: n }),
getThemeSchema: d
};
function d(r) {
switch (r) {
case "Dracula":
return b;
case "Rose Pine":
return g;
case "Tokyo Night":
return f;
case "Sepia":
return l;
}
return {};
}
return a;
};
export {
i as default
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve theme management robustness and maintainability.

The theme management implementation could be enhanced:

  1. Version should be sourced from package.json
  2. Add theme existence validation
  3. Replace switch with object lookup
  4. Add TypeScript types

Consider applying these changes:

+const THEMES = {
+  "Dracula": b,
+  "Rose Pine": g,
+  "Tokyo Night": f,
+  "Sepia": l,
+};

 const i = (e) => {
   const t = u(e), a = {
     name: "Example",
-    version: "0.0.1",
+    version: process.env.PACKAGE_VERSION || "0.0.1",
     availableThemes: () => ["Dracula", "Sepia", "Rose Pine", "Tokyo Night"],
     getThemeIcon: (r, o, n) => /* @__PURE__ */ e.createElement(t, { themeName: r, darkMode: o, style: n }),
     getThemeSchema: d
   };
   function d(r) {
-    switch (r) {
-      case "Dracula":
-        return b;
-      case "Rose Pine":
-        return g;
-      case "Tokyo Night":
-        return f;
-      case "Sepia":
-        return l;
-    }
-    return {};
+    if (!r || !THEMES[r]) {
+      console.warn(`Theme not found: ${r}`);
+      return {};
+    }
+    return THEMES[r];
   }
   return a;
 };
📝 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
}, i = (e) => {
const t = u(e), a = {
name: "Example",
version: "0.0.1",
availableThemes: () => ["Dracula", "Sepia", "Rose Pine", "Tokyo Night"],
getThemeIcon: (r, o, n) => /* @__PURE__ */ e.createElement(t, { themeName: r, darkMode: o, style: n }),
getThemeSchema: d
};
function d(r) {
switch (r) {
case "Dracula":
return b;
case "Rose Pine":
return g;
case "Tokyo Night":
return f;
case "Sepia":
return l;
}
return {};
}
return a;
};
export {
i as default
};
const THEMES = {
"Dracula": b,
"Rose Pine": g,
"Tokyo Night": f,
"Sepia": l,
};
const i = (e) => {
const t = u(e), a = {
name: "Example",
version: process.env.PACKAGE_VERSION || "0.0.1",
availableThemes: () => ["Dracula", "Sepia", "Rose Pine", "Tokyo Night"],
getThemeIcon: (r, o, n) => /* @__PURE__ */ e.createElement(t, { themeName: r, darkMode: o, style: n }),
getThemeSchema: d
};
function d(r) {
if (!r || !THEMES[r]) {
console.warn(`Theme not found: ${r}`);
return {};
}
return THEMES[r];
}
return a;
};
export {
i as default
};

try {
const parsedUrl = new URL(url);

if (!parsedUrl.hostname.includes('github.com')) {

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
github.com
' can be anywhere in the URL, and arbitrary hosts may come before or after it.
@github-actions github-actions bot removed the Stale label Dec 28, 2024
Copy link
Contributor

github-actions bot commented Jan 7, 2025

👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review.

@github-actions github-actions bot added the Stale label Jan 7, 2025
Copy link
Contributor

This PR was closed because it has been stalled for 5 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant