Repository navigation
fix: prevent receipt-settings startup race and allow bill roles to read storefront settings - #1226
Merged
Merged
Conversation
…oles to read storefront settings (#1225) - Widen GET /api/settings/business permission gate to include bills.read, bills.print, orders.read, and bills.generate - Export ensureReceiptSettingsLoaded in useReceiptSettingsSync with in-flight deduplication and graceful offline fallback - Await ensureReceiptSettingsLoaded before printBill and printTaxBill in usePrinter - Add tests covering role-based storefront access, non-admin protections, and startup race prevention
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
khaira777
changed the title
…undary and print imports
… update settings E2E assertion (#1225)
…llback metadata in settings route
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Prevent receipt-settings startup race and allow bill roles to read storefront settings
What Changed
GET /api/settings/businessto be accessed by accounts withbills.read,bills.print,orders.read, orbills.generatepermissions in addition tosettings.view.ensureReceiptSettingsLoadedwith in-flight request deduplication inuseReceiptSettingsSync, and awaited hydration inusePrinterStorebefore printing bills or tax bills.Risk Assessment
✅ Low: The changes cleanly resolve the startup race and permission gating for receipt settings with thorough test coverage and robust offline fallbacks.
Testing
Executed focused unit/hydration suites and stood up the live product in Playwright headless Chromium against the real Express and SQLite backend to validate all 5 derived authorization, startup race, and receipt-rendering scenarios end-to-end with visual and HTTP artifacts.
Evidence: Rendered Receipt HTML Document
Evidence: HTTP 200 Storefront Settings Responses for Cashier and Server Roles
Evidence: HTTP 403 Forbidden (Chef) & HTTP 401 Unauthorized Responses
Evidence: HTTP 403 Forbidden for Cashier Settings Mutation (PUT) and Tax Config Access (GET)
Evidence: Concurrent Cold-Start ensureReceiptSettingsLoaded Coalescing Trace
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
frontend/src/hooks/useReceiptSettingsSync.ts:47- In ensureReceiptSettingsLoaded (frontend/src/hooks/useReceiptSettingsSync.ts:47), checking if (isHydrated) return; before checking if (syncPromise) await syncPromise; causes any active in-flight sync (e.g. triggered on window focus or tenant change) to be ignored once initial hydration has completed, allowing bill prints to proceed with stale settings while fresh settings are actively being fetched. Additionally, in syncReceiptSettings (frontend/src/hooks/useReceiptSettingsSync.ts:28), calling the function while syncPromise is already in-flight starts a duplicate request and reassigns syncPromise, leading to premature clearing of syncPromise when the first request completes in finally. Remedy: In syncReceiptSettings, check if (syncPromise) return syncPromise; and in ensureReceiptSettingsLoaded, await syncPromise before checking isHydrated.tests/receipt-settings-hydration.test.ts:537- In Test 7 (tests/receipt-settings-hydration.test.ts:537), ensureReceiptSettingsLoaded imports frontend/src/lib/api, which sets baseURL: '/api' in Node environments where window is undefined. Calling api.get('/settings/business') in Node fails with an invalid relative URL error, which syncReceiptSettings silently swallows via .catch(() => {}). As a result, isHydrated is never set to true and usePosSettingsStore is never hydrated during Test 7. Because Test 7 contains no assertions verifying store state or hydration after awaiting ensureReceiptSettingsLoaded, the test passes against the swallowed failure path rather than verifying successful hydration and subsequent cached resolution. Remedy: Configure frontend api.defaults.baseURL = baseUrl in the test before running ensureReceiptSettingsLoaded, and assert that usePosSettingsStore.getState() contains the hydrated settings.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
npm run buildnpm run build:frontendnpm run test:receipt-settings-hydrationnpx playwright test e2e/qa-settings-receipts-printers.spec.ts --project=chromiumnpx playwright test e2e/live-receipt-settings-authz.spec.ts --project=chromium✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.