Handle overlay config reload on live updates#98
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/overlay/overlay.cpp
Outdated
| void Overlay::apply_pending_config() { | ||
| PendingConfig pending; | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_pending_mutex); | ||
| if (!m_pending_config) { | ||
| return; | ||
| } | ||
| pending = *m_pending_config; | ||
| m_pending_config.reset(); | ||
| } | ||
|
|
||
| if (pending.spawn_strategy == "cursor_follow") { | ||
| m_spawn_strategy = BadgeSpawnStrategy::CursorFollow; | ||
| } else { | ||
| m_spawn_strategy = BadgeSpawnStrategy::RandomScreen; | ||
| } | ||
|
|
||
| m_badge_min_px = pending.badge_min_px; | ||
| m_badge_max_px = pending.badge_max_px; | ||
| m_badges_per_second_max = pending.badges_per_second_max; | ||
|
|
||
| if (pending.fps_mode == "fixed") { | ||
| set_fps_fixed(pending.fps_fixed); | ||
| set_fps_mode(platform::FpsMode::Fixed); | ||
| } else { | ||
| set_fps_mode(platform::FpsMode::Auto); | ||
| } | ||
|
|
||
| auto normalized = normalize_path(pending.emoji_atlas); | ||
| bool atlas_changed = normalized != m_current_emoji_path; | ||
| if (atlas_changed) { | ||
| if (load_atlas_from_path(normalized)) { | ||
| m_badges.clear(); | ||
| m_spawn_times.clear(); | ||
| } else { | ||
| normalized = m_current_emoji_path; | ||
| } | ||
| } | ||
|
|
||
| build_selector(pending.emoji, pending.emoji_weighted); |
There was a problem hiding this comment.
The new apply_pending_config mutates m_spawn_strategy, m_badge_min_px, m_badge_max_px, m_badges_per_second_max, m_selector_indices, m_selector, and the sprite atlas in place when a reload occurs. These members are read from the keyboard hook thread via spawn_badge/select_sprite without any synchronization. With runtime refresh now possible, a config reload can reorder or resize the sprite/selector vectors while the hook thread is in the middle of adding a badge, yielding a data race and potential use-after-free or corrupted RNG state. Consider protecting these shared structures (e.g., with a mutex or event queue) or ensuring spawns occur on the same thread as the refresh.
Useful? React with 👍 / 👎.
|
@codex fix comments |
|
Summary
Testing
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/overlay/overlay.cpp
Outdated
| m_sprite_lookup = std::move(lookup); | ||
| m_sprites = std::move(sprites); | ||
| m_current_emoji_path = normalized; |
There was a problem hiding this comment.
The new load_atlas_from_path swaps m_sprite_lookup and m_sprites directly, but spawn_badge/select_sprite read these structures under m_spawn_config_mutex to generate badges. When a config reload changes the atlas, apply_pending_config calls this function before it acquires the mutex to rebuild the selector, so another thread can call spawn_badge while the atlas vectors have already been replaced but the selector still contains indices for the old arrays. Rendering later indexes m_sprites[b.sprite] and can hit out‑of‑range data, producing crashes during live reloads under key spam. The atlas update should also hold m_spawn_config_mutex (or defer swapping until inside the locked section) so the sprite table and selector stay consistent.
Useful? React with 👍 / 👎.
|
@codex fix comments |
|
Summary
Testing
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/overlay/overlay.cpp
Outdated
| void Overlay::apply_pending_config() { | ||
| PendingConfig pending; | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_pending_mutex); | ||
| if (!m_pending_config) { | ||
| return; | ||
| } | ||
| pending = *m_pending_config; | ||
| m_pending_config.reset(); | ||
| } | ||
|
|
||
| auto normalized = normalize_path(pending.emoji_atlas); | ||
| bool atlas_changed = normalized != m_current_emoji_path; | ||
| std::optional<AtlasData> atlas; | ||
| if (atlas_changed) { | ||
| atlas = load_atlas_from_path(normalized); | ||
| if (atlas) { | ||
| m_badges.clear(); | ||
| m_spawn_times.clear(); | ||
| } else { |
There was a problem hiding this comment.
When a config refresh arrives, apply_pending_config clears m_badges and m_spawn_times before taking m_spawn_config_mutex. Key presses on the hook thread call spawn_badge without holding any mutex for these containers. If the user reloads the configuration while keys are still being pressed, the reload thread running on the render loop can erase the vectors at the same time another thread pushes into them, leading to iterator invalidation and undefined behaviour (crash or corrupted badge state). The same mutex that guards spawn configuration should also wrap the container clear, or spawns should be stopped while the vectors are reset.
Useful? React with 👍 / 👎.
|
@codex fix comments |
|
Summary
Testing
|
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_69050af7b1548325a6f95a7e36efcc47