[codex] Structure client persistence errors#3385
Conversation
Co-authored-by: codex <codex@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Superseded by the existing connection-persistence owner PR #3255. The remaining catalog-specific structural improvements are being consolidated onto that branch. |
ApprovabilityVerdict: Approved This PR restructures ConnectionPersistenceError to carry structured context fields (environmentId, threadId, cause) instead of a formatted message string. The changes are mechanical - same error type, same operations - affecting only error diagnostics rather than runtime behavior. You can customize Macroscope's approvability policy. Learn more. |
Summary
ConnectionPersistenceErrorderive its message from a typed operation while retaining environment/thread identifiers and a required exact causeValidation
pnpm vp test packages/client-runtime/src/platform/persistence.test.ts packages/client-runtime/src/connection/registry.test.ts apps/web/src/connection/storage.test.ts apps/mobile/src/connection/storage.test.tspnpm vp checkpnpm vp run typecheckpnpm vp run lint:mobileNote
Medium Risk
Touches all client persistence error paths on web and mobile; behavior is mostly observability and error typing, but incorrect mapping could change how registry handles failed removals.
Overview
ConnectionPersistenceErrorno longer takes a free-formmessage. It now requires a typedoperation, optionalenvironmentId/threadId, and acause(Schema.Defect()); the user-facingmessageis derived fromoperation(e.g."Could not load thread.").Web and mobile connection storage drop local
persistenceError/shellPersistenceErrorhelpers and constructConnectionPersistenceErrorat each failure site, attaching environment (and thread where relevant) plus the underlying failure. Catalog/target mapping no longer wraps transient errors in a custom message—it passescausethrough. On mobile, thread file deletion is wrapped inEffect.tryso sync delete failures becomeConnectionPersistenceErrorinstead of escaping as defects.Tests add
persistence.test.tsfor the new shape and update registry tests to usecauseinstead ofmessage.Reviewed by Cursor Bugbot for commit 9e1df91. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Structure
ConnectionPersistenceErrorto carry contextual fields and standardize error messagesenvironmentId,threadId, andcausefields toConnectionPersistenceErrorin persistence.ts; themessagegetter now produces a standardized'Could not <operation>'string instead of embedding the original cause text.ConnectionPersistenceErrordirectly with contextual fields, removing theshellPersistenceErrorandpersistenceErrorhelper utilities.error.messagecontent will need to inspecterror.causeinstead.Macroscope summarized 9e1df91.