Skip to content

Commit 1578a0b

Browse files
authored
Merge pull request #4305 from anyproto/ios-5068-release-to-main
Ios 5068 release to main
2 parents 96dd049 + 08ed9e9 commit 1578a0b

File tree

937 files changed

+15787
-5587
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

937 files changed

+15787
-5587
lines changed

.claude/CODE_REVIEW_GUIDE.md

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
# Code Review Guide
2+
3+
Comprehensive 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)