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 message completion trigger to work anywhere in the message #3696

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

vickcoo
Copy link

@vickcoo vickcoo commented Jan 22, 2025

This PR resolves issue #3522

Pull Request Checklist

UI changes have been tested with:

  • iPhone and iPad simulators in portrait and landscape orientations.
  • Dark mode enabled and disabled.
  • Various sizes of dynamic type.
  • Voiceover enabled.

@vickcoo vickcoo requested a review from a team as a code owner January 22, 2025 14:46
@vickcoo vickcoo requested review from pixlwave and removed request for a team January 22, 2025 14:46
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2025

CLA assistant check
All committers have signed the CLA.

@stefanceriu
Copy link
Member

Can you try testing out this new behavior within the CompletionSuggestionServiceTests?

@vickcoo
Copy link
Author

vickcoo commented Jan 22, 2025

I've run the testing on CompletionSuggestionServiceTests, and they all pass

@stefanceriu
Copy link
Member

I've run the testing on CompletionSuggestionServiceTests, and they all pass

As in write a brand new one just for this special case and assert that the suggestions show up

@vickcoo
Copy link
Author

vickcoo commented Jan 23, 2025

Oh! Do you mean I need to write a new unit test for this?
I have no idea what happened cause the flow fail

*I apologize If my English is unclear.

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.68%. Comparing base (a70189d) to head (4346d9e).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3696      +/-   ##
===========================================
- Coverage    78.71%   78.68%   -0.03%     
===========================================
  Files          792      792              
  Lines        67839    67863      +24     
===========================================
- Hits         53399    53398       -1     
- Misses       14440    14465      +25     
Flag Coverage Δ
unittests 70.27% <100.00%> (+0.04%) ⬆️

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.

@vickcoo
Copy link
Author

vickcoo commented Jan 25, 2025

Why did the CI/CD fail, And how can I solve the problems?

@stefanceriu
Copy link
Member

Why did the CI/CD fail

Only the enterprise tests failed which is to be expected as you don't have access to that code, it's private.
As far as I can tell your part is good but I'll review it properly next week.

@vickcoo
Copy link
Author

vickcoo commented Jan 26, 2025

As far as I can tell your part is good but I'll review it properly next week.

I got it! thank you for your reply

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Hey @vickcoo, thanks for opening your first contribution 🎉

I've just tested it out, but I think we probably need to refine it a bit more. If you type @ into the composer but don't want to mention someone it now always shows a suggestion and if you subsequently try to @mention someone it uses that first @ as the trigger so your filtering doesn't work. If we're going to allow mentioning from anywhere in the message, we really need to use the caret position as the trigger rather than iterating through all the words. See an example of the unexpected behaviour here:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-27.at.10.12.40.mp4

@vickcoo
Copy link
Author

vickcoo commented Jan 27, 2025

Hi @pixlwave! thank you for your feedback, here are a few ideas.

Solution 1
The @ symbol triggers suggestions, but the cursor must be positioned next to it, even if the message contains multiple "@" symbols.
For example, suggestions appear when the cursor is positioned at 1️⃣ or 2️⃣; otherwise, no suggestions are shown.

    1️⃣     2️⃣
    ↓      ↓
Hi @, I'm @

Solution 2
We can trigger suggestions anywhere when typing the @, and other @ will be a plain string.

@pixlwave
Copy link
Member

Solution 1 sounds super to me if it's possible!

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.

4 participants