Empty profile#7
Open
seekey13 wants to merge 15 commits into
Open
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…e display spacing Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
- Implemented a logging module to centralize logging functions (printf, warnf, errorf, debugf). - Created a modifiers helper module for handling key modifier conversions and string manipulations. - Developed a comprehensive UI functions module for managing key bindings, including validation, command generation, and profile file operations. - Introduced a virtual key codes module for mapping Windows VK codes to key names and vice versa, enhancing key detection capabilities.
This was
linked to
issues
May 13, 2026
📝 WalkthroughWalkthroughThis PR introduces a cross-job global keybindings system that allows shared keybinds to override job-specific ones. The changes add centralized logging and modifier utilities, refactor binding operations to track global status, extend the UI to load and display global bindings with visual indicators, and update the main script to load global bindings after job bindings. ChangesGlobal Bindings Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds automatic empty profile creation and introduces global binding support, while refactoring shared helpers for logging, modifiers, and virtual key mappings.
Changes:
- Creates missing job/subjob profile files instead of failing to load.
- Adds global binding loading/saving through
JobBinds.txt. - Refactors key/modifier handling and logging into shared modules.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents global bindings behavior and usage. |
| lib/vk_codes.lua | Adds VK code/name lookup helpers. |
| lib/ui_functions.lua | Refactors binding parsing/saving and modifier handling. |
| lib/modifiers.lua | Adds shared Ctrl/Alt/Shift conversion helpers. |
| lib/log.lua | Adds centralized addon logging/debug handling. |
| lib/keyboard_ui.lua | Adds global binding UI/state handling and keyboard layout updates. |
| lib/blocked_keybinds.lua | Simplifies blocked-key validation and updates blocked keys. |
| jobbinds.lua | Creates missing profiles and loads global bindings after job profiles. |
Comments suppressed due to low confidence (2)
lib/keyboard_ui.lua:141
- The new Insert/Delete buttons use
INSandDEL, but the blocked-key checks use the canonical names inblocked_keybinds(INSERTandDELETE). Clicking these buttons therefore bypasses the protection for Insert and Ctrl+Alt+Delete; use the same key names as the blocked-key table/vk mapping or normalize aliases before validation.
lib/keyboard_ui.lua:120 - Global bindings are only suppressing job-specific bindings with the same modifier combination. The documented restriction is key-wide (a global
Mshould block job-specificM,Shift+M,Ctrl+M, etc.), so existing job-specific modifier variants remain visible and can stay active unless they are explicitly filtered/removed for any key that has a global binding.
This issue also appears on line 136 of the same file.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+178
to
+185
| -- Then load global bindings (JobBinds.txt) to override job-specific ones | ||
| local global_path = string.format('%s/scripts/JobBinds.txt', AshitaCore:GetInstallPath()) | ||
| local global_file = io.open(global_path, "r") | ||
| if global_file then | ||
| global_file:close() | ||
| debugf('Loading global bindings from JobBinds.txt (overriding job-specific)') | ||
| pcall(function() | ||
| AshitaCore:GetChatManager():QueueCommand(-1, '/exec JobBinds.txt') |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/keyboard_ui.lua (1)
584-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate delete failures instead of always returning success.
delete_current_binding()ignores everydelete_modifier_binding()return value, then re-merges, clears the editor, and returnstrueunconditionally. A real write/delete failure will still produce the “Bindings deleted successfully” path and can leave the profile only partially updated.Suggested fix
- -- Delete all 4 modifier combinations - delete_modifier_binding(false, false, false) - delete_modifier_binding(true, false, false) - delete_modifier_binding(false, true, false) - delete_modifier_binding(false, false, true) + -- Delete all 4 modifier combinations + if not delete_modifier_binding(false, false, false) then + all_success = false + end + if not delete_modifier_binding(true, false, false) then + all_success = false + end + if not delete_modifier_binding(false, true, false) then + all_success = false + end + if not delete_modifier_binding(false, false, true) then + all_success = false + end + + if not all_success then + keyboard_ui.error_message = last_error + return false + end🤖 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 `@lib/keyboard_ui.lua` around lines 584 - 620, The delete flow currently ignores failures from delete_modifier_binding/delete_current_binding and always proceeds to update bindings and return success; change it to propagate errors by checking each delete_modifier_binding(...) return value and, on the first false, set last_error (from delete_modifier_binding) and immediately abort: skip updating global/current_bindings, skip re-merge via merge_bindings(), and return false to the caller; ensure delete_modifier_binding forwards ui_functions.delete_current_binding's error_msg into last_error so the calling function can return that failure instead of unconditionally returning true.
🤖 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 `@jobbinds.lua`:
- Around line 178-191: The profile loader (load_profile) currently builds
last_profile_keys only from the job-specific file, so global bindings loaded via
'/exec JobBinds.txt' are not tracked and thus never unbound; update load_profile
to also parse the global bindings (the same global_path/JobBinds.txt used in the
pcall) and include those keys in the set used to populate last_profile_keys, or
alternatively add a separate last_global_keys and ensure the unload routine
removes keys from both last_profile_keys and last_global_keys; make sure the
code that queues '/exec JobBinds.txt' and the logic that constructs
last_profile_keys (and the unload sequence) reference the same
key-identification logic so global binds get tracked and removed when changed.
In `@lib/keyboard_ui.lua`:
- Around line 101-124: merge_bindings currently only blocks job bindings that
match key+modifier, but the codebase treats global ownership as whole-key
(ignoring modifiers) and save_current_binding never removes opposite-scope rows,
so toggling to global leaves stale job entries; update merge_bindings to compare
only the normalized key (e.g., binding.key:upper()) when filtering
current_bindings against global keys so any job binding with the same key is
suppressed regardless of modifiers, and update save_current_binding to, when
marking a binding as global (or removing global), remove any opposite-scope
bindings that share the same normalized key (use the same key normalization
logic as has_global_binding_on_key()); ensure the change is applied consistently
wherever key exclusivity is enforced (merge_bindings, has_global_binding_on_key,
and save_current_binding) so global bindings are exclusive at the key level.
In `@lib/log.lua`:
- Line 6: Wrap the require('chat') call in a protected call (pcall) and handle a
failed load by setting a safe fallback for the chat symbol (e.g., a stub table
with no-op methods like sendMessage) so that the logging functions in this
module won't error if chat is nil; update lib/log.lua to replace the direct
local chat = require('chat') with a pcall-based require and add a
nil-check/fallback for chat used by the module's logging functions.
---
Outside diff comments:
In `@lib/keyboard_ui.lua`:
- Around line 584-620: The delete flow currently ignores failures from
delete_modifier_binding/delete_current_binding and always proceeds to update
bindings and return success; change it to propagate errors by checking each
delete_modifier_binding(...) return value and, on the first false, set
last_error (from delete_modifier_binding) and immediately abort: skip updating
global/current_bindings, skip re-merge via merge_bindings(), and return false to
the caller; ensure delete_modifier_binding forwards
ui_functions.delete_current_binding's error_msg into last_error so the calling
function can return that failure instead of unconditionally returning true.
🪄 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: bb29e849-7050-43c9-b0b9-ce36f5bbd0c4
📒 Files selected for processing (8)
README.mdjobbinds.lualib/blocked_keybinds.lualib/keyboard_ui.lualib/log.lualib/modifiers.lualib/ui_functions.lualib/vk_codes.lua
Comment on lines
+178
to
191
| -- Then load global bindings (JobBinds.txt) to override job-specific ones | ||
| local global_path = string.format('%s/scripts/JobBinds.txt', AshitaCore:GetInstallPath()) | ||
| local global_file = io.open(global_path, "r") | ||
| if global_file then | ||
| global_file:close() | ||
| debugf('Loading global bindings from JobBinds.txt (overriding job-specific)') | ||
| pcall(function() | ||
| AshitaCore:GetChatManager():QueueCommand(-1, '/exec JobBinds.txt') | ||
| end) | ||
| else | ||
| debugf('No global bindings file found at: %s', global_path) | ||
| end | ||
|
|
||
| -- Update the keyboard UI with the current profile |
There was a problem hiding this comment.
Track global keys in the unload set too.
load_profile() now applies JobBinds.txt, but last_profile_keys is still rebuilt from the job-specific file only. When a global bind is removed from JobBinds.txt, the next job switch never unbinds that old key before reloading, so stale global binds can stick around indefinitely. Include the global file’s keys in the tracked unload list, or keep a separate last_global_keys and unload both.
Also applies to: 201-206
🤖 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 `@jobbinds.lua` around lines 178 - 191, The profile loader (load_profile)
currently builds last_profile_keys only from the job-specific file, so global
bindings loaded via '/exec JobBinds.txt' are not tracked and thus never unbound;
update load_profile to also parse the global bindings (the same
global_path/JobBinds.txt used in the pcall) and include those keys in the set
used to populate last_profile_keys, or alternatively add a separate
last_global_keys and ensure the unload routine removes keys from both
last_profile_keys and last_global_keys; make sure the code that queues '/exec
JobBinds.txt' and the logic that constructs last_profile_keys (and the unload
sequence) reference the same key-identification logic so global binds get
tracked and removed when changed.
Comment on lines
+101
to
+124
| -- Helper: Merge global and job-specific bindings (global overrides job-specific) | ||
| local function merge_bindings() | ||
| local merged = {}; | ||
| local added_keys = {}; -- Track key+modifier combinations | ||
|
|
||
| -- Add global bindings first (they take precedence) | ||
| for _, binding in ipairs(global_bindings) do | ||
| local key_id = binding.key:upper() .. '|' .. (binding.modifiers or ''); | ||
| binding.is_global = true; -- Ensure global flag is set | ||
| merged[#merged + 1] = binding; | ||
| added_keys[key_id] = true; | ||
| end | ||
|
|
||
| -- Add job-specific bindings that don't conflict with global | ||
| for _, binding in ipairs(current_bindings) do | ||
| local key_id = binding.key:upper() .. '|' .. (binding.modifiers or ''); | ||
| if not added_keys[key_id] then | ||
| binding.is_global = false; -- Explicitly ensure job-specific bindings are NOT global | ||
| merged[#merged + 1] = binding; | ||
| added_keys[key_id] = true; | ||
| end | ||
| end | ||
|
|
||
| return merged; |
There was a problem hiding this comment.
Enforce key-level exclusivity consistently for global bindings.
has_global_binding_on_key() and the save guard treat global ownership as a whole-key rule, but merge_bindings() only suppresses job bindings for the same key+modifier, and save_current_binding() never removes the same key from the opposite scope. Toggling an existing job binding to global therefore leaves stale job entries behind, and older/manual mixed files still leak job rows into combined_bindings. That breaks the “no job-specific bindings on globally-bound keys” contract and can make old job binds reappear after the global one is removed.
Also applies to: 487-569
🤖 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 `@lib/keyboard_ui.lua` around lines 101 - 124, merge_bindings currently only
blocks job bindings that match key+modifier, but the codebase treats global
ownership as whole-key (ignoring modifiers) and save_current_binding never
removes opposite-scope rows, so toggling to global leaves stale job entries;
update merge_bindings to compare only the normalized key (e.g.,
binding.key:upper()) when filtering current_bindings against global keys so any
job binding with the same key is suppressed regardless of modifiers, and update
save_current_binding to, when marking a binding as global (or removing global),
remove any opposite-scope bindings that share the same normalized key (use the
same key normalization logic as has_global_binding_on_key()); ensure the change
is applied consistently wherever key exclusivity is enforced (merge_bindings,
has_global_binding_on_key, and save_current_binding) so global bindings are
exclusive at the key level.
| * Centralizes printf/warnf/errorf/debugf and the debug_mode flag. | ||
| --]] | ||
|
|
||
| local chat = require('chat') |
There was a problem hiding this comment.
Consider adding error handling for the chat module require.
If the chat module fails to load, all logging functions will fail with unhelpful errors. Consider wrapping the require in pcall or adding a nil check.
🛡️ Proposed fix to add error handling
-local chat = require('chat')
+local ok, chat = pcall(require, 'chat')
+if not ok then
+ error('Failed to load chat module: ' .. tostring(chat))
+end📝 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.
Suggested change
| local chat = require('chat') | |
| local ok, chat = pcall(require, 'chat') | |
| if not ok then | |
| error('Failed to load chat module: ' .. tostring(chat)) | |
| end |
🤖 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 `@lib/log.lua` at line 6, Wrap the require('chat') call in a protected call
(pcall) and handle a failed load by setting a safe fallback for the chat symbol
(e.g., a stub table with no-op methods like sendMessage) so that the logging
functions in this module won't error if chat is nil; update lib/log.lua to
replace the direct local chat = require('chat') with a pcall-based require and
add a nil-check/fallback for chat used by the module's logging functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
JobBinds.txtscript file