Add a context menu to the toolbar button#39
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a toolbar-button context menu and supporting infra: manifest permission, new menu and command handler classes, background routing updates, Thunderbird-specific ButtonMenu implementation, API/type additions, startup wiring, and small utilities and tests to wire and persist menu items. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TB as Thunderbird UI
participant Router as BackgroundEventRouter
participant MenuHandler as MenuHandler
participant ButtonMenu as ButtonMenu
participant CmdHandler as CommandHandler
participant Notifier as ComposeActionNotifier
participant ComposeTab
User->>TB: Right-click toolbar button
TB->>Router: menus.onShown event
Router->>MenuHandler: handleMenuShown(info)
alt not initialized
MenuHandler->>ButtonMenu: isInitialized()
ButtonMenu-->>MenuHandler: false
MenuHandler->>ButtonMenu: initItems(menuItems, info)
ButtonMenu->>TB: messenger.menus.create/create submenu items
TB-->>ButtonMenu: ack
ButtonMenu->>ButtonMenu: persist initialized flag
else initialized
MenuHandler->>ButtonMenu: isInitialized()
ButtonMenu-->>MenuHandler: true
end
User->>TB: Click menu item (e.g., stop_ghostbird)
TB->>Router: menus.onClicked event
Router->>MenuHandler: handleMenuClick(menuItemId, tab)
MenuHandler->>CmdHandler: runCommand(menuItemId, composeTab)
CmdHandler->>Notifier: dispatch corresponding action
Notifier->>ComposeTab: perform start/stop/toggle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
- Coverage 48.43% 47.12% -1.31%
==========================================
Files 55 59 +4
Lines 2141 2264 +123
Branches 286 292 +6
==========================================
+ Hits 1037 1067 +30
- Misses 1097 1190 +93
Partials 7 7 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
59-59: Same verification needed as in check-pr.yml.This change mirrors the one in
.github/workflows/check-pr.yml. Please ensure the test output structure is consistent with this specific file path.
🧹 Nitpick comments (8)
src/ghosttext-adaptor/email_editor.ts (1)
29-32: Consider filtering out false values for clarity.The current implementation works correctly because
Promise.allhandles non-Promise values, but the patternisNewStatus && promisecan be confusing since it evaluates to eitherfalseor aPromise.For better readability, consider filtering out the false value:
- await Promise.all([ - isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)), - this.composeWindow.setIcon(icon), - ]) + const tasks = [this.composeWindow.setIcon(icon)] + if (isNewStatus) { + tasks.push(this.notificationTray.showNotification(icon, messageForStatus(status))) + } + await Promise.all(tasks)Alternatively, use
filter:- await Promise.all([ - isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)), - this.composeWindow.setIcon(icon), - ]) + await Promise.all( + [ + isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)), + this.composeWindow.setIcon(icon), + ].filter(Boolean) + )src/thunderbird/messenger/i18n.ts (1)
3-20: Clarify documentation to match the simplified signature.The documentation mentions:
- Returning
undefinedfor format errors, but the signature only allowsstring- A
substitutionsparameter that doesn't exist in the signatureSince this interface appears to be a simplified version of
browser.i18n.getMessage(), update the documentation to reflect that this implementation:
- Only accepts
messageName(no substitutions support)- Only returns
string(clarify what happens on errors)Apply this diff to align documentation with the actual signature:
/** * Gets the localized string for the specified message. If the message is - * missing, this method returns an empty string (''). If the format of - * the `getMessage()` call is wrong — for example, _messageName_ is not a - * string or the _substitutions_ array has more than 9 elements — this - * method returns `undefined`. + * missing, this method returns an empty string (''). * * @param messageName The name of the message, as specified in the * `messages.json` file. * - * @param [substitutions] Substitution strings, if the message requires - * any. - * * @returns Message localized for current locale. */ getMessage(messageName: MessageId): stringsrc/app-background/menu_handler.ts (1)
27-32: Add validation before castingmenuItemIdtoCommandId.The type assertion on line 30 assumes
menuItemIdis always a validCommandId, but there's no runtime validation. If an invalid ID is passed (e.g., from a stale menu or external source),CommandHandler.runCommandwill fail at runtime.Consider adding validation:
/** Handles click on a menu item */ handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> { // We use command ID as menu item ID, so we can directly pass it to the command handler + const validCommands: CommandId[] = ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"] + if (!validCommands.includes(menuItemId as CommandId)) { + return Promise.reject(new Error(`Invalid menu item ID: ${menuItemId}`)) + } return this.commandHandler.runCommand(menuItemId as CommandId, composeTab) }Alternatively, create a type guard function:
function isCommandId(id: string): id is CommandId { return ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"].includes(id) }src/root/background.ts (1)
53-59: Remove unsafe assertion and add a type guard
Replace the cast (info as MenuShownInfo) with a runtime check to ensureinfo.menuIdsis an array before passing it tohandleMenuShown. For example:if (!Array.isArray(info.menuIds)) { throw new TypeError('Unexpected menu info shape'); } return router.handleMenuShown(info, tab);src/thunderbird/background_util/button_menu.ts (3)
39-41: Avoid globalmenus.removeAll()if you have other context menus.This removes all of this extension’s menu items across contexts. Prefer tracking created IDs and removing only those, or only calling this once at startup.
31-36: Remove noisy debug logging (or gate it).
console.debug(got)on every load is chatty. Consider removing or gating behind a verbose flag.Suggested change:
- console.debug(got)
17-24: Optionally validate current menu vs. expected items before early-return.You accept
currentShownbut ignore it. To self-heal if menus get cleared, compareinfo.menuIdswith expected and re-init when mismatched.src/app-background/background_event_router.ts (1)
16-23: Validatecommandbefore casting toCommandId.Unsafe cast can pass an unknown string to
runCommand, which then returnsundefined(not a Promise). Fail fast on unknown commands.Apply this diff:
handleCommand(command: string, tab: ITab): Promise<void> { let composeTab = this.composeTabDetector.tryWrap(tab) if (!composeTab) { return Promise.reject(Error("Event dropped")) } - return this.commandHandler.runCommand(command as CommandId, composeTab) + const allowed = new Set<CommandId>(["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"]) + if (!allowed.has(command as CommandId)) { + return Promise.reject(Error(`Unknown command: ${command}`)) + } + return this.commandHandler.runCommand(command as CommandId, composeTab) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/build.yml(1 hunks).github/workflows/check-pr.yml(1 hunks)ext/options.css(1 hunks)ext/options.nolint.css(1 hunks)manifest_template.json(1 hunks)src/app-background/api.ts(2 hunks)src/app-background/background_event_router.ts(3 hunks)src/app-background/command_handler.ts(1 hunks)src/app-background/menu_handler.ts(1 hunks)src/ghosttext-adaptor/email_editor.ts(1 hunks)src/root/background.ts(3 hunks)src/root/startup/startup_background.ts(2 hunks)src/test/sanity.test.ts(1 hunks)src/test/startup.test.ts(1 hunks)src/thunderbird/background_util/button_menu.ts(1 hunks)src/thunderbird/background_util/notification_tray.ts(3 hunks)src/thunderbird/messenger/i18n.ts(1 hunks)src/util/time.ts(1 hunks)src/util/types.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/thunderbird/messenger/i18n.ts (1)
src/util/types.ts (1)
MessageId(22-22)
src/app-background/command_handler.ts (3)
src/app-background/compose_action_notifier.ts (1)
ComposeActionNotifier(4-57)src/app-background/api.ts (1)
CommandId(7-7)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/app-background/menu_handler.ts (3)
src/app-background/api.ts (4)
IButtonMenu(46-56)MenuItem(65-72)MenuShownInfo(59-62)CommandId(7-7)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/ghosttext-adaptor/email_editor.ts (1)
src/ghosttext-runner/api.ts (1)
ClientStatus(109-109)
src/app-background/background_event_router.ts (3)
src/app-background/api.ts (4)
IComposeWindowDetector(34-43)CommandId(7-7)MenuShownInfo(59-62)ITab(26-31)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/app-background/menu_handler.ts (1)
MenuHandler(5-32)
src/thunderbird/background_util/button_menu.ts (1)
src/app-background/api.ts (3)
IButtonMenu(46-56)MenuItem(65-72)MenuShownInfo(59-62)
src/root/background.ts (3)
src/app-background/api.ts (2)
MenuItem(65-72)MenuShownInfo(59-62)src/root/startup/startup_background.ts (2)
AlarmHeart(14-14)prepareBackgroundRouter(33-40)src/thunderbird/background_util/alarm_heart.ts (1)
AlarmHeart(3-21)
src/app-background/api.ts (1)
src/util/types.ts (1)
MessageId(22-22)
src/root/startup/startup_background.ts (3)
src/app-background/api.ts (2)
MenuItem(65-72)IComposeWindowDetector(34-43)src/app-background/menu_handler.ts (1)
MenuHandler(5-32)src/app-background/command_handler.ts (1)
CommandHandler(7-23)
src/thunderbird/background_util/notification_tray.ts (1)
src/ghosttext-adaptor/api.ts (1)
INotificationTray(5-8)
🔇 Additional comments (22)
tsconfig.json (1)
43-45: Confirm TypeDoc version supportssluggerConfiguration.
sluggerConfigurationis only understood by recent TypeDoc releases; older versions error out on unknown options. Please double-check that the project is already on a TypeDoc build new enough to accept this setting (and bump it if not), otherwise docs generation will fail.src/util/time.ts (1)
1-13: Clean implementation with appropriate logging.The timer utility is well-structured and uses
performance.now()for accurate measurements. Theconsole.debugwith object formatting provides clear, structured output for development.ext/options.nolint.css (1)
5-5: LGTM!The addition of
--align-startcomplements the existing--align-endvariable and properly supports bidirectional text layouts for internationalization.src/thunderbird/background_util/notification_tray.ts (2)
9-11: LGTM!The notification ID rotation mechanism is well-designed. It limits concurrent notifications to 5 slots, preventing notification spam while maintaining the ability to show multiple notifications if needed.
34-37: LGTM!The ID rotation logic is correct:
(this.lastId + 1) % this.slotCountwill cycle through 0→1→2→3→4→0→..., ensuring notification IDs stay within the defined slot range.src/ghosttext-adaptor/email_editor.ts (1)
25-25: LGTM!Good optimization to only trigger notifications when the status actually changes, preventing redundant notifications.
src/util/types.ts (1)
23-23: LGTM!The clarifying comment explains the design decision to rely on unit tests rather than TypeScript enumerations for
MessageIdvalidation.manifest_template.json (1)
26-26: LGTM!The addition of the
"menus"permission is essential for implementing the toolbar context menu feature described in the PR objectives, which aims to provide a mouse-only way to disconnect by right-clicking the toolbar button.src/test/startup.test.ts (1)
37-37: LGTM!The addition of the
menuItemsconstant aligns with the expandedBackgroundConstantsAPI surface and follows the established pattern for test constants.src/root/startup/startup_background.ts (3)
1-1: LGTM!The import updates correctly reflect the architectural shift to menu-based command handling via
MenuHandlerandCommandHandler.
20-20: LGTM!Adding
menuItemstoBackgroundConstantsproperly extends the configuration surface to support toolbar context menus.
25-26: LGTM!The catalog updates correctly expose
MenuHandlerandCommandHandleras the new public API surface while maintaining the underlyingComposeActionNotifieras an implementation detail.src/test/sanity.test.ts (1)
23-23: LGTM!The test correctly includes the new
menuItemsparameter with an appropriate empty array value for basic router resolution testing.src/app-background/command_handler.ts (1)
7-23: LGTM!The
CommandHandlerimplementation correctly:
- Delegates all commands to the appropriate
ComposeActionNotifiermethods- Omits the default case to leverage TypeScript's exhaustiveness checking
- Returns promises consistently for all command types
src/root/background.ts (2)
16-23: LGTM!The
menuItemsconfiguration correctly includes only thestop_ghostbirdcommand, which aligns with the PR objective of providing a mouse-only way to disconnect.
61-71: LGTM!The menu click handler includes proper validation:
- Checks for missing
tabormenuItemId- Returns explicit error for dropped events
- Handles unknown commands with rejection
src/app-background/api.ts (2)
45-56: LGTM!The
IButtonMenuinterface provides a clean abstraction for menu initialization with appropriate method signatures and documentation.
58-72: LGTM!The
MenuShownInfoandMenuItemtypes are well-defined and properly documented, providing clear contracts for menu-related functionality.src/app-background/menu_handler.ts (1)
14-25: LGTM!The menu initialization logic correctly:
- Checks if the menu is already initialized before calling
initItems- Logs appropriately for debugging
- Returns the promise from
initItemswhen initialization is neededsrc/app-background/background_event_router.ts (3)
38-40: Menu shown routing looks good.Delegation to
MenuHandler.handleMenuShownis clean and aligns with the new menu flow.
43-50: Menu click routing LGTM.Properly wraps tab -> compose window and forwards id to
MenuHandler.Please ensure menu item IDs match
CommandIdvalues soCommandHandlerreceives valid commands.
1-4: Verify barrel exports for types.
import type { MenuHandler, MenuShownInfo } from "."assumes these are re-exported from the index. Confirm the barrel re-exports to avoid import errors after refactors.
snipsnipsnip
force-pushed
the
add-context-menu
branch
from
cd4f5cc to
0456484
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/thunderbird/background_util/button_menu.ts (1)
17-24: Concurrent initialization concern.This was flagged in a previous review: multiple rapid
onShownevents could potentially cause race conditions leading to duplicatemenus.createcalls and "duplicate id" errors. While the??=operator provides some protection, the suggested explicit serialization pattern would be more robust.Additionally, lines 18-20 perform a check but don't return early, making them redundant with the
??=operator on line 21. Consider either:
- Removing the check entirely (let
??=handle it), or- Adding an early return after the log for efficiency:
initItems(menuItems: ReadonlyArray<MenuItem>, _currentShown: MenuShownInfo | undefined): Promise<void> { if (this.initWork) { console.debug("ButtonMenu is already initialized; Skip creating menu items.") + return this.initWork } this.initWork ??= this.createMenuItems(menuItems) return this.initWork }
🧹 Nitpick comments (5)
src/app-background/menu_handler.ts (2)
15-25: Implement the comparison hinted by the comment.Re-init when current menu differs from expected item IDs.
handleMenuShown(info: MenuShownInfo): Promise<void> | void { - // Compare the shown menu with menuItems and (re-)initialize the menu if necessary - console.debug(info) - - if (!this.buttonMenu.isInitialized()) { + // (Re-)initialize if uninitialized or IDs differ + console.debug(info) + if (!this.buttonMenu.isInitialized()) { console.debug("Initializing menu") return this.buttonMenu.initItems(this.menuItems, info) } - console.debug("Menu is already initialized") + const expected = new Set(this.menuItems.map((m) => m.id)) + const actual = new Set(info.menuIds) + const same = + expected.size === actual.size && [...expected].every((id) => actual.has(id)) + if (!same) { + console.debug("Menu items changed; re-initializing menu") + return this.buttonMenu.initItems(this.menuItems, info) + } + + console.debug("Menu is already initialized") }
27-31: Defensive guard on menuItemId before casting.Avoid relying on casts; reject unknown IDs early.
- handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> { - // We use command ID as menu item ID, so we can directly pass it to the command handler - return this.commandHandler.runCommand(menuItemId as CommandId, composeTab) - } + handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> { + // Validate the ID before delegating + const isCommandId = (id: string): id is CommandId => + id === "start_ghostbird" || id === "stop_ghostbird" || id === "toggle_ghostbird" + if (!isCommandId(menuItemId)) { + return Promise.reject(Error(`unknown command ${menuItemId}`)) + } + return this.commandHandler.runCommand(menuItemId, composeTab) + }src/app-background/background_event_router.ts (2)
22-22: Validate command string before casting.Reject unknown commands here to avoid undefined bubbling.
- return this.commandHandler.runCommand(command as CommandId, composeTab) + if (!isCommandId(command)) { + return Promise.reject(Error(`unknown command ${command}`)) + } + return this.commandHandler.runCommand(command, composeTab)Add this helper in the module (outside the class):
const isCommandId = (id: string): id is CommandId => id === "start_ghostbird" || id === "stop_ghostbird" || id === "toggle_ghostbird"
43-50: Also guard menuItemId here.Mirror the command guard for menu clicks.
- return this.menuHandler.handleMenuItemClicked(menuItemId, composeTab) + if (!isCommandId(menuItemId)) { + return Promise.reject(Error(`unknown command ${menuItemId}`)) + } + return this.menuHandler.handleMenuItemClicked(menuItemId, composeTab)src/thunderbird/background_util/button_menu.ts (1)
13-15: Consider clarifying initialization state semantics.The method returns
truewhen initialization has started, not when it has completed. While this matches the interface contract, it could be semantically confusing. Consider renaming tohasInitializationStarted()or documenting this behavior more explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
manifest_template.json(1 hunks)src/app-background/api.ts(2 hunks)src/app-background/background_event_router.ts(3 hunks)src/app-background/command_handler.ts(1 hunks)src/app-background/menu_handler.ts(1 hunks)src/root/background.ts(3 hunks)src/root/startup/startup_background.ts(2 hunks)src/test/sanity.test.ts(1 hunks)src/test/startup.test.ts(1 hunks)src/thunderbird/background_util/button_menu.ts(1 hunks)src/thunderbird/messenger/i18n.ts(1 hunks)src/util/time.ts(1 hunks)src/util/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/util/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- manifest_template.json
- src/util/time.ts
- src/test/startup.test.ts
- src/thunderbird/messenger/i18n.ts
- src/app-background/api.ts
- src/root/startup/startup_background.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T13:30:46.981Z
Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/thunderbird/background_util/button_menu.ts:47-66
Timestamp: 2025-10-15T13:30:46.981Z
Learning: `Promise.withResolvers` is supported in TB128/Gecko runtime used by the Thunderbird extension and is already used in multiple files like `src/util/promisifying_queue.ts` and `src/util/lazy_then.ts`.
Applied to files:
src/thunderbird/background_util/button_menu.ts
📚 Learning: 2025-10-15T14:05:25.989Z
Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/thunderbird/background_util/button_menu.ts:52-53
Timestamp: 2025-10-15T14:05:25.989Z
Learning: In Thunderbird WebExtension API, the `menus.create` method's `icons` parameter accepts either a string (MenuIconPath) or an object dictionary (MenuIconDictionary), unlike the standard WebExtension API which only accepts an object. A string can be passed directly as the value for the icons property.
Applied to files:
src/thunderbird/background_util/button_menu.ts
🧬 Code graph analysis (5)
src/app-background/command_handler.ts (3)
src/app-background/compose_action_notifier.ts (1)
ComposeActionNotifier(4-57)src/app-background/api.ts (1)
CommandId(7-7)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/app-background/background_event_router.ts (3)
src/app-background/api.ts (4)
IComposeWindowDetector(34-43)CommandId(7-7)MenuShownInfo(59-62)ITab(26-31)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/app-background/menu_handler.ts (1)
MenuHandler(5-32)
src/app-background/menu_handler.ts (3)
src/app-background/api.ts (4)
IButtonMenu(46-56)MenuItem(65-72)MenuShownInfo(59-62)CommandId(7-7)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/thunderbird/background_util/button_menu.ts (1)
src/app-background/api.ts (3)
IButtonMenu(46-56)MenuItem(65-72)MenuShownInfo(59-62)
src/root/background.ts (3)
src/app-background/api.ts (2)
MenuItem(65-72)MenuShownInfo(59-62)src/root/startup/startup_background.ts (2)
AlarmHeart(14-14)prepareBackgroundRouter(33-40)src/thunderbird/background_util/alarm_heart.ts (1)
AlarmHeart(3-21)
🔇 Additional comments (7)
src/test/sanity.test.ts (1)
23-23: LGTM — test updated for new parameter.Good addition to cover the new prepareBackgroundRouter option shape.
src/app-background/command_handler.ts (1)
12-22: Runtime guard lives upstream; keep this exhaustive.Implementation is clean and exhaustive for CommandId. Since callers cast strings, ensure upstream validates inputs to prevent undefined being returned at runtime.
Confirm router/menu paths validate strings before casting to CommandId.
src/root/background.ts (1)
53-59: Type cast on menus.onShown is valid
Thunderbird’s menus.onShown passes an OnShowData with menuIds for action/toolbar contexts and matches Firefox’s API shape.src/thunderbird/background_util/button_menu.ts (4)
1-11: LGTM!The class structure, singleton pattern, and dependency injection via constructor are well-implemented.
26-33: LGTM!The initialization flag logic properly handles MV3 suspension by checking session storage before recreating menus.
46-50: LGTM!The menu creation pattern is solid: clearing existing items, parallel creation, and final refresh ensure correct state.
52-75: LGTM!The menu item creation is correctly implemented:
Promise.withResolversis supported in TB128 (as confirmed in previous reviews)- The
iconsparameter correctly accepts a string in Thunderbird's API- Error handling via
lastErrorcallback is appropriate- The comment about
commandoption limitations is helpful contextBased on learnings.
snipsnipsnip
force-pushed
the
add-context-menu
branch
from
0456484 to
8ec5a66
Compare
The menu lets users disconnect with a mouse click. Also added a stopwatch utility to measure the time while debugging.
snipsnipsnip
force-pushed
the
add-context-menu
branch
from
0809944 to
aae0aef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/root/background.ts (2)
16-23: LGTM!The menu configuration is correct. The i18n key
"manifest_commands_stop_ghostbird_description"is properly defined in/locales.toml(per project conventions), and the icon path"gray.svg"aligns with existing assets.Based on learnings.
61-71: LGTM!The validation logic correctly filters out events from special windows (when
tabis falsy) and ensuresmenuItemIdis a string before delegating to the router. The error handling appropriately rejects unknown commands.Based on learnings.
🧹 Nitpick comments (1)
src/app-background/menu_handler.ts (1)
28-31: Validate menuItemId before casting to CommandId.The type assertion
menuItemId as CommandIdon line 30 is unsafe. If a caller passes an invalidmenuItemId, the cast will succeed butcommandHandler.runCommandwill fail with a type error at runtime (the switch statement has no default case).Consider adding validation:
handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> { - // We use command ID as menu item ID, so we can directly pass it to the command handler - return this.commandHandler.runCommand(menuItemId as CommandId, composeTab) + // We use command ID as menu item ID, so we can directly pass it to the command handler + const validCommands: ReadonlyArray<CommandId> = ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"] + if (!validCommands.includes(menuItemId as CommandId)) { + return Promise.reject(Error(`Invalid menu item ID: ${menuItemId}`)) + } + return this.commandHandler.runCommand(menuItemId as CommandId, composeTab) }Alternatively, if the upstream validation in
src/root/background.ts(line 64) already ensuresmenuItemIdis a string, consider documenting this contract or adding a runtime assertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
manifest_template.json(1 hunks)src/app-background/api.ts(2 hunks)src/app-background/background_event_router.ts(3 hunks)src/app-background/command_handler.ts(1 hunks)src/app-background/menu_handler.ts(1 hunks)src/root/background.ts(3 hunks)src/root/startup/startup_background.ts(2 hunks)src/test/sanity.test.ts(1 hunks)src/test/startup.test.ts(1 hunks)src/thunderbird/background_util/button_menu.ts(1 hunks)src/thunderbird/messenger/i18n.ts(1 hunks)src/util/time.ts(1 hunks)src/util/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/util/types.ts
- src/test/startup.test.ts
- src/thunderbird/messenger/i18n.ts
- src/app-background/api.ts
- src/thunderbird/background_util/button_menu.ts
- src/util/time.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T08:50:50.310Z
Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/root/background.ts:61-71
Timestamp: 2025-10-16T08:50:50.310Z
Learning: In Thunderbird WebExtensions, when `messenger.menus.onClicked` fires with a falsy `tab` argument, it indicates the event originated from a special window (such as Thunderbird's debug tools) rather than a regular compose window. In `src/root/background.ts`, the intentional design is to reject such events by checking `if (!tab)` to filter out these non-compose-window events.
Applied to files:
src/app-background/background_event_router.tssrc/root/background.ts
📚 Learning: 2025-10-16T08:52:27.564Z
Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/root/background.ts:16-23
Timestamp: 2025-10-16T08:52:27.564Z
Learning: In the ghostbird project, i18n keys are defined in `/locales.toml` and then generated into `_locales/*/messages.json` files using the build script `tools/generate_locale_messages.ts`. When reviewing i18n key usage, check `/locales.toml` rather than the generated `messages.json` files.
Applied to files:
src/root/background.ts
🧬 Code graph analysis (5)
src/root/startup/startup_background.ts (3)
src/app-background/api.ts (2)
MenuItem(65-72)IComposeWindowDetector(34-43)src/app-background/menu_handler.ts (1)
MenuHandler(5-32)src/app-background/command_handler.ts (1)
CommandHandler(7-23)
src/app-background/menu_handler.ts (3)
src/app-background/api.ts (4)
IButtonMenu(46-56)MenuItem(65-72)MenuShownInfo(59-62)CommandId(7-7)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/app-background/command_handler.ts (3)
src/app-background/compose_action_notifier.ts (1)
ComposeActionNotifier(4-57)src/app-background/api.ts (1)
CommandId(7-7)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(41-72)
src/app-background/background_event_router.ts (3)
src/app-background/api.ts (4)
IComposeWindowDetector(34-43)CommandId(7-7)MenuShownInfo(59-62)ITab(26-31)src/app-background/command_handler.ts (1)
CommandHandler(7-23)src/app-background/menu_handler.ts (1)
MenuHandler(5-32)
src/root/background.ts (2)
src/app-background/api.ts (2)
MenuItem(65-72)MenuShownInfo(59-62)src/root/startup/startup_background.ts (1)
prepareBackgroundRouter(33-40)
🔇 Additional comments (13)
manifest_template.json (1)
26-26: LGTM!The
"menus"permission is correctly placed and necessary for the toolbar context menu functionality introduced in this PR.src/app-background/command_handler.ts (1)
7-23: LGTM!The
CommandHandlercorrectly delegates command execution toComposeActionNotifier. The switch statement handles allCommandIdvariants, and the omitted default case enables TypeScript exhaustiveness checking.src/test/sanity.test.ts (1)
23-23: LGTM!The addition of
menuItems: []correctly updates the test to match the newprepareBackgroundRoutersignature.src/root/startup/startup_background.ts (3)
1-1: LGTM!The import changes correctly reflect the refactoring from
ComposeActionNotifiertoMenuHandlerandCommandHandler.
20-20: LGTM!Adding
menuItemstoBackgroundConstantscorrectly extends the configuration to support the new menu functionality.
25-26: LGTM!The
BackgroundCatalogupdates correctly reflect the architectural shift to separate menu and command handling viaMenuHandlerandCommandHandler.src/root/background.ts (1)
53-59: LGTM!The
menus.onShownlistener correctly delegates torouter.handleMenuShownwith appropriate logging.src/app-background/menu_handler.ts (1)
14-25: LGTM!The menu initialization logic correctly checks whether the menu has been initialized and only calls
initItemswhen necessary.src/app-background/background_event_router.ts (5)
9-13: LGTM!The constructor correctly refactors to use
CommandHandlerandMenuHandlerinstead of directComposeActionNotifierdependency.
16-23: LGTM!Command handling correctly delegates to
CommandHandler.runCommandwith proper tab validation.
25-35: LGTM!The compose action handler correctly delegates to
CommandHandlerwith the"start_ghostbird"command.
37-40: LGTM!The
handleMenuShownmethod correctly delegates menu initialization toMenuHandler.
42-50: LGTM!The
handleMenuClickmethod correctly validates the compose tab before delegating toMenuHandler, maintaining consistency with other event handlers.
github-project-automation
bot
moved this from 📥️Inbox
to 🪺Experimental
in Ghostbird Issues
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @snipsnipsnip. * #39 (comment) The following files were modified: * `src/util/time.ts`
Docstrings generation was requested by @snipsnipsnip. * #39 (comment)
Docstrings generation was requested by @snipsnipsnip. * #39 (comment) The following files were modified: * `src/util/time.ts` <details> <summary>These files were ignored</summary> * `src/test/sanity.test.ts` * `src/test/startup.test.ts` </details> <details> <summary>These file types are not supported</summary> * `manifest_template.json` </details> <details> <summary>ℹ️ Note</summary><blockquote> CodeRabbit cannot perform edits on its own pull requests yet. </blockquote></details>
1 participant
Closes #30