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

fix: pseudoterminal issue #4242

Merged
merged 2 commits into from
Dec 18, 2024
Merged

fix: pseudoterminal issue #4242

merged 2 commits into from
Dec 18, 2024

Conversation

Aaaaash
Copy link
Member

@Aaaaash Aaaaash commented Dec 17, 2024

Types

  • 🎉 New Features
  • 🐛 Bug Fixes
  • 📚 Documentation Changes
  • 💄 Code Style Changes
  • 💄 Style Changes
  • 🪚 Refactors
  • 🚀 Performance Improvements
  • 🏗️ Build System
  • ⏱ Tests
  • 🧹 Chores
  • Other Changes

Background or solution

Changelog

Summary by CodeRabbit

  • 新功能

    • 改进了终端管理逻辑,增强了终端实例及其相关进程的管理。
    • 更新了终端启动过程的跟踪,确保使用正确的终端标识符。
  • Bug 修复

    • 解决了在处理终端退出事件和输入接受方法时的标识符一致性问题。

@opensumi opensumi bot added the 🐞 bug Something isn't working label Dec 17, 2024
Copy link
Contributor

coderabbitai bot commented Dec 17, 2024

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.

warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning eslint > @humanwhocodes/config-array@0.13.0: Use @eslint/config-array instead
warning eslint > @humanwhocodes/config-array > @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
warning eslint > file-entry-cache > flat-cache > rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning eslint-import-resolver-typescript > glob@7.2.3: Glob versions prior to v9 are no longer supported
error Couldn't find any versions for "@opensumi/ide-dev-tool" that matches "workspace:*"

概述

演练

ext.host.terminal.ts 文件中,对 ExtHostTerminal 类进行了重要的修改。新增了一个私有属性 _terminalStartDeferreds,这是一个 Map,用于管理终端启动的延迟操作。修改涉及终端启动过程、终端 ID 处理和终端进程管理的改进,旨在更精确地跟踪和管理终端实例的生命周期。

变更

文件路径 变更概要
packages/extension/src/hosted/api/vscode/ext.host.terminal.ts - 新增私有属性 _terminalStartDeferreds
- 更新 $startExtensionTerminal 方法
- 修改 _setupExtHostProcessListeners 方法
- 调整终端 ID 处理逻辑

序列图

sequenceDiagram
    participant Extension
    participant ExtHostTerminal
    participant VSCode
    
    Extension->>ExtHostTerminal: 创建终端
    ExtHostTerminal->>ExtHostTerminal: 生成短 ID
    ExtHostTerminal->>ExtHostTerminal: 创建延迟对象
    ExtHostTerminal->>VSCode: 启动终端
    VSCode-->>ExtHostTerminal: 终端启动确认
    ExtHostTerminal->>ExtHostTerminal: 解析延迟对象
Loading

这个序列图展示了终端创建和启动的基本流程,突出了新增的短 ID 和延迟对象管理机制。

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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
Contributor

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

🧹 Nitpick comments (2)
packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (2)

81-82: 缺少对新添加属性的注释说明

请为新添加的私有属性 _terminalStartDeferreds 添加注释,以提高代码的可读性和维护性。


Line range hint 463-495: 优化 _setupExtHostProcessListeners 方法中的事件监听顺序

建议调整事件监听的顺序,确保在设置完所有监听器后再处理终端启动,以避免可能的事件丢失。

 this._terminalProcesses.set(shortId, p);

+ // 确保终端进程已存在,再处理启动逻辑
 this._terminalStartDeferreds.get(shortId)?.promise.then(() => {
   const awaitingStart = this._extensionTerminalAwaitingStart[shortId];
   if (awaitingStart && p instanceof ExtHostPseudoterminal) {
     p.startSendingEvents(awaitingStart.initialDimensions);
     delete this._extensionTerminalAwaitingStart[shortId];
   }
 });
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between cf537d1 and d903e0c.

📒 Files selected for processing (1)
  • packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (6 hunks)
🔇 Additional comments (3)
packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (3)

490-495: 避免异步操作中的竞态条件

在终端启动的 Promise 中,可能存在竞态条件,导致 startSendingEvents 在终端进程未完全准备好时被调用。

请确保在调用 startSendingEvents 前,终端进程已完全就绪。


451-456: ⚠️ Potential issue

确保终端进程存在时再调用 startSendingEvents

$startExtensionTerminal 方法中,您假设了终端进程一定存在,但实际情况下可能为空,需增加空值检查。

请在调用 startSendingEvents 前添加空值检查:

 const terminalProcess = this._terminalProcesses.get(this.getTerminalShortId(id));
+ if (!terminalProcess) {
+   return { message: 'Terminal process not found' };
+ }

Likely invalid or redundant comment.


459-459: 避免在终端未启动时调用 resolve

_terminalStartDeferreds 中调用 resolve 时,需要确保终端已正确启动,否则可能导致状态不一致。

请运行以下脚本以验证所有调用 resolve 的地方都确保了终端已启动:

@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 17, 2024

/next

@opensumi
Copy link
Contributor

opensumi bot commented Dec 17, 2024

🎉 PR Next publish successful!

3.6.3-next-1734429911.0

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.

Project coverage is 53.93%. Comparing base (b5efb7c) to head (b7af69e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...tension/src/hosted/api/vscode/ext.host.terminal.ts 63.63% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4242   +/-   ##
=======================================
  Coverage   53.92%   53.93%           
=======================================
  Files        1618     1618           
  Lines       98295    98300    +5     
  Branches    20113    20115    +2     
=======================================
+ Hits        53010    53014    +4     
- Misses      37623    37624    +1     
  Partials     7662     7662           
Flag Coverage Δ
jsdom 49.52% <63.63%> (+<0.01%) ⬆️
node 15.51% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@life2015 life2015 left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaaaash Aaaaash merged commit b95c686 into main Dec 18, 2024
11 checks passed
@Aaaaash Aaaaash deleted the fix/extension-pseudoterminal branch December 18, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants