Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enhance multiple platform #64

Merged
merged 11 commits into from
Feb 6, 2025
Merged

Conversation

gcfeng
Copy link
Collaborator

@gcfeng gcfeng commented Dec 23, 2024

Summary by CodeRabbit

  • New Features

    • Introduced enhanced chat functionality allowing users to seamlessly toggle between streaming and polling chat modes.
    • API now exposes detailed response errors for improved troubleshooting.
  • Bug Fixes

    • Addressed issues related to test functionality within the package.
  • Chores

    • Released a beta version update.
    • Streamlined platform-specific behaviors and event handling for enhanced stability.

Copy link

coderabbitai bot commented Dec 23, 2024

Walkthrough

This pull request removes multiplatform support from the @coze/taro-api package and updates related configurations. It consolidates API implementations by introducing a new CozeAPI class and deprecating older API files. Changes also standardize event-source behaviors, streamline platform mixin functions with environment checks, and improve test asynchronous handling. Additionally, new chat functionality is introduced with separate handlers for streaming and polling modes, and documentation along with package metadata is updated accordingly.

Changes

File(s) Change Summary
common/changes/@coze/taro-api/feat-taro-multi_*.json New JSON patch files documenting removal of multiplatform, an "example" patch, a "weapp" patch, exposure of response errors, and a test case fix.
examples/coze-js-taro3/config/index.ts Simplified compiler config: changed from an object with prebundle exclusions to a simple 'webpack5' string, removing multiplatform settings.
packages/coze-taro/README.md Removed the "Notes" section containing Taro@3 configuration instructions.
packages/coze-taro/package.json Updated version from "0.0.6" to "0.0.7-beta.1" and removed the "type": "module" field.
packages/coze-taro/src/event-source/request.ts Introduced a dynamic ES variable to select an environment-specific EventSource and enhanced error handling using APIError.
packages/coze-taro/src/mixins/platform.ts Renamed the parameter from _ to api and added conditional logic to apply platform-specific mixins based on the environment.
packages/coze-taro/src/api.ts, packages/coze-taro/src/index.ts
(and deleted: src/api/index.h5.ts, src/api/index.ts, src/api/types.ts)
Introduced a new CozeAPI implementation with a custom adapter and onBeforeAPICall hook, consolidating exports and removing outdated API files.
packages/coze-taro/src/event-source/base.ts
packages/coze-taro/src/event-source/index.tt.ts
packages/coze-taro/src/event-source/index.weapp.ts
Standardized event triggering: updated method signatures to accept structured arguments, added an isClosed flag, and introduced a safe JSON parser in the web implementation.
packages/coze-taro/test/setup.ts
packages/coze-taro/test/api.spec.ts
packages/coze-taro/test/request.spec.ts
packages/coze-taro/test/stubs.ts
(removed: packages/coze-taro/test/api.h5.spec.ts)
Updated test files and stubs: revised Taro mocks, improved asynchronous handling, and added assertions for the onBeforeAPICall callback.
examples/coze-js-taro/src/pages/index/index.tsx
examples/coze-js-taro3/src/pages/index/index.vue
Introduced new chat functionality with a toggle switch and separate handlers for streaming and polling chat modes.
packages/coze-taro/src/mixins/mixins.ts Removed try-catch blocks and the CozeError import to simplify error handling in asynchronous generator functions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI
    participant API

    User->>UI: Toggle chat mode (Streaming/Polling)
    alt Streaming Mode
        User->>UI: Click "Start Streaming Chat"
        UI->>API: Send streaming request
        API-->>UI: Return streaming response
        UI->>User: Display streaming message
    else Polling Mode
        User->>UI: Click "Start Polling Chat"
        UI->>API: Send polling request
        API-->>UI: Return polling response
        UI->>User: Display polling message
    end
Loading

Possibly related PRs

  • feat: support h5 #57: The changes in the main PR, which involve removing the "multiplatform" feature from the @coze/taro-api package, are related to the modifications in the retrieved PR that enhance the configuration for H5 support, particularly in how the @coze/taro-api package is handled in the build process.

Suggested reviewers

  • jsongo

Poem

In a field of code, I hop along,
New changes bloom like a springtime song.
Streams and polls now dance in the air,
Modules refined with the utmost care.
Hoppy and bright, I cheer this feat 🐇💻!
CodeRabbit leads with leaps so sweet.

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.

📜 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 5c13acd and 20d0c2f.

📒 Files selected for processing (4)
  • common/changes/@coze/api/feat-taro-multi_2025-02-06-01-53.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2025-02-06-01-53.json (1 hunks)
  • packages/coze-js/package.json (1 hunks)
  • packages/coze-taro/package.json (2 hunks)
✅ Files skipped from review due to trivial changes (3)
  • common/changes/@coze/api/feat-taro-multi_2025-02-06-01-53.json
  • common/changes/@coze/taro-api/feat-taro-multi_2025-02-06-01-53.json
  • packages/coze-js/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/coze-taro/package.json

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 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. (Beta)
  • @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.

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 77.63158% with 17 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/coze-taro/src/api.ts 88.33% 7 Missing ⚠️
packages/coze-taro/src/mixins/platform.ts 0.00% 6 Missing and 1 partial ⚠️
packages/coze-taro/src/event-source/request.ts 75.00% 2 Missing ⚠️
packages/coze-taro/src/index.ts 0.00% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (77.63%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
- Coverage   91.05%   90.38%   -0.68%     
==========================================
  Files          99       57      -42     
  Lines        4596     2226    -2370     
  Branches      900      413     -487     
==========================================
- Hits         4185     2012    -2173     
+ Misses        404      210     -194     
+ Partials        7        4       -3     
Components Coverage Δ
coze-js 92.80% <ø> (+0.39%) ⬆️
realtime-api ∅ <ø> (∅)
Files with missing lines Coverage Δ
packages/coze-taro/src/index.ts 0.00% <0.00%> (ø)
packages/coze-taro/src/event-source/request.ts 94.44% <75.00%> (-5.56%) ⬇️
packages/coze-taro/src/api.ts 88.33% <88.33%> (ø)
packages/coze-taro/src/mixins/platform.ts 0.00% <0.00%> (ø)

... and 52 files with indirect coverage changes

Copy link

@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: 1

🧹 Nitpick comments (3)
packages/coze-taro/test/setup.ts (1)

10-10: Consider adding support for multiple environments in the mock.
Currently, the mock always returns "TT". If you plan to test other environments (e.g., "WEAPP"), you might extend this to return different values based on test needs.

 getEnv: () => 'TT',
+// Possible extension:
+// getEnv: () => process.env.TEST_ENV || 'TT',
packages/coze-taro/src/event-source/request.ts (1)

9-15: Consider caching the environment value once.
You’re calling getEnv() multiple times, which might be repeated overhead. Storing or memoizing the environment value can improve performance and readability.

-const ES =
-  getEnv() === 'TT'
-    ? EventSourceTT
-    : getEnv() === 'WEAPP'
-      ? EventSourceWeapp
-      : EventSource;
+const env = getEnv();
+const ES = env === 'TT'
+  ? EventSourceTT
+  : env === 'WEAPP'
+    ? EventSourceWeapp
+    : EventSource;
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 12-14: packages/coze-taro/src/event-source/request.ts#L12-L14
Added lines #L12 - L14 were not covered by tests

README.md (1)

23-24: Improve example descriptions to highlight differences

Both examples have identical descriptions but target different Taro versions. Consider making the descriptions more specific to help users choose the appropriate example for their Taro version.

Apply this diff to clarify the differences:

-- [coze-js-taro](./examples/coze-js-taro) - Sample Call Up Demo for @coze/taro-api
-- [coze-js-taro3](./examples/coze-js-taro3) - Sample Call Up Demo for @coze/taro-api
+- [coze-js-taro](./examples/coze-js-taro) - Sample Demo for @coze/taro-api with Taro 2.x
+- [coze-js-taro3](./examples/coze-js-taro3) - Sample Demo for @coze/taro-api with Taro 3.x
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between c25cf8b and 0847f6a.

📒 Files selected for processing (9)
  • README.md (2 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-08-11.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-13-09.json (1 hunks)
  • examples/coze-js-taro3/config/index.ts (1 hunks)
  • packages/coze-taro/README.md (0 hunks)
  • packages/coze-taro/package.json (0 hunks)
  • packages/coze-taro/src/event-source/request.ts (2 hunks)
  • packages/coze-taro/src/mixins/platform.ts (1 hunks)
  • packages/coze-taro/test/setup.ts (1 hunks)
💤 Files with no reviewable changes (2)
  • packages/coze-taro/package.json
  • packages/coze-taro/README.md
✅ Files skipped from review due to trivial changes (2)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-08-11.json
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-13-09.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/coze-taro/src/event-source/request.ts

[warning] 12-14: packages/coze-taro/src/event-source/request.ts#L12-L14
Added lines #L12 - L14 were not covered by tests

packages/coze-taro/src/mixins/platform.ts

[warning] 10-15: packages/coze-taro/src/mixins/platform.ts#L10-L15
Added lines #L10 - L15 were not covered by tests

🔇 Additional comments (7)
packages/coze-taro/src/mixins/platform.ts (2)

1-2: Imports for environment detection are well-integrated.
The new import of getEnv allows for platform-based logic. Good addition.


4-5: Explicitly naming platform mixins improves maintainability.
By referencing each platform with a distinct alias, the code is clearer about which environment implementation is being used.

packages/coze-taro/src/event-source/request.ts (3)

1-2: New imports enable environment-specific EventSource selection.
These imports are necessary for the dynamic environment logic.


5-6: Multiple EventSource implementations recognized.
This setup is good for platform-specific event streaming logic.


28-28: Event source instantiation is correct.
No issues found with the usage. It properly references the selected ES.

examples/coze-js-taro3/config/index.ts (1)

26-26: Confirm removal of prebundle exclusions aligns with your build strategy.
By changing "compiler" to a simple string, you lose the explicit prebundle exclude configuration. If you no longer require that optimization, this is fine; otherwise, confirm your build sizes and performance remain acceptable.

README.md (1)

12-12: ⚠️ Potential issue

Fix inconsistent package directory path

The package link ./packages/coze-taro seems inconsistent with the package name @coze/taro-api. This might confuse users trying to locate the package source code.

Apply this diff to fix the path:

-| [@coze/taro-api](./packages/coze-taro) | Taro API | [![npm](https://img.shields.io/npm/v/@coze/taro-api.svg)](https://www.npmjs.com/package/@coze/taro-api) |
+| [@coze/taro-api](./packages/taro-api) | Taro API | [![npm](https://img.shields.io/npm/v/@coze/taro-api.svg)](https://www.npmjs.com/package/@coze/taro-api) |

Let's verify the correct package directory:

packages/coze-taro/src/mixins/platform.ts Show resolved Hide resolved
Copy link

@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: 2

🧹 Nitpick comments (12)
examples/coze-js-taro3/src/pages/index/index.vue (5)

3-5: Use descriptive labels or placeholders for toggling states
These lines introduce a switch that toggles between streaming and polling chats. Consider adding user-friendly labels or placeholders beside the switch to clarify its purpose.


6-9: Ensure loading or feedback is displayed during streaming
When users tap the "streaming chat" button, it might take a moment before any response appears. Consider providing a loading indicator or textual feedback while awaiting chunks to improve user experience.


10-13: Likewise, provide explicit feedback for polling
Similar to streaming chat, ensure there's a loading indicator or textual feedback to inform the user while waiting for the polled results.


29-29: Organize component imports
All component imports are properly declared here. Consider grouping them logically (UI components vs. custom classes) to keep the file neater.


46-47: Handle concurrency or user cancellation
A user might tap the "streaming chat" button multiple times or want to cancel the operation. Consider adding concurrency checks or a clear cancellation flow (e.g., a "Stop" button) to avoid overlapping streams or indefinite loops.

Also applies to: 68-68

examples/coze-js-taro/src/pages/index/index.tsx (3)

Line range hint 23-46: Handle streaming concurrency and cancellations
Similar to the Vue version, guard the handleStreamingChat method from repeated calls or consider adding a "Stop" button. If concurrency can occur, the method might process multiple parallel responses. Adding a check or resetting state can help.


56-90: Provide polling fallback or timeout
The polling logic is correct but may hang if the server takes too long to respond. Consider implementing timeouts or user-driven abort controls to avoid getting stuck waiting for data.


94-111: Enhance toggling UI
The toggling mechanism and conditional rendering look clean. Consider labeling or theming the streaming vs. polling areas for better visual distinction.

packages/coze-taro/src/api.ts (4)

1-6: Consider removing or narrowing the ESLint disable comment.
Disabling @typescript-eslint/no-invalid-void-type for the entire file might hide other valid issues. If needed only for specific lines, consider disabling it locally.


16-20: Replace void with undefined to align with TypeScript best practices.
Using void in a union type can be confusing. This change ensures clarity and consistency:

-  ) => ({ token?: string } | void) | Promise<{ token?: string } | void>;
+  ) => ({ token?: string } | undefined) | Promise<{ token?: string } | undefined>;
🧰 Tools
🪛 Biome (1.9.4)

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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


22-43: Consider handling token retrieval failures.
The request interceptor logic looks good. However, consider adding error-handling or validations in case onBeforeAPICall fails or returns invalid data.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 30-38: packages/coze-taro/src/api.ts#L30-L38
Added lines #L30 - L38 were not covered by tests


30-38: Add unit test coverage for critical functionality.
The new request interceptor, environment check, and Taro adapter logic are vital for API calls. Testing these lines ensures reliability and prevents regressions.

Would you like help creating unit tests or mocking Taro’s request in a testing environment?

Also applies to: 46-47, 50-56, 58-75

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 30-38: packages/coze-taro/src/api.ts#L30-L38
Added lines #L30 - L38 were not covered by tests

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0847f6a and 2d61948.

📒 Files selected for processing (10)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-30-11-19.json (1 hunks)
  • examples/coze-js-taro/src/pages/index/index.tsx (4 hunks)
  • examples/coze-js-taro3/src/pages/index/index.vue (4 hunks)
  • packages/coze-taro/src/api.ts (1 hunks)
  • packages/coze-taro/src/api/index.h5.ts (0 hunks)
  • packages/coze-taro/src/api/index.ts (0 hunks)
  • packages/coze-taro/src/api/types.ts (0 hunks)
  • packages/coze-taro/src/index.ts (1 hunks)
  • packages/coze-taro/test/api.h5.spec.ts (0 hunks)
  • packages/coze-taro/test/setup.ts (1 hunks)
💤 Files with no reviewable changes (4)
  • packages/coze-taro/src/api/index.h5.ts
  • packages/coze-taro/src/api/index.ts
  • packages/coze-taro/test/api.h5.spec.ts
  • packages/coze-taro/src/api/types.ts
✅ Files skipped from review due to trivial changes (1)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-30-11-19.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/coze-taro/src/api.ts

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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

🪛 GitHub Check: codecov/patch
packages/coze-taro/src/api.ts

[warning] 30-38: packages/coze-taro/src/api.ts#L30-L38
Added lines #L30 - L38 were not covered by tests


[warning] 46-47: packages/coze-taro/src/api.ts#L46-L47
Added lines #L46 - L47 were not covered by tests


[warning] 50-56: packages/coze-taro/src/api.ts#L50-L56
Added lines #L50 - L56 were not covered by tests


[warning] 58-75: packages/coze-taro/src/api.ts#L58-L75
Added lines #L58 - L75 were not covered by tests

🔇 Additional comments (9)
examples/coze-js-taro3/src/pages/index/index.vue (5)

18-18: Check for potential conflicts in imports
You've correctly imported Switch from @tarojs/components. Ensure there's no naming conflict if another UI library with a similarly named component is used in the future.


33-35: Validate initial states
Using ref(true), ref(''), and ref('') for the chat states is straightforward. Make sure the business logic fully supports the default streaming mode (true), particularly in any higher-level UI flows.


42-44: Confirm event detail usage in Vue
The framework automatically passes evt.detail.value to Switch’s change event. Confirm that it consistently returns a boolean in Taro + Vue environment.


77-107: Ensure robust error handling for polling
This async block is managing the polling process. If partial data is returned or an error occurs mid-request, be sure to handle partial results and user notifications.


110-115: Return all reactive references for improved debugging
Returning these refs and methods is correct. Consider adding console logging or dev-mode logs if debugging or troubleshooting is needed, particularly for asynchronous chat issues.

examples/coze-js-taro/src/pages/index/index.tsx (2)

4-4: Tuple import of Switch is correct
Including Switch from @tarojs/components is correct. No immediate issues found here.


10-12: Verify the default states
By default, streaming is set to true. Confirm that this initial setting matches expected user flows (e.g., do you consistently want to begin in streaming mode?).

packages/coze-taro/src/api.ts (1)

79-85: Mixins usage looks correct.
Conditionally applying both sharedMixins and platformMixins for non-web environments is a neat approach to keep platform-specific logic isolated.

packages/coze-taro/src/index.ts (1)

1-1: Consolidated export is clear.
Exporting CozeAPI and ClientOptions from ./api simplifies imports and clarifies their source.

packages/coze-taro/test/setup.ts Outdated Show resolved Hide resolved
packages/coze-taro/test/setup.ts Show resolved Hide resolved
Copy link

@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: 1

🧹 Nitpick comments (3)
packages/coze-taro/test/stubs.ts (2)

216-216: Use optional chaining for concise and clearer checks

Instead of multiple && checks, consider:

- if (data && data.workflow_id && data.workflow_id === 'fail') {
+ if (data?.workflow_id === 'fail') {

This simplifies the code and aligns with modern TypeScript best practices.

🧰 Tools
🪛 Biome (1.9.4)

[error] 216-216: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


249-253: Ensure timed abort aligns with user expectations

Aborting the task when the specified timeout elapses can be helpful but may surprise users if partial data has already been received. Consider adding a user-facing callback or additional status codes to clarify that the abort was triggered by a timeout.

packages/coze-taro/test/api.spec.ts (1)

38-42: Well-structured test verifying multiple API calls.

Verifying onBeforeAPICall is called twice captures both the streaming and the direct approach (workflows.runs.create). You may consider also validating the arguments used in onBeforeAPICall to ensure correctness.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2d61948 and df0fa45.

📒 Files selected for processing (5)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-31-02-23.json (1 hunks)
  • packages/coze-taro/test/api.spec.ts (2 hunks)
  • packages/coze-taro/test/request.spec.ts (1 hunks)
  • packages/coze-taro/test/setup.ts (1 hunks)
  • packages/coze-taro/test/stubs.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-31-02-23.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/coze-taro/test/setup.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/coze-taro/test/stubs.ts

[error] 216-216: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (5)
packages/coze-taro/test/stubs.ts (2)

197-203: Refined function signature for taroRequest

Great job adding the optional data and timeout parameters. This makes the API more flexible and allows better control of request behavior.


208-210: Non-streaming case returning a resolved Promise

The early return for workflow_id === 'nonStreaming' is cleanly handled. Double-check that this code path is exercised in unit tests to ensure future changes don't break this flow.

packages/coze-taro/test/request.spec.ts (1)

55-55: Good job awaiting all timers.

Awaiting vi.runAllTimersAsync() ensures that pending promises are properly resolved/rejected, making the test more reliable.

packages/coze-taro/test/api.spec.ts (2)

4-4: Use consistent mocking approach for .weapp modules.

Switching from .tt to .weapp is consistent with the environment detection changes. This helps keep the platform-specific logic clear and testable.


9-9: Maintain the same convention for event-source.

Similarly, mocking index.weapp aligns with the new platform name, providing consistency in the test suite.

packages/coze-taro/test/request.spec.ts Show resolved Hide resolved
Copy link

@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 (16)
packages/coze-taro/src/api.ts (2)

19-23: Consider improving type definition clarity.

The void type in the union might be confusing. Consider using undefined instead for better type clarity.

  onBeforeAPICall?: (
    options: unknown,
-  ) => ({ token?: string } | void) | Promise<{ token?: string } | void>;
+  ) => ({ token?: string } | undefined) | Promise<{ token?: string } | undefined>;
🧰 Tools
🪛 Biome (1.9.4)

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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


60-61: Consider using type-safe method handling.

Instead of using @ts-expect-error, consider defining a type-safe approach for method handling.

-          // @ts-expect-error -- ignore
-          method: (method ?? 'GET').toUpperCase(),
+          method: (method?.toUpperCase() ?? 'GET') as 'GET' | 'POST' | 'PUT' | 'DELETE',
common/changes/@coze/taro-api/feat-taro-multi_2025-01-22-10-14.json (1)

5-5: Enhance the change description.

The current comment "expose response error" is too brief. Consider providing more context about what errors are being exposed and why this change is beneficial.

-      "comment": "expose response error",
+      "comment": "feat: expose API response errors for better error handling and debugging capabilities",
examples/coze-js-taro3/src/pages/index/index.vue (3)

50-52: Remove commented-out code.

The commented-out abort timeout code should be removed if it's not needed.

-        // setTimeout(() => {
-        //   controller.abort();
-        // }, 10);

Also applies to: 81-83


72-74: Improve error handling.

Error handling currently only logs to console. Consider updating the respective message states to show error feedback to users.

      } catch (e) {
        console.log('failed: ', e);
+       streamingMessage.value = 'Failed to get response. Please try again.';
      }

Also applies to: 104-106


97-102: Simplify null check in reduce operation.

The messages || [] check is redundant since messages is already defaulted to an empty array in the destructuring.

-        pollingMessage.value = (messages || []).reduce((acc, cur) => {
+        pollingMessage.value = messages.reduce((acc, cur) => {
examples/coze-js-taro/src/pages/index/index.tsx (4)

28-30: Remove commented-out code.

The commented-out abort timeout code should be removed if it's not needed.

-        // setTimeout(() => {
-        //   controller.abort();
-        // }, 10);

Also applies to: 61-63


50-52: Improve error handling.

Error handling currently only logs to console. Consider updating the respective message states to show error feedback to users.

      } catch (e) {
        console.log('failed: ', e);
+       setStreamingMessage('Failed to get response. Please try again.');
      }

Also applies to: 86-88


65-65: Simplify null checks in polling handler.

  1. The messages = [] default is redundant with the messages || [] check in reduce.
  2. The reduce operation can be simplified.
-        const { messages = [] } = await clientRef.current.chat.createAndPoll(
+        const { messages } = await clientRef.current.chat.createAndPoll(
           {
             bot_id: process.env.TARO_APP_COZE_BOT_ID ?? '',
             user_id: 'abc',
             additional_messages: [
               { role: RoleType.User, content: 'hello', content_type: 'text' },
             ],
           },
           {
             signal: controller.signal,
           },
         );
         setPollingMessage(
-          (messages || []).reduce((acc, cur) => {
+          (messages ?? []).reduce((acc, cur) => {

Also applies to: 77-84


46-47: Consider extracting shared chat logic.

Both Vue and React implementations share similar chat handling logic. Consider creating a shared utility module to reduce code duplication and maintain consistency.

Example structure:

// shared/chat-utils.ts
export async function handleStreamingChat(client, message, onUpdate) {
  // ... shared streaming logic
}

export async function handlePollingChat(client, onUpdate) {
  // ... shared polling logic
}

Also applies to: 77-84

packages/coze-taro/src/event-source/index.tt.ts (1)

30-37: Adopt optional chaining for cleaner error-message extraction.
At line 32, you can simplify (e && e.errMsg) ?? 'fail' to e?.errMsg ?? 'fail':

- const errMsg = e instanceof Error ? e.message : ((e && e.errMsg) ?? 'fail');
+ const errMsg = e instanceof Error ? e.message : (e?.errMsg ?? 'fail');
🧰 Tools
🪛 Biome (1.9.4)

[error] 32-32: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/coze-taro/src/event-source/request.ts (2)

1-5: Imports offer refined error handling and environment detection.
Introducing APIError and getEnv enhances error classification and dynamic environment resolution. The ESLint ignore for magic numbers might be best minimized if those numeric constants are replaced or documented later, but it’s acceptable for now.


12-17: Dynamic EventSource selection is well-structured.
Consolidating environment logic in ES is a good approach. For readability, consider storing const env = getEnv(); and then using env in your ternary checks, but this is optional.

packages/coze-taro/src/event-source/index.weapp.ts (1)

65-78: Consider enhancing the chunk validation logic.

The new chunk validation logic is good, but could be made more robust by adding a maximum chunk size check to prevent potential memory issues with large responses.

 if (!this.cacheChunk) {
+  // Prevent memory issues with large chunks
+  if (chunk.length > 1024 * 1024) { // 1MB limit
+    this.trigger(EventName.Fail, {
+      errMsg: 'Chunk size exceeds limit',
+      data: { size: chunk.length },
+    });
+    this.abort();
+    return;
+  }
   const chunkJson = this.safeParseJSON(chunk);
🧰 Tools
🪛 Biome (1.9.4)

[error] 69-69: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/coze-taro/test/stubs.ts (1)

216-220: Consider using optional chaining for nested property access.

The condition checking could be simplified using optional chaining.

-    if (data && data.workflow_id && data.workflow_id === 'fail') {
+    if (data?.workflow_id === 'fail') {
🧰 Tools
🪛 Biome (1.9.4)

[error] 216-216: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/coze-taro/src/mixins/mixins.ts (1)

249-274: Consider adding error type information.

The error handling is consistent, but adding type information for the errors would improve error handling in consuming code.

 error: Error | null;
+// Add error type definition
+type StreamError = {
+  code: string;
+  message: string;
+  details?: unknown;
+};
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between df0fa45 and 64dcb71.

📒 Files selected for processing (27)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-08-11.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-13-09.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-30-11-19.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-31-02-23.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2025-01-22-10-14.json (1 hunks)
  • common/changes/@coze/taro-api/feat-taro-multi_2025-02-05-03-02.json (1 hunks)
  • examples/coze-js-taro/src/pages/index/index.tsx (4 hunks)
  • examples/coze-js-taro3/config/index.ts (1 hunks)
  • examples/coze-js-taro3/src/pages/index/index.vue (4 hunks)
  • packages/coze-taro/README.md (0 hunks)
  • packages/coze-taro/package.json (1 hunks)
  • packages/coze-taro/src/api.ts (1 hunks)
  • packages/coze-taro/src/api/index.h5.ts (0 hunks)
  • packages/coze-taro/src/api/index.ts (0 hunks)
  • packages/coze-taro/src/api/types.ts (0 hunks)
  • packages/coze-taro/src/event-source/base.ts (2 hunks)
  • packages/coze-taro/src/event-source/index.tt.ts (2 hunks)
  • packages/coze-taro/src/event-source/index.weapp.ts (4 hunks)
  • packages/coze-taro/src/event-source/request.ts (3 hunks)
  • packages/coze-taro/src/index.ts (1 hunks)
  • packages/coze-taro/src/mixins/mixins.ts (2 hunks)
  • packages/coze-taro/src/mixins/platform.ts (1 hunks)
  • packages/coze-taro/test/api.h5.spec.ts (0 hunks)
  • packages/coze-taro/test/api.spec.ts (1 hunks)
  • packages/coze-taro/test/request.spec.ts (1 hunks)
  • packages/coze-taro/test/setup.ts (1 hunks)
  • packages/coze-taro/test/stubs.ts (2 hunks)
💤 Files with no reviewable changes (5)
  • packages/coze-taro/src/api/types.ts
  • packages/coze-taro/test/api.h5.spec.ts
  • packages/coze-taro/README.md
  • packages/coze-taro/src/api/index.h5.ts
  • packages/coze-taro/src/api/index.ts
✅ Files skipped from review due to trivial changes (1)
  • common/changes/@coze/taro-api/feat-taro-multi_2025-02-05-03-02.json
🚧 Files skipped from review as they are similar to previous changes (11)
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-13-09.json
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-30-11-19.json
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-31-02-23.json
  • packages/coze-taro/test/setup.ts
  • examples/coze-js-taro3/config/index.ts
  • packages/coze-taro/src/mixins/platform.ts
  • packages/coze-taro/test/api.spec.ts
  • packages/coze-taro/test/request.spec.ts
  • common/changes/@coze/taro-api/feat-taro-multi_2024-12-23-08-11.json
  • packages/coze-taro/package.json
  • packages/coze-taro/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/coze-taro/src/event-source/index.tt.ts

[error] 32-32: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/coze-taro/src/api.ts

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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


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

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

packages/coze-taro/test/stubs.ts

[error] 216-216: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/coze-taro/src/event-source/index.weapp.ts

[error] 69-69: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (20)
packages/coze-taro/src/api.ts (3)

25-45: Well-structured initialization with secure token handling.

The constructor properly initializes the API with secure token handling and request interception.


64-90: Well-implemented error handling and response transformation.

The error handling is comprehensive, properly handling different error scenarios and normalizing headers.


100-105: Clean mixin application with proper environment check.

The method correctly applies mixins only for non-web environments.

examples/coze-js-taro3/src/pages/index/index.vue (3)

3-13: LGTM! Clean and well-structured template.

The template effectively uses conditional rendering and proper component bindings for the chat mode toggle functionality.


18-30: LGTM! Proper imports and component registration.

All required components are correctly imported and registered.


109-116: LGTM! Proper state and method exposure.

All required state variables and methods are correctly exposed in the return statement.

examples/coze-js-taro/src/pages/index/index.tsx (2)

1-13: LGTM! Clean imports and state management.

The code follows React hooks best practices for state management.


92-113: LGTM! Clean and well-structured JSX.

The template effectively uses conditional rendering and proper component bindings for the chat mode toggle functionality.

packages/coze-taro/src/event-source/base.ts (2)

7-7: Introduce a brief note on usage/reusability of isClosed.
You’ve added private isClosed = false;, which is straightforward. If this class can be restarted or reused, remember to reset isClosed to false appropriately. If the class is single-use only, then this is perfectly fine.


28-49: Logic for ensuring "Success/Fail" can only fire once looks solid.
The early return for subsequent success/fail events helps avoid multiple final states. The usage of args for data and errMsg is consistent with the new structured-object approach. Looks good overall.

packages/coze-taro/src/event-source/index.tt.ts (2)

28-28: Object-based event triggering for Chunk is consistent.
Replacing this.trigger(EventName.Chunk, msg) with this.trigger(EventName.Chunk, { data: msg }) standardizes the parameters passed to event handlers. Good job.


48-48: Consistent error message object in abort scenario.
Switching from a plain string to { errMsg: 'abort' } ensures all event triggers follow the same structured payload pattern.

packages/coze-taro/src/event-source/request.ts (3)

8-9: Including platform-specific EventSource imports is correct.
Importing EventSourceWeapp and EventSourceTT keeps your code well-organized for multi-environment usage.


31-31: Creating eventSource with a single environment-based class.
This is a clean way to avoid environment-specific branching in the rest of the code.


42-51: Comprehensive error object creation provides more clarity.
Mapping data.detail to data.error and using APIError.generate yields consistent error structures. Ensure that any unrecognized fields in data won’t silently overwrite important properties. Otherwise, this is an improvement over a basic Error object.

packages/coze-taro/src/event-source/index.weapp.ts (2)

32-33: LGTM! Improved error handling structure.

The error handling now includes structured data objects instead of just error messages, which provides more context for error handling.

Also applies to: 36-38


110-116: LGTM! Well-implemented safe JSON parsing.

The safeParseJSON method is a good addition that prevents JSON parsing errors from crashing the application.

packages/coze-taro/test/stubs.ts (2)

208-210: LGTM! Good addition of non-streaming support.

The early return for non-streaming workflows improves efficiency by avoiding unnecessary streaming setup.


249-253: LGTM! Proper timeout handling implementation.

The timeout handling is well-implemented, properly cleaning up resources by calling abort.

packages/coze-taro/src/mixins/mixins.ts (1)

69-92: Verify error handling after try-catch removal.

The removal of try-catch blocks means errors will propagate up the call stack. Ensure that calling code properly handles these errors to prevent unhandled rejections.

@jsongo jsongo added this pull request to the merge queue Feb 6, 2025
Merged via the queue into coze-dev:main with commit d3f8585 Feb 6, 2025
2 checks passed
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.

3 participants