Skip to content

Conversation

@weareoutman
Copy link
Member

@weareoutman weareoutman commented Apr 30, 2025

依赖检查

组件之间的依赖声明,是微服务组件架构下的重要信息,请确保其正确性。

请勾选以下两组选项其中之一:

  • 本次 MR 没有使用上游组件(例如框架、后台组件等)的较新版本提供的特性。

或者:

  • 本次 MR 使用了上游组件(例如框架、后台组件等)的较新版本提供的特性。
  • 在对应的文件中更新了该上游组件的依赖版本(或确认了当前声明的依赖版本已包含本次 MR 使用的新特性)。

提交信息检查

Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG,请检查工作内容与提交信息是否相符,并在以下每组选项中都依次确认。

破坏性变更是针对于下游使用者而言,可以通过本次改动对下游使用者的影响来识别变更类型:

  • 下游使用者不做任何改动,仍可以正常工作时,那么它属于普通变更。
  • 反之,下游使用者不做改动就无法正常工作时,那么它属于破坏性变更。

例如,构件修改了一个属性名,小产品 Storyboard 中需要使用新属性名才能工作,那么它就是破坏性变更。
又例如,构件还没有任何下游使用者,那么它的任何变更都是普通变更。

破坏性变更:

  • ⚠️ 本次 MR 包含破坏性变更的提交,请继续确认以下所有选项:
  • 没有更好的兼容方案,必须做破坏性变更。
  • 使用了 feat 作为提交类型。
  • 标注了 BREAKING CHANGE: 你的变更说明
  • 同时更新了本仓库中所有下游使用者的调用。
  • 同时更新了本仓库中所有下游使用者对该子包的依赖为即将发布的 major 版本。
  • 同时为其它仓库的 Migrating 做好了准备,例如文档或批量改动的方法。
  • 手动验证过破坏性变更在 Migrate 后可以正常工作。
  • 破坏性变更所在的提交没有意外携带其它子包的改动。

新特性:

  • 本次 MR 包含新特性的提交,且该提交不带有破坏性变更,并使用了 feat 作为提交类型。
  • 给新特性添加了单元测试。
  • 手动验证过新特性可以正常工作。

问题修复:

  • 本次 MR 包含问题修复的提交,且该提交不带有新特性或破坏性变更,并使用了 fix 作为提交类型。
  • 给问题修复添加了单元测试。
  • 手动验证过问题修复得到解决。

杂项工作:

即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动,例如修改注释、测试用例、开发文档等:

  • 本次 MR 包含杂项工作的提交,且该提交不带有问题修复、新特性或破坏性变更,并使用了 chore, docs, test 等作为提交类型。

Summary by CodeRabbit

  • 功能改进

    • 优化了错误处理逻辑,连接失败时将更详细地显示后端返回的错误信息,无论是 JSON 格式还是纯文本格式,提升了错误提示的准确性和可读性。
  • 测试

    • 新增了针对错误信息提取的单元测试,确保不同格式的错误响应都能被正确处理和记录日志。

@coderabbitai
Copy link

coderabbitai bot commented Apr 30, 2025

Walkthrough

本次变更主要聚焦于改进 SSE(服务器发送事件)连接建立阶段的错误处理逻辑,并对相关测试用例进行了补充。此外,还简化了代理服务中处理安全 Cookie 标志的策略。具体包括:在 SSE 连接建立失败时,优先尝试从响应体中提取更详细的错误信息(支持 JSON 和纯文本),并在测试中覆盖了这些场景;同时,移除了代理服务中一项关于 HTTPS/HTTP 环境下安全 Cookie 标志调整的旧有条件。

Changes

文件/路径 变更摘要
packages/utils/src/general/createSSEStream.ts 增强 onopen 错误处理逻辑:连接失败时,优先读取响应体内容,尝试解析为 JSON 并提取 error 字段,否则使用原始文本,提升错误信息的详细度。
packages/utils/src/general/createSSEStream.spec.ts 新增两条测试用例,覆盖 SSE 连接建立失败时响应体为 JSON 或纯文本错误信息的场景,验证错误信息提取与日志输出。
packages/brick-container/serve/getProxy.js 简化安全 Cookie 标志处理策略,移除“远程 HTTPS 本地 HTTP”条件,调整相关注释,其他逻辑保持不变。

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/brick-container/serve/getProxy.js

Oops! Something went wrong! :(

ESLint: 9.22.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs
at packageResolve (node:internal/modules/esm/resolve:839:9)
at moduleResolve (node:internal/modules/esm/resolve:908:18)
at defaultResolve (node:internal/modules/esm/resolve:1038:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
at ModuleLoader.resolve (node:internal/modules/esm/loader:525:25)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:246:38)
at ModuleJob._link (node:internal/modules/esm/module_job:126:49)

packages/utils/src/general/createSSEStream.spec.ts

Oops! Something went wrong! :(

ESLint: 9.22.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs
at packageResolve (node:internal/modules/esm/resolve:839:9)
at moduleResolve (node:internal/modules/esm/resolve:908:18)
at defaultResolve (node:internal/modules/esm/resolve:1038:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
at ModuleLoader.resolve (node:internal/modules/esm/loader:525:25)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:246:38)
at ModuleJob._link (node:internal/modules/esm/module_job:126:49)

packages/utils/src/general/createSSEStream.ts

Oops! Something went wrong! :(

ESLint: 9.22.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs
at packageResolve (node:internal/modules/esm/resolve:839:9)
at moduleResolve (node:internal/modules/esm/resolve:908:18)
at defaultResolve (node:internal/modules/esm/resolve:1038:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
at ModuleLoader.resolve (node:internal/modules/esm/loader:525:25)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:246:38)
at ModuleJob._link (node:internal/modules/esm/module_job:126:49)


📜 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 7f9c3e6 and 8267021.

📒 Files selected for processing (3)
  • packages/brick-container/serve/getProxy.js (1 hunks)
  • packages/utils/src/general/createSSEStream.spec.ts (1 hunks)
  • packages/utils/src/general/createSSEStream.ts (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/brick-container/serve/getProxy.js (1)
packages/brick-container/serve/env.js (1)
  • env (126-162)
packages/utils/src/general/createSSEStream.spec.ts (1)
packages/utils/src/general/createSSEStream.ts (1)
  • createSSEStream (43-171)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build (20.x)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/utils/src/general/createSSEStream.ts (1)

65-82: 改进的错误处理逻辑非常棒!

这个更改显著提升了错误处理的健壮性。在SSE连接失败时,通过优先尝试从响应体中读取更详细的错误信息,而不是仅依赖状态文本,可以提供更有意义的错误消息给用户。对JSON和非JSON格式错误信息的处理都很周全。

packages/brick-container/serve/getProxy.js (1)

119-128:

❓ Verification inconclusive

简化了Cookie安全标志处理逻辑

移除了"如果远程服务器是HTTPS但本地环境是HTTP,则清除安全标志"的条件,简化了代码逻辑。这样的更改使策略更加清晰直观:

  1. 如果是localhost且启用了cookieSameSiteNone,添加安全标志
  2. 如果本地环境是HTTPS,不做任何处理
  3. 否则,清除安全标志

确保在测试过程中验证这一变更不会影响身份验证流程。

验证本次更改是否会影响现有的身份验证流程:


🏁 Script executed:

#!/bin/bash
# 检查本地身份验证流程是否正常工作
echo "检查是否有依赖此逻辑的测试或其他代码"
rg -A 5 "cookieSameSiteNone" --glob "!*.{lock,md,json}"
echo "检查是否有依赖此逻辑的安全相关代码"
rg -A 5 "SameSite=None" --glob "!*.{lock,md,json}"

Length of output: 4489


请手动验证 Cookie 安全标志逻辑

本次将 getProxy.js 中对 SameSite=None; Secure 标志的处理简化为:

  1. 当启用 cookieSameSiteNonehost === "localhost" 时,添加 安全标志;
  2. 当本地 serve 使用 HTTPS 时,保持原样(不做任何修改);
  3. 其他场景下,清除 该标志。

经检索,仅有以下文件引入或依赖该逻辑:

  • packages/brick-container/serve/env.jscookieSameSiteNone 的默认配置与来源;
  • packages/brick-container/serve/getProxy.js:策略计算与 set-cookie 头部处理。

未发现其他自动化测试或业务代码依赖此分支逻辑。请在真实的身份验证流程下,特别是代理远端服务、HTTP/HTTPS 混合等场景中,手动验证 Cookie 的最终行为是否符合预期。

packages/utils/src/general/createSSEStream.spec.ts (2)

142-166: 很好的测试JSON格式错误响应

这个测试很好地验证了当服务器返回包含JSON格式错误信息的响应时,错误处理逻辑能正确提取和传播错误消息。测试覆盖了模拟响应、错误提取和日志记录等方面,确保了功能的正确性。


168-187: 很好的测试非JSON格式错误响应

这个测试验证了当服务器返回纯文本错误信息时的处理逻辑。与JSON错误测试一起,这两个测试全面覆盖了新增的错误处理功能,确保了不同类型的错误响应都能被正确处理和传播。

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 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 generate sequence diagram to generate a sequence diagram of the changes in 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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request enhances error handling in the SSE stream creation logic and updates secure cookie handling in the proxy configuration. Key changes include:

  • Updating the onopen handler in the SSE stream to parse error responses and re-throw a more specific error.
  • Adding new tests to verify error handling for both JSON and non-JSON error responses in createSSEStream.
  • Modifying the secure cookie strategy logic in getProxy.js to simplify the conditional branches.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/utils/src/general/createSSEStream.ts Enhances error handling in the onopen event by parsing the response text and extracting JSON error information where possible.
packages/utils/src/general/createSSEStream.spec.ts Adds tests for error scenarios in createSSEStream, specifically covering both JSON error responses and non-JSON error responses.
packages/brick-container/serve/getProxy.js Updates the secure cookie strategy logic; the comments and branching have been modified to remove the https→http "clear" scenario.
Comments suppressed due to low confidence (1)

packages/brick-container/serve/getProxy.js:119

  • The comment still references a branch for clearing secure cookie flags that has been removed in the updated code. Please update the comment to reflect the new logic.
// - If the server is https, but the local is http, clear the secure cookie flags;

@codecov
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.25%. Comparing base (7f9c3e6) to head (8267021).
Report is 3 commits behind head on v3.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##               v3    #4686      +/-   ##
==========================================
- Coverage   95.26%   95.25%   -0.01%     
==========================================
  Files         209      209              
  Lines        9037     9044       +7     
  Branches     1737     1738       +1     
==========================================
+ Hits         8609     8615       +6     
  Misses        319      319              
- Partials      109      110       +1     
Files with missing lines Coverage Δ
packages/utils/src/general/createSSEStream.ts 95.40% <100.00%> (-0.85%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cypress
Copy link

cypress bot commented Apr 30, 2025

next-core    Run #11201

Run Properties:  status check passed Passed #11201  •  git commit e45b78f86f ℹ️: Merge 8267021f07dfb982402429fdb82a1e199b1a81c1 into 7f9c3e61a18cf01cee61959e345e...
Project next-core
Branch Review steve/v3-sse
Run status status check passed Passed #11201
Run duration 00m 26s
Commit git commit e45b78f86f ℹ️: Merge 8267021f07dfb982402429fdb82a1e199b1a81c1 into 7f9c3e61a18cf01cee61959e345e...
Committer Shenwei Wang
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 16
View all changes introduced in this branch ↗︎

@weareoutman weareoutman merged commit 393dcf7 into v3 Apr 30, 2025
7 of 8 checks passed
@weareoutman weareoutman deleted the steve/v3-sse branch April 30, 2025 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants