Detect dead code and unused dependencies #1687
Conversation
|
Warning Rate limit exceeded@alexeyr-ci has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (14)
WalkthroughThis pull request introduces several configuration and code modifications across multiple files. The changes include updating the ESLint configuration by adding the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
2 times, most recently
from
1f1180b to
d3ffeef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (8)
knip.ts (2)
4-5: Consider using glob patterns consistently.The entry points use
!suffix while project patterns mix both with and without it. For consistency, either use or omit the!suffix across all patterns.- entry: ['node_package/src/ReactOnRails.ts!', 'node_package/src/ReactOnRails.node.ts!'], - project: ['node_package/src/**/*.[jt]s!', 'node_package/tests/**/*.[jt]s'], + entry: ['node_package/src/ReactOnRails.ts', 'node_package/src/ReactOnRails.node.ts'], + project: ['node_package/src/**/*.[jt]s', 'node_package/tests/**/*.[jt]s'],
6-16: Document the rationale for ignored items.While there are some comments explaining why certain items are ignored, consider documenting all ignored items for better maintainability.
ignoreBinaries: [ - // not detected in production mode + // nps is only used in development scripts and not detected in production mode 'nps', + // scripts are development utilities and not part of the production bundle 'node_package/scripts/.*', ], ignoreDependencies: [ - // Not detected because turbolinks itself is not used? + // @types/turbolinks is required for TypeScript compilation but the runtime dependency is optional '@types/turbolinks', - // used in package-scripts.yml + // concurrently is used in package-scripts.yml for running multiple commands 'concurrently', ],script/convert (1)
18-19: Fix string literal style to match Ruby guidelines.The linter indicates a preference for double-quoted strings.
-# Knip doesn't work on the oldest supported Node version and isn't needed there anyway +# Knip doesn"t work on the oldest supported Node version and isn"t needed there anyway🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 19-19: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
node_package/src/buildConsoleReplay.ts (2)
12-12: Consider using a more descriptive JSDoc comment.The current comment only indicates internal usage but could provide more context about the function's purpose and usage.
-/** @internal Exported only for tests */ +/** + * @internal + * Generates a string of console replay commands from console history. + * This function is exported only for testing purposes. + * @param customConsoleHistory - Optional custom console history to use instead of global + * @param numberOfMessagesToSkip - Number of initial messages to skip + * @returns Formatted string of console commands + */
Line range hint
21-36: Enhance error handling and type safety.The current implementation could be improved to handle edge cases more robustly and provide better type safety.
const stringifiedList = msg.arguments.map(arg => { let val: string; try { - if (typeof arg === 'string') { + if (arg === null) { + val = 'null'; + } else if (arg === undefined) { + val = 'undefined'; + } else if (typeof arg === 'string') { val = arg; } else if (arg instanceof String) { val = String(arg); } else { - val = JSON.stringify(arg); - } - if (val === undefined) { - val = 'undefined'; + // Handle circular references + val = JSON.stringify(arg, (key, value) => + typeof value === 'object' && value !== null + ? Object.assign({}, value) + : value + ); } } catch (e) { - val = `${(e as Error).message}: ${arg}`; + const error = e instanceof Error ? e : new Error(String(e)); + val = `Error during stringification: ${error.message}`; + console.warn('Console replay stringification failed:', error); }node_package/src/reactHydrateOrRender.ts (2)
26-28: Consider extracting React version detection logic.The conditional assignment could be moved to a separate utility function for better maintainability.
+const getHydrateFunction = (): HydrateOrRenderType => { + if (!supportsRootApi) { + return (domNode, reactElement) => ReactDOM.hydrate(reactElement, domNode); + } + return reactDomClient.hydrateRoot; +}; -const reactHydrate: HydrateOrRenderType = supportsRootApi ? - reactDomClient.hydrateRoot : - (domNode, reactElement) => ReactDOM.hydrate(reactElement, domNode); +const reactHydrate: HydrateOrRenderType = getHydrateFunction();
Line range hint
30-38: Extract React 18 rendering logic for consistency.Similar to the hydration function, the render logic could be extracted for better organization.
-function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType { - if (supportsRootApi) { - const root = reactDomClient.createRoot(domNode); - root.render(reactElement); - return root; - } - - // eslint-disable-next-line react/no-render-return-value - return ReactDOM.render(reactElement, domNode); -} +const getRenderFunction = () => { + if (!supportsRootApi) { + // eslint-disable-next-line react/no-render-return-value + return (domNode: Element, reactElement: ReactElement) => + ReactDOM.render(reactElement, domNode); + } + return (domNode: Element, reactElement: ReactElement) => { + const root = reactDomClient.createRoot(domNode); + root.render(reactElement); + return root; + }; +}; +const reactRender = getRenderFunction();.github/workflows/main.yml (1)
74-74: Maintain consistency in yarn commands.The addition of
runinyarn run build:rescriptis unnecessary as yarn commands work identically with or without it. For consistency with other yarn commands in the workflow, consider removing it.- run: cd spec/dummy && rm -rf public/webpack/test && yarn run build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer + run: cd spec/dummy && rm -rf public/webpack/test && yarn build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer
🛑 Comments failed to post (2)
.eslintrc (1)
7-8: 🛠️ Refactor suggestion
Remove duplicate prettier configuration.
The 'prettier' plugin appears both in the
extendsarray (as 'prettier' and 'prettier/react') and in thepluginsarray. This could lead to conflicts or redundant processing.plugins: - import - - prettier📝 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.- importpackage.json (1)
18-18:
⚠️ Potential issueFix invalid @types/node version.
The specified version
^22.7.5is invalid as it exceeds the current Node.js version (20.x). This could cause type definition conflicts.- "@types/node": "^22.7.5", + "@types/node": "^20.11.0",📝 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."@types/node": "^20.11.0",
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/package-js-tests.yml (1)
36-38: Add error handling and document Node.js version compatibility.A few suggestions to improve the robustness of this step:
- Add error handling to prevent silent failures
- Document why Knip needs to be removed for Node 16
- name: run conversion script if: matrix.versions == 'oldest' - run: script/convert + run: | + # Remove Knip configuration for Node 16 compatibility + if ! script/convert; then + echo "Error: Failed to convert package.json for Node 16" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.eslintrc(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/package-js-tests.yml(1 hunks)knip.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)package.json(3 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/main.yml
- knip.ts
- script/convert
- .eslintrc
- node_package/src/buildConsoleReplay.ts
- node_package/src/reactHydrateOrRender.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (1)
.github/workflows/package-js-tests.yml (1)
36-38: Verify the conversion script existence and permissions.The workflow references
script/convertbut we should ensure it exists and has proper execution permissions.✅ Verification successful
Script verified and Node.js compatibility explained
The conversion script exists, has proper permissions, and includes clear documentation explaining that Knip is removed for the oldest Node.js version due to compatibility issues. The conditional execution in the workflow is intentional and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify script existence and permissions # Check if the script exists if ! fd -t f "convert" "script"; then echo "Error: script/convert not found" exit 1 fi # Check if the script has execution permissions if ! fd -t x "convert" "script"; then echo "Warning: script/convert may not have execution permissions" fi # Show script contents for review echo "Script contents:" cat script/convertLength of output: 1220
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
2 times, most recently
from
dbb7dbf to
80d5196
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
node_package/src/buildConsoleReplay.ts (1)
Line range hint
13-41: Add input validation and improve error handlingThe function could benefit from more robust input validation and error handling:
- Consider validating
numberOfMessagesToSkipto ensure it's non-negative- Add type narrowing for
msg.argumentsarray elements- Consider adding logging for stringification errors
export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined, numberOfMessagesToSkip: number = 0): string { + if (numberOfMessagesToSkip < 0) { + throw new Error('numberOfMessagesToSkip must be non-negative'); + } // console.history is a global polyfill used in server rendering. const consoleHistory = customConsoleHistory ?? console.history; if (!(Array.isArray(consoleHistory))) { return ''; } const lines = consoleHistory.slice(numberOfMessagesToSkip).map(msg => { const stringifiedList = msg.arguments.map(arg => { let val: string; try { if (typeof arg === 'string') { val = arg; } else if (arg instanceof String) { val = String(arg); } else { val = JSON.stringify(arg); } if (val === undefined) { val = 'undefined'; } } catch (e) { + console.warn(`Failed to stringify console argument:`, e); val = `${(e as Error).message}: ${arg}`; } return scriptSanitizedVal(val); }); return `console.${msg.level}.apply(console, ${JSON.stringify(stringifiedList)});`; }); return lines.join('\n'); }package.json (1)
Line range hint
24-34: Remove unused dependency flagged by CIThe pipeline indicates that
eslint-plugin-jsx-a11yis unused. Consider removing it if it's not required."eslint-config-prettier": "^7.0.0", "eslint-config-shakacode": "^16.0.1", "eslint-plugin-import": "^2.29.1", - "eslint-plugin-jsx-a11y": "^6.8.0", "eslint-plugin-prettier": "^3.4.1",🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] Unused dev dependency: eslint-plugin-jsx-a11y
🧹 Nitpick comments (5)
node_package/src/buildConsoleReplay.ts (2)
12-12: Consider using TypeScript's internal modifierInstead of using JSDoc comment, consider using TypeScript's built-in
internalmodifier for better type-safety and tooling support.-/** @internal Exported only for tests */ +/** @internal */
12-12: Consider enhancing the JSDoc comment.While the
@internaltag is helpful, the documentation could be more descriptive about the function's purpose, parameters, and return value.-/** @internal Exported only for tests */ +/** + * Processes console history into a string of executable console statements. + * @internal Exported only for tests + * @param customConsoleHistory - Optional custom console history to process + * @param numberOfMessagesToSkip - Number of initial messages to skip (default: 0) + * @returns Formatted string of console statements + */.github/workflows/lint-js-and-ruby.yml (2)
52-55: Consider caching Knip resultsRunning Knip twice (with and without --production) might impact CI performance. Consider caching the results or combining the runs.
- name: Detect dead code run: | + # Cache Knip results to improve CI performance + mkdir -p .knip-cache + KNIP_CACHE_DIR=.knip-cache yarn run knip yarn run knip --production
52-55: Consider combining Knip runs for efficiency.Running Knip twice (with and without --production) might impact CI time. Consider combining these runs if possible.
- - name: Detect dead code - run: | - yarn run knip - yarn run knip --production + - name: Detect dead code + run: yarn run knip:allThen add a new script in package.json:
{ "scripts": { "knip:all": "knip && knip --production" } }package.json (1)
67-68: Add description for the knip scriptConsider adding a comment in the package.json to describe the purpose of the knip script for better maintainability.
"release:major": "node_package/scripts/release major", - "knip": "knip" + "knip": "knip", // Detect dead code and unused dependencies🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] Unused dev dependency: eslint-plugin-jsx-a11y
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.eslintrc(1 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/package-js-tests.yml(1 hunks)knip.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(2 hunks)node_package/src/context.ts(1 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)package.json(3 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- script/convert
- .github/workflows/main.yml
- .github/workflows/package-js-tests.yml
- .eslintrc
- knip.ts
- node_package/src/reactHydrateOrRender.ts
🧰 Additional context used
🪛 Biome (1.9.4)
node_package/src/context.ts
[error] 6-6: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 GitHub Actions: Lint JS and Ruby
package.json
[warning] Unused dev dependency: eslint-plugin-jsx-a11y
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (7)
node_package/src/context.ts (2)
1-1: LGTM! Modern type definition using globalThis.The new type alias provides better type safety and follows modern TypeScript practices by using
globalThis.
6-9: Consider the static analysis warning about void in union type.While the static analyzer suggests replacing
voidwithundefined, the current usage is actually correct here. Thevoidreturn type is appropriate when explicitly indicating that a function might not return a value, which is different from returningundefined.The function's behavior should be verified in these scenarios:
- Browser context (window)
- Node.js context (global)
- Neither context (this)
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
node_package/src/clientStartup.ts (2)
10-10: LGTM! Consistent type usage.Good practice to import the shared Context type, promoting type consistency across the codebase.
26-29: Verify Node.js compatibility after removing NodeJS.Global.The switch from
NodeJS.GlobaltoglobalThismodernizes the code but requires verification in Node.js environments.Let's verify Node.js compatibility:
✅ Verification successful
The switch to
globalThisis compatible with Node.js environmentsThe codebase already uses modern JavaScript features and proper TypeScript configurations that support
globalThis. The implementation follows type-safe practices with correct global declarations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Node.js version requirements in package.json echo "Checking Node.js version requirements..." rg "engines" -A 2 package.json # Search for other potential NodeJS.Global usages that might need updating echo "Checking for other NodeJS.Global usages..." rg "NodeJS\.Global" --type tsLength of output: 248
Script:
#!/bin/bash # Find and check all package.json files echo "=== Checking package.json files ===" fd package.json --type f --exec cat {} # Check TypeScript configurations echo -e "\n=== Checking TypeScript configurations ===" fd tsconfig.json --type f --exec cat {} # Search for global type declarations and globalThis usage echo -e "\n=== Checking global type declarations and globalThis usage ===" rg "declare global|globalThis" --type ts -A 2Length of output: 9080
node_package/src/buildConsoleReplay.ts (2)
Line range hint
43-45: LGTM! Well-structured integration.The default export
buildConsoleReplaycleanly integrates with the newconsoleReplayfunction while maintaining its existing behavior.
Line range hint
13-41: Verify error handling for edge cases.The function handles various type conversions and potential errors, but there are some edge cases to consider:
- Handling of
nullvalues in console arguments- Handling of circular references in objects
- Potential memory issues with very large console histories
Let's verify the handling of these edge cases:
✅ Verification successful
Edge cases are properly handled in the implementation ✅
The code effectively handles all the mentioned edge cases:
- Null values are safely stringified within the try-catch block
- Circular references are caught when JSON.stringify throws
- Large histories can be paginated using
numberOfMessagesToSkipparameter🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering edge cases ast-grep --pattern 'describe($_, function() { $$$ it($_, function() { $$$ consoleReplay($$$) $$$ }) $$$ })'Length of output: 129
Script:
#!/bin/bash # Find test files and implementation details echo "=== Looking for test files ===" fd "test" --type f echo -e "\n=== Searching for circular reference handling ===" rg -l "circular" --type ts echo -e "\n=== Checking implementation for null handling ===" ast-grep --pattern 'if ($_ === null) { $$$ }' echo -e "\n=== Looking for array size limitations ===" rg -l "slice|length.*limit" --type tsLength of output: 1640
Script:
#!/bin/bash # Check test file content and implementation details echo "=== Examining test file ===" cat node_package/tests/buildConsoleReplay.test.js echo -e "\n=== Checking slice implementation ===" rg -A 5 "slice" node_package/src/buildConsoleReplay.tsLength of output: 3612
.github/workflows/lint-js-and-ruby.yml (1)
52-56: Verify error handling in workflow.Ensure the workflow fails appropriately if Knip detects dead code or unused dependencies.
✅ Verification successful
Error handling is properly implemented through GitHub Actions' default behavior ✅
The workflow will fail if Knip detects any issues since:
- GitHub Actions fails any step that returns a non-zero exit code by default
- Both Knip commands must succeed for the step to pass
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Knip's exit codes are properly handled rg -A 5 'continue-on-error|if \[\[ \$\? -ne 0 \]\]' .github/workflows/Length of output: 72
Script:
#!/bin/bash # Check for Knip configuration and step-level configurations rg -g '*.yml' -g '*.json' 'knip' .Length of output: 106
| "@types/jest": "^29.5.14", | ||
| "@types/node": "^20.17.16", |
There was a problem hiding this comment.
💡 Codebase verification
Update @types/node to match Node.js version
The project is running on Node.js 22.x but using type definitions for Node.js 20.x. Update to @types/node@^22.0.0 to ensure proper type coverage for Node.js 22 features.
🔗 Analysis chain
Verify Node.js type definitions version
The @types/node version ^20.17.16 is quite recent. Ensure it's compatible with your supported Node.js versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version compatibility
node -v
npm view @types/[email protected] engines
Length of output: 57
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] Unused dev dependency: eslint-plugin-jsx-a11y
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
2 times, most recently
from
9b211d4 to
458c74e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
node_package/src/context.ts (1)
6-6: Consider usingundefinedinstead ofvoidin the return type.The use of
voidin a union type can be confusing as it's primarily meant for function return type annotations. Consider usingundefinedinstead.-export default function context(this: void): Context | void { +export default function context(this: void): Context | undefined {🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
knip.ts (1)
7-12: Consider using more specific patterns for script exclusions.The pattern
'node_package/scripts/.*'might be too broad. Consider listing specific script files to avoid accidentally ignoring new scripts that should be analyzed.node_package/src/buildConsoleReplay.ts (5)
12-12: Consider stronger access control for internal function.While the
@internalJSDoc tag is good, consider moving this function to a separate internal utility file to prevent accidental usage.
12-12: Consider enhancing the JSDoc comment.While the
@internaltag is helpful, consider adding more detailed documentation including:
- Parameter descriptions
- Return value description
- Example usage
- Why it's exported for tests
-/** @internal Exported only for tests */ +/** + * Processes console history and formats it for replay. + * @internal Exported only for tests + * @param customConsoleHistory - Optional custom console history to process + * @param numberOfMessagesToSkip - Number of initial messages to skip (must be non-negative) + * @returns Formatted string of console replay commands + * @example + * const replay = consoleReplay(console.history, 2); + */
13-13: Add input validation for numberOfMessagesToSkip.The parameter
numberOfMessagesToSkipshould be validated to ensure it's non-negative.export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined, numberOfMessagesToSkip: number = 0): string { + if (numberOfMessagesToSkip < 0) { + throw new Error('numberOfMessagesToSkip must be non-negative'); + }
Line range hint
21-36: Consider performance and error handling improvements.The stringification logic could be enhanced to:
- Handle circular references in objects
- Implement size limits for large objects
- Add more specific error types in catch block
try { if (typeof arg === 'string') { val = arg; } else if (arg instanceof String) { val = String(arg); } else { - val = JSON.stringify(arg); + // Add circular reference handling and size limits + const seen = new WeakSet(); + val = JSON.stringify(arg, (key, value) => { + if (typeof value === 'object' && value !== null) { + if (seen.has(value)) { + return '[Circular Reference]'; + } + seen.add(value); + } + return value; + }); } if (val === undefined) { val = 'undefined'; } } catch (e) { - val = `${(e as Error).message}: ${arg}`; + val = `Error stringifying console argument: ${(e as Error).message}`; }
Line range hint
4-10: Consider enhancing type safety.The type declarations could be more specific:
- Use a union type for console levels
- Add readonly modifiers where applicable
declare global { interface Console { - history?: { - arguments: Array<string | Record<string, string>>; level: "error" | "log" | "debug"; - }[]; + history?: ReadonlyArray<{ + readonly arguments: ReadonlyArray<string | Record<string, string>>; + readonly level: 'error' | 'warn' | 'info' | 'log' | 'debug'; + }>; } }package.json (1)
68-68: Consider enhancing the Knip script configuration.The basic
knipscript is functional but could be improved with additional options for different use cases.Consider adding these script variants:
- "knip": "knip" + "knip": "knip", + "knip:ci": "knip --strict", + "knip:fix": "knip --fix"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.eslintrc(1 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/package-js-tests.yml(1 hunks)knip.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(2 hunks)node_package/src/context.ts(1 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)package.json(3 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- script/convert
- .eslintrc
- node_package/src/reactHydrateOrRender.ts
- .github/workflows/lint-js-and-ruby.yml
- .github/workflows/main.yml
- .github/workflows/package-js-tests.yml
🧰 Additional context used
🪛 Biome (1.9.4)
node_package/src/context.ts
[error] 6-6: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (8)
node_package/src/context.ts (1)
1-1: Good modernization usingglobalThis!The new
Contexttype alias leverages the standardglobalThisinstead of the Node-specificNodeJS.Global, making the type definition more portable and standardized.node_package/src/clientStartup.ts (2)
10-10: LGTM! Clean type import.The explicit type import keeps the type dependencies clear and maintainable.
26-29: Good modernization of global type declarations!The switch from
NodeJS.Globalinterface toglobalThisnamespace withvardeclarations:
- Better aligns with modern JavaScript standards
- More accurately represents the runtime behavior
- Maintains type safety while being more portable
knip.ts (2)
4-6: LGTM! Entry points are well defined.The entry configuration correctly identifies the main production TypeScript files with clear marking using
!suffix.
13-21: Verify the necessity of ignoring eslint-plugin-jsx-a11y.The comment suggests this plugin is "transitively required by a config" but not directly used. This aligns with the static analysis warning from the previous review about it being an unused dev dependency.
node_package/src/buildConsoleReplay.ts (2)
Line range hint
13-41: LGTM! Robust implementation with proper error handling.The function implementation is thorough with:
- Proper null checks for console history
- Comprehensive type handling
- Error catching during stringification
- Security consideration with script sanitization
Line range hint
46-48: LGTM! Clean integration with existing code.The
buildConsoleReplayfunction cleanly integrates with the newconsoleReplayfunction while maintaining its original functionality.package.json (1)
34-34: LGTM! Appropriate Knip version selected.Version ^5.43.1 is recent and stable, suitable for dead code detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
knip.ts (2)
9-24: Consider using more specific patterns for script ignores.The ignore patterns are well-documented. However, the pattern
'node_package/scripts/.*'might be too broad. Consider listing specific scripts or using more targeted patterns to avoid accidentally ignoring future scripts that should be analyzed.
3-56: Well-structured workspace separation with clear documentation.The configuration effectively separates concerns between the main package and the dummy spec environment. The documentation for ignored items is particularly helpful for maintenance. Consider adding a comment at the top of the file explaining the overall purpose of each workspace and when developers should modify them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
knip.ts(1 hunks)spec/dummy/package.json(2 hunks)spec/dummy/postcss.config.js(0 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/postcss.config.js
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (6)
spec/dummy/package.json (1)
77-78: Verify the impact of script command changes.The changes to the script commands:
formatnow usesyarn run npsinstead ofyarn starttestnow usesbin/rspecinstead ofrspecThese changes might affect the build process and CI pipeline.
Let's verify the nps configuration and rspec setup:
✅ Verification successful
Script command changes are properly configured and safe to use
The changes are consistent with the project setup:
yarn run nps formatis valid as nps is properly configured as the project's script runnerbin/rspecexists and follows Rails conventions for running tests🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if nps configuration exists and contains format script fd -t f "package-scripts.{js,yml,json}" -X cat # Check if bin/rspec exists and is executable fd -t f "^rspec$" -p "bin/"Length of output: 501
Script:
#!/bin/bash # Check package.json files for nps configuration echo "=== Package.json files ===" fd -t f "package.json" -X cat {} # Look for any nps related files echo -e "\n=== NPS related files ===" fd -t f -g "package-scripts*" # Check for rspec binary and CI configurations echo -e "\n=== RSpec binary ===" fd -t f "rspec" -x ls -l {} echo -e "\n=== CI Configurations ===" fd -t f -g "*.{yml,yaml}" -p ".github/workflows/"Length of output: 8265
knip.ts (5)
1-3: LGTM! Type-safe configuration setup.The configuration is properly typed using the
KnipConfigtype from the Knip package.
58-58: LGTM! Standard export syntax.The configuration is properly exported as the default export.
7-8: Verify the entry point paths exist.The entry points and project patterns look correct, but let's verify they exist in the repository.
✅ Verification successful
Entry points and project patterns are valid ✓
Both entry points exist and the project patterns correctly capture all TypeScript source files and JavaScript test files in their respective directories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify entry points and project patterns exist echo "Checking entry points..." fd -t f "ReactOnRails.ts$|ReactOnRails.node.ts$" node_package/src/ echo -e "\nChecking if project patterns match files..." fd -t f -e ts -e js node_package/src/ fd -t f -e ts -e js node_package/tests/Length of output: 1139
Script:
#!/bin/bash # Verify project patterns with correct syntax echo "Checking files in node_package/src/..." fd -e ts -e js . node_package/src/ echo -e "\nChecking files in node_package/tests/..." fd -e ts -e js . node_package/tests/Length of output: 1385
35-37: Verify webpack configuration structure.The comment about webpack config naming suggests a potential limitation in Knip's webpack support. Let's verify the webpack configuration structure.
✅ Verification successful
Webpack configuration structure is valid ✅
The webpack configuration files exist in
spec/dummy/config/webpack/and follow the React on Rails conventions. The comment about webpack.config naming is a valid note about Knip's behavior rather than an issue.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify webpack configuration structure echo "Checking webpack configuration files..." fd -t f "webpack" config/webpack/ echo -e "\nChecking webpack config references..." rg -l "webpack.config" config/webpack/Length of output: 411
Script:
#!/bin/bash # Search for webpack config files anywhere in the codebase echo "Searching for webpack config files..." fd -t f "webpack.config.js|webpack.*.js" echo -e "\nChecking if this is a template/example..." rg -l "example|template" knip.ts echo -e "\nChecking file content for context..." cat knip.tsLength of output: 3069
32-33: Investigate why the server bundle isn't detected.The comment indicates an issue with dependency detection for
server-bundle.js. This might indicate a configuration problem in the webpack setup or an issue with Knip's analysis.✅ Verification successful
Server bundle dependency is correctly marked as required
The file
client/app/generated/server-bundle-generated.jsis a legitimate dependency ofserver-bundle.jsas shown by the import statement inspec/dummy/client/app/packs/server-bundle.js. The explicit inclusion inknip.tsis necessary since this dependency is dynamically generated and not automatically detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check webpack configuration and bundle references echo "Checking webpack configurations..." fd -t f "webpack.config.js|server-bundle.js" config/webpack/ client/app/ echo -e "\nChecking bundle dependencies..." rg -A 5 "server-bundle" client/app/packs/Length of output: 493
Script:
#!/bin/bash # Find webpack configs and bundle files echo "Locating webpack configurations and bundle files..." fd -t f "webpack.config" echo -e "\nLocating server bundle files..." fd -t f "server-bundle" echo -e "\nSearching for server bundle references..." rg "server-bundle" -A 3Length of output: 27460
spec/dummy/package.json
Outdated
| @@ -45,6 +44,7 @@ | |||
| "react-router-dom": "^5.2.0", | |||
| "redux": "^4.0.1", | |||
| "redux-thunk": "^2.2.0", | |||
| "regenerator-runtime": "^0.13.4", | |||
There was a problem hiding this comment.
💡 Codebase verification
Update regenerator-runtime from 0.13.4 to 0.14.1
The package is used in your client bundle for async/generator function support. The latest version (0.14.1) includes improvements while maintaining backward compatibility. Update the version in spec/dummy/package.json.
🔗 Analysis chain
Consider updating regenerator-runtime to a newer version.
The added dependency [email protected] is quite old (released in 2020). Consider updating to the latest stable version for potential bug fixes and improvements.
Run this script to check the latest version and any breaking changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and changelog
curl -s https://registry.npmjs.org/regenerator-runtime | jq -r '.["dist-tags"].latest'
Length of output: 95
Script:
#!/bin/bash
# Search for regenerator-runtime imports and usage
echo "=== Searching for regenerator-runtime imports ==="
rg "require.*regenerator-runtime|import.*regenerator-runtime" -g '!package*.json'
echo -e "\n=== Checking other package.json files for regenerator-runtime ==="
fd package.json --exec grep -l "regenerator-runtime"
Length of output: 501
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
5 times, most recently
from
4291617 to
41c13b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/dummy/config/webpack/commonWebpackConfig.js (1)
27-27: LGTM! Good defensive programming practice.The addition of optional chaining (
?.) makes the code more robust by safely handling cases where the SCSS rule'susearray might be undefined.Consider adding a warning log when the
usearray is undefined, as this might indicate a misconfiguration:-baseClientWebpackConfig.module.rules[scssConfigIndex]?.use.push(sassLoaderConfig); +const scssRule = baseClientWebpackConfig.module.rules[scssConfigIndex]; +if (scssRule?.use) { + scssRule.use.push(sassLoaderConfig); +} else { + console.warn('SCSS rule or its use array is undefined. Sass resources loader not added.'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
knip.ts(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/package.json(2 hunks)spec/dummy/postcss.config.js(0 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/postcss.config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- knip.ts
🧰 Additional context used
🪛 GitHub Actions: Main test
spec/dummy/package.json
[warning] Browserslist: caniuse-lite is outdated. Please run 'npx browserslist@latest --update-db'
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (4)
spec/dummy/package.json (4)
28-28: Update regenerator-runtime to the latest version.The package is outdated and should be updated to the latest stable version for bug fixes and improvements.
🧰 Tools
🪛 GitHub Actions: Main test
[warning] Browserslist: caniuse-lite is outdated. Please run 'npx browserslist@latest --update-db'
38-63: LGTM! Correctly moved build dependencies to devDependencies.This is a good practice as these packages are only needed during development/build time, not at runtime.
🧰 Tools
🪛 GitHub Actions: Main test
[warning] Browserslist: caniuse-lite is outdated. Please run 'npx browserslist@latest --update-db'
73-74: LGTM! Script updates look good.The changes to use
yarn run nps formatandbin/rspecare appropriate updates to the build scripts.🧰 Tools
🪛 GitHub Actions: Main test
[warning] Browserslist: caniuse-lite is outdated. Please run 'npx browserslist@latest --update-db'
Line range hint
1-85: Update caniuse-lite database.The pipeline is warning about an outdated caniuse-lite database.
Run the following command to update the database:
🧰 Tools
🪛 GitHub Actions: Main test
[warning] Browserslist: caniuse-lite is outdated. Please run 'npx browserslist@latest --update-db'
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
4 times, most recently
from
76e5519 to
6697543
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/lint-js-and-ruby.yml (1)
57-59: Consider adding error handling for Knip execution.The Knip commands look good, but consider adding error handling and documentation.
- name: Detect dead code run: | + # First run detects dead code in all dependencies yarn run knip + # Second run focuses on production dependencies only yarn run knip --production + # Store the results for later analysis + yarn run knip --reporter json > knip-report.json || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/lint-js-and-ruby.yml(1 hunks)knip.ts(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/package.json(2 hunks)spec/dummy/postcss.config.js(0 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/postcss.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/dummy/config/webpack/commonWebpackConfig.js
- knip.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/lint-js-and-ruby.yml
55-55: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (3)
.github/workflows/lint-js-and-ruby.yml (1)
52-55: Verify the dummy app setup steps.The new steps for setting up the dummy app look good, but there's a potential improvement in the environment variable usage.
Consider extracting
RAILS_ENV=testto a job-level environment variable for consistency and maintainability:jobs: build: runs-on: ubuntu-22.04 + env: + RAILS_ENV: test✅ Verification successful
Environment variable usage is consistent across workflows
The
RAILS_ENV=testis used consistently across both workflow files, always in the context of dummy app commands. While moving it to job-level environment variables would be a valid refactoring option, the current implementation is correct and doesn't pose any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that RAILS_ENV is consistently used across the workflow rg "RAILS_ENV=" .github/workflows/Length of output: 603
🧰 Tools
🪛 actionlint (1.7.4)
55-55: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
spec/dummy/package.json (2)
42-42: Update regenerator-runtime to the latest version.The added regenerator-runtime version (0.13.4) is outdated. The latest version (0.14.1) includes improvements while maintaining backward compatibility.
71-72: Verify the impact of script command changes.The changes to format and test scripts look good, but let's verify the commands exist and work as expected.
✅ Verification successful
All script commands are properly configured and available ✅
- The format command correctly references the root nps format command
- The test command's dependencies (build:test, lint) exist with proper implementations
- The bin/rspec executable is available at spec/dummy/bin/rspec
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if nps format command exists in the root package.json echo "=== Checking format command in root package.json ===" jq '.scripts | keys | .[] | select(contains("format"))' package.json # Verify bin/rspec exists and is executable echo -e "\n=== Checking bin/rspec ===" if [ -f "bin/rspec" ]; then ls -l bin/rspec else echo "bin/rspec not found" fiLength of output: 326
Script:
#!/bin/bash # Check commands in spec/dummy/package.json echo "=== Checking commands in spec/dummy/package.json ===" if [ -f "spec/dummy/package.json" ]; then echo "Format command:" jq '.scripts.format' spec/dummy/package.json echo -e "\nTest command:" jq '.scripts.test' spec/dummy/package.json echo -e "\nBuild:test command:" jq '.scripts["build:test"]' spec/dummy/package.json echo -e "\nLint command:" jq '.scripts.lint' spec/dummy/package.json fi # Search for rspec files echo -e "\n=== Searching for rspec files ===" fd rspec # Check if this is a Ruby project echo -e "\n=== Checking for Gemfile ===" if [ -f "Gemfile" ]; then echo "Found Gemfile. Checking for rspec gem:" grep -i "rspec" Gemfile fiLength of output: 1110
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
from
6697543 to
5da30fd
Compare
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
from
2e12b43 to
5c8e554
Compare
alexeyr-ci
force-pushed
the
alexeyr/dead-code
branch
from
5c8e554 to
4baac71
Compare
Summary
Use Knip to detect dead code and unused dependencies
Pull Request checklist
Update documentationUpdate CHANGELOG fileThis change is
Summary by CodeRabbit
Release Notes
Dependency Management
@types/nodefor improved type definitions.knipfor dependency management.regenerator-runtimeto the dummy package.Configuration Updates
importplugin.Code Changes
consoleReplayfunction for enhanced console history management.globalThisfor improved type definitions.Build Process