feat: Add open/close all Godley tables menu items. For feature #173.#657
feat: Add open/close all Godley tables menu items. For feature #173.#657highperformancecoder wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds the ability to open and close all Godley tables in bulk through menu commands. The changes include new backend C++ methods to enumerate Godley tables and resolve named items, a TypeScript bridge to access them, refactored command dispatching, and menu items wired to the bulk operations. ChangesGodley Table Bulk Operations
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts`:
- Around line 894-897: openAllGodleyTables currently calls openGodleyTable(id)
without awaiting it, causing fire-and-forget async calls and possible unhandled
rejections; update openAllGodleyTables to await the async opens (either
sequentially with "for ... of" + await openGodleyTable(id) or concurrently with
Promise.all over minsky.allGodleyTables().map(id => openGodleyTable(id))) and
ensure errors are propagated/handled so unhandled rejections are avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb3e2587-e75f-416f-9509-e7474ca49f6d
📒 Files selected for processing (6)
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.tsgui-js/apps/minsky-electron/src/app/managers/CommandsManager.tsgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.tsgui-js/libs/shared/src/lib/backend/minsky.tsmodel/minsky.ccmodel/minsky.h
| static async openAllGodleyTables() { | ||
| for (const id of await minsky.allGodleyTables()) | ||
| this.openGodleyTable(id); | ||
| } |
There was a problem hiding this comment.
Await bulk Godley opens to avoid unhandled async failures.
At Line 895, openGodleyTable(id) is async but not awaited, so bulk-open runs as fire-and-forget. That can produce unhandled rejections and nondeterministic behavior when many tables are opened.
Proposed fix
static async openAllGodleyTables() {
- for (const id of await minsky.allGodleyTables())
- this.openGodleyTable(id);
+ for (const id of await minsky.allGodleyTables()) {
+ await this.openGodleyTable(id);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static async openAllGodleyTables() { | |
| for (const id of await minsky.allGodleyTables()) | |
| this.openGodleyTable(id); | |
| } | |
| static async openAllGodleyTables() { | |
| for (const id of await minsky.allGodleyTables()) { | |
| await this.openGodleyTable(id); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
894 - 897, openAllGodleyTables currently calls openGodleyTable(id) without
awaiting it, causing fire-and-forget async calls and possible unhandled
rejections; update openAllGodleyTables to await the async opens (either
sequentially with "for ... of" + await openGodleyTable(id) or concurrently with
Promise.all over minsky.allGodleyTables().map(id => openGodleyTable(id))) and
ensure errors are propagated/handled so unhandled rejections are avoided.
There was a problem hiding this comment.
How do menu handlers handle asynchronous rejections?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ApplicationMenuManager to see how menu handlers are structured
fd -e ts ApplicationMenuManager --exec cat {}
# Also search for how click handlers are generally structured in menus
rg -n "click.*async\|async.*click\|openAllGodleyTables\|closeAllGodleyTables" --type ts -C 3
# Check for global unhandledRejection handlers
rg -n "unhandledRejection\|uncaughtException" --type ts -C 3Length of output: 22444
@highperformancecoder — They don't, at least not at the menu level. Here's what the actual wiring looks like in ApplicationMenuManager:
{
label: 'Open All Godley Tables',
click() { CommandsManager.openAllGodleyTables(); }, // synchronous click, Promise dropped
},The click function is synchronous — it calls an async function but neither awaits nor .catch()-es the returned Promise. That means:
- Any rejection from
openAllGodleyTables()(or fromopenGodleyTableinside it) immediately becomes an unhandled Promise rejection. - There is no global
unhandledRejection/uncaughtExceptionhandler found anywhere in the TypeScript code to catch it as a fallback. - In Electron ≥14 / Node.js ≥15 the process-level default for unhandled rejections is to terminate the process (though Electron may demote it to a warning in practice).
So my original comment about awaiting openGodleyTable(id) inside the loop is still relevant (it prevents racing opens and surfaces errors to the caller), but the more complete fix is at the menu-handler level too:
// Option A: propagate errors sequentially
{
label: 'Open All Godley Tables',
async click() {
try {
await CommandsManager.openAllGodleyTables();
} catch (err) {
console.error('openAllGodleyTables failed', err);
}
},
},// Option B: open concurrently and catch per-table
static async openAllGodleyTables() {
const ids = await minsky.allGodleyTables();
await Promise.allSettled(ids.map(id => this.openGodleyTable(id)));
}Promise.allSettled is particularly handy here because it lets all opens run concurrently and collects both successes and failures without short-circuiting, matching the "best-effort bulk open" intent.
That said, looking at the broader codebase, several other menu items follow the same fire-and-forget pattern (e.g. upgrade(), randomLayout()), so this is a pre-existing pattern rather than something introduced by this PR. Whether to address it consistently across all handlers is a separate question, but for the new bulk-open items it would be worth being explicit since they involve network-style async window creation.
There was a problem hiding this comment.
Pull request overview
This PR adds UI commands and backend support to open/close Godley table windows in bulk, as well as refactors Godley-table opening to be ID-based.
Changes:
- Added a backend API to enumerate Godley table IDs and exposed it to the JS/Electron client.
- Added “Open All Godley Tables” / “Close All Godley Tables” actions to the application menu and Godley context menu.
- Updated Godley table opening logic to operate on item IDs and to (re)populate
namedItemswhen needed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| model/minsky.h | Declares allGodleyTables() and moves itemFromNamedItem() to an out-of-line definition. |
| model/minsky.cc | Implements itemFromNamedItem() fallback population and adds allGodleyTables() implementation. |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Exposes allGodleyTables() to the TypeScript client wrapper. |
| gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts | Adds context menu entries for opening/closing all Godley tables and adjusts single-table open behavior. |
| gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts | Adds bulk open/close commands and refactors Godley opening to accept an ID. |
| gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts | Adds application menu items for opening/closing all Godley tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const id=await minsky.canvas.item.id(); | ||
| if (id) { | ||
| await minsky.nameCurrentItem(id); | ||
| CommandsManager.openGodleyTable(id); |
| for (const id of await minsky.allGodleyTables()) | ||
| this.openGodleyTable(id); |
There was a problem hiding this comment.
Backend commands are queued and run on a single backend thread, rather than run concurrently. There is no issue with running the commands out of order.
| const id=await minsky.canvas.item.id(); | ||
| if (id) | ||
| CommandsManager.openGodleyTable(id); |
| click() {CommandsManager.openAllGodleyTables();}, | ||
| }, | ||
| { | ||
| label: 'Close All Godley Tables', | ||
| click() {CommandsManager.closeAllGodleyTables();}, |
This change is
Summary by CodeRabbit