Code Review Practices

These are proposed code review guidelines ensure Open Library's code is effective, understandable, maintainable, and secure. They have not yet been ratified by the team.

1. • Passes CI; linters, tests, etc
2. • Strings are internationalized -- js
3. • No debugging or print statements, successfully use logging
4. • The approach is secure; does not expose secret keys, introduce security vulnerabilities, not susceptible to XSS, sql injection, MITM attack, etc.
5. • All code paths are reachable (i.e. no dead code), unless a note is included about why it's being staged.
6. • Meets performance requirements (i.e. author has sufficiently acknowledged edge cases and scale of use). Does not place unreasonable burden on cache, solr, database, web workers, third-party services
7. • No self-merges (unless on a call w/ reviewer and receiving check-off or patch-deploying in case of emergency and no one else reachable -- must leave a comment on PR and slack). Small e.g. bundle fixes that get merged must be annotated and the author must be willing to fix responsibly.
8. • Code is well designed and maintainable: i.e. DRY, documented, well structured, appropriate patterns, is not overly complex, and is understandable.
9. • PR is appropriately scoped and addresses the issue in question, is not a burden on reviewer.
10. • Code functionally works, is testable, and has been tested, e.g. deployed to testing. The author has explained how to test, the reviewer has tested. Changes of incontrovertible consequence (e.g. db modifications, account deletion, bulk imports) should have test cases. Author should test the code and provide evidence. Reviewer should leave a comment about what was or wasn't tested (and why, if applicable).
11. • AI Slop. PRs that seem low-effort and not tested, auto-generated without verification, or not submitted in good faith / with respect for the reviewer's time may be closed at the discretion of staff members. When this is the case, let's note where there may be some portion that is useful or relevant to someone else who may implement the solution, but not spend time getting the PR over the finish line.

Reference Standards:

• Google's Code Review Standard
• GitLab's Code Review Guidelines

While not finalized, this document may be referenced by staff to explain why a PR has been closed.