[Keyboard Manager] WinUI 3 Keyboard Manager #44305
Conversation
* Implement a dev version ui which is copy of current WinUI2 UX * Implement WinUI3 dropdown and P/Invoke the key list from existing C++ Library * Update Dllimport path
* Set up static new UX for Keyboard Manager Editor * Improve the formatting and performance * fix spelling
* load URL remapping * Save Remapping Config and Data * update mock data for testing * fix app mapping * update xaml
* add press button * add right press button * add right press button by original button * change toggle button position * fix type error * merge * add link * brush blue in new keys * remove unuse annotations * remove unuse keyvisual
* Make Disable toggle button functional in Shortcuts page * Clean up main window and aggregate Constants * Update key remapping naming * Fix the key visual stretch out issue when the shortcut contains too many keys * Refactor code structure * fix wrapping in dialog * update naming * Implement Keyboard hook to get the user input of remappings * Implement Reset for InputControl to make sure InputControl can have correct status for various dialog
…oard Hook and Input Validator (#39081) * Add delete remapping function * Fix the problem that Keyboard Hook is not cleaned properly when the toggle button lost focus (user clicked other checkbox or switched to another app) * Make sure original toggle button is checked for InputControl * Disable app setting for single key remapping * Update KBM Editor UI Signing * Ensuring Modifier Keys Don't Repeat * Make sure the shortcut takes at most 4 modifier keys and show notification to the user * Show teaching tip for reserved illegal shortcuts - Win+L and Ctrl+Alt+Delete * Add validation to check if the user is trying to remap a key or shortcut that's already mapped for the same app context * Add validation to make sure key/shortcut cannot be mapped to itself * Add validation to check the original/new keys are not empty. Application name must be entered if the specific app checkbox is checked. * Add validation to check shortcuts must contain at least one action key in addition to modifier keys * Add validation to warn orphaned key to user * Unify and consolidate the validation code * Refactor the validation mechanism * Delete the existing remapping when user modified the content
…e-behind functionalities (#39888) <!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request > [Target feature branch ``feature/KeyboardManagerWinUI3``] Refactor the New Keyboard Manager code to generalize functions (like Keyboard Hook) for use across pages. Implement Code-behind and the Text Remapping Page. - Implement the Text Remapping page - Restructure the Key Remappings code - Implement unified Keyboard Hook for all Pages - Implement Save and Delete for Text Page - Adjust the Text Page list UI - Create Input Control for Text Page - Validate the user input in Text Page and show teaching tip for the validation error - Move Remapping Saving into Helper - Move Remapping Delete into Helper - Don't check for duplicates if the original keys are the same as the remapping being edited - Use InputMode to indicate the active toggle for Original Keys/Remapped Keys - Add Rectangle for all pages      
This reverts commit 0009bbf.
remove ref to programs and url pages
…t/PowerToys into zt/new-kbm-localization
…xt page resource keys
zateutsch
added
the
Product-Keyboard Shortcut Manager
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return ValidationErrorType.NoError; | ||
| } | ||
|
|
||
| // Temporary program shorctut validation |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| return KeyboardManagerInterop.AddShortcutRemap(_configHandle, originalKeys, targetKeys, targetApp, (int)operationType); | ||
| } | ||
|
|
||
| public bool AddShorcutMapping(ShortcutKeyMapping shortcutKeyMapping) |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| try | ||
| { | ||
| _mappingService = new KeyboardMappingService(); | ||
| LoadProgramShrotcuts(); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| Dispose(); | ||
| } | ||
|
|
||
| private void LoadProgramShrotcuts() |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| Elevation = elevationLevel, | ||
| }; | ||
|
|
||
| saved = _mappingService.AddShorcutMapping(shortcutKeyMapping); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| <data name="RemappingsPageOriginalKeysTextBlock.Text" xml:space="preserve"> | ||
| <value>Original key(s)</value> | ||
| </data> | ||
| <data name="RemmapingsPageOrphanedKeysTeachingTip.ActionButtonContent" xml:space="preserve"> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| <data name="RemmapingsPageOrphanedKeysTeachingTip.ActionButtonContent" xml:space="preserve"> | ||
| <value>Continue anyway</value> | ||
| </data> | ||
| <data name="RemmapingsPageOrphanedKeysTeachingTip.CloseButtonContent" xml:space="preserve"> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| <data name="RemmapingsPageOrphanedKeysTeachingTip.CloseButtonContent" xml:space="preserve"> | ||
| <value>Cancel</value> | ||
| </data> | ||
| <data name="RemmapingsPageOrphanedKeysTeachingTip.Title" xml:space="preserve"> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| <data name="TextPageInputControlTextContentTextBox.Header" xml:space="preserve"> | ||
| <value>Text to insert</value> | ||
| </data> | ||
| <data name="TextPageDeleteButton.ToolTipSevice.ToolTip" xml:space="preserve"> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
| ProcessStartInfo startInfo = new ProcessStartInfo(path); | ||
| startInfo.UseShellExecute = true; // LOAD BEARING | ||
| startInfo.Arguments = $"{type.ToString(CultureInfo.InvariantCulture)} {Environment.ProcessId}"; | ||
| System.Environment.SetEnvironmentVariable("MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY", null); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling Error
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (6)Remmapings These words are not needed and should be removedCLITo CVS Notavailable toolgood UninitializesTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the git@github.com:microsoft/PowerToys.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/c635c2f3f714eec2fcf27b643a1919b9a811ef2e/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/20256621830/attempts/1' &&
git commit -m 'Update check-spelling metadata'If the flagged items are 🤯 false positivesIf items relate to a ...
|
Changes SummaryThis PR introduces a new WinUI 3-based Keyboard Manager Editor UI application with full CRUD functionality for keyboard remappings, text shortcuts, program shortcuts, and URL shortcuts. It replaces a placeholder implementation with a complete C#/XAML UI that interoperates with C++ backend through P/Invoke, implementing a modern navigation-based interface with comprehensive validation. Type: feature Components Affected: KeyboardManagerEditorUI (new WinUI3 application), KeyboardManagerEditorLibraryWrapper (C++ interop layer), Settings.UI (KeyboardManagerViewModel), Build configuration (Directory.Build.props, Directory.Packages.props), ESRP signing pipeline Architecture Impact
Risk Areas: Native interop (P/Invoke) memory management - strings allocated in C++ must be freed properly via FreeString, Keyboard hook lifecycle management - improper cleanup could leave hooks active, Race conditions in keyboard hook activation/deactivation during window focus changes, Resource leaks from IDisposable implementations (KeyboardMappingService, KeyboardHookHelper), Validation logic correctness for illegal shortcuts (Win+L, Ctrl+Alt+Del), Process startup changes in KeyboardManagerViewModel (UseShellExecute=true, environment variable clearing) Suggestions
Full review in progress... | Powered by diffray |
| /// </summary> | ||
| public App() | ||
| { | ||
| global::System.Diagnostics.Debugger.Break(); |
There was a problem hiding this comment.
🔴 CRITICAL - Debug code left in production
Category: bug
Description:
Debugger.Break() is called unconditionally in the App constructor, which will break into the debugger on every application launch. This should never be in production code.
Suggestion:
Remove the Debugger.Break() call or wrap it in #if DEBUG preprocessor directive:
#if DEBUG
global::System.Diagnostics.Debugger.Break();
#endif
Confidence: 100%
Rule: cs_remove_debug_code_from_production
| private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args) | ||
| { | ||
| // if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error)) | ||
| // { | ||
| // ValidationTip.Title = error.Title; | ||
| // ValidationTip.Subtitle = error.Message; | ||
| // ValidationTip.IsOpen = true; | ||
| // args.Cancel = true; | ||
| // } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Empty validation error handler silently fails
Category: bug
Description:
The ShowValidationError method is completely commented out (lines 242-248), meaning validation errors are silently ignored. The dialog provides no feedback when validation fails.
Suggestion:
Implement the validation error display logic or remove the commented code:
private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args)
{
if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error))
{
ValidationTip.Title = error.Title;
ValidationTip.Subtitle = error.Message;
ValidationTip.IsOpen = true;
args.Cancel = true;
}
}Confidence: 100%
Rule: cs_avoid_empty_catch_blocks
| private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args) | ||
| { | ||
| // if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error)) | ||
| // { | ||
| // ValidationTip.Title = error.Title; | ||
| // ValidationTip.Subtitle = error.Message; | ||
| // ValidationTip.IsOpen = true; | ||
| // args.Cancel = true; | ||
| // } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Empty validation error handler silently fails
Category: bug
Description:
The ShowValidationError method is completely commented out (lines 213-219), meaning validation errors are silently ignored. The dialog provides no feedback when validation fails.
Suggestion:
Implement the validation error display logic or remove the commented code:
private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args)
{
if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error))
{
ValidationTip.Title = error.Title;
ValidationTip.Subtitle = error.Message;
ValidationTip.IsOpen = true;
args.Cancel = true;
}
}Confidence: 100%
Rule: cs_avoid_empty_catch_blocks
| try | ||
| { | ||
| _mappingService = new KeyboardMappingService(); | ||
| LoadProgramShrotcuts(); |
There was a problem hiding this comment.
🔵 LOW - Typo in method name: LoadProgramShrotcuts
Category: quality
Description:
The method 'LoadProgramShrotcuts' appears to be misspelled. It should be 'LoadProgramShortcuts'.
Suggestion:
Rename the method from 'LoadProgramShrotcuts' to 'LoadProgramShortcuts'. Update all references.
Confidence: 100%
Rule: cs_use_pascalcase_for_type_names
| return KeyboardManagerInterop.AddShortcutRemap(_configHandle, originalKeys, targetKeys, targetApp, (int)operationType); | ||
| } | ||
|
|
||
| public bool AddShorcutMapping(ShortcutKeyMapping shortcutKeyMapping) |
There was a problem hiding this comment.
🔵 LOW - Typo in method name
Category: quality
Description:
Method name 'AddShorcutMapping' contains a typo - 'Shorcut' should be 'Shortcut'.
Suggestion:
Rename the method to 'AddShortcutMapping' for consistency.
Confidence: 100%
Rule: qual_semantic_naming_csharp
| private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args) | ||
| { | ||
| // if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error)) | ||
| // { | ||
| // ValidationTip.Title = error.Title; | ||
| // ValidationTip.Subtitle = error.Message; | ||
| // ValidationTip.IsOpen = true; | ||
| // args.Cancel = true; | ||
| // } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - ShowValidationError implementation is commented out
Category: bug
Description:
The entire ShowValidationError method body is commented out, meaning validation errors are silently ignored. Users won't see any feedback when validation fails.
Suggestion:
Implement the validation error display or add a clear comment explaining why validation is disabled. Compare with Text.xaml.cs which has a working implementation.
Confidence: 95%
Rule: quality_commented_code
| // if (isAppSpecific && !string.IsNullOrEmpty(appName)) | ||
| // { | ||
| // saved = _mappingService.AddShortcutMapping(originalKeysString, programPath, appName, ShortcutOperationType.RemapText); | ||
| // } | ||
| // else | ||
| // { |
There was a problem hiding this comment.
🟡 MEDIUM - Commented out code block without explanation
Category: quality
Description:
6 consecutive lines of commented-out code containing if-else statements with no accompanying TODO or explanation. Same pattern as in Programs.xaml.cs.
Suggestion:
Either remove this commented code and rely on git history for recovery, or add a clear TODO/NOTE explaining why it's temporarily disabled.
Confidence: 90%
Rule: quality_commented_code
| private void ShowValidationError(ValidationErrorType errorType, ContentDialogButtonClickEventArgs args) | ||
| { | ||
| // if (ValidationHelper.ValidationMessages.TryGetValue(errorType, out (string Title, string Message) error)) | ||
| // { | ||
| // ValidationTip.Title = error.Title; | ||
| // ValidationTip.Subtitle = error.Message; | ||
| // ValidationTip.IsOpen = true; | ||
| // args.Cancel = true; | ||
| // } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - ShowValidationError implementation is commented out
Category: bug
Description:
The entire ShowValidationError method body is commented out, meaning validation errors are silently ignored. Users won't see any feedback when validation fails.
Suggestion:
Implement the validation error display or add a clear comment explaining why validation is disabled. Compare with Text.xaml.cs which has a working implementation.
Confidence: 95%
Rule: quality_commented_code
| else | ||
| { | ||
| string originalKeysString = string.Join(";", originalKeys.Select( | ||
| k => mappingService.GetKeyCodeFromName(k).ToString(CultureInfo.InvariantCulture))); | ||
|
|
||
| // Don't check for duplicates if the original keys are the same as the remapping being edited | ||
| bool isEditingExistingRemapping = false; | ||
| if (isEditMode && editingRemapping != null) | ||
| { | ||
| string editingOriginalKeysString = string.Join(";", editingRemapping.OriginalKeys.Select(k => | ||
| mappingService.GetKeyCodeFromName(k).ToString(CultureInfo.InvariantCulture))); | ||
|
|
||
| if (KeyboardManagerInterop.AreShortcutsEqual(originalKeysString, editingOriginalKeysString)) | ||
| { | ||
| isEditingExistingRemapping = true; | ||
| } | ||
| } | ||
|
|
||
| // Check if the shortcut is already remapped in the same app context | ||
| foreach (var mapping in mappingService.GetShortcutMappingsByType(ShortcutOperationType.RemapShortcut)) | ||
| { | ||
| if (KeyboardManagerInterop.AreShortcutsEqual(originalKeysString, mapping.OriginalKeys)) | ||
| { | ||
| // If both are global (all apps) | ||
| if (!isAppSpecific && string.IsNullOrEmpty(mapping.TargetApp)) | ||
| { | ||
| // Skip if the remapping is the same as the one being edited | ||
| if (editingRemapping != null && editingRemapping.OriginalKeys.Count > 1 && editingRemapping.IsAllApps && isEditingExistingRemapping) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| // If both are for the same specific app | ||
| else if (isAppSpecific && !string.IsNullOrEmpty(mapping.TargetApp) | ||
| && string.Equals(mapping.TargetApp, appName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| // Skip if the remapping is the same as the one being edited | ||
| if (editingRemapping != null && editingRemapping.OriginalKeys.Count > 1 && !editingRemapping.IsAllApps && | ||
| string.Equals(editingRemapping.AppName, appName, StringComparison.OrdinalIgnoreCase) && isEditingExistingRemapping) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Deeply nested if-else blocks instead of guard clauses
Category: quality
Description:
The shortcut remapping validation logic (else block) contains 4-5 levels of nesting with complex conditional logic checking for duplicate mappings.
Suggestion:
Extract the duplicate checking logic into separate helper methods. Use early returns for edge cases. Consider extracting 'CheckGlobalShortcutDuplicate' and 'CheckAppSpecificShortcutDuplicate' methods.
Confidence: 70%
Rule: quality_guard_clauses
| public static ValidationErrorType ValidateKeyMapping( | ||
| List<string> originalKeys, | ||
| List<string> remappedKeys, | ||
| bool isAppSpecific, | ||
| string appName, | ||
| KeyboardMappingService mappingService, | ||
| bool isEditMode = false, | ||
| Remapping? editingRemapping = null) | ||
| { | ||
| // Check if original keys are empty | ||
| if (originalKeys == null || originalKeys.Count == 0) | ||
| { | ||
| return ValidationErrorType.EmptyOriginalKeys; | ||
| } | ||
|
|
||
| // Check if remapped keys are empty | ||
| if (remappedKeys == null || remappedKeys.Count == 0) | ||
| { | ||
| return ValidationErrorType.EmptyRemappedKeys; | ||
| } | ||
|
|
||
| // Check if shortcut contains only modifier keys | ||
| if ((originalKeys.Count > 1 && ContainsOnlyModifierKeys(originalKeys)) || | ||
| (remappedKeys.Count > 1 && ContainsOnlyModifierKeys(remappedKeys))) | ||
| { | ||
| return ValidationErrorType.ModifierOnly; | ||
| } | ||
|
|
||
| // Check if app specific is checked but no app name is provided | ||
| if (isAppSpecific && string.IsNullOrWhiteSpace(appName)) | ||
| { | ||
| return ValidationErrorType.EmptyAppName; | ||
| } | ||
|
|
||
| // Check if this is a shortcut (multiple keys) and if it's an illegal combination | ||
| if (originalKeys.Count > 1) | ||
| { | ||
| string shortcutKeysString = string.Join(";", originalKeys.Select(k => mappingService.GetKeyCodeFromName(k).ToString(CultureInfo.InvariantCulture))); | ||
|
|
||
| if (KeyboardManagerInterop.IsShortcutIllegal(shortcutKeysString)) | ||
| { | ||
| return ValidationErrorType.IllegalShortcut; | ||
| } | ||
| } | ||
|
|
||
| // Check for duplicate mappings | ||
| if (IsDuplicateMapping(originalKeys, isAppSpecific, appName, mappingService, isEditMode, editingRemapping)) | ||
| { | ||
| return ValidationErrorType.DuplicateMapping; | ||
| } | ||
|
|
||
| // Check for self-mapping | ||
| if (IsSelfMapping(originalKeys, remappedKeys, mappingService)) | ||
| { | ||
| return ValidationErrorType.SelfMapping; | ||
| } | ||
|
|
||
| return ValidationErrorType.NoError; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Method with 7 parameters - consider parameter object
Category: quality
Description:
The method 'ValidateKeyMapping' has 7 parameters (5 required + 2 optional). This makes call sites verbose and harder to maintain.
Suggestion:
Create a parameter object like 'KeyMappingValidationContext' that groups the logically related parameters (originalKeys, remappedKeys, isAppSpecific, appName, mappingService, isEditMode, editingRemapping).
Confidence: 65%
Rule: cs_group_related_parameters_in_objects
Review Summary
Validated 76 issues: 33 kept, 43 filtered Issues Found: 33See 33 individual line comment(s) for details. 📊 16 unique issue type(s) across 33 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Debug code left in productionCategory: bug File: Description: Debugger.Break() is called unconditionally in the App constructor, which will break into the debugger on every application launch. This should never be in production code. Suggestion: Remove the Debugger.Break() call or wrap it in #if DEBUG preprocessor directive: Confidence: 100% Rule: 🟠 HIGH - Empty validation error handler silently fails (2 occurrences)Category: bug 📍 View all locations
private void Sh... | 100% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/URLs.xaml.cs:211-220` | The ShowValidationError method is completely commented out (lines 213-219), meaning validation error... | Implement the validation error display logic or remove the commented code:
```csharp
private void Sh... | 100% |
</details>
**Rule:** `cs_avoid_empty_catch_blocks`
---
#### 🟠 HIGH - Memory leak: Raw pointer allocations without cleanup path (5 occurrences)
**Category:** bug
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:50-88` | In GetSingleKeyRemap(), AllocateAndCopyString() allocates memory for mapping->targetKey in multiple ... | Document that callers MUST call FreeString(mapping->targetKey) after use. Consider adding a cleanup ... | 75% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:96-113` | In GetSingleKeyToTextRemap(), AllocateAndCopyString() allocates memory for mapping->targetText. If t... | Document that callers MUST call FreeString(mapping->targetText) after use. Add null check for mappin... | 75% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:214-375` | In GetShortcutRemapByType(), the function allocates up to 7 raw pointers per call using AllocateAndC... | Provide a helper function like 'FreeShortcutMapping(ShortcutMapping* mapping)' that frees all pointe... | 75% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:390-476` | In GetShortcutRemap(), the function allocates up to 7 raw pointers per call using AllocateAndCopyStr... | Document that callers MUST call FreeString() on all allocated pointers. Provide a helper function to... | 75% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:16-24` | CreateMappingConfiguration() uses 'new MappingConfiguration()' which could throw std::bad_alloc. In ... | Wrap the allocation in a try-catch block and return nullptr on allocation failure, or use std::nothr... | 60% |
</details>
**Rule:** `cpp_raw_pointer_delete`
---
#### 🟠 HIGH - Placeholder implementation returns hardcoded value
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ResourceLoaderInstance.cs:17`
**Description:** The GetString method always returns 'Hello' instead of actually loading the resource, indicating incomplete implementation that will cause all localized strings to display 'Hello'.
**Suggestion:** Implement the method properly: 'internal static string GetString(string resourceId) => ResourceLoader.GetString(resourceId);'
**Confidence:** 98%
**Rule:** `cs_extract_duplicated_business_logic`
---
#### 🟡 MEDIUM - Missing null pointer check for output parameter (5 occurrences)
**Category:** bug
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:50-88` | In GetSingleKeyRemap(), the 'mapping' parameter is dereferenced without checking if it's null. If a ... | Add a null check at the beginning: if (mapping == nullptr) { return false; } | 85% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:96-113` | In GetSingleKeyToTextRemap(), the 'mapping' parameter is dereferenced without checking if it's null.... | Add a null check at the beginning: if (mapping == nullptr) { return false; } | 85% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:214-375` | In GetShortcutRemapByType(), the 'mapping' parameter is dereferenced without checking if it's null. ... | Add a null check at the beginning: if (mapping == nullptr) { return false; } | 85% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:390-476` | In GetShortcutRemap(), the 'mapping' parameter is dereferenced without checking if it's null. This w... | Add a null check at the beginning: if (mapping == nullptr) { return false; } | 85% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:515-575` | In AddShortcutRemap(), the function doesn't check if 'originalKeys', 'targetKeys', or 'appPathOrUri'... | Add null checks: if (!originalKeys \|\| !targetKeys) { return false; }. Also check appPathOrUri when... | 80% |
</details>
**Rule:** `cpp_use_after_free`
---
#### 🟡 MEDIUM - Potential NullReferenceException in event handler
**Category:** bug
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/Remappings.xaml.cs:243`
**Description:** OrphanedKeysTeachingTip_ActionButtonClick uses null-forgiving operator on _mappingService without null check. While unlikely, _mappingService could be null if Dispose() was called.
**Suggestion:** Add a null check for _mappingService at the beginning of the method: if (_mappingService == null) { return; }
**Confidence:** 65%
**Rule:** `bug_null_pointer_csharp`
---
#### 🟡 MEDIUM - Magic numbers for key count limits
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs:94-95`
**Description:** The numeric literals 4 and 5 represent maximum modifier key count and total key count but are hardcoded without named constants. The comment on line 93 explains the meaning but named constants would be clearer.
**Suggestion:** Extract these magic numbers into named constants: 'private const int MaxModifierKeys = 4;' and 'private const int MaxTotalKeysInShortcut = 5;'
**Confidence:** 75%
**Rule:** `cs_replace_magic_numbers_with_named_constan`
---
#### 🟡 MEDIUM - Magic numbers for VirtualKey values
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/KeyVisual/KeyVisual.xaml.cs:120-122`
**Description:** The numeric literals 160 and 161 represent Left Shift and Right Shift key codes. While there are inline comments, named constants would improve maintainability.
**Suggestion:** Define named constants: 'private const int VK_LSHIFT = 160;' and 'private const int VK_RSHIFT = 161;' or use Windows.System.VirtualKey enum values if available.
**Confidence:** 70%
**Rule:** `qual_magic_numbers_csharp`
---
#### 🟡 MEDIUM - O(N) linear search in duplicate detection (5 occurrences)
**Category:** performance
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:177-191` | IsDuplicateMapping performs a foreach loop over all single key mappings to check for duplicates. Cal... | Pre-index single key mappings using a HashSet<int> or Dictionary<int, KeyMapping> by originalKey. Ch... | 65% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:214-244` | IsDuplicateMapping performs foreach over all shortcut mappings, calling KeyboardManagerInterop.AreSh... | Create an index using normalized shortcut strings as keys: Dictionary<string, ShortcutKeyMapping>. C... | 70% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:115-212` | Contains nested loops iterating over osLevelShortcutReMap and appSpecificShortcutReMap to count by o... | Pre-filter and cache the counts by operation type when mappings are loaded or modified, rather than ... | 70% |
| `src/modules/keyboardmanager/KeyboardManagerEditorLibraryWrapper/KeyboardManagerEditorLibraryWrapper.cpp:214-309` | Iterates through all mappings with nested loops, filtering by operation type and building a vector. ... | Maintain filtered collections by operation type and invalidate cache when mappings change. | 70% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:295-312` | IsKeyOrphaned iterates over all single key and shortcut mappings to check if a key is used as a targ... | Pre-build a HashSet<int> of all keys that are used as targets in any mapping. Check orphaned status ... | 65% |
</details>
**Rule:** `perf_quadratic_loops`
---
#### 🟡 MEDIUM - Commented out code block without explanation (4 occurrences)
**Category:** quality
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/Programs.xaml.cs:168-173` | 6 consecutive lines of commented-out code containing if-else statements with no accompanying TODO or... | Either remove this commented code and rely on git history for recovery, or add a clear TODO/NOTE exp... | 90% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/Programs.xaml.cs:240-249` | The entire ShowValidationError method body is commented out, meaning validation errors are silently ... | Implement the validation error display or add a clear comment explaining why validation is disabled.... | 95% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/URLs.xaml.cs:182-187` | 6 consecutive lines of commented-out code containing if-else statements with no accompanying TODO or... | Either remove this commented code and rely on git history for recovery, or add a clear TODO/NOTE exp... | 90% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/URLs.xaml.cs:211-220` | The entire ShowValidationError method body is commented out, meaning validation errors are silently ... | Implement the validation error display or add a clear comment explaining why validation is disabled.... | 95% |
</details>
**Rule:** `quality_commented_code`
---
#### 🟡 MEDIUM - Deeply nested if-else blocks instead of guard clauses
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:195-244`
**Description:** The shortcut remapping validation logic (else block) contains 4-5 levels of nesting with complex conditional logic checking for duplicate mappings.
**Suggestion:** Extract the duplicate checking logic into separate helper methods. Use early returns for edge cases. Consider extracting 'CheckGlobalShortcutDuplicate' and 'CheckAppSpecificShortcutDuplicate' methods.
**Confidence:** 70%
**Rule:** `quality_guard_clauses`
---
#### 🟡 MEDIUM - Method with 7 parameters - consider parameter object
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:29-87`
**Description:** The method 'ValidateKeyMapping' has 7 parameters (5 required + 2 optional). This makes call sites verbose and harder to maintain.
**Suggestion:** Create a parameter object like 'KeyMappingValidationContext' that groups the logically related parameters (originalKeys, remappedKeys, isAppSpecific, appName, mappingService, isEditMode, editingRemapping).
**Confidence:** 65%
**Rule:** `cs_group_related_parameters_in_objects`
---
#### 🔵 LOW - Typo in method name: LoadProgramShrotcuts
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/Programs.xaml.cs:48`
**Description:** The method 'LoadProgramShrotcuts' appears to be misspelled. It should be 'LoadProgramShortcuts'.
**Suggestion:** Rename the method from 'LoadProgramShrotcuts' to 'LoadProgramShortcuts'. Update all references.
**Confidence:** 100%
**Rule:** `cs_use_pascalcase_for_type_names`
---
#### 🔵 LOW - Typo in method name
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Interop/KeyboardMappingService.cs:188`
**Description:** Method name 'AddShorcutMapping' contains a typo - 'Shorcut' should be 'Shortcut'.
**Suggestion:** Rename the method to 'AddShortcutMapping' for consistency.
**Confidence:** 100%
**Rule:** `qual_semantic_naming_csharp`
---
#### 🔵 LOW - Missing XML documentation for public static class (2 occurrences)
**Category:** docs
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/ValidationHelper.cs:15` | ValidationHelper lacks XML documentation. Public APIs should be documented for IntelliSense support.... | Add XML documentation: /// <summary>Provides validation methods for keyboard mappings.</summary> | 70% |
| `src/modules/keyboardmanager/KeyboardManagerEditorUI/Interop/KeyboardMappingService.cs:15` | KeyboardMappingService lacks XML documentation. Public APIs should be documented. | Add XML documentation: /// <summary>Provides a managed wrapper for keyboard mapping configuration op... | 70% |
</details>
**Rule:** `cs_xml_documentation_for_public_apis`
---
#### 🔵 LOW - Missing culture specification in ToString
**Category:** quality
**File:** `src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/URLs.xaml.cs:244`
**Description:** Using ToString() without CultureInfo when converting integer key codes to strings in DeleteButton_Click. While integer ToString() is culture-invariant by default in .NET, explicit specification would be consistent with other code in the file.
**Suggestion:** Specify CultureInfo.InvariantCulture for consistency with line 174 and 180:
string originalKeys = string.Join(";", shortcut.Shortcut.Select(k => _mappingService.GetKeyCodeFromName(k).ToString(CultureInfo.InvariantCulture)));
**Confidence:** 62%
**Rule:** `csharp_string_comparison_culture`
---
</details>
<sub>Powered by [diffray](https://diffray.ai?utm_source=github-public) - Multi-Agent Code Review Agent</sub> |
Validation Steps
Running the Project
Option 1: Test via runner
zt/new-kbm-localizationModules/KeyboardManagerEditorUIproject separatelyrunnerprojectOption 2: Test via installer
Pending installer build is successful on this PR,
Validation
For each page (Text, Remappings, Programs, URLs):