Side Effects

This learning is based on a real production incident that affected customers.

Let’s examine how to conduct thorough code reviews considering the wider impact of changes. The example here is a real-world example where changes in our gateway service, aka wallets-api, affected the multiple services.

The Scenario

A developer made changes to Circle’s gateway service, aka wallets-api, consolidating the auth context processing. As part of the refactoring, the developer also removed a legacy header that was still being used in some codepaths. This unintended change resulted in the incorrect data being persisted to the database, and ultimately incomplete data were displayed in the customer dashboard. The changes were not caught during code review because:

The engineer did not fully understand the impact of the header.
The existing tests were using a hardcoded value in the legacy header, and did not verify how the header was set depending on the request.
The new tests were not comprehensive enough to verify the headers.
There was no end-to-end testing of the feature that was using the legacy header.

auth.ts


function buildHeaders() {
  const headers = new HttpHeaders();
  headers.append('HeaderKey', 'HeaderValue');
- headers.append('LegacyHeader', 'LegacyValue');
}

PR Comment

Choose the comment that you think is the most constructive and helpful.

Shalne Browncommented on Apr 2, 2025

The changes look good. Approved assuming you’ve tested the header removal.

Rubetta Aquamarinecommented on Apr 2, 2025

The refactoring looks good, but we need to verify that the legacy header is not being used and is safe to remove.

Madelle Blackcommented on Apr 2, 2025

Thanks for implementing the data migration! Before we merge, we should reach out to the team that uses the legacy header to verify that it is safe to remove. If it’s still being used, we should add an integration test to verify that the header is being set correctly, so we can catch any unintended consequences before merge/deployment.

Click here to learn more

Key Lessons

1. Understanding Wider Impact

Consider how changes affect the entire system
Map out downstream effects
Verify changes don’t break existing functionality
Communicate with cross functional teams that might be impacted

2. Testing Strategy

Don’t just update unit tests without understanding why
Include end-to-end testing for critical flows
Test integration points between systems

3. Code Review Best Practices

Both authors and reviewers should understand the wider impact
Ask questions about system-wide impact
Verify test coverage matches the change scope
Be precise about the testing strategy, even if it’s just communication with cross functional teams

Tips for Reviewers

1. Ask System-Wide Questions

How does this change affect the downstream systems?
Are there any downstream effects we should test?

2. Verify Testing Strategy

Are the tests comprehensive enough?
Do they cover all affected systems?
Is there end-to-end testing for critical flows?
Do we need regression tests for the existing systems?

3. Document Dependencies

List systems that might be affected
Document integration points

Common Pitfalls to Avoid

1. Focusing Only on Code

❌ “The code looks good, let’s merge.”
✅ “The code looks good, but let’s verify the header removal doesn’t break the dependent services.”

2. Assuming Tests Are Sufficient

❌ “The tests pass, so it’s good.”
✅ “The tests pass, but let’s verify the end-to-end flow.”

3. Missing Integration Points

❌ “This is just a refactoring, and cleanup of legacy code.”
✅ “This header removal might be affecting multiple systems, let’s reach out to the teams that use it to verify it’s safe to remove.”

Remember: A good code review considers both technical correctness and wider impact. If needed, we should communicate with relevant stakeholders to verify the impact of the changes instead of making assumptions.