Skip to content

Conversation

@techcodie
Copy link

  • Add ResolverNotSupported error variant to TlsError enum
  • Replace unimplemented!() panics with proper error returns in create_client_config
  • Improves error handling by returning graceful errors instead of panicking

fix(tsc): improve error message for invalid 'this' parameter syntax

  • Add helpful error message rewrite for TS2684 (invalid 'this' parameter)
  • Provides guidance on correct React/Preact component typing syntax
  • Helps users understand the difference between 'this' parameter and function return types

- Add ResolverNotSupported error variant to TlsError enum
- Replace unimplemented!() panics with proper error returns in create_client_config
- Improves error handling by returning graceful errors instead of panicking

fix(tsc): improve error message for invalid 'this' parameter syntax

- Add helpful error message rewrite for TS2684 (invalid 'this' parameter)
- Provides guidance on correct React/Preact component typing syntax
- Helps users understand the difference between 'this' parameter and function return types
@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

Two changes across TypeScript compiler diagnostics and TLS error handling: (1) New diagnostic rewrite branch for code 2684 handling 'this' parameter messages with React/Preact component typing guidance; (2) New TlsError variant replacing unimplemented!() when resolver-based TLS keys are used in client connections.

Changes

Cohort / File(s) Summary
TypeScript Compiler Diagnostics
cli/tsc/go.rs
Added new diagnostic rewrite branch in maybe_rewrite_message for code 2684. When message contains "'this' parameter", appends a note explaining that the 'this' parameter types the context rather than the function return type, with example patterns for typing React/Preact components (FC<Props> or function returning JSX.Element).
TLS Error Handling
ext/tls/lib.rs
Introduced new public error variant ResolverNotSupported in the TlsError enum. Updated create_client_config to return Err(TlsError::ResolverNotSupported) instead of unimplemented!() when TlsKeys::Resolver(_) is encountered in both code paths (with and without unsafely_ignore_certificate_errors).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • cli/tsc/go.rs: Straightforward conditional branch addition with message formatting logic; limited scope and isolated to diagnostic rewriting
  • ext/tls/lib.rs: Simple enum variant addition and direct replacement of panic paths with explicit error returns; well-contained change with clear intent

Poem

🐰 A fuzzy-eared fix in the TypeScript stream,
Where 'this' now whispers its React-component dream,
And over in TLS, no more panics to fear—
Just proper resolver errors, crystal and clear!

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing unimplemented!() with a proper TlsError for TLS resolver handling. However, it focuses only on the TLS change and omits the significant TSC error message improvement also included in the changeset.
Description check ✅ Passed The description is well-organized and clearly relates to the changeset, covering both the TLS resolver error handling fix and the TSC error message improvements with appropriate context and rationale.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c0f289 and 738f856.

📒 Files selected for processing (2)
  • cli/tsc/go.rs (1 hunks)
  • ext/tls/lib.rs (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test release linux-x86_64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: test release macos-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: build libs
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: lint debug windows-x86_64
🔇 Additional comments (2)
cli/tsc/go.rs (1)

185-189: TS2684 rewrite looks good; guidance is clear and preserves original message

This branch cleanly appends React/Preact-specific guidance while keeping the original TS2684 text intact and is gated on both the code and a 'this' parameter substring, which should keep it reasonably targeted.

One minor thought: since this depends on TypeScript’s exact phrasing, it may be worth keeping an eye on upstream TS message wording changes so we don’t silently stop applying this rewrite or start applying it in cases where the React/Preact note is less relevant.

ext/tls/lib.rs (1)

54-57: Replacing unimplemented!() with ResolverNotSupported is a solid behavioral improvement

The new TlsError::ResolverNotSupported variant and its use in both TlsKeys::Resolver(_) match arms make the client config path fail gracefully instead of panicking, which is much better for robustness and user-facing error handling.

The chosen error class "NotSupported" and message clearly communicate that resolver-based keys are only intended for server-side usage.

The only follow-up to double-check is:

  • Any exhaustive matches on TlsError in this crate or downstream consumers that may now need to handle ResolverNotSupported.
  • Error expectation tests for client TLS config that may previously have assumed a panic and should now assert on this specific variant / JS error class.

Otherwise, this change aligns well with the stated PR goal.

Also applies to: 316-318, 351-353

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant