-
Notifications
You must be signed in to change notification settings - Fork 358
Communication: Fix message cut in half while sending issue
#12036
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
base: develop
Are you sure you want to change the base?
Communication: Fix message cut in half while sending issue
#12036
Conversation
WalkthroughAdds a 250ms delay before creating a post in the message inline input component to wait for the markdown editor's textChangedEmitDelay; corresponding unit tests updated to advance time by 300ms to accommodate the delay. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/communication/message/message-inline-input/message-inline-input.component.spec.ts (1)
203-217: Rename test or switch to creation test data to match test name.The test at lines 203-217 is named "should clear draft after successful post creation", but the
beforeEach(line 143) usesdirectMessageUser1—an existing message withid: 9. This triggers the update path, not the creation path. The test is functionally correct (update has notick(300)delay), but the name is misleading.Either:
- Rename to "should clear draft after successful post update" to reflect actual behavior, or
- Switch to
metisPostToCreateUser1and addtick(300)to test the creation path as the name implies.
🧹 Nitpick comments (1)
src/main/webapp/app/communication/message/message-inline-input/message-inline-input.component.spec.ts (1)
69-76: Consider adding a comment to explain the 300ms delay.The
tick(300)accommodates the 250ms delay in the component'sconfirm()method, but this relationship isn't immediately obvious. A brief comment would help future maintainers understand why this specific value is used and prevent accidental breakage if the component's delay changes.component.confirm(); + // Wait for the 250ms textChangedEmitDelay in the component before the service call tick(300);
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||||||||||||||
atharvamp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS6, works as intended. Messages are no longer cut off after clicking enter.
anian03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested o TS6. While the issue no longer persists, I'm not sure if this is the best solution as it makes the web app seem slower. Do you know if there is a way to wait until the test update was complete? That way it would be ≤200ms, no always exactly 250ms
NadiaCurumi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS6
Messges are not cut in half when pressing send.
lana-ati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested during working session on TS3. Worked as expected.
kristi-balla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested on t3, works as expected
oTaczkowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3, everything works.
hanna20022005
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested on ts3 using tsest user 18, works as intended ^^
Summary
Checklist
General
Client
Motivation and Context
It fixes the issue #12029.
There was a timing issue occurred by having a delay for updating the message. Currently if user hits enter or clicks on send button quickly, the message was sent as 'cut in half'.
Description
I added a delay in creatingPost function to wait for update message events to emit successfully before sending it.
Steps for Testing
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.