Skip to content

refactor: staff engineer cleanup — inline thin wrappers, colocate tests#391

Merged
cmbays merged 1 commit intomainfrom
worktree-rosy-twirling-petal
Mar 17, 2026
Merged

refactor: staff engineer cleanup — inline thin wrappers, colocate tests#391
cmbays merged 1 commit intomainfrom
worktree-rosy-twirling-petal

Conversation

@cmbays
Copy link
Owner

@cmbays cmbays commented Mar 17, 2026

Summary

  • Inline 3 remaining trivially thin wrappers for consistency with the 8 removed in PR refactor: re-inline thin helpers and extract saved-kata CRUD #390
  • Create colocated saved-kata-store.test.ts and remove duplicated tests from execute.test.ts
  • Rename test names that referenced removed hasObservations helper
  • Simplify checkExpiry type guard from double assertion to single cast
  • Fix session-bridge.helpers.test.ts to use path alias per project conventions
  • Add saved-kata-store.test.ts to mutationTestFiles in vitest.test-groups.ts

Verification

  • 3341 unit tests passing
  • 299 integration tests passing
  • Mutation score: 90.92% (up from 90.76%, break threshold: 70%)
  • Typecheck and lint clean

Test plan

  • npm run test:unit — 3341 passing
  • npm test — 299 integration passing
  • npm run test:mutation — 90.92%
  • npm run typecheck — clean
  • npm run lint — clean

Summary by CodeRabbit

  • Tests

    • Added comprehensive test coverage for saved kata storage functionality, including creation, retrieval, validation, and deletion operations.
    • Updated test infrastructure and module resolution paths for consistency.
  • Refactor

    • Simplified internal logic for diary and session recording by removing redundant validation helpers, streamlining the codebase.

Inline shouldRecordBetOutcomes, shouldWriteDojoDiary, and
shouldWriteDojoSession — same trivial-wrapper pattern as the 8
helpers removed in PR #390. Simplify checkExpiry type guard. Fix
test names referencing removed hasObservations helper. Create
colocated saved-kata-store.test.ts and remove duplicated tests
from execute.test.ts. Fix import alias in session-bridge helpers
test. Mutation score holds at 90.92%.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai
Copy link

coderabbitai bot commented Mar 17, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR refactors saved-kata CRUD operations and cooldown helper functions. It moves saved-kata CRUD tests from execute.test.ts to a new dedicated test suite, removes three thin wrapper helpers from cooldown-session, and inlines their logic at call sites. An import path is updated to use namespaced resolution.

Changes

Cohort / File(s) Summary
Saved-kata CRUD extraction
src/cli/commands/execute.test.ts, src/cli/commands/saved-kata-store.test.ts
Removed saved-kata CRUD tests and related imports from execute.test.ts; added comprehensive new test suite for saved-kata-store module covering list, load, save, and delete operations.
Helper function removal
src/features/cycle-management/cooldown-session.helpers.ts, src/features/cycle-management/cooldown-session.helpers.test.ts
Deleted three exported helper functions: shouldRecordBetOutcomes, shouldWriteDojoDiary, and shouldWriteDojoSession; removed corresponding test coverage.
Guard inlining
src/features/cycle-management/cooldown-session.ts
Replaced calls to removed helpers with inline early-return guards that check for empty arrays, missing dojoDir, or missing dojoSessionBuilder.
Test updates
src/features/cycle-management/cooldown-session.unit.test.ts, src/infrastructure/execution/session-bridge.helpers.test.ts, vitest.test-groups.ts
Updated test descriptions in cooldown-session tests for clarity, changed session-bridge import from relative to namespaced path, and added saved-kata-store.test.ts to mutation test files.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 A store for katas, now standing tall and free,
Three helpers retired, their logic inlined with glee,
Guards stand watch where thin functions once did play,
The code flows cleaner now, in its refactored way! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: inlining thin wrapper functions and colocating tests, which are the core refactoring efforts in this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-rosy-twirling-petal
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cmbays cmbays merged commit a2a5fa8 into main Mar 17, 2026
2 of 3 checks passed
@cmbays cmbays deleted the worktree-rosy-twirling-petal branch March 17, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant