Skip to content

feat(client)!: update ClientRetryPlugin options#280

Merged
dinwwwh merged 2 commits into
mainfrom
feat/client/update-ClientRetryPlugin
Mar 21, 2025
Merged

feat(client)!: update ClientRetryPlugin options#280
dinwwwh merged 2 commits into
mainfrom
feat/client/update-ClientRetryPlugin

Conversation

@dinwwwh
Copy link
Copy Markdown
Member

@dinwwwh dinwwwh commented Mar 21, 2025

  • rename eventIteratorLastRetrylastEventRetry
  • rename eventIteratorLastEventIdlastEventId
  • add some missing tests

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation

    • Updated plugin documentation to reflect the revised default retry configuration.
  • New Features

    • Enhanced the retry mechanism with a customizable decision process and improved error context during retry attempts.
  • Refactor

    • Streamlined internal retry logic by standardizing parameter naming for improved clarity and consistency.
    • Updated default retry delay configuration for better functionality.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 21, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
orpc ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 11:33am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 21, 2025

Walkthrough

The changes update the retry mechanism in the ClientRetryPlugin. The default value for the retryDelay parameter now references lastEventRetry instead of eventIteratorLastRetry. A new shouldRetry function is added to the test context, ensuring that retries occur with more detailed context (including lastEventId) during error handling. Property names within the plugin and associated tests have been consistently renamed for clarity and consistency.

Changes

Files Change Summary
apps/.../client-retry.md Updated the default retryDelay function to reference lastEventRetry instead of eventIteratorLastRetry.
packages/client/src/plugins/retry.test.ts, packages/client/src/plugins/retry.ts Introduced a new shouldRetry function in tests, adjusted error handling to include lastEventId, and renamed properties from eventIteratorLastRetry/eventIteratorLastEventId to lastEventRetry/lastEventId throughout.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant P as ClientRetryPlugin
    participant R as Retry Logic
    participant S as shouldRetry Func

    C->>P: Call client with context ({ retry, retryDelay, shouldRetry })
    P->>R: Begin retry process (using lastEventRetry & lastEventId)
    R->>S: Invoke shouldRetry(error, lastEventId)
    S-->>R: Return true (retry allowed)
    R-->>P: Continue with updated retry attempt
Loading

Possibly related PRs

  • unnoq/orpc#267: Adjusts the event iterator retry behavior, relating to the modifications of the default retry mechanism in this change.
  • unnoq/orpc#265: Introduced the initial ClientRetryPlugin logic; the current changes refine and update this functionality.

Poem

I'm a rabbit hopping through the code,
With changes that lighten my digital load.
Renamed delays and a retry so true,
"shouldRetry" guides the path anew.
Bunny bytes dance in a playful mode!


📜 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 bfd5c23 and 30190a3.

📒 Files selected for processing (1)
  • packages/client/src/plugins/retry.test.ts (6 hunks)
🔇 Additional comments (10)
packages/client/src/plugins/retry.test.ts (10)

224-227: Clear naming improvement and enhanced metadata.

The test name now better reflects the purpose: testing retry with complete metadata instead of just ID. The addition of the retry: 5678 field in the event metadata aligns with the PR goal of renaming properties for clarity and consistency.


231-233: Good addition of the shouldRetry function.

Adding the shouldRetry mock function and including it in the context properly tests the functionality of conditional retries with the renamed properties. This is a necessary test enhancement that complements the property renaming changes.


244-244: Proper assertion update for metadata.

The expectation has been correctly updated to verify that both the ID and retry value are present in the event metadata, ensuring the complete metadata is being properly passed through the retry mechanism.


257-265: Important assertion for shouldRetry behavior.

This new assertion block verifies that the shouldRetry function is called with the correct parameters, particularly validating that lastEventId and lastEventRetry (the renamed properties) are properly passed in the context. This is essential for ensuring backward compatibility with the renamed properties.


268-273: Improved error metadata handling.

The test name change reflects the enhanced functionality more accurately. The error now includes both an ID and a retry value, allowing for better testing of the retry mechanism with complete metadata during error scenarios.


275-277: Consistent implementation of shouldRetry.

Similar to the previous test, this one adds the shouldRetry functionality but with different retry parameters (retry: 1 instead of 3), ensuring the retry mechanism works correctly with different configurations.


297-307: Complete testing of iterator behavior.

This new assertion block properly tests the iterator's behavior after a retry, ensuring that the values are correctly yielded and that the error with its metadata is properly propagated. This is important for validating the complete retry flow.


312-320: Thorough verification of shouldRetry parameters.

These assertions ensure that the shouldRetry function is called with all necessary context information, including the properly renamed properties, validating that the retry mechanism correctly handles the metadata from errors.


375-378: Clever implementation of unique error IDs.

Using a counter to generate unique IDs for each error allows for more precise testing of the retry mechanism, ensuring that each retry attempt can be tracked and verified individually.

🧰 Tools
🪛 Biome (1.9.4)

[error] 376-378: This generator function doesn't contain yield.

(lint/correctness/useYield)


392-407: Enhanced onRetry assertions with lastEventId.

The onRetry call expectations now include verification of the lastEventId parameter, ensuring that the retry mechanism correctly passes this information to the onRetry callback. This completes the testing of the renamed properties throughout the retry flow.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 21, 2025

Open in Stackblitz

More templates

@orpc/arktype

npm i https://pkg.pr.new/@orpc/arktype@280

@orpc/client

npm i https://pkg.pr.new/@orpc/client@280

@orpc/contract

npm i https://pkg.pr.new/@orpc/contract@280

@orpc/openapi

npm i https://pkg.pr.new/@orpc/openapi@280

@orpc/openapi-client

npm i https://pkg.pr.new/@orpc/openapi-client@280

@orpc/react-query

npm i https://pkg.pr.new/@orpc/react-query@280

@orpc/server

npm i https://pkg.pr.new/@orpc/server@280

@orpc/solid-query

npm i https://pkg.pr.new/@orpc/solid-query@280

@orpc/shared

npm i https://pkg.pr.new/@orpc/shared@280

@orpc/standard-server

npm i https://pkg.pr.new/@orpc/standard-server@280

@orpc/standard-server-fetch

npm i https://pkg.pr.new/@orpc/standard-server-fetch@280

@orpc/standard-server-node

npm i https://pkg.pr.new/@orpc/standard-server-node@280

@orpc/svelte-query

npm i https://pkg.pr.new/@orpc/svelte-query@280

@orpc/valibot

npm i https://pkg.pr.new/@orpc/valibot@280

@orpc/vue-colada

npm i https://pkg.pr.new/@orpc/vue-colada@280

@orpc/vue-query

npm i https://pkg.pr.new/@orpc/vue-query@280

@orpc/zod

npm i https://pkg.pr.new/@orpc/zod@280

commit: 30190a3

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/client/src/plugins/retry.ts (1)

93-93: Consider using undefined instead of void in union type

The static analysis tool flagged that using void in a union type can be confusing. Consider using undefined instead for better clarity.

-  let unsubscribe: void | (() => void)
+  let unsubscribe: undefined | (() => void)
🧰 Tools
🪛 Biome (1.9.4)

[error] 93-93: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ea8333 and bfd5c23.

📒 Files selected for processing (3)
  • apps/content/docs/plugins/client-retry.md (1 hunks)
  • packages/client/src/plugins/retry.test.ts (6 hunks)
  • packages/client/src/plugins/retry.ts (9 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/client/src/plugins/retry.ts

[error] 93-93: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

🔇 Additional comments (17)
apps/content/docs/plugins/client-retry.md (1)

58-58: LGTM - Documentation updated to align with property renaming

The documentation has been correctly updated to reference the renamed property lastEventRetry instead of eventIteratorLastRetry in the default value description for the retryDelay parameter.

packages/client/src/plugins/retry.test.ts (8)

231-233: LGTM - Added shouldRetry function to event iterator test

The shouldRetry mock function and its inclusion in the client call is implemented correctly. This enhancement improves test coverage by verifying that the retry logic passes the correct context to the retry decision function.


258-265: LGTM - Added verification for shouldRetry parameters

Good addition of expectations to verify that the shouldRetry function is called with the correct parameters, including the lastEventId. This ensures the property renaming maintains correct behavior during retries.


275-277: LGTM - Consistent application of shouldRetry to error test

Consistently applying the same pattern to the error test case with shouldRetry function and updated client parameters.


301-308: LGTM - Added verification for shouldRetry in error context

Proper verification that the shouldRetry function is called with the correct parameters when errors contain event metadata.


363-366: LGTM - Enhanced error handling test with timing

Good improvement to the error handling test by adding a time counter and including it in the error metadata, which helps verify that the lastEventId is properly tracked across retries.

🧰 Tools
🪛 Biome (1.9.4)

[error] 364-366: This generator function doesn't contain yield.

(lint/correctness/useYield)


380-380: LGTM - Updated onRetry parameter verification

Correctly updated the expected parameters for onRetry to include the lastEventId, ensuring consistent behavior with the renamed properties.


387-387: LGTM - Consistent onRetry parameter verification

Consistently updated all onRetry verification calls with the correct lastEventId property.


394-394: LGTM - Complete onRetry parameter verification

All verification points for onRetry consistently updated to check for the proper lastEventId in retry attempt options.

packages/client/src/plugins/retry.ts (8)

9-10: LGTM - Renamed interface properties for clarity

The property names have been renamed from eventIteratorLastRetry to lastEventRetry and eventIteratorLastEventId to lastEventId, which makes them clearer and more concise.


27-27: LGTM - Updated JSDoc default value

Documentation for the default value of the retryDelay parameter has been updated to use the renamed property lastEventRetry.


73-73: LGTM - Updated default retry delay implementation

The implementation of the default retry delay function has been updated to use the renamed property lastEventRetry.


91-92: LGTM - Renamed local variables

Local variables have been renamed from eventIteratorLastEventId and eventIteratorLastRetry to lastEventId and lastEventRetry for consistency.


100-101: LGTM - Updated client options with renamed property

The client options object correctly includes the renamed lastEventId property.


110-111: LGTM - Updated attempt options with renamed properties

The attempt options object correctly uses the renamed properties lastEventId and lastEventRetry.


183-184: LGTM - Updated metadata retrieval

The code for retrieving and setting metadata from event items has been updated to use the renamed properties.


194-195: LGTM - Updated error metadata handling

The error metadata handling has been updated to use the renamed properties consistently.

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