UN-4187 [MISC] Standardise optional plugin loading with a single loadPlugin helper - #2307
Open
jaseemjaskp wants to merge 1 commit into
Open
jaseemjaskp wants to merge 1 commit into
jaseemjaskp wants to merge 1 commit into
Conversation
…helper
Every non-route plugin import used a module-level
`try { await import("../plugins/...") } catch {}`, most with a bare catch,
so a real chunk-load or runtime error was indistinguishable from "plugin
not installed". Fallbacks differed per site, and several sites shared one
try block so a single missing plugin disabled the others.
Add loadPlugin(importer, fallback) to helpers/pluginLoader.js: it returns
the fallback silently when the plugin is absent and logs every other
failure once. Absence is now isPluginAbsent, which is narrower than the
old isModuleMissing: a chunk that failed to fetch belongs to a plugin
that IS shipped, so it is logged instead of mistaken for absence.
pluginRegistry shares the same classifier.
Migrate every plugin import site to one loadPlugin call per plugin
module. Two call sites get a guard where code relied on the old
all-or-nothing loading: the Summarize tab now requires its view, and
PersistentLogin calls setSelectedProduct optionally.
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // plugins lets one missing plugin disable the others. | ||
| export async function loadPlugin(importer, fallback = null) { | ||
| try { | ||
| return (await importer()) ?? fallback; |
Contributor
There was a problem hiding this comment.
undefined and loadPlugin silently uses the fallback. The component or hook disappears without an error explaining why, making a broken plugin harder to diagnose. lazyPlugin already reports missing exports.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/helpers/pluginLoader.js
Line: 49
Comment:
**Missing exports go unnoticed** If a shipped plugin loads but its requested export was renamed or removed, the importer returns `undefined` and `loadPlugin` silently uses the fallback. The component or hook disappears without an error explaining why, making a broken plugin harder to diagnose. `lazyPlugin` already reports missing exports.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function isPluginAbsent(err) { | ||
| return (err?.message || "").includes("Optional plugin not available"); | ||
| } | ||
| import { isPluginAbsent } from "./pluginLoader.js"; |
Contributor
There was a problem hiding this comment.
MODULE_NOT_FOUND while loading, the shared classifier now treats it as absent. lazyPlugin then renders NotFound instead of surfacing the plugin failure, making the broken route look like a missing route. The previous route-specific check matched only the optional-plugin stub error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/helpers/pluginRegistry.js
Line: 4
Comment:
**Route errors become NotFound** If a shipped route plugin throws an error containing “Cannot find module” or carrying `MODULE_NOT_FOUND` while loading, the shared classifier now treats it as absent. `lazyPlugin` then renders NotFound instead of surfacing the plugin failure, making the broken route look like a missing route. The previous route-specific check matched only the optional-plugin stub error.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
Unstract test resultsPer-group results
Critical paths
|
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.
What
loadPlugin(importer, fallback = null)tofrontend/src/helpers/pluginLoader.js, with unit tests.try { await import("../plugins/…") } catch {}toloadPlugin, one plugin module per call.isModuleMissingwithisPluginAbsent, shared byloadPluginandlazyPlugin.Why
catch. A real chunk-load or runtime error in a shipped plugin looked exactly like "plugin not installed" and vanished silently.null,undefined, no-ops, stores leftundefined).ToolIde,CombinedOutput,SettingsModal).How
loadPluginreturns the fallback when the plugin is absent (silent), when the import fails for any other reason (logged once as[plugin] failed to load), or when the picked export is missing.isPluginAbsentmatches only the build stub'sOptional plugin not availableplus the Node equivalents. It deliberately does not matchFailed to fetch dynamically imported module: that is a shipped plugin whose chunk failed, so it is logged rather than treated as absent. The fallback is returned either way; only the logging differs.PersistentLogincallssetSelectedProduct?.().Router.jsx,useMainAppRoutes.jsxandPageLayout.jsxdrop their hand-rolledisModuleMissing+console.errorblocks;lazyPluginroute wrappers are unchanged.Can this PR break any existing features? If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
useSessionValid.jshas called theusePlatformAdminhook at module level since Feat/friction less onboarding and usage reporting #256, which always throwsInvalid hook call, soisPlatformAdminhas never been set. That pre-existing bug is now visible on every page load; fixing it changes access toRequirePlatformAdmin, so it is left for a separate ticket.nullrather thanundefined; no call site compares strictly againstundefined.Relevant Docs
prompting/FRONTEND_DEV_GUIDE.mdin the companion cloud PR.Related Issues or PRs
Dependencies Versions / Env Variables
Notes on Testing
npx vitest run: 759/761 pass. The 2 failures are incascade-and-affordances.test.jsxand flag CSS/defaultPropsin cloud plugin files copied into the checkout; none are touched here.vite buildsucceeds with the cloud plugins and as an OSS-only build withsrc/pluginsremoved.catchremains around a plugin import.throwinTrialDaysInfo.jsx): console shows[plugin] failed to load … deliberate broken pluginpointing atTopNavBar.jsx; the page still renders without the badge.Screenshots
...
Checklist
I have read and understood the Contribution Guidelines.