Conversation
Migrated large/transient history data (word borders) to sidecar files, reducing memory usage and improving performance. HistoryService now lazily loads/caches histories and manages cleanup of unused files. SettingsService manages large JSON settings as disk files, with migration and caching for thread safety. WebSearchUrlModel updated to use new settings methods. Overall, improves scalability and robustness for history and settings management.
Centralize settings serialization/deserialization in SettingsService, replacing scattered JSON logic in utility classes. Update history export/import to handle all relevant files except text-only/history files. Remove unused JSON options and streamline error handling for settings. Improves maintainability and reliability of user customizations.
Replaced manual JSON handling with AppUtilities.TextGrabSettingsService for loading and saving regex patterns. Improved error handling and code reuse. Updated history and word border info management for better robustness and memory handling. Introduced helper methods to centralize pattern loading. Overall, enhances maintainability and consistency across the app.
Introduce HistoryServiceTests and SettingsServiceTests to verify migration, lazy loading, and persistence logic. Tests cover history file updates, word border JSON migration, regex settings migration, and post-grab check state storage. Improves coverage for file operations and settings management.
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing memory usage and improving persistence of large/cached data by moving several JSON-heavy settings and history payloads out of in-memory/settings-container storage and into file-backed storage.
Changes:
- Introduces
SettingsService“managed JSON settings” persisted tosettings-data/*.jsonwith caching/cloning, and updates callers to use it. - Refactors
HistoryServiceto lazy-load histories, release cached history data after idle time, and moveWordBorderInfoJSON to sidecar files. - Updates release workflow to publish with explicit self-contained flags and adds Azure Trusted Signing steps.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Text-Grab/Views/GrabFrame.xaml.cs | Loads word border info via HistoryService and clears transient images to reduce memory retention. |
| Text-Grab/Views/EditTextWindow.xaml.cs | Resolves history items by ID on click to avoid retaining heavy history instances. |
| Text-Grab/Utilities/ShortcutKeysUtilities.cs | Switches shortcut key set persistence to SettingsService managed JSON storage. |
| Text-Grab/Utilities/SettingsImportExportUtilities.cs | Exports managed JSON settings via SettingsService and exports/imports history sidecar artifacts. |
| Text-Grab/Utilities/PostGrabActionManager.cs | Moves post-grab actions + check state persistence to SettingsService. |
| Text-Grab/Utilities/GrabTemplateExecutor.cs | Loads stored regex patterns via SettingsService. |
| Text-Grab/Utilities/CustomBottomBarUtilities.cs | Moves bottom bar button persistence to SettingsService. |
| Text-Grab/Utilities/AppUtilities.cs | Exposes TextGrabSettingsService for centralized settings persistence. |
| Text-Grab/Services/SettingsService.cs | Implements managed JSON settings persisted to disk with caching and migration from classic settings/container. |
| Text-Grab/Services/HistoryService.cs | Adds lazy loading, cache release timer, sidecar storage for word borders, and transient payload cleanup. |
| Text-Grab/Models/WebSearchUrlModel.cs | Switches web search URL persistence to SettingsService. |
| Text-Grab/Models/HistoryInfo.cs | Makes word-border JSON nullable, adds sidecar filename, and adds transient cleanup helpers. |
| Text-Grab/Controls/RegexManager.xaml.cs | Switches regex pattern load/save to SettingsService. |
| Text-Grab/Controls/FindAndReplaceWindow.xaml.cs | Uses SettingsService to check if a regex pattern is already saved. |
| Text-Grab/App.xaml.cs | Removes eager history loading at startup (history now lazy-loads). |
| Tests/SettingsServiceTests.cs | Adds tests for managed JSON migration/caching and file persistence. |
| Tests/HistoryServiceTests.cs | Adds tests for lazy-loading/releasing history and word-border sidecar migration. |
| .github/workflows/Release.yml | Adds Azure Trusted Signing steps and adjusts publish flags for self-contained/non-self-contained outputs. |
Comments suppressed due to low confidence (1)
Text-Grab/Services/HistoryService.cs:418
LoadHistoryAsynccallsJsonSerializer.Deserializewithout a try/catch. A partially-written or user-edited history JSON file will throwJsonExceptionand can take down history loading. Wrap the deserialize in atry/catch (JsonException)and treat failures as an empty history (optionally logging to Debug).
private static async Task<List<HistoryInfo>> LoadHistoryAsync(string fileName)
{
string rawText = await FileUtilities.GetTextFileAsync($"{fileName}.json", FileStorageKind.WithHistory);
if (string.IsNullOrWhiteSpace(rawText)) return [];
List<HistoryInfo>? tempHistory = JsonSerializer.Deserialize<List<HistoryInfo>>(rawText, HistoryJsonOptions);
Text-Grab/Services/HistoryService.cs
Outdated
| string historyBasePath = await FileUtilities.GetPathToHistory(); | ||
| string wordBorderInfoPath = Path.Combine(historyBasePath, history.WordBorderInfoFileName); | ||
|
|
||
| if (File.Exists(wordBorderInfoPath)) | ||
| { | ||
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | ||
| List<WordBorderInfo>? wordBorderInfos = | ||
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | ||
|
|
||
| return wordBorderInfos ?? []; |
There was a problem hiding this comment.
history.WordBorderInfoFileName comes from persisted history data and is combined directly with the history folder path. If the value is rooted or contains path traversal segments, Path.Combine can escape the history directory and read arbitrary files. Sanitize with Path.GetFileName(...) (and optionally validate the expected suffix) before building the full path.
| string historyBasePath = await FileUtilities.GetPathToHistory(); | |
| string wordBorderInfoPath = Path.Combine(historyBasePath, history.WordBorderInfoFileName); | |
| if (File.Exists(wordBorderInfoPath)) | |
| { | |
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | |
| List<WordBorderInfo>? wordBorderInfos = | |
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | |
| return wordBorderInfos ?? []; | |
| // Sanitize the persisted file name to prevent path traversal outside the history directory | |
| string sanitizedFileName = Path.GetFileName(history.WordBorderInfoFileName); | |
| // If sanitization results in an empty value, skip file-based loading | |
| if (!string.IsNullOrWhiteSpace(sanitizedFileName)) | |
| { | |
| // Optionally ensure we only read expected JSON metadata files | |
| // If an unexpected extension is encountered, fall back to inline JSON | |
| if (string.Equals(Path.GetExtension(sanitizedFileName), ".json", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| string historyBasePath = await FileUtilities.GetPathToHistory(); | |
| string wordBorderInfoPath = Path.Combine(historyBasePath, sanitizedFileName); | |
| if (File.Exists(wordBorderInfoPath)) | |
| { | |
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | |
| List<WordBorderInfo>? wordBorderInfos = | |
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | |
| return wordBorderInfos ?? []; | |
| } | |
| } |
Text-Grab/Services/HistoryService.cs
Outdated
| string historyBasePath = await FileUtilities.GetPathToHistory(); | ||
| string wordBorderInfoPath = Path.Combine(historyBasePath, history.WordBorderInfoFileName); | ||
|
|
||
| if (File.Exists(wordBorderInfoPath)) | ||
| { | ||
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | ||
| List<WordBorderInfo>? wordBorderInfos = | ||
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | ||
|
|
||
| return wordBorderInfos ?? []; | ||
| } | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(history.WordBorderInfoJson)) | ||
| return []; | ||
|
|
||
| List<WordBorderInfo>? inlineWordBorderInfos = | ||
| JsonSerializer.Deserialize<List<WordBorderInfo>>(history.WordBorderInfoJson, HistoryJsonOptions); | ||
|
|
||
| return inlineWordBorderInfos ?? []; |
There was a problem hiding this comment.
GetWordBorderInfosAsync does file IO + JSON deserialization without any exception handling. File.OpenRead / JsonSerializer.DeserializeAsync / Deserialize can throw (e.g., corrupt JSON, sharing violations), which would crash opening a history item. Consider catching IOException/JsonException and returning an empty list (or falling back to inline JSON) instead.
| string historyBasePath = await FileUtilities.GetPathToHistory(); | |
| string wordBorderInfoPath = Path.Combine(historyBasePath, history.WordBorderInfoFileName); | |
| if (File.Exists(wordBorderInfoPath)) | |
| { | |
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | |
| List<WordBorderInfo>? wordBorderInfos = | |
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | |
| return wordBorderInfos ?? []; | |
| } | |
| } | |
| if (string.IsNullOrWhiteSpace(history.WordBorderInfoJson)) | |
| return []; | |
| List<WordBorderInfo>? inlineWordBorderInfos = | |
| JsonSerializer.Deserialize<List<WordBorderInfo>>(history.WordBorderInfoJson, HistoryJsonOptions); | |
| return inlineWordBorderInfos ?? []; | |
| try | |
| { | |
| string historyBasePath = await FileUtilities.GetPathToHistory(); | |
| string wordBorderInfoPath = Path.Combine(historyBasePath, history.WordBorderInfoFileName); | |
| if (File.Exists(wordBorderInfoPath)) | |
| { | |
| await using FileStream wordBorderInfoStream = File.OpenRead(wordBorderInfoPath); | |
| List<WordBorderInfo>? wordBorderInfos = | |
| await JsonSerializer.DeserializeAsync<List<WordBorderInfo>>(wordBorderInfoStream, HistoryJsonOptions); | |
| if (wordBorderInfos is not null) | |
| return wordBorderInfos; | |
| } | |
| } | |
| catch (IOException ex) | |
| { | |
| Debug.WriteLine($"Failed to read word border info file for history item '{history.ID}': {ex}"); | |
| } | |
| catch (JsonException ex) | |
| { | |
| Debug.WriteLine($"Failed to deserialize word border info file for history item '{history.ID}': {ex}"); | |
| } | |
| } | |
| if (string.IsNullOrWhiteSpace(history.WordBorderInfoJson)) | |
| return []; | |
| try | |
| { | |
| List<WordBorderInfo>? inlineWordBorderInfos = | |
| JsonSerializer.Deserialize<List<WordBorderInfo>>(history.WordBorderInfoJson, HistoryJsonOptions); | |
| return inlineWordBorderInfos ?? []; | |
| } | |
| catch (JsonException ex) | |
| { | |
| Debug.WriteLine($"Failed to deserialize inline word border info for history item '{history.ID}': {ex}"); | |
| return []; | |
| } |
| string historyBasePath = GetHistoryPathBlocking(); | ||
| string filePath = Path.Combine(historyBasePath, Path.GetFileName(historyFileName)); | ||
|
|
||
| if (File.Exists(filePath)) | ||
| File.Delete(filePath); | ||
| } |
There was a problem hiding this comment.
DeleteHistoryFile deletes files without handling IO exceptions. If an image/sidecar file is locked or access is denied, File.Delete will throw and can break history cleanup/write. Wrap deletes in a try/catch (similar to other file ops in this service) and continue on failure.
Text-Grab/Services/HistoryService.cs
Outdated
| string fileName = Path.GetFileName(wordBorderInfoFile); | ||
|
|
||
| if (!expectedFileNames.Contains(fileName)) | ||
| File.Delete(wordBorderInfoFile); |
There was a problem hiding this comment.
DeleteUnusedWordBorderFiles deletes sidecar files without exception handling. A transient lock/permission issue will throw and could prevent WriteHistory from completing. Catch IOException/UnauthorizedAccessException around File.Delete and keep iterating so history writes remain resilient.
| File.Delete(wordBorderInfoFile); | |
| { | |
| try | |
| { | |
| File.Delete(wordBorderInfoFile); | |
| } | |
| catch (IOException ex) | |
| { | |
| Debug.WriteLine($"Failed to delete word border info file '{wordBorderInfoFile}': {ex}"); | |
| } | |
| catch (UnauthorizedAccessException ex) | |
| { | |
| Debug.WriteLine($"Access denied when deleting word border info file '{wordBorderInfoFile}': {ex}"); | |
| } | |
| } |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@TheJoeFin I've opened a new pull request, #636, to work on those changes. Once the pull request is ready, I'll request review from you. |
…vice Co-authored-by: TheJoeFin <7809853+TheJoeFin@users.noreply.github.com>
HistoryService: path traversal sanitization and exception resilience
Added three WpfFact tests to verify correct export and import of managed JSON settings (e.g., regex lists, post-grab check states), including round-trip and legacy inline storage scenarios. Ensured all managed setting keys are present in exports and that legacy imports are routed to sidecar files. Added necessary using statements for new test coverage.
No description provided.