React Code Review - Unraveling A Tightly Coupled Component & Mixed Responsibilities (Incl Video)

This blog post is a code review of a pull request from a student of the React Job Simulator.

We’ll see how the filter component in the screenshot below is tightly coupled to the pagination of its parent component.

After the code review, we’ll refactor the component to unravel this coupling so each component focuses on its own responsibilities.

Table Of Contents

1. The Task
2. The Code
3. Tight Coupling Of The Filter Component To Its Parent
4. Refactoring The Filter Component
5. Refactoring The Parent Component
6. Summary

If you'd rather watch the video version of this code review here you go.

The Task

The page in the screenshot above shows a list of issues. This list is paginated, meaning it only shows some of the issues immediately. The user needs to navigate to the next page of the list to see the next set of issues.

The student's task here was to implement the filter at the top.

The filter has three fields. Two select inputs, so the user can filter for a certain issue status and level, plus a text input that allows filtering for a project name.

The Code

This is the Filter component as it was implemented by my student.

import { useEffect, useState } from "react";
import { Level, Status } from "@api/issues.types";
import { useFilter } from "../context/filter-context";
import styles from "./filter.module.scss";

interface FilterProps {
  navigateToPage: (
    newPage: number,
    status: Status,
    Level: Level,
    projectName: string,
  ) => void;
}

export function Filter({ navigateToPage }: FilterProps) {
  const { setStatus, setLevel, setProjectName, status, level, projectName } =
    useFilter();
  const [name, setName] = useState(projectName);

  useEffect(() => {
    if (projectName === "") setName("");
  }, [projectName]);

  return (
    <form
      className={styles.filter}
      onSubmit={(e) => {
        e.preventDefault();
        if (setStatus && setLevel && setProjectName) {
          setStatus(status);
          setLevel(level);
          setProjectName(projectName);
        }
        navigateToPage(1, status, level, name);
      }}
    >
            {/* THE FILTER INPUTS */}
        </form>
    );
}

The code looks solid. Let’s start dissecting it step by step.

const { setStatus, setLevel, setProjectName, status, level, projectName } =
    useFilter();

The component uses a custom hook that returns the filter values and allows updating them. This is a great approach as all the logic for handling the filters is encapsulated in this hook. The component doesn’t care whether the values are stored in a local state, context, or the URL.

Let’s move to the next lines:

const [name, setName] = useState(projectName);

useEffect(() => {
  if (projectName === "") setName("");
}, [projectName]);

For some reason, the project name filter is stored in a separate state variable, which is initialized with a value returned from the custom hook. The state is also updated by the useEffect, specifically when the project name is empty. I'm not sure why this is necessary, so usually I’d dig a bit deeper. But since it's not the main concern of this review, let's ignore it here.

return (
  <form
    className={styles.filter}
    onSubmit={(e) => {
      e.preventDefault();
      if (setStatus && setLevel && setProjectName) {
        setStatus(status);
        setLevel(level);
        setProjectName(projectName);
      }
      navigateToPage(1, status, level, name);
    }}
  >
        {/* THE FILTER INPUTS */}
    </form>
);

The filter inputs are wrapped in a form with a submit handler attached. The handler updates the filter values by calling the functions returned by the custom hook. After updating the filter values, the navigateToPage function is called. It sets the page to 1 and passes the new filter values.

So it seems that this code is responsible for resetting the pagination to the first page of the issue list whenever the filter values change.

Tight Coupling Of The Filter Component To Its Parent

Since the navigateToPage function is a prop, this is where the Filter component is coupled to its parent. Besides this function, there's nothing that connects the Filter component to the pagination of the issue list.

If you think about it there’s actually no reason why resetting the pagination should be the responsibility of the Filter component. It doesn’t “know” that it’s used on a paginated page. And it should be possible to use it on a page without pagination.

Refactoring The Filter Component

The idea of the following refactoring is that the resetting of the pagination should be the responsibility of the parent. This again will decouple the filter component from the rest of the page.

The first step is to rename the prop. onFilterChange is a much more neutral name that moves the responsibility for any action triggered by a filter change to the parent.

export function Filter({ onFilterChange }: FilterProps) {

return (
  <form
    onSubmit={(e) => {
      // ...
      onFilterChange(1, status, level, name);
    }}
  >

The new name already suggests that we should only pass the filters to the onFilterChange callback. So let's remove the new page param.

<form
  onSubmit={(e) => {
    // ...
    onFilterChange(status, level, name);
  }}
>

Now we decoupled the pagination functionality from the filter updates in this component.

It might look like a small change, but the filter component is now much more "pure". We could use it now in other places that have nothing to do with pagination.

My next suggestion is to group the filters in one object instead of splitting them into separate parameters.

<form
  onSubmit={(e) => {
    // ...
    onFilterChange({ status, level, name });
  }}
>

This way it'll be much easier to add new filter parameters in the future.

This change also enables another valuable refactoring. We can introduce a shared Filters type that can be reused by other parts of the app.

interface Filters {
  status: Status;
  level: Level;
  projectName: string;
}

interface FilterProps {
  onFilterChange: ({ status, level, projectName }: Filters) => void;
}

I added the type to the component file here, but I'd recommend moving it to a higher level. This way it's obvious that it's a shared type and can be used by other parts of the app and different layers like the API requests as well.

Refactoring The Parent Component

Finally, we need to adjust the parent component as well. In this case, it's the IssueList component.

import { useRouter } from "next/router";
import type { Level, Query, Status } from "@api/issues.types";
import { Filter } from "../filter";
import styles from "./issue-list.module.scss";

export function IssueList() {
  const router = useRouter();

  const page = Number(router.query.page || 1);
  const navigateToPage = (
    newPage: number,
    status: Status,
    level: Level,
    projectName: string,
  ) => {
    const query: Query = { page: newPage };
    if (status) query.status = status;
    if (level) query.level = level;
    if (projectName) query.project = projectName;

    router.push({
      pathname: router.pathname,
      query: { ...query },
    });
  };

  return (
    <div>
      <div className={styles.issueOptions}>
        <Filter onFilterChange={navigateToPage} />
      </div>
            {/* THE REST OF THE COMPONENT HERE */}

The first change is to use the shared Filter type.

import { Filter, type Filters } from "../filter";

export function IssueList() {
  // ...

  const navigateToPage = (
    newPage: number,
    filters: Filters,
  ) => {
    const { status, level, projectName } = filters;
    const query: Query = { page: newPage };
    if (status) query.status = status;
    if (level) query.level = level;
    if (projectName) query.project = projectName;

    router.push({
      pathname: router.pathname,
      query: { ...query },
    });
  };

You can see that it's much neater, as we don't have to redefine the type of the filters all over again.

The next problem is that the navigateToPage function isn’t just updating the page but the filter values in the URL as well. That should be the responsibility of the Filter component or the custom useFilters hook. So let’s adjust that.

const navigateToPage = (newPage: number) => {
  router.push({
    pathname: router.pathname,
    query: { 
            ...router.query,
            page: newPage,
        },
  });
};

Now this looks a lot cleaner. The navigateToPage function really only updates the page while copying the existing URL query parameters.

The last step is to adjust the props we pass to the Filter component.

<Filter onFilterChange={navigateToPage} />

We can't directly pass the navigateToPage function anymore. Instead, we use an inline function that accepts the filters and passes them onto navigateToPage, along with a new page value of 1.

<Filter onFilterChange={() => navigateToPage(1)} />

Again, here we can see that we introduced a clear separation of concerns.

The filter component is responsible for handling the filters, while the IssueList component is responsible for any action after the filters are updated, like resetting the page to 1.

Summary

In this code review, we saw that the filter component was tightly coupled to the pagination of the issue list. These couplings aren’t easy to spot. And in this case, the refactoring wasn't huge. But removing unnecessary coupling in your code can have huge effects on maintainability in the long run.

I hope you enjoyed this small code review and refactoring session. I’m planning to turn this into a series of code reviews. So if you're interested in getting a review of your own code, leave a comment with a screenshot of the code or a link to a GitHub repository or a pull request below the video mentioned at the top.

And, if you want to get hands-on experience on a production-grade codebase and professional workflows, join the React Job Simulator.