mostalive/pi-tdd-cycle

Fork 0

Willem van den Ende 414140b962 initial commit

2026-06-20 17:02:23 +01:00

12 KiB

Raw Blame History

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:

  // 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:

  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:

  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

  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:

  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.

12 KiB Raw Blame History