PR Review Feedback

May 16, 2023 · 4 min read · PR review Summary ·

Share on:

I do not have a good memory and cannot remember all the mistakes I made in the previous PR reviewing. Since The palest ink is better than the best memory, so let me summarize the mistakes I have made here to reminder myself, hope it may help.

React

Necessary of `useEffect`?

When we use the useEffect, we need to think about if it's necessary, especially for the initial mount effect. Instead of updating some values based on the dependency of useEffect, we should check if it's possible to set the value initially directly.

React-query

Use the query data as source of data, instead of triggering/update from UI

One example would be undoing the editing to a table item with Material UI's DataGrid. The procedure of undo is

Edit the table cell text with Material UI's editable function (Imperative)
Trigger the undo by clicking somewhere undo button/link (Declarative)

Here we have two ways to trigger the undo:

use Material UI's method to recover the data in the table and then the data would be updated sequentially

 1undoCallback: async () => {
 2  // Undo the process should be fast with update only one field value
 3  setUndoStateMap((value) => ({ ...value, [newRow.id]: key }))
 4  // during the undo, use data grid's way to update the cell value manually
 5  await tableApiRef.current.startCellEditMode({ id: newRow.id, field: key })
 6  await tableApiRef.current.setEditCellValue({
 7      id: newRow.id,
 8      field: key,
 9      value: oldValue,
10  })
11  // If the cell is in edit mode, stop it
12-   if (tableApiRef.current.getCellMode(newRow.id, key) === 'edit')
13-     await tableApiRef.current.stopCellEditMode({ id: newRow.id, field: key })
14}

update the table cell data directly and it would be reflected by React automatically

1undoCallback: async () => {
2  updateKnowTheDriverMutation.mutate({
3    vehicleId: newRow.vehicleId,
4    category: key,
5    score: oldValue,
6  })
7},

From the code we can see the second one is better since it's simpler and meets the react's principle declarative way. The imperative way needs to know each step of whole undo processing and it may contribute the mistake.

Think of the rollback situation when using mutation

When we mutate the data, we need to think in this way: if the action fails, do we need to rollback the previous value? Here are the example codes about mutation:

 1onMutate: async (variables) => {
 2  const knowTheDriverQuery = knowTheDriverListQuery()
 3
 4  // Cancel any outgoing refetches (so they don't overwrite our optimistic update)
 5  await queryClient.cancelQueries(knowTheDriverQuery)
 6
 7  const previousQueryData = knowTheDriverQuery.getData(queryClient)
 8
 9  if (previousQueryData) {
10    knowTheDriverQuery.setData(queryClient, {
11      updater: updateFirstWhere(
12        previousQueryData,
13        (c) => c.vehicleId === variables.vehicleId,
14        // Explicit return type so that we don't add properties by mistake
15        (c): (typeof previousQueryData)[number] => ({
16          ...c,
17          [variables.category]: Number(variables.score),
18        }),
19      ),
20    })
21  }
22
23  return { previousQueryData }
24},
25onError: (err, _variables, context) => {
26  if (context?.previousQueryData) {
27    knowTheDriverListQuery().setData(queryClient, {
28      updater: context.previousQueryData,
29    })
30  }
31  makeMutationErrorHandlerWithToast().onError(err)
32}

Date

Send Js native Date to server, it would be serialized automatically and use this as the format in API

When we need to send the date to server, no matter what library we use to handle it, Luxon, Moment, etc, we should save the native Date to sever since it's native supported and could be used in all the browsers without supporting from third-part libraries.

Typescript

Parse data from external sources

When we get some data which may be arbitrary from external source, we can use zod to parse the value, because Typescript cannot check the values in the runtime.

For example, we have a field about filters' initial values in the response of api, and it should have static options:

vehicle_group_init,
department_init,
sub_department_init,
drivers_init,
event_type_init,

1export const apiCoachingDashboardFilterType = z.enum([
2  'vehicle_group_init',
3  'department_init',
4  'sub_department_init',
5  'drivers_init',
6  'event_type_init',
7])

In frontend, we cannot confirm the values sent from backend are correct. So before converting the values, we need to check if they meet our requirements

 1export declare namespace CoachingDashboardFilterType {
 2  type ApiInput = {
 3    id: z.infer<typeof apiCoachingDashboardFilterType>
 4    default: string
 5  }
 6  type ParsedEvent = {
 7    id: 'vehicleGroup' | 'department' | 'section' | 'driver' | 'eventType'
 8    value: string
 9  }
10}
11
12export function parseUserSettings() {
13  // ...
14
15  const coachingDashboardFiltersRaw: Array<CoachingDashboardFilterType.ApiInput> =
16    settings.coaching_dashboard_filters
17
18  const coachingDashboardFilters = coachingDashboardFiltersRaw
19    .map((filter) => {
20      const parsedResult = apiCoachingDashboardFilterType.safeParse(filter.id)
21      if (!parsedResult.success) {
22        return null
23      }
24      const parsed: CoachingDashboardFilterType.ParsedEvent = {
25        id: parsedResult.data,
26        value: `${filter.default}`,
27      }
28      return parsed
29    })
30    .filter(Re.isNot(Re.isNil))
31  // ...
32}

And when we return the parsed values, instead of returning it directly, we can define a variable as parsed value type. With this, if the input data have some different values in the future, typescript would report it.

1const parsed: CoachingDashboardFilterType.ParsedEvent = {
2  id: parsedResult.data,
3  value: `${filter.default}`,
4}
5return parsed

Form

When we use the react-hook-form's Controller to