Desktop, Mobile: Resolves #15081: Speed up app startup by skipping unnecessary plugin file processing - #15085
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a persisted per-plugin extraction-state cache and switches JPL cache validation from MD5 to file size + mtime checks. Plugin extraction is skipped when the cache is valid; otherwise the package is re-extracted, manifest reloaded and cache updated. A test verifies skip/re-extract behaviour. Changes
Sequence Diagram(s)sequenceDiagram
participant PS as PluginService
participant FS as Filesystem
participant TAR as TarExtractor
participant MAN as ManifestLoader
PS->>FS: stat(plugin.jpl) → {size, mtime}
PS->>PS: lookup extractionStates_[fname]
alt cache valid (size+mtime match && manifest(+index.js) exist)
PS-->>PS: skip extraction, use unpack dir
else cache missing/invalid
PS->>FS: remove/create unpack dir
PS->>TAR: tarExtract(plugin.jpl → unpack dir)
TAR-->>PS: extraction complete
PS->>MAN: load unpack/manifest.json
MAN-->>PS: manifest loaded
PS->>FS: write plugin-extraction-state.json (size, timestamp)
end
Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/lib/testing/plugins/createTestPlugin.ts (1)
33-41: Avoid shared temp build paths for packed plugin creation.Line 34 uses a deterministic folder (
plugin-build-${manifest.id}), which can collide in parallel test workers and make this helper flaky. Please generate a unique temp folder per call and clean it up aftertarCreate.Suggested hardening
if (format === 'jpl') { - const tempDir = join(Setting.value('tempDir'), `plugin-build-${manifest.id}`); - await mkdirp(tempDir); - await writeFile(join(tempDir, 'manifest.json'), JSON.stringify(manifest), 'utf-8'); - await writeFile(join(tempDir, 'index.js'), scriptSource, 'utf-8'); - - const jplPath = join(Setting.value('pluginDir'), `${manifest.id}.jpl`); - await shim.fsDriver().tarCreate({ cwd: tempDir, file: jplPath }, ['manifest.json', 'index.js']); + const tempDir = join( + Setting.value('tempDir'), + `plugin-build-${manifest.id}-${Date.now()}-${Math.random().toString(36).slice(2)}`, + ); + await mkdirp(tempDir); + try { + await writeFile(join(tempDir, 'manifest.json'), JSON.stringify(manifest), 'utf-8'); + await writeFile(join(tempDir, 'index.js'), scriptSource, 'utf-8'); + + const jplPath = join(Setting.value('pluginDir'), `${manifest.id}.jpl`); + await shim.fsDriver().tarCreate({ cwd: tempDir, file: jplPath }, ['manifest.json', 'index.js']); + } finally { + await shim.fsDriver().remove(tempDir); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/testing/plugins/createTestPlugin.ts` around lines 33 - 41, In createTestPlugin, avoid the shared deterministic temp folder used for packing (the tempDir built from `plugin-build-${manifest.id}`); instead create a unique per-call temporary directory (e.g., use a random/suffixed name or system mkdtemp) before calling `mkdirp`/writing `manifest.json` and `index.js`, use that path for `shim.fsDriver().tarCreate` to produce `jplPath`, and then remove/cleanup the temp directory after `tarCreate` completes (or in a finally block) to prevent parallel-test collisions and leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/services/plugins/PluginService.ts`:
- Around line 321-333: The extraction validity check in PluginService (variables
unpackDir, manifestFilePath, extractionStates, extractionState, extractionValid)
only verifies manifest.json; update extractionValid to also verify the presence
(and optionally integrity) of the plugin entry script (e.g.,
`${unpackDir}/index.js` or the actual script name expected by the plugin
manifest) by checking shim.fsDriver().exists for that script before skipping
extraction; ensure the existence check uses the same unpackDir and include the
script path variable in the condition so extraction is only skipped when both
manifest and the plugin script are present.
- Around line 222-225: The saveExtractionStates method currently lets write
errors bubble up and block plugin startup; modify saveExtractionStates (and its
use of this.extractionStates_, extractionStatePath() and
shim.fsDriver().writeFile) to catch any errors from writeFile, set
this.extractionStates_ as before, attempt the async write inside a try/catch,
and on failure log the error (do not rethrow) so the Promise resolves normally
and plugin loading can continue; ensure the function signature and behavior
remain Promise<void> but never rejects due to cache write failures.
---
Nitpick comments:
In `@packages/lib/testing/plugins/createTestPlugin.ts`:
- Around line 33-41: In createTestPlugin, avoid the shared deterministic temp
folder used for packing (the tempDir built from `plugin-build-${manifest.id}`);
instead create a unique per-call temporary directory (e.g., use a
random/suffixed name or system mkdtemp) before calling `mkdirp`/writing
`manifest.json` and `index.js`, use that path for `shim.fsDriver().tarCreate` to
produce `jplPath`, and then remove/cleanup the temp directory after `tarCreate`
completes (or in a finally block) to prevent parallel-test collisions and leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c385204-6bcb-4084-85a7-365f1e622ea6
📒 Files selected for processing (3)
packages/lib/services/plugins/PluginService.tspackages/lib/services/plugins/loadPlugins.test.tspackages/lib/testing/plugins/createTestPlugin.ts
laurent22
force-pushed
the
plugin_extraction_state
branch
from
April 14, 2026 09:59
474aca0 to
fe8b81b
Compare
laurent22
changed the title
coderabbitai
Bot
removed
enhancement
coderabbitai
Bot
added
plugins
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Don't extract plugin files if the files are already present. Keep track of what is available using an index file.
Partially addresses #12301