[MPDX-9369] - Address PR feedback for Hours Per Week calculator by wjames111 · Pull Request #1740 · CruGlobal/mpdx-react

wjames111 · 2026-04-22T20:24:59Z

Description

Follow-up PR addressing code review feedback from #1737.

Addressed Fragment concern in GraphQL query (discussion)
Fixed double snackbar issue (discussion)
Removed code block that was no longer needed (discussion)
Removed extra snackbars (discussion)
Extracted component logic into useHoursPerWeekGrid hook with dedicated tests (discussion)

Testing

Go to PDS Goal Calculator setup page
Verify the Hours Per Week grid loads correctly with existing data
Add, edit, and delete rows — confirm autosave works with a single success snackbar (no duplicates)
Adjust weeks to exceed available total — confirm clamping works correctly
Check that the average hours per week recalculates and saves automatically

Checklist:

I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
I have run the Claude Code /pr-review command locally and fixed any relevant suggestions
I have requested a review from another person on the project
I have tested my changes in preview or in staging
I have cleaned up my commit history

…ull/1737\#discussion_r3126204715

…ll/1737\#discussion_r3126219630

…ct/pull/1737\#discussion_r3126236399

…737\#discussion_r3126242447

…1737\#discussion_r3126255662

Restructure act() calls to avoid returning values directly from act(), which expects void return. Capture return values via outer variables instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

github-actions · 2026-04-22T20:28:35Z

Bundle sizes [mpdx-react]

Compared against 685ea40

No significant changes found

Moved the title to the page-level right panel header and removed the duplicate heading from HoursPerWeekGrid component. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

github-actions · 2026-04-22T20:35:06Z

Preview branch generated at https://MPDX-9369-PR-feedback.d3dytjb8adxkk5.amplifyapp.com

wjames111

Multi-Agent Code Review Report

Agents: 6 specialized reviewers (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting) with cross-examination debate
Mode: Standard
Risk Score: 8/10 (HIGH)

Verdict: APPROVED WITH SUGGESTIONS

No blockers found. 5 medium-priority improvements recommended.

Summary

This is a well-structured refactoring PR that:

Extracts ~300 lines of business logic from HoursPerWeekGrid into a dedicated useHoursPerWeekGrid hook with comprehensive tests
Creates a reusable GraphQL fragment (DesignationSupportHoursItemFields) eliminating field duplication across 3 operations
Makes the right panel title dynamic via context (setRightPanel(title, content) API)
Correctly removes duplicate error snackbars (global Apollo error link already handles them)

Debate Resolution

The Data Integrity agent initially flagged a CRITICAL issue claiming saveHoursItem would fail for temp entries by sending temp- IDs to updateHoursItem. The Architecture agent disproved this by identifying the isCellEditable guard in HoursPerWeekGrid.tsx that explicitly blocks editing of temp- prefixed rows. The finding was withdrawn during debate.

Dependency Impact

useSaveField.ts behavioral change (error handling removed) affects ReimbursableExpensesGrid.tsx outside this PR — verify that component handles propagated errors.

Standards Checklist

Standard	Status
Named exports only	✅
File naming conventions	✅
GraphQL `id` fields	✅
Mutation cache updates	✅
All strings use `t()`	✅
No `any` types	✅
Colocated tests	✅
No debug output	✅

wjames111 · 2026-04-22T20:51:56Z

+        throw new Error(t('Failed to save hours entry.'));
+      }
+    },
+    [calculation, createHoursItem, updateHoursItem, trackMutation, t],


[Medium] **Stale dependency in useCallback** (4 agents agree)
createHoursItem is listed in the dependency array but never used in the saveHoursItem function body — only updateHoursItem is called. This is a leftover from the extraction refactor that causes unnecessary callback re-creation.

Suggested change

[calculation, createHoursItem, updateHoursItem, trackMutation, t],

[calculation, updateHoursItem, trackMutation, t],

wjames111 · 2026-04-22T20:51:57Z

+
+      await saveHoursItem(updatedEntry);
+
+      const updatedEntries = entries.map((entry) =>


[Medium] **Stale `entries` closure for average calculation** (3 agents agree)
processRowUpdate reads entries from its closure to compute the new average, but updateEntry (line 222) already updated state via the functional setEntries updater. During rapid sequential edits, the entries used here would be stale.

Mitigated by DataGrid blocking behavior (cell edit waits for promise resolution) and refetchQueries re-sync, so unlikely in practice. But the average sent to server via saveField could be temporarily incorrect.

Consider computing the average inside a useEffect that watches entries changes, or using a ref for the latest entries.

wjames111 · 2026-04-22T20:51:57Z

+    } catch {
+      setEntries((prev) => prev.filter((e) => e.id !== tempId));
+    }
+  }, [t, calculation, createHoursItem, trackMutation, entries.length]);


[Medium] **Dependency array uses `entries.length` but callback reads `entries`**
addEntry computes newPosition via Math.max(...entries.map(e => e.position)) but only lists entries.length in the dependency array. If entries change content (e.g., positions) without changing length, the callback would use a stale closure.

Suggested change

}, [t, calculation, createHoursItem, trackMutation, entries.length]);

}, [t, calculation, createHoursItem, trackMutation, entries]);

wjames111 · 2026-04-22T20:51:57Z

+      const updatedEntry: HoursPerWeekEntry = {
+        id: newRow.id as string,
+        label: newRow.label as string,
+        hoursPerWeek: Number(newRow.hoursPerWeek) || 0,


[Suggestion] **No floor on `hoursPerWeek` — negative values possible** (Financial agent)
weeks is properly floored with Math.max(0, clampedWeeks) on the next line, but hoursPerWeek has no equivalent guard. A negative hours value would corrupt the weighted average sent to the server.

Consider:

hoursPerWeek: Math.max(0, Number(newRow.hoursPerWeek) || 0),

Or add min: 0 to the column definition in HoursPerWeekGrid.tsx.

wjames111 · 2026-04-22T20:51:57Z

        <Alert severity="warning" sx={{ mt: 1 }}>
          {t('Weeks must add up to {{max}}. {{remaining}} week(s) remaining.', {
-            max: MAX_TOTAL_WEEKS,
+            max: 52,


[Medium] **Magic number `52` — constant moved to hook but not used here**
MAX_TOTAL_WEEKS was moved to useHoursPerWeekGrid.ts but this line uses the raw number 52. Consider exporting the constant from the hook to prevent drift if the value ever changes.

zweatshirt

I'll try to finish up tomorrow. It looks good though

zweatshirt · 2026-04-22T22:01:35Z

+    const newPosition =
+      entries.length > 0 ? Math.max(...entries.map((e) => e.position)) + 1 : 0;


Could we try something like this: const newPosition = (entries.at(-1)?.position ?? -1) + 1;
It's minor, but it will make this O(1) instead of O(n)

zweatshirt · 2026-04-22T22:07:44Z

+    }
+  }, [t, calculation, createHoursItem, trackMutation, entries.length]);
+
+  const deleteEntry = useCallback(


In practice it probably doesn't matter as long as all new entries are greater than the highest position, but we aren't fixing the positions every time an entry is deleted. E.g. [1, 2, 3] -> delete at idx 1 and we will always be left with [1, 3] when it should probably be fixed to be [1, 2]. I don't think it matters too much though.

zweatshirt

Looks good Will! I think there some small places to do additional refactoring but but this is working well.

I only had one real concern I'm hoping we could fix before merging: When the averageHoursPerWeek is applied in the input field, it is never rounded. E.g. we get 2.0384615384615383 and not 2.04 displayed in the UI. We can either round just the display value, or if necessary, we can round what is sent to the server I guess (which I suggested previously we don't do that, but maybe it's not too critical)

I also noticed one small thing, if a user has already maxed out the number of weeks to 52, and returns to the calculator at a later time, the user can't input new # of Weeks, but has no alert as to why. This is minor.

zweatshirt · 2026-04-23T13:20:07Z

          rows={dataWithTotal}
          columns={columns}
          processRowUpdate={processRowUpdate}
          onCellEditStart={(params, event) => {


In the future I'm thinking we can change this to use BaseGrid's onCellEditStart={selectAllOnEditStart} behavior

zweatshirt · 2026-04-23T13:22:30Z

+      const newAverage = newTotalWeeks > 0 ? newTotalHours / newTotalWeeks : 0;
+
+      try {
+        if (!entryId.startsWith('temp-') && !entryId.startsWith('default-')) {


Do we need to still check for this 'default-' prefix?

Suggested change

if (!entryId.startsWith('temp-') && !entryId.startsWith('default-')) {

if (!entryId.startsWith('temp-')) {

zweatshirt · 2026-04-23T13:25:56Z

+  }, [t, calculation, createHoursItem, trackMutation, entries.length]);
+
+  const deleteEntry = useCallback(
+    async (id: string | number) => {


What do you think of changing this to `HoursPerWeekEntry['id']? Not necessary, just curious

zweatshirt · 2026-04-23T13:27:11Z

+                numberOfWeeks: entry.weeks,
+              },
+            },
+            refetchQueries: ['PdsGoalCalculation'],


Claude note: refetchQueries: ['PdsGoalCalculation'] is likely wasted given the initializedRef one-time-load guard.

zweatshirt · 2026-04-23T13:27:33Z

+    [entries, deleteHoursItem, trackMutation, saveField],
+  );
+
+  const processRowUpdate = useCallback(


Claude suggestion: Duplicated totals math across processRowUpdate / deleteEntry / the useMemos — candidate for a small helper.

wjames111 and others added 6 commits April 22, 2026 15:36

Addressing Fragment Concern https://github.com/CruGlobal/mpdx-react/p…

f50cb96

…ull/1737\#discussion_r3126204715

Addressing double snackbar https://github.com/CruGlobal/mpdx-react/pu…

7ca5400

…ll/1737\#discussion_r3126219630

Removing block no longer needed https://github.com/CruGlobal/mpdx-rea…

9098dda

…ct/pull/1737\#discussion_r3126236399

Remove extra snackbars https://github.com/CruGlobal/mpdx-react/pull/1…

ddab623

…737\#discussion_r3126242447

Extract logic into hook https://github.com/CruGlobal/mpdx-react/pull/…

aafc236

…1737\#discussion_r3126255662

Fix TypeScript errors in useHoursPerWeekGrid tests

9cfa798

Restructure act() calls to avoid returning values directly from act(), which expects void return. Capture return values via outer variables instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

wjames111 and others added 2 commits April 22, 2026 16:30

Move right panel title from 'Details' to 'Hours Per Week Calculator'

c90e5f7

Moved the title to the page-level right panel header and removed the duplicate heading from HoursPerWeekGrid component. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Make right panel title dynamic via context

1d05c18

wjames111 self-assigned this Apr 22, 2026

wjames111 added On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview labels Apr 22, 2026

wjames111 changed the title ~~MPDX-9369 Address PR feedback for Hours Per Week calculator~~ [MPDX-9369] - Address PR feedback for Hours Per Week calculator Apr 22, 2026

wjames111 requested a review from zweatshirt April 22, 2026 20:34

wjames111 commented Apr 22, 2026

View reviewed changes

zweatshirt reviewed Apr 22, 2026

View reviewed changes

zweatshirt requested changes Apr 23, 2026

View reviewed changes

	[calculation, createHoursItem, updateHoursItem, trackMutation, t],
	[calculation, updateHoursItem, trackMutation, t],


		await saveHoursItem(updatedEntry);

		const updatedEntries = entries.map((entry) =>

	}, [t, calculation, createHoursItem, trackMutation, entries.length]);
	}, [t, calculation, createHoursItem, trackMutation, entries]);

		const newPosition =
		entries.length > 0 ? Math.max(...entries.map((e) => e.position)) + 1 : 0;

	if (!entryId.startsWith('temp-') && !entryId.startsWith('default-')) {
	if (!entryId.startsWith('temp-')) {

Conversation

wjames111 commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

github-actions Bot commented Apr 22, 2026

Bundle sizes [mpdx-react]

Uh oh!

github-actions Bot commented Apr 22, 2026

Uh oh!

wjames111 left a comment

Choose a reason for hiding this comment

Multi-Agent Code Review Report

Verdict: APPROVED WITH SUGGESTIONS

Summary

Debate Resolution

Dependency Impact

Standards Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zweatshirt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zweatshirt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wjames111 commented Apr 22, 2026 •

edited

Loading

zweatshirt left a comment •

edited

Loading