Skip to content

Commit a154791

Browse files
ignatovvignatovv
committed
IOS-5364 Create shared review guidelines and local code review command
1 parent 395a3f5 commit a154791

File tree

3 files changed

+187
-123
lines changed

3 files changed

+187
-123
lines changed

‎.claude/commands/codeReview.md‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Code Review Command
2+
3+
Review local code changes on the current branch against the base branch (develop).
4+
5+
## Usage
6+
Run `/codeReview` on your current branch to review changes against develop.
7+
8+
## Process
9+
10+
### Step 1: Gather Current Branch Context
11+
Get the current branch name, list of changed files, and full diff against develop:
12+
13+
```bash
14+
# Get current branch and changes summary
15+
git branch --show-current
16+
git diff --name-status develop...HEAD
17+
git diff --stat develop...HEAD
18+
19+
# Get full diff for review
20+
git diff develop...HEAD
21+
```
22+
23+
### Step 2: Apply Review Standards
24+
25+
**Apply the shared review guidelines from:**
26+
`.github/workflows/code-review-guidelines.md`
27+
28+
Follow all core rules, review sections, common mistakes, and analysis checklist defined in that file.
29+
30+
**Context adaptation for local reviews:**
31+
- This is a local review (not a GitHub PR)
32+
- Reference file:line locations from the git diff
33+
- Focus on changes between current branch and develop
34+
- Output review directly (no need to post comments)
35+
36+
### Step 3: Review Focus Areas
37+
38+
When analyzing the diff, pay special attention to:
39+
40+
1. **Migration to @Observable**: Check that all properties are properly annotated and dependencies marked with `@ObservationIgnored`
41+
2. **Localization**: Ensure no hardcoded strings in UI, all text uses `Loc.*` constants
42+
3. **Feature Flags**: New features should be wrapped in `FeatureFlags.*` checks
43+
4. **Generated Files**: Never manually edit files marked with `// Generated using Sourcery/SwiftGen`
44+
5. **Tests & Mocks**: When refactoring, ensure tests and mocks are updated
45+
6. **Unused Code**: After refactoring, check that old code is removed
46+
7. **Comments**: Only add if explicitly needed (per CLAUDE.md guidelines)
47+
48+
### Step 4: Output Review
49+
50+
Present the review following the summary format from the shared guidelines.
51+
52+
Include detailed findings for each issue category only if issues are found.

‎.github/workflows/claude-code-review-prompt.md‎

Lines changed: 10 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
1-
# Claude Code Review Prompt
1+
# Claude Code Review Prompt (GitHub Actions)
22

33
## Context Variables
44
- **REPO**: Repository name (injected by workflow)
55
- **PR NUMBER**: Pull request number (injected by workflow)
66
- **COMMIT SHA**: Commit SHA being reviewed (injected by workflow)
77

8-
## Reference Documentation
8+
## Review Standards
9+
10+
**Apply the shared review guidelines from:**
11+
`.github/workflows/code-review-guidelines.md`
12+
13+
Follow all core rules, review sections, common mistakes, and analysis checklist defined in that file.
14+
15+
## CI-Specific Reference Documentation
916

1017
### Valid GitHub Actions Runners
1118
https://github.com/actions/runner-images/tree/main/images/macos
@@ -16,44 +23,6 @@ https://github.com/actions/runner-images/tree/main/images/macos
1623

1724
**VALIDATION RULE**: When reviewing xcode-version in workflows, verify it matches the runs-on runner version using the mapping above.
1825

19-
## Review Instructions
20-
21-
Review this PR using CLAUDE.md for project conventions. Be LEAN and ACTIONABLE - only provide value, avoid noise.
22-
23-
### Core Rules
24-
- ONLY include sections when there are ACTUAL issues to report
25-
- NO "Strengths" or praise sections
26-
- NO "no concerns" statements (skip the section entirely)
27-
- NO design/UI/spacing suggestions (padding, margins, colors, etc.) - you cannot see the visual design
28-
- Reference specific file:line locations for issues
29-
- **If no issues found**:
30-
- Comment ONLY: "✅ **Approved** - No issues found"
31-
- DO NOT describe what the PR does
32-
- DO NOT list changes made
33-
- DO NOT provide any summary or explanation
34-
- Zero noise, zero fluff - just the approval statement
35-
36-
### Review Sections
37-
Include ONLY if issues exist:
38-
39-
#### Bugs/Issues
40-
Logic errors, potential bugs that need fixing
41-
42-
#### Best Practices
43-
Violations of Swift/SwiftUI conventions or CLAUDE.md guidelines (code quality only, not design)
44-
45-
#### Performance
46-
Actual performance problems (not theoretical)
47-
48-
#### Security
49-
Real security vulnerabilities
50-
51-
### Summary Format
52-
End with ONE sentence only with status emoji:
53-
- ✅ **Approved** - [brief reason]
54-
- ⚠️ **Minor Issues** - [what needs fixing]
55-
- 🚨 **Major Issues** - [critical problems]
56-
5726
## Review Comment Strategy
5827

5928
### 1. Small, Localized Issues (Preferred)
@@ -138,88 +107,6 @@ The \`attentionDotView\` overlay positioning is incorrect...
138107
**Important**:
139108
- Use single quotes to wrap multi-line review text if needed
140109
- Escape special characters appropriately for bash
141-
- Always include the status emoji summary at the end
110+
- Always include the status emoji summary at the end (as defined in shared guidelines)
142111
- The workflow provides ${PR_NUMBER} and ${REPO} variables
143112

144-
## Common Analysis Mistakes to Avoid
145-
146-
### Mistake: Assuming Unused Code After UI Element Removal
147-
148-
**Scenario**: A PR removes a visible UI element (e.g., a menu button) but leaves related parameters in the API.
149-
150-
**Wrong Analysis**:
151-
- ❌ "Parameter is unused, should be removed"
152-
- ❌ "Remove all related infrastructure"
153-
- ❌ Not checking where else the parameter is used
154-
155-
**Correct Approach**:
156-
1. **Trace all usages** of parameters/closures before declaring them unused
157-
2. **Understand dual UX patterns**: iOS commonly uses button + long-press for same actions
158-
3. **Check for multiple consumers**: A closure/parameter may serve multiple UI patterns
159-
160-
**Example - Menu Button + Context Menu Pattern**:
161-
```swift
162-
// Component accepts menu closure
163-
struct Widget<MenuContent: View> {
164-
let menu: () -> MenuContent
165-
166-
var body: some View {
167-
content
168-
.toolbar {
169-
// Visible menu button
170-
Menu { menu() } label: { Image(...) }
171-
}
172-
.contextMenu {
173-
// Long-press menu (same content!)
174-
menu()
175-
}
176-
}
177-
}
178-
```
179-
180-
**Analysis**:
181-
- Removing the toolbar menu button does NOT make `menu` parameter unused
182-
- The `menu()` closure is still actively used by `.contextMenu`
183-
- Both are valid access patterns for the same functionality
184-
185-
**Key Questions to Ask**:
186-
- Where else is this parameter/closure called in the file?
187-
- Is this a dual-access pattern (button + long-press)?
188-
- Was the removal intentional (UX change) or accidental?
189-
- Are there separate flags controlling each access method?
190-
191-
### Mistake: Not Understanding Conditional Rendering Flags
192-
193-
**Scenario**: A component has multiple boolean flags like `allowMenuContent` and `allowContextMenuItems`.
194-
195-
**Wrong Analysis**:
196-
- ❌ "These flags serve the same purpose, consolidate them"
197-
- ❌ Not recognizing they control different UI elements
198-
199-
**Correct Approach**:
200-
1. Each flag controls a specific UI element/pattern
201-
2. `allowMenuContent`: Controls visible button
202-
3. `allowContextMenuItems`: Controls long-press menu
203-
4. They can be independently enabled/disabled
204-
205-
**Example**:
206-
```swift
207-
// Widget with independent menu controls
208-
LinkWidgetViewContainer(
209-
allowMenuContent: false, // No visible button
210-
allowContextMenuItems: true, // Long-press still works
211-
menu: { MenuItem() } // Used by context menu only
212-
)
213-
```
214-
215-
### Analysis Checklist
216-
217-
Before suggesting removal of "unused" code:
218-
- [ ] Searched ALL usages in the file
219-
- [ ] Checked for dual UX patterns (button + context menu)
220-
- [ ] Understood purpose of each boolean flag
221-
- [ ] Verified it's not used by multiple consumers
222-
- [ ] Asked clarifying questions about design intent
223-
224-
If unsure, ask:
225-
> "Was removing [UI element] intentional? The [parameter] is still used by [other pattern]. Should we keep both access methods or restore the [UI element]?"

‎.github/workflows/code-review-guidelines.md‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
# Code Review Guidelines
2+
3+
Shared review standards for both local reviews and automated CI reviews.
4+
5+
## Core Review Rules
6+
7+
Review using CLAUDE.md for project conventions. Be LEAN and ACTIONABLE - only provide value, avoid noise.
8+
9+
- ONLY include sections when there are ACTUAL issues to report
10+
- NO "Strengths" or praise sections
11+
- NO "no concerns" statements (skip the section entirely)
12+
- NO design/UI/spacing suggestions (padding, margins, colors, etc.) - you cannot see the visual design
13+
- Reference specific file:line locations for issues
14+
- **If no issues found**:
15+
- Comment ONLY: "✅ **Approved** - No issues found"
16+
- DO NOT describe what the changes do
17+
- DO NOT list changes made
18+
- DO NOT provide any summary or explanation
19+
- Zero noise, zero fluff - just the approval statement
20+
21+
## Review Sections
22+
23+
Include ONLY if issues exist:
24+
25+
### Bugs/Issues
26+
Logic errors, potential bugs that need fixing
27+
28+
### Best Practices
29+
Violations of Swift/SwiftUI conventions or CLAUDE.md guidelines (code quality only, not design)
30+
31+
### Performance
32+
Actual performance problems (not theoretical)
33+
34+
### Security
35+
Real security vulnerabilities
36+
37+
## Summary Format
38+
39+
End with ONE sentence only with status emoji:
40+
- ✅ **Approved** - [brief reason]
41+
- ⚠️ **Minor Issues** - [what needs fixing]
42+
- 🚨 **Major Issues** - [critical problems]
43+
44+
## Common Analysis Mistakes to Avoid
45+
46+
### Mistake: Assuming Unused Code After UI Element Removal
47+
48+
**Scenario**: A PR removes a visible UI element (e.g., a menu button) but leaves related parameters in the API.
49+
50+
**Wrong Analysis**:
51+
- ❌ "Parameter is unused, should be removed"
52+
- ❌ "Remove all related infrastructure"
53+
- ❌ Not checking where else the parameter is used
54+
55+
**Correct Approach**:
56+
1. **Trace all usages** of parameters/closures before declaring them unused
57+
2. **Understand dual UX patterns**: iOS commonly uses button + long-press for same actions
58+
3. **Check for multiple consumers**: A closure/parameter may serve multiple UI patterns
59+
60+
**Example - Menu Button + Context Menu Pattern**:
61+
```swift
62+
// Component accepts menu closure
63+
struct Widget<MenuContent: View> {
64+
let menu: () -> MenuContent
65+
66+
var body: some View {
67+
content
68+
.toolbar {
69+
// Visible menu button
70+
Menu { menu() } label: { Image(...) }
71+
}
72+
.contextMenu {
73+
// Long-press menu (same content!)
74+
menu()
75+
}
76+
}
77+
}
78+
```
79+
80+
**Analysis**:
81+
- Removing the toolbar menu button does NOT make `menu` parameter unused
82+
- The `menu()` closure is still actively used by `.contextMenu`
83+
- Both are valid access patterns for the same functionality
84+
85+
**Key Questions to Ask**:
86+
- Where else is this parameter/closure called in the file?
87+
- Is this a dual-access pattern (button + long-press)?
88+
- Was the removal intentional (UX change) or accidental?
89+
- Are there separate flags controlling each access method?
90+
91+
### Mistake: Not Understanding Conditional Rendering Flags
92+
93+
**Scenario**: A component has multiple boolean flags like `allowMenuContent` and `allowContextMenuItems`.
94+
95+
**Wrong Analysis**:
96+
- ❌ "These flags serve the same purpose, consolidate them"
97+
- ❌ Not recognizing they control different UI elements
98+
99+
**Correct Approach**:
100+
1. Each flag controls a specific UI element/pattern
101+
2. `allowMenuContent`: Controls visible button
102+
3. `allowContextMenuItems`: Controls long-press menu
103+
4. They can be independently enabled/disabled
104+
105+
**Example**:
106+
```swift
107+
// Widget with independent menu controls
108+
LinkWidgetViewContainer(
109+
allowMenuContent: false, // No visible button
110+
allowContextMenuItems: true, // Long-press still works
111+
menu: { MenuItem() } // Used by context menu only
112+
)
113+
```
114+
115+
## Analysis Checklist
116+
117+
Before suggesting removal of "unused" code:
118+
- [ ] Searched ALL usages in the file
119+
- [ ] Checked for dual UX patterns (button + context menu)
120+
- [ ] Understood purpose of each boolean flag
121+
- [ ] Verified it's not used by multiple consumers
122+
- [ ] Asked clarifying questions about design intent
123+
124+
If unsure, ask:
125+
> "Was removing [UI element] intentional? The [parameter] is still used by [other pattern]. Should we keep both access methods or restore the [UI element]?"

0 commit comments

Comments
 (0)