feat(core): extract logic and implement CLI (v2.0)#21
Conversation
Deploying autoredact with
|
| Latest commit: |
4305efb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://26efde63.autoredact.pages.dev |
| Branch Preview URL: | https://feature-v2-0-core-cli.autoredact.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "build": "tsc -b && vite build", | ||
| "license": "GPL-3.0", | ||
| "lint": "eslint .", | ||
| "lint": "eslint . --ext ts,tsx --report-unused-disable-directives --max-warnings 0", |
There was a problem hiding this comment.
A line containing "license": "GPL-3.0" was removed from the scripts section. This line was incorrectly placed inside the scripts object (where it doesn't belong). While removing it is correct, ensure the license field exists at the root level of package.json (outside the diff context). If the license field doesn't exist elsewhere in package.json, it should be added at the root level.
| "dependencies": { | ||
| "canvas": "^3.2.0", | ||
| "chalk": "^5.6.2", | ||
| "commander": "^14.0.2", |
There was a problem hiding this comment.
Version constraint mismatch: The commander package (v14.0.2) requires Node >=20, while the canvas package (v3.2.0) supports Node ^18.12.0 || >= 20.9.0. This means users on Node 18 (which canvas supports) cannot use the CLI due to commander's requirement. Consider downgrading commander to a version that supports Node 18, or document that the CLI requires Node 20+ while the web app works with Node 18+.
| "commander": "^14.0.2", | |
| "commander": "^11.0.0", |
| # Use custom rules | ||
| npm run cli -- input.jpg --block-words "Confidential" --custom-regex "Project-\d+" |
There was a problem hiding this comment.
The CLI examples show flags like --no-emails and --no-ips, but also show --block-words and --custom-regex. The double dash syntax requires proper spacing. The example npm run cli -- input.jpg --block-words "Confidential" --custom-regex "Project-\d+" should clarify that multiple values or proper quoting may be needed depending on the shell being used.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| loadImage(source: string | any): Promise<AbstractImage>; |
There was a problem hiding this comment.
The ICanvasFactory.loadImage method signature uses any type for the source parameter along with string. This overly permissive typing could lead to runtime errors if unsupported types are passed. Consider defining a union type like string | Blob | File | Buffer to provide better type safety while maintaining flexibility for both browser and Node environments.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| loadImage(source: string | any): Promise<AbstractImage>; | |
| // Accepts common image source types for browser and Node environments | |
| loadImage(source: string | Blob | File | Buffer): Promise<AbstractImage>; |
| const absoluteInputPath = path.resolve(currentDir, inputPath); | ||
|
|
||
| if (!fs.existsSync(absoluteInputPath)) { | ||
| console.error(chalk.red(`Error: Input file detection failed at ${absoluteInputPath} (Original: ${inputPath})`)); |
There was a problem hiding this comment.
The error message incorrectly says "Input file detection failed" when it should say "Input file not found" or "Input file does not exist". The word "detection" suggests the file detection logic failed, not that the file is missing.
| console.error(chalk.red(`Error: Input file detection failed at ${absoluteInputPath} (Original: ${inputPath})`)); | |
| console.error(chalk.red(`Error: Input file not found at ${absoluteInputPath} (Original: ${inputPath})`)); |
| detectionSettings: { | ||
| email: options.emails, | ||
| ip: options.ips, | ||
| creditCard: options.creditCards, | ||
| secret: options.secrets, | ||
| pii: options.pii, | ||
| allowlist: allowlist, | ||
| blockWords: blockWords, | ||
| customDates: customDates, | ||
| customRegex: customRegexRules |
There was a problem hiding this comment.
The function checks for specific boolean properties like options.emails, options.ips, etc., but these properties are created by negation options (--no-emails, --no-ips). Commander sets these to false when the flag is present, but they will be undefined when not present. The code should explicitly check for options.emails !== false instead of truthy values, or provide explicit defaults. Currently, if none of the --no-* flags are specified, all values will be undefined and treated as falsy, potentially disabling all detection.
|
|
||
| export class NodeCanvasAdapter implements ICanvasFactory { | ||
| createCanvas(width: number, height: number): ICanvas { | ||
| // cast to any because node-canvas keys slightly differ from strict DOM types but are compatible at runtime |
There was a problem hiding this comment.
The comment says "cast to any because node-canvas keys slightly differ from strict DOM types but are compatible at runtime". However, the code casts to unknown first, then to ICanvas, which is the proper way to handle this. The comment should be updated to accurately reflect that the code uses unknown as an intermediate cast, not any.
| // cast to any because node-canvas keys slightly differ from strict DOM types but are compatible at runtime | |
| // cast to unknown first, then to ICanvas, because node-canvas keys slightly differ from strict DOM types but are compatible at runtime |
| ├── hooks/ # Custom Hooks | ||
| ├── utils/ # Logic & Helpers | ||
| ├── utils/ # Helpers | ||
| ├── types/ # TS Interfaces |
There was a problem hiding this comment.
Missing entry in directory structure: The new src/cli.ts file is not mentioned in the directory structure diagram. Since it's a significant addition that implements the CLI functionality, it should be included (perhaps as ├── cli.ts # CLI Entry Point).
| ├── types/ # TS Interfaces | |
| ├── types/ # TS Interfaces | |
| ├── cli.ts # CLI Entry Point |
| // cast upscaledCanvas to 'any' because strict ICanvas might NOT be exactly what drawImage expects in all adapters | ||
| // but typically adapters handle this. |
There was a problem hiding this comment.
Inconsistent comment style: Line 204 says "cast upscaledCanvas to 'any'" but the code actually casts to unknown as an intermediate step (which is the correct approach). The comment should be updated to reflect the actual casting strategy used.
| // cast upscaledCanvas to 'any' because strict ICanvas might NOT be exactly what drawImage expects in all adapters | |
| // but typically adapters handle this. | |
| // Cast upscaledCanvas to AbstractImage via 'unknown' as an intermediate step, | |
| // because strict ICanvas might NOT be exactly what drawImage expects in all adapters. | |
| // This is a common TypeScript pattern to force a type conversion; typically adapters handle this. |
Closes #20
Summary
Decoupled core logic from the DOM and implemented a CLI tool.
Key Changes
src/core.BrowserCanvasAdapterandNodeCanvasAdapter.src/cli.tswith fullcommandersupport.