Conversation
There was a problem hiding this comment.
Pull request overview
プロファイルを「名前」ではなく安定した id で識別するように移行し、GUI 上でプロファイル名をリネーム可能にする PR です(既存の profiles.json には起動時に id を自動付与して移行)。
Changes:
Profileにidを追加し、ロード時に欠損/重複idを自動補完(移行)する処理を追加- 適用/削除/連動グループなどの参照を
nameベースからidベースに変更 - DataGrid 上でプロファイル名の編集(LostFocus で確定)とリネーム処理、ならびに Core 側のテストを追加
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/WindowController.Core/WindowController.Core.csproj | テストから internal を参照できるよう InternalsVisibleTo を追加 |
| src/WindowController.Core/ProfileStore.cs | id 移行・FindById/DeleteProfileById/リネームとユニーク名解決を追加 |
| src/WindowController.Core/Models/Profile.cs | JSON スキーマに id フィールドを追加 |
| src/WindowController.Core.Tests/ProfileStoreTests.cs | id 移行・FindById・削除・リネーム・ユニーク名解決のテストを追加 |
| src/WindowController.App/ViewModels/MainViewModel.cs | UI 側を id 参照に切替、DataGrid 名称編集→リネーム連携を追加 |
| src/WindowController.App/SyncManager.cs | sync グループのキーを profileId に変更し、ログ用の名前マップを導入 |
| src/WindowController.App/MainWindow.xaml | プロファイル名列を編集可能に変更(LostFocus で更新) |
| README.md | id 追加とリネーム操作の説明を追記 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var newName = pi.Name?.Trim(); | ||
| if (string.IsNullOrEmpty(newName)) | ||
| { | ||
| // Revert to current stored name | ||
| var current = _store.FindById(pi.Id); | ||
| if (current != null) | ||
| pi.Name = current.Name; |
There was a problem hiding this comment.
Setting pi.Name = current.Name inside the PropertyChanged handler will trigger PropertyChanged again and re-enter OnProfileNameChanged, causing an extra rename/save/timestamp update (and potentially repeated status updates). Consider suppressing re-entrancy (e.g., a guard flag) or temporarily detaching the handler while reverting the value.
| var newName = pi.Name?.Trim(); | |
| if (string.IsNullOrEmpty(newName)) | |
| { | |
| // Revert to current stored name | |
| var current = _store.FindById(pi.Id); | |
| if (current != null) | |
| pi.Name = current.Name; | |
| // Get the current profile from the store | |
| var current = _store.FindById(pi.Id); | |
| if (current == null) | |
| { | |
| StatusText = "プロファイルが見つかりません。"; | |
| return; | |
| } | |
| // If the name has not actually changed compared to the store, ignore. | |
| if (string.Equals(current.Name, pi.Name)) | |
| { | |
| return; | |
| } | |
| var newName = pi.Name?.Trim(); | |
| if (string.IsNullOrEmpty(newName)) | |
| { | |
| // Revert to current stored name | |
| pi.Name = current.Name; |
| // If the name was adjusted due to conflict, update the UI | ||
| if (finalName != newName) | ||
| pi.Name = finalName; | ||
|
|
||
| _syncManager.ScheduleRebuild(); | ||
| StatusText = $"名前を変更しました: {finalName}"; |
There was a problem hiding this comment.
When finalName != newName, assigning pi.Name = finalName will re-trigger OnProfileNameChanged and call _store.RenameProfile a second time, resulting in duplicate saves and timestamp changes. Add a re-entrancy guard or a check against the store's current name before calling rename/setting the property.
| // profileId -> set of hwnds | ||
| private Dictionary<string, HashSet<nint>> _syncGroups = new(); | ||
| // profileId -> display name (for logging) | ||
| private Dictionary<string, string> _profileNames = new(); | ||
| private Dictionary<nint, int> _lastMinMaxByHwnd = new(); |
There was a problem hiding this comment.
When sync is disabled in UpdateHooksIfNeeded, _syncGroups and _lastMinMaxByHwnd are cleared but _profileNames is left as-is. This can leave stale name mappings around longer than needed; consider clearing _profileNames alongside _syncGroups for consistency.
| // Check if any profile received a new Id during normalization (migration) | ||
| bool needsSave = json != null && normalized.Profiles.Any(p => | ||
| !json.Contains($"\"id\": \"{p.Id}\"", StringComparison.Ordinal)); | ||
|
|
||
| Data = normalized; | ||
|
|
||
| if (needsSave) | ||
| Save(); |
There was a problem hiding this comment.
needsSave detection is based on a raw string json.Contains("\"id\": \"{p.Id}\""), which is sensitive to whitespace/formatting (e.g. older/minified JSON with "id":"..." will cause a save on every startup) and can also be fooled by unrelated occurrences. Consider detecting migration from the parsed object (missing/duplicate Ids) before normalization, or compare pre/post normalization at the object level / via re-serialization with the same options.
| public string? RenameProfile(string profileId, string desiredName) | ||
| { | ||
| var profile = FindById(profileId); | ||
| if (profile == null) return null; | ||
|
|
||
| var finalName = ResolveUniqueName(desiredName, profileId); | ||
| profile.Name = finalName; | ||
| profile.UpdatedAt = DateTime.Now.ToString("yyyy-MM-dd'T'HH:mm:ss"); | ||
| Save(); | ||
| return finalName; |
There was a problem hiding this comment.
RenameProfile updates UpdatedAt and writes to disk even when the resolved name equals the current name. This will unnecessarily touch profiles.json and mutate timestamps (especially since the UI can trigger rename callbacks even for programmatic reverts). Consider early-returning when finalName == profile.Name (after normalization/trim) to avoid redundant saves.
| foreach (var p in root.Profiles) | ||
| { | ||
| if (string.IsNullOrEmpty(p.Id) || !seenIds.Add(p.Id)) | ||
| { | ||
| // Generate a new unique Id (duplicate Ids are also re-assigned) | ||
| p.Id = Guid.NewGuid().ToString("D"); | ||
| seenIds.Add(p.Id); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
|
|
||
| public void Dispose() | ||
| { | ||
| try { Directory.Delete(_tempDir, true); } catch { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
| try { Directory.Delete(_tempDir, true); } catch { } | |
| try | |
| { | |
| Directory.Delete(_tempDir, true); | |
| } | |
| catch (Exception ex) | |
| { | |
| _log.Warning(ex, "Failed to delete temporary test directory {TempDir}", _tempDir); | |
| } |
| { | ||
| [ObservableProperty] private bool _syncMinMax; | ||
| public string Name { get; init; } = ""; | ||
| [ObservableProperty] private string _name = ""; |
There was a problem hiding this comment.
Field '_name' can be 'readonly'.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…, optimize saves Co-authored-by: hu-ja-ja <107774991+hu-ja-ja@users.noreply.github.com>
Co-authored-by: hu-ja-ja <107774991+hu-ja-ja@users.noreply.github.com>
Fix re-entrancy and redundant saves in profile rename handlers
No description provided.