Closed
Conversation
Contributor
📝 WalkthroughWalkthroughPlugin loading infrastructure transitions from synchronous to asynchronous approaches. Synchronous fallback methods are removed, and webpack configuration adds test file exclusion rules to prevent test files from being bundled. Dynamic imports replace require calls with Promise-based loading patterns. Changes
Sequence DiagramsequenceDiagram
participant App as App Route
participant OldFlow as Old (Sync)
participant NewFlow as New (Async)
rect rgb(240, 248, 255)
Note over OldFlow: Synchronous Flow
App->>OldFlow: Load plugin routes
OldFlow->>OldFlow: processRoutes() (sync require)
alt Success
OldFlow-->>App: Return routes
else Error
OldFlow->>OldFlow: Fallback to processRoutesSync()
OldFlow-->>App: Return routes
end
end
rect rgb(240, 255, 240)
Note over NewFlow: Asynchronous Flow
App->>NewFlow: Load plugin routes
NewFlow->>NewFlow: processRoutes() (async)
NewFlow->>NewFlow: for each plugin: dynamic import
activate NewFlow
NewFlow-->>NewFlow: Promise.allSettled()
deactivate NewFlow
alt Success
NewFlow-->>App: Return routes
else Error
NewFlow->>NewFlow: Log error
NewFlow-->>App: Return empty array
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
mo-auto
added
comp-admin-ui
|
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/app/routes/index.tsx(2 hunks)admin-ui/config/webpack.config.client.dev.ts(2 hunks)admin-ui/plugins/PluginMenuResolver.js(2 hunks)admin-ui/plugins/PluginReducersResolver.js(1 hunks)admin-ui/plugins/PluginSagasResolver.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/PluginSagasResolver.js (2)
admin-ui/plugins/PluginMenuResolver.js (4)
pluginPromises(7-26)pluginPromises(42-60)results(28-28)results(62-62)admin-ui/plugins/PluginReducersResolver.js (2)
pluginName(9-9)metadata(11-16)
🔇 Additional comments (1)
admin-ui/config/webpack.config.client.dev.ts (1)
193-196: Ensure the new loader is available.Please double‑check that
ignore-loaderis listed indevDependencies; otherwise the build will fail with “Cannot resolve module 'ignore-loader'” when this rule runs.
Comment on lines
+4
to
+24
| async function process() { | ||
| const metadataFilePath = plugins.map((item) => item.metadataFile) | ||
| let pluginReducers = [] | ||
| metadataFilePath.forEach(async (path) => { | ||
| pluginReducers = await [...pluginReducers, ...require(`${path}`).default.reducers] | ||
|
|
||
| pluginReducers.forEach((element) => { | ||
| reducerRegistry.register(element.name, element.reducer) | ||
| }) | ||
| }) | ||
| for (const path of metadataFilePath) { | ||
| const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1] | ||
| if (pluginName) { | ||
| const metadata = await import( | ||
| /* webpackChunkName: "plugin-[request]" */ | ||
| /* webpackMode: "lazy" */ | ||
| /* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */ | ||
| `./${pluginName}/plugin-metadata` | ||
| ) | ||
| const reducers = metadata.default.reducers || [] | ||
| pluginReducers = [...pluginReducers, ...reducers] | ||
|
|
||
| reducers.forEach((element) => { | ||
| reducerRegistry.register(element.name, element.reducer) | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Return the reducer list.
process() now resolves to undefined, but the rest of the codebase still expects an array of plugin reducers. Even in this file we keep building pluginReducers, so this looks accidental. Add return pluginReducers at the end (after the loop) so callers aren’t broken.
async function process() {
const metadataFilePath = plugins.map((item) => item.metadataFile)
let pluginReducers = []
for (const path of metadataFilePath) {
const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1]
if (pluginName) {
const metadata = await import(
/* webpackChunkName: "plugin-[request]" */
/* webpackMode: "lazy" */
/* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */
`./${pluginName}/plugin-metadata`
)
const reducers = metadata.default.reducers || []
pluginReducers = [...pluginReducers, ...reducers]
reducers.forEach((element) => {
reducerRegistry.register(element.name, element.reducer)
})
}
}
+ return pluginReducers
}📝 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
| async function process() { | |
| const metadataFilePath = plugins.map((item) => item.metadataFile) | |
| let pluginReducers = [] | |
| metadataFilePath.forEach(async (path) => { | |
| pluginReducers = await [...pluginReducers, ...require(`${path}`).default.reducers] | |
| pluginReducers.forEach((element) => { | |
| reducerRegistry.register(element.name, element.reducer) | |
| }) | |
| }) | |
| for (const path of metadataFilePath) { | |
| const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1] | |
| if (pluginName) { | |
| const metadata = await import( | |
| /* webpackChunkName: "plugin-[request]" */ | |
| /* webpackMode: "lazy" */ | |
| /* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */ | |
| `./${pluginName}/plugin-metadata` | |
| ) | |
| const reducers = metadata.default.reducers || [] | |
| pluginReducers = [...pluginReducers, ...reducers] | |
| reducers.forEach((element) => { | |
| reducerRegistry.register(element.name, element.reducer) | |
| }) | |
| } | |
| } | |
| async function process() { | |
| const metadataFilePath = plugins.map((item) => item.metadataFile) | |
| let pluginReducers = [] | |
| for (const path of metadataFilePath) { | |
| const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1] | |
| if (pluginName) { | |
| const metadata = await import( | |
| /* webpackChunkName: "plugin-[request]" */ | |
| /* webpackMode: "lazy" */ | |
| /* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */ | |
| `./${pluginName}/plugin-metadata` | |
| ) | |
| const reducers = metadata.default.reducers || [] | |
| pluginReducers = [...pluginReducers, ...reducers] | |
| reducers.forEach((element) => { | |
| reducerRegistry.register(element.name, element.reducer) | |
| }) | |
| } | |
| } | |
| return pluginReducers | |
| } |
🤖 Prompt for AI Agents
In admin-ui/plugins/PluginReducersResolver.js around lines 4 to 24, the async
function process builds pluginReducers but never returns it, causing callers to
receive undefined; add a final statement returning pluginReducers after the
for-loop so the function resolves to the array of reducers that the rest of the
code expects.
Comment on lines
+8
to
+24
| for (const path of metadataFilePath) { | ||
| const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1] | ||
| if (pluginName) { | ||
| const metadata = await import( | ||
| /* webpackChunkName: "plugin-[request]" */ | ||
| /* webpackMode: "lazy" */ | ||
| /* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */ | ||
| `./${pluginName}/plugin-metadata` | ||
| ) | ||
| const reducers = metadata.default.reducers || [] | ||
| pluginReducers = [...pluginReducers, ...reducers] | ||
|
|
||
| reducers.forEach((element) => { | ||
| reducerRegistry.register(element.name, element.reducer) | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Bring back the fallback import.
Same concern as in the sagas resolver: without importing item.metadataFile when the regex misses, reducers shipped in plugins with non-standard metadata paths stop registering. Please add the fallback import branch so behaviour stays consistent with the menu resolver.
🤖 Prompt for AI Agents
In admin-ui/plugins/PluginReducersResolver.js around lines 8 to 24, the code
only imports plugin metadata when the path matches the regex and skips importing
item.metadataFile for non-standard paths, causing reducers from plugins with
custom metadata locations to not register; add a fallback branch that imports
the original path (e.g., await import(item.metadataFile)) when pluginName is not
found by the regex, then extract metadata.default.reducers from that fallback
import, append them to pluginReducers and register each reducer with
reducerRegistry.register exactly as done for the regex-success path so behavior
matches the menu resolver.
Comment on lines
+8
to
+21
| const pluginPromises = plugins.map(async (item) => { | ||
| const path = item.metadataFile | ||
| const pluginName = path?.match(/\.\/([^/]+)\/plugin-metadata/)?.[1] | ||
| if (pluginName) { | ||
| const metadata = await import( | ||
| /* webpackChunkName: "plugin-[request]" */ | ||
| /* webpackMode: "lazy" */ | ||
| /* webpackExclude: /\.test\.(js|jsx|ts|tsx)$/ */ | ||
| `./${pluginName}/plugin-metadata` | ||
| ) | ||
| return metadata.default.sagas || [] | ||
| } | ||
| return [] | ||
| }) |
Contributor
There was a problem hiding this comment.
Restore the metadata fallback.
Previously we supported plugin entries whose metadataFile didn’t follow ./<plugin>/plugin-metadata by importing item.metadataFile directly (see the menu resolver’s fallback). Dropping that path here means those plugins now return [] and their sagas never load. Please reintroduce the fallback import (ideally inside a try/catch) so non-standard metadataFile paths keep working.
🤖 Prompt for AI Agents
In admin-ui/plugins/PluginSagasResolver.js around lines 8 to 21, the code only
imports metadata when metadataFile matches "./<plugin>/plugin-metadata" and
returns [] for all other paths; reintroduce the previous fallback by wrapping
the import in a try/catch: first attempt the existing dynamic import using the
extracted pluginName, and if that path isn't present or pluginName is falsy, try
await import(item.metadataFile) (or if the first import throws, fall back to
importing item.metadataFile inside the catch), then return
metadata.default.sagas || []; on any import error return [] (and optionally log
or ignore the error) so non-standard metadataFile paths still load their sagas.
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
3 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.
error log https://jenkins.gluu.org/jenkins/job/gluu-admin-ui/2664/console
Summary by CodeRabbit
Bug Fixes
Chores