docs/maintainers/PR_REVIEW_GUIDE.md
This guide is for NOFX maintainers reviewing pull requests.
Check PR alignment with roadmap
Verify PR completeness
Apply appropriate labels
Assign reviewers
β
**Questions to Ask:**
- Does it solve the stated problem?
- Are edge cases handled?
- Will this break existing functionality?
- Is the approach correct for our architecture?
- Are there better alternatives?
Testing:
Go Backend Code:
// β Bad - Reject
func GetData(a, b string) interface{} {
d := doSomething(a, b)
return d
}
// β
Good - Approve
func GetAccountBalance(apiKey, secretKey string) (*Balance, error) {
if apiKey == "" || secretKey == "" {
return nil, fmt.Errorf("API credentials required")
}
balance, err := client.FetchBalance(apiKey, secretKey)
if err != nil {
return nil, fmt.Errorf("failed to fetch balance: %w", err)
}
return balance, nil
}
Check for:
TypeScript/React Frontend Code:
// β Bad - Reject
const getData = (data: any) => {
return data.map(d => <div>{d.name}</div>)
}
// β
Good - Approve
interface Trader {
id: string;
name: string;
status: 'running' | 'stopped';
}
const TraderList: React.FC<{ traders: Trader[] }> = ({ traders }) => {
return (
<div className="trader-list">
{traders.map(trader => (
<TraderCard key={trader.id} trader={trader} />
))}
</div>
);
};
Check for:
any unless absolutely necessary)Critical Checks:
// π¨ REJECT - Security Issue
func Login(username, password string) {
query := "SELECT * FROM users WHERE username='" + username + "'" // SQL Injection!
db.Query(query)
}
// β
APPROVE - Secure
func Login(username, password string) error {
query := "SELECT * FROM users WHERE username = ?"
row := db.QueryRow(query, username) // Parameterized query
// ... proper password verification with bcrypt
}
Use these criteria to assign priority:
Critical:
High:
Medium:
Low:
needs review β in review β needs changes β needs review β approved β merged
β
on hold
Status Labels:
status: needs review - Ready for initial reviewstatus: in progress - Being actively reviewedstatus: needs changes - Reviewer requested changesstatus: on hold - Waiting for discussion/decisionstatus: blocked - Blocked by another PR/issueβ Bad Comments:
This is wrong.
Change this.
Why did you do this?
β Good Comments:
This approach might cause issues with concurrent requests.
Consider using a mutex or atomic operations here.
Suggestion: Extract this logic into a separate function for better testability:
```go
func validateTraderConfig(config *TraderConfig) error {
// validation logic
}
Question: Have you considered using the existing ExchangeClient interface
instead of creating a new one? This would maintain consistency with the rest
of the codebase.
### Comment Types
**π΄ Blocking (must be addressed):**
```markdown
**BLOCKING:** This introduces a SQL injection vulnerability.
Please use parameterized queries instead.
π‘ Non-blocking (suggestions):
**Suggestion:** Consider using `strings.Builder` here for better performance
when concatenating many strings.
π’ Praise (encourage good practices):
**Nice!** Great use of context for timeout handling. This is exactly what
we want to see.
β Directive (can feel demanding):
Change this to use the factory pattern.
Add tests for this function.
β Question (more collaborative):
Would the factory pattern be a better fit here? It might make testing easier.
Could you add a test case for the error path? I want to make sure we handle
failures gracefully.
| PR Type | Initial Review | Follow-up | Merge Decision |
|---|---|---|---|
| Critical Bug | 4 hours | 2 hours | Same day |
| Bounty PR | 24 hours | 12 hours | 2-3 days |
| Feature | 2-3 days | 1-2 days | 3-5 days |
| Documentation | 2-3 days | 1-2 days | 3-5 days |
| Large PR | 3-5 days | 2-3 days | 5-7 days |
A PR should be approved when:
Functionality
Quality
Security
Documentation
Process
Reject a PR if:
Immediate Rejection:
Request Changes:
Close with Explanation:
Extra care needed:
Merge when:
Squash Merge (default for most PRs):
Merge Commit (for complex PRs):
Rebase and Merge (rarely):
Format:
<type>(<scope>): <PR title> (#123)
Brief description of changes.
- Key change 1
- Key change 2
Co-authored-by: Contributor Name <[email protected]>
Monitor these metrics monthly:
If unsure about a PR:
Remember: Reviews should be respectful, constructive, and educational. We're building a community, not just code. π