refactor: extract duplicate string literals to constants#301
Conversation
- Add SDK Integration section explaining the two Anthropic SDKs - Document detection confidence levels for programmatic detection - Add jscpd, madge, and clean commands to command reference - Fix type guards file path reference (now in detection/ directory) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract duplicated string literals to named constants to improve maintainability and enable single-source-of-truth updates. Changes: - resume.ts: Extract "Resume complete!" to RESUME_COMPLETE_MESSAGE - models.ts: Export MODEL_OPUS_4_5, MODEL_SONNET_4_5, MODEL_HAIKU_4_5 constants for default model IDs - defaults.ts: Use exported model constants from models.ts - schema.ts: Use exported model constants from models.ts - plugin-loader.ts: Extract "needs-auth" to MCP_STATUS_NEEDS_AUTH This resolves all 5 sonarjs/no-duplicate-string ESLint warnings and establishes model ID constants as the single source of truth for default model versions across the codebase. Fixes #299 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Claude finished @sjnims's task in 1m 42s —— View job Code Review Complete ✅Quality ChecksAll quality checks pass successfully:
Verdict: ✅ Looks GoodThis refactoring successfully addresses issue #299 by extracting duplicate string literals to named constants. The changes improve maintainability by creating single sources of truth for repeated values. What's Good
Suggestions1. Consider consolidating the "needs-auth" constant locationThe status: m.authRequired ? "needs-auth" : "pending",Recommendation: Move
This is a minor improvement and not a blocker for this PR. 2. Test coverage considerationThe PR description mentions "N/A - refactoring only" for adding tests. While this refactoring doesn't require new tests, it might be worth verifying that existing tests cover the affected code paths to ensure the constant substitutions work correctly. The test suite passes, which is a good sign. Technical Notes
Overall: This is a clean, well-executed refactoring that improves code maintainability. The extracted constants follow TypeScript best practices and the changes are minimal and focused. Approved! 🚀 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Per PR review suggestion, consolidate the "needs-auth" constant by: - Moving MCP_STATUS_NEEDS_AUTH to src/types/plugin.ts alongside the McpServerStatus type definition (single source of truth) - Exporting via src/types/index.ts for consistent access - Updating Stage 1 (analysis) and Stage 3 (execution) to use the shared constant instead of raw string literals This avoids potential circular dependencies and co-locates the constant with its type definition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Addressed the review suggestion in commit 51801e7: Changes:
This creates a single source of truth for the MCP auth status value, co-located with its type definition, and avoids potential circular dependency issues. All tests passing (1556/1556) ✅ |
|
Claude finished @sjnims's task in 2m 26s —— View job Code Review Complete ✅Todo List
Quality ChecksAll quality checks pass successfully:
Verdict: ✅ Excellent UpdateThe latest commit (51801e7) successfully addresses the review feedback by moving What's Good
Technical NotesConstant Usage Pattern: The refactoring establishes an excellent pattern: // Define in src/types/plugin.ts (alongside the type)
export const MCP_STATUS_NEEDS_AUTH: McpServerStatus["status"] = "needs-auth";
// Export via src/types/index.ts
export { MCP_STATUS_NEEDS_AUTH } from "./plugin.js";
// Import and use everywhere
import { MCP_STATUS_NEEDS_AUTH } from "../../types/index.js";
status: m.authRequired ? MCP_STATUS_NEEDS_AUTH : "pending"This pattern:
Model Constants: The model ID constants ( Resume Message Constant: The SummaryThis PR successfully extracts all 5 duplicate string literals flagged by ESLint into well-named, properly-typed constants. The latest commit perfectly addresses the review feedback by moving the MCP status constant to the types file, creating a maintainable architecture that avoids circular dependencies and establishes a single source of truth for each value. All quality checks pass, tests are green, and the code follows TypeScript best practices. This is a clean, focused refactoring that improves code maintainability without changing any behavior. Approved! 🚀 |
Description
Extract duplicated string literals to named constants to improve maintainability and enable single-source-of-truth updates. This resolves all 5
sonarjs/no-duplicate-stringESLint warnings reported in the issue.Type of Change
Component(s) Affected
Pipeline Stages
src/stages/3-execution/)Core Infrastructure
src/cli/)src/config/)Other
tests/)CLAUDE.md,README.md)config.yaml,eslint.config.js,tsconfig.json, etc.).github/)Motivation and Context
ESLint reported 5 warnings for duplicate string literals that should be extracted to constants. This improves:
Fixes #299
Changes Made
src/cli/commands/resume.ts"Resume complete!"toRESUME_COMPLETE_MESSAGEconstantsrc/config/models.tsMODEL_OPUS_4_5,MODEL_SONNET_4_5,MODEL_HAIKU_4_5constants for default model IDssrc/config/defaults.tsmodels.tssrc/config/schema.tsmodels.tssrc/stages/3-execution/plugin-loader.ts"needs-auth"toMCP_STATUS_NEEDS_AUTHconstantHow Has This Been Tested?
Test Configuration:
Test Steps:
npm run lint- Verified all 5 duplicate string warnings are resolved ✅npm run typecheck- TypeScript compilation passes ✅npm run format- Code formatting is consistent ✅npm run knip- No dead code detected ✅npm run madge- No circular dependencies ✅npm test- All 1556 tests pass ✅Checklist
General
TypeScript / Code Quality
npm run typecheck)_anytypes without justificationDocumentation
Linting
npm run lintand fixed all issuesnpm run format:checkmarkdownlint "*.md"on Markdown files (N/A - no markdown changes)uvx yamllint -c .yamllint.ymlon YAML files (N/A - no YAML changes)actionlinton workflow files (N/A - no workflow changes)Testing
npm testand all tests passReviewer Notes
Areas that need special attention:
models.tsand imported bydefaults.tsandschema.ts. This creates a single source of truth for default model versions.Known limitations or trade-offs:
pricing.tsandbatch-calculator.tsstill use raw model ID strings as object keys. Thesonarjs/no-duplicate-stringrule only flagged duplicates within the same file, so these were not modified.🤖 Generated with Claude Code