feat: add new lint command
#15
Conversation
JounQin
added
enhancement
🦋 Changeset detectedLatest commit: 11a9ab5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces a new linting feature to the project. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LintCommand
participant Helpers
participant ESLint
participant Config
User->>CLI: Run "doom lint"
CLI->>LintCommand: Invoke lintCommand
LintCommand->>Config: Load user config and lint options
LintCommand->>Helpers: parseTerms()
Helpers->>Helpers: Fetch and parse remote YAML (if not cached)
Helpers-->>LintCommand: Return terms
LintCommand->>ESLint: Run lint with config, spellchecker, and terms
ESLint-->>LintCommand: Return lint results
LintCommand->>User: Output linting results and set exit code
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Signed-off-by: JounQin <admin@1stg.me>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new lint command that integrates cspell and ESLint into the documentation workflow while updating related configurations and dependencies. Key changes include:
- Adding a new LintOptions interface to the shared types.
- Implementing the new lint command in src/cli/lint.ts and wiring it into the CLI.
- Updating configuration files (package.json, doom.config.yml, eslint.config.js) to support linting.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds LintOptions interface and extends the user config. |
| src/cli/translate.ts | Refactors term parsing by replacing direct YAML parsing with a call to parseTerms(). |
| src/cli/lint.ts | Implements the new lint command using ESLint and merges cspell options. |
| src/cli/index.ts | Registers the new lint command with the CLI program. |
| src/cli/helpers.ts | Adds a caching mechanism for parsed terms and imports required for YAML parsing. |
| package.json | Introduces new lint scripts and updates several dependency versions. |
| fixture-docs/doom.config.yml | Adds lint configuration for cspell options in fixture docs. |
| eslint.config.js | Updates ESLint configuration, removing a block for JavaScript files. |
| doom.config.yml | Includes lint configuration with custom cspell words. |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/cli/lint.ts (2)
30-30: Consider addressing the type issue instead of suppressing it.Type suppression with
@ts-expect-errorshould be avoided when possible. Consider addressing the underlying type issue to enhance code robustness.
60-82: Remove the commented-out ESLint configuration.The commented-out TypeScript configuration block should be removed to maintain clean code. If it's kept for future reference, add a clear comment explaining why.
🧹 Nitpick comments (2)
doom.config.yml (1)
23-30: Good configuration for custom spell checking.The added configuration provides domain-specific words to be recognized by the spell checker, which will help prevent false positives during linting.
Consider maintaining this word list in alphabetical order for easier maintenance as more terms are added in the future.
lint: cspellOptions: cspell: words: - - artifactcleanupruns - - katanomi - - testplans - - testmodules + - artifactcleanupruns + - katanomi + - testmodules + - testplanssrc/cli/lint.ts (1)
86-96: Consider adding a success message for clean runs.The code currently only provides output when linting issues are found. Consider adding a success message when all files pass linting for better user feedback.
const results = await eslint.lintFiles('**/*.{js,jsx,ts,tsx,md,mdx}') if (!results.length) { + console.log('Linting completed successfully: no issues found.') return } process.exitCode = 1 const formatter = await eslint.loadFormatter('stylish') process.stderr.write(await formatter.format(results))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.changeset/weak-apricots-warn.md(1 hunks).gitignore(1 hunks)doom.config.yml(1 hunks)eslint.config.js(1 hunks)fixture-docs/doom.config.yml(1 hunks)package.json(3 hunks)src/cli/helpers.ts(2 hunks)src/cli/index.ts(2 hunks)src/cli/lint.ts(1 hunks)src/cli/translate.ts(2 hunks)src/types.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/cli/helpers.ts (1)
src/terms.ts (1)
NormalizedTermItem(40-40)
src/cli/translate.ts (1)
src/cli/helpers.ts (1)
parseTerms(65-65)
src/cli/lint.ts (3)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/cli/helpers.ts (1)
parseTerms(65-65)
🪛 ESLint
src/cli/helpers.ts
[error] 4-4: Unable to resolve path to module 'x-fetch'.
(import-x/no-unresolved)
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🪛 GitHub Actions: CI
src/cli/helpers.ts
[error] 33-33: TypeError: fetch failed during rendering pages. Error originated in async function parseTerms_.
src/cli/lint.ts
[error] 15-15: Async command execution failed due to fetch error in helper function. 'lint:doom' and 'lint' scripts exited with code 1.
🔇 Additional comments (17)
.changeset/weak-apricots-warn.md (1)
1-5: Changeset looks good and follows proper conventions.This changeset correctly documents the minor version update for adding the new
lintcommand feature..gitignore (1)
1-1: Good addition to ignore type coverage artifacts.Adding
.type-coverageto the gitignore is appropriate as these are generated files that shouldn't be tracked in version control.src/cli/index.ts (2)
23-23: Correctly imports the lint command module.Following the established pattern for importing command modules.
257-257: Properly integrates the lint command into the CLI.The lint command is correctly added to the program using the same pattern as the other commands.
fixture-docs/doom.config.yml (1)
49-63: New lint configuration looks good.The addition of a spell-checking configuration with domain-specific technical terms will help prevent false positives during linting of documentation. The structure aligns with the new lint command implementation.
src/types.ts (3)
1-1: Import looks good.Adding the import for the CSpell plugin Options type supports the new linting functionality.
41-43: New LintOptions interface is well-designed.The interface correctly allows for partial CSpell configuration options to be specified by users.
59-59: Good extension of UserConfig interface.Adding the lint property to the UserConfig interface properly integrates the linting capability with the existing configuration system.
src/cli/helpers.ts (1)
63-65: Cache implementation looks good.The module-level caching of the parsed terms promise is a good performance optimization to avoid redundant network requests and parsing operations.
src/cli/translate.ts (2)
32-32: Good refactoring of imports.Importing the parseTerms function from helpers.js simplifies this file by removing direct dependencies on x-fetch and yaml.
98-104: Clean refactoring of term resolution logic.The refactoring of
resolveTerms_to use the new centralizedparseTerms()helper function improves code maintainability.package.json (2)
48-48: Great addition of the new lint script.Adding a dedicated script for running the new
doom lintcommand is a good practice and maintains consistency with other lint commands in the project.
60-61: Good dependency additions for the linting feature.The new dependencies align with the linting functionality being implemented:
@cspell/eslint-pluginandeslint-plugin-mdxenable linting for spell checking and MDX files@eslint-react/eslint-pluginadds React-specific linting support- Moving
eslintfrom devDependencies to dependencies makes sense for the new featureThese provide a comprehensive linting capability for documentation files.
Also applies to: 74-75, 77-77, 91-91, 97-98
src/cli/lint.ts (2)
16-19: Well-structured command definition.The lint command is clearly defined with an appropriate description and an optional root directory argument, following the command pattern used elsewhere in the application.
40-59: Well-implemented spellchecking configuration.The spellchecking configuration is well-implemented with:
- File pattern targeting English documentation
- Auto-fixing capability
- Integration with parsed terms for custom words and bad cases
- Merging with user-defined configuration options
This provides flexible and extensible spellchecking functionality.
eslint.config.js (2)
16-18: Good modularization of TypeScript linting configuration.Moving TypeScript-specific configurations under a file pattern filter is a good practice. This improves the organization of the ESLint configuration and makes it more maintainable.
32-35: Well-structured import resolver configuration.Separating the import resolver configuration into its own block improves organization and makes the configuration more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/cli/helpers.ts (1)
55-62: Add error handling for remote fetch operationThe current implementation will throw an exception if the fetch operation fails, which can happen due to network issues or if the remote server is unavailable. While this may be intentional (based on past comments), consider adding at least minimal error handling with a specific error message to make debugging easier.
const parseTerms_ = async () => { - const terms = await xfetch( - process.env.RAW_TERMS_URL || - 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', - { type: 'text' }, - ) - return parse(terms) as NormalizedTermItem[] + try { + const terms = await xfetch( + process.env.RAW_TERMS_URL || + 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', + { type: 'text' }, + ) + return parse(terms) as NormalizedTermItem[] + } catch (error) { + throw new Error(`Failed to fetch or parse terms: ${error instanceof Error ? error.message : String(error)}`) + } }Let's check how this function is used in the codebase:
#!/bin/bash # Check where parseTerms is used grep -r "parseTerms" src/ --include="*.ts" --exclude="helpers.ts"src/cli/lint.ts (2)
30-31: Address TypeScript type issue instead of using@ts-expect-errorUsing
@ts-expect-errorsuppresses the type checking, which might hide important issues. Consider addressing the underlying type problem or providing a more specific comment explaining why it can't be fixed.Let's examine tseslint's typing requirements:
#!/bin/bash # Check tseslint.config's type definition grep -r "config" --include="*.d.ts" node_modules/typescript-eslint/
60-82: Remove or properly document the commented-out TypeScript configurationThis large commented-out block takes up space and might confuse future maintainers. If it's important to keep as a reference, add a more detailed comment explaining why it's commented out and when it might be re-enabled.
If you're keeping it as a reference for an ESLint issue that needs to be fixed later, consider adding a more descriptive comment:
-// https://github.com/eslint/eslint/issues/19722 -// { -// files: ['**/*.{ts,tsx}'], -// extends: [ -// tseslint.configs.recommendedTypeChecked, -// react.configs['recommended-typescript'], -// ], -// rules: { -// '@typescript-eslint/no-misused-promises': 'off', -// '@typescript-eslint/no-non-null-assertion': 'off', -// '@typescript-eslint/restrict-template-expressions': [ -// 'error', -// { allowNumber: true }, -// ], -// 'prefer-const': ['error', { destructuring: 'all' }], -// }, -// languageOptions: { -// parser: tseslint.parser, -// parserOptions: { -// projectService: true, -// }, -// }, -// }, +// TODO: Re-enable TypeScript-specific linting when ESLint issue #19722 is fixed +// This configuration will provide better type checking for TypeScript files +// See: https://github.com/eslint/eslint/issues/19722
🧹 Nitpick comments (3)
src/cli/helpers.ts (1)
4-8: Consider adding import type for NormalizedTermItemSince
NormalizedTermItemis only used as a type, consider usingimport type { NormalizedTermItem } from '../terms.js'to ensure it's not included in the runtime bundle.-import { xfetch } from 'x-fetch' -import { parse } from 'yaml' +import { xfetch } from 'x-fetch' +import { parse } from 'yaml' import { FALSY_VALUES } from '../shared/index.js' -import type { NormalizedTermItem } from '../terms.js' +import type { NormalizedTermItem } from '../terms.js'🧰 Tools
🪛 ESLint
[error] 4-4: Unable to resolve path to module 'x-fetch'.
(import-x/no-unresolved)
src/cli/lint.ts (2)
1-15: Consider organizing imports by type and alphabetizing for better readabilityThe imports are currently mixed without clear organization. Consider grouping them by external dependencies, internal types, and internal modules, then sorting alphabetically within each group.
-import type { Options } from '@cspell/eslint-plugin' -import cspellRecommended from '@cspell/eslint-plugin/recommended' -import js from '@eslint/js' -import react from '@eslint-react/eslint-plugin' -import { Command } from 'commander' -import { merge } from 'es-toolkit/compat' -import { ESLint } from 'eslint' -import * as mdx from 'eslint-plugin-mdx' -import tseslint from 'typescript-eslint' +// External dependencies +import type { Options } from '@cspell/eslint-plugin' +import cspellRecommended from '@cspell/eslint-plugin/recommended' +import { Command } from 'commander' +import { ESLint } from 'eslint' +import js from '@eslint/js' +import { merge } from 'es-toolkit/compat' +import * as mdx from 'eslint-plugin-mdx' +import react from '@eslint-react/eslint-plugin' +import tseslint from 'typescript-eslint' -import type { GlobalCliOptions } from '../types.js' +// Internal types +import type { GlobalCliOptions } from '../types.js' -import { parseTerms } from './helpers.js' -import { loadConfig } from './load-config.js' +// Internal modules +import { loadConfig } from './load-config.js' +import { parseTerms } from './helpers.js'🧰 Tools
🪛 ESLint
[error] 5-5: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
86-97: Consider checking for errors rather than any results before setting exitCodeCurrently, the process exitCode is set to 1 if there are any linting results, regardless of their severity. Consider checking if there are actual errors before failing the process, so warnings don't cause failures.
- if (!results.length) { + const formatter = await eslint.loadFormatter('stylish') + + process.stderr.write(await formatter.format(results)) + + // Only set exitCode to 1 if there are actual errors (not just warnings) + if (results.some(result => result.errorCount > 0)) { + process.exitCode = 1 + } + + if (!results.length) { return } - - process.exitCode = 1 - - const formatter = await eslint.loadFormatter('stylish') - - process.stderr.write(await formatter.format(results))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.yml(1 hunks)docs/en/start.mdx(3 hunks)docs/en/usage/configuration.md(2 hunks)docs/en/usage/deploy.md(1 hunks)docs/en/usage/reference.md(1 hunks)docs/zh/start.mdx(3 hunks)docs/zh/usage/configuration.md(1 hunks)package.json(3 hunks)src/cli/helpers.ts(2 hunks)src/cli/lint.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- docs/en/usage/reference.md
- docs/en/usage/deploy.md
- docs/zh/usage/configuration.md
- .github/workflows/ci.yml
- docs/en/usage/configuration.md
- docs/zh/start.mdx
- docs/en/start.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/cli/helpers.ts (1)
src/terms.ts (1)
NormalizedTermItem(40-40)
src/cli/lint.ts (3)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/cli/helpers.ts (1)
parseTerms(66-66)
🪛 ESLint
src/cli/helpers.ts
[error] 4-4: Unable to resolve path to module 'x-fetch'.
(import-x/no-unresolved)
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🔇 Additional comments (3)
src/cli/helpers.ts (1)
64-66: LGTM: Good use of caching to avoid redundant network requestsThe caching mechanism is well implemented using the nullish coalescing assignment operator to avoid redundant network requests when
parseTermsis called multiple times.src/cli/lint.ts (2)
19-26: LGTM: Good use of Commander action patternThe command implementation follows the Commander pattern effectively, using
this.optsWithGlobals<GlobalCliOptions>()to access global options.
40-59: LGTM: Well-structured spell checking configurationThe linting configuration for English files effectively uses the parsed terms for spell checking, with good organization of words and flagged words. The use of
mergeto combine with user-provided options is a nice touch.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cli/lint.ts (1)
60-82: Remove commented-out ESLint configuration block if not needed.This commented-out TypeScript configuration block should be removed if it's no longer needed to keep the codebase clean and maintainable.
🧹 Nitpick comments (2)
src/cli/helpers.ts (1)
55-62: Consider making the terms URL more configurable.The URL for fetching terms is hardcoded with only an environment variable override. Consider making this URL configurable in the application's configuration file for better flexibility and to avoid hardcoding external resources.
const parseTerms_ = async () => { const terms = await xfetch( process.env.RAW_TERMS_URL || - 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', + config.termsUrl || 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', { type: 'text' }, ) return parse(terms) as NormalizedTermItem[] }src/cli/lint.ts (1)
26-27: Consider adding fallback for terms in offline environments.While the author confirmed that throwing errors is the intended behavior, for improved developer experience, consider adding a fallback for situations where network access is limited, such as local development in offline environments.
- const parsedTerms = await parseTerms() + let parsedTerms = [] + try { + parsedTerms = await parseTerms() + } catch (error) { + console.warn('Failed to fetch terms, continuing with empty terms list:', error) + // Continue with empty terms list for offline development scenarios + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.yml(1 hunks)docs/en/start.mdx(3 hunks)docs/en/usage/configuration.md(2 hunks)docs/en/usage/deploy.md(1 hunks)docs/en/usage/reference.md(1 hunks)docs/zh/start.mdx(3 hunks)docs/zh/usage/configuration.md(1 hunks)package.json(3 hunks)src/cli/helpers.ts(2 hunks)src/cli/lint.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/en/usage/reference.md
- docs/en/usage/deploy.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci.yml
- docs/en/usage/configuration.md
- docs/en/start.mdx
- docs/zh/usage/configuration.md
- docs/zh/start.mdx
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/cli/helpers.ts (1)
src/terms.ts (1)
NormalizedTermItem(40-40)
src/cli/lint.ts (3)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/cli/helpers.ts (1)
parseTerms(66-66)
🪛 ESLint
src/cli/helpers.ts
[error] 4-4: Unable to resolve path to module 'x-fetch'.
(import-x/no-unresolved)
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🔇 Additional comments (3)
src/cli/helpers.ts (1)
64-66: Good use of memoization pattern.The memoization pattern using a module-level cache variable is a good approach to ensure the terms are only fetched and parsed once per runtime, which helps with performance.
src/cli/lint.ts (2)
48-53: Excellent use of parsed terms for spellchecking.The mapping of parsed terms to allowed words and flatMapping of bad cases to flagged words is a clever approach to leverage the term dictionary for spellchecking. This will help ensure consistent terminology throughout the documentation.
86-97: Well-structured linting output handling.This implementation correctly:
- Sets the process exit code to 1 if linting issues are found
- Uses the 'stylish' formatter for readable output
- Writes to stderr which is appropriate for error output
The approach follows standard CLI tool conventions for exit codes and output streams.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/cli/lint.ts (2)
38-38: Consider addressing the underlying type issue instead of suppressing itUsing
@ts-expect-errorsuppresses the type checking error without fixing the underlying problem. It would be better to improve the typing or use appropriate type assertions.
68-90: Remove commented-out ESLint configuration if no longer neededThis large commented block contains TypeScript-specific ESLint configuration that's currently disabled. If it's no longer needed, consider removing it to keep the code clean. If it's for future use, consider adding a TODO comment explaining when it will be used.
The GitHub issue reference (#19722) suggests this might be disabled due to a known issue. Check if the issue is still relevant before removing.
🧹 Nitpick comments (3)
src/cli/lint.ts (3)
26-26: Consider handling potential undefined config.root valueThe non-null assertion (
!) onconfig.rootassumes the configuration will always have a root property that's defined. Adding a fallback or validation would make the code more robust.- const docsDir = config.root! + if (!config.root) { + logger.error('Documentation root not defined in configuration') + process.exitCode = 1 + return + } + const docsDir = config.root
96-98: Add a success message for clear feedbackWhen no lint errors are found, the command silently returns. Consider adding a success message to provide clear feedback to the user.
if (!results.length) { + logger.success('No lint errors found') return }
94-94: Consider allowing users to specify custom lint patternsThe lint command currently uses a hardcoded pattern for file extensions. Consider allowing users to specify custom patterns through configuration or command-line options.
- const results = await eslint.lintFiles('**/*.{js,jsx,ts,tsx,md,mdx}') + const filePattern = config.lint?.filePattern || '**/*.{js,jsx,ts,tsx,md,mdx}' + const results = await eslint.lintFiles(filePattern)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/cli/lint.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module '@rspress/shared/logger'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: publish
- GitHub Check: Build and Lint with Node.js 22 on ubuntu-latest
🔇 Additional comments (2)
src/cli/lint.ts (2)
30-34: Good error handling implementationThe try-catch block for
parseTerms()is a good addition, allowing the linting to continue even if term parsing fails. This improves robustness compared to letting the error propagate.
57-61:Details
✅ Verification successful
Review the term extraction logic
The term handling logic extracts English terms and bad cases from parsed terms. Ensure this matches the expected format of your terms data structure, especially the handling of potentially undefined
badCasesproperty.
🏁 Script executed:
#!/bin/bash # Check the structure of NormalizedTermItem and how parseTerms is implemented echo "Examining NormalizedTermItem structure:" rg -A 10 "NormalizedTermItem" src/ echo "Examining parseTerms implementation:" rg -A 10 "parseTerms =" src/Length of output: 4145
🏁 Script executed:
#!/bin/bash echo "Imports in src/cli/helpers.ts:" rg -n "^import" src/cli/helpers.ts echo -e "\nLook for parseTerms_ definition:" rg -n -A5 "parseTerms_" src/cli/helpers.ts echo -e "\nContents around return parse(" rg -n -A5 "return parse" src/cli/helpers.ts echo -e "\nShow NamedTermItem definition in src/terms.ts:" rg -A20 "export type NamedTermItem" src/terms.tsLength of output: 1266
#!/bin/bash # Find the NamedTermItem definition in src/terms.ts rg -n "type NamedTermItem" -A20 src/terms.ts || true
🏁 Script executed:
#!/bin/bash echo "Searching for NamedTermItem definition in src/terms.ts" rg -n -C5 "NamedTermItem" src/terms.ts echo -e "\nInspecting badCases property in terms.ts" rg -n -C5 "badCases" src/terms.tsLength of output: 987
Term extraction logic validated
TheTermIteminterface definesbadCasesas an optional object with an optionalenarray, and usingbadCases?.en ?? []correctly yields an empty array when no bad cases are provided. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/cli/lint.ts (2)
38-38: Address the TypeScript typing issue instead of suppressing it.The
@ts-expect-errorsuppression hides a potential type mismatch between ESLint's expected configuration type and what's being provided.Consider addressing the underlying type issue rather than suppressing it. This would improve code robustness and prevent potential runtime errors.
68-90: Remove the commented-out ESLint configuration if not needed.This large commented-out block of TypeScript-specific ESLint configuration adds noise to the codebase.
If this configuration is no longer needed, consider removing it. If it's intended for future use, add a TODO comment explaining why it's kept and when it will be used.
🧹 Nitpick comments (3)
src/cli/lint.ts (3)
48-67: Expand linting support for non-English documentation.Currently, spellchecking is only applied to English files (
**/en/**/*.{js,jsx,md,mdx,ts,tsx}). If your documentation supports other languages, consider adding similar configurations for those languages with their respective terms.{ files: ['**/en/**/*.{js,jsx,md,mdx,ts,tsx}'], extends: [cspellRecommended], rules: { '@cspell/spellchecker': [ 'error', merge( { autoFix: true, cspell: { words: parsedTerms.map((it) => it.en), flagWords: parsedTerms.flatMap( ({ badCases }) => badCases?.en ?? [], ), }, } satisfies Partial<Options>, config.lint?.cspellOptions, ), ], }, }, + // Example for Chinese documentation + { + files: ['**/zh/**/*.{js,jsx,md,mdx,ts,tsx}'], + extends: [cspellRecommended], + rules: { + '@cspell/spellchecker': [ + 'error', + merge( + { + autoFix: true, + cspell: { + words: parsedTerms.map((it) => it.zh), + flagWords: parsedTerms.flatMap( + ({ badCases }) => badCases?.zh ?? [], + ), + }, + } satisfies Partial<Options>, + config.lint?.cspellOptions, + ), + ], + }, + },
94-98: Consider adding progress reporting for large documentation sets.For large documentation sets, linting can take significant time. Adding progress reporting would improve user experience.
- const results = await eslint.lintFiles('**/*.{js,jsx,ts,tsx,md,mdx}') + logger.info('Starting linting process...') + const results = await eslint.lintFiles('**/*.{js,jsx,ts,tsx,md,mdx}') + if (!results.length) { + logger.success('Linting completed: no issues found') return }
100-104: Enhance error output with summary statistics.Consider enhancing the error output with summary statistics to make it easier for users to understand the scope of issues.
process.exitCode = 1 + + // Add summary statistics + const errorCount = results.reduce((count, result) => count + result.errorCount, 0) + const warningCount = results.reduce((count, result) => count + result.warningCount, 0) + logger.error(`Linting completed with ${errorCount} errors and ${warningCount} warnings`) const formatter = await eslint.loadFormatter('stylish') process.stderr.write(await formatter.format(results))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)package.json(3 hunks)src/cli/lint.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/cli/lint.ts (4)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/terms.ts (1)
NormalizedTermItem(40-40)src/cli/helpers.ts (1)
parseTerms(66-66)
🪛 ESLint
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module '@rspress/shared/logger'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🔇 Additional comments (2)
src/cli/lint.ts (2)
30-34: Evaluate if error handling aligns with the linting philosophy.The error handling for
parseTerms()logs the error but continues execution. This makes spellchecking with terms optional when terms can't be loaded - is this the intended behavior? Earlier comments indicate errors should be thrown, but here they're caught.Consider whether linting should fail entirely when terms cannot be parsed, especially if term validation is a critical part of your documentation quality process.
5-7:Details
✅ Verification successful
Consider standardizing import paths for better maintainability.
The static analysis indicates potential issues with resolving these modules:
- '@rspress/shared/logger'
- 'commander'
- 'es-toolkit/compat'
While these may be false positives, ensuring consistent import patterns throughout the codebase would help with maintainability.
🏁 Script executed:
#!/bin/bash # Verify how imports are handled in other parts of the codebase echo "Checking import patterns for these packages in other files:" rg -l '@rspress/shared/logger|commander|es-toolkit/compat' --type ts src/ | head -n 10Length of output: 532
🏁 Script executed:
#!/bin/bash # Check import statements for consistency across the codebase echo "=== @rspress/shared/logger imports ===" rg -n "import.*logger.*from.*['\"]@rspress/shared/logger['\"]" src/ echo "" echo "=== commander imports ===" rg -n "import.*from.*['\"]commander['\"]" src/ echo "" echo "=== es-toolkit/compat imports ===" rg -n "import.*from.*['\"]es-toolkit/compat['\"]" src/Length of output: 2099
Imports are already consistent across the codebase
All occurrences of these modules use the same import paths, so no further standardization is needed:
@rspress/shared/loggercommanderes-toolkit/compatNo changes required.
🧰 Tools
🪛 ESLint
[error] 5-5: Unable to resolve path to module '@rspress/shared/logger'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/cli/lint.ts (2)
38-38: Consider addressing the underlying type issue instead of suppressing it.Using
@ts-expect-errorto bypass type checking is a quick solution, but addressing the underlying typing issue would improve code quality and maintainability.
68-90: Remove the commented-out ESLint configuration block.This large block of commented code makes the file harder to read and maintain. If it's for future reference, consider:
- Moving it to documentation
- Creating a separate file for TypeScript-specific configurations
- Removing it entirely if it's obsolete
🧹 Nitpick comments (3)
src/cli/lint.ts (3)
96-96: Consider adding file exclusion patterns.The pattern
'**/*.{js,jsx,ts,tsx,md,mdx}'will match all files in the directory. For large documentation sites, this could include generated files or dependencies that you might want to exclude.- const results = await eslint.lintFiles('**/*.{js,jsx,ts,tsx,md,mdx}') + const results = await eslint.lintFiles([ + '**/*.{js,jsx,ts,tsx,md,mdx}', + '!**/node_modules/**', + '!**/dist/**', + '!**/build/**' + ])
102-109: Consider different exit codes for errors vs. warnings.Currently, the command exits with code 1 if there are any linting results, regardless of whether they are errors or just warnings. This is common in CI environments, but you might want to make this behavior configurable.
const errorCount = results.reduce( (count, result) => count + result.errorCount, 0, ) const warningCount = results.reduce( (count, result) => count + result.warningCount, 0, ) + + // Only set exit code if there are errors (not just warnings) + if (errorCount > 0) { + process.exitCode = 1 + }
119-119: Consider making the exit behavior configurable.Setting
process.exitCode = 1unconditionally means the command will always exit with an error if there are any linting issues. You might want to make this configurable, allowing users to treat warnings as non-fatal.- process.exitCode = 1 + // Only set exit code for errors if the global option isn't set to ignore + if (!globalOptions.ignoreWarnings) { + process.exitCode = 1 + } else if (errorCount > 0) { + // Still fail on errors even if warnings are ignored + process.exitCode = 1 + }This would require adding an
ignoreWarningsoption to theGlobalCliOptionsinterface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)package.json(2 hunks)src/cli/lint.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/cli/lint.ts (4)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/terms.ts (1)
NormalizedTermItem(40-40)src/cli/helpers.ts (1)
parseTerms(66-66)
🪛 ESLint
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module '@rspress/shared/logger'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🔇 Additional comments (2)
src/cli/lint.ts (2)
30-34: Good error handling for term parsing.The try-catch block properly handles potential failures when parsing terms, allowing the linting process to continue even if term parsing fails. This is a good practice for graceful degradation.
48-66: LGTM: Well-structured spell checking configuration.The configuration properly uses the parsed terms for English documentation files, both for allowed words and flagged words. The
mergefunction ensures that custom options from the user's configuration are respected.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/cli/helpers.ts (1)
55-62: Consider handling possible fetch failures.The
parseTerms_function fetches a YAML file from a remote URL without error handling. While you've indicated this is intentional behavior, consider adding error handling to provide clearer error messages to users if the fetch fails.const parseTerms_ = async () => { - const terms = await xfetch( - process.env.RAW_TERMS_URL || - 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', - { type: 'text' }, - ) - return parse(terms) as NormalizedTermItem[] + try { + const terms = await xfetch( + process.env.RAW_TERMS_URL || + 'https://gitlab-ce.alauda.cn/alauda-public/product-doc-guide/-/raw/main/terms.yaml', + { type: 'text' }, + ) + return parse(terms) as NormalizedTermItem[] + } catch (error) { + throw new Error(`Failed to fetch or parse terms: ${error instanceof Error ? error.message : String(error)}`) + } }src/cli/lint.ts (3)
23-27: Consider providing fallback for missing root directory.You're correctly loading the configuration and extracting the documentation root directory. However, there's no explicit check that
config.rootis defined before using it with the non-null assertion operator (!).- const docsDir = config.root! + const docsDir = config.root ?? process.cwd()
29-32: Remove or document the TypeScript type suppression.You're using
@ts-expect-errorto suppress a TypeScript error, but it's unclear why this is necessary. If this is due to a known issue with the ESLint types, consider adding a comment explaining the reason or linking to the relevant issue.- // @ts-expect-error -- stronger types + // @ts-expect-error -- ESLint.BaseConfig doesn't properly type the flat config format
61-83: Remove or document commented-out TypeScript configuration.There's a large block of commented-out ESLint configuration for TypeScript files. If this is planned for future implementation, consider adding a TODO comment explaining when and why it would be uncommented. If it's no longer needed, consider removing it to reduce clutter.
- // https://github.com/eslint/eslint/issues/19722 - // { - // files: ['**/*.{ts,tsx}'], - // extends: [ - // tseslint.configs.recommendedTypeChecked, - // react.configs['recommended-typescript'], - // ], - // rules: { - // '@typescript-eslint/no-misused-promises': 'off', - // '@typescript-eslint/no-non-null-assertion': 'off', - // '@typescript-eslint/restrict-template-expressions': [ - // 'error', - // { allowNumber: true }, - // ], - // 'prefer-const': ['error', { destructuring: 'all' }], - // }, - // languageOptions: { - // parser: tseslint.parser, - // parserOptions: { - // projectService: true, - // }, - // }, - // }, + // TODO: Enable TypeScript-specific linting once ESLint issue #19722 is resolved + // See: https://github.com/eslint/eslint/issues/19722
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.yml(1 hunks)docs/en/start.mdx(3 hunks)docs/en/usage/configuration.md(2 hunks)docs/en/usage/deploy.md(1 hunks)docs/en/usage/reference.md(1 hunks)docs/zh/start.mdx(3 hunks)docs/zh/usage/configuration.md(1 hunks)package.json(3 hunks)src/cli/helpers.ts(2 hunks)src/cli/lint.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- docs/en/usage/reference.md
- docs/zh/usage/configuration.md
- docs/en/usage/deploy.md
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/en/usage/configuration.md
- docs/zh/start.mdx
- docs/en/start.mdx
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/cli/helpers.ts (1)
src/terms.ts (1)
NormalizedTermItem(40-40)
src/cli/lint.ts (3)
src/types.ts (1)
GlobalCliOptions(18-34)src/cli/load-config.ts (1)
loadConfig(325-484)src/cli/helpers.ts (1)
parseTerms(66-66)
🪛 ESLint
src/cli/helpers.ts
[error] 4-4: Unable to resolve path to module 'x-fetch'.
(import-x/no-unresolved)
src/cli/lint.ts
[error] 5-5: Unable to resolve path to module '@rspress/shared/logger'.
(import-x/no-unresolved)
[error] 6-6: Unable to resolve path to module 'commander'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module 'es-toolkit/compat'.
(import-x/no-unresolved)
🔇 Additional comments (5)
src/cli/helpers.ts (1)
64-66: Good use of caching with memoization pattern.The caching mechanism using a module-level variable and the nullish coalescing operator is an effective way to avoid repeated network requests. This is a good optimization, especially for a CLI tool where performance is important.
src/cli/lint.ts (4)
17-20: Command definition is clear and follows best practices.The lint command is well-defined with a clear description and an optional root directory argument, following Command pattern best practices.
41-60: Well-structured spellchecking configuration.Your approach to configuring
@cspell/spellcheckeris well thought out:
- You're correctly targeting only English documentation files
- You're setting up the spellchecker with terms from the parsed YAML
- You're enabling autofix and merging user-provided options
This provides good flexibility while ensuring consistent spellchecking rules.
87-101: Good job on progress reporting and result aggregation.You're correctly using the logger to indicate the start of linting and providing a summary of the results. The reduction of results to count errors and warnings is clean and efficient.
103-114: Well-implemented result formatting and process exit code.The code effectively loads the "stylish" formatter, formats the results, and correctly sets the process exit code to 1 if errors are found. This follows CLI best practices.
close #11
Summary by CodeRabbit
New Features
Configuration
Chores
Documentation