context menu interactivity in edit mode + after wedge clicking #592
Conversation
📝 WalkthroughWalkthroughAdds a new CLICK_MENU IPC path and left-click handling by introducing ContextMenuManager.initClickMenu; CommunicationService now emits CLICK_MENU for plain mousedown; ContextMenuManager gains async refresh semantics, leftMouseGodley flow and optional Help menu flag; editor coordinate accessors exposed in TypeScript and C++ and small UI/style and popup-size tweaks. Changes
Sequence DiagramsequenceDiagram
participant User
participant Renderer as Renderer Process
participant Frontend as Frontend Service
participant IPC as IPC Channel
participant Main as Main Process
participant ContextMgr as ContextMenuManager
participant Menu as Context Menu
User->>Renderer: Left-click on canvas/Godley
Renderer->>Frontend: mousedown(x,y,type) (no ctrl/meta)
Frontend->>IPC: emit CLICK_MENU(x,y,type,command)
IPC->>Main: IPC click-menu event
Main->>ContextMgr: initClickMenu(event,x,y,type,command)
rect rgb(235,245,235)
Note over ContextMgr,Frontend: left-click path resolves item & coords
ContextMgr->>Frontend: request item/info & toEditorX/Y, colX, rowY
Frontend-->>ContextMgr: editor coordinates (Promises)
ContextMgr->>ContextMgr: build import-stock submenu (if Godley)
end
ContextMgr->>Menu: display context menu (withHelp true by default)
Menu-->>User: rendered menu/submenu
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| click: () => { | ||
| pubTab.rotateItemAt(this.x,this.y); | ||
| }, | ||
| click: () => pubTab.rotateItemAt(this.x,this.y), |
There was a problem hiding this comment.
you'll notice I've changed a lot of these click methods to makes sure the promises are properly awaited... in some cases this also makes the code more compact by using one-liner syntax although that wasn't my primary concern... Not sure if this has any noticeable consequences but I could just not let these "unawaited promises" pass on my watch...
There was a problem hiding this comment.
It would depend on whether the promise needs to be resolved prior to the next operation or not. Obviously an unwaited promise is an error in the former case. In the latter case (eg an event handler that terminates immediately afterwards), waiting on the promise is essentially harmless, but unnecessary, ie it is really just a matter of style. I guess because I'm used to C++, where waiting causes the actual thread of execution to pause, as opposed to Javascript where waiting simply switches context to processing the next event, I'll often leave those promises unwaited.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts (1)
679-683: Fix Map key check when cleaning upactiveGodleyWindowItems
activeGodleyWindowItemsis aMap, butonPopupWindowCloseuses theinoperator:if (uid in this.activeGodleyWindowItems) { this.activeGodleyWindowItems.delete(uid); }
uid in mapchecks for object properties, not Map keys, so this condition will never be true and entries are never removed. That can leave stale state tracking Godley windows.Consider switching to
.has():Proposed fix
private static onPopupWindowClose(uid: string) { minsky.namedItems.elem(uid).destroyFrame(); - if (uid in this.activeGodleyWindowItems) { - this.activeGodleyWindowItems.delete(uid); - } + if (this.activeGodleyWindowItems.has(uid)) { + this.activeGodleyWindowItems.delete(uid); + } }
🧹 Nitpick comments (1)
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
740-741: New Godley editor coordinate bindings match existing patterns
toEditorX/toEditorYonGodleyIconandcolX/rowYonGodleyTableEditor/GodleyTableWindoware thin$callMethodwrappers consistent with the rest of this generated API.As this file is marked “built”, ensure the underlying generator is updated so these methods persist across regenerations.
Also applies to: 810-810, 854-854, 892-892, 956-956
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
gui-js/apps/minsky-electron/src/app/events/electron.events.tsgui-js/apps/minsky-electron/src/app/managers/CommandsManager.tsgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.tsgui-js/libs/core/src/lib/services/communication/communication.service.tsgui-js/libs/shared/src/lib/backend/minsky.tsgui-js/libs/ui-components/src/lib/input-modal/input-modal.component.scssmodel/godleyIcon.hmodel/godleyTableWindow.ccmodel/godleyTableWindow.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 575
File: gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts:654-668
Timestamp: 2025-10-27T05:57:05.886Z
Learning: For Godley icon context menus in `gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts`, the "Import stock variables" menu item is restricted by column (using the clicked x-coordinate to determine the column) rather than being limited to row0 clicks only. This column-based restriction is sufficient for the context menu approach, as confirmed by practical testing.
📚 Learning: 2025-10-27T05:57:05.886Z
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 575
File: gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts:654-668
Timestamp: 2025-10-27T05:57:05.886Z
Learning: For Godley icon context menus in `gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts`, the "Import stock variables" menu item is restricted by column (using the clicked x-coordinate to determine the column) rather than being limited to row0 clicks only. This column-based restriction is sufficient for the context menu approach, as confirmed by practical testing.
Applied to files:
gui-js/libs/core/src/lib/services/communication/communication.service.tsmodel/godleyTableWindow.hgui-js/apps/minsky-electron/src/app/events/electron.events.tsmodel/godleyTableWindow.ccgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.tsgui-js/libs/shared/src/lib/backend/minsky.ts
🧬 Code graph analysis (5)
gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
gui-js/libs/shared/src/lib/constants/constants.ts (1)
events(18-68)
model/godleyIcon.h (2)
gui-js/libs/shared/src/lib/backend/minsky.ts (2)
toEditorX(740-740)toEditorY(741-741)model/godleyIcon.cc (4)
toEditorX(604-605)toEditorX(604-604)toEditorY(606-607)toEditorY(606-606)
model/godleyTableWindow.h (1)
model/godleyTableWindow.cc (4)
colX(436-444)colX(436-436)rowY(446-452)rowY(446-446)
gui-js/apps/minsky-electron/src/app/events/electron.events.ts (2)
gui-js/libs/shared/src/lib/constants/constants.ts (1)
events(18-68)gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (1)
ContextMenuManager(13-1398)
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
engine/minskyTensorOps.cc (16)
a1(104-219)a1(104-105)a1(281-303)a1(281-283)a1(573-582)a1(573-574)a1(606-636)a1(606-607)a1(786-891)a1(786-787)a1(969-1035)a1(969-970)a1(1406-1415)a1(1406-1408)a1(1422-1450)a1(1422-1424)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (13)
gui-js/libs/ui-components/src/lib/input-modal/input-modal.component.scss (1)
3-8: Input modal spacing change looks consistentThe reduced top margin and
calc(100% - 10px)width with 5px margins are coherent and should avoid overflow while tightening the layout. Just confirm dialogs still look correct on Linux/macOS window chrome.Also applies to: 13-16
gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
330-337: Left-click CONTEXT_MENU flagging is reasonable, but verify non-Godley impactAdding
leftClickto the contextmenu payload and emitting aCONTEXT_MENUevent on every non-ctrl/meta leftmousedowngives the backend enough information to drive Godley-specific left-click menus.However, it also means all plain left-clicks on the canvas now trigger a
CONTEXT_MENUIPC beforecanvas.mouseDown. Please double‑check that:
- ContextMenuManager ignores
leftClick: trueevents except where explicitly desired (e.g. Godley items/wedges), and- There is no visible flicker or accidental menu opening when clicking non-Godley canvas areas.
If necessary, you could early‑filter here (e.g. only emit for certain tabs or hit‑tested items), but that may already be handled downstream.
Also applies to: 394-406
model/godleyIcon.h (1)
170-172: Public exposure of editor coordinate helpers looks fineMaking
toEditorX/toEditorYpublic matches their usage from the generated TS bindings and doesn’t alter behavior. Just treat these as part of the public API going forward and keep the signatures in sync withgodleyIcon.ccandminsky.ts.gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts (1)
224-245: Reduced Godley title/currency popup height – confirm no clippingDropping these popups’ heights to 80px aligns with the tighter input-modal spacing, but it’s a fairly small vertical size. Please sanity‑check on all target platforms that:
- The input field and buttons are fully visible (no scrollbars or clipped borders), and
- Window manager chrome/titlebars don’t eat into the usable content area excessively.
If everything looks fine in manual testing, the change is good to go.
model/godleyTableWindow.cc (2)
206-211: LGTM: Visual indicator for import-stock columns.The ▼ marker is correctly positioned and restricted to stock variable columns (col > 1), consistent with the column-based import workflow. The pulldownHot spacing adjustment to 16 properly accommodates the marker.
Based on learnings, the column-based restriction for import stock variables is the correct approach.
516-517: LGTM: Proper handling of importStock click type.The explicit case with break correctly prevents fall-through and aligns with the left-click context menu workflow introduced in the frontend layer.
model/godleyTableWindow.h (2)
91-91: LGTM: Appropriate spacing adjustment for UI indicator.The increase from 12 to 16 provides adequate space for the ▼ marker in stock variable columns, consistent with the rendering logic in the .cc file.
154-158: LGTM: Public coordinate accessors enable external integration.Exposing
colXandrowYas public methods is appropriate for supporting coordinate mapping in the backend API surface. Both methods are const with no side effects, making them safe to expose.gui-js/apps/minsky-electron/src/app/events/electron.events.ts (1)
233-234: LGTM: IPC handler correctly forwards leftClick parameter.The handler properly destructures and forwards the
leftClickparameter toContextMenuManager.initContextMenu, enabling left-click context menu support for Godley items.gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (4)
19-62: LGTM: Clean separation of left-click and right-click handling.The refactored
initContextMenumethod cleanly separates left-click and right-click paths. The switch statement improves readability and maintainability, covering all expected context menu types with appropriate default-case logging.
220-239: LGTM: Consistent refactoring to inline arrow functions.The conversion to inline arrow functions is consistent with the refactoring pattern throughout the file, with no functional changes.
635-643: Verify coordinate transformation consistency.The coordinate transformation at lines 635-639 matches the pattern in
leftMouseGodley(lines 198-201), which is good. However, ensure this transformation is correct for both left-click and right-click scenarios.Both methods use:
await godley.toEditorX(this.x) / await godley.editor.zoomFactor()This pattern divides by
zoomFactor, suggestingtoEditorXreturns zoomed coordinates. Verify this matches the C++ implementation behavior.The verification script from the previous comment will cover this as well.
1186-1186: LGTM: Consistent Promise-based refresh pattern.Changing
refreshFunctionto returnPromise<void>properly reflects the async nature of refresh operations. All call sites correctly await the result.
highperformancecoder
left a comment
highperformancecoder
left a comment
There was a problem hiding this comment.
I think there are a few minor suggestions for code improvements here, but otherwise the changes look good. I haven't tested this yet, of course, so everything is subject to the code working as intended :)
There's also a couple of CodeRabbit nits worth following up on too.
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
Outdated
Show resolved
Hide resolved
|
Feature works, but I don't like the Help menu item at the top. I checked the Godley title and set currency dialogs on MacOSX, and all good. I tried checking them on a Windows builds, and nada! Canvas is completely stuffed, reminiscient of ticket 1897: https://sourceforge.net/p/minsky/tickets/1897/. Funny thing was, these builds were working yesterday, but now anything built after 10th December fails to work, with no error messages on console to indicate what might be going wrong. The last Windows update I had is dated 11/12, but then why would builds I did yesterday, work yesterday, but stop working today. Very mysterious. |
Help item is removed in latest commit. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (1)
1172-1172: Optional: Async wrapper unnecessary for synchronous IPC send.The
asynckeyword here wraps the synchronousevent.sender.sendcall in a Promise, which works but adds no functional value sincesendreturns void immediately. The current implementation is consistent with the async refresh pattern and causes no issues, so this is purely a style consideration.🔎 Optional simplification
If you prefer to avoid the async overhead for synchronous operations:
- const refreshFunction = async () => event.sender.send(events.GODLEY_POPUP_REFRESH); + const refreshFunction = () => { event.sender.send(events.GODLEY_POPUP_REFRESH); return Promise.resolve(); };Or simply document that the async wrapper is intentional for pattern consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gui-js/apps/minsky-electron/src/app/events/electron.events.tsgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.tsgui-js/libs/core/src/lib/services/communication/communication.service.tsgui-js/libs/shared/src/lib/constants/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- gui-js/apps/minsky-electron/src/app/events/electron.events.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 575
File: gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts:654-668
Timestamp: 2025-10-27T05:57:05.886Z
Learning: For Godley icon context menus in `gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts`, the "Import stock variables" menu item is restricted by column (using the clicked x-coordinate to determine the column) rather than being limited to row0 clicks only. This column-based restriction is sufficient for the context menu approach, as confirmed by practical testing.
📚 Learning: 2025-10-27T05:57:05.886Z
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 575
File: gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts:654-668
Timestamp: 2025-10-27T05:57:05.886Z
Learning: For Godley icon context menus in `gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts`, the "Import stock variables" menu item is restricted by column (using the clicked x-coordinate to determine the column) rather than being limited to row0 clicks only. This column-based restriction is sufficient for the context menu approach, as confirmed by practical testing.
Applied to files:
gui-js/libs/shared/src/lib/constants/constants.tsgui-js/libs/core/src/lib/services/communication/communication.service.tsgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
📚 Learning: 2025-07-24T23:00:53.077Z
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 558
File: gui-js/libs/ui-components/src/lib/connect-database/connect-database.component.ts:47-50
Timestamp: 2025-07-24T23:00:53.077Z
Learning: In the Minsky application, backend calls (like database operations) are already wrapped in try-catch blocks at the backend level, and error dialogs are automatically shown to users when exceptions occur. Frontend components don't need additional error handling for these backend operations.
Applied to files:
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
🧬 Code graph analysis (1)
gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
gui-js/libs/shared/src/lib/constants/constants.ts (1)
events(18-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript, 22.04)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (6)
gui-js/libs/shared/src/lib/constants/constants.ts (1)
26-26: LGTM! Clean addition of the CLICK_MENU event constant.The new event key follows the existing naming convention and integrates well with the IPC event flow for left-click menu handling.
gui-js/libs/core/src/lib/services/communication/communication.service.ts (1)
393-404: LGTM! CLICK_MENU emission properly sequenced before mouseDown.The new event emission allows the context menu system to handle left-click interactions (e.g., Godley stock import menu) before the standard mouseDown processing occurs. The implementation preserves existing behavior while adding the new capability.
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (4)
58-66: LGTM! Clean separation of left-click handling from context menu flow.The new
initClickMenumethod provides a clear entry point for left-click interactions, separate from the right-clickinitContextMenu. The structure is extensible—additional left-click behaviors can be added alongside the Godley handling.
188-217: LGTM! Left-click Godley handling is well-structured.The method correctly:
- Retrieves editor coordinates with proper zoomFactor scaling (line 195 pulls zoomFactor first, as suggested in past reviews)
- Determines clickType and column for context-sensitive menu display
- Builds dynamic stock-import submenu based on
matchingTableColumnsByCol- Conditionally shows menu only when in editor mode with 'importStock' clickType (otherwise silent no-op, which is appropriate)
The coordinate transformations align with the existing
rightMouseGodleypattern and C++ backend expectations per learnings.Based on learnings, column-based restriction for "Import stock variables" is sufficient for the context menu approach.
341-344: Good addition of thewithHelpparameter for menu flexibility.The optional
withHelp = trueparameter allows selectively disabling the Help menu item (used in the stock-import submenu at line 213). The default value preserves backward compatibility for all existing call sites.Also applies to: 347-353
634-644: LGTM! Improved separation of cell-level vs. icon-level Godley menus.The coordinate calculations enable context-aware menu display:
- When clicking a cell (clickType !== 'background'), delegates to the detailed
initContextMenuForGodleyfor cell-specific operations- When clicking the background, shows the icon-level menu with editor mode toggles, export options, etc.
The explicit
!== 'background'check is clearer than checking for clickType existence.
| menuItems: MenuItem[], | ||
| mainWindow: Electron.BrowserWindow | ||
| mainWindow: Electron.BrowserWindow, | ||
| withHelp = true |
There was a problem hiding this comment.
OK - another boolean flag I'm not totally happy with, but we can leave that for another refactor (if it happens).
| await godley.popup.selectedCol(c); | ||
| await godley.popup.insertIdx(0); | ||
| await godley.popup.selectIdx(0); | ||
| await Promise.all([godley.popup.selectedRow(r), godley.popup.selectedCol(c), godley.popup.insertIdx(0), godley.popup.selectIdx(0)]); |
There was a problem hiding this comment.
In Promise.all, are the arguments evaluated in order, or are they executed in random order? Not that it matters here, as they are all independent methods anyway.
I got to the bottom (sort of) of things - the problem was the upgrade to Electron 39, which interfered with the native window drawing of the Canvas on Windows only (other platforms fine). Downgrading Electron to version 37 fixed the Canvas issue. Another pro of implementing the canvas in something like D3 will be to avoid this sort of issue in future upgrades. |
highperformancecoder
merged commit 6d37ac8
into
highperformancecoder:master
2 participants
This change is
Summary by CodeRabbit
New Features
UI
Public API
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.