Convert the thumbnail context menu to display in React (BL-15895)

[StephenMcConnel]

---

---

This change is 

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

[hatton]

1. JS requests menu items from C# → receives [{id, label, enabled}, ...]
2. User clicks an item → JS sends the commandId back to C# for execution

@StephenMcConnel would it be more natural for the UI (typescript) to know the menu items it wants?

[StephenMcConnel]

I thought about it but came up with two reasons for keeping the way the AI structured things:

1. There would need to be an API call for each menu item to get its localized label.
2. There would need to be an API call for each menu item to get its enabled state, although maybe that could be computed on the client side. I haven't really looked at that.
   Making a single API call to get all of the menu items with those values already computed seemed like a reasonable choice with
   simpler setup.

@StephenMcConnel made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

[hatton]

Yeah, let's "go native" for all of these menus, not rely on the server to act like part of the UI. We have UI all over in typescript land that manages to localize itself one way or the other :-)

@hatton made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

[StephenMcConnel]

OK.

@StephenMcConnel made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

[StephenMcConnel]

Done.

@StephenMcConnel made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

[andrew-polk]

@andrew-polk reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx line 496 at r2 (raw file):

                >
                    {contextMenuItems.map((item) => (
                        <MenuItem

Should we be using LocalizableMenuItem here?
If not, let's add a comment.

[StephenMcConnel]

@StephenMcConnel made 1 comment.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on andrew-polk).

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx line 496 at r2 (raw file):

Previously, andrew-polk wrote…

Should we be using LocalizableMenuItem here?
If not, let's add a comment.

Done. I had to enhance LocalizableMenuItem to accept a pair of css attributes to make the menu look reasonable.

[StephenMcConnel]

@StephenMcConnel made 1 comment.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on andrew-polk).

[andrew-polk]

@andrew-polk reviewed all commit messages and resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on StephenMcConnel).

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 10 additional findings in Devin Review.

[devin-ai-integration]

🚩 Task.Run + BeginInvoke pattern: command execution is fire-and-forget with error swallowing

The HandleContextMenuItemClickedRequest at PageListApi.cs:148-174 uses Task.Run with a 100ms delay, then BeginInvoke to execute commands on the UI thread. The request.PostSucceeded() is returned immediately at line 178. If the command fails (e.g., ConfirmRemovePageDialog.Confirm() throws, or Model.DeletePage fails), the error is caught and logged at lines 163-169, but the user gets no feedback — the JS side already received a success response. The comment on line 165 acknowledges this concern ("Should we notify the user as well?"). For commands like removePage and chooseDifferentLayout that show modal dialogs, the 100ms delay is needed to let the React menu close first; without it, the modal could fight with the menu for focus.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[andrew-polk]

@andrew-polk partially reviewed 3 files and made 1 comment.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on StephenMcConnel).

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx line 510 at r4 (raw file):

                                min-height: 24px !important;
                                padding: 3px 8px !important;
                            `}

Sorry I wasn't clear about this earlier. Here's another try with specifics...

You can get rid of the itemCss prop and just set css. The className={props.className} in LocalizableMenuItem means the css prop gets passed to the underlying MenuItem.

(and you can get rid of setting the className here)

[StephenMcConnel]

@StephenMcConnel made 2 comments and resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on andrew-polk).

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx line 510 at r4 (raw file):

Previously, andrew-polk wrote…

Sorry I wasn't clear about this earlier. Here's another try with specifics...

You can get rid of the itemCss prop and just set css. The className={props.className} in LocalizableMenuItem means the css prop gets passed to the underlying MenuItem.

(and you can get rid of setting the className here)

I think I see what is happening, but it's way too magical and obscure. You have to 1) NOT pass in a className value as well as pass in a css value for this magic to work.

[StephenMcConnel]

@StephenMcConnel resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on andrew-polk).

[andrew-polk]

@andrew-polk reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on StephenMcConnel).