A complete refactor of the Vencord Webpack API. This rewrites how Webpack finds and module patches are done.
Changes to Webpack finding:
Completely deprecates the lazy way that we currently do Webpack finds. Instead, now the default will be a unified way which is a combination of the plain cacheFind for already loaded modules, and usage of waitFor for when modules were not yet loaded.
The way this works is:
- If you use the find methods in the top level, you will leverage the performance gain of using the
waitFor alternative. By using waitFor, everytime a new module is loaded, Vencord will check if that module matches any of the finds requested, and if it does, it sets the result of that find to the module. This avoids having to iterate through the module cache for every find.
By using the waitFor alternative, the result of calling the find methods is a Proxy, which has a value inside which points to the module. When the module is found that value is set, and now the Proxy acts as the module found.
Trying to access a Proxy value before the module value is set will cause an error, which shows the filter that failed. This allows users to post better logs for us.
- However, if you don't use in the top level, you will mostly likely just find the module using cacheFind, which does need to iterate through the module cache. But of course, if the module is not in the cache it will still register a waitFor, to find the module in case it is loaded later.
By having the unified way, we no longer need a lazy alternative of the methods, so the following changes to the API occurred:
- Old
find -> cacheFind
- New
find (wrapper around waitFor and cacheFind)
findLazy -> find (wrapper)findByProps, findByPropsLazy -> findByPropsfindByCode, findByCodeLazy -> findByCodefindStore, findStoreLazy -> findStorefindComponent, findComponentLazy -> findComponentfindExportedComponent, findExportedComponentLazy -> findExportedComponentfindComponentByCode, findComponentByCodeLazy -> findComponentByCodemapMangledModule, mapMangledModuleLazy -> mapMangledModule
The following were added for additional filters and refactors purposes:
findProp -> Useful as an alternative to a top level destructure. See it's JSDoc documentation for more information.findComponentByFields -> Used for finding a class component using its prototype fieldsfindByFactoryCode -> Use for finding all the exports of a module using its factory code
The following were changed for more appropriate names:
findAll -> cacheFindAllfindBulk -> cacheFindBulkfindModuleId -> cacheFindModuleId
The following were changed for less weird names:
proxyLazyWebpack -> webpackDependantLazyLazyComponentWebpack -> webpackDependantLazyComponent
findModuleFactory now uses the same tactic as the normal finds, and its old version using cache is now cacheFindModuleFactory
[!WARNING]
Destructuring Webpack finds at top level is now deprecated. It used a very hacky tricky which turns out to not always have the expected result (pshhhh Firefox)
mapMangledModule is the only exception for this
Consider switching to mapMangledModule or findProp
Important additions:
- mapMangledModule can now properly find and wrap components, simply use a component filter in one of its mappings
Additional:
- Lots of cleanups related to the main Webpack finding API and lazy stuff, and some plugins practices regarding modules
- Webpack commons and a lot of plugins finds were changed for more stability and clean code
- Mildly refactor to reporter Webpack find testing, taking advantage of the way the new finds work, so it no longer has to re-search everything
- As stated before, better error for modules not found
- Migrated all plugins to use the new settings API
[!IMPORTANT]
This change is fully backwards compatible for now. Old methods were kept and just point to the new APIs and top level destructuring is kept with a warning.
The plan is to remove the deprecated methods/features in the future.
Changes to Webpack module patching:
Centralizes Webpack patching around a single thing: Patching the module factories object of the WebpackRequire (wreq.m). This wraps the modules object with a proxy to intercept the addition of new modules, and allow us to decide what to do with them.
A new internal setting was added, eagerPatches, and it decides what to do with the newly added modules. With it enabled, we patch the factories as soon as they are set, and with it disabled we only patch them once they are accessed for the first time (default).
For correctly handling patching, because of multiple WebpackInstances, and to allow for patching factories only when they are accessed, our proxy defines getters and setters for each module factory added, instead of doing a simple Reflect.set on the module factories object.
Additionally, these were done:
- Patch timings testing! Reporter now says if a patch took a long time to complete (we don't want slow patches)
- A lot of clean up
- Harder, yet stable conditions for choosing what to do with the WebpackInstances found, and whether it is the main instance, used for Webpack finding
I analyzed years old instances of webpack to see what was less likely to change and rely on.
- Complete typings and documentation of the WebpackRequire. All of the code now properly uses the correct typings.
- A more future proof and memory efficient patchedFactory wrapper. It includes a fallback to set the main WebpackInstance in case our other approach somehow fails, and also keeps less things into its closure, allowing the garbage collector to act.
- Small reporter refactor to fit the removal of the beforeInitListeners API
PR Code Suggestions ✨
Latest suggestions up to 7e30481
| Category | Suggestion | Impact |
| General |
Ensure listener cleanup on error
If chunksSearchingDone rejects or throws early, the listener remains registered,
causing memory leaks and duplicate processing on subsequent calls. Ensure the
listener is removed in a finally block to guarantee cleanup.
src/debug/loadLazyChunks.ts [113-120]
Webpack.factoryListeners.add(factoryListener);
-for (const factoryId in wreq.m) {
- factoryListener(wreq.m[factoryId]);
+try {
+ for (const factoryId in wreq.m) {
+ factoryListener(wreq.m[factoryId]);
+ }
+ await chunksSearchingDone;
+} finally {
+ Webpack.factoryListeners.delete(factoryListener);
}
-await chunksSearchingDone;
-Webpack.factoryListeners.delete(factoryListener);
-
Suggestion importance[1-10]: 8
__
Why: Using a finally block to always remove the factory listener prevents leaks and duplicate processing; this is a robust fix with clear impact and aligns with the surrounding code’s intent.
| Medium
|
Guard missing trace entries
Accessing traces[name] without a guard can throw if beginTrace wasn't called or was
already finished. Add a null-safe check to avoid runtime errors and return 0 when no
trace exists.
src/debug/Tracer.ts [36-46]
export const finishTrace = !(IS_DEV || IS_REPORTER) ? () => 0 :
function finishTrace(name: string) {
const end = performance.now();
- const [start, args] = traces[name];
+ const entry = traces[name];
+ if (!entry) return 0;
+
+ const [start, args] = entry;
delete traces[name];
const totalTime = end - start;
logger.debug(`${name} took ${totalTime}ms`, args);
return totalTime;
};
Suggestion importance[1-10]: 7
__
Why: Adding a null check before accessing traces[name] avoids potential runtime errors when finishTrace is called without a matching beginTrace; it's a solid safety improvement with moderate impact.
| Medium
|
Respect descriptor intent when defining
Avoid unconditionally setting writable: true for all defined properties. This can
inadvertently make getters with value writable or mutate intended read-only
properties, potentially breaking invariants of proxied objects.
src/plugins/consoleShortcuts/index.ts [38-48]
const define: Define = (target, p, attributes) => {
- if (Object.hasOwn(attributes, "value")) {
- attributes.writable = true;
- }
+ const hasValue = Object.hasOwn(attributes, "value");
+ const hasAccessor = Object.hasOwn(attributes, "get") || Object.hasOwn(attributes, "set");
return Object.defineProperty(target, p, {
configurable: true,
enumerable: true,
- ...attributes
+ ...(hasAccessor ? attributes : { ...attributes, writable: hasValue ? true : attributes["writable"] })
});
};
Suggestion importance[1-10]: 5
__
Why: The concern about forcing writable: true on value descriptors is reasonable, but the current helper mirrors similar patterns elsewhere and no clear bug is demonstrated; still a safe refinement.
| Low
|
Make lazy set trap fault-tolerant
Ensure set trap does not throw when the lazy factory has not resolved and is allowed
to fail; currently it always forces evaluation and throws with the generic error
path. Add a safe early return when the factory isn't ready to avoid breaking
property writes on immediate destructuring.
src/utils/lazy.ts [41-45]
set: (target, p, newValue) => {
- const lazyTarget = target[SYM_LAZY_GET]();
- return Reflect.set(lazyTarget, p, newValue, lazyTarget);
+ try {
+ const lazyTarget = target[SYM_LAZY_GET]();
+ return Reflect.set(lazyTarget, p, newValue, lazyTarget);
+ } catch {
+ // If the target isn't ready yet, signal failure to set without crashing
+ return false;
+ }
},
Suggestion importance[1-10]: 4
__
Why: Returning false on set when the lazy target isn't ready may prevent crashes, but it also silently masks errors and could hide initialization issues; impact is modest and context-dependent.
| Low
|
| Possible issue |
Fix default export key name
Remove the trailing space from the exported key label for default exports. The
current value "default " may break consumers that rely on the exact export key
string. This also makes returned metadata consistent with non-default export keys.
src/webpack/api.tsx [714-716]
if (mod.exports.default != null && filter(mod.exports.default)) {
- return { result: mod.exports.default, id: key, exportKey: "default ", factory };
+ return { result: mod.exports.default, id: key, exportKey: "default", factory };
}
Suggestion importance[1-10]: 7
__
Why: Correctly removing the stray space in exportKey: "default " to "default" fixes inconsistent metadata and potential consumer breakage; small but accurate change.
| Medium
|
Prevent double initialization
Guard the initialization to prevent multiple invocations when multiple Webpack
instances or reassignments of e occur. Without a guard, _initWebpack can be called
more than once, leading to duplicate listeners and inconsistent state.
src/webpack/patchWebpack.ts [66-72]
set(this: WebpackRequire, ensureChunk: WebpackRequire["e"]) {
define(this, "e", { value: ensureChunk });
clearTimeout(setterTimeout);
+
+ if ((this as any).$$vencordInitDone) return;
+ (this as any).$$vencordInitDone = true;
logger.info("Main WebpackInstance found" + interpolateIfDefined` in ${fileName}` + ", initializing internal references to WebpackRequire");
_initWebpack(this);
}
Suggestion importance[1-10]: 7
__
Why: Adding a guard avoids multiple _initWebpack invocations if e is reassigned, reducing risk of duplicate listeners; the change fits the surrounding code and is low-risk, though not strictly critical.
| Medium
|
Scan all export keys
Remove the arbitrary exportKey.length <= 3 restriction when scanning exports. This
can cause missed matches for valid exports with longer names, leading to
hard-to-debug failures. Iterate over all export keys instead.
src/webpack/api.tsx [718-724]
-for (const exportKey in mod.exports) if (exportKey.length <= 3) {
+for (const exportKey in mod.exports) {
const exportValue = mod.exports[exportKey];
-
if (exportValue != null && filter(exportValue)) {
return { result: exportValue, id: key, exportKey, factory };
}
}
Suggestion importance[1-10]: 6
__
Why: Removing the exportKey.length <= 3 limit avoids missing valid exports and improves correctness; however, it may have perf implications if intentional, so moderate score.
| Low
|
Remove key-length cap in bulk
Align the multi-match path with the single-match fix by removing exportKey.length <=
3. The cap causes legitimate matches to be skipped in bulk searches, returning
incomplete results.
src/webpack/api.tsx [774-781]
-for (const exportKey in mod.exports) if (exportKey.length <= 3) {
+for (const exportKey in mod.exports) {
const exportValue = mod.exports[exportKey];
-
if (exportValue != null && filter(exportValue)) {
results.push({ result: exportValue, id: key, exportKey, factory });
break;
}
}
Suggestion importance[1-10]: 6
__
Why: Matching the bulk path to scan all keys improves result completeness and consistency with the single-result path; potentially intentional optimization, so moderate impact.
| Low
|
Stabilize modal component references
This reassigns ModalRoot, ModalHeader, etc. but leaves Modals itself typed as a
found object while you also export the individual components defaulting to
NoopComponent. If findByProps never resolves, downstream code might render
NoopComponent silently. Expose a stable object shape and ensure components are
always referentially updated to avoid stale captures.
src/utils/modal.tsx [109-114]
-export const Modals = findByProps<Modals>("ModalRoot", "ModalCloseButton", (m: Modals) => {
- ({ ModalRoot, ModalHeader, ModalContent, ModalFooter, ModalCloseButton } = m);
+// keep stable references to the component exports and ensure Modals mirrors them
+export let ModalRoot: Modals["ModalRoot"] = NoopComponent;
+export let ModalHeader: Modals["ModalHeader"] = NoopComponent;
+export let ModalContent: Modals["ModalContent"] = NoopComponent;
+export let ModalFooter: Modals["ModalFooter"] = NoopComponent;
+export let ModalCloseButton: Modals["ModalCloseButton"] = NoopComponent;
+// bind Modals getters to the stable refs so consumers always see latest values
+export const Modals: Modals = {
+ get ModalRoot() { return ModalRoot; },
+ get ModalHeader() { return ModalHeader; },
+ get ModalContent() { return ModalContent; },
+ get ModalFooter() { return ModalFooter; },
+ get ModalCloseButton() { return ModalCloseButton; },
+} as unknown as Modals;
+
+// resolve and update the stable refs
+findByProps<Modals>("ModalRoot", "ModalCloseButton", (m: Modals) => {
+ ModalRoot = m.ModalRoot;
+ ModalHeader = m.ModalHeader;
+ ModalContent = m.ModalContent;
+ ModalFooter = m.ModalFooter;
+ ModalCloseButton = m.ModalCloseButton;
return m;
});
Suggestion importance[1-10]: 5
__
Why: The proposal ensures stable references and cleanup of stale captures, which can improve reliability if lazy resolution fails; it’s a reasonable refactor but not strictly required for correctness given current pattern.
| Low
|
Previous suggestions
Suggestions up to commit 7e30481
| Category | Suggestion | Impact |
| Possible issue |
Prevent chunk load aborts
Unhandled rejections from wreq.e will abort Promise.all and stop loading other
chunks. Wrap each call with a .catch to collect failures without breaking the entire
loading process, and optionally log them for diagnostics.
src/debug/loadLazyChunks.ts [66-71]
await Promise.all(
Array.from(validChunkGroups)
.map(([chunkIds]) =>
- Promise.all(chunkIds.map(id => wreq.e(id)))
+ Promise.all(chunkIds.map(id => wreq.e(id).catch(err => {
+ console.error("Failed to load chunk", id, err);
+ invalidChunks.add(id);
+ })))
)
);
Suggestion importance[1-10]: 7
__
Why: Wrapping each wreq.e(id) with a catch prevents one failing chunk from aborting all loads and logs failures, which improves resiliency. It directly applies to the shown code and is contextually accurate.
| Medium
|
Add initialization null checks
Guard against cache or wreq being undefined to avoid runtime errors during early
initialization. Add null checks before iterating and referencing wreq.m.
src/webpack/api.tsx [687-726]
export const _cacheFind = traceFunction("cacheFind", function _cacheFind(filter: FilterFn): CacheFindResult {
if (typeof filter !== "function") {
throw new Error("Invalid filter. Expected a function got " + typeof filter);
+ }
+ if (!cache || !wreq || !wreq.m) {
+ return undefined;
}
for (const key in cache) {
const mod = cache[key];
if (!mod?.loaded || mod?.exports == null) continue;
const factory = wreq.m[key] as AnyModuleFactory;
+ if (!factory) continue;
if (filter.$$vencordIsFactoryFilter) {
- if (filter(wreq.m[key])) {
+ const factoryFn = wreq.m[key];
+ if (factoryFn && filter(factoryFn)) {
return { result: mod.exports, id: key, exportKey: null, factory };
}
-
continue;
}
if (filter(mod.exports)) {
return { result: mod.exports, id: key, exportKey: null, factory };
}
if (typeof mod.exports !== "object") {
continue;
}
if (mod.exports.default != null && filter(mod.exports.default)) {
- return { result: mod.exports.default, id: key, exportKey: "default ", factory };
+ return { result: mod.exports.default, id: key, exportKey: "default", factory };
}
for (const exportKey in mod.exports) if (exportKey.length <= 3) {
const exportValue = mod.exports[exportKey];
-
if (exportValue != null && filter(exportValue)) {
return { result: exportValue, id: key, exportKey, factory };
}
}
}
+ return undefined;
});
Suggestion importance[1-10]: 6
__
Why: Adding guards for cache/wreq can prevent early-init crashes and is reasonable given _initWebpack sets them later. The change is minor but improves robustness; however, current call sites likely ensure initialization, so impact is moderate.
| Low
|
Guard webpack runtime usage
Validate wreq and wreq.e/wreq.m before use and guard String(module) when the proxy
has not resolved. This prevents runtime errors if called before webpack
initialization or if findModuleFactory didn’t resolve.
src/webpack/api.tsx [633-669]
export function extractAndLoadChunksLazy(code: CodeFilterWithSingle, matcher: RegExp = DefaultExtractAndLoadChunksRegex) {
const module = findModuleFactory(code, { isIndirect: true });
const extractAndLoadChunks = makeLazy(async () => {
- if (module[SYM_PROXY_INNER_GET] != null && module[SYM_PROXY_INNER_VALUE] == null) {
+ if (!wreq || !wreq.e || !wreq.m) {
+ return handleWebpackError("extractAndLoadChunks: Webpack runtime is not initialized", false, "Code:", code, "Matcher:", matcher);
+ }
+
+ // If proxy not resolved yet, abort gracefully
+ if ((module as any)[SYM_PROXY_INNER_GET] != null && (module as any)[SYM_PROXY_INNER_VALUE] == null) {
return handleWebpackError("extractAndLoadChunks: Couldn't find module factory", false, "Code:", code, "Matcher:", matcher);
}
- const match = String(module).match(canonicalizeMatch(matcher));
+ const moduleStr = typeof module === "function" ? String(module) : String((module as any)[SYM_PROXY_INNER_VALUE] ?? module);
+ const match = moduleStr.match(canonicalizeMatch(matcher));
if (!match) {
return handleWebpackError("extractAndLoadChunks: Couldn't find chunk loading in module factory code", false, "Code:", code, "Matcher:", matcher);
}
const [, rawChunkIds, entryPointId] = match;
- if (Number.isNaN(Number(entryPointId))) {
- return handleWebpackError("extractAndLoadChunks: Matcher didn't return a capturing group with the chunk ids array, or the entry point id returned as the second group wasn't a number", false, "Code:", code, "Matcher:", matcher);
+ const entryIdNum = Number(entryPointId);
+ if (!Number.isFinite(entryIdNum)) {
+ return handleWebpackError("extractAndLoadChunks: Matcher didn't return valid entry point id as second group", false, "Code:", code, "Matcher:", matcher);
}
if (rawChunkIds) {
- const chunkIds = Array.from(rawChunkIds.matchAll(ChunkIdsRegex)).map(m => Number(m[1]));
- await Promise.all(chunkIds.map(id => wreq.e(id)));
+ const chunkIds = Array.from(rawChunkIds.matchAll(ChunkIdsRegex)).map(m => Number(m[1])).filter(n => Number.isFinite(n));
+ await Promise.all(chunkIds.map(id => wreq.e!(id)));
}
- if (wreq.m[entryPointId] == null) {
+ if (wreq.m[entryIdNum] == null) {
return handleWebpackError("extractAndLoadChunks: Entry point is not loaded in the module factories, perhaps one of the chunks failed to load", false, "Code:", code, "Matcher:", matcher);
}
- wreq(Number(entryPointId));
+ wreq(entryIdNum);
return true;
});
if (IS_REPORTER) {
webpackSearchHistory.push(["extractAndLoadChunks", [extractAndLoadChunks, code, matcher]]);
}
return extractAndLoadChunks;
}
Suggestion importance[1-10]: 6
__
Why: Additional checks for wreq, wreq.e, wreq.m, and safer stringification reduce runtime errors during early calls. It’s a sensible defensive improvement, though the original already handles some failure paths.
| Low
|
Add keys for mapped elements
Add a stable key when mapping badges to prevent React reconciliation bugs and
warnings. Use a unique identifier if available, or a composite key fallback.
src/plugins/reviewDB/components/ReviewComponent.tsx [150]
-{review.sender.badges.map(badge => <ReviewBadge {...badge} />)}
+{review.sender.badges.map(badge => (
+ <ReviewBadge key={badge.id ?? `${badge.name}-${badge.type}`} {...badge} />
+))}
Suggestion importance[1-10]: 6
__
Why: Adding a stable React key for list rendering prevents warnings and potential reconciliation issues; the snippet maps over review.sender.badges without keys, so the fix is accurate and improves maintainability.
| Low
|
Harden proxy get behavior
Accessing target for getOwnPropertyDescriptor and ownKeys must be consistent with
using the inner target to avoid leaking proxyDummy traps. Cache innerTarget once per
get and use it for all Reflect calls on dynamic paths. Also guard against symbols in
the destructuring proxy path to avoid wrapping non-string keys.
src/utils/proxyInner.ts [64-101]
const proxy = new Proxy(proxyDummy, {
...handler,
get(target, p, receiver) {
if (p === SYM_PROXY_INNER_GET || p === SYM_PROXY_INNER_VALUE) {
return Reflect.get(target, p, receiver);
}
- // If we're still in the same tick, it means the proxy was immediately used.
- // And, if the inner value is still nullish, it means the proxy was used before setInnerValue was called.
- // So, proxy the get access to make things like destructuring work as expected.
- // We dont need to proxy if the inner value is available, and recursiveSetInnerValue won't ever be called anyways,
- // because the top setInnerValue was called before we proxied the get access
- // example here will also be a proxy:
- // `const { example } = findByProps("example");`
- if (isSameTick && !isChild && proxyDummy[SYM_PROXY_INNER_VALUE] == null) {
+ if (isSameTick && !isChild && proxyDummy[SYM_PROXY_INNER_VALUE] == null && typeof p === "string") {
console.warn(
"Destructuring webpack finds/proxyInner/proxyLazy at top level is deprecated. For more information read https://github.com/Vendicated/Vencord/pull/2409#issue-2277161516" +
"\nConsider not destructuring, using findProp or if you really need to destructure, using mapMangledModule instead."
);
const [recursiveProxy, recursiveSetInnerValue] = proxyInner(err, primitiveErr, true);
recursiveSetInnerValues.push((innerValue: T) => {
- // Set the inner value of the destructured value as the prop value p of the parent
recursiveSetInnerValue(Reflect.get(innerValue as object, p, innerValue));
});
return recursiveProxy;
}
const innerTarget = target[SYM_PROXY_INNER_GET]();
if (typeof innerTarget === "object" || typeof innerTarget === "function") {
return Reflect.get(innerTarget, p, innerTarget);
}
throw new Error(primitiveErr);
}
});
Suggestion importance[1-10]: 6
__
Why: The added typeof check for non-string property keys during same-tick destructuring is a reasonable hardening and aligns with the existing proxy logic. It’s a minor robustness improvement without contradicting the PR; impact is moderate.
| Low
|
| General |
Fix default key whitespace
Remove the stray trailing space in the exportKey value for default exports to
prevent mismatches where callers compare or log exportKey. This avoids subtle bugs
in consumers relying on exact keys.
src/webpack/api.tsx [714-716]
if (mod.exports.default != null && filter(mod.exports.default)) {
- return { result: mod.exports.default, id: key, exportKey: "default ", factory };
+ return { result: mod.exports.default, id: key, exportKey: "default", factory };
}
Suggestion importance[1-10]: 7
__
Why: The existing code returns exportKey: "default " with a trailing space, which is almost certainly unintended and can cause consumer mismatches. The fix is precise and directly corrects a subtle bug.
| Medium
|
Clone global regex before replace
Resetting lastIndex on a shared RegExp mutates it globally and can corrupt
subsequent searches. Clone global regexes before use to avoid side effects in other
patches.
src/webpack/patchWebpack.ts [419-426]
-if (match instanceof RegExp && match.global) {
- match.lastIndex = 0;
+const safeMatch = match instanceof RegExp && match.global
+ ? new RegExp(match.source, match.flags) // clone to avoid mutating shared RegExp
+ : match;
+
+if (safeMatch instanceof RegExp && safeMatch.global) {
+ safeMatch.lastIndex = 0;
}
-return code.replace(match, replace);
+return code.replace(safeMatch as any, replace);
Suggestion importance[1-10]: 7
__
Why: Mutating shared global RegExp lastIndex can cause subtle bugs; cloning before use avoids side effects. The change aligns with the existing replace logic and improves robustness without altering behavior.
| Medium
|
Add sandbox-disabling flag
Running headless Chromium in many environments requires both --no-sandbox and
--disable-setuid-sandbox to avoid sandbox errors. Add the second flag to improve
reliability in CI and containerized setups.
scripts/generateReport.ts [38-42]
const browser = await pup.launch({
headless: true,
executablePath: process.env.CHROMIUM_BIN,
- args: ["--no-sandbox"]
+ args: ["--no-sandbox", "--disable-setuid-sandbox"]
});
Suggestion importance[1-10]: 5
__
Why: Adding --disable-setuid-sandbox is a common reliability tweak for headless Chromium in CI. It’s correct and safe but relatively minor in impact compared to functional fixes.
| Low
|
Use microtask for deferral
In rare environments, a zero-delay timeout can still run within the same tick,
causing unexpected lazy-proxy behavior. Switch to queueMicrotask (or
Promise.resolve().then) to reliably defer to the next microtask.
src/utils/lazy.ts [80]
-setTimeout(() => isSameTick = false, 0);
+if (!isChild) {
+ // Ensure deferral to a future microtask, avoiding same-tick edge cases
+ (typeof queueMicrotask === "function" ? queueMicrotask : (cb => Promise.resolve().then(cb)))(() => { isSameTick = false; });
+}
Suggestion importance[1-10]: 4
__
Why: Switching from setTimeout(0) to a microtask can make deferral more predictable, but the current approach is generally fine; impact is minor and not critical.
| Low
|
Suggestions up to commit c183018
| Category | Suggestion | Score |
| Possible issue |
Add error handling for the asynchronous call to getText()
Consider handling the case where getText() might return a promise that could reject.
Currently, if getText() throws an error, it will lead to an unhandled promise
rejection. You can add a try-catch block around the await getText() call to handle
potential errors gracefully.
scripts/generateReport.ts [220]
-console.error(await getText());
+try {
+ console.error(await getText());
+} catch (error) {
+ console.error("Error fetching text:", error);
+}
Suggestion importance[1-10]: 9
Why: This suggestion is crucial as it addresses potential unhandled promise rejections, which can lead to runtime errors and crashes. Adding a try-catch block ensures that any errors from the getText() call are handled gracefully.
| 9
|
Add error handling to the chunk loading promises to prevent one failure from affecting others
Consider adding error handling for the wreq.e(id) call within the Promise.all to
handle potential rejections that could cause the entire promise chain to fail. This
will improve the robustness of the code by ensuring that one failing chunk load does
not prevent other operations from continuing.
src/debug/loadLazyChunks.ts [73]
-Promise.all(chunkIds.map(id => wreq.e(id)))
+Promise.all(chunkIds.map(id => wreq.e(id).catch(error => console.error(`Failed to load chunk ${id}:`, error))))
Suggestion importance[1-10]: 9
Why: Adding error handling to the chunk loading promises is crucial for robustness, ensuring that one failure does not halt the entire process. This is a significant improvement in error resilience.
| 9
|
Add error handling for the findByCode function to manage cases where no results are found
Ensure that the findByCode function is correctly typed to handle the case where no
matching code is found. It's good practice to handle potential undefined or null
values that might be returned if the search criteria do not match any entries.
src/api/Commands/commandHelpers.ts [27]
-const createBotMessage = findByCode('username:"Clyde"');
+const createBotMessage = findByCode('username:"Clyde"') ?? throw new Error('Bot message configuration not found.');
Suggestion importance[1-10]: 8
Why: This suggestion improves robustness by handling cases where findByCode might return undefined. This prevents potential runtime errors and ensures the code behaves as expected even when the search criteria do not match any entries.
| 8
|
Add null checks and default values for methods that might return undefined
It's important to handle the case where PluginManager.startDependenciesRecursive or
PluginManager.startPlugin might return undefined or an unexpected result. Consider
adding checks or default values to handle these cases more robustly.
src/components/PluginSettings/index.tsx [103-129]
-const { restartNeeded, failures } = PluginManager.startDependenciesRecursive(plugin);
-const result = wasEnabled ? PluginManager.stopPlugin(plugin) : PluginManager.startPlugin(plugin);
+const { restartNeeded, failures } = PluginManager.startDependenciesRecursive(plugin) ?? { restartNeeded: false, failures: [] };
+const result = wasEnabled ? PluginManager.stopPlugin(plugin) : PluginManager.startPlugin(plugin) ?? false;
Suggestion importance[1-10]: 7
Why: This suggestion enhances code robustness by adding null checks and default values, preventing potential issues when methods return undefined. However, the impact is slightly less critical compared to handling unhandled promise rejections.
| 7
|
| Maintainability |
Improve readability and robustness of component existence checks
Replace the complex conditional check with a more readable and maintainable approach
using optional chaining and nullish coalescing operators.
src/plugins/betterSettings/index.tsx [136]
-if (FocusLock === NoopComponent || ComponentDispatch[SYM_PROXY_INNER_VALUE] == null || Classes[SYM_PROXY_INNER_VALUE] == null) {
+if (!FocusLock || !ComponentDispatch?.[SYM_PROXY_INNER_VALUE] || !Classes?.[SYM_PROXY_INNER_VALUE]) {
Suggestion importance[1-10]: 9
Why: The use of optional chaining and nullish coalescing operators improves readability and robustness, making the code more maintainable and less prone to errors.
| 9
|
Use a more descriptive variable name for the factory parameter in the factoryListener function
To improve the maintainability and readability of the code, consider using a more
descriptive variable name than factory for the factoryListener function parameter. A
name like moduleFactory would provide more context about the type and purpose of the
parameter.
src/debug/loadLazyChunks.ts [113]
-function factoryListener(factory: AnyModuleFactory | ModuleFactory) {
+function factoryListener(moduleFactory: AnyModuleFactory | ModuleFactory) {
Suggestion importance[1-10]: 6
Why: Using a more descriptive variable name enhances code readability and maintainability, though it is a minor improvement.
| 6
|
| Possible bug |
Ensure fact is a string before using replaceAll to avoid runtime errors
The replaceAll method might throw an error if fact is not a string. Ensure that fact
is always a string before calling replaceAll on it, or handle cases where fact might
not be a string.
src/components/VencordSettings/PatchHelperTab.tsx [59]
-const src = String(fact).replaceAll("\n", "");
+const src = typeof fact === 'string' ? fact.replaceAll("\n", "") : '';
Suggestion importance[1-10]: 8
Why: This suggestion is important as it prevents potential runtime errors by ensuring fact is a string before calling replaceAll. This adds a layer of safety and ensures the code does not break unexpectedly.
| 8
|
Ensure the module function exists before invoking it to avoid runtime errors
It's recommended to check the existence of wreq.m[id] before attempting to call
wreq(id) to avoid potential runtime errors if id does not exist in wreq.m.
src/debug/loadLazyChunks.ts [163]
-if (wreq.m[id]) wreq(id);
+if (wreq.m[id] && typeof wreq.m[id] === 'function') wreq(id);
Suggestion importance[1-10]: 8
Why: Ensuring the module function exists before invoking it prevents potential runtime errors, which is a good practice for improving code reliability.
| 8
|
| Best practice |
Add safety checks before accessing properties of potentially undefined objects
Ensure consistency and safety by checking the existence of
ClientThemesBackgroundStore before accessing its properties.
src/plugins/clientTheme/index.tsx [47]
-const nitroTheme = useStateFromStores([ClientThemesBackgroundStore], () => ClientThemesBackgroundStore.gradientPreset);
+const nitroTheme = useStateFromStores([ClientThemesBackgroundStore], () => ClientThemesBackgroundStore?.gradientPreset);
Suggestion importance[1-10]: 8
Why: Adding a safety check with optional chaining ensures that the code does not throw an error if ClientThemesBackgroundStore is undefined, improving robustness.
| 8
|
Define the regular expression as a constant to improve code readability and maintainability
Instead of using a regular expression directly in the matchAll method, consider
defining it as a constant at the beginning of the module. This enhances readability
and maintainability, especially if the regular expression is complex or used
multiple times.
src/debug/loadLazyChunks.ts [139]
-for (const currentMatch of String(wreq.u).matchAll(/(?:"([\deE]+?)")|(?:([\deE]+?):)/g)) {
+const CHUNK_ID_REGEX = /(?:"([\deE]+?)")|(?:([\deE]+?):)/g;
+for (const currentMatch of String(wreq.u).matchAll(CHUNK_ID_REGEX)) {
Suggestion importance[1-10]: 7
Why: Defining the regular expression as a constant improves readability and maintainability, which is a good practice, though not critical.
| 7
|
Suggestions
| Category | Suggestion | Score |
| Best practice |
Replace string literal exceptions with descriptive error messages or custom error types
Replace the string literal thrown in the exception with a more meaningful error message or
a custom error type. Throwing string literals can make error handling and debugging more
difficult.
scripts/generateReport.ts [512]
-throw "a rock at ben shapiro";
+throw new Error("Descriptive error message or custom error type");
Suggestion importance[1-10]: 10
Why: Throwing string literals is a bad practice as it makes error handling and debugging more difficult. Using a descriptive error message or custom error type improves code maintainability and clarity.
| 10
|
Replace non-null assertions with specific types or handle potential null values explicitly
Avoid using non-null assertions (as any) as they bypass TypeScript's type checking. If the
type is uncertain, consider using a more specific type or handling the undefined/null case
explicitly.
scripts/generateReport.ts [481]
-let result = null as any;
+let result: SpecificType | null = null;
Suggestion importance[1-10]: 8
Why: Avoiding non-null assertions improves type safety and helps prevent runtime errors. Using a specific type or handling null values explicitly makes the code more robust and maintainable.
| 8
|
Rename BlobMask to BlobMaskComponent to follow React naming conventions
The BlobMask variable is imported using findExportedComponent which suggests that it is a
React component. It is recommended to follow the React component naming convention by
capitalizing the first letter of the component name.
src/plugins/betterSessions/index.tsx [37]
-const BlobMask = findExportedComponent("BlobMask");
+const BlobMaskComponent = findExportedComponent("BlobMask");
Suggestion importance[1-10]: 8
Why: Following naming conventions is important for code readability and maintainability, especially in a collaborative environment. This suggestion aligns with best practices for React components.
| 8
|
Improve type safety and readability by using a more specific type for lastGuildId
Consider using a more specific type for lastGuildId instead of string | null. If
lastGuildId is expected to hold only certain types of strings (e.g., UUIDs, specific
identifiers), using a more specific type or a type alias could improve type safety and
code readability.
src/plugins/betterFolders/index.tsx [38]
-let lastGuildId = null as string | null;
+type GuildId = string; // Define this type according to your specific use case
+let lastGuildId: GuildId | null = null;
Suggestion importance[1-10]: 7
Why: The suggestion improves type safety and readability, which is beneficial for maintainability. However, it is not critical to the functionality of the code.
| 7
|
Improve the function name cacheFindAll to findAllWithCache for better clarity
Replace the cacheFindAll function with a more descriptive name that indicates its purpose
and functionality, especially if it involves caching mechanisms.
src/plugins/consoleShortcuts/index.ts [45]
-const matches = cacheFindAll(filterFactory(...filterProps));
+const matches = findAllWithCache(filterFactory(...filterProps));
Suggestion importance[1-10]: 6
Why: The suggestion enhances code clarity by providing a more descriptive function name, which helps in understanding the code better. However, it is a minor improvement and does not affect the functionality.
| 6
|
Rename setDecorationGridItem to setDecorationGridItemComponent to clarify its purpose
It is recommended to use a more descriptive name for the setDecorationGridItem function to
clarify that it is a setter function. This enhances code readability and maintainability.
src/plugins/decor/ui/components/index.ts [20]
-export const setDecorationGridItem = v => DecorationGridItem = v;
+export const setDecorationGridItemComponent = v => DecorationGridItem = v;
Suggestion importance[1-10]: 6
Why: The suggestion improves code readability by making the function's purpose clearer. This is a minor enhancement that aids in maintainability but does not impact the core functionality.
| 6
|
| Performance |
Replace eager loading methods with lazy loading equivalents to enhance performance
Replace the direct imports and usage of findByProps and findStore with their lazy
counterparts to avoid potential performance issues due to eager loading. This is
especially important in a plugin system where minimizing the initial load time can
significantly affect the overall performance.
src/plugins/implicitRelationships/index.ts [22-27]
-import { findByProps, findStore } from "@webpack";
-const UserAffinitiesStore = findStore("UserAffinitiesStore");
-const { FriendsSections } = findByProps("FriendsSections");
+import { findByPropsLazy, findStoreLazy } from "@webpack";
+const UserAffinitiesStore = findStoreLazy("UserAffinitiesStore");
+const { FriendsSections } = findByPropsLazy("FriendsSections");
Suggestion importance[1-10]: 9
Why: The suggestion correctly identifies a potential performance issue with eager loading and proposes a valid solution by using lazy loading methods. This change can significantly improve the initial load time of the plugin, which is crucial in a plugin system.
| 9
|
Use a lazy loading approach for findByProps to improve module loading efficiency
Replace the direct use of findByProps with a lazy or asynchronous variant to ensure that
the module is loaded only when necessary, which can help in reducing the initial load time
and potentially avoid runtime errors due to the module not being ready.
src/plugins/decor/ui/index.ts [11]
-export const DecorationModalStyles = findByProps("modalFooterShopButton");
+export const DecorationModalStyles = findByPropsLazy("modalFooterShopButton");
Suggestion importance[1-10]: 7
Why: The suggestion to use lazy loading can improve performance by reducing initial load time. However, the current use of findByProps may be intentional for immediate availability, so the impact might be minor.
| 7
|
| Possible bug |
Correct potential typo in the findComponentByCode function argument
Ensure that the string argument for findComponentByCode does not contain a trailing comma
unless it is intentional to match specific criteria. This could be a typo that might lead
to unexpected behavior or errors.
src/plugins/decor/ui/modals/CreateDecorationModal.tsx [20]
-const FileUpload = findComponentByCode("fileUploadInput,");
+const FileUpload = findComponentByCode("fileUploadInput");
Suggestion importance[1-10]: 9
Why: This suggestion addresses a potential typo that could lead to unexpected behavior or errors, making it a crucial fix for code correctness.
| 9
|
| Possible issue |
Add error handling for cacheFind to ensure stability
Consider handling the case where cacheFind returns null or an unexpected result to prevent
runtime errors. Implement error checking or default value assignment.
src/plugins/devCompanion.dev/index.tsx [207]
-results = cacheFind(filters.byProps(...parsedArgs));
+results = cacheFind(filters.byProps(...parsedArgs)) || [];
Suggestion importance[1-10]: 8
Why: Adding error handling for cacheFind enhances code stability by preventing runtime errors due to null or unexpected results, which is important for robust code.
| 8
|
| Maintainability |
Refactor nested if-else conditions into separate functions or a switch-case structure for better readability
Refactor the nested if-else conditions into separate functions or use a switch-case
structure to improve readability and maintainability.
scripts/generateReport.ts [483-509]
-if (searchType === "webpackDependantLazy" || searchType === "webpackDependantLazyComponent") {
- const [factory] = args;
- result = factory();
-} else if (searchType === "extractAndLoadChunks") {
- const [code, matcher] = args;
- const module = Vencord.Webpack.findModuleFactory(...code);
- if (module) {
- result = module.toString().match(Vencord.Util.canonicalizeMatch(matcher));
- }
-} else {
- const findResult = args.shift();
- if (findResult != null) {
- if (findResult.$$vencordCallbackCalled != null && findResult.$$vencordCallbackCalled()) {
- result = findResult;
+switch (searchType) {
+ case "webpackDependantLazy":
+ case "webpackDependantLazyComponent":
+ const [factory] = args;
+ result = factory();
+ break;
+ case "extractAndLoadChunks":
+ const [code, matcher] = args;
+ const module = Vencord.Webpack.findModuleFactory(...code);
+ if (module) {
+ result = module.toString().match(Vencord.Util.canonicalizeMatch(matcher));
}
- if (findResult[Vencord.Util.proxyInnerGet] != null) {
- result = findResult[Vencord.Util.proxyInnerValue];
+ break;
+ default:
+ const findResult = args.shift();
+ if (findResult != null) {
+ if (findResult.$$vencordCallbackCalled != null && findResult.$$vencordCallbackCalled()) {
+ result = findResult;
+ }
+ if (findResult[Vencord.Util.proxyInnerGet] != null) {
+ result = findResult[Vencord.Util.proxyInnerValue];
+ }
+ if (findResult.$$vencordInner != null) {
+ result = findResult.$$vencordInner();
+ }
}
- if (findResult.$$vencordInner != null) {
- result = findResult.$$vencordInner();
- }
- }
+ break;
}
Suggestion importance[1-10]: 7
Why: Refactoring nested if-else conditions into a switch-case structure improves readability and maintainability. It makes the code easier to understand and reduces the risk of errors.
| 7
|
Add fallback for findByProps to handle potential undefined properties safely
Replace the direct use of findByProps with a more specific method if available, or ensure
that the properties being accessed are correctly encapsulated within the method to prevent
accessing potentially undefined properties.
src/plugins/fakeNitro/index.tsx [40]
-const ProtoUtils = findByProps("BINARY_READ_OPTIONS");
+const ProtoUtils = findByProps("BINARY_READ_OPTIONS") || {};
Suggestion importance[1-10]: 7
Why: The suggestion improves maintainability by ensuring that the code handles potential undefined properties safely, though the current implementation might already be sufficient in many cases.
| 7
|
Closing as I will likely revive this in a different way from scratch
Look VERY promising! Hopefully will get revived and pushed real soon! 👍
Vendicated
