Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR updates configuration and dependencies, broadens blacklist entries to wildcard patterns, adds a pattern-matching blacklist helper, and replaces direct blacklist includes checks with that helper. It also contains small JSX/comment syntax tweaks and a NodeList iteration style change. Changes
Sequence Diagram(s)(Skipped — changes are localized and do not introduce a multi-component sequential flow that requires visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Modals/Settings/DnDList.tsx (1)
73-90: Make the interactive element keyboard accessible.The
divhas anonClickhandler but isn't keyboard accessible. This prevents keyboard users from toggling the tab enabled state.🔎 Proposed fix
Replace the
divwith abuttonelement:- <div className="dnd-box" {...provided.dragHandleProps} onClick={() => onToggleEnabled(item.name)}> + <button type="button" className="dnd-box" {...provided.dragHandleProps} onClick={() => onToggleEnabled(item.name)}> <svg className="dnd-icon" fill="currentColor" width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" aria-label="Drag icon" role="img" > <path fill-rule="evenodd" d="M7.375 3.67c0-.645-.56-1.17-1.25-1.17s-1.25.525-1.25 1.17c0 .646.56 1.17 1.25 1.17s1.25-.524 1.25-1.17zm0 8.66c0-.646-.56-1.17-1.25-1.17s-1.25.524-1.25 1.17c0 .645.56 1.17 1.25 1.17s1.25-.525 1.25-1.17zm-1.25-5.5c.69 0 1.25.525 1.25 1.17 0 .645-.56 1.17-1.25 1.17S4.875 8.645 4.875 8c0-.645.56-1.17 1.25-1.17zm5-3.16c0-.645-.56-1.17-1.25-1.17s-1.25.525-1.25 1.17c0 .646.56 1.17 1.25 1.17s1.25-.524 1.25-1.17zm-1.25 7.49c.69 0 1.25.524 1.25 1.17 0 .645-.56 1.17-1.25 1.17s-1.25-.525-1.25-1.17c0-.646.56-1.17 1.25-1.17zM11.125 8c0-.645-.56-1.17-1.25-1.17s-1.25.525-1.25 1.17c0 .645.56 1.17 1.25 1.17s1.25-.525 1.25-1.17z" /> </svg> {item.name === "Extensions" ? "Extens." : item.name} - </div> + </button>src/extensions/extension.tsx (1)
245-246: Fix potential runtime error from undefined blacklist.If
getBlacklist()returnsundefined,JSON.stringify(undefined)will be stored as the string"undefined"in sessionStorage. Later, at line 194,JSON.parse("undefined")will throw aSyntaxError, breaking the extension initialization.🔎 Proposed fix to handle undefined blacklist
const BLACKLIST = await getBlacklist(); - window.sessionStorage.setItem("marketplace:blacklist", JSON.stringify(BLACKLIST)); + window.sessionStorage.setItem("marketplace:blacklist", JSON.stringify(BLACKLIST ?? []));
🧹 Nitpick comments (3)
src/logic/Utils.ts (2)
39-48: Consider adding pattern validation and ReDoS protection.The regex construction from pattern input is flagged by static analysis. While the current implementation is relatively safe (patterns come from trusted blacklist.json and the regex pattern
[^/]+doesn't cause catastrophic backtracking), adding pattern validation would provide defense in depth.🔎 Suggested improvements
- Add pattern validation to ensure patterns are well-formed:
const matchesBlacklistPattern = (url: string, pattern: string): boolean => { + // Validate pattern format + if (!pattern || !pattern.startsWith("https://github.com/")) { + console.warn(`Invalid blacklist pattern: ${pattern}`); + return false; + } + const normalizedUrl = url.toLowerCase(); const normalizedPattern = pattern.toLowerCase(); if (!normalizedPattern.includes("*")) return normalizedUrl === normalizedPattern; const regexPattern = normalizedPattern.replace(/[.+?^${}()|[\]\\]/g, "\\$&").replace(/\*/g, "[^/]+"); + // Limit regex complexity + if (regexPattern.length > 500) { + console.warn(`Blacklist pattern too complex: ${pattern}`); + return false; + } + return new RegExp(`^${regexPattern}$`).test(normalizedUrl); };
- Consider handling URLs with query parameters or fragments by normalizing them before matching.
Based on static analysis (ast-grep).
29-38: Update JSDoc examples to use actual asterisk characters.The JSDoc comments use
{asterisk}placeholder text instead of actual*characters in the pattern examples.🔎 Proposed fix
/** * Check if a GitHub URL matches a blacklist pattern * Supports case-insensitive matching and glob patterns: * - Exact URL: "https://github.com/user/repo" - * - All repos from user: "https://github.com/user/{asterisk}" - * - Specific repo name from any user: "https://github.com/{asterisk}/reponame" + * - All repos from user: "https://github.com/user/*" + * - Specific repo name from any user: "https://github.com/*/reponame" * @param url The GitHub URL to check * @param pattern The blacklist pattern to match against * @returns True if the URL matches the pattern */src/extensions/extension.tsx (1)
194-194: Consider adding error handling for malformed JSON.The fallback to
"[]"handles the null case well, but if the sessionStorage value is manually corrupted or contains invalid JSON,JSON.parse()will throw. While unlikely in normal operation, consider wrapping in a try-catch block for robustness.🔎 Optional: Add try-catch for defensive programming
- const BLACKLIST: string[] = JSON.parse(window.sessionStorage.getItem("marketplace:blacklist") || "[]"); + let BLACKLIST: string[] = []; + try { + BLACKLIST = JSON.parse(window.sessionStorage.getItem("marketplace:blacklist") || "[]"); + } catch { + console.warn("Failed to parse blacklist from sessionStorage, using empty array"); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
biome.jsonresources/blacklist.jsonsrc/components/Card/Card.tsxsrc/components/Grid.tsxsrc/components/Modals/Settings/DnDList.tsxsrc/components/ReadmePage.tsxsrc/components/TabBar.tsxsrc/extensions/extension.tsxsrc/logic/FetchRemotes.tssrc/logic/Utils.tssrc/resources/locales/index.ts
💤 Files with no reviewable changes (1)
- src/components/Card/Card.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/extensions/extension.tsx (1)
src/logic/Utils.ts (1)
isBlacklisted(56-58)
src/logic/FetchRemotes.ts (1)
src/logic/Utils.ts (1)
isBlacklisted(56-58)
🪛 ast-grep (0.40.3)
src/logic/Utils.ts
[warning] 46-46: 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(^${regexPattern}$)
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)
🪛 GitHub Actions: Lint code
src/components/Grid.tsx
[warning] 1-1: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
src/components/TabBar.tsx
[error] 61-61: Suppression comment has no effect. Remove the suppression or suppress the correct rule. biome-ignore lint/correctness/useExhaustiveDependencies.
[error] 98-98: Suppression comment has no effect. Remove the suppression or suppress the correct rule. biome-ignore lint/correctness/useExhaustiveDependencies.
[error] 109-109: Suppression comment has no effect. Remove the suppression or suppress the correct rule. biome-ignore lint/correctness/useExhaustiveDependencies.
resources/blacklist.json
[error] 1-1: JSON formatting issue detected. Formatter would have reformatted the file.
src/resources/locales/index.ts
[warning] 1-1: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
src/components/Modals/Settings/DnDList.tsx
[error] 73-73: lint/a11y/noStaticElementInteractions - Static Elements should not be interactive.
[warning] 1-1: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
biome.json
[error] 2-2: biome: configuration schema version does not match the CLI version 2.3.2. Found 2.1.3. Run 'biome migrate' to migrate the configuration file.
🔇 Additional comments (5)
resources/blacklist.json (1)
2-9: The blacklist patterns are correctly implemented and working as intended. Testing confirms all patterns—including the wildcard patterns for user (https://github.com/FoxRefire/*) and repository name (https://github.com/*/spiceDL,https://github.com/*/Speedify)—properly match their intended targets with case-insensitive matching and no false positives.src/extensions/extension.tsx (2)
21-21: LGTM: Import addition is correct.The
isBlacklistedfunction is properly imported from the Utils module to support the new pattern-based blacklist checking.
215-215: Pattern matching implementation verified and working correctly.The
isBlacklistedfunction properly delegates tomatchesBlacklistPattern, which correctly handles both exact matches (case-insensitive string comparison when no wildcard is present) and wildcard patterns (where*is converted to[^/]+to match path segments, with proper regex escaping). The implementation uses full-string anchoring (^and$) to ensure accurate matching.src/logic/FetchRemotes.ts (2)
5-5: LGTM: Import addition is correct.The
isBlacklistedfunction is properly imported from Utils, consistent with the refactoring to centralize blacklist pattern matching.
45-45: LGTM: Blacklist filtering updated correctly.The migration to
isBlacklisted(item.html_url, BLACKLIST)is consistent with the refactoring approach and properly integrated with the existingshowArchivedfilter logic. The pattern-based matching will enable more flexible blacklist rules.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.vscode/settings.jsonbiome.jsonpackage.jsonresources/blacklist.jsonsrc/components/Card/Card.tsxsrc/components/Modals/Settings/DnDList.tsxsrc/components/Modals/ThemeDevTools/index.tsx
💤 Files with no reviewable changes (1)
- src/components/Modals/ThemeDevTools/index.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/Card/Card.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/blacklist.json
- src/components/Modals/Settings/DnDList.tsx
- biome.json
🔇 Additional comments (2)
.vscode/settings.json (1)
8-8: LGTM: Trailing newline added.Adding a trailing newline at the end of the file is consistent with POSIX standards and many linter/formatter requirements.
package.json (1)
40-49: Verify i18next 25.x breaking changes don't affect the application.The
i18nextupdate to ^25.7.3 includes breaking changes to TypeScript types and runtime behavior:
- If using
returnObjects: true, theCustomTypeOptionstype must also be set accordingly- Type exports changed:
StringMap→$Dictionary(internal),TFuncKey→ParseKeys,InterpolationOptions.nsis now constrained toNamespace- Migration codemods are available
Confirm that
react-i18next^16.5.0 is compatible with the newi18nextversion and that the codebase doesn't rely on the removed/renamed type exports or changedreturnObjectsbehavior.The
semver^7.7.3 and@biomejs/biome^2.3.10 updates have no known security advisories.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.