All posts by Alberto Gimeno

Rendering huge pull requests in the GitHub Copilot app

Post Syndicated from Alberto Gimeno original https://github.blog/engineering/user-experience/rendering-huge-pull-requests-in-the-github-copilot-app/


Broad refactors and migrations often have to land as one change.

Stacked pull requests are a great way to split work into smaller changes, which makes reviews easier and helps teams ship with less risk. But some changes, like this one, can’t be split cleanly. That leaves you with a single pull request that can get very large, and the review conversation causes it to grow.

The review experience needs to remain fast and smooth even when the diff and its conversation are enormous. In the GitHub Copilot app, we rebuilt the pull request view with that requirement in mind.

To see how far that goes, we opened the biggest pull request we could find: an open source one with 2,200 files, over a million changed lines, and more than 400 inline review comments. Here’s how we made even this extreme pull request performant.

The scope of the problem

Rendering a large diff at speed is well-understood: virtualize the rows, keep the mounted DOM small, and lean on the fact that every row is a line of code at a known height.

Comments are the hard part. A comment’s height depends on how its markdown wraps, the expandable sections, whether there’s a reply box in it, and whether its images have loaded yet. You find all of that out at render time. This forces a different architecture.

Three problems:

  1. Measurement. You can’t know how tall a comment is until you render it. This breaks the design that lets big diffs stay responsive as you scroll.
  2. The data pipeline. A fast diff surface is worthless if the data pipeline feeding it stalls, or if it throws away work it already did.
  3. How we actually found the bugs. These problems surface under load, on a specific engine, at a specific scroll position. So we defined what healthy meant, instrumented the surface to answer it, and ran the whole change → measure → improve loop unattended.

Part 1: Virtualization, and why comments break it

The first step is to understand the geometry that makes a code-only diff fast. Once comments enter the picture, that geometry is no longer enough.

What makes big diffs fast

You cannot put a million DOM nodes on a page. The standard answer is virtualization: mount only the rows that are on screen, plus a small margin, and recycle those same DOM elements as the user scrolls. The list behaves as if all million rows exist. The scrollbar is the right size, scroll-to-row works. But only about 100 rows are ever real at once.

For this illusion to hold, something has to supply the geometry. The scrollbar height is the sum of all row heights. The position of row N is the sum of the heights of the rows above it. Jumping to a row, drawing the scrollbar, deciding what’s on screen, it’s all arithmetic over a table of heights. You can build that table from estimates and correct it as rows get measured, and general-purpose variable-height virtualizers do exactly that.

But if every row is a line of code at a known font size, you don’t have to. You can compute the whole table up front and it never changes, so there’s nothing to correct later.

Call this the “all heights known before paint” contract. Our diff surface is built around it:

  • An imperative, recycled code-row renderer (no React component per row)
  • Typed-array geometry for the offset math
  • Backend-owned diff documents streamed structure-first
  • An imperative scroll API with exact “scroll to row N“

None of it scales badly, because no per-frame work grows with the total row count. On pure code this design is the right one, and we kept all of it.

How comments change the contract

Now put a review thread in the middle of the diff. How tall is it?

You don’t know, and you can’t know without rendering it. Its height depends on things that only exist at render time, and they can keep changing after first paint:

  • Markdown that wraps differently at different widths
  • <details> blocks the user can expand or collapse in place
  • A reply composer that opens inside the existing thread and grows as you type
  • Suggested-change diffs, reactions, edit mode, resolution banners
  • Images and async assets that change height when they finish loading

The obvious answer is to reserve a fixed-height slot for each comment, sized by an estimator. It falls apart on a big pull request. An estimator that’s right on average is still wrong at the extremes. It over-reserves most comments, leaving gaps of whitespace, and under-reserves the expensive ones, which clip or sprout a nested scrollbar. If you measure the real height after paint and write it back into the shared offset table, everything below moves, while the user is already scrolling. That’s a scroll jump, and on a big pull request it’s a large one.

So comments need a different contract. “All heights known before paint” is unachievable for this content. What we could promise instead: heights are bounded, measured lazily, and corrections are small and anchored to whatever the user is looking at.

Two geometries instead of one

The idea that made this tractable was to stop forcing one geometry to serve both kinds of content. We split the document’s height into two independent domains:

total height = deterministic code height          (exact, known up front)
             + Σ dynamic block effective heights   (estimated, then measured)
             + scroll padding

Code geometry keeps the original world. It’s deterministic, prefix-summed, exact, never rebuilt when a comment resizes.

Dynamic block geometry covers everything whose height we can’t predict, such as review threads, drafts, and reply composers. Each one is a block identified by what it is rather than where it currently sits. It has a stable key that survives its content loading, and it’s anchored to a file, line and side rather than to a pixel coordinate, so a reflow can’t lose track of it. We also keep a fingerprint of everything that could change the block’s height: its content, whether a <details> is open, whether a composer is active. And we record the width it was last measured at, rounded into buckets, so an ordinary window resize doesn’t invalidate every measurement in the document.

A block’s effective height is then simple: the measured height if we have a valid one, a cached height if the fingerprint and width still match, and the estimate otherwise. Those heights live in their own index, separate from the code rows, so a resizing comment never forces the code geometry to be rebuilt. And the number of blocks is bounded by comments, not by rows. A few thousand blocks is fine, as long as first paint never mounts or measures all of them at once.

The measurement scheduler, and the mistake we made first

This part took the longest to get right, because our first design was wrong in an instructive way.

The obvious way to measure dynamic content is one ResizeObserver per block, which watches the element and writes its measured height back into the layout whenever it changes. This is what we designed and then rejected during performance hardening. It is the feedback loop that big virtualized surfaces have to avoid. An observer that writes a height back into the layout of the element it’s watching can retrigger itself, and the cost grows with every mounted block.

What shipped instead is a single idle- and scroll-gated measurement pass, held to the same discipline as the deterministic side:

  • Off the hot path. It runs when the visible range settles, never once per scroll frame, and waits entirely while a scroll is in flight. A reflow mid-scroll is exactly the jank we’re avoiding. It runs again once scrolling stops.
  • Scoped to the viewport. Only blocks within roughly 2400px of the viewport are candidates, so the work is O(viewport). Distant blocks keep riding their estimate and get corrected as they approach.
  • On-screen reads win. A mounted block is on screen, so its rendered height is ground truth. The pass reads every mounted candidate in one batch, a single reflow with no writes in between, and records what it finds. A mounted block is never skipped in favor of a stale estimate. That one rule fixed the nastiest bug we hit: comments that rendered with a strip of blank space underneath, because a mounted block had been filtered out of measurement and left sitting on a too-tall estimate.
  • Off-screen measurement is a bounded fallback. For a nearby block that hasn’t mounted yet, the pass does at most one off-screen render, to correct its reservation before it scrolls into view. Blocks taller than the viewport skip even that. Their over-reservation hides below the fold, so the render isn’t worth paying for.
  • An observer catches the rest. Some height changes don’t move the fingerprint and don’t coincide with a scroll: typing in a reply composer, an image finishing loading, toggling a <details>. Each mounted block keeps a ResizeObserver, but by default all it does is flag the block so the idle pass re-reads it. It never writes a height itself, which is what would close the feedback loop we rejected. It disconnects on unmount, and an inactive pull request tab observes nothing.
  • With one deliberate exception. Waiting was visibly wrong for resizes you caused yourself: expanding a <details>, opening a reply composer, an image landing. The block grew immediately, but the code below it only moved on the next idle pass. For one frame the comment was taller while everything under it sat at its old position, and you could see the two steps. So when a block is mounted and on screen, the observer now measures it and applies the correction in the same frame, before paint. The block grows, the code repositions, and everything below shifts together. Two safeguards keep this from becoming the loop we were avoiding: at most one synchronous commit per frame, so a burst of resizes collapses into one, and never during an active scroll, where it falls back to the batched pass.

Scroll anchoring: Correcting without fighting the user

When a measured height differs from its estimate, the scrollbar arithmetic changes, and the naive result is that the viewport jumps. The fix is to correct by identity rather than by pixel:

  1. Before applying height updates, capture what the user is anchored to (a row or a block, by identity), plus the offset within it.
  2. Apply the height deltas.
  3. Resolve that same anchor to its new pixel position.
  4. Scroll so the anchor stays put in the viewport.

Plus a few rules that keep it from feeling wrong:

  • A block above the viewport changing height → adjust by the delta (keeps your place).
  • Content hydrating below the viewport → don’t adjust (you can’t see it).
  • If you toggled a <details> or opened a reply in a visible block → suppress above-block correction for that block, so the interaction feels direct, and let the content below flow down naturally.
  • Never fight active pointer or wheel momentum; batch the correction after the frame.

That last rule has a sharp edge, and it bit us. “Don’t correct while the user is scrolling” was implemented as a guard on the last observed scroll, and programmatic scrolls refreshed that timestamp too. Toggling the file-tree sidebar changes the width of the diff pane. With line wrapping on, every wrapped line above you reflows to a different number of visual lines, the whole coordinate space shifts, and the surface emits a small scroll of its own as it settles. The guard read that as “the user just scrolled” and skipped the very correction that was supposed to keep your place, so the file you were reading drifted off screen. The fix was to tell user scrolls apart from ones the surface caused itself. Any “is the user interacting?” check has to be one your own side effects can’t satisfy.

So corrections stay small, they reuse measurements we already have, and they follow whatever you’re looking at.

Part 2: The pipeline behind the surface

A diff surface can only be as fast as the data feeding it, and three habits from that side of the work shaped what the UI could do. The first is stream structure before content. The diff is requested incrementally, so the file tree and metadata paint while the document is still loading, and the full set of review threads is resolved up front rather than trickling in. The second is defer per-item work until something needs it. Syntax highlighting runs off the main thread, so rows appear as plain text immediately and get colored when the results arrive. Highlighting improves the surface instead of blocking the scroll. Large markdown bodies and suggested-change context work the same way: nothing is built until it approaches the viewport.

The third habit is about which costs are worth keeping. Releasing a diff document when you navigate away is the right default. These documents are large, and holding on to every one you’ve visited is how a long session ends up eating memory. But pull request metadata persists, so the shell around the diff, the header and the file tree, repaints instantly when you go back, and then sits there for several seconds waiting for a diff it had complete moments ago. An instantly-drawn shell around an empty diff looks broken, even though you’re waiting less time overall. So the policy stayed and we added a cache: keep the last few diffs resident, evict anything beyond that, and let the background refresh notice when one has gone stale.

Part 3: The measurement loop, or how we actually found the bugs

Almost every bug in this project was invisible until it wasn’t, and reproducing one by hand is miserable. A typical report reads: “a strip of whitespace appears below some comments, but only sometimes, only on big pull requests, and it heals if you scroll past and back.” You can’t debug that by staring at the screen, so we built tooling to debug it mechanically.

Instrument with the app’s real signals, not throwaway logs

The naive workflow is to sprinkle console.log calls, exercise the flow by hand, copy the output, paste it to someone (or something) that can analyze it, delete the logs, and repeat. It’s slow, it needs a human in the loop, and worst of all you end up measuring your own hand-rolled instrumentation rather than the app’s real behavior.

So the surface carries permanent, structured probes for its own invariants. They’re plain questions it answers about itself on every render:

  • Is the surface actually viewport-bound? How many rows and comment blocks are mounted right now?
  • Is measurement coalescing to a single commit per frame, and how long does that frame take?
  • How large are the scroll corrections we’re making?
  • Did any comment block get inserted after scrolling started? (Must be zero once the backend topology has landed.)
  • Do the per-block observers actually tear down on unmount, or are we leaking one per block?

These are the objective pass/fail signals, and they’re asserted as budgets in an end-to-end test against a synthetic many-comment huge-pull-request fixture. CI can now tell us whether the surface is healthy.

Put the loop on autopilot

The centerpiece was an autonomous change → measure → improve loop. Two lanes:

A headless probe lane ran a declarative flow (open a pull request, scroll to a fraction, toggle a details block, resize the window) against a mock server, reading the app’s own production instrumentation: React render counts, the performance timeline, and a requestAnimationFrame sampler for jank. It did the whole instrument, drive, collect, analyze, rank cycle by itself and printed the bottlenecks in order. Because the flow is just JSON handed to the probe at runtime, an agent could profile any flow by describing it in plain English, without editing a line of source.

An autopilot drove the actual desktop app through the huge-pull-request flow, unattended, on a loop: first cold, with comments still skeletons, then warm, with comments loaded, toggling <details> blocks, opening and cancelling reply composers, collapsing and expanding files, toggling the sidebar tree, sweeping deep into the file list, resizing the window. Every measurement was mirrored to the app’s on-disk log, so an agent could read runtime behavior with nobody at the keyboard. Each sample carried a health signal, and that was the objective check. A warm sample counted as healthy only if there were no unfilled gaps between comments, no comment blocks left blank, and real thread content actually mounted, across the entire scroll range, deep-file sweep included.

The loop we ran was:

  1. Reproduce unattended, on the real engine. Arm the autopilot, let it loop, read the on-disk log.
  2. Detect with a health signal, not with your eyes. Trust the sample fields.
  3. Probe the suspect seam. When a signal goes bad, add one narrow structured probe there, re-arm, re-read. (Editing the surface hot-reloads the live window and re-arms the autopilot, so a fresh capture is about one cycle away.)
  4. Remove the scaffolding. Once you understand the invariant, pin it in a test and the design doc, and keep only the detector-grade signals.

Where this leaves us

Reviewing a pull request this large used to mean one of two things: waiting, or giving up and reading it somewhere else. A review isn’t a document with known dimensions. It’s a conversation that changes shape while you’re reading it, and the surface underneath has to be built for that from the start rather than patched into it afterwards.

The result is a pull request view where a million-line diff with hundreds of threaded comments opens, scrolls, and behaves like a normally sized pull request. Comments render in full instead of clipping into a scrollable box. Expanding a collapsed section moves the code below it and nothing else. Coming back to a pull request you just left puts you where you were.

If you review code for a living, it’s worth feeling the difference on a pull request you already know is painful. Open the worst one you’ve got.

Try the GitHub Copilot app >

The post Rendering huge pull requests in the GitHub Copilot app appeared first on The GitHub Blog.

How we ship code faster and safer with feature flags

Post Syndicated from Alberto Gimeno original https://github.blog/2021-04-27-ship-code-faster-safer-feature-flags/

At GitHub, we’re continually working to improve existing features and shipping new ones all the time. From our launch of GitHub Discussions to the release of manual approvals for GitHub Actions—in order to ship new features and improvements faster while lowering the risk in our deployments, we have a simple but powerful tool: feature flags.

Reducing deployment risk

We deploy changes around the clock, and we need to keep the service running with no disruptions. For these reasons, deploying needs to be an almost risk-free process. Any problem that comes up during a deployment requires a rollback of the code. During this rollback period, users could be impacted and other deployments delayed; the more time that elapses the bigger the impact.

We use feature flags to reduce the risk of deploying to production. Any potentially risky change is put behind a feature flag in the code and then, when the deployment is done, we enable the feature flag to everyone or to a percentage of actors. This way we minimize the impact of the new changes, and if something goes wrong we can disable the feature flag completely in a matter of seconds without interrupting other deployments. This is fundamental to us for the following reasons:

  • It isolates and reduces the deployment risk.
  • We have the ability to disable changes in seconds without rolling back a deployment, which could take minutes.

What kind of things can go wrong during a deployment, where a feature flag can help? These are some examples:

  • Changes in database queries can potentially make existing functionality slower.
  • Changes in the logic on existing features can produce unexpected behavior, edge cases not considered, and more.
  • Changes in how we store or process information, such as saving new columns in the database can accidentally start inserting invalid values or generate too many writes to the database.

Working on new features

Feature flags are not only useful to reduce deployment risk when fixing or improving existing features, we also use them when developing new features!

Using feature flags allows us to work on features incrementally. Only staff members working on the project have the corresponding feature flag enabled, so they can see the new feature that is under development while other users cannot. We don’t use long-lived feature branches. Instead, feature flags allow us to work on small batches, which brings us many benefits:

  • Small batches are easier to review by other engineers in pull requests.
  • The smaller the change, the lesser the chance to get something wrong during the production deployment.
  • Not using long-lived feature branches avoids lots of potential merge conflicts and clashes with other features under development.

In order to adopt this way of building new features, you need to do a little bit of additional planning upfront to divide the work. For example, a new feature typically needs new data models or changes in existing ones. If you create a pull request only for the data model changes, then you may get blocked until those changes are merged. There are a couple of things we usually do to prevent this:

  • Create a main pull request. A spike, where you put the data model changes, and continue working on other areas that require changes: UI, API, background jobs, etc. Then, start extracting those changes to smaller pull requests that your peers can review.
  • Alternatively, you can create an initial pull request, and then create new branches off of that first branch, even if it’s not merged to the main branch yet. If you need to make changes to the first branch, you’ll need to rebase the other branches.

This way of working could introduce some friction due to having to ask for reviews more frequently. Sometimes it can take days until a team reviews your pull request. To prevent this from being a blocker, we follow one or more of these strategies:

Keep working even if the changes are not merged into the main branch. Either on that “spike” pull request or branching off the branch that is waiting for reviews.

Have a first responder person in each team that is focused on reviewing pull requests quickly to unblock the progress of their own team or other teams.
Sometimes if a critical pull request is ready to be deployed sooner rather than later, but has outstanding non-critical feedback waiting to be addressed, we do that in follow up pull requests.

Testing features

We have to make sure that the new functionality works as expected while also maintaining the existing functionality. In order to do that, we have many mechanisms to enable or disable feature flags at all levels as follows:

  • In our development environments, we can toggle feature flags from the command line.
  • In automated tests, we can enable or disable feature flags in the code.
  • In our CI, we have two different builds: one that runs with all feature flags disabled by default, and another one that runs with all feature flags enabled by default. This drastically reduces the chances of not covering most code paths in automated tests properly.
  • In production, we can enable or disable feature flags in the query string of a request.

Different shipping strategies

We already mentioned that we can flag particular actors, or we can enable a feature flag for a percentage of actors. Those are the most common options, but others include:

  • Individual actors: We use this mechanism to flag employees working on a feature or to enable the flag to customers that are experimenting with a feature, or if they have a problem we are trying to fix.
  • Staff shipping: When a feature is almost ready to be released, we first enable it for all GitHub staff, publish an internal post for awareness, and create an internal issue for gathering feedback or possible bugs.
  • Early access maintainers or other beta groups: When we are about to release an important feature that will impact the workflows of open source software (OSS) maintainers or other types of users, we test the feature with a small group first. After some time, we may interview them and gather their feedback to confirm our hypothesis, and validate the implementation.
  • Percentage of actors: As mentioned earlier, we can specify a percentage of actors for which to enable the feature flag. In this case when an actor is flagged, it remains flagged, unless the feature flag is rolled back. When the percentage of actors is changed, a message is sent to our deployments channel on Slack so other engineers are aware of the change.
  • Dark shipping: This allows us to enable the feature flag for a percentage of calls. This is different from the previous mechanism because an actor can get the feature enabled in one call (one request, for example) but not in the next one. This mechanism is not meant for features that are visible to the users, but for internal changes, such as a performance improvement in a query.

All these strategies and the full administration of feature flags is done through a web UI. Not all staff members have access to this UI, but most engineers do. The interface allows us to manage the shipping status of a feature flag, creation of new ones, and deletion. Additionally, we can see a history of changes made to the feature flag: who made the change, when and what changes were made, and other metadata, such as which team owns it.

What to flag

When we say that we can flag an actor, we mean that we can flag not only users, but other entities. In particular, we can flag users, organizations, teams, enterprises, repositories, or GitHub Apps. When writing the code that checks if the flag is enabled or not, we need to specify which actor has the flag enabled or not. These are some examples:

  • If the flag is purely visual, we flag the user, and we check the current logged user. Examples of this could be: dark mode, layout or navigation changes, and new components in the UI that don’t depend on data changes.
  • If the flag changes the way we store data, it is usually better to flag the repository for consistency for all users using the repository. For example, if we start storing timestamps for specific actions, we don’t want some users to produce the new timestamp while others do not.
  • For API changes, we also flag GitHub Apps.
  • Sometimes, especially for some risky changes, we create custom, context-specific actor types, where the existing actor types just wouldn’t have worked.

Sometimes we do more sophisticated checks, such as checking multiple actors or checking if a repository is public or not. For example, when we staff ship a feature it may make a new feature available for all repositories of an employee. If the feature must not be visible to other users, we want to enable the feature only for private repositories to prevent public repositories of an employee leaking the new feature.

The cost of a feature flag

As we have seen, using feature flags extensively changes the way we work, and requires a bit of additional planning and coordination. But there’s also additional costs associated with feature flags that we need to keep in mind. First, we have a runtime cost. When we check if a feature flag is enabled to an actor, we need to first load the metadata information of the flag, which is stored in MySQL but cached in memcached. We are now considering having the feature metadata and the list of actors always in memory to reduce this overhead. There’s an exception for this: we consider some feature flags “large.” An example of this was GitHub Actions, a feature that we rolled out to tens of thousands of users every week. For these large feature flags, when they are still not fully enabled, we need to do a per-actor query to MySQL to check if the actor is in the list of enabled actors.

For runtime, there’s also a cost in the form of technical debt. Once a feature flag is fully enabled, it leaves a lot of dead code and old tests in the repository that need to be deleted. Usually we do this manually after a feature has been rolled out for a few days or weeks depending on the case. But we are now automating the process: we have created a script that can be either invoked locally or by triggering a workflow manually.

The script uses regular expressions to find usages of the feature flag using git grep. Then for the matched Ruby files, it modifies the code using rubocop-ast. It is capable of deleting code blocks, such as if/else statements, and modifying boolean expressions, in addition to reindenting the code afterwards. When the file is a test, it deletes the code that enables the feature flag. For tests that check the behavior of the application when the feature flag is disabled, it just leaves a comment and makes the test fail, since in many cases we don’t want to just delete the test, but simply update it. As a result, even when the script requires some intervention from an engineer, it drastically reduces the manual work required in the process.

When the script is run using a workflow dispatch, it also creates a new branch and a pull request. Since we do extensive use of CODEOWNERS, the pull request will have the proper teams and people as reviewers. In the near future, we want to automate this process even more by running the script automatically when we can consider a feature flag stale.

Conclusion

Feature flags allow us to ship code faster with higher confidence. Using them we reduce deployment risk and make code reviews and development on new features easier, because we can work on small batches. There are also associated costs, but the benefits highly exceed them.