I Reject More PRs for Boring Mistakes Than for Clever Bugs

I reject more PRs for boring mistakes than for clever bugs.

I lead a team of Flutter developers and I review PRs every day. Here is the uncomfortable truth I keep coming back to: the bugs that reach production are almost never exotic. They are forgotten disposables, missing error states, and widgets rebuilding the whole screen for one changed value.

That is not a criticism of junior developers. It is a criticism of how most of us review. We spend our attention on the interesting parts of a PR and skim the boring parts. The boring parts are exactly where the real bugs hide.

The two wrong ways to review

The first wrong way is hunting for clever bugs and starting an architecture debate on every PR. You can spend an hour arguing about state management patterns while a leaked stream subscription sits three files away, quietly waiting to cause a problem that nobody will connect to this PR.

The second wrong way is skimming and hitting approve because CI is green. CI tells you the code compiles and the tests you wrote pass. It tells you nothing about a controller that was never disposed, a screen that shows a blank page when the list is empty, or a widget tree that rebuilds on every keystroke. Green checks are a floor, not a review.

Both approaches miss the same thing. The boring stuff is what ships bugs.

I review in a fixed order now, and I do not skip steps because a PR looks small. Small PRs cause just as much trouble as large ones, and they cause it faster because everyone assumes they are safe.

1. Disposables cleaned up

Every controller, focus node, and stream subscription that a widget creates must be cleaned up when that widget goes away. If it is not, you get a silent memory leak, or worse, a callback firing on a widget that no longer exists.

This is the first thing I check, because it is the easiest to miss and the hardest to notice in testing. A leaked subscription often works fine in a short manual test. It only becomes visible after a user has navigated back and forth a few times, which is exactly the kind of usage a reviewer never reproduces.

class SearchScreen extends StatefulWidget {
  const SearchScreen({super.key});

  @override
  State<SearchScreen> createState() => _SearchScreenState();
}

class _SearchScreenState extends State<SearchScreen> {
  final TextEditingController _controller = TextEditingController();
  final FocusNode _focusNode = FocusNode();
  StreamSubscription<String>? _subscription;
  String _query = '';

  @override
  void initState() {
    super.initState();
    _subscription = searchStream.listen((value) {
      if (!mounted) return;
      setState(() => _query = value);
    });
  }

  @override
  void dispose() {
    _controller.dispose();
    _focusNode.dispose();
    _subscription?.cancel();
    super.dispose();
  }
}

If a PR creates something that needs disposing, the dispose method should be right there in the same PR. I do not accept "I will clean it up later" for this. Later never comes, and the leak ships in the meantime.

2. Every async flow handles four states

Every async flow has four states: loading, error, empty, and success. Most code I review handles one of them well, the success case, and hopes the other three never happen. They always happen.

A user on a slow connection sees loading for a long time. A user with no connection sees an error. A user with a fresh account sees an empty list. If your screen only looks right when data arrives quickly and in full, it only looks right in your demo.

I want to see all four handled explicitly, even if the handling is simple. A loading indicator, a readable error message with a retry action, a clear empty message that tells the user what to do next, and then the real content. Nothing fancy. Just complete.

Widget build(BuildContext context) {
  if (isLoading) {
    return const Center(child: CircularProgressIndicator());
  }
  if (errorMessage != null) {
    return Center(
      child: Column(
        mainAxisSize: MainAxisSize.min,
        children: [
          Text(errorMessage!),
          TextButton(
            onPressed: loadData,
            child: const Text('Try again'),
          ),
        ],
      ),
    );
  }
  if (items.isEmpty) {
    return const Center(
      child: Text('Nothing here yet. Pull to refresh or add your first item.'),
    );
  }
  return ListView.builder(
    itemCount: items.length,
    itemBuilder: (context, index) => ItemTile(item: items[index]),
  );
}

When I see a PR that only renders the success case, I send it back with one question: what does this screen show when the request fails? If the author cannot answer that from the code, the code is not finished.

3. Mounted checked after every await

This one is small, and it causes an outsized number of crashes. You await a network call, the user navigates away while it is in flight, the call returns, and your code touches context or calls setState on a widget that is already gone.

The rule is simple: after every await, before you touch context, check mounted and return early if the widget is gone.

Future<void> _submit() async {
  setState(() => _isSubmitting = true);
  final result = await api.submitOrder(_order);
  if (!mounted) return;
  setState(() => _isSubmitting = false);
  if (result.ok) {
    Navigator.of(context).pop();
  }
}

That one line, if (!mounted) return;, belongs after every await that is followed by UI work. I check for it mechanically now. If an async method awaits something and then uses context, and there is no mounted check between the two, the PR is not ready. It does not matter how correct the rest of the logic is. A crash on navigation is a crash a user will hit.

4. Rebuild scope

The question here is simple: is this widget rebuilding more than it should?

A common pattern I see is a whole screen rebuilding because one text field changed, or a list rebuilding every item because one item was selected. Users feel that as jank, especially on lower end phones, and developers rarely notice it on a fast test device.

Two habits fix most of it. Use const wherever a widget does not depend on changing data, so Flutter can skip rebuilding it entirely. And split the changing part out into its own small widget, so only that part rebuilds when its value changes.

class OrderScreen extends StatelessWidget {
  const OrderScreen({super.key, required this.order});

  final Order order;

  @override
  Widget build(BuildContext context) {
    return Column(
      children: [
        const OrderHeader(),
        OrderStatusBadge(status: order.status),
        OrderItemsList(items: order.items),
      ],
    );
  }
}

In that example, the header never rebuilds when the status changes, because it is const and it takes no changing input. The status badge rebuilds on its own. The list only rebuilds when its items change. That separation is the whole point: a rebuild should touch the smallest part of the tree that actually changed.

When I review, I look for a build method that does work for the entire screen when only one value moved. If I find it, I ask for the changing piece to be extracted. It is usually a ten minute change, and it is the kind of change that makes the app feel noticeably smoother.

5. No logic hiding in build methods

Build methods should build. They should not fetch data, filter lists, sort results, or compute derived values that could have been computed once.

Logic inside build runs every time the widget rebuilds, which means it runs far more often than the author expects, and it can produce a different result each time if it depends on anything mutable. It also makes the widget impossible to test in isolation, because the logic is tangled up with the UI.

Move the work out. Filter and sort when the data arrives, or in a method that runs on a clear trigger, and let build read the finished result.

class _OrdersState extends State<OrdersScreen> {
  List<Order> _visibleOrders = [];

  @override
  void initState() {
    super.initState();
    _refreshVisibleOrders();
  }

  void _refreshVisibleOrders() {
    final paid = widget.orders.where((o) => o.isPaid).toList();
    paid.sort((a, b) => b.createdAt.compareTo(a.createdAt));
    _visibleOrders = paid;
  }

  @override
  Widget build(BuildContext context) {
    return ListView.builder(
      itemCount: _visibleOrders.length,
      itemBuilder: (context, index) => OrderTile(order: _visibleOrders[index]),
    );
  }
}

Build reads a prepared list. It does not decide which orders count or what order they go in. That decision lives in one place, it runs when it should run, and a test can call it directly.

What I never comment on

I never comment on style. The linter owns style. If the formatting is wrong, the linter should catch it, and if the linter does not catch it, that is a linter config problem, not a PR problem.

That rule matters more than it sounds. Style comments are the easiest comments to write, and they are also the most useless. They fill a review with noise, they slow the author down, and they train the team to treat review as a formatting exercise. I want every comment I leave to be about something the linter cannot see: a leak, a missing state, a crash on navigation, a rebuild that is too broad, logic in the wrong place. If a comment is not about one of those, I delete it before I submit the review.

Five minutes, in order

Five minutes with this checklist catches more real bugs than an hour of architecture debate. Not because architecture does not matter, but because architecture debates happen on PRs that already pass the basics, and most PRs do not pass the basics yet.

Run it in order, every time: disposables cleaned up, four async states handled, mounted checked after every await, rebuild scope kept small, no logic in build. Then, and only then, talk about architecture, if the PR actually needs it.

Most of the time it does not. Most of the time the checklist is the review.

What is on your review checklist that most teams skip?

I'm Sohail Khan, a Senior Full Stack Developer building AI agents and automation. Tech Lead at LoopxCorp. I write about backend engineering, AI integration and practical system design.