Code Review Guidelines

​

Overview

Code review is a critical part of the Kolibri development process. Reviews help maintain code quality, share knowledge across the team, and ensure that changes align with project standards and goals.

​

Code Review Philosophy

When reviewing PRs, keep feedback focused on critical changes. Lengthy conversations should be moved to a real-time chat when possible. Be polite, respectful, and constructive.

​

General Guidelines for Reviewers

​

Run and Test the PR

Reviewers should actually run and test the PR. Don’t just review the code—verify that it works as intended.

​

Clarify Blocking vs. Non-Blocking Comments

When giving opinions, clarify whether the comment is meant to be a “blocking” comment or if it is just a conversation. This helps the PR author understand what must be addressed before merging.

• Blocking: Must be fixed before merge
• Non-blocking: Suggestions, questions, or future improvements

​

Pre-existing Issues

Pre-existing issues or other cleanup suggestions can be:

• Opened as new issues, or
• Mentioned as “non-blocking” comments

​

Code Formatting

Code formatting comments should be rare because we use Prettier and Black. If you see formatting issues, the author likely didn’t run pre-commit hooks.

​

Code Review Process

A helpful practice in reviewing PRs is to ask yourself questions about what you’re looking at. By raising questions for yourself, you have a goal to accomplish: answer that question.

​

1. Determine the Context

Get an idea of what the PR is trying to accomplish. You don’t have to understand it all yet, but once you have some context, you can review the PR in that lens. Guidelines:

• Review the issue(s) the PR is closing, if applicable
• If you’re still unsure, review the PR description or details
• If you still don’t understand the context, ask a question

​

2. Review the Code

You won’t have a complete picture of how the PR’s code changes work to accomplish the goal within its context until you give a preliminary review of the code. First, review the code in a technical sense looking out for obvious errors or issues:

• Linting issues
• Committed debugging code
• Code that looks buggy

Then, dig deeper with these questions:

​

Code Quality

• Does any code look buggy?
  • Does it have sufficient defensive logic against edge cases?
• Could the code be simplified while still achieving its purpose?
• Are there any worthwhile optimizations?
• Does the code fit within the pattern that we expect?
  • Are there any common conventions the team or codebase has that apply?
• Could the code be made more readable, not necessarily with comments?
• Does the code have sufficient code comments?
• Does the code depart from any familiar structure unnecessarily?

​

Testing

• Are unit tests actually achieving accurate coverage of the code its testing?
  • For instance, do the tests hack something in order to test it?
• Do tests assert behavior rather than implementation?

​

Design Patterns

• Can anything be consolidated or should anything be refactored?
  • Don’t Repeat Yourself (DRY)
  • Single Responsibility Principle (SRP)
• Alternatively, is there any premature abstraction?
  • Rule of Three

​

Feedback Quality

• Are any thoughts you have just opinions or is it worthwhile change?
  • Should your feedback be a blocker?

​

3. Check Completeness

With a once-over of the PR’s code complete, you can start digging deeper. If further changes are required, you may postpone this until those are completed. Now start tracing the usage and interactions of code it’s adding or modifying:

​

Dependencies and Integration

• Does the code have any dependencies, like libraries or packages, that are missing?
  • Does it use features that are missing in the included version?
• Was something refactored, but not all usages also refactored?
  • For example, a function’s arguments were changed, and perhaps one usage of the function was updated, but there exist other usages

​

Technical Debt

• Does the code add any tech debt?
  • Could the debt be eliminated?
  • Is the debt tracked in an issue?

​

API and Data Flow

• Are any interconnects between the code properly handled?
  • Do API calls pass the correct information?
  • Do API calls use the appropriate formatting?
  • Should code use any existing caching?

​

Feature Completeness

• Does it only make sense within a larger featureset or implementation?
  • Will it break development for others if it’s merged?
  • Should it be merged to a feature branch?
• Does the code have any consequences?
  • Is a feature breaking change?
  • Does that breaking change fit within the context?

​

Internationalization and Accessibility

• Is it missing things like strings, internationalization fixes, or accessibility integrations?
  • aria-* attributes?
  • LTR and RTL language support?

​

Security

• What are the security considerations?
  • Do API calls have the appropriate authentication permissions?
  • Could submitted data be mishandled and cause issues?
  • Does the code bypass any known security practices? (e.g., prepared SQL statements)

​

4. Manual Testing

So you’ve reviewed the code and you feel like you understand it. You can see how the PR achieves its goal through the code changes. Now you’re left with whether it actually does achieve it, which can be determined through manual testing.

​

Functionality

• Does something go wrong when you try to test it?
  • Missing dependencies?
• Does it work as intended?
  • Does it fix the issue(s) it closes?
• Can you break it?
  • Any edge cases come to mind?
  • Any odd or unexpected pathways?
  • Anything problematic in the developer tools console or logs?

​

UI and UX

• Are there any cosmetic issues?
  • Does it follow Kolibri Design System guidelines?
  • Is spacing or alignment consistent?
  • Is anything overflowing where/when it shouldn’t?
  • How does it respond to different screen sizes?
  • How does it respond to different use cases? (i.e., within the Android app)
  • Does it break with certain wildcard conditions? (i.e., different content kinds, or learning activities)

​

Accessibility

• Does preliminary review of the accessibility features function as expected?

​

QA Review

• Does the QA team approve?
  • Mention them on the PR, if applicable, so they can do more thorough testing

​

Providing Feedback

​

Be Constructive

• Focus on the code, not the person
• Explain why a change is needed
• Suggest alternatives when possible
• Acknowledge good solutions and clean code

​

Use Clear Language

Examples:

• Blocking: “This will cause a bug when X happens. We need to handle this case.”
• Non-blocking: “Consider extracting this into a helper function for reusability.”
• Question: “Why did we choose approach X over Y? Just trying to understand the tradeoffs.”

​

Respond to Questions

If the PR submitter asks questions or disagrees with feedback:

• Engage in good faith discussion
• Be open to being wrong
• Move complex discussions to real-time chat if they’re getting lengthy

​

Committing to PR Branches

If you see a very trivial but important necessary change, the reviewer can commit the change directly to a pull request branch. This can greatly speed up the process of getting a PR merged, especially for minor fixes like:

• Typos
• Missing commas
• Formatting issues

​

Merging PRs

​

Who Should Merge?

First, all automated checks need to pass before merging. Then:

• If there is just one reviewer and they approve the changes, the reviewer should merge the PR immediately
• If there are multiple reviewers or stakeholders, the last one to approve can merge
• The reviewer might approve the PR, but also request minor changes such as a typo fix or variable name update. The submitter can then make the change and merge it themselves, with the understanding that the new changes will be limited in scope
• Stale reviews should be dismissed by the PR submitter when the feedback has been addressed

​

Resources

• Code Quality Standards
• Testing Guidelines
• Development Workflow
• Unlearning Toxic Behaviors in Code Review Culture