Skip to content

Conversation

@wkordalski
Copy link
Contributor

@wkordalski wkordalski commented May 27, 2025

Related GitHub Issue

Closes: #4034

Description

Both Task.addToClineMessages and Task.updateClineMessage are async functions.
However, when addToClineMessages is called, it is awaited,
and when updateClineMessage is called, it is not awaited.

This can result in reordering the execution of these functions,
and thus reordering the this.emit("message", ...) calls (and also messages sent to webview probably).

The solution is to await on updateClineMessage. It should not introduce much performance penalty, as it only sends message to webview and emits an event (addToClineMessages does much more and it is awaited).

Test Procedure

Subscribe to message event, run a task and watch the order of message events.
The procedure must be run multiple times — not always the events are emitted in wrong order.

However, probably as the change is very small and obvious, it does not need additional testing.

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint)¹.
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.²
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

¹ Getting Invalid option '--ext' - perhaps you meant '-c'? from eslint.
² Getting Missing script: changeset

Screenshots / Videos

Documentation Updates

Does this PR necessitate updates to user-facing documentation?

  • No documentation updates are required.
  • Yes, documentation updates are required.

Additional Notes

Get in Touch

Discord handle: wkordalski


Important

Fixes message event order by awaiting updateClineMessage() in Task.ts to ensure correct event emission sequence.

  • Behavior:
    • Await updateClineMessage() in Task.ts to ensure correct order of message events.
    • Affects ask(), say(), and other methods where updateClineMessage() is called.
  • Functions:
    • updateClineMessage() now awaited in ask(), say(), and 2 other places in Task.ts.
  • Misc:

This description was created by Ellipsis for ede7cc7. You can customize this summary. It will automatically update as commits are pushed.

@wkordalski wkordalski requested review from cte and mrubens as code owners May 27, 2025 11:38
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label May 27, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Preliminary Review] in Roo Code Roadmap May 28, 2025
@daniel-lxs daniel-lxs added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels May 28, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented May 30, 2025

Hey @wkordalski, Thank you for the repro branch, unfortunately I wasn't able to run the tests from your branch correctly and thus couldn't reproduce the issue.

Does this issue have any noticeable impact in the user experience? I saw you mentioned that we are reordering the messages anyways.

I'll be closing this temporarily until we have more information.

@daniel-lxs daniel-lxs closed this May 30, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap May 30, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

message events are emitted in wrong order

3 participants