Skip to content

Conversation

siltomato
Copy link
Collaborator

@siltomato siltomato commented Oct 3, 2025

This PR fixes all lint no-floating-promises warnings. All files used the void methodology to handle the floating promises (maintaining existing fire-and-forget behavior) except for the two instances described below.

  1. In the lynx-insight-panel.component.ts onLeafNodeClick() function, the promise chain is changed to async/await. This is more of a readability change than functional, as, in both cases, the nav completes before the call to update display state.

  2. In the realtime-doc.ts delete() function:

async delete(): Promise<void> {
  await this.adapter.delete();
  await this.updateOfflineData();
  await this.realtimeService.onLocalDocUpdate(this);
}

await this.adapter.delete() was chosen instead of void this.adapter.delete() with the idea to maintain data consistency in the case that the adapter.delete() call fails. Please review if this is the correct approach.


This change is Reviewable

@siltomato siltomato marked this pull request as ready for review October 3, 2025 23:10
Copy link

codecov bot commented Oct 3, 2025

Codecov Report

❌ Patch coverage is 77.11864% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.30%. Comparing base (eea0a15) to head (ef08444).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...Forge.Scripture/ClientApp/src/app/app.component.ts 57.14% 6 Missing ⚠️
...ientApp/src/xforge-common/user-projects.service.ts 0.00% 4 Missing ⚠️
...slate/draft-generation/draft-generation.service.ts 62.50% 3 Missing ⚠️
...ntApp/src/app/translate/editor/editor.component.ts 84.21% 3 Missing ⚠️
...p/src/app/machine-api/remote-translation-engine.ts 33.33% 2 Missing ⚠️
...e/ClientApp/src/app/shared/project-router.guard.ts 0.00% 2 Missing ⚠️
...anslate/biblical-terms/biblical-terms.component.ts 71.42% 0 Missing and 2 partials ⚠️
...eneration/draft-sources/draft-sources.component.ts 0.00% 2 Missing ⚠️
...nx-insights-panel/lynx-insights-panel.component.ts 50.00% 2 Missing ⚠️
...pp/src/xforge-common/exception-handling.service.ts 50.00% 2 Missing ⚠️
... and 25 more
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3492   +/-   ##
=======================================
  Coverage   82.30%   82.30%           
=======================================
  Files         613      613           
  Lines       36877    36879    +2     
  Branches     6046     6046           
=======================================
+ Hits        30352    30354    +2     
  Misses       5629     5629           
  Partials      896      896           

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

@Nateowami
Copy link
Collaborator

Thanks for tackling this @siltomato. One major concern I have is that it's going to create a slew of merge conflicts with #3199, which is already big, difficult, and affects async/await in a lot of places. Maybe we should merge the two instances you found that you think should be awaited, and then have the rest of them updated after #3199 to reduce conflicts (or at least to simplify the conflicts, since adding void is a lot simpler than the other PR's changes)?

@siltomato siltomato force-pushed the task/handle-floating-promises branch from c001568 to 45423d5 Compare October 8, 2025 20:55
Copy link
Collaborator Author

@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 tested merging this branch with #3199, and there were merge conflicts in just 5 files that were pretty straightforward to fix. But just for peace of mind, there's no harm in just waiting to merge this until that branch is merged.

Reviewable status: 0 of 70 files reviewed, all discussions resolved

@Nateowami
Copy link
Collaborator

@siltomato 5 straightforward fixes is very manageable. Let's proceed.

@Nateowami Nateowami requested a review from Copilot October 8, 2025 21:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses lint no-floating-promises warnings throughout the codebase by adding void to promises that are intentionally fire-and-forget. Two instances in the editor components use async/await for improved readability, and one instance in realtime-doc.ts uses await to maintain data consistency during deletion operations.

Key Changes

  • Added void prefix to floating promises to maintain existing fire-and-forget behavior while satisfying linting requirements
  • Converted onLeafNodeClick() in lynx insights panel to async/await pattern for better readability
  • Changed delete() method in realtime-doc.ts to await adapter deletion for data consistency

Reviewed Changes

Copilot reviewed 69 out of 69 changed files in this pull request and generated 2 comments.

File Description
src/xforge-common/models/realtime-doc.ts Changed delete() method to await adapter deletion for consistency
src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts Converted onLeafNodeClick() to async/await pattern
Multiple service and component files Added void prefix to floating promises across the application

Comment on lines 148 to 152
async delete(): Promise<void> {
this.adapter.delete();
await this.adapter.delete();
await this.updateOfflineData();
await this.realtimeService.onLocalDocUpdate(this);
}
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

The change to await this.adapter.delete() is correct for maintaining data consistency. However, consider adding error handling to ensure the subsequent operations don't execute if the adapter deletion fails, which could leave the document in an inconsistent state.

Copilot generated this review using guidance from repository custom instructions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the other pull request which deals with realtime doc cleanup, the order in which the disposal methods are called turns out to be very complicated and very important. I would suggest that we leave this line unchanged and fix it in the other pull request in the interest of simplicity. Basically, the situation is just too complicated for me to mentally untangle and be certain that this change is going to work with the changes in the other PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, changed await to void.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we leave this line unchanged. The change might be correct, and the existing behavior could even be wrong. I'd rather have a lint warning reminding us to deal with it. Marking it void is essentially saying "a developer looked at this and conclusively determined that we don't want to await this", which is definitely not correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Changed.

Copy link
Collaborator Author

@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.

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

Comment on lines 148 to 152
async delete(): Promise<void> {
this.adapter.delete();
await this.adapter.delete();
await this.updateOfflineData();
await this.realtimeService.onLocalDocUpdate(this);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, changed await to void.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami dismissed @copilot from a discussion.
Reviewable status: 0 of 70 files reviewed, all discussions resolved

Comment on lines 148 to 152
async delete(): Promise<void> {
this.adapter.delete();
await this.adapter.delete();
await this.updateOfflineData();
await this.realtimeService.onLocalDocUpdate(this);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we leave this line unchanged. The change might be correct, and the existing behavior could even be wrong. I'd rather have a lint warning reminding us to deal with it. Marking it void is essentially saying "a developer looked at this and conclusively determined that we don't want to await this", which is definitely not correct.

@siltomato siltomato force-pushed the task/handle-floating-promises branch from 0bb903a to ef08444 Compare October 9, 2025 00:19
Copy link
Collaborator Author

@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.

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

Comment on lines 148 to 152
async delete(): Promise<void> {
this.adapter.delete();
await this.adapter.delete();
await this.updateOfflineData();
await this.realtimeService.onLocalDocUpdate(this);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants