Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated 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, enqueue cleanup
Window->>Queue: queue Cleanup block
Note right of Window: poll/sleep until completed or timeout
Queue->>Cleanup: run queued cleanup
Cleanup->>Cleanup: destroy note pins -> markers -> menus -> layout children
Cleanup->>GTK: destroy `@gtk_window`, disconnect handlers
Cleanup->>Window: set completed = true, clear GC anchors
Window->>Script: return after polling sees completed
Script->>GC: Procs/handlers become eligible for collection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
|
1 participant
@coderabbitai summary