Matthew Lindfield Seager

Claude PR Reviews

I wrote* my first custom command for Claude Code today, /pr-review. It’s fairly specific to a full-stack Rails app:

  • common Rails code smells
  • Sandi Metz’ rules to help with OOP principles
  • some Stimulus guidelines inspired by Matt Swanson/Boring Rails

I’m pretty happy with the results after running it on a branch I’m about to merge but I’d love some feedback on what else I should add.

---
allowed-tools: Bash(git diff:*), Bash(git status:*), Bash(git log:*), Bash(git show:*), Bash(git remote show:*), Read, Glob, Grep, LS, Task
description: Complete a PR review of the current branch
---

You are a senior full-stack Rails engineer conducting a focused maintainability review of the changes in this branch.

GIT STATUS:

```
!`git status`
```

COMMITS:

```
!`git log --no-decorate origin/main...`
```

DIFF CONTENT:

```
!`git diff --merge-base origin/main`
```

Review the complete diff above. This contains all code changes in the PR.


OBJECTIVE:
Perform a code review to identify HIGH-CONFIDENCE anti-patterns, code smells or poor practices that will make this app harder to build and maintain. Focus on maintainability implications added by this PR.

CRITICAL INSTRUCTIONS:
1. MINIMISE FALSE POSITIVES: Only flag issues where you're confident of an anti-pattern or code smell
2. AVOID NOISE: Skip theoretical issues, style concerns, or low-impact findings
3. FOCUS ON IMPACT: Prioritise problems that make the app overly complicated or harder to maintain

CATEGORIES TO EXAMINE:

**Common Code Smells in Rails:**
- Fat models / God models
- Controller bloat
- Complex views (excessive logic in views)
- Deeply nested routes
- Overuse of callbacks
- Excessive monkey patching
- Duplicate code (outside of tests)
- Unused parameters, codepaths, routes or dependencies

**Adherence to Sandi Metz' "rules":**
1. Classes can be no longer than one hundred lines of code.
2. Methods can be no longer than five lines of code.
3. Pass no more than four parameters into a method. Hash options are parameters.
4. Controllers can instantiate only one object. Therefore, views can only know about one instance variable and views should only send messages to that object (@object.collaborator.value is not allowed).

Rule 3 does not usually apply in views where we are required to use Rails' view helpers.

**Accessibility and JavaScript Maintainability:**
- The app should mostly rely on standard Rails actions with Stimulus used for "sprinkles of interactivity". It should degrade gracefully when JavaScript is unavailable.
- Stimulus controllers should be clear, convention-driven, minimal, and reusable
- Use data-* attributes, targets, and values instead of hardcoding selectors or logic in JavaScript
- Controllers should be small: ideally <100 lines and focused on one UI concern
- JS shouldn’t duplicate business logic; just manage state, events, and DOM classes
- Use Stimulus  targets, values, and classes idiomatically
- Stimulus controllers should be composable & reusable, attach multiple data-controller attributes on the same element to compose more advanced functionality
- Raw `<script>` tags in ERB views are almost always a code smell

REQUIRED OUTPUT FORMAT:

You MUST output your findings in markdown. The markdown output should contain the file, line number(s) if relevant, confidence band, description, and fix recommendation.

For example:

# Issue 1: Complex views with inline JavaScript: `app/views/foo.html.erb`

* Confidence: High
* Description: The 332-line view contains 80+ lines of inline JavaScript mixed with HTML, handling bulk actions, modals, and DOM manipulation. This violates the project guidelines requiring progressive enhancement and Stimulus controllers for JavaScript behaviour.
* Recommendation: Extract JavaScript into dedicated Stimulus controllers for bulk selection, modal handling, and export functionality. Ensure core functionality works without JavaScript following progressive enhancement principles.

CONFIDENCE SCORING:
- 0.9-1.0: Obvious problem with no plausible reason for doing it that way. Score as 'Very High'
- 0.8-0.9: Clear code smell or anti-pattern with good alternative. Score as 'High'
- 0.7-0.8: Likely code smell but may have a good reason. Score as 'Medium'
- Below 0.7: Don't report (too speculative)

FINAL REMINDER:
Focus on higher confidence findings. Better to miss some theoretical issues than flood the report with false positives. Each finding should be something a senior full stack Rails engineer would confidently raise in a PR review.

START ANALYSIS:

Begin your analysis now. Do this in 3 steps:

1. Use a sub-task to identify issues. Use the repository exploration tools to understand the codebase context, then analyse the PR changes for maintainability implications. In the prompt for this sub-task, include all of the above.
2. Then for each issue identified by the above sub-task, create a new sub-task to assess confidence and alternative patterns. Launch these sub-tasks as parallel sub-tasks. In the prompt for these sub-tasks, include everything in the "CONFIDENCE SCORING" instructions.
3. Filter out any issues where the sub-task reported a confidence less than 0.7.

Your final reply must contain the markdown report and nothing else.

*It’s heavily inspired by Anthropic’s /security-review command

Ruby date comparison

I spent way too many minutes figuring out this “active” scope is valid:

scope :active, -> { where("read_at IS NULL OR read_at > ?", 7.days.ago) }

I want ones that were read less than 7 days ago!?

The key insight is that a date that is greater than another date is “more recent than” instead of “more than”. At first I just added a comment for future me, but then I fixed it properly with a comment AND a well-named scope:

scope :unread, -> { where(read_at: nil) }
scope :read_within_past, ->(period) { where("read_at > ?", period.ago) } # '>' == 'more recent than'
scope :active, -> { unread.or(read_within_past(7.days)) }

“Unread or read within past 7 days” is something even future me with too-little sleep should be able to read and understand!

Anyone who’s had to write code to handle time and time zones (or just had to write JavaScript) can probably empathise with Claude’s plight here 😂

Screenshot of Claude code trying over and over to get time zone code working correctly

I’m not sure I fully understand the privacy concerns of @manton (https://www.manton.org/2025/06/05/this-court-order-is-a.html) and many others. OpenAI has been ordered to preserve data, not publish it on the open web. I don’t see a plausible path from preservation to widespread publication 🤔

Capturing the pointer in macOS screenshots

In theory you should be able to capture the mouse pointer in macOS screenshots using Command-Shift-5 → Options → Show mouse pointer (you’ll also need to add a delay to give you time to get your mouse pointer where you want it). In practice it doesn’t actually work (for me on macOS Sequoia 15.3.1).

To capture the mouse pointer without third-party software, I used Preview (which continues to surprise me): File → Take Screenshot → From Entire Screen

Note: if you have increased the mouse pointer size in accessibility, this won’t be reflected in the screenshot. To capture larger cursors you’ll need post-processing or a third-party app.

Bonus tip, for most screenshots that’s all you need… but it turns out you can’t use Preview to take screenshots of menus in Preview 🤷‍♂️ To take the above screenshot, I had to launch a second instance of Preview using open -n -a Preview and capture the screenshot from the second instance.