Conversation
📝 WalkthroughWalkthroughBumped Changes
Sequence DiagramsequenceDiagram
participant Script as Main Script
participant Window as ElanthiaMap::Window
participant Queue as Gtk.queue
participant Cleanup as Cleanup Block
participant GTK as GTK Engine
participant GC as Ruby GC
Script->>Window: call destroy
Window->>Window: set completed = false, arrange cleanup state
Window->>Queue: queue Cleanup block (captures Procs/widgets via anchors)
Note right of Window: bounded polling loop waits for completed
Queue->>Cleanup: run queued cleanup
Cleanup->>Cleanup: destroy note-pin widgets -> markers -> menus -> layout children
Cleanup->>GTK: destroy `@gtk_window`, disconnect signal handlers
Cleanup->>Window: set completed = true, clear `@_dynamic_menu_gc` and `@_prevent_gc`
Window->>Script: polling sees completed, return
Script->>GC: Procs/handlers become eligible for collection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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)
scripts/map.lic (1)
1579-1605:⚠️ Potential issue | 🟠 MajorUnanchored scale submenu items when global mode is enabled.
When
global_scale_enabledis true at line 1584, the scale submenu rebuild is skipped. However, line 1581 clears@_dynamic_menu_gcunconditionally for all menus, releasing the old scale submenu items before that conditional check. These items—which contain Procs with signal handlers—remain in the GTK widget tree but lose their Ruby anchor, leaving them vulnerable to garbage collection during normal operation while GTK still references them. Either use a fresh anchor array and keep the previous one alive until replaced submenus are destroyed, or rebuild the scale submenu regardless of the global_scale_enabled setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/map.lic` around lines 1579 - 1605, Clearing the global anchor array `@_dynamic_menu_gc` before you decide whether to rebuild the scale submenu unanchors Procs for `@menu_scale` when `@settings`[:global_scale_enabled] is true; either preserve the old anchors until the submenu is explicitly destroyed or always rebuild the scale submenu. Fix by changing the order in the refresh logic: either (A) move `@_dynamic_menu_gc.clear` into each menu-specific branch so you only clear anchors for menus you rebuild (references: `@_dynamic_menu_gc`, `@menu_scale`, `@menu_tags`, `@menu_locations`, build_scale_submenu), or (B) allocate a fresh anchor array (old_anchor = `@_dynamic_menu_gc`; `@_dynamic_menu_gc` = []) and only discard old_anchor after you have destroyed/replaced the corresponding submenu widgets so the Procs remain anchored while GTK still holds them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/map.lic`:
- Around line 602-670: The destroy logic currently relies only on the
`@_timeout_proc` ivar and never cancels scheduled GLib timeouts, so earlier
timeouts can leak C-side references; modify the code that schedules callbacks
(where GLib::Timeout.add(50, &@_timeout_proc) is called) to capture and store
the returned source id in a dedicated ivar (e.g. `@_timeout_source_id` or
similar), and in the destroy method (the block that runs before clearing
`@_prevent_gc` and `@_dynamic_menu_gc`) call
GLib::Timeout.remove(`@_timeout_source_id`) for any existing source id (and
clear/reset both `@_timeout_source_id` and `@_timeout_proc`) before releasing GC
anchors to ensure pending timeouts are canceled.
---
Outside diff comments:
In `@scripts/map.lic`:
- Around line 1579-1605: Clearing the global anchor array `@_dynamic_menu_gc`
before you decide whether to rebuild the scale submenu unanchors Procs for
`@menu_scale` when `@settings`[:global_scale_enabled] is true; either preserve the
old anchors until the submenu is explicitly destroyed or always rebuild the
scale submenu. Fix by changing the order in the refresh logic: either (A) move
`@_dynamic_menu_gc.clear` into each menu-specific branch so you only clear anchors
for menus you rebuild (references: `@_dynamic_menu_gc`, `@menu_scale`, `@menu_tags`,
`@menu_locations`, build_scale_submenu), or (B) allocate a fresh anchor array
(old_anchor = `@_dynamic_menu_gc`; `@_dynamic_menu_gc` = []) and only discard
old_anchor after you have destroyed/replaced the corresponding submenu widgets
so the Procs remain anchored while GTK still holds them.
|
@coderabbitai review again |
✅ Actions performedReview triggered.
|
|
✅ Actions performedReview triggered.
|
…C safety Updated signal handler connections to use Ruby Procs for better garbage collection management. Improved crash fixes and menu item handling.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/map.lic (1)
2067-2077:⚠️ Potential issue | 🟡 MinorStore and cancel pending timeout in destroy.
The proc is correctly anchored, but
GLib::Timeout.addreturns a source_id that should be stored to allow cancellation. Ifdestroy()is called while a timeout is pending, the callback may fire on a partially-destroyed window. Store the source_id and callGLib::Timeout.remove()indestroy()before clearing GC anchors.,
🛡️ Proposed fix to store and cancel timeout
# Wait a moment for the layout to be realized with new size # Store Proc in ivar to prevent GC before GLib invokes it `@_timeout_proc` = proc do begin center_viewport_on(center_x, center_y) rescue StandardError # Ignore if window is being destroyed end `@_timeout_proc` = nil + `@_timeout_source_id` = nil false # Don't repeat end - GLib::Timeout.add(50, &@_timeout_proc) + `@_timeout_source_id` = GLib::Timeout.add(50, &@_timeout_proc)And in
destroy(), before clearing GC anchors (around line 663):+ # Cancel any pending timeout + if `@_timeout_source_id` + GLib::Timeout.remove(`@_timeout_source_id`) rescue nil + `@_timeout_source_id` = nil + `@_timeout_proc` = nil + end + # Release all GC-prevention refs now that widgets are destroyed `@_prevent_gc.clear` `@_dynamic_menu_gc.clear`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/map.lic` around lines 2067 - 2077, The timeout source_id returned by GLib::Timeout.add is not stored and thus cannot be canceled; modify the code around GLib::Timeout.add to assign its return to an instance variable (e.g. `@_timeout_source_id`) while keeping `@_timeout_proc` to prevent GC, and update destroy (the destroy method) to check for and call GLib::Timeout.remove(`@_timeout_source_id`) (and clear `@_timeout_source_id` and `@_timeout_proc`) before clearing other GC anchors so the pending callback cannot run on a partially-destroyed window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/map.lic`:
- Around line 2067-2077: The timeout source_id returned by GLib::Timeout.add is
not stored and thus cannot be canceled; modify the code around GLib::Timeout.add
to assign its return to an instance variable (e.g. `@_timeout_source_id`) while
keeping `@_timeout_proc` to prevent GC, and update destroy (the destroy method) to
check for and call GLib::Timeout.remove(`@_timeout_source_id`) (and clear
`@_timeout_source_id` and `@_timeout_proc`) before clearing other GC anchors so the
pending callback cannot run on a partially-destroyed window.
1 participant
Summary by CodeRabbit
Release Notes - v2.1.1