Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify data entry forms #964

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

Simplify data entry forms #964

wants to merge 30 commits into from

Conversation

marlonbaeten
Copy link
Contributor

Fixes #850

This PR is an overhaul of the state management for the data entry forms.

It introduces a "reducer" to centralize state manipulation and simplify the data flow using events.

Apart from the central form state, the local form hooks and components are cleaned up, in particular the number of hard-to-reason-about useEffect and useRef hooks are removed.

See frontend/app/component/form/data_entry/state for the hooks, context and providers related to the data entry state.

Notes:

  • This refactor is the MVP for less complex state management in the data entry forms, its not perfect, especially in the util functions there is still some refactoring to be done.
  • This PR introduces the reducer and simplifies the central state; however, the central state is not perfectly compact as it should be.
  • We focussend on rewriting the most complex parts and getting it back into main ASAP
  • See https://react.dev/reference/react/useReducer for documentation about the useReducer

Other notes on React state management:

The data flow we try to adhere to

┌────> State ──> View ───┐
│                        │
│                        │
└──────── Events <───────┘

In our case events are triggered from the UI, which are dispatched to the central reducer. The reducer updates the otherwise immutable state object. The React components render this immutable state as a UI.

Don't overuse useRef

From the React documentation:

When to Use Refs

There are a few good use cases for refs:

  • Managing focus, text selection, or media playback.
  • Triggering imperative animations.
  • Integrating with third-party DOM libraries.

Avoid using refs for anything that can be done declaratively.

Don’t Overuse Refs

Your first inclination may be to use refs to “make things happen” in your app. If this is the case, take a moment and think more critically about where state should be owned in the component hierarchy. Often, it becomes clear that the proper place to “own” that state is at a higher level in the hierarchy. See the Lifting State Up guide for examples of this.

Don't overuse useEffect

From the React documentation:

You Might Not Need an Effect

Effects are an escape hatch from the React paradigm. They let you “step outside” of React and synchronize your components with some external system like a non-React widget, network, or the browser DOM. If there is no external system involved (for example, if you want to update a component’s state when some props or state change), you shouldn’t need an Effect. Removing unnecessary Effects will make your code easier to follow, faster to run, and less error-prone.

Strive for simple components with unidirectional data flow

  • Event handlers should trigger state updates
  • The events objects passed from the DOM are excellent references to DOM elements and their values -> we don't need useRef
  • Only use useEffect to preload API data, or trigger explicit side-effects
  • Keep state minimal and calculate derived state in the render component
  • The useMemo and useCallback hooks should only be used if they prevents expensive calculations - in Abacus this is almost never the case!

@marlonbaeten marlonbaeten added the frontend Issues or pull requests that relate to the frontend label Feb 4, 2025
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 95.04260% with 64 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (facaf67) to head (1dee793).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../form/data_entry/state/useInitialDataEntryState.ts 79.26% 17 Missing ⚠️
...end/app/component/form/data_entry/state/reducer.ts 89.05% 15 Missing ⚠️
...end/app/component/form/data_entry/state/actions.ts 88.76% 10 Missing ⚠️
...nt/pollingstation/PollingStationFormNavigation.tsx 92.79% 8 Missing ⚠️
...m/data_entry/candidates_votes/useCandidateVotes.ts 95.58% 6 Missing ⚠️
...ata_entry/candidates_votes/CandidatesVotesForm.tsx 95.23% 2 Missing ⚠️
...onent/form/data_entry/state/useDataEntryContext.ts 87.50% 2 Missing ⚠️
frontend/lib/util/format.ts 88.88% 2 Missing ⚠️
.../component/form/data_entry/state/dataEntryUtils.ts 96.15% 1 Missing ⚠️
...nt/form/data_entry/state/useDataEntryNavigation.ts 97.14% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
+ Coverage   89.76%   90.65%   +0.89%     
==========================================
  Files         248      257       +9     
  Lines       12987    13197     +210     
  Branches     1318     1329      +11     
==========================================
+ Hits        11658    11964     +306     
+ Misses       1235     1139      -96     
  Partials       94       94              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 4, 2025

Sigrid maintainability feedback

↗️ You improved the maintainability of the code towards your objective of 3.5 stars

Show details

Sigrid compared your code against the baseline of 2025-02-11.

👍 What went well?

You fixed or improved 1 refactoring candidates.

Risk System property Location
🟡 Unit Size
(Improved)
frontend/app/component/form/data_entry/check_and_save/CheckAndSaveForm.tsx
CheckAndSaveForm.tsx.CheckAndSaveForm()

👎 What could be better?

Unfortunately, 60 refactoring candidates were introduced or got worse.

Risk System property Location
🔴 Duplication
(Introduced)
frontend/app/component/form/data_entry/candidates_votes/useCandidateVotes.ts (lines 78-94)
frontend/app/component/form/data_entry/differences/useDifferences.ts (lines 54-70)
🔴 Duplication
(Introduced)
frontend/app/component/form/data_entry/candidates_votes/useCandidateVotes.ts (lines 83-94)
frontend/app/component/form/data_entry/differences/useDifferences.ts (lines 59-70)
frontend/app/component/form/data_entry/voters_and_votes/useVotersAndVotes.ts (lines 65-76)
🔴 Duplication
(Introduced)
frontend/app/component/form/data_entry/candidates_votes/useCandidateVotes.ts (lines 55-70)
frontend/app/component/form/data_entry/differences/useDifferences.ts (lines 32-47)
🔴 Duplication
(Introduced)
frontend/app/component/form/data_entry/state/dataEntryUtils.ts (line 293-298)
frontend/app/component/form/data_entry/state/dataEntryUtils.ts (line 315-320)
frontend/app/component/form/data_entry/state/dataEntryUtils.ts (line 330-335)
+ 2 occurrences
🔴 Duplication
(Introduced)
frontend/app/component/form/data_entry/candidates_votes/useCandidateVotes.ts (line 83-90)
frontend/app/component/form/data_entry/differences/useDifferences.ts (line 59-66)
frontend/app/component/form/data_entry/recounted/useRecounted.ts (line 45-52)
+ 1 occurrences
🔴 Unit Size
(Introduced)
frontend/app/component/form/data_entry/state/reducer.ts
reducer.ts.dataEntryReducer(DataEntryState,DataEntryAction)
🔴 Unit Size
(Introduced)
frontend/app/component/form/data_entry/state/dataEntryUtils.ts
dataEntryUtils.ts.getInitialFormState(Required<Election>,Partial<FormState>)
🟠 Unit Size
(Introduced)
frontend/app/component/form/data_entry/state/types.ts
types.ts
⚫️ + 52 more

📚 Remaining technical debt

3 refactoring candidates didn't get better or worse, but are still present in the code you touched.

View this system in Sigrid** to explore your technical debt

⭐️ Sigrid ratings

System property System on 2025-02-11 Before changes New/changed code
Volume 5.2 N/A N/A
Duplication 4.3 2.3 3.9
Unit Size 2.4 3.1 2.2
Unit Complexity 3.4 1.2 2.4
Unit Interfacing 3.0 4.8 3.2
Module Coupling 4.0 5.5 2.8
Component Independence 2.8 N/A N/A
Component Entanglement 3.7 N/A N/A
Maintainability 3.6 3.3 2.8

💬 Did you find this feedback helpful?

We would like to know your thoughts to make Sigrid better.
Your username will remain confidential throughout the process.


View this system in Sigrid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues or pull requests that relate to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement reducer solution for data entry
3 participants