Mobile: Load plugin scripts directly from filesystem instead of passing through bridge#15095
Open
Mobile: Load plugin scripts directly from filesystem instead of passing through bridge#15095
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlugin script loading on mobile was changed to prefer passing a script file path and optionally the original script text. The background runner now asynchronously loads script text from Changes
Sequence Diagram(s)sequenceDiagram
participant PluginService
participant PluginRunner
participant WebView
participant startStopPlugin as startStopPlugin.runPlugin()
participant FileSystem as File System
PluginService->>PluginRunner: loadPluginFromPath() / run(plugin)
PluginRunner->>WebView: runPlugin(scriptFilePath, scriptText)
WebView->>startStopPlugin: runPlugin(scriptFilePath, scriptText)
alt scriptText provided
startStopPlugin->>startStopPlugin: use scriptText directly
else scriptText empty
startStopPlugin->>FileSystem: GET file://{scriptFilePath} (XMLHttpRequest)
FileSystem-->>startStopPlugin: respond with script content
startStopPlugin->>startStopPlugin: use loaded script
end
startStopPlugin->>startStopPlugin: create iframe and inject script
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. 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: 3
🤖 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/app-mobile/components/plugins/backgroundPage/startStopPlugin.ts`:
- Around line 29-50: Reserve a pending entry in loadedPlugins keyed by pluginId
before performing the async file load in runPlugin so concurrent stopPlugin or
duplicate runPlugin calls can detect and cancel a startup-in-progress;
specifically, when entering runPlugin (before awaiting the
XMLHttpRequest/promise that sets pluginScript) set loadedPlugins[pluginId] = {
status: 'starting', cancelled: false } (or another small sentinel object), then
after the XHR resolves check that loadedPlugins[pluginId]?.cancelled !== true
before continuing to fully initialize (and if cancelled, abort startup and
cleanup the entry); ensure stopPlugin checks for this sentinel and sets
cancelled = true (and removes the entry if necessary), and on any XHR error
remove the pending loadedPlugins entry to avoid leaking the slot.
In `@packages/app-mobile/components/plugins/PluginRunner.ts`:
- Around line 49-60: The injected call to pluginBackgroundPage.runPlugin is
fire-and-forget, so if loading the plugin script fails the WebView rejection is
unhandled and the host (PluginRunner.run) already marks plugin.running = true;
fix by adding a .catch in the injected JS around
pluginBackgroundPage.runPlugin(...) that posts an error back to the host
(include messageChannelId and plugin.id) via the WebView bridge (e.g.,
window.ReactNativeWebView.postMessage) and ensure PluginRunner.run listens for
that error message on the messageChannelId and clears plugin.running (or only
sets plugin.running = true after receiving a success/ready message). Update the
injected payload in PluginRunner (webviewRef.current.injectJS(...)) to include
this catch-and-report logic and update PluginRunner.run's message handling to
unmark running on error or mark running only on explicit success.
In `@packages/lib/services/plugins/PluginService.ts`:
- Around line 354-356: The upfront existence/readability check for the plugin
bundle was removed—restore a lightweight validation so mobile still verifies
`${distPath}/index.js` before proceeding: in the loadPluginFromPath() flow
(where shim.mobilePlatform() is used and fsDriver.readFile was called), add a
cheap fsDriver.exists or fsDriver.stat check for `${distPath}/index.js` when
shim.mobilePlatform() returns true and throw the same validation error used on
desktop if the file is missing/unreadable so installPlugin() and registration
keep the original failure semantics without reading the full file.
🪄 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: 9955bd50-81c1-40cc-bec1-9e422ff8df9f
📒 Files selected for processing (4)
packages/app-mobile/components/plugins/PluginRunner.tspackages/app-mobile/components/plugins/PluginRunnerWebView.tsxpackages/app-mobile/components/plugins/backgroundPage/startStopPlugin.tspackages/lib/services/plugins/PluginService.ts
coderabbitai
Bot
removed
enhancement
Comment on lines
+51
to
+52
| // script text across the React Native bridge). On web, file:// URLs | ||
| // are blocked by CSP so we pass the script text directly. |
Collaborator
There was a problem hiding this comment.
are blocked by CSP
Note: Rather than due to the CSP, this doesn't work on web because it uses a virtual file system to store plugins (because of browser sandboxing). Most files need to be read through fs-driver on web, rather than through native APIs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially addresses #12301
Tested on desktop, Android and iOS
Summary
PluginService.loadPluginFromPathskips readingindex.jsentirely since it's no longer needed on the React Native sideMotivation
Plugin script bundles can be hundreds of KB. Previously, each enabled plugin's full JS bundle crossed the React Native bridge twice on every startup: once when read from disk (
readFile), and again when injected into the WebView (injectJS). This contributed to a 2-3 second freeze on app startup.Now the script content never enters the React Native JS thread at all — the WebView fetches it directly from disk via
file://URL.