173 lines
12 KiB
Markdown
173 lines
12 KiB
Markdown
|
|
# Asked qwen3.6 35B about code smells
|
|
|
|
Analyze the code in src/ for code smells and improvements.
|
|
|
|
Qwen:
|
|
|
|
# Here's my analysis of the codebase:
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
Code Smells & Improvements
|
|
|
|
### 1. Duplicated Constants (REFACTOR_FOCI)
|
|
|
|
Location: src/state.ts and src/ui.ts
|
|
|
|
Both files define their own copy of REFACTOR_FOCI:
|
|
|
|
```ts
|
|
// state.ts
|
|
export const REFACTOR_FOCI: RefactorFocus[] = [...];
|
|
// ui.ts (line 16)
|
|
const REFACTOR_FOCI: RefactorFocus[] = [...];
|
|
```
|
|
|
|
Fix: Export the constant from state.ts and import it in ui.ts. This is a direct DRY violation.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 2. canTransition is Dead Code
|
|
|
|
canTransition() in state.ts is never called by any tool or event handler. The phase enforcement in index.ts is done via imperative if checks in the go-to-refactor tool and the tool_call event
|
|
handler. The function sits there with full test coverage but zero usage.
|
|
|
|
Options:
|
|
- Delete it if it's not needed — the enforcement logic is already in the tools.
|
|
- Use it in go-to-refactor instead of the manual if/else checks, making the tool logic cleaner and the guard centralized.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 3. redToGreen is Never Called Directly
|
|
|
|
redToGreen() is only called internally inside recordTestResult() — it's never invoked by any tool. It's effectively a private helper, yet it's exported and has its own test suite.
|
|
|
|
Fix: Either make it private (or just not export it) and remove its standalone tests, or keep it exported if you want it as a reusable state-machine primitive.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 4. Magic String "npm test"
|
|
|
|
go-to-green hardcodes const testCommand = "npm test 2>&1"; while run-tests allows a custom command. The go-to-green tool should also accept an optional command parameter for consistency.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 5. persistState / restoreState — Fragile Serialization
|
|
|
|
The state persistence relies on pi.appendEntry / ctx.sessionManager.getEntries() with string-based filtering (e.type === "custom" && e.customType === "tdd-cycle"). This is fragile:
|
|
- No versioning — if the CycleState interface changes, old entries may corrupt state.
|
|
- No deduplication logic beyond .pop() — if multiple entries exist, only the last one is kept (which is correct, but the intent isn't explicit).
|
|
- restoreState is called at the start of every tool execution, meaning state is reloaded on each call — this is fine for a TDD cycle, but worth noting.
|
|
|
|
Suggestion: Add a version field to CycleState and handle version mismatches gracefully.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 6. buildSteeringMessage is a Long Switch
|
|
|
|
This function has 5 if branches that could be replaced with a Record<RefactorFocus, (decision: RefactorDecision) => string> mapping. This would make it data-driven and easier to extend:
|
|
|
|
```ts
|
|
const STEERING_MESSAGES: Record<RefactorFocus, (d: RefactorDecision) => string> = {
|
|
"next-test": () => "...",
|
|
audit: () => "...",
|
|
// etc.
|
|
};
|
|
```
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 7. Repeated ctx.ui.setWidget Calls
|
|
|
|
Every tool and event handler calls ctx.ui.setWidget("tdd-status", ...) with similar patterns. This could be extracted into a helper like updateStatusWidget(ctx) to reduce repetition and ensure
|
|
consistency.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 8. renderResult Type Guards are Repetitive
|
|
|
|
All four tools have the same boilerplate in renderResult:
|
|
|
|
```ts
|
|
const details = result.details as { ... } | undefined;
|
|
if (!details) {
|
|
return new Text(result.content[0]?.type === "text" ? result.content[0].text : "", 0, 0);
|
|
}
|
|
```
|
|
|
|
Fix: Extract this into a shared renderResultFallback helper.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 9. go-to-refactor Non-TUI Fallback is Silent
|
|
|
|
When ctx.mode !== "tui", the tool silently defaults to { focus: "next-test" } without any user confirmation. This could be confusing — the developer has no way to choose a different refactoring
|
|
focus.
|
|
|
|
Suggestion: In non-TUI mode, return the REFACTOR phase status without auto-advancing, and let the developer call the tool again with an explicit parameter.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 10. invalidate in UI Component is a No-Op
|
|
|
|
```ts
|
|
return { render, invalidate: () => {}, handleInput };
|
|
```
|
|
|
|
The invalidate callback does nothing. If the TUI framework expects it to trigger a re-render, this is a bug. If it's intentionally unused, document why.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 11. recordTestResult Returns TransitionResult but Caller Ignores valid/error
|
|
|
|
In index.ts, recordTestResult is called and its state is used, but the valid and error fields are discarded:
|
|
|
|
```ts
|
|
const transition = recordTestResult(state, passed);
|
|
state = transition.state; // valid/error ignored
|
|
```
|
|
|
|
If recordTestResult returns invalid (e.g., called from REFACTOR phase), the state is still updated. The tool should check transition.valid and handle errors.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### 12. go-to-green Hardcodes Timeout
|
|
|
|
Both go-to-green and run-tests use timeout: 60_000. This is fine, but consider making it configurable via a tool parameter for projects with slower test suites.
|
|
|
|
────────────────────────────────────────────────────────────────────────────────
|
|
|
|
### Summary Table
|
|
|
|
┌────┬───────────────────────────────────────────┬──────────┬────────┐
|
|
│ # │ Smell │ Severity │ Effort │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 1 │ Duplicated REFACTOR_FOCI │ Low │ 5 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 2 │ Dead canTransition │ Medium │ 10 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 3 │ Unused export redToGreen │ Low │ 5 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 4 │ Hardcoded "npm test" in go-to-green │ Low │ 5 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 5 │ Fragile state persistence (no versioning) │ Medium │ 20 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 6 │ Long switch → data-driven mapping │ Low │ 10 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 7 │ Repeated setWidget calls │ Low │ 10 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 8 │ Repetitive renderResult guards │ Low │ 10 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 9 │ Silent non-TUI fallback │ Medium │ 15 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 10 │ No-op invalidate │ Low │ 5 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 11 │ Ignored valid/error from recordTestResult │ High │ 10 min │
|
|
├────┼───────────────────────────────────────────┼──────────┼────────┤
|
|
│ 12 │ Hardcoded timeout │ Low │ 5 min │
|
|
└────┴───────────────────────────────────────────┴──────────┴────────┘
|
|
|
|
The highest priority fix is #11 — the recordTestResult return value's valid field is silently ignored, which means invalid state transitions (e.g., recording test results while in REFACTOR phase)
|
|
would silently corrupt state.
|