perf: use filters for plugin hooks #3820
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors several Rollup-style build plugins to use explicit filter+handler hook objects and standardizes internal virtual-module prefixes to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/build/plugins/route-meta.ts (2)
13-28: ResolveId regex and PREFIX wiring look correct; adjust lint suppression for Biome if needed.The
resolveIdhandler correctly targets non-virtual?metaimports and rewrites them toPREFIX + resolved.id. If Biome is part of your toolchain, the current// eslint-disable-next-line no-control-regexwon’t silencelint/suspicious/noControlCharactersInRegex; consider adding a Biome-specific ignore comment above the regex while keeping the pattern as‑is.- resolveId: { - // eslint-disable-next-line no-control-regex - filter: { id: /^(?!\u0000)(.+)\?meta$/ }, + resolveId: { + // biome-ignore lint/suspicious/noControlCharactersInRegex: allow matching non-virtual ?meta imports + // eslint-disable-next-line no-control-regex + filter: { id: /^(?!\u0000)(.+)\?meta$/ },
31-72: Load/transform filters are safe; consider fixing the log message typo.The
load/transformhooks correctly gate onPREFIXusingescapeRegExp, so the “regex from variable” warnings are benign here given the constant, internal PREFIX. One small polish item is the warning message text.- } catch (error) { - nitro.logger.warn( - `[handlers-meta] Cannot extra route meta for: ${id}: ${error}` - ); - } + } catch (error) { + nitro.logger.warn( + `[handlers-meta] Cannot extract route meta for: ${id}: ${error}` + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/build/plugins/oxc.ts(1 hunks)src/build/plugins/raw.ts(2 hunks)src/build/plugins/route-meta.ts(1 hunks)src/build/plugins/virtual.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/build/plugins/oxc.ts (1)
src/types/rollup.ts (1)
OXCOptions(20-23)
src/build/plugins/virtual.ts (1)
src/utils/regex.ts (1)
escapeRegExp(1-3)
src/build/plugins/route-meta.ts (1)
src/utils/regex.ts (1)
escapeRegExp(1-3)
🪛 ast-grep (0.40.0)
src/build/plugins/raw.ts
[warning] 14-14: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${HELPER_ID}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 14-14: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${PREFIX})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 34-34: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${HELPER_ID}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 34-34: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${RESOLVED_PREFIX})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 52-52: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${RESOLVED_PREFIX})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
src/build/plugins/route-meta.ts
[warning] 31-31: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(PREFIX)})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 42-42: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(PREFIX)})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
src/build/plugins/route-meta.ts
[error] 15-15: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-rolldown (windows-latest)
- GitHub Check: tests-rollup (ubuntu-latest)
- GitHub Check: tests-rollup (windows-latest)
🔇 Additional comments (2)
src/build/plugins/virtual.ts (1)
4-4: PREFIX and resolveId filter are consistent and efficient.Using a single
PREFIXand precomputedidregexp over themoduleskeys cleanly limitsresolveIdto known virtual IDs, andescapeRegExpprevents any regex metacharacter issues from custom IDs. The behavior lines up with theloadhook’s PREFIX usage, so no changes are needed here.Also applies to: 31-33
src/build/plugins/raw.ts (1)
5-8: Raw plugin prefixes and filters look coherent; regex-from-variable warnings are safe here.The new
HELPER_ID/RESOLVED_PREFIX/PREFIXflow inresolveId,load, andtransformcleanly isolates raw assets and the helper module, so only the relevant IDs hit these hooks. Thenew RegExp(\^${...}`)` usages are all built from internal constants (not user input), so the ReDoS-style “regexp-from-variable” warnings are effectively false positives in this context; no changes are strictly necessary.Also applies to: 14-16, 34-36, 52-57, 85-96
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/build/plugins/oxc.ts (1)
8-10: Past review comment is outdated.The previous review flagged that
oxc()returnsPromise<Plugin>and requires async handling ingetRollupConfig. However, the current implementation showsoxc()is synchronous and returnsPlugindirectly (not a Promise). The concerns raised in the past review are not applicable to this version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/build/plugins/oxc.ts(1 hunks)src/build/plugins/resolve.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/build/plugins/oxc.ts (1)
src/types/rollup.ts (1)
OXCOptions(20-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests-rolldown (windows-latest)
🔇 Additional comments (2)
src/build/plugins/resolve.ts (1)
19-19: LGTM! Prefix update aligns with new virtual module convention.The prefix change correctly reflects the new
\0nitro:virtual:convention introduced in the virtual.ts plugin refactoring.src/build/plugins/oxc.ts (1)
1-6: LGTM! Clean import reorganization.The type-only import for
MinifyOptionsand the separated imports fortransformandminifyfollow best practices.
Using regex filters for plugin hooks can help to avoid unnecessary calls.