-
Couldn't load subscription status.
- Fork 12
Refactor storage & models; add change events, UX polish, and read-only mode #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Move `JSONContext.cs` to `Helpers/` - Add `Helpers/JsonFileStore.cs`; introduce `DataFile<T>` persistence contract - Add `Version` and `LastModified` metadata to `DataFile<T>`to enable future compatibility checks and migrations - Rename `WebSearchShortcutDataEntry` → `ShortcutEntry` (under `Shortcut` namespace) - Make `ShortcutEntry.Id` init-only to prevent mutation after creation. - Split `Storage.cs` into two layers: - `ShortcutStore.cs` — file I/O via `JsonFileStore` - `ShortcutService.cs` — central API for UI/Commands - Enforce defensive copies in `ShortcutService` to prevent shared mutable state, aliasing, and concurrency issues - Update commands/forms/pages to use the new model & store - Reorder `AddShortcutForm` sections/fields for a more logical flow and consistent UX.
…from data updates
- Introduce `ShortcutService.ChangedEvent` with `ShortcutsChangedEventArgs { Before, After }`
- Fire events from `Add/Remove/Update` after persistence;
- Make `Add` return `void`; align with snapshot-at-boundary design
- `AddShortcutForm`: remove `AddedCommand`; call `ShortcutService.Add/Update` directly
- `WebSearchShortcutCommandsProvider`: subscribe to `ShortcutService.ChangedEvent` and `Refresh` on change
- Make `AddShortcutForm` lazy: remove the page-level `_addShortcutForm` field; keep only `ShortcutEntry? _shortcut` and create the form in `GetContent()` - Call `BrowserDiscovery.Reload()` when constructing the form to ensure the installed browser list is refreshed before populating the ChoiceSet. - In `WebSearchShortcutCommandsProvider.Refresh`, early-return when `Name`, `Domain`, and `IconUrl` are unchanged to avoid unnecessary `ReloadCommands()` and `RaiseItemsChanged(0)`
* Replace immediate delete with `CommandResult.Confirm(...)` * Resource updates: add `DeleteShortcutConfirm_TitleTemplate`, `DeleteShortcutConfirm_DescriptionTemplate`, `DeleteShortcutConfirm_ButtonTemplate`
* Add `ShortcutService.ReadOnlyMode` (backed by `volatile _readOnlyMode`); set to true on load/save exceptions; make `Save(...)` a no-op when read-only. * Show a prominent warning banner in `AddShortcutForm` during read-only, including `ShortcutService.ShortcutFilePath`. * Switch delete confirmation copy to a read-only variant (passes `filePath`) when applicable. * Resource updates: add `AddShortcutForm_ReadOnlyModeWarning` and `DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode`. * Minor: improve generated Adaptive Card JSON readability by joining options with.
2b1f06f to
9add31e
Compare
|
I'm very sorry that I was very busy recently and didn't have time to review this PR. I will review it in the coming period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reorganizes the web search shortcut extension by introducing a more robust storage system with versioning, implementing read-only mode protection, decoupling UI from storage through event-driven architecture, and adding safer delete operations with confirmation dialogs.
- Refactored storage architecture with
JsonFileStore<T>andDataFile<T>envelope supporting versioning and metadata - Replaced direct data manipulation with event-driven
ShortcutServiceAPI that emits change notifications - Added read-only mode fallback when file operations fail to prevent data loss from user-edited files
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| WebSearchShortcutCommandsProvider.cs | Replaced direct storage manipulation with service calls and event-driven UI updates |
| Storage.cs | Completely removed legacy storage class |
| ShortcutStore.cs | New JSON file store implementation with default shortcuts and versioning |
| ShortcutService.cs | New service layer providing thread-safe CRUD operations with change events |
| ShortcutEntry.cs | Renamed from WebSearchShortcutDataEntry to ShortcutEntry with init-only Id property |
| IconService.cs | Updated type references from WebSearchShortcutDataEntry to ShortcutEntry |
| Resources files | Added localized strings for delete confirmation and read-only mode warnings |
| SearchWebPage.cs | Updated constructor parameter type |
| AddShortcutPage.cs | Simplified to lazy-create form content and removed event plumbing |
| JSONContext.cs | Removed and replaced with Helpers/JsonContext.cs |
| JsonFileStore.cs | New generic file store with DataFile envelope supporting versioning |
| JsonContext.cs | New JSON serialization context with enhanced options |
| AddShortcutForm.cs | Direct service integration, read-only banner, and browser discovery refresh |
| SearchWebCommand.cs | Updated type references |
| OpenHomePageCommand.cs | Updated type references |
| BrowserExecutionInfo.cs | Updated constructor parameter type |
Files not reviewed (1)
- CmdPalWebSearchShortcut/WebSearchShortcut/Properties/Resources.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <value>编辑搜索捷径</value> | ||
| </data> | ||
| <data name="AddShortcutForm_ReadOnlyModeWarning" xml:space="preserve"> | ||
| <value /> |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Chinese (Simplified) localization for AddShortcutForm_ReadOnlyModeWarning is empty. This should contain a translated version of the read-only mode warning message.
| <value /> | |
| <value>当前为只读模式,无法编辑或保存更改。</value> |
| <value /> | ||
| </data> | ||
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | ||
| <value /> | ||
| </data> | ||
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | ||
| <value /> |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple Chinese (Simplified) localizations for delete confirmation dialog are empty (DeleteShortcutConfirm_TitleTemplate, DeleteShortcutConfirm_DescriptionTemplate, DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode). These should contain translated versions of the confirmation dialog text.
| <value /> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | |
| <value /> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | |
| <value /> | |
| <value>删除搜索捷径</value> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | |
| <value>您确定要删除“{shortcut}”这个搜索捷径吗?</value> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | |
| <value>在只读模式下无法删除此搜索捷径。</value> |
| <value>Modifier le raccourci de recherche</value> | ||
| </data> | ||
| <data name="AddShortcutForm_ReadOnlyModeWarning" xml:space="preserve"> | ||
| <value /> |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The French localization for AddShortcutForm_ReadOnlyModeWarning is empty. This should contain a translated version of the read-only mode warning message.
| <value /> | |
| <value>Ce formulaire est en mode lecture seule. Les modifications ne peuvent pas être enregistrées.</value> |
| <value /> | ||
| </data> | ||
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | ||
| <value /> | ||
| </data> | ||
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | ||
| <value /> |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple French localizations for delete confirmation dialog are empty (DeleteShortcutConfirm_TitleTemplate, DeleteShortcutConfirm_DescriptionTemplate, DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode). These should contain translated versions of the confirmation dialog text.
| <value /> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | |
| <value /> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | |
| <value /> | |
| <value>Supprimer le raccourci ?</value> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate" xml:space="preserve"> | |
| <value>Voulez-vous vraiment supprimer le raccourci « {shortcut} » ?</value> | |
| </data> | |
| <data name="DeleteShortcutConfirm_DescriptionTemplate_ReadOnlyMode" xml:space="preserve"> | |
| <value>Vous ne pouvez pas supprimer le raccourci « {shortcut} » car il est en lecture seule.</value> |
|
@Daydreamer-riri I don’t know any French, and I’m less familiar with Simplified Chinese expressions than you are, so I can’t assess Copilot’s suggestions. Honestly, I let GPT handle all the English content. I’m counting on you for the rest. |
Summary
This PR reorganizes persistence into a reusable JSON store, unifies the shortcut data model, decouples UI from storage via change events, and ships two UX improvements: a delete-confirmation flow and a safe “Read-only Mode” fallback when persistence fails. Together these reduce coupling, make future migrations/versioning feasible, and improve reliability and user trust.
Why
Prevent regressions from backward-incompatible changes. We previously shipped changes that broke backward compatibility. Adding an explicit
Versionto the persisted envelope makes the format self-describing, enables forward/backward checks and controlled migrations, and helps ensure we don’t repeat that issue.Protect users from irreversible deletes. Deleting a shortcut is destructive and cannot be undone; a confirmation step prevents accidental loss from misclicks.
Respect power users, protect their data. We do not oppose users hand-editing the JSON file. If an edit results in an invalid format, Read-only Mode prevents the app from overwriting the file on save, avoiding irreversible data loss and giving users a chance to fix the file safely.
What
refactor: persistence + models — Introduce
JsonFileStore<T>with aDataFile<T>envelope (Version,LastModified); moveJSONContext→Helpers; renameWebSearchShortcutDataEntry→ShortcutEntryand makeIdinit-only; split legacyStorageintoShortcutStore(I/O) andShortcutService(API); return defensive snapshots; tidyAddShortcutForm.refactor(event): decouple UI from data — Add
ShortcutService.ChangedEventemitting{ Before, After }onAdd/Update/Remove; remove ad-hoc UI plumbing (e.g.,AddedCommand); UI refreshes by listening to the event.perf(ui): reduce churn — Lazy-create
AddShortcutForminGetContent(); refresh installed browsers on open; skip command reloads when only non-visual fields change (no change toName/Domain/IconUrl).feat(ui): safer deletes — Replace immediate delete with
CommandResult.Confirm(...)and localized copy (title/description/button) to guard an irreversible action.feat(readonly): protect user-edited files — Add
ShortcutService.ReadOnlyMode(activated on load/save failure); show an in-form banner with the file path; makeSave(...)a no-op in read-only to avoid overwriting invalid or user-edited JSON.