Arrange for Godley tables to refresh all other Godley tables on canva… #599
Conversation
…s, for #1466. Also arrange for Godley popups to refresh all other Godley popups on finishing editing.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an event-driven cross-window Godley popup refresh (new shared event constant, IPC listener, and WindowManager broadcaster), changes Godley widget to emit that event instead of calling refresh directly, calls balanceDuplicateColumns on Godley icon mouse-leave, and updates two submodule pointers and Makefile LD_LIBRARY_PATH. Changes
Sequence DiagramsequenceDiagram
actor User
participant Renderer as "GodleyWidgetView (Renderer)"
participant Main as "Electron Main"
participant WindowMgr as "WindowManager (Main)"
participant Other as "Other Renderer Window"
User->>Renderer: trigger hardRefresh()
Renderer->>Main: emit REFRESH_ALL_GODLEY_POPUPS
Main->>WindowMgr: forward REFRESH_ALL_GODLEY_POPUPS
WindowMgr->>WindowMgr: iterate activeWindows
WindowMgr->>Other: send GODLEY_POPUP_REFRESH
Other->>Other: refresh Godley popups
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR implements functionality to refresh all Godley tables and popups when changes are made, addressing issue #1466. The changes ensure that when a user finishes editing in one Godley table or popup, all other related instances are automatically updated to reflect those changes.
Changes:
- Added backend support to balance duplicate columns across Godley tables
- Implemented IPC event handling to trigger refresh of all Godley popups
- Modified the Godley widget component to send refresh events instead of local-only updates
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| model/godleyIcon.cc | Adds call to balance duplicate columns when exiting editor mode |
| gui-js/libs/ui-components/src/lib/godley-widget-view/godley-widget-view.component.ts | Changes local refresh to broadcast refresh event to all Godley popups |
| gui-js/libs/shared/src/lib/constants/constants.ts | Defines new IPC event constant for refreshing all Godley popups |
| gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts | Implements method to send refresh events to all Godley popup windows |
| gui-js/apps/minsky-electron/src/app/events/electron.events.ts | Registers IPC handler for the refresh all Godley popups event |
| RavelCAPI | Updates subproject commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| static refreshAllGodleyPopups() { | ||
| for (const [num, win] of WindowManager.activeWindows) |
There was a problem hiding this comment.
The loop variable 'num' is declared but never used. Consider using an underscore to indicate it's intentionally unused: for (const [_num, win] of WindowManager.activeWindows).
| for (const [num, win] of WindowManager.activeWindows) | |
| for (const [_num, win] of WindowManager.activeWindows) |
| ); | ||
|
|
||
| ipcMain.on( | ||
| events.REFRESH_ALL_GODLEY_POPUPS,async (event) => { |
There was a problem hiding this comment.
Add a space after the comma before 'async' for consistent formatting: events.REFRESH_ALL_GODLEY_POPUPS, async (event) => {.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ); | ||
|
|
||
| ipcMain.on( | ||
| events.REFRESH_ALL_GODLEY_POPUPS,async (event) => { |
There was a problem hiding this comment.
The 'event' parameter is declared but never used in the handler. Consider using an underscore to indicate it's intentionally unused: async (_event) => {.
| events.REFRESH_ALL_GODLEY_POPUPS,async (event) => { | |
| events.REFRESH_ALL_GODLEY_POPUPS,async (_event) => { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model/godleyIcon.cc (1)
630-639: Guard against invalid selected column before balancing.Line 635 calls
balanceDuplicateColumnseven wheneditor.selectedColmay be-1. If the callee assumes a valid column index, this can cause out-of-range behavior or unintended updates.🛠️ Proposed fix
- minsky().balanceDuplicateColumns(*this, editor.selectedCol); + if (editor.selectedCol >= 0) + minsky().balanceDuplicateColumns(*this, editor.selectedCol);
🧹 Nitpick comments (1)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
375-378: Remove unused loop variable to satisfy static analysis.Line 376 binds
numbut never uses it.♻️ Suggested tweak
- for (const [num, win] of WindowManager.activeWindows) + for (const [, win] of WindowManager.activeWindows) if (win.context!=WindowManager.getMainWindow()) win.context?.webContents?.send(events.GODLEY_POPUP_REFRESH);
…rt, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
@highperformancecoder I've opened a new pull request, #600, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts`:
- Around line 375-379: The refreshAllGodleyPopups loop can call webContents.send
on destroyed objects; update WindowManager.refreshAllGodleyPopups to skip any
entry where the BrowserWindow is destroyed or where its webContents is missing
or destroyed (check win.context?.isDestroyed() and
win.context.webContents?.isDestroyed()) and continue if it is the main window
via WindowManager.getMainWindow(); only call
webContents.send(events.GODLEY_POPUP_REFRESH) when both the window and its
webContents are present and not destroyed.
…rmancecoder/minsky into bugfix-1466-godleyTableUpdates
…disappearing throwing.
…s, for #1466.
Also arrange for Godley popups to refresh all other Godley popups on finishing editing.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.