fix(pos): guard degraded storage mode#167
Open
kilbot wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a storage-health system: observable state in the database package, a StorageHealthProvider and hook, UI banner and guards that disable POS actions and barcode handling when storage is degraded, global replication pause/resume APIs, and related tests and translations. Changes
Sequence Diagram(s)sequenceDiagram
actor Staff as Staff (POS Operator)
participant UI as POS UI
participant Provider as StorageHealthProvider
participant DB as Database/Worker
participant RepMgr as Replication Manager
participant Handler as Wrapped Error Handler
Staff->>UI: Scan barcode / press checkout
UI->>Provider: check isDegraded?
opt worker communication fails
DB->>Handler: bulkWrite/query error ("could not requestRemote")
Handler->>Handler: classify error (WORKER_CONNECTION_LOST)
Handler->>DB: markStorageDegraded(source, reason)
DB->>Provider: emit degraded via storageHealth$
Provider->>RepMgr: pauseAllReplications("storage-health")
end
Provider->>UI: isDegraded = true
UI-->>Staff: disable buttons, skip barcode search, show StorageHealthBanner
Staff->>UI: reload/recover
UI->>DB: recovery actions
DB->>Provider: markStorageHealthy()
Provider->>RepMgr: resumeAllReplications()
Provider->>UI: isDegraded = false (banner cleared, buttons enabled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/database/src/plugins/wrapped-error-handler-storage.ts (1)
58-69:⚠️ Potential issue | 🟠 MajorNormalize worker-connection errors before rethrow to match degraded guards.
This path marks degraded state, but it rethrows the original error unchanged. Downstream degraded checks in
packages/core/src/screens/main/contexts/storage-health/error.tsonly matcherror.code === WORKER_CONNECTION_LOSTor message'storage unavailable', so raw'could not requestRemote'errors can bypass degraded-mode short-circuit handling.🛠️ Proposed fix
// Worker communication failures if (message.includes('could not requestRemote')) { + if (error instanceof Error) { + (error as Error & { code?: string }).code = ERROR_CODES.WORKER_CONNECTION_LOST; + } markStorageDegraded(methodName, message); storageLogger.error(`Storage worker error in ${methodName}`, { saveToDb: true, context: { errorCode: ERROR_CODES.WORKER_CONNECTION_LOST, method: methodName, }, }); // Don't suppress -- this is critical return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/plugins/wrapped-error-handler-storage.ts` around lines 58 - 69, The handler currently marks storage degraded for "could not requestRemote" but rethrows the original error, so downstream guards (which check error.code === WORKER_CONNECTION_LOST or message === 'storage unavailable') miss it; update the branch in wrapped-error-handler-storage (the block using methodName, markStorageDegraded and storageLogger) to normalize the thrown error by either setting err.code = ERROR_CODES.WORKER_CONNECTION_LOST and/or replacing the message with 'storage unavailable' (or throw a new Error object with those properties) before rethrowing so downstream checks in storage-health/error.ts will match.
🧹 Nitpick comments (5)
packages/core/src/screens/main/pos/hooks/use-update-line-item.test.ts (1)
108-117: Add degraded-path coverage forsplitLineItemtoo.This hook exposes two mutation paths (
updateLineItem,splitLineItem), but only one degraded guard is asserted here. A matchingsplitLineItemdegraded test would prevent regressions on the second write path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/screens/main/pos/hooks/use-update-line-item.test.ts` around lines 108 - 117, Add a sibling test to the existing degraded-storage case that asserts the hook's splitLineItem path is also blocked: when mockUseStorageHealth returns { status: 'degraded', isDegraded: true }, render the hook via renderHook(() => useUpdateLineItem()) and call result.current.splitLineItem(...) expecting it to reject with 'storage unavailable' and verifying mockLocalPatch (or other write mocks) was not called; mirror the structure of the updateLineItem test so splitLineItem's degraded-guard cannot regress.packages/database/src/plugins/storage-health-events.ts (1)
33-39: Small DRY cleanup opportunity in health reset helpers.
resetStorageHealth()can delegate tomarkStorageHealthy()so healthy-state reset logic stays in one place.♻️ Proposed refactor
export function markStorageHealthy() { subject.next(initialState); } export function resetStorageHealth() { - subject.next(initialState); + markStorageHealthy(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/plugins/storage-health-events.ts` around lines 33 - 39, resetStorageHealth duplicates the same logic as markStorageHealthy; update resetStorageHealth to delegate to markStorageHealthy instead of calling subject.next(initialState) directly so the healthy-state reset logic is centralized in markStorageHealthy (reference functions: markStorageHealthy, resetStorageHealth, and subject/initialState).packages/query/tests/manager.test.ts (1)
219-239: Good baseline test; consider adding a paused-registration assertion.This covers pausing active replications well. To protect the new start-gating path, also assert that a query registered after
pauseAllReplications()does not auto-start replication until resume.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query/tests/manager.test.ts` around lines 219 - 239, Add an assertion to the test that registering a query after calling manager.pauseAllReplications('storage-health') does not auto-start replication: after calling manager.pauseAllReplications, register another query via manager.registerQuery(...) (use same or new queryKeys like 'products-health-late'), then locate the new replication in manager.activeCollectionReplications and/or manager.activeQueryReplications and assert its subjects.paused.getValue() is true (and/or that its start() was not invoked/replication not running) to verify the start-gating behavior; reference manager.registerQuery, manager.pauseAllReplications, manager.activeCollectionReplications, manager.activeQueryReplications, and subjects.paused in your checks.packages/database/src/plugins/wrapped-error-handler-storage.test.ts (1)
216-237: Harden subscription cleanup in degraded-health tests.If an assertion fails before Line 236/Line 375, the subscription is left active and can bleed state into later tests. Wrap the assertions in
try/finallyto always unsubscribe.♻️ Suggested test-hardening pattern
- const sub = storageHealth$.subscribe((state) => healthStates.push(state.status)); - await expect(wrappedInstance.query({} as any)).rejects.toThrow('could not requestRemote'); - expect(healthStates).toContain('degraded'); - sub.unsubscribe(); + const sub = storageHealth$.subscribe((state) => healthStates.push(state.status)); + try { + await expect(wrappedInstance.query({} as any)).rejects.toThrow('could not requestRemote'); + expect(healthStates).toContain('degraded'); + } finally { + sub.unsubscribe(); + }Also applies to: 357-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/plugins/wrapped-error-handler-storage.test.ts` around lines 216 - 237, The test "emits degraded health when query loses worker connection" creates a subscription (healthStates via storageHealth$.subscribe assigned to sub) but doesn't guarantee cleanup on assertion failures; update the test (and the similar one around lines 357-376) to wrap the assertions and expectations in a try/finally so that sub.unsubscribe() is always called in the finally block, ensuring the subscription is cleaned up even if wrappedInstance.query or expectations throw.packages/core/src/screens/main/pos/components/storage-health-banner.tsx (1)
19-29: Consider adding accessibility semantics for this blocking warning.Since this is a critical operational warning, adding alert semantics (or equivalent RN a11y props) would improve assistive-tech visibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/screens/main/pos/components/storage-health-banner.tsx` around lines 19 - 29, The storage-health-banner element (testID 'storage-health-banner' in the storage-health-banner component) needs explicit accessibility semantics for a blocking alert; update the root element (where testID: 'storage-health-banner' is set) to include RN a11y props such as accessibilityRole="alert" (or accessibilityRole="header" + accessibilityLiveRegion="assertive"/accessibilityLiveRegion on platforms that support it), importantForAccessibility="yes" (or "yes-hide-descendants" as appropriate), and a clear accessibilityLabel that includes the localized message (e.g., combine t('common.pos_storage_connection_lost') and t('common.stop_using_this_register_until_reloaded')); ensure these props are applied on the same React element that renders the banner so assistive tech will announce it immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/screens/main/contexts/storage-health/error.ts`:
- Around line 10-15: isStorageDegradedError currently only checks
ERROR_CODES.WORKER_CONNECTION_LOST and exact message 'storage unavailable',
which misses worker-connection failures rethrown as messages like "could not
requestRemote"; update isStorageDegradedError to also detect these raw worker
connection failures by checking error instanceof Error and whether error.message
contains a worker-connection indicator (e.g. includes 'could not requestRemote'
or other known worker connection substrings) in addition to the existing code
check (use the same error cast pattern Error & { code?: string } and referenced
ERROR_CODES.WORKER_CONNECTION_LOST).
In `@packages/core/src/screens/main/pos/hooks/use-update-line-item.test.ts`:
- Around line 112-115: The test's exact-string assertion is brittle; change the
assertion for result.current.updateLineItem(...) so it doesn't rely on the
literal 'storage unavailable' string. Instead reject and capture the error and
assert its message includes the translation value produced by the mocked useT
(e.g., check error.message includes t('common.pos_storage_connection_lost')) or
simply use expect(...).rejects.toThrow() plus a separate
expect(error.message).toContain(t('common.pos_storage_connection_lost')); locate
this in the use-update-line-item.test.ts where updateLineItem is called and
adjust the assertion accordingly.
---
Outside diff comments:
In `@packages/database/src/plugins/wrapped-error-handler-storage.ts`:
- Around line 58-69: The handler currently marks storage degraded for "could not
requestRemote" but rethrows the original error, so downstream guards (which
check error.code === WORKER_CONNECTION_LOST or message === 'storage
unavailable') miss it; update the branch in wrapped-error-handler-storage (the
block using methodName, markStorageDegraded and storageLogger) to normalize the
thrown error by either setting err.code = ERROR_CODES.WORKER_CONNECTION_LOST
and/or replacing the message with 'storage unavailable' (or throw a new Error
object with those properties) before rethrowing so downstream checks in
storage-health/error.ts will match.
---
Nitpick comments:
In `@packages/core/src/screens/main/pos/components/storage-health-banner.tsx`:
- Around line 19-29: The storage-health-banner element (testID
'storage-health-banner' in the storage-health-banner component) needs explicit
accessibility semantics for a blocking alert; update the root element (where
testID: 'storage-health-banner' is set) to include RN a11y props such as
accessibilityRole="alert" (or accessibilityRole="header" +
accessibilityLiveRegion="assertive"/accessibilityLiveRegion on platforms that
support it), importantForAccessibility="yes" (or "yes-hide-descendants" as
appropriate), and a clear accessibilityLabel that includes the localized message
(e.g., combine t('common.pos_storage_connection_lost') and
t('common.stop_using_this_register_until_reloaded')); ensure these props are
applied on the same React element that renders the banner so assistive tech will
announce it immediately.
In `@packages/core/src/screens/main/pos/hooks/use-update-line-item.test.ts`:
- Around line 108-117: Add a sibling test to the existing degraded-storage case
that asserts the hook's splitLineItem path is also blocked: when
mockUseStorageHealth returns { status: 'degraded', isDegraded: true }, render
the hook via renderHook(() => useUpdateLineItem()) and call
result.current.splitLineItem(...) expecting it to reject with 'storage
unavailable' and verifying mockLocalPatch (or other write mocks) was not called;
mirror the structure of the updateLineItem test so splitLineItem's
degraded-guard cannot regress.
In `@packages/database/src/plugins/storage-health-events.ts`:
- Around line 33-39: resetStorageHealth duplicates the same logic as
markStorageHealthy; update resetStorageHealth to delegate to markStorageHealthy
instead of calling subject.next(initialState) directly so the healthy-state
reset logic is centralized in markStorageHealthy (reference functions:
markStorageHealthy, resetStorageHealth, and subject/initialState).
In `@packages/database/src/plugins/wrapped-error-handler-storage.test.ts`:
- Around line 216-237: The test "emits degraded health when query loses worker
connection" creates a subscription (healthStates via storageHealth$.subscribe
assigned to sub) but doesn't guarantee cleanup on assertion failures; update the
test (and the similar one around lines 357-376) to wrap the assertions and
expectations in a try/finally so that sub.unsubscribe() is always called in the
finally block, ensuring the subscription is cleaned up even if
wrappedInstance.query or expectations throw.
In `@packages/query/tests/manager.test.ts`:
- Around line 219-239: Add an assertion to the test that registering a query
after calling manager.pauseAllReplications('storage-health') does not auto-start
replication: after calling manager.pauseAllReplications, register another query
via manager.registerQuery(...) (use same or new queryKeys like
'products-health-late'), then locate the new replication in
manager.activeCollectionReplications and/or manager.activeQueryReplications and
assert its subjects.paused.getValue() is true (and/or that its start() was not
invoked/replication not running) to verify the start-gating behavior; reference
manager.registerQuery, manager.pauseAllReplications,
manager.activeCollectionReplications, manager.activeQueryReplications, and
subjects.paused in your checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43aa2876-d918-4fde-ac13-4b5c09a1ce65
📒 Files selected for processing (37)
apps/main/app/(app)/(drawer)/(pos)/_layout.tsxapps/main/app/(app)/_layout.tsxpackages/core/src/contexts/translations/locales/en/core.jsonpackages/core/src/screens/main/contexts/storage-health/error.tspackages/core/src/screens/main/contexts/storage-health/provider.test.tsxpackages/core/src/screens/main/contexts/storage-health/provider.tsxpackages/core/src/screens/main/contexts/use-push-document.test.tspackages/core/src/screens/main/contexts/use-push-document.tspackages/core/src/screens/main/pos/cart/buttons/pay.tsxpackages/core/src/screens/main/pos/cart/buttons/save-order.tsxpackages/core/src/screens/main/pos/cart/buttons/storage-health-guards.test.tsxpackages/core/src/screens/main/pos/cart/buttons/void.tsxpackages/core/src/screens/main/pos/components/storage-health-banner.test.tsxpackages/core/src/screens/main/pos/components/storage-health-banner.tsxpackages/core/src/screens/main/pos/hooks/block-on-storage-degraded.tspackages/core/src/screens/main/pos/hooks/use-add-coupon.test.tspackages/core/src/screens/main/pos/hooks/use-add-coupon.tspackages/core/src/screens/main/pos/hooks/use-add-fee.test.tspackages/core/src/screens/main/pos/hooks/use-add-fee.tspackages/core/src/screens/main/pos/hooks/use-add-item-to-order.test.tspackages/core/src/screens/main/pos/hooks/use-add-item-to-order.tspackages/core/src/screens/main/pos/hooks/use-add-product.tspackages/core/src/screens/main/pos/hooks/use-add-shipping.test.tspackages/core/src/screens/main/pos/hooks/use-add-shipping.tspackages/core/src/screens/main/pos/hooks/use-add-variation.tspackages/core/src/screens/main/pos/hooks/use-update-line-item.test.tspackages/core/src/screens/main/pos/hooks/use-update-line-item.tspackages/core/src/screens/main/pos/products/use-barcode.test.tspackages/core/src/screens/main/pos/products/use-barcode.tspackages/database/src/index.tspackages/database/src/plugins/storage-health-events.tspackages/database/src/plugins/wrapped-error-handler-storage.test.tspackages/database/src/plugins/wrapped-error-handler-storage.tspackages/query/src/manager.tspackages/query/src/query-replication-state.tspackages/query/tests/manager.test.tspackages/query/tests/query-replication-state.test.ts
Contributor
📊 Test Coverage Report
Coverage Legend
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d7820aaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
🚀 Deployment Summary
🔗 Quick Links🤖 Updated by GitHub Actions |
Contributor
Fix Summary — Round 1
Commit: 2dd77a9 |
- Expand isStorageDegradedError to detect raw 'could not requestRemote' worker-connection failures, not just synthetic StorageDegradedError - Normalize rethrown worker errors by stamping error.code with WORKER_CONNECTION_LOST before rethrow in wrapped-error-handler-storage - DRY: resetStorageHealth delegates to markStorageHealthy - Harden subscription cleanup in degraded-health tests with try/finally Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/database/src/plugins/storage-health-events.ts`:
- Around line 20-22: getStorageHealthSnapshot currently returns the internal
StorageHealthState by reference and the code re-emits the shared initialState
object, which allows callers to mutate global state; change
getStorageHealthSnapshot to return a defensive copy of subject.getValue() (e.g.,
structuredClone or a deep-clone helper) and ensure any code that emits or resets
to initialState (the place that re-emits initialState) also emits a cloned copy
rather than the original initialState object so the subject always holds an
immutable/internal instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f99443b-b7b1-4f58-ac98-365c210f6ef3
📒 Files selected for processing (4)
packages/core/src/screens/main/contexts/storage-health/error.tspackages/database/src/plugins/storage-health-events.tspackages/database/src/plugins/wrapped-error-handler-storage.test.tspackages/database/src/plugins/wrapped-error-handler-storage.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/database/src/plugins/wrapped-error-handler-storage.test.ts
- packages/core/src/screens/main/contexts/storage-health/error.ts
- packages/database/src/plugins/wrapped-error-handler-storage.ts
Comment on lines
+20
to
+22
| export function getStorageHealthSnapshot(): StorageHealthState { | ||
| return subject.getValue(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid exposing and reusing mutable state objects.
Line 21 returns the internal state object by reference, and Line 34 re-emits a shared initialState object. A caller can accidentally mutate that object and silently corrupt global health state (including future resets).
Suggested fix
export interface StorageHealthState {
- status: StorageHealthStatus;
- reason?: string;
- source?: string;
- lastErrorAt?: number;
+ readonly status: StorageHealthStatus;
+ readonly reason?: string;
+ readonly source?: string;
+ readonly lastErrorAt?: number;
}
-const initialState: StorageHealthState = {
- status: 'healthy',
-};
+function createHealthyState(): StorageHealthState {
+ return { status: 'healthy' };
+}
-const subject = new BehaviorSubject<StorageHealthState>(initialState);
+const subject = new BehaviorSubject<StorageHealthState>(createHealthyState());
@@
export function getStorageHealthSnapshot(): StorageHealthState {
- return subject.getValue();
+ return { ...subject.getValue() };
}
@@
export function markStorageHealthy() {
- subject.next(initialState);
+ subject.next(createHealthyState());
}Also applies to: 33-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/src/plugins/storage-health-events.ts` around lines 20 - 22,
getStorageHealthSnapshot currently returns the internal StorageHealthState by
reference and the code re-emits the shared initialState object, which allows
callers to mutate global state; change getStorageHealthSnapshot to return a
defensive copy of subject.getValue() (e.g., structuredClone or a deep-clone
helper) and ensure any code that emits or resets to initialState (the place that
re-emits initialState) also emits a cloned copy rather than the original
initialState object so the subject always holds an immutable/internal instance.
Contributor
|
|
Contributor
|
@kilbot: This PR is unassigned and has been open since March 6th. Please assign yourself or ping the relevant team for review/assignment. Adding 'priority/P2' and 'bug' labels based on the summary. |
Open
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.
Summary
could not requestRemotefailures as a first-class degraded storage state in the database layerTest plan
bulkWriteand read-path worker failures (query/requestRemote) so the UI enters degraded mode consistently.pnpm jest packages/database/src/plugins/wrapped-error-handler-storage.test.ts --runInBandnpx jest --config jest.config.cjs tests/manager.test.ts tests/query-replication-state.test.ts --runInBandinpackages/querynpx jest src/screens/main/contexts/storage-health/provider.test.tsx src/screens/main/pos/components/storage-health-banner.test.tsx src/screens/main/pos/cart/buttons/storage-health-guards.test.tsx src/screens/main/pos/hooks/use-add-item-to-order.test.ts src/screens/main/pos/hooks/use-update-line-item.test.ts src/screens/main/pos/hooks/use-add-fee.test.ts src/screens/main/pos/hooks/use-add-shipping.test.ts src/screens/main/pos/hooks/use-add-coupon.test.ts src/screens/main/contexts/use-push-document.test.ts src/screens/main/pos/products/use-barcode.test.ts --runInBand --collectCoverage=falseinpackages/corepnpm run lintinpackages/database,packages/query,packages/core, andapps/mainCloses #163
🤖 Generated with Claude Code
Summary by CodeRabbit