remove biome, update eslint + ts configs #192
Conversation
|
Warning Rate limit exceeded@fitzk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 55 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. WalkthroughWalkthroughThe changes involve significant updates to ESLint and TypeScript configurations across various files, including the introduction of new configuration files and the removal of outdated ones. The project structure has been refined, with a focus on utilizing workspace configurations for ESLint, Prettier, and TypeScript. Additionally, the GitHub Actions workflows have been modified to include a new linting step, while several packages have been updated or replaced to streamline development processes. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant ESLint
participant TypeScript
Developer->>GitHub Actions: Push changes
GitHub Actions->>ESLint: Run linting step
ESLint-->>GitHub Actions: Lint results
GitHub Actions->>TypeScript: Compile TypeScript
TypeScript-->>GitHub Actions: Compilation results
GitHub Actions-->>Developer: Build results
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range and nitpick comments (24)
apps/mocksi-lite-next/.eslintrc.js (1)
1-5: LGTM! Consider adding parser options for improved TypeScript support.The ESLint configuration looks good and follows best practices:
- Extending from a shared configuration (
@repo/config-eslint/web.js) promotes consistency.- Setting
root: trueis appropriate for a sub-project in a monorepo.Consider adding parser options to improve TypeScript support:
/** @type {import("eslint").Linter.Config} */ module.exports = { extends: ["@repo/config-eslint/web.js"], root: true, + parserOptions: { + project: "./tsconfig.json", + tsconfigRootDir: __dirname, + }, };This addition will ensure that ESLint uses the correct TypeScript configuration for this specific app.
packages/config-prettier/index.js (1)
10-10: LGTM: Correct module exportThe configuration object is correctly exported using CommonJS syntax, which is consistent with the
.jsfile extension.For future updates, consider using ES modules (e.g.,
export default config;) if your project supports it. This would align with modern JavaScript practices and potentially improve compatibility with tools that prefer ES modules.packages/config-typescript/web-app.json (1)
2-5: Compiler options look good, consider sorting types.The compiler options are well-configured for a modern React web application. The
jsxoption is correctly set to "react-jsx" for the new JSX transform, and thetypesarray includes essential libraries for web development and testing.Consider sorting the
typesarray alphabetically for better readability:- "types": ["chrome", "node", "react-dom", "react", "uuid", "vitest", "vite"] + "types": ["chrome", "node", "react", "react-dom", "uuid", "vite", "vitest"]turbo.json (1)
14-18: LGTM: Good addition of "format" task and restructuring of "lint" taskThe changes look good:
- The new "format" task with
"dependsOn": ["^format"]ensures that formatting is done hierarchically, running parent format tasks first.- The "lint" task is similarly configured with
"dependsOn": ["^lint"], maintaining consistency.- The order of tasks (format before lint) is logical and follows best practices.
These changes improve the project's structure and task execution flow in the monorepo setup.
Consider adding
"outputs"to both "format" and "lint" tasks if they generate any artifacts or reports. This can help Turborepo optimize caching. For example:"format": { "dependsOn": ["^format"], "outputs": [] }, "lint": { "dependsOn": ["^lint"], "outputs": [] }If these tasks don't generate any outputs, you can leave the
"outputs"array empty or omit it entirely.packages/config-prettier/package.json (1)
2-6: Consider pinning the version of prettier-plugin-organize-attributesUsing "latest" for
prettier-plugin-organize-attributescould lead to unexpected breaking changes in the future. It's generally a good practice to pin all dependencies to specific versions for better consistency and reproducibility.Consider updating the devDependencies section as follows:
"devDependencies": { "prettier": "^3.2.5", - "prettier-plugin-organize-attributes": "latest", + "prettier-plugin-organize-attributes": "^0.2.2", "prettier-plugin-tailwindcss": "^0.5.11" },Replace "^0.2.2" with the latest stable version available at the time of implementation.
packages/config-eslint/package.json (1)
15-17: Consider starting with a lower version number.For a newly created package, it's often better to start with a version number less than 1.0.0 (e.g., 0.1.0) to indicate that it's still in initial development and may undergo significant changes.
Here's a suggested change:
{ ... "name": "@repo/config-eslint", "private": true, - "version": "1.0.0" + "version": "0.1.0" }package.json (2)
2-9: LGTM! Consider pinning the turbo version.The updates to the dependencies align well with the PR objectives. The move to workspace references for ESLint, TypeScript, and Prettier configs is a good practice for maintaining consistency across the project.
Consider pinning the
turboversion to a specific version instead of using"latest". This ensures reproducible builds and prevents unexpected breaking changes. For example:- "turbo": "latest" + "turbo": "^1.10.0" // Replace with the current latest version
17-21: LGTM! Consider adding a combined script for convenience.The addition of
formatandlintscripts is excellent. These will help maintain code quality and consistency across the project.For convenience, consider adding a combined script that runs both formatting and linting:
"scripts": { "build": "turbo build", "dev": "turbo dev", "format": "turbo format", - "lint": "turbo lint" + "lint": "turbo lint", + "check": "turbo format lint" }This allows developers to run both tasks with a single command:
npm run checkoryarn check.packages/config-typescript/package.json (2)
2-10: LGTM! Consider adding a package.json script for type checking.The devDependencies section looks good. It includes necessary TypeScript type definitions for various libraries, which will enhance type safety and developer experience. The use of caret (^) versioning allows for automatic updates to minor and patch versions, keeping dependencies up-to-date while avoiding potential breaking changes.
Consider adding a
check-typesscript in the"scripts"section of your package.json to easily run type checking across your project:"scripts": { "check-types": "tsc --noEmit" }This will allow developers to quickly verify type correctness by running
npm run check-typesoryarn check-types.
11-15: LGTM! Consider adding a brief description for each configuration file.The
filessection correctly specifies which files should be included in the package. The separation into web-library, web-app, and base configurations is a good practice for maintaining different TypeScript settings for various project types.To improve clarity, consider adding a brief comment in the package.json (or in a separate README.md file) describing the purpose of each configuration file. For example:
"files": [ "web-library.json", // TypeScript config for web libraries "web-app.json", // TypeScript config for web applications "base.json" // Base TypeScript config extended by others ]This will help users of your package quickly understand which configuration file they should use for their specific project type.
packages/config-typescript/base.json (1)
8-21: Excellent compiler options for enhanced type safety and modern development.The configuration enforces strict type-checking and includes several safety features that will greatly improve code quality:
strict: trueenables all strict type-checking options.noImplicitOverrideandnoUncheckedIndexedAccesscatch potential bugs related to class inheritance and indexed access.- Source map generation and JSON module resolution support debugging and configuration management.
- Including ESLint types suggests good integration with linting tools.
Consider documenting the implications of
verbatimModuleSyntax: truefor the development team, as it may affect how import/export statements are handled, especially for developers new to this setting.packages/config-eslint/base.js (1)
19-22: LGTM! Consider specifying the TypeScript config file path.The use of "@typescript-eslint/parser" and enabling the "project" option in parserOptions is correct for TypeScript projects. This allows for more advanced linting rules that require type information.
Consider explicitly specifying the path to your TypeScript configuration file for clarity:
parserOptions: { - project: true, + project: "./tsconfig.json", },This makes the configuration more explicit and can help avoid issues if multiple TypeScript config files are present in the project.
packages/reactor/package.json (1)
1-30: Summary of changes and potential impact.The changes in this file represent a significant shift in the project's development workflow and tooling:
- Transition from Biome to ESLint and Prettier for code quality management.
- Adoption of workspace-level configurations for ESLint, Prettier, and TypeScript.
- Change in the main entry point from a compiled JavaScript file to a TypeScript file.
These changes should improve consistency across the project and align with modern TypeScript development practices. However, they may require adjustments in how the package is built, published, and consumed by other projects.
To ensure a smooth transition:
- Update any CI/CD pipelines to use the new linting and formatting commands.
- Review and update the build process to accommodate the new main entry point.
- Communicate these changes to other team members and update any relevant documentation.
- Consider creating a migration guide for any projects that depend on this package.
.github/workflows/build.yml (2)
41-42: Excellent addition of a linting step!The inclusion of a linting step in the CI process is a great practice. It will help maintain code quality and catch potential issues early in the development cycle.
Consider adding error handling to ensure the workflow fails if linting errors are found:
- name: Lint - run: pnpm lint + run: pnpm lint || exit 1This ensures that the workflow will fail if there are linting errors, preventing problematic code from being merged.
Line range hint
1-44: Overall workflow structure looks great!The workflow is well-structured and follows good practices, including caching for pnpm store and using recent versions of actions.
Consider updating the checkout action to the latest version:
- uses: actions/checkout@v3 + uses: actions/checkout@v4This will ensure you're using the latest features and security updates for the checkout action.
apps/mocksi-lite-next/package.json (1)
42-42: Consider: Optimize lint script for performanceThe updated lint script
"lint": "eslint --fix ."simplifies the command and ensures all files are linted. This is generally a good practice for maintaining code quality across the entire project.However, to optimize performance, especially in larger projects, consider specifying directories or using a more targeted approach. For example:
"lint": "eslint --fix \"src/**/*.{js,jsx,ts,tsx}\""This would lint all JavaScript and TypeScript files in the
srcdirectory and its subdirectories, potentially reducing linting time while still covering all relevant source files.packages/reactor/tests/mutation.test.ts (1)
Line range hint
1-78: Consider adding more test cases for comprehensive coverageThe current test suite provides good coverage for basic mutation handling and reactor detachment scenarios. To further enhance the robustness of the test suite, consider adding the following test cases:
- Test handling of invalid selectors in the modification request.
- Verify behavior when applying multiple modifications in a single request.
- Test edge cases such as empty content or very large content modifications.
- Verify error handling for malformed modification requests.
- Test concurrent modifications to ensure thread safety if applicable.
Here's an example of an additional test case you could add:
it("should handle multiple modifications in a single request", async () => { doc.body.innerHTML = "<h1>train</h1><p>car</p><div>plane</div>"; const request: ModificationRequest = { description: "Change multiple transportation modes", modifications: [ { selector: "h1", action: "replaceAll", content: "/train/subway/", }, { selector: "p", action: "replaceAll", content: "/car/bicycle/", }, { selector: "div", action: "replaceAll", content: "/plane/helicopter/", }, ], }; await reactor.pushModification(request); await waitForChanges(); expect(doc.body.innerHTML).toMatchIgnoringMocksiTags( "<h1>subway</h1><p>bicycle</p><div>helicopter</div>" ); });Adding these test cases will provide more comprehensive coverage and help ensure the robustness of the mutation handling functionality.
apps/mocksi-lite-next/vite.config.ts (2)
54-55: Good use of environment variables for security configurations!The updates to
externally_connectable.matchesandcontent_security_policy.extension_pagesusing theVITE_NEST_APPenvironment variable are well-implemented:
- This approach allows for flexible configuration across different environments.
- It properly sets important security-related settings based on the deployment context.
For improved readability, consider extracting the CSP string into a separate constant:
const cspPolicy = `object-src 'none'; child-src ${env.VITE_NEST_APP}; frame-src ${env.VITE_NEST_APP}; script-src 'self'`; manifest.content_security_policy.extension_pages = cspPolicy;This minor refactoring would make the CSP more visible and easier to modify if needed.
Line range hint
58-93: Well-structured environment-specific configurations!The environment-specific manifest configurations are well-implemented:
- Different settings for various deployment scenarios (QA, development, staging) allow for flexible and appropriate configurations.
- The use of different keys for unpacked builds helps maintain stable extension IDs across environments.
- Adding source map access in the development environment is beneficial for debugging.
- The check to ensure the production build doesn't include a key is an important security measure.
For consistency, consider using a constant for the extension name prefix:
const EXTENSION_NAME_PREFIX = "Mocksi Lite"; // ... case "QA": manifest.name = `[QA Production] ${EXTENSION_NAME_PREFIX}`; // ... case "development": manifest.name = `[Development] ${EXTENSION_NAME_PREFIX}`; // ... case "staging": manifest.name = `[Staging] ${EXTENSION_NAME_PREFIX}`; // ...This would make it easier to update the extension name across all environments if needed in the future.
packages/reactor/modifications/replaceAll.ts (1)
169-170: Consider using optional chaining for conciseness.In the
replaceTextfunction, you can simplify the code using optional chaining. This change would make the code more concise and easier to read.Consider updating the code as follows:
- let split = node.nodeValue?.split(patternRegexp) || []; - split = split.map((part, index) => { + let split = node.nodeValue?.split(patternRegexp)?.map((part, index) => {This change combines the split and map operations, reducing the need for a separate variable assignment.
packages/reactor/tests/main.test.ts (3)
31-85: LGTM: Comprehensive test for regex-based text replacement.The test case thoroughly verifies the regex-based text replacement functionality, including checks for both modified and unmodified parts of the document.
Consider adding a test for case sensitivity to ensure the regex flag
iis working as expected. For example:const capitalizedText = document.evaluate( "//html/body/div/h2/text()[1]", doc, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null ).singleNodeValue; expect(capitalizedText?.textContent).toBe("About the brain");
121-136: LGTM with suggestion: Test case for toast notification creation.The test case verifies the basic functionality of creating a toast notification.
Consider enhancing the test to verify the structure and styling of the toast notification. For example:
expect(result).toMatch(/<div[^>]*class="[^"]*toast[^"]*"[^>]*>[\s\S]*This is a toast message[\s\S]*<\/div>/);This regex-based assertion would ensure that the toast message is wrapped in a div with the appropriate class.
Line range hint
138-163: LGTM with suggestion: Test case for adding a DaisyUI component.The test case verifies the basic functionality of adding a DaisyUI card component.
Consider enhancing the test to verify the complete structure of the added DaisyUI component. For example:
expect(result).toMatch(/<div[^>]*class="[^"]*card w-96 bg-base-100 shadow-xl[^"]*"[^>]*>[\s\S]*<div[^>]*class="[^"]*card-body[^"]*"[^>]*>[\s\S]*<h2[^>]*class="[^"]*card-title[^"]*"[^>]*>[\s\S]*<p>[\s\S]*<div[^>]*class="[^"]*card-actions justify-end[^"]*"[^>]*>[\s\S]*<button[^>]*class="[^"]*btn btn-primary[^"]*"[^>]*>/);This regex-based assertion would ensure that all expected elements and classes of the DaisyUI card component are present.
packages/reactor/tests/modifications.test.ts (1)
472-503: Good addition of createToast and generateModifications testsThe separate test cases for
createToastandgenerateModificationsare valuable additions to the test suite. They ensure that these functions are tested independently and cover important scenarios like handling empty selectors.For the
createToasttest, consider using a shorter duration to speed up the test execution. For example:- await new Promise((resolve) => setTimeout(resolve, 3100)); // Adjusted slightly above the 3000ms to account for any delays + await new Promise((resolve) => setTimeout(resolve, 110)); // Use a shorter duration for faster testsThis change would make the test run faster while still verifying the toast's creation and removal.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (53)
- .eslintrc.js (1 hunks)
- .github/workflows/build.yml (1 hunks)
- .github/workflows/ci.yml (0 hunks)
- apps/mocksi-lite-next/.eslintrc (0 hunks)
- apps/mocksi-lite-next/.eslintrc.js (1 hunks)
- apps/mocksi-lite-next/package.json (2 hunks)
- apps/mocksi-lite-next/tsconfig.json (1 hunks)
- apps/mocksi-lite-next/vite.config.ts (2 hunks)
- package.json (1 hunks)
- packages/config-eslint/README.md (1 hunks)
- packages/config-eslint/base.js (1 hunks)
- packages/config-eslint/package.json (1 hunks)
- packages/config-eslint/web.js (1 hunks)
- packages/config-prettier/index.js (1 hunks)
- packages/config-prettier/package.json (1 hunks)
- packages/config-typescript/base.json (1 hunks)
- packages/config-typescript/package.json (1 hunks)
- packages/config-typescript/web-app.json (1 hunks)
- packages/config-typescript/web-library.json (1 hunks)
- packages/eslint-config/library.js (0 hunks)
- packages/eslint-config/package.json (0 hunks)
- packages/eslint-config/react-internal.js (0 hunks)
- packages/reactor/.eslintrc.js (1 hunks)
- packages/reactor/biome.json (0 hunks)
- packages/reactor/index.ts (1 hunks)
- packages/reactor/interfaces.ts (1 hunks)
- packages/reactor/main.ts (1 hunks)
- packages/reactor/modifications.ts (1 hunks)
- packages/reactor/modifications/adjacentHTML.ts (1 hunks)
- packages/reactor/modifications/highlight.ts (1 hunks)
- packages/reactor/modifications/noop.ts (1 hunks)
- packages/reactor/modifications/remove.ts (1 hunks)
- packages/reactor/modifications/replace.ts (1 hunks)
- packages/reactor/modifications/replaceAll.ts (1 hunks)
- packages/reactor/modifications/swapImage.ts (1 hunks)
- packages/reactor/modifications/toast.ts (1 hunks)
- packages/reactor/mutationObserver.ts (1 hunks)
- packages/reactor/package.json (1 hunks)
- packages/reactor/reactor.ts (2 hunks)
- packages/reactor/tests/index.test.ts (1 hunks)
- packages/reactor/tests/main.test.ts (2 hunks)
- packages/reactor/tests/modifications.test.ts (1 hunks)
- packages/reactor/tests/mutation.test.ts (1 hunks)
- packages/reactor/tests/test.utils.ts (1 hunks)
- packages/reactor/tsconfig.json (1 hunks)
- packages/reactor/utils.ts (1 hunks)
- packages/reactor/vitest.config.ts (1 hunks)
- packages/typescript-config/base.json (0 hunks)
- packages/typescript-config/nextjs.json (0 hunks)
- packages/typescript-config/package.json (0 hunks)
- packages/typescript-config/react-library.json (0 hunks)
- tsconfig.json (1 hunks)
- turbo.json (1 hunks)
Files not reviewed due to no reviewable changes (10)
- .github/workflows/ci.yml
- apps/mocksi-lite-next/.eslintrc
- packages/eslint-config/library.js
- packages/eslint-config/package.json
- packages/eslint-config/react-internal.js
- packages/reactor/biome.json
- packages/typescript-config/base.json
- packages/typescript-config/nextjs.json
- packages/typescript-config/package.json
- packages/typescript-config/react-library.json
Files skipped from review due to trivial changes (16)
- packages/config-eslint/README.md
- packages/reactor/index.ts
- packages/reactor/interfaces.ts
- packages/reactor/main.ts
- packages/reactor/modifications.ts
- packages/reactor/modifications/highlight.ts
- packages/reactor/modifications/noop.ts
- packages/reactor/modifications/remove.ts
- packages/reactor/modifications/replace.ts
- packages/reactor/modifications/swapImage.ts
- packages/reactor/modifications/toast.ts
- packages/reactor/mutationObserver.ts
- packages/reactor/reactor.ts
- packages/reactor/tests/index.test.ts
- packages/reactor/tests/test.utils.ts
- packages/reactor/vitest.config.ts
Additional context used
Biome
packages/config-eslint/web.js
[error] 12-15: This property value named globals is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named globals(lint/suspicious/noDuplicateObjectKeys)
Additional comments not posted (73)
.eslintrc.js (1)
1-5: Significant simplification of ESLint configurationThe ESLint configuration has been substantially simplified, which aligns with the PR objective of updating eslint configs. Here are the key changes and their implications:
The
extendsproperty now uses"@repo/config-eslint/base.js"instead of the previous library configuration. This change centralizes the ESLint rules, potentially making it easier to maintain consistency across the project.TypeScript-specific configurations (parser, plugins, rules) have been removed. This suggests that TypeScript linting might now be handled differently, possibly through the new base configuration or another mechanism.
The
ignorePatternsandrootproperties remain unchanged, maintaining the existing file exclusion logic and ensuring this configuration acts as the root.These changes should simplify maintenance, but it's crucial to ensure that all necessary linting rules, especially for TypeScript, are still being applied effectively.
To confirm the existence and contents of the new base configuration, please run the following script:
This script will help us verify:
- The existence of the new base configuration file.
- The contents of the base configuration, allowing us to check if it includes necessary rules, especially for TypeScript.
- Any other TypeScript-related ESLint configurations in the project that might complement this simplified root configuration.
apps/mocksi-lite-next/tsconfig.json (4)
1-9: Overall assessment of tsconfig.json changesThe modifications to this
tsconfig.jsonfile represent a significant simplification and standardization of the TypeScript configuration. By extending a base configuration file, the project can maintain consistency across different parts of the codebase. However, this change comes with a few considerations:
The effectiveness of this new configuration depends on the contents of the base file (
@repo/config-typescript/web-app.json). Ensure that all necessary compiler options are properly set in the base configuration.The addition of the "ignore" section for the "public" directory is typical, but verify that this aligns with your project structure and build process.
The changes in the "include" section, particularly the removal of the "utils" directory, may impact the project structure. Ensure that all necessary TypeScript files are still being included in the compilation process.
These changes appear to be part of a larger effort to streamline and standardize the project configuration. While this is generally a positive direction, it's crucial to verify that no essential settings or files have been inadvertently excluded in this process.
To ensure the changes don't negatively impact the project, please review the verification scripts in the previous comments and their results.
3-5: Confirm the necessity of ignoring the "public" directory.The addition of an "ignore" section to exclude the "public" directory is typical for static assets. However, it's important to ensure that this aligns with your project structure and build process.
Let's verify the existence and content of the "public" directory:
#!/bin/bash # Description: Verify the existence and content of the "public" directory # Test 1: Check if the directory exists if [ -d "apps/mocksi-lite-next/public" ]; then echo "Public directory exists." # Test 2: List the contents of the directory ls -la "apps/mocksi-lite-next/public" else echo "Warning: Public directory not found at apps/mocksi-lite-next/public" fi
6-9: Review the changes in the "include" section.The "include" section has been modified to retain "src" and "vite.config.ts", but the "utils" directory has been removed. This change might impact the project structure.
Let's verify the existence of the included and potentially removed directories:
#!/bin/bash # Description: Verify the existence of included and potentially removed directories # Test 1: Check if the "src" directory exists if [ -d "apps/mocksi-lite-next/src" ]; then echo "src directory exists." else echo "Warning: src directory not found at apps/mocksi-lite-next/src" fi # Test 2: Check if the "vite.config.ts" file exists if [ -f "apps/mocksi-lite-next/vite.config.ts" ]; then echo "vite.config.ts file exists." else echo "Warning: vite.config.ts file not found at apps/mocksi-lite-next/vite.config.ts" fi # Test 3: Check if the "utils" directory still exists if [ -d "apps/mocksi-lite-next/utils" ]; then echo "Warning: utils directory still exists at apps/mocksi-lite-next/utils" # List contents of the utils directory ls -la "apps/mocksi-lite-next/utils" else echo "utils directory has been removed as expected." fiIf the "utils" directory still exists and contains important files, we may need to address this in our project structure or update the "include" section accordingly.
2-2: Verify the existence and appropriateness of the base configuration.Extending a base configuration file is a good practice for maintaining consistency across projects. However, we need to ensure that the referenced file exists and contains appropriate settings for this project.
Let's verify the existence and content of the base configuration:
Verification successful
Continuing verification by checking the existence and content of the base TypeScript configuration file.
Continuing verification by checking the existence and content of the base TypeScript configuration file
packages/config-typescript/base.json.
Base TypeScript configuration is correctly extended and properly set up.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the base TypeScript configuration # Test 1: Check if the file exists if [ -f "packages/config-typescript/web-app.json" ]; then echo "Base configuration file exists." # Test 2: Display the content of the file cat "packages/config-typescript/web-app.json" else echo "Error: Base configuration file not found at packages/config-typescript/web-app.json" fiLength of output: 348
Script:
#!/bin/bash # Description: Verify the existence and content of the base TypeScript configuration # Test 1: Check if the base configuration file exists if [ -f "packages/config-typescript/web-app.json" ]; then echo "Base configuration file exists." # Test 2: Display the content of the base configuration file cat "packages/config-typescript/web-app.json" else echo "Error: Base configuration file not found at packages/config-typescript/web-app.json" fiLength of output: 402
Script:
#!/bin/bash # Description: Verify the existence and content of the base TypeScript base.json configuration # Test 1: Check if the base.json configuration file exists if [ -f "packages/config-typescript/base.json" ]; then echo "Base configuration file (base.json) exists." # Test 2: Display the content of the base.json configuration file cat "packages/config-typescript/base.json" else echo "Error: Base configuration file not found at packages/config-typescript/base.json" fiLength of output: 836
packages/reactor/.eslintrc.js (1)
1-6: LGTM with a minor concernThe overall structure of the ESLint configuration looks good. It extends a base configuration and is set as the root configuration, which are both good practices. However, there's a minor concern:
The
typesproperty in the configuration object is not a standard ESLint option. This might be a custom property used by your setup, or it could be a typo.To verify the intended use of the
typesproperty, please run the following script:If the
typesproperty is not found in other configurations or documentation, consider removing it or replacing it with the correct property name if it was a typo.packages/config-prettier/index.js (3)
1-1: LGTM: Proper type annotation for Prettier configThe JSDoc type annotation correctly references the Prettier Config type, which is a good practice for ensuring type safety in JavaScript files using TypeScript types.
7-7: LGTM: Semicolon usage enforcedThe configuration sets
semi: true, which enforces the use of semicolons at the end of statements. This is generally considered a good practice for JavaScript code as it helps prevent potential issues with Automatic Semicolon Insertion (ASI).
3-6: LGTM: Appropriate Prettier plugins configuredThe configuration includes two relevant Prettier plugins:
prettier-plugin-organize-attributes: Helps organize HTML attributes.prettier-plugin-tailwindcss: Useful for projects using Tailwind CSS.These plugins will enhance the formatting capabilities of Prettier for your project.
Please ensure that these plugins are installed as dependencies. You can verify this by running the following command:
If the plugins are not found, please install them using:
npm install --save-dev prettier-plugin-organize-attributes prettier-plugin-tailwindcsspackages/reactor/tsconfig.json (3)
8-14: LGTM! Verify project structure alignment.The updated
includearray provides a more comprehensive coverage of the project's TypeScript files. The removal of theexcludeproperty simplifies the configuration.To ensure the
includearray aligns with the actual project structure, run:#!/bin/bash # Description: Verify the project structure aligns with the include array # Test: List directories and TypeScript files in the package echo "Directories:" fd -t d . packages/reactor -d 1 echo "TypeScript files:" fd -e ts . packages/reactor -d 1
7-7: LGTM! Verify the new extended configuration file.The update to a more specific configuration (
web-library.json) is a good change. It likely provides more tailored settings for web library development.To ensure the new extended configuration file exists and contains the expected settings, run:
#!/bin/bash # Description: Verify the existence and content of the new extended configuration file # Test 1: Check if the file exists fd -t f web-library.json packages/config-typescript # Test 2: If the file exists, display its content if [ $? -eq 0 ]; then echo "File content:" cat $(fd -t f web-library.json packages/config-typescript) else echo "Error: Extended configuration file not found" fi
3-5: LGTM! Verify extended configuration for removed options.The simplification of
compilerOptionsto only includetypesis good. It reduces duplication and improves maintainability by relying on the extended configuration.To ensure all necessary compiler options are present in the extended configuration, run:
Verification successful
Verified! The
excludeproperty is correctly defined inbase.json, ensuring that exclusions remain effective even after its removal fromtsconfig.json.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compiler options in the extended configuration file # Test: Check the content of the extended configuration file cat $(fd -t f web-library.json packages/config-typescript)Length of output: 1034
Script:
#!/bin/bash # Description: Verify compiler options in the extended configuration file # Path to the extended configuration file EXT_CONFIG="packages/config-typescript/web-library.json" # Check if the extended configuration file exists if [ -f "$EXT_CONFIG" ]; then echo "Compiler Options in $EXT_CONFIG:" jq '.compilerOptions' "$EXT_CONFIG" else echo "Error: '$EXT_CONFIG' does not exist." exit 1 fiLength of output: 391
Script:
#!/bin/bash # Description: Verify the presence of 'exclude' in the base TypeScript configuration file # Path to the base configuration file BASE_CONFIG="packages/config-typescript/base.json" # Check if the base configuration file exists if [ -f "$BASE_CONFIG" ]; then echo "Content of $BASE_CONFIG:" cat "$BASE_CONFIG" echo echo "Checking for 'exclude' property in compilerOptions:" jq '.compilerOptions.exclude' "$BASE_CONFIG" || echo "'exclude' property not found." else echo "Error: '$BASE_CONFIG' does not exist." exit 1 fiLength of output: 1096
packages/config-typescript/web-app.json (4)
6-6: Display name is appropriate.The display name "Web App" clearly indicates the purpose of this configuration file.
8-8: Library inclusions are appropriate for a web application.The
libarray includes "es2022", "dom", and "dom.iterable", which are suitable choices for a modern web application:
- "es2022" provides access to the latest ECMAScript features.
- "dom" and "dom.iterable" are necessary for working with web browser APIs.
These inclusions align well with the configuration's purpose as a web application setup.
1-9: Overall, this is a well-configured TypeScript setup for a web application.The configuration file is concise, well-structured, and includes all necessary settings for a modern React-based web application. It extends a base configuration, specifies appropriate compiler options, and includes relevant library types. The minor suggestions provided earlier (sorting the types array and verifying the base configuration) would further enhance its quality, but the current state is already solid and ready for use.
7-7: Good use of base configuration, verify its contents.Extending a base configuration is a good practice for maintaining consistent settings.
Please ensure that the
./base.jsonfile exists and contains appropriate base configurations. Run the following script to verify:Verification successful
Base configuration verified successfully.
The
./base.jsonfile exists and contains the appropriate TypeScript compiler options for consistent project settings.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the base.json file # Test 1: Check if base.json exists if [ -f "packages/config-typescript/base.json" ]; then echo "base.json exists." # Test 2: Display the content of base.json echo "Content of base.json:" cat "packages/config-typescript/base.json" else echo "Error: base.json does not exist in the expected location." fiLength of output: 837
packages/config-typescript/web-library.json (4)
2-4: LGTM: Appropriate type definitions for a web library.The
compilerOptions.typesarray includes relevant type definitions for a web library project, covering DOM manipulation (jsdom), browser APIs (chrome), UUID generation (uuid), and testing (vitest). This configuration will provide good type support for web development and testing scenarios.
5-6: Good configuration for build performance and developer experience.Enabling
compositeanddeclarationMapoptions is beneficial for this web library:
composite: trueenables TypeScript project references, which can significantly improve build times for large projects.declarationMap: truegenerates source map files for declaration files, enhancing debugging and navigation in IDEs.These settings will improve both build performance and developer experience, especially as the project grows.
9-9: LGTM: Appropriate lib configuration for a web library.The
libarray is well-configured for a web library project:
- "es2022" includes modern JavaScript features, ensuring access to the latest language capabilities.
- "dom" and "dom.iterable" provide types for browser APIs, which is essential for web development.
This configuration aligns perfectly with the project's purpose as a web library, providing a good balance between modern language features and browser compatibility.
7-8: Verify the purpose of thedisplayproperty and approve base configuration extension.The configuration correctly extends from a base configuration file, which is a good practice for maintaining consistent settings across different parts of the project.
However, the
displayproperty is not a standard TypeScript compiler option. Please verify its purpose and ensure it's being used correctly by your build tools or documentation generators.To check if the
displayproperty is used elsewhere in the project, run the following command:turbo.json (3)
4-4: LGTM: Updated to use "tasks" instead of "pipeline"This change aligns with the current best practices for Turborepo configuration. It's a good update that keeps the project up-to-date with the latest Turborepo standards.
11-13: LGTM: Appropriate configuration for the "dev" taskThe changes to the "dev" task are well-considered:
- Setting
"cache": falseensures that the development task is always run fresh, which is crucial for development environments.- Adding
"persistent": truecorrectly indicates that this is a long-running task, typical for development servers.These modifications align with best practices for configuring development tasks in Turborepo.
1-21: Overall, excellent updates to the Turborepo configurationThe changes made to
turbo.jsonare well-thought-out and align with current best practices:
- Updating to the "tasks" format from "pipeline" keeps the project up-to-date with the latest Turborepo standards.
- The "dev" task is correctly configured for development environments.
- The addition of the "format" task and restructuring of the "lint" task improve the project's structure and task execution flow.
These modifications should enhance the efficiency and maintainability of your build and development processes in the monorepo setup.
packages/config-prettier/package.json (2)
7-9: Exports configuration looks goodThe exports field is correctly configured, specifying "index.js" as the main entry point. This aligns well with modern JavaScript module systems and provides a clear interface for package consumers.
10-12: Files configuration is appropriateThe files array correctly specifies "index.js" as the only file to be included in the package. This is a good practice to ensure that only necessary files are published.
Let's verify the existence of the index.js file:
Verification successful
Files configuration verified
The existence of
index.jsin thepackages/config-prettierdirectory has been confirmed. Thefilesarray correctly specifies this file, ensuring only the necessary files are included in the package.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of index.js in the package directory if [ -f "packages/config-prettier/index.js" ]; then echo "index.js exists in the package directory." else echo "Warning: index.js is missing from the package directory." fiLength of output: 139
packages/config-eslint/package.json (1)
1-18: Good initiative to centralize ESLint configurations.Creating a separate package for ESLint configurations is a good approach to maintain consistency across the project. This setup will make it easier to update and manage ESLint rules in the future, especially for a monorepo structure.
package.json (1)
14-16: LGTM! Structural changes look good.The reorganization of the project metadata fields (
name,packageManager, andprivate) is a minor structural change that doesn't affect functionality. It's a matter of preference and potentially aligns better with common package.json structures.packages/config-typescript/package.json (1)
16-18: LGTM! Please clarify the package's private/public status.The package metadata looks good:
- The scoped package name "@repo/config-typescript" follows best practices for organization.
- Starting with version 1.0.0 is appropriate for a new package.
- The MIT license is a good choice for open-source projects.
However, there's a potential inconsistency:
- The package is marked as
"private": true, but there's apublishConfigsection that sets"access": "public".Could you clarify if this package is intended to be private or public? If it's meant to be public, consider changing
"private": trueto"private": false. If it's meant to be private, you may want to remove thepublishConfigsection.To help verify the intended status, could you run the following command and share the output?
This will help us understand if this is a common pattern in your monorepo or if it's unique to this package.
Also applies to: 22-22
packages/config-eslint/web.js (5)
2-5: LGTM: Environment settings are appropriate.The environment settings correctly specify a browser environment with ES6 support, which is suitable for a modern web application.
6-11: LGTM: Comprehensive set of extended configurations.The configuration extends from a local base configuration and recommended settings for ESLint, React, and TypeScript. This provides a robust foundation for linting rules, ensuring code quality and consistency.
19-19: LGTM: Appropriate ignore patterns.The ignore patterns exclude the "dist" directory (which typically contains built files) and "nodemon.js". These are reasonable files to exclude from linting.
20-27: LGTM: Parser and parser options are well-configured.The parser is correctly set to "@typescript-eslint/parser" for TypeScript support. The parser options are appropriately configured for a React project using TypeScript and modern JavaScript features, including JSX support and the latest ECMAScript version.
29-31: LGTM: Appropriate rule configuration.Disabling the "react/react-in-jsx-scope" rule is common in modern React projects, especially when using React 17+ or Next.js where importing React in every file is not necessary. This configuration aligns with current best practices.
packages/config-typescript/base.json (2)
1-7: LGTM: Good initial TypeScript configuration setup.The configuration starts with a solid foundation:
- Using a JSON schema enhances validation and editor support.
- Allowing JavaScript files facilitates gradual migration to TypeScript.
- Enabling synthetic default imports and ES2022 module compatibility aligns with modern JavaScript practices.
These settings provide a good balance between flexibility and modern development practices.
23-24: Consider the implications of excluding the "test" directory.The exclusions for "node_modules" and "dist" are appropriate:
- Excluding "node_modules" is standard practice to avoid processing external dependencies.
- Excluding "dist" prevents reprocessing of already compiled files.
However, excluding the "test" directory might prevent type-checking of test files. This could lead to type-related issues in tests going unnoticed.
To ensure this won't cause issues, let's check if there are separate TypeScript configurations for tests:
If no test-specific configurations are found, consider removing "test" from the exclude list to ensure type-checking for test files.
packages/reactor/utils.ts (3)
1-1: LGTM: Import statement simplifiedThe import statement has been simplified, improving readability without changing functionality.
3-6: LGTM: Improved formatting inparseRequestfunctionThe indentation adjustments in the
parseRequestfunction enhance code readability while maintaining the original functionality.
13-15: LGTM: Improved formatting ingenerateRandomStringfunctionThe indentation adjustments in the
generateRandomStringfunction enhance code readability while maintaining the original functionality.packages/config-eslint/base.js (4)
12-16: LGTM! Ignoring build output is a good practice.Adding "dist/" to the ignore patterns is a good practice. It prevents ESLint from linting build output, which is unnecessary and can potentially slow down the linting process.
24-26: LGTM! Proper configuration for unused variables in TypeScript.The configuration for unused variables is correct for TypeScript projects:
- Using "@typescript-eslint/no-unused-vars" instead of "no-unused-vars" is appropriate.
- Ignoring arguments starting with underscore is a common and useful pattern.
- Disabling "no-unused-vars" prevents conflicts with the TypeScript-specific rule.
This setup will help catch unused variables while allowing for intentionally unused parameters (prefixed with underscore).
23-23: LGTM! Consider the implications of "only-warn" plugin.The addition of "@typescript-eslint/eslint-plugin" is correct and necessary when using the TypeScript parser. It provides TypeScript-specific linting rules.
However, please consider the implications of using the "only-warn" plugin. While it can be useful during development, it might mask critical issues in production. Run the following script to check if this plugin is used in other configuration files:
#!/bin/bash # Description: Check for "only-warn" plugin usage in other ESLint configs # Test: Search for "only-warn" in all JS files in the packages directory rg --type js 'only-warn' packages/If it's only used in this base configuration, consider removing it or conditionally including it based on the environment.
9-11: LGTM! Verify the impact of removed style guide.The addition of "eslint-config-prettier" before "prettier" is a good practice. It ensures that ESLint rules that might conflict with Prettier are turned off.
However, please verify the impact of removing "@vercel/style-guide/eslint/next". Run the following script to check for any custom rules or configurations that might have been lost:
If any custom rules are found, consider adding them manually to maintain consistent code style.
Verification successful
Verified: No custom ESLint rules from "@vercel/style-guide/eslint/next" were found.
Removing "@vercel/style-guide/eslint/next" does not impact any custom ESLint configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for custom ESLint rules from @vercel/style-guide that might need to be manually added # Test: Search for custom ESLint rules in the old configuration rg --type js 'rules:' packages/config-eslint/base.jsLength of output: 238
Script:
#!/bin/bash # Description: Corrected script to check for custom ESLint rules from @vercel/style-guide that might need to be manually added # Test: Search for custom ESLint rules in the old configuration rg --type js 'rules:' packages/config-eslint/base.jsLength of output: 63
packages/reactor/package.json (3)
16-16: Clarify the impact of changing the main entry point.The main entry point has been changed from "dist/index.js" to "index.ts". This shift towards using TypeScript directly without pre-compilation may impact how the package is consumed or published.
Please clarify:
- How will this affect the package's build process?
- Are there any changes needed in how other projects import this package?
- Does this change align with the project's publishing strategy?
To verify the impact, you can run:
#!/bin/bash # Description: Check for potential issues with the new main entry point. # Test: Verify if index.ts exists ls index.ts # Test: Check if there are any import statements in the project using the old path rg --type typescript 'from ["\']@repo/reactor/dist' # Test: Examine the build script to ensure it's compatible with the new entry point cat package.json | jq .scripts.build
20-27: Approve script changes and suggest verification.The changes to the "lint" and "format" scripts align with the shift from Biome to ESLint and Prettier. This is consistent with the PR objectives and the changes in dependencies.
To ensure the new scripts work as expected, please run:
#!/bin/bash # Description: Verify the functionality of the updated lint and format scripts. # Test: Run the lint script pnpm run lint # Test: Run the format script pnpm run format # Test: Check if there are any remaining references to Biome in the scripts cat package.json | jq .scripts | grep -i biome
7-15: Approve dependency changes and suggest verification.The changes in dependencies align with the PR objective of removing Biome and updating ESLint. The shift from Biome to ESLint and Prettier, along with the addition of centralized TypeScript configuration, should improve code consistency across the project.
To ensure the new dependencies are correctly set up, please run the following script:
packages/reactor/modifications/adjacentHTML.ts (5)
4-6: LGTM: Class properties are well-defined.The class properties are correctly typed and appropriately declared for the
AdjacentHTMLModificationclass.
8-20: LGTM: Constructor is well-implemented.The constructor is correctly implemented with appropriate parameter handling and property initialization. The use of
superandthis.addModifiedElementsuggests good integration with the parent class.
22-28: LGTM:applymethod is correctly implemented.The
applymethod correctly handles the modification of elements, storing their original state before applying changes. The use ofinsertAdjacentHTMLis appropriate for the class's purpose.
30-37: LGTM:unapplymethod is correctly implemented.The
unapplymethod effectively restores the original state of modified elements. It properly checks for the existence of the old state before applying it, ensuring safe operation.
1-37: LGTM: Formatting improvements enhance readability.The changes in this file are primarily formatting-related, improving the overall readability of the code without altering its functionality. The class structure, logic, and behavior remain intact and well-implemented.
apps/mocksi-lite-next/package.json (3)
18-20: Approve: Centralized configuration using workspace dependenciesThe addition of
@repo/config-eslint,@repo/config-prettier, and@repo/config-typescriptas workspace dependencies is a positive change. This approach centralizes configuration management, promoting consistency across the project and simplifying maintenance.These changes align well with the PR objective of updating ESLint and TypeScript configs. The removal of individual type definitions and ESLint plugins (as noted in the AI summary) further supports this streamlined approach.
33-33: Approve: Centralized Prettier configurationThe addition of
"prettier": "@repo/config-prettier"is a positive change. It ensures consistent code formatting across the project by using a shared Prettier configuration.This change complements the earlier additions of workspace dependencies and supports the overall goal of centralizing and standardizing project configurations.
32-32: Verify: Package name change impactThe package name has been simplified from "vite-web-extension" to "web-extension". This change is appropriate and aligns with the project's nature.
However, it's important to ensure that this change doesn't break any existing references to the package name in other parts of the project. Please run the following script to check for any occurrences of the old package name:
Verification successful
Package Name Change Verified
No occurrences of "vite-web-extension" were found in the codebase. The package name change does not break any existing references.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for occurrences of the old package name rg "vite-web-extension" --type json --type javascript --type typescriptLength of output: 106
Script:
#!/bin/bash # Search for occurrences of the old package name with corrected file types rg "vite-web-extension" --type json --type js --type tsLength of output: 55
packages/reactor/tests/mutation.test.ts (1)
9-21: LGTM: Test setup is well-structuredThe test setup is well-organized, using
beforeEachto ensure a clean state for each test. The initialization of theDocumentandReactorinstances, along with the attachment of mock highlight functions, provides a solid foundation for the subsequent tests.apps/mocksi-lite-next/vite.config.ts (3)
5-18: Excellent improvements to type safety and code organization!The introduction of the
baseManifestimport and theManifestConfiginterface are great additions:
- Importing
baseManifestfrom a separate JSON file improves maintainability by separating concerns.- The
ManifestConfiginterface enhances type safety, which will help catch potential errors early in the development process and improve overall code quality.These changes align well with TypeScript best practices and will contribute to a more robust configuration setup.
51-52: Good use of type-safe manifest initialization!The initialization of
manifestas aManifestConfigtype usingbaseManifestis a solid approach:
- It ensures type safety for the manifest object throughout the configuration.
- It allows for environment-specific customizations while maintaining the base configuration.
- This setup provides a clear starting point for any further modifications to the manifest.
This change enhances the reliability and flexibility of the manifest configuration process.
Line range hint
1-138: Overall, excellent improvements to the Vite configuration!This review has found that the changes to
vite.config.tssignificantly enhance the project's type safety, maintainability, and configuration flexibility. Key improvements include:
- Introduction of the
ManifestConfiginterface for better type checking.- Use of a separate
baseManifestfor improved code organization.- Environment-specific configurations that allow for flexible deployment settings.
- Proper use of environment variables for security-related configurations.
The minor suggestions provided (extracting the CSP string and using a constant for the extension name prefix) are aimed at further improving code readability and maintainability.
These changes align well with TypeScript best practices and contribute to a more robust and maintainable configuration setup for the Mocksi Lite extension.
packages/reactor/modifications/replaceAll.ts (4)
86-95: LGTM! Proper use of theoverridekeyword.The
modifiedElementRemovedmethod is correctly implemented with theoverridekeyword, indicating that it overrides a method from the parent class. The logic for removing changes associated with the removed element and determining if the modification can be safely removed looks good.
98-103: LGTM! Improved property ordering inTreeChangetype.The properties of the
TreeChangetype have been reordered, which enhances readability and consistency. This change aligns with best practices for maintaining clean and organized code structures.
105-108: LGTM! Minor type ordering change inwalkTreefunction signature.The
walkTreefunction signature has been updated to usenull | TreeChangeinstead ofTreeChange | null. This change is minor and doesn't affect functionality, but it maintains consistency with TypeScript conventions.
220-285: LGTM! Well-implemented utility functions.The
replaceFirstLetterCaseAndPluralandtoRegExpPatternfunctions are implemented correctly. They effectively handle case sensitivity, plural forms, and pattern parsing, which are crucial for the text replacement functionality of theReplaceAllModificationclass.packages/reactor/tests/main.test.ts (7)
14-29: LGTM: Test case for text content replacement.The test case is well-structured and correctly verifies the text replacement functionality of the
modifyHtmlfunction.
87-102: LGTM: Test case for image source swapping.The test case effectively verifies the image source swapping functionality of the
modifyHtmlfunction.
104-119: LGTM: Test case for element highlighting.The test case properly verifies the element highlighting functionality of the
modifyHtmlfunction.
165-208: LGTM: Comprehensive test for multiple modifications.The test case effectively verifies multiple types of modifications (text replacement, image swapping, and toast notification) using the
modifyDomfunction. The assertions are thorough and cover all expected changes.
210-256: LGTM: Test case for unapplying multiple modifications.The test case effectively verifies the functionality of unapplying multiple modifications, ensuring that the original content is restored correctly.
258-296: LGTM: Test case for unapplying removal of multiple elements.The test case thoroughly verifies the functionality of unapplying removal modifications for multiple elements, ensuring that all removed elements are correctly restored.
298-332: LGTM: Error handling test cases.Both test cases effectively verify the error handling capabilities of the
modifyHtmlfunction:
- "should handle missing elements gracefully" ensures that attempts to modify non-existent elements do not affect the original content.
- "should ignore invalid selectors" confirms that the function gracefully handles invalid CSS selectors without modifying the original content.
These tests contribute to the robustness of the
modifyHtmlfunction by covering important edge cases.packages/reactor/tests/modifications.test.ts (5)
13-15: Good addition ofbeforeEachblockThe introduction of a
beforeEachblock to create a fresh HTML document for each test is a good practice. This ensures that each test runs in isolation, preventing potential inter-test dependencies and improving test reliability.
18-120: Improved test coverage and structure for replace operationsThe restructuring of "replace" and "replaceAll" test cases is well done. The separation of application and unapplication tests, along with the addition of new cases for preserving capitals and plurals, significantly improves the test coverage. This ensures that the replacement functionality is thoroughly tested for various scenarios.
123-219: Well-maintained complex test scenariosThe retention and reformatting of test cases for complex HTML structures and Wikipedia content is commendable. These tests are crucial for ensuring the robustness of the modification functions when dealing with real-world, complex DOM structures. The improved formatting enhances readability without altering the test logic.
221-469: Comprehensive coverage of modification actionsThe test suite now provides excellent coverage for various modification actions including append, prepend, remove, swapImage, highlight, and addComponent. The consistent structure of having separate tests for application and unapplication of each action enhances maintainability and ensures that both directions of the modifications are verified.
The addition of the toast test case is particularly noteworthy as it covers an important user feedback feature. This comprehensive approach to testing will help maintain the reliability of the modification system as it evolves.
Line range hint
1-503: Excellent improvements to the test suiteThe changes made to this test file represent a significant enhancement to the quality and coverage of the test suite for the modification system. Key improvements include:
- Consistent testing of both application and unapplication for each modification type.
- Addition of new test cases covering edge scenarios and complex HTML structures.
- Improved organization and readability of the test suite.
- Coverage of all supported modification actions.
- Independent testing of utility functions like
createToastandgenerateModifications.These improvements will greatly contribute to the maintainability and reliability of the codebase as it evolves. Great job on this refactoring effort!
| @@ -1,3 +1,3 @@ | |||
| { | |||
| "extends": "@repo/typescript-config/base.json" | |||
| } | |||
| "extends": "@repo/config-typescript/base.json" | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Invalid TypeScript configuration path
The new "extends" path @repo/config-typescript/base.json does not exist. Please update it to the correct path ./packages/config-typescript/base.json.
- File: tsconfig.json
- Line: 2
- Change:
"extends": "./packages/config-typescript/base.json"
Analysis chain
LGTM! Verify the new TypeScript configuration.
The update to the "extends" property aligns with the PR objectives of updating TypeScript configurations. This change suggests a reorganization of the TypeScript configuration files, which could affect the entire project's TypeScript compilation process.
To ensure the new configuration is correctly set up, please run the following verification script:
This script will verify the existence of the new configuration file and check its basic structure. Please ensure that the new configuration file is properly set up to maintain consistent TypeScript compilation across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of the new TypeScript configuration file.
# Test 1: Check if the new configuration file exists
if fd -t f "base.json" -p "@repo/config-typescript"; then
echo "New TypeScript configuration file found."
else
echo "Error: New TypeScript configuration file not found."
exit 1
fi
# Test 2: Verify the basic structure of the new configuration file
fd -t f "base.json" -p "@repo/config-typescript" -x cat {} | jq -e 'has("compilerOptions")' > /dev/null
if [ $? -eq 0 ]; then
echo "New TypeScript configuration file has a valid basic structure."
else
echo "Warning: New TypeScript configuration file may not have a valid basic structure."
fi
Length of output: 279
Script:
#!/bin/bash
# Description: Search for 'config-typescript' directories and verify the existence of 'base.json'.
# Find all directories named 'config-typescript'
config_dirs=$(fd -t d "config-typescript" .)
if [ -z "$config_dirs" ]; then
echo "Error: 'config-typescript' directory not found in the repository."
exit 1
fi
# Initialize a flag to track if 'base.json' is found
found=false
# Iterate through each found directory to check for 'base.json'
for dir in $config_dirs; do
if [ -f "$dir/base.json" ]; then
echo "Found 'base.json' in $dir."
found=true
else
echo "Warning: 'base.json' not found in $dir."
fi
done
# Final check to ensure at least one 'base.json' was found
if ! $found; then
echo "Error: No 'base.json' file found in any 'config-typescript' directories."
exit 1
fi
Length of output: 349
| "license": "MIT", | ||
| "name": "@repo/config-prettier", | ||
| "private": true, |
There was a problem hiding this comment.
Resolve conflict between private flag and publishConfig
There's a conflict in the package configuration:
- The package is marked as
"private": true, which typically prevents the package from being published. - However, there's also a
publishConfigsection specifying public access.
This combination can lead to confusion about whether the package is intended to be published or not.
If the package should be publishable, remove the "private": true line. If it should remain private, remove the publishConfig section. For example:
"license": "MIT",
"name": "@repo/config-prettier",
-"private": true,
"publishConfig": {
"access": "public"
},
"version": "1.0.0"Or, if it should remain private:
"license": "MIT",
"name": "@repo/config-prettier",
"private": true,
-"publishConfig": {
- "access": "public"
-},
"version": "1.0.0"Please clarify the intended publishing status of this package and adjust the configuration accordingly.
Also applies to: 19-19
| "devDependencies": { | ||
| "@eslint/js": "latest", | ||
| "@typescript-eslint/eslint-plugin": "latest", | ||
| "@typescript-eslint/parser": "latest", | ||
| "eslint": "latest", | ||
| "eslint-config-turbo": "^1.13.4", | ||
| "eslint-plugin-only-warn": "^1.1.0", | ||
| "typescript": "^5.3.3" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning dependency versions for better reproducibility.
While using "latest" for dependencies allows you to always have the most recent updates, it can lead to unexpected breaking changes and make builds less reproducible. Consider pinning all dependency versions to specific numbers.
Here's a suggested change:
{
"devDependencies": {
- "@eslint/js": "latest",
- "@typescript-eslint/eslint-plugin": "latest",
- "@typescript-eslint/parser": "latest",
- "eslint": "latest",
+ "@eslint/js": "^8.56.0",
+ "@typescript-eslint/eslint-plugin": "^6.21.0",
+ "@typescript-eslint/parser": "^6.21.0",
+ "eslint": "^8.56.0",
"eslint-config-turbo": "^1.13.4",
"eslint-plugin-only-warn": "^1.1.0",
"typescript": "^5.3.3"
},
...
}Note: The versions I've suggested are the latest as of my knowledge cutoff. Please verify and use the most recent stable versions when implementing this change.
Committable suggestion was skipped due to low confidence.
| "files": [ | ||
| "web.js", | ||
| "base.js" | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Duplicate module.exports found in configuration files.
Both web.js and base.js contain multiple module.exports declarations, which can cause ESLint to ignore initial configurations. Please consolidate each file to have only one module.exports to ensure configurations are applied correctly.
Analysis chain
Verify the existence of configuration files.
The package includes "web.js" and "base.js" files. Please ensure these files exist in the repository and contain the appropriate ESLint configurations.
Let's verify the existence of these files:
Verify the contents of configuration files.
Please ensure that "web.js" and "base.js" contain the correct ESLint configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if web.js and base.js exist in the packages/config-eslint directory
# Test: Check for the existence of web.js and base.js
fd -p "packages/config-eslint/(web|base).js"
Length of output: 107
Script:
#!/bin/bash
# Description: Output the contents of web.js and base.js for verification
cat packages/config-eslint/web.js packages/config-eslint/base.js
Length of output: 1447
| { | ||
| "name": "harlighter", | ||
| "private": true, | ||
| "scripts": { | ||
| "build": "turbo build", | ||
| "dev": "turbo dev" | ||
| "dependencies": { | ||
| "@repo/reactor": "workspace:*" | ||
| }, | ||
| "devDependencies": { | ||
| "@repo/eslint-config": "workspace:*", | ||
| "@repo/typescript-config": "workspace:*", | ||
| "prettier": "^3.2.5", | ||
| "turbo": "1.13.3" | ||
| "@repo/config-eslint": "workspace:*", | ||
| "@repo/config-prettier": "workspace:*", | ||
| "@repo/config-typescript": "workspace:*", | ||
| "turbo": "latest" | ||
| }, | ||
| "packageManager": "[email protected]", | ||
| "engines": { | ||
| "node": ">=18" | ||
| }, | ||
| "dependencies": { | ||
| "@repo/reactor": "workspace:*" | ||
| "name": "harlighter", | ||
| "packageManager": "[email protected]", | ||
| "private": true, | ||
| "scripts": { | ||
| "build": "turbo build", | ||
| "dev": "turbo dev", | ||
| "format": "turbo format", | ||
| "lint": "turbo lint" | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Biome configurations still present in the codebase.
Please ensure that all references to biome have been removed or are intentional:
biome.jsonpackages/reactor/utils.tspackages/reactor/main.tspackages/reactor/modifications.tspackages/reactor/tests/main.test.ts
Analysis chain
Verify complete removal of biome.
The changes in this file align well with the PR objectives. However, to ensure a complete transition from biome to ESLint and updated TypeScript configs:
Run the following script to check for any remaining biome configurations:
If any results are found, ensure that these are intentional or remove them as part of this PR.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining biome configurations
# Test: Search for biome-related files or configurations
echo "Searching for biome-related files or configurations:"
fd -H 'biome' -t f
# Test: Search for biome mentions in other configuration files
echo "Searching for biome mentions in configuration files:"
rg -i 'biome' -g '*.json' -g '*.js' -g '*.ts'
Length of output: 759
| globals: { | ||
| JSX: true, | ||
| React: true, | ||
| }, | ||
| globals: { | ||
| chrome: "readonly", | ||
| }, |
There was a problem hiding this comment.
Merge duplicate 'globals' sections.
There are two 'globals' sections in the configuration. The second one overwrites the first, which means the JSX and React globals are not actually being set. This could lead to unexpected linting errors.
Please merge the two 'globals' sections to ensure all global variables are properly defined. Here's the suggested fix:
- globals: {
- JSX: true,
- React: true,
- },
- globals: {
+ globals: {
+ JSX: true,
+ React: true,
chrome: "readonly",
},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.
| globals: { | |
| JSX: true, | |
| React: true, | |
| }, | |
| globals: { | |
| chrome: "readonly", | |
| }, | |
| globals: { | |
| JSX: true, | |
| React: true, | |
| chrome: "readonly", | |
| }, |
Tools
Biome
[error] 12-15: This property value named globals is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named globals(lint/suspicious/noDuplicateObjectKeys)
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | ||
| } catch (error: any) { | ||
| console.error("Error parsing user request:", error); | ||
| throw new Error("Invalid user request format"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and removing biome-ignore comment
The current error handling uses the any type, which is generally discouraged in TypeScript as it bypasses type checking. Consider the following improvements:
- Use a more specific type for the error, such as
Erroror a custom error type. - Remove the
biome-ignorecomment by addressing the underlying issue.
Here's a suggested improvement:
try {
return JSON.parse(userRequest);
} catch (error) {
console.error("Error parsing user request:", error instanceof Error ? error.message : String(error));
throw new Error("Invalid user request format");
}This change eliminates the need for the biome-ignore comment and provides more type-safe error handling.
packages/reactor/utils.ts
Outdated
| export function generateRandomString(length: number) { | ||
| const characters = 'abcdefghijklmnopqrstuvwxyz0123456789'; | ||
| let result = ''; | ||
| const charactersLength = characters.length; | ||
| for (let i = 0; i < length; i++) { | ||
| result += characters.charAt(Math.floor(Math.random() * charactersLength)); | ||
| } | ||
| return result; | ||
| } | ||
| const characters = "abcdefghijklmnopqrstuvwxyz0123456789"; | ||
| let result = ""; | ||
| const charactersLength = characters.length; | ||
| for (let i = 0; i < length; i++) { | ||
| result += characters.charAt(Math.floor(Math.random() * charactersLength)); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving security and input validation in generateRandomString
While the function works as intended, there are two areas for potential improvement:
-
Security:
Math.random()is not cryptographically secure. For generating random strings in a security-sensitive context, consider usingcrypto.randomBytes(). -
Input validation: The function doesn't validate the
lengthparameter. It's good practice to ensure the input is a positive integer.
Here's a suggested improvement:
import crypto from 'crypto';
export function generateRandomString(length: number): string {
if (!Number.isInteger(length) || length <= 0) {
throw new Error('Length must be a positive integer');
}
const characters = 'abcdefghijklmnopqrstuvwxyz0123456789';
const bytes = crypto.randomBytes(length);
let result = '';
for (let i = 0; i < length; i++) {
result += characters[bytes[i] % characters.length];
}
return result;
}This implementation is more secure and includes input validation.
| it("should handle an added mutation", async () => { | ||
| doc.body.innerHTML = | ||
| "<h1>train</h1><h2>about</h2><div>Trains are really cool. I use my train every day.</div>"; | ||
|
|
||
| doc.body.innerHTML = "<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>"; | ||
| // wait for the changes to be applied | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| expect(doc.body.innerHTML).toMatchIgnoringMocksiTags("<h1>brains are uncool</h1><h2>avoid</h2><div>I hate brains. I never ride them.</div>"); | ||
| }); | ||
| const request: ModificationRequest = { | ||
| description: "Change train to brain", | ||
| modifications: [ | ||
| { | ||
| selector: "body", | ||
| action: "replaceAll", | ||
| content: "/train/brain/wip", | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| it("should handle undoing after an added mutation", async () => { | ||
| doc.body.innerHTML = "<h1>train</h1><h2>about</h2><div>Trains are really cool. I use my train every day.</div>"; | ||
|
|
||
| const request: ModificationRequest = { | ||
| description: "Change train to brain", | ||
| modifications: [ | ||
| { | ||
| selector: "body", | ||
| action: "replaceAll", | ||
| content: "/train/brain/", | ||
| } | ||
| ], | ||
| }; | ||
| await reactor.pushModification(request); | ||
| expect(doc.body.innerHTML).toMatchIgnoringMocksiTags( | ||
| "<h1>brain</h1><h2>about</h2><div>Brains are really cool. I use my brain every day.</div>", | ||
| ); | ||
|
|
||
| await reactor.pushModification(request); | ||
| doc.body.innerHTML = "<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>"; | ||
| // wait for the changes to be applied | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| reactor.detach(); | ||
| doc.body.innerHTML = | ||
| "<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>"; | ||
| // wait for the changes to be applied | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| expect(doc.body.innerHTML).toMatchIgnoringMocksiTags( | ||
| "<h1>brains are uncool</h1><h2>avoid</h2><div>I hate brains. I never ride them.</div>", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more robust waiting mechanism
The test case effectively checks the application and persistence of modifications. However, the use of a fixed 1-second timeout (line 46) to wait for changes to be applied could potentially lead to flaky tests, especially on slower systems or under high load.
Consider replacing the fixed timeout with a more robust waiting mechanism. For example, you could use a polling approach or, if possible, listen for an event that signifies the completion of the modification. Here's a potential implementation using a polling approach:
const waitForChanges = async (maxWait = 5000, interval = 100) => {
const startTime = Date.now();
while (Date.now() - startTime < maxWait) {
if (doc.body.innerHTML.includes('brains are uncool')) {
return;
}
await new Promise(resolve => setTimeout(resolve, interval));
}
throw new Error('Timed out waiting for changes');
};
// Replace line 46 with:
await waitForChanges();This approach will make the test more reliable and potentially faster on average, as it won't always wait for the full second if changes are applied quickly.
| it("should handle undoing after an added mutation", async () => { | ||
| doc.body.innerHTML = | ||
| "<h1>train</h1><h2>about</h2><div>Trains are really cool. I use my train every day.</div>"; | ||
|
|
||
| const request: ModificationRequest = { | ||
| description: "Change train to brain", | ||
| modifications: [ | ||
| { | ||
| selector: "body", | ||
| action: "replaceAll", | ||
| content: "/train/brain/", | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| await reactor.pushModification(request); | ||
| doc.body.innerHTML = | ||
| "<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>"; | ||
| // wait for the changes to be applied | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| reactor.detach(); | ||
|
|
||
| expect(doc.body.innerHTML).toMatchIgnoringMocksiTags( | ||
| "<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for reactor detachment
The test case effectively verifies that manual changes persist after detaching the reactor. However, it doesn't explicitly check if the initial modification was applied before detaching.
Consider adding an assertion immediately after applying the modification to ensure it was correctly applied before detaching the reactor. This will provide more comprehensive coverage of the reactor's behavior. Here's a suggested modification:
await reactor.pushModification(request);
// Add this assertion
expect(doc.body.innerHTML).toMatchIgnoringMocksiTags(
"<h1>brain</h1><h2>about</h2><div>Brains are really cool. I use my brain every day.</div>"
);
doc.body.innerHTML =
"<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>";
// Use the waitForChanges function suggested in the previous comment
await waitForChanges();
reactor.detach();
expect(doc.body.innerHTML).toMatchIgnoringMocksiTags(
"<h1>trains are uncool</h1><h2>avoid</h2><div>I hate trains. I never ride them.</div>"
);This change will ensure that both the application of the modification and its subsequent removal upon detachment are explicitly verified.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores