Skip to content

SF-3418 Lynx fix text is being inserted at wrong index #3238

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

Merged
merged 2 commits into from
Jun 18, 2025

Conversation

ddaspit
Copy link
Collaborator

@ddaspit ddaspit commented Jun 4, 2025

This change is Reviewable

@ddaspit ddaspit requested a review from siltomato June 4, 2025 19:39
@ddaspit ddaspit force-pushed the fix/lynx-handle-embeds branch from b1a6d24 to 975ab17 Compare June 4, 2025 19:50
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

Attention: Patch coverage is 13.63636% with 19 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (d58f79b) to head (0475971).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...r-objects/lynx-insight-editor-objects.component.ts 0.00% 10 Missing ⚠️
...ion-prompt/lynx-insight-action-prompt.component.ts 0.00% 5 Missing ⚠️
...hts/quill-services/quill-insight-render.service.ts 0.00% 2 Missing ⚠️
...e/ClientApp/src/app/shared/text/text-view-model.ts 50.00% 1 Missing ⚠️
...-insight-overlay/lynx-insight-overlay.component.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3238      +/-   ##
==========================================
+ Coverage   81.99%   82.09%   +0.09%     
==========================================
  Files         605      605              
  Lines       35112    35121       +9     
  Branches     5716     5695      -21     
==========================================
+ Hits        28790    28832      +42     
+ Misses       5490     5455      -35     
- Partials      832      834       +2     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Nateowami
Copy link
Collaborator

@ddaspit Is there an issue this is supposed to solve?

@ddaspit
Copy link
Collaborator Author

ddaspit commented Jun 5, 2025

This solves an issue in Lynx.

@Nateowami
Copy link
Collaborator

Nateowami commented Jun 5, 2025

@ddaspit could you try to come up with a title that's more descriptive (as defined by CONTRIBUTING.md)?

Something like:

SF-1234 Fix Lynx inserting instead of replacing quote

(Not sure whether there's a Jira issue for this. If not, there should be one so it can go through the testing process before merging)

@Nateowami Nateowami added the will require testing PR should not be merged until testers confirm testing is complete label Jun 5, 2025
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

The code changes look good, but please create a jira issue with acceptance tests so that I can confirm if this change solves the issue it is supposed to solve.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

Copy link
Collaborator

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Nice! This looks good to me and solves the problem of note embeds causing lynx fix offsets to be wrong.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@ddaspit
Copy link
Collaborator Author

ddaspit commented Jun 5, 2025

@siltomato Do you know if there is a Jira issue for this?

Copy link
Collaborator

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

I created one: https://jira.sil.org/browse/SF-3418

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@Nateowami Nateowami changed the title Convert Lynx update deltas to editor view model SF-3418 Convert Lynx update deltas to editor view model Jun 6, 2025
@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jun 6, 2025
@ddaspit ddaspit force-pushed the fix/lynx-handle-embeds branch from 975ab17 to dd51312 Compare June 9, 2025 16:40
Copy link
Collaborator

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

Copy link
Collaborator Author

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

I added a unit test to cover the issue and updated the title.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@ddaspit ddaspit force-pushed the fix/lynx-handle-embeds branch 2 times, most recently from 34b0deb to 3546926 Compare June 9, 2025 16:52
@ddaspit ddaspit changed the title SF-3418 Convert Lynx update deltas to editor view model SF-3418 Lynx fix text is being inserted at wrong index Jun 9, 2025
@ddaspit ddaspit force-pushed the fix/lynx-handle-embeds branch from 3546926 to f8a5d63 Compare June 9, 2025 22:34
Copy link
Collaborator

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@ddaspit ddaspit force-pushed the fix/lynx-handle-embeds branch from f8a5d63 to d383c85 Compare June 10, 2025 19:59
@siltomato siltomato force-pushed the fix/lynx-handle-embeds branch from d383c85 to 0475971 Compare June 16, 2025 21:17
Copy link
Collaborator

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

I added a fix for the incorrect action prompt location (also due to note embeds).

Reviewable status: 6 of 14 files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jun 18, 2025
@Nateowami Nateowami force-pushed the fix/lynx-handle-embeds branch from 0475971 to db0cacc Compare June 18, 2025 14:32
@Nateowami Nateowami enabled auto-merge (squash) June 18, 2025 14:33
@Nateowami Nateowami merged commit c18dbdf into master Jun 18, 2025
15 of 16 checks passed
@Nateowami Nateowami deleted the fix/lynx-handle-embeds branch June 18, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants