SF-2442 Add AI draft tabs when draft generation completes

[nigel-wells]

• Removed "Preview draft" button and highlighting from the editor
• Show/hide the "Auto Draft" tab automatically when a draft is available/not available
• Auto Draft tab is no longer closable

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 76.60%. Comparing base (29b4ff6) to head (f42536f).
Report is 1 commits behind head on master.

❗ Current head f42536f differs from pull request most recent head 28ba473. Consider uploading reports for the commit 28ba473 to get more accurate results

Files	Patch %	Lines
...shared/sf-tab-group/tab-state/tab-state.service.ts	50.00%	2 Missing and 1 partial ⚠️
...ntApp/src/app/translate/editor/editor.component.ts	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
- Coverage   76.83%   76.60%   -0.23%     
==========================================
  Files         508      506       -2     
  Lines       28942    28734     -208     
  Branches     4708     4679      -29     
==========================================
- Hits        22237    22012     -225     
- Misses       5966     5982      +16     
- Partials      739      740       +1     

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

[Nateowami]

This works great in my testing. Good work.

(This is not a code review)

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @pmachapman)

[pmachapman]

The code looks good, I just have a few questions/issues with the implementation.

First, I notice an issue when I

1. Navigate to a book that has a draft (the tab opens - expected)
2. Then navigate to a book without a draft
   The tab stays open, but says a message along the line of "Philemon 1 has no draft."

Shouldn't the tab auto close in this case?

Second, previously the AI tab was openable on the left hand panel (a requested feature), but this feature is now gone. I assume that you are waiting for tab moving to be implemented to restore that functionality?

Third, the workflow to apply the draft from the editor (via the preview draft button) is disrupted. Do we need an apply draft button here, now that the Preview draft button is gone?

I guess I raise these last two points because it appears to regress existing user-requested or workflow behavior which is scheduled for release to production (i.e. currently in QA).

Perhaps it is just a matter of timing for this PR, after a few more pieces like movable tabs and applying drafts from the editor are in place? I suppose I am just mindful that we don't want to deploy this PR without the other pieces in place.

Fourth, I (personally) don't like how the close button is removed. It feels like we have removed freedom from the user to customize their workspace, without a real necessity? (i.e. did UAT testing show that users were closing the tab then wondering why it was gone and getting confused). I can't see any spec to remove the close button in the JIRA ticket.

Finally, #2372 updated the model to specify what chapters have drafts. I think your PR might benefit from this feature (i.e. speed to check if there is a draft), when it is merged (it is awaiting testing).

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nigel-wells)

[Nateowami]

Regarding now allowing the draft to be closed, I think we did talk about not allowing it to be closed, but perhaps didn't add that to the issue. It probably makes sense to allow it to be closed, but it should re-open if a user clicks to preview the draft from the generate draft page. Also, tabs are currently persisted, so at present if the user closes it, leaves, and comes back, it won't remember that. I don't have a super strong opinion either way.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nigel-wells)

[Nateowami]

Nigel, Joseph and I discussed this a little more and came to the conclusion that we should leave the close button off because:

• Persistence of tabs isn't available yet, so closing doesn't have much of an effect. Switching to another chapter would cause it to open again.
• Even once persistence is available, in most cases we'll persist that a tab has been opened, not that it has been closed. The draft has the distinction of being open by default. This makes designing this more complicated.

We may revisit this in the future if we decide there are good reasons to be able to close it.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nigel-wells)

[nigel-wells]

Thanks Peter. The tab should auto close if you're on a book/chapter with no draft. If, however, you manually add the tab then it will be open. I'll do a little more testing around this in case it is something else.

I hadn't intended on removing the ability to open the tab via the left hand panel - I'll look into that as well.

I'm happy for this to sit until we get SF-2443 merged as well so the apply draft button is available - that probably isn't too far away now.

SF-2483 will be great to speed up the delay. What I've done may also need to be tweaked again depending on tab persistence which is also being worked on at the moment.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nigel-wells)

[nigel-wells]

@pmachapman I've looked into the draft tab not being available on the left hand panel. This is existing functionality where, if the auto draft tab has been added to one panel then it can't also be added to the other at the same time. As I've auto opened it on the target it means there won't be an opportunity to add it to the source/left.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nigel-wells)

[Nateowami]

@nigel-wells We have an issue to show tabs from one side in the menu for the other, so they can be moved that way. However, it's somewhat de-prioritized since @siltomato has implemented dragging of tabs from one side to the other. That change is going to get merged shortly, so I don't think we need to worry about the limitation in this PR.

[Nateowami]

@nigel-wells Wouldn't it make more sense to add the draft tab to the left side? If users want to use it as a comparison, that's the ideal place for it to be. And if they want to import it, having it on the left will allow them to visually see how the text imports to the project and make it clearer exactly what is happening.

[nigel-wells]

Makes sense - done.

Reviewable status: 3 of 7 files reviewed, all discussions resolved (waiting on @pmachapman)

[pmachapman]

Code looks good - just a couple of bugs with the behavior of the change.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nigel-wells)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 2313 at r2 (raw file):

      .getGeneratedDraft(this.activatedProject.projectId!, this.bookNum!, this.chapter!)
      .subscribe((draft: DraftSegmentMap) => {
        this.hasDraft = this.draftViewerService.hasDraftOps(draft, targetOps);

hasDraftOps only returns true if there are blank segments in the target chapter.

We have had feedback that people would like to view the AI draft whenever it is generated (i.e. as a comparison tool). Are we able to display this tab if there is a draft at all, not just if the target can support a draft being applied?

Also, due to how the subscribe is structured, the tab can pop in and out if the user navigates between chapters (note: you will still need to fix this bug if you show the AI tab whenever a draft is available, as some chapters or books may not have a draft. See my recording of the bug: https://youtu.be/3cAqruPBsXY )

Code quote:

this.hasDraft = this.draftViewerService.hasDraftOps(draft, targetOps);

[nigel-wells]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 2313 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

hasDraftOps only returns true if there are blank segments in the target chapter.

We have had feedback that people would like to view the AI draft whenever it is generated (i.e. as a comparison tool). Are we have to display this tab if there is a draft at all, not just if the target can support a draft being applied?

Also, due to how the subscribe is structured, the tab can pop in and out if the user navigates between chapters (note: you will still need to fix this bug if you show the AI tab whenever a draft is available, as some chapters or books may not have a draft. See my recording of the bug: https://youtu.be/3cAqruPBsXY

As I'm already waiting for SF-2443 to be completed it means SF-2483 will also likely be ready. This will allow me to much more easily remove the glitchyness you're seeing.

[nigel-wells]

This is ready for review and testing again, thanks @pmachapman

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.55%. Comparing base (2b74452) to head (8c7e7aa).

Files	Patch %	Lines
...ntApp/src/app/translate/editor/editor.component.ts	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
+ Coverage   77.51%   77.55%   +0.03%     
==========================================
  Files         510      510              
  Lines       29233    29248      +15     
  Branches     4765     4774       +9     
==========================================
+ Hits        22660    22683      +23     
+ Misses       5818     5809       -9     
- Partials      755      756       +1     

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

[Nateowami]

Reviewed 2 of 7 files at r3.
Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @nigel-wells and @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1309 at r3 (raw file):

  }

  /** Insert or remove note thread embeds into the quill editor. */

This comment is supposed to go with the toggleNoteThreadVerses() method.

[Nateowami]

Excellent work! Very excited to see this coming together.

Reviewed 4 of 7 files at r3.
Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @nigel-wells and @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-state/tab-state.service.ts line 104 at r3 (raw file):

  }

  getTabIndex(groupId: TGroupId, type: string): number | undefined {

I think there could be more than one tab of a given group so this should be something like getFirstTabOfTypeIndex()

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-state/tab-state.service.ts line 109 at r3 (raw file):

  hasTab(groupId: TGroupId, type: string): boolean {
    if (!this.groups.has(groupId)) {

Couldn't this whole method be simplified to:

    const group = this.groups.get(groupId);
    return group != null && group.tabs.some(t => t.type === type);

Was there a reason you had .tabs? ? The property isn't supposed to be optional.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1310 at r3 (raw file):

  /** Insert or remove note thread embeds into the quill editor. */
  private toggleAutoDraftTab(): void {

Personally I think "toggle" means to swap to the state other than the current one. Meaning calling this method twice would have the same effect as not calling it. That's not what it means in this case; it's more like "update presence of auto draft tab".

(I know you were copying the other use of "toggle" in this file; I think it's incorrect as well)

[Nateowami]

I have moved the Jira issue to "ready to test", even though the PR hasn't passed code review, since none of my suggestions are significant, and I could approve as-is.

[nigel-wells]

Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @Nateowami and @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-state/tab-state.service.ts line 109 at r3 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Couldn't this whole method be simplified to:

    const group = this.groups.get(groupId);
    return group != null && group.tabs.some(t => t.type === type);

Was there a reason you had .tabs? ? The property isn't supposed to be optional.

Yes, that works as well. Some of the code may have moved on from when I first looked at this. I've updated it to your suggestions.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1309 at r3 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

This comment is supposed to go with the toggleNoteThreadVerses() method.

Whoops! Have moved back.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1310 at r3 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Personally I think "toggle" means to swap to the state other than the current one. Meaning calling this method twice would have the same effect as not calling it. That's not what it means in this case; it's more like "update presence of auto draft tab".

(I know you were copying the other use of "toggle" in this file; I think it's incorrect as well)

How about updateAutoDraftTabVisibility ?

[Nateowami]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1310 at r3 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

How about updateAutoDraftTabVisibility ?

I like that better. Thanks.

[Nateowami]

Dismissed @pmachapman from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)

Dismiss old review