Skip to content

hotfix: Implemented Task 7.4b#135

Open
edsonwade wants to merge 1 commit into
mainfrom
develop
Open

hotfix: Implemented Task 7.4b#135
edsonwade wants to merge 1 commit into
mainfrom
develop

Conversation

@edsonwade

Copy link
Copy Markdown
Owner

Purpose
Fix the recurring ReviewConcurrencyIntegrationTest deadlock in product-service, then build the missing frontend UI for F7 Task 7.4b (product reviews) so the whole review flow can actually be verified by clicking through the app at localhost:8080.

Current state
Backend fix (done, unverified): ProductRepository.lockProductForRatingRecompute changed from FOR UPDATE to FOR NO KEY UPDATE — root cause was a lock conflict with the FK insert's FOR KEY SHARE. Test assertion hardened to surface actual exceptions instead of silently counting failures.

Frontend (done, unverified): Full reviews UI built from scratch — none existed before this session.

New: api/reviews.api.ts, components/product/{StarRating,ReviewList,ReviewForm,ReviewsSection}.tsx, pages/admin/AdminReviewsPage.tsx
Modified: types.ts, constants.ts, i18n/config.ts, ProductPage.tsx, ProductCard.tsx, routes/index.tsx, AdminLayout.tsx
Build errors found and fixed: npm run build surfaced DeleteOutline doesn't exist in @mui/icons-material (correct name: DeleteOutlined) in ReviewList.tsx and AdminReviewsPage.tsx. Fixed and confirmed via grep — no remaining bad references. Root cause was an unverified icon name; corrected the habit, not just the two instances.

Outcome
Nothing has been run since the icon fix — per project rules, Claude never runs npm run build/mvn verify/Docker; the user does. Three things block browser verification:

Re-run cd frontend && npm run build && npm run lint to confirm the icon fix is clean.
Run mvn verify -pl product-service -Dapi.version=1.44 to confirm the deadlock fix passes.
Rebuild both product-service and frontend containers (both currently serve stale images).
Once those three are done, next step is browser verification of the full review flow (buyer form, eligibility states, star ratings on catalog/detail, ADMIN moderation at /admin/reviews) — no further confirmation needed to proceed once the user confirms the stack is up.

Full picture confirmed — the key is split into two namespaces, and it's worse than I reported:

Site Type Role
ProductPage:38 string read
ProductForm:52 / :104 string read + invalidate (self-consistent, works)
OrderDetailPage:48 number read
ReviewsSection:61 number invalidate
AdminReviewsPage:48 number invalidate

Both review writers invalidate the number namespace, which only OrderDetailPage populates. Canonicalising on number is the right direction — ProductResponse.id is a number and getById takes a number; the string is only a URL artifact.

Now verifying no string-keyed usage remains, and confirming the writers really pass numbers.

All confirmed numeric (AdminReviewResponse.productId, OrderLineResponse.productId, and the ReviewsSection prop), so all six sites now share one namespace.

Updating the memory records to match reality.

Fixed — but the bug was wider than I reported

My original diagnosis named two files. Checking every call site first showed the real shape: [QUERY_KEYS.PRODUCT, …] was split across two cache namespaces, at six sites.

Both review writers (ReviewsSection, AdminReviewsPage) invalidated the numeric namespace, which only OrderDetailPage ever populated. ProductPage lived in the string namespace, so it was never reachable by any invalidation — not just the review ones. Had I applied the "one-line fix" I proposed, I'd have patched ReviewsSection to string and left AdminReviewsPage and OrderDetailPage mismatched.

Changes

ProductPage.tsx:38 — derives const productId = Number(id) and uses it for both key and queryFn, with a comment explaining why the key must be numeric.

ProductForm.tsx:52 — same derivation; :100 and :104 now use productId for the update call and the invalidation.

The three already-numeric sites needed no change. Verified via type declarations, not assumption: AdminReviewResponse.productId, OrderLineResponse.productId, and the ReviewsSection prop are all number.

Two deliberate restraints:

  • I left enabled: !!id / enabled: isEdit untouched. Tightening it to a NaN guard was tempting, but it would change what happens on a non-numeric URL like /catalog/abc — currently a fetch that errors into the error UI, versus a permanently disabled query. That's a behaviour change beyond this bug.
  • No [PRODUCTS] invalidation anywhere, per Decision A1's no-evict.

Side effect worth knowing: ProductPage and OrderDetailPage now share one cache entry per product instead of duplicating the fetch.

Not verified — and I can't verify it here

This fix is unproven. I haven't run npm run build or lint (project rule: you run those), and :8080 serves the previously built container image, so the running app still has the old bundle. My earlier browser evidence does not cover this change.

To close it out: npm run build + npm run lint, rebuild the frontend image, then repeat the flow — the star summary next to the price must update without a reload on both publish and delete. Tell me when it's rebuilt and I'll re-run the browser verification.

Memory updated in all three places, with the fix marked implemented-but-unverified and a new standing rule recorded so the key type doesn't drift back.

@edsonwade edsonwade self-assigned this Jul 20, 2026
@edsonwade edsonwade added documentation Improvements or additions to documentation duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers dependencies Pull requests that update a dependency file labels Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant