Skip to content

SF-3959 Fix incorrect draft history displayed - #4191

Open
RaymondLuong3 wants to merge 4 commits into
masterfrom
fix/sf-3959-incorrect-history
Open

RaymondLuong3 wants to merge 4 commits into
masterfrom
fix/sf-3959-incorrect-history

Conversation

@RaymondLuong3

@RaymondLuong3 RaymondLuong3 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

The build history on the generation draft page can display incorrect information. The Project Notification Service listens for notifications and sends messages using signal R. This means that a project may be listening to the notification and that notification does not necessarily apply to the current project.

UPDATE:
This PR updates how consumers use the project notification service. Consumers now subscribe to observables that directly expose SignalR events. These subscriptions are automatically unsubscribed and the connection is closed when all subscriptions have been unsubscribed.


This change is Reviewable

@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Oct 15, 2026
@RaymondLuong3
RaymondLuong3 deployed to screenshot_diff October 15, 2026 22:22 — with GitHub Actions Active
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.80645% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.25%. Comparing base (cf45fae) to head (4e91f81).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ate/draft-generation/draft-notification.service.ts 0.00% 4 Missing ⚠️
...lientApp/src/app/core/notification-service-base.ts 89.28% 3 Missing ⚠️
...ntApp/src/app/core/project-notification.service.ts 0.00% 3 Missing ⚠️
.../app/sync/sync-progress/sync-progress.component.ts 71.42% 2 Missing ⚠️
...aft-import-wizard/draft-import-wizard.component.ts 60.00% 2 Missing ⚠️
...late/editor/editor-draft/editor-draft.component.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4191      +/-   ##
==========================================
+ Coverage   81.20%   81.25%   +0.05%     
==========================================
  Files         670      670              
  Lines       42920    42901      -19     
  Branches     7113     7112       -1     
==========================================
+ Hits        34853    34861       +8     
+ Misses       6917     6889      -28     
- Partials     1150     1151       +1     

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

@RaymondLuong3
RaymondLuong3 deployed to screenshot_diff October 15, 2026 23:34 — with GitHub Actions Active
@RaymondLuong3 RaymondLuong3 added the ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes label Oct 16, 2026

@Nateowami Nateowami left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Nateowami made 1 comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/app/core/notification-service-base.ts line 61 at r1 (raw file):

   * @param owner The same object that was passed to start().
   */
  stop(owner: object): Promise<void> {

Thanks for tracking down the cause and fixing this. My first thought in reading this start/stop code with owner objects is that this feels exactly like an observable that needs to be unsubscribed when it's no longer needed, and that manually implementing start/stop which the consumer has to remember to call means there's more work in every class that uses it.

I asked Claude to look into implementing this with observables, and it appears while it's not trivial, the consumers get a better experience. I haven't reviewed it, but I pushed it to refactor/sf-3959-notification-observables. To me it looks like the consumers would be a lot simpler and less error-prone.

What do you think?

RaymondLuong3 and others added 4 commits October 20, 2026 17:34
Replace start/stop/subscribeToProject/set...Handler/remove...Handler with
methods such as buildProgress$(projectId) that return an observable of that
project's notifications. The connection is started for the first subscriber,
restarted when the app comes back online, and stopped a short while after the
last subscriber unsubscribes. Notifications are filtered by project in the
service, so components no longer need to check the project ID themselves.

Components now subscribe with quietTakeUntilDestroyed instead of registering
handlers and stopping the connection in onDestroy. Operations that wait for
progress notifications (applying a draft) call whenListening(projectId)
before starting, so that no notifications are missed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@RaymondLuong3
RaymondLuong3 force-pushed the fix/sf-3959-incorrect-history branch from 4ea7acc to 4e91f81 Compare October 20, 2026 23:35
@RaymondLuong3 RaymondLuong3 removed the ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes label Oct 20, 2026

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on Nateowami).


src/SIL.XForge.Scripture/ClientApp/src/app/core/notification-service-base.ts line 61 at r1 (raw file):

Previously, Nateowami wrote…

Thanks for tracking down the cause and fixing this. My first thought in reading this start/stop code with owner objects is that this feels exactly like an observable that needs to be unsubscribed when it's no longer needed, and that manually implementing start/stop which the consumer has to remember to call means there's more work in every class that uses it.

I asked Claude to look into implementing this with observables, and it appears while it's not trivial, the consumers get a better experience. I haven't reviewed it, but I pushed it to refactor/sf-3959-notification-observables. To me it looks like the consumers would be a lot simpler and less error-prone.

What do you think?

Ok, I looked through the code and I can see the simplification for the consumer components. The project-notification service handles the closing of the connection when a component unsubscribes for the SignalR observable. I think this is an improvement on the current behaviour. I tested this for draft generation and draft applying and everything is behaving as I would expect.

@RaymondLuong3
RaymondLuong3 deployed to screenshot_diff October 20, 2026 23:41 — with GitHub Actions Active
@RaymondLuong3 RaymondLuong3 added the ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes label Oct 21, 2026
@Nateowami Nateowami removed the ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes label Oct 21, 2026

This branch was successfully deployed

1 active deployment
screenshot_diff — 4e91f818 Deployed Oct 20, 2026 by RaymondLuong3 via Compare Screenshots #1143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

will require testing PR should not be merged until testers confirm testing is complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants