Fix BL-16809 Remove Page sometimes prevented by race condition - #8309
Merged
Conversation
Contributor
Author
|
[Claude Fable 5.1 from John Thomson's machine during preflight] Consulted Devin on 2026-09-04 20:30 UTC up to commit 4668a41. Review clean: no bugs, no investigate flags, no informational items. |
JohnThomson
commented
Sep 4, 2026
Contributor
Author
|
[Claude Fable 5.1 from John Thomson's machine during preflight] Consulted Devin on 2026-09-04 20:45 UTC up to commit 66e587e. It raised one bug ("Modal backdrops still miss page list"), assessed as not an issue and resolved with the reasoning on its thread. No investigate or informational flags. CI (pr-automation) green. |
JohnThomson
commented
Sep 4, 2026
JohnThomson
left a comment
JohnThomson
left a comment
Contributor
Author
There was a problem hiding this comment.
@JohnThomson reviewed 14 files and all commit messages.
Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion.
https://issues.bloomlibrary.org/youtrack/issue/BL-16809 Since BL-16421 the "Really Remove Page?" confirmation is a browser dialog. Like the other edit-tab dialogs it posts editView/setModalState from a React effect, which on the C# side disabled the page list (PageThumbnailList.Enabled) as well as locking the workspace tabs. When the user clicked Remove, the browser sent the removePage command and the "modal closed" notice as two concurrent HTTP requests. Whenever the command was handled first (about 100ms after the click, after HandleContextMenuItemClickedRequest's deliberate delay), the page list was still disabled, so IsContextMenuCommandEnabled returned false and ExecuteContextMenuCommand dropped the command silently. Captured over CDP against 6.5.3018: the unlock was processed roughly 350ms after the command. Disabling the page list from C# dates from when the page list was its own browser and a dialog's backdrop could not cover it. The whole edit tab is in one browser now, so a dialog shown from the workspace root already has a backdrop that blocks the page list (and the tabs). The C# gate could therefore only do harm, so this removes it: SetModalState now only locks the tabs, and PageListController's Enabled setter, PageThumbnailList.Enabled and its two checks (page clicks and context-menu commands) are gone. The same lag could also swallow a page click made right after a dialog closed. Two dialogs were still launched from code running in the page iframe with a direct import of ShowEditViewDialog, so they rendered inside the book pane and their backdrop covered only that pane: Choose Link Target (from the text hyperlink command and the navigation canvas element's "Set Destination") and Book Grid Setup. Both are now exposed on the workspace bundle, following the copyright dialog's pattern, and the page-side callers go through getWorkspaceBundleExports(), so their backdrop covers the whole workspace. The comments on the dialogs that post the flag are updated to say what it now does. Also logs a papercut about the dev launcher timing out while a direct dotnet watch run works. Tests: full C# suite through build/agent-dotnet.ps1 (3319 passed, 0 failed, 13 skipped) and the full vitest suite (784 passed, 5 skipped) pass. Verified live in a dev build: removing a page via the thumbnail context menu deletes it, refreshes the list, saves to disk and navigates to the next page. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
JohnThomson
force-pushed
the
BL-16809-remove-page-race
branch
from
September 4, 2026 21:19
66e587e to
dccd09b
Compare
JohnThomson
marked this pull request as ready for review
September 4, 2026 21:19
StephenMcConnel
approved these changes
Sep 9, 2026
StephenMcConnel
left a comment
StephenMcConnel
left a comment
Contributor
There was a problem hiding this comment.
@StephenMcConnel reviewed 14 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on JohnThomson).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In the 6.5 betas, removing a page often does nothing: the context-menu command is enabled, the "Really Remove Page?" dialog appears, but after clicking Remove the page stays. It happens on any page, in any book, and repeats after a restart (BL-16809).
Cause
Since BL-16421 the confirmation is a browser dialog. Like the other edit-tab dialogs it tells C# it is open and closed via
editView/setModalState, and C# used that flag to disable the page list as well as to lock the workspace tabs. When the user clicks Remove, the browser sends the remove command and the "dialog closed" notice as two concurrent HTTP requests. Whenever the command is handled first (it runs on a deliberate short delay), the page list is still disabled and the command is refused silently. Captured live: the unlock was processed about 350 ms after the command had already been dropped.What the PR does
SetModalStatenow only locks the tabs. Disabling the page list dates from when it lived in its own browser and a dialog's backdrop could not cover it; a dialog shown from the workspace root now has a backdrop that already blocks the page list.PageListController.Enabled,PageThumbnailList.Enabled, and its checks on page clicks and context-menu commands. The same lag could also swallow a page click made right after a dialog closed.dotnet watch runworks.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16809
Devin review
This change is