Desktop, Cli: Do not load plugin if it is disabled

[laurent22]

Loading a plugin on mobile is expensive in terms of resources and can make the app freeze. Currently even if a plugin is disabled, the freeze will still happen since it loads the plugin whether it is disabled or not. With this change, only the manifest is read if the plugin is disabled.

This will also makes things faster on desktop although any performance gain might not be noticeable.

Partially addresses #12301

[coderabbitai]

📝 Walkthrough

Walkthrough

Plugin loading now supports a manifest-only mode: PluginService loads cached manifests first to determine enabled state via settings, then performs full extraction and script loading only for enabled plugins; disabled plugins are left with an empty script payload.

Changes

Cohort / File(s)	Summary
Plugin Service Loading Optimisation packages/lib/services/plugins/PluginService.ts	Added manifestOnly = false parameter to loadPluginFromPackage and loadPluginFromPath. manifestOnly=true reads cached manifest.json (skips MD5/tar extraction) and omits script text for non-enabled plugins. loadAndRunPlugins now loads manifests first, checks settings to compute enabled, then performs full loads only for enabled plugins.
Plugin Loading Test Coverage packages/lib/services/plugins/loadPlugins.test.ts	Updated imports and added a test should not load the script for disabled plugins that creates two plugin dirs, marks one enabled/one disabled in settings, runs loadAndRunPlugins, and asserts the disabled plugin has empty scriptText while the enabled plugin has non-empty script.

Sequence Diagram(s)

sequenceDiagram
    participant PS as PluginService
    participant FS as FileSystem/Cache
    participant S as Settings
    participant P as Plugin (in-memory)

    PS->>FS: read cached manifest.json (manifestOnly=true)
    FS-->>PS: manifest data
    PS->>S: query plugin enabled state
    S-->>PS: enabled? / settings value
    alt enabled
        PS->>FS: ensure package extracted / compute MD5 (manifestOnly=false)
        FS-->>PS: script/index.js + manifest
        PS->>P: construct full Plugin with scriptText
    else disabled
        PS->>P: construct Plugin with manifest only (scriptText = "")
    end

Loading

Suggested labels

mobile

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Pr Description Must Follow Guidelines	❌ Error	PR description lacks explicit Test Plan or verification steps section required by GSoC guidelines, despite comprehensive tests being added to the codebase.	Add Test Plan section documenting the new test case, instructions to run tests, and any manual mobile testing performed to verify the performance fix.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: avoiding full plugin loading for disabled plugins by reading only the manifest.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The pull request description clearly explains the problem (expensive plugin loading on mobile even when disabled) and the solution (loading only manifest for disabled plugins).

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch disabled_plugin

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/lib/services/plugins/PluginService.ts (1)

297-305: Clarify fallthrough behaviour when cache is missing.

When manifestOnly is true but no cached manifest exists (first run with a new .jpl plugin), the code falls through to the full MD5/extraction path. This means disabled plugins will still incur the expensive extraction on their first load, but subsequent loads will benefit from the cache.

This is acceptable since the plugin ID is only known after extracting the manifest at least once. Consider adding a brief comment explaining this fallthrough is intentional for first-time loading scenarios.

📝 Suggested clarifying comment

 		if (manifestOnly) {
 			// When loading only the manifest (e.g. for disabled plugins), try
 			// to use an already-extracted manifest from cache to avoid the
 			// expensive MD5 hash and tar extraction.
+			// If the cache doesn't exist yet (first load), fall through to
+			// full extraction below.
 			const manifest = await this.loadManifestToObject(manifestFilePath);
 			if (manifest) {
 				return this.loadPlugin(unpackDir, JSON.stringify(manifest), '', makePluginId(fname));
 			}
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/lib/services/plugins/PluginService.ts` around lines 297 - 305, Add a
brief clarifying comment explaining that when manifestOnly is true and
loadManifestToObject(manifestFilePath) returns null, the code intentionally
falls through to the full MD5/extraction path so the manifest (and thus plugin
ID from makePluginId(fname)) can be obtained on first run; reference the
manifestOnly check, the loadManifestToObject call, and the subsequent
loadPlugin(unpackDir, JSON.stringify(manifest), '', makePluginId(fname)) path to
make clear this is expected behavior for first-time loads and disabled-plugin
cache warming.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/lib/services/plugins/PluginService.ts`:
- Around line 297-305: Add a brief clarifying comment explaining that when
manifestOnly is true and loadManifestToObject(manifestFilePath) returns null,
the code intentionally falls through to the full MD5/extraction path so the
manifest (and thus plugin ID from makePluginId(fname)) can be obtained on first
run; reference the manifestOnly check, the loadManifestToObject call, and the
subsequent loadPlugin(unpackDir, JSON.stringify(manifest), '',
makePluginId(fname)) path to make clear this is expected behavior for first-time
loads and disabled-plugin cache warming.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7797abee-66b4-4f68-b0e9-b53160c2f752

📥 Commits

Reviewing files that changed from the base of the PR and between 1bbd603 and 1cde576.

📒 Files selected for processing (2)

• packages/lib/services/plugins/PluginService.ts
• packages/lib/services/plugins/loadPlugins.test.ts

[coderabbitai]

🧹 Nitpick comments (1)

packages/lib/services/plugins/PluginService.ts (1)

351-354: Note: .js plugins do not support manifest-only loading.

For .js files (line 351-352), the manifestOnly parameter is ignored and the plugin is always fully loaded. This is inconsistent with .jpl handling. Given the PR focuses on mobile where .jpl is the standard format, this is likely acceptable, but worth noting for future reference.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/lib/services/plugins/PluginService.ts` around lines 351 - 354, The
.js branch ignores the manifestOnly flag and always fully loads the plugin;
update the call to loadPluginFromJsBundle to accept and honor manifestOnly (or
add a new helper like loadManifestFromJsBundle) so manifest-only requests are
handled consistently with .jpl. Change the invocation at the .js branch to pass
manifestOnly (e.g., loadPluginFromJsBundle(dirname(path), await
fsDriver.readFile(path), filename(path), manifestOnly)), and update the
loadPluginFromJsBundle function signature/implementation to return only the
manifest when manifestOnly is true (or implement a dedicated manifest-extraction
function and call it from the .js branch); ensure all callers of
loadPluginFromJsBundle are updated to the new signature if needed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/lib/services/plugins/PluginService.ts`:
- Around line 351-354: The .js branch ignores the manifestOnly flag and always
fully loads the plugin; update the call to loadPluginFromJsBundle to accept and
honor manifestOnly (or add a new helper like loadManifestFromJsBundle) so
manifest-only requests are handled consistently with .jpl. Change the invocation
at the .js branch to pass manifestOnly (e.g.,
loadPluginFromJsBundle(dirname(path), await fsDriver.readFile(path),
filename(path), manifestOnly)), and update the loadPluginFromJsBundle function
signature/implementation to return only the manifest when manifestOnly is true
(or implement a dedicated manifest-extraction function and call it from the .js
branch); ensure all callers of loadPluginFromJsBundle are updated to the new
signature if needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59f73979-0b4e-4594-b43a-45547fbbb5cf

📥 Commits

Reviewing files that changed from the base of the PR and between 1cde576 and 33e8f0e.

📒 Files selected for processing (1)

• packages/lib/services/plugins/PluginService.ts