ErrorInfo.cause: must be an errorinfo if present, not a string

[SimonWoolf]

ErrorInfo.cause should never have been permitted to be an Error or a string in the typings; if present it must be an ErrorInfo (TI1).

AFAICS it's never a string, there were some places where it could be an Error which I've fixed.

Summary by CodeRabbit

• Bug Fixes & Improvements
  • Standardized error handling across the SDK to ensure consistent and reliable error reporting.
  • Improved error information structure by restricting error cause types to maintain type safety.
  • Enhanced error messages in service worker registration failures for better clarity.

[coderabbitai]

Walkthrough

This PR narrows the type definitions and implementations of error cause handling across the codebase, restricting the cause parameter from accepting string | Error | ErrorInfo unions to strictly accepting ErrorInfo types, with corresponding implementation updates to comply with the stricter type contracts.

Changes

Cohort / File(s)	Summary
Error Type Definitions ably.d.ts, src/common/lib/types/errorinfo.ts	Narrowed cause property type from string | Error | ErrorInfo to ErrorInfo (or ErrorInfo | PartialErrorInfo for PartialErrorInfo). Updated ErrorInfo and PartialErrorInfo constructor signatures to enforce the stricter type constraint.
Type-constrained Declarations src/common/lib/client/realtimechannel.ts	Restricted errorReason field from ErrorInfo | string | null to ErrorInfo | null.
Error Handling Implementations src/platform/nodejs/lib/util/crypto.ts, src/platform/web/lib/util/crypto.ts, src/plugins/push/getW3CDeviceDetails.ts	Removed Error objects as cause parameters in ErrorInfo constructors; implementations either omit the cause parameter or embed error messages directly into the ErrorInfo message string.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Errors once loose, now tethered tight,
No strings or native Errors in sight,
Just ErrorInfo causes, pure and clean,
The strictest contracts we've ever seen!
Type safety hops to new heights,
As cause fields settle into rights. 🎯

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main change: narrowing ErrorInfo.cause to only accept ErrorInfo types, not strings or Error objects.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch error-cause

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/common/lib/client/realtimechannel.ts (1)

76-81: ⚠️ Potential issue | 🟠 Major

Public typings for errorReason don't match the implementation in both RealtimeChannel and Connection.

Implementation at src/common/lib/client/realtimechannel.ts line 80 declares errorReason: ErrorInfo | null and initializes it as null (line 130), but ably.d.ts still declares both as non-nullable:

• RealtimeChannel.errorReason: ErrorInfo (line 2452)
• Connection.errorReason: ErrorInfo (line 3263)

This mismatch can mislead TypeScript consumers into unsafe dereferences. Update both declarations in ably.d.ts to include | null.

Suggested fix

-  errorReason: ErrorInfo;
+  errorReason: ErrorInfo | null;

Apply to both RealtimeChannel and Connection interfaces in ably.d.ts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/common/lib/client/realtimechannel.ts` around lines 76 - 81, The public
type for errorReason is declared non-nullable in ably.d.ts but the runtime
classes RealtimeChannel and Connection set errorReason to null; update both
interface/type declarations for RealtimeChannel.errorReason and
Connection.errorReason in ably.d.ts to be nullable (ErrorInfo | null) so the
typings match the implementation and avoid unsafe dereferences; ensure you
change both occurrences (the RealtimeChannel and Connection definitions)
consistently and run type checks.

🧹 Nitpick comments (1)

src/plugins/push/getW3CDeviceDetails.ts (1)

85-89: Preserve ErrorInfo cause when available (optional).
Right now you drop the cause even when err is already an ErrorInfo. You can retain it without violating the new contract and also avoid "undefined" when err isn’t an Error.

♻️ Suggested tweak

   } catch (err) {
-    machine.handleEvent(
-      new GettingPushDeviceDetailsFailed(
-        new ErrorInfo('Failed to register service worker: ' + (err as Error).message, 50000, 500),
-      ),
-    );
+    const cause = err instanceof ErrorInfo ? err : undefined;
+    const errMessage = err instanceof Error ? err.message : String(err);
+    machine.handleEvent(
+      new GettingPushDeviceDetailsFailed(
+        new ErrorInfo('Failed to register service worker: ' + errMessage, 50000, 500, cause),
+      ),
+    );
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/push/getW3CDeviceDetails.ts` around lines 85 - 89, In the catch
block where you construct GettingPushDeviceDetailsFailed using new
ErrorInfo(...), preserve the original error as the ErrorInfo cause when
available: detect if err is already an ErrorInfo (or at least an Error) and pass
it into the new ErrorInfo as the cause instead of dropping it; update the catch
that creates new ErrorInfo(...) inside GettingPushDeviceDetailsFailed so it
forwards err (when err instanceof ErrorInfo or err instanceof Error) as the
cause argument to ErrorInfo to avoid "undefined" and retain original error
details.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/common/lib/client/realtimechannel.ts`:
- Around line 76-81: The public type for errorReason is declared non-nullable in
ably.d.ts but the runtime classes RealtimeChannel and Connection set errorReason
to null; update both interface/type declarations for RealtimeChannel.errorReason
and Connection.errorReason in ably.d.ts to be nullable (ErrorInfo | null) so the
typings match the implementation and avoid unsafe dereferences; ensure you
change both occurrences (the RealtimeChannel and Connection definitions)
consistently and run type checks.

---

Nitpick comments:
In `@src/plugins/push/getW3CDeviceDetails.ts`:
- Around line 85-89: In the catch block where you construct
GettingPushDeviceDetailsFailed using new ErrorInfo(...), preserve the original
error as the ErrorInfo cause when available: detect if err is already an
ErrorInfo (or at least an Error) and pass it into the new ErrorInfo as the cause
instead of dropping it; update the catch that creates new ErrorInfo(...) inside
GettingPushDeviceDetailsFailed so it forwards err (when err instanceof ErrorInfo
or err instanceof Error) as the cause argument to ErrorInfo to avoid "undefined"
and retain original error details.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba5b64 and 5453ce6.

📒 Files selected for processing (6)

• ably.d.ts
• src/common/lib/client/realtimechannel.ts
• src/common/lib/types/errorinfo.ts
• src/platform/nodejs/lib/util/crypto.ts
• src/platform/web/lib/util/crypto.ts
• src/plugins/push/getW3CDeviceDetails.ts