Building Culture and Community with Code Reviews

Code review is at the heart of what we do at Cesium. In this blog we discuss core principles around our review process and introduce our code review guide, which we recently created and made available in our new public community repo. At Cesium we maintain multiple open source repositories, most notably our runtime libraries for visualizing geospatial graphics on the web (CesiumJS) and via native engines (Cesium Native, Cesium for Unreal, Cesium for Unity, and Cesium for Omniverse), and since 2012 we have been making monthly releases of these libraries. CesiumJS alone has close to 250,000 lines of source code and 280,000 lines of test code. We also maintain Cesium ion SaaS and Cesium ion Self-Hosted, platforms for tiling, hosting, and streaming 3D geospatial data as 3D Tiles. To top it off, Cesium projects are built collaboratively between the community and the core team, with more than 300 external contributors committing source code to CesiumJS and dozens contributing commits to our native runtimes. 

Maintaining projects at this scale and in open source is not possible without strong engineering processes. Pull requests (PRs) and code reviews—more than most any other engineering process—are where rules are enforced and culture is built. Cesium has structured our guide to reinforce the core values of our engineering teams: Enhance code quality and consistency, streamline collaboration, facilitate knowledge sharing, and foster shared ownership. You may notice our guide doesn’t drill deep into language and framework-specific practices; we designed it as an overarching framework that can be augmented with specific tooling or style guides for our individual projects.

Here are the key principles at work in our guide. 

Tone: Seek great code and motivated people

We often say, "comments are about the code, not the people." We believe review comments are positive opportunities to discuss and improve. Our guide suggests using guiding questions and presenting evidence. Questions invite discussion and are a more gentle way to suggest changes. Every suggestion should be able to link to an authoritative source, like the project's coding guide, rather than relying on the reviewer's opinion. Empathy on the reviewer's part can go a long way toward keeping the contributor motivated in the review process.

Organize branches and commits 

Code review cannot happen without well organized projects, and we lean heavily on Git to organize our engineering projects. Git branches and branch protection rules enable collaboration and help us avoid forcing work in progress upon others before it's ready. Our guide advises keeping branches as focused as possible, avoiding mixing large-scale, cross-cutting changes with other changes. We suggest breaking complex changes into a sequence of smaller pull requests, each of which builds upon the previous. Lastly, we caution to avoid rebasing to preserve a usable version history.

Open timely and scoped PRs that create a historical record

We believe PRs should be small, targeted, and atomic. The PR author should create tasks lists covering and linking to all additional work required before merging. Team members should start code review and respond promptly (within 24 hours) to maintain momentum and investment from the author. Pull requests serve as a historical record of a change, so it is important to document the context of the pull request to help preserve it for future reference. The author should include why the change is being made and the test plan with steps and configuration needed to verify the change. Linking is an effective tool for documenting context: We create links to relevant issues (in the current repository or other projects), community forum posts or support threads, standards or API documentation, changelogs for dependencies, and best practices, such as blogs or guides.

Communicate status with automation, CI, and GitHub tooling

No matter the project, our guide advises using Github Actions workflows as part of continuous integration (CI) processes to enforce style guidelines through linting, code quality through unit tests and code scanning, and security through secrets checking and vulnerability scanning. The specific implementations of these checks depend entirely on the language and other features of the project, but automating them, as our guide recommends, streamlines and strengthens the code review process. The reviewer should ensure all checks pass and check artifacts for each stage to ensure that there are no “silent failures.”

Key responsibilities for reviewers

Reviewers should maintain velocity by being concise and direct in their comments, avoiding the temptation to expand scope of the PR, and always trying to level up the team by providing impactful feedback that will then be sought after by other engineers. Our guide provides an overview of the critical areas a reviewer should look at before approving a PR, including design, execution, performance, complexity, third-party libraries, style, tests, and documentation of the code changes. 

Approving and merging a pull request means that the reviewer is accepting some responsibility for the code being merged. The author of the code is still largely accountable for any critical issues that immediately arise, but the reviewer shares some accountability as well. In practice, reviews differ by level of toughness depending on the change, which we thought was valuable to break as follows:

• Rubber stamp: suitable for pull requests that only deal with documentation.
• Skim the code and ensure all CI automation is passing: suitable for small pull requests.
• Compile locally and run the code: suitable for most pull requests.
• Deep read of the code changes: suitable for most large pull requests.
• Synchronously review one-to-one with the author: suitable for giant pull requests.

Lastly, reviews should be public. If the reviewer and author meet in a video call or in a room, the reviewer should summarize the discussion in a PR comment. This helps everyone looking at the PR later and ensures we are transparent about the life of the PR.

Carefully review updates to public APIs

Our guide calls out important steps embedded in our code review process for maintaining consistency and usability of our public APIs. Changelogs and release notes should provide clear and open communication about project updates, while also managing user expectations and promoting new features. The code review should ensure the PR meets the following expectations critical for public facing APIs:

• Document changes in the changelog.
• Update documentation.
• Have a versioning strategy: If the changes are significant or potentially break backward compatibility, verify that the project is prepared for a version bump in accordance with its versioning strategy, e.g., according to semantic versioning principles.
• Highlight deprecations and breaking changes: Clearly mark any deprecations or breaking changes in the PR description.

Reject PRs respectfully

It is sometimes necessary to reject a pull request, in particular to maintain the project’s scope. When doing so, first begin with thanks and acknowledge the contributor’s needs as valid, even if the contribution doesn’t align with the project’s goals. Then be clear and constructive about why the PR does not fit with the project. Following that, present the author with ways out, such as a workaround or forking the repo. And finally, encourage them to contribute again in the future.  

Our code review guide is a living document that can be edited and expanded as needed, so it can and will change over time. Cesium has designed our guide to be a forward-looking overarching framework, which can then be implemented as specific guides for individual projects. 

We invite anyone to open issues with feedback or submit PRs with suggestions about our new guide in our community repo, which houses the guide on GitHub.