chore: separate non-render cleanup#865
Conversation
f956492 to
b9e003b
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts non-render cleanup work into a dedicated change, including build/test workflow tightening, documentation/instruction updates, and xWorks headword-number configuration refactors (plus tests), along with a more deterministic TonePars generated-content comparison.
Changes:
- Tighten build/test entrypoints and agent helpers (Windows-only enforcement, native artifact freshness checks, dependency verification tweaks).
- Refactor xWorks headword-number custom digit handling (UI/controller/model serialization) with corresponding test updates.
- Update docs/instructions and installer packaging configuration (CPM scoping for WiX packages, OpenSpec prompt/skill cleanup, misc whitespace/config normalization).
Reviewed changes
Copilot reviewed 81 out of 84 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/toolshims/environ.cmd | Updates deprecated shim messaging to clarify non-Windows limitations. |
| scripts/Agent/Git-Search.ps1 | Improves repo-root inference and errors when not in a git repo. |
| build.ps1 | Renames tracing switch, blocks non-Windows builds, and refreshes stale Views native artifacts. |
| Test.runsettings | Normalizes indentation/formatting. |
| Src/xWorks/xWorksTests/HeadwordNumbersControllerTests.cs | Updates/extends tests for new custom digits + OK enablement behavior. |
| Src/xWorks/xWorksTests/DictionaryPublicationDecoratorTests.cs | Adds regression test for refresh notifications when filtering changes. |
| Src/xWorks/xWorksTests/DictionaryConfigurationModelTests.cs | Extends schema/clone tests to include custom homograph numbers. |
| Src/xWorks/XmlDocView.cs | Adds delete handler behavior for reversal entries routing. |
| Src/xWorks/RecordClerk.cs | Adjusts delete handler routing/return semantics and unsubscribes from DeleteRecord message. |
| Src/xWorks/IHeadwordNumbersView.cs | Extends view interface with CustomDigitsChanged + WS factory hook. |
| Src/xWorks/HeadWordNumbersHelper.cs | Removes helper used for splitting Unicode digits (now using config-driven digits). |
| Src/xWorks/HeadWordNumbersDlg.cs | Updates dialog to use FwTextBox digits, apply stylesheet/WS, and emit CustomDigitsChanged. |
| Src/xWorks/HeadWordNumbersController.cs | Drives view from model’s custom number list and validates OK enablement via event. |
| Src/xWorks/FwXWindow.cs | Removes popup window icon assignment. |
| Src/xWorks/DictionaryPublicationDecorator.cs | Adds change detection in Refresh and notifies roots when visible filtering changes. |
| Src/xWorks/DictionaryDetailsController.cs | Ensures Headword numbers dialog receives stylesheet. |
| Src/xWorks/DictionaryConfigurationModel.cs | Adds XML serialization for custom homograph numbers and cloning/export support. |
| Src/xWorks/ConfiguredLcmGenerator.cs | Uses custom homograph digits for sense numbering when defined. |
| Src/Utilities/pcpatrflex/DisambiguateInFLExDB/DisambiguateInFLExDBTests/ToneParsInvokerTests.cs | Makes TonePars output comparison deterministic by stripping absolute Windows path prefixes via regex. |
| Src/AppForTests.config | Reformats diagnostics config and trims assembly-qualified type string. |
| SDK_MIGRATION.md | Updates docs to reference -EnableTracing switch. |
| ReadMe.md | Adds explicit Linux/macOS guidance (editing/review only; no builds/tests). |
| FLExInstaller/wix6/Shared/CustomActions/CustomActions/CustomActions.csproj | Removes explicit WixToolset package versions (moves to local CPM). |
| FLExInstaller/Directory.Packages.props | Enables CPM for installer subtree and centralizes WixToolset package versions. |
| Docs/CONTRIBUTING.md | Clarifies Windows-only build/test/installer workflows; warns non-Windows users. |
| Directory.Build.targets | Adds target to delete stale _wpftmp files before WPF markup pass 2. |
| Directory.Build.props | Sets SolutionDir/SolutionName explicitly and globally excludes _wpftmp items. |
| Build/scripts/Invoke-CppTest.ps1 | Uses new Views-native artifact freshness helper for TestViews prerequisites. |
| Build/Src/NativeBuild/NativeBuild.csproj | Formatting-only change for PackageReference. |
| Build/Src/FwBuildTasks/CollectTargets.cs | Collects ScrChecks info and wires ScrChecks dependency into certain test targets. |
| Build/Installer.legacy.targets | Adds ScrChecks.dll/.pdb to installer merge modules list. |
| Build/Agent/fix-whitespace.ps1 | Simplifies whitespace fixer and changes how files are written. |
| Build/Agent/Verify-FwDependencies.ps1 | Minor refactor for required/optional dependency handling and array materialization. |
| Build/Agent/README.md | Documents Run-AllRenders helper script. |
| Build/Agent/FwBuildHelpers.psm1 | Adds artifact freshness helpers and exports new functions. |
| AGENTS.md | Updates agent guidance and adds liblcm dependency note. |
| .vscode/settings.json | Removes auto-approval for Windows-only build tasks and clarifies manual-approval list. |
| .github/workflows/patch-installer-cd.yml | Whitespace-only YAML cleanup. |
| .github/skills/openspec-verify-change/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/openspec-sync-specs/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/openspec-onboard/SKILL.md | Updates onboarding instructions and command reference around /opsx:propose and CLI install check. |
| .github/skills/openspec-new-change/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/openspec-ff-change/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/openspec-explore/SKILL.md | Adjusts explore guidance to steer toward proposals rather than implementation. |
| .github/skills/openspec-continue-change/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/openspec-bulk-archive-change/SKILL.md | Updates messaging when no changes exist to archive. |
| .github/skills/openspec-archive-change/SKILL.md | Updates sync instructions to invoke skill/tool-based sync prompt flow. |
| .github/skills/openspec-apply-change/SKILL.md | Updates openspec-generated metadata version. |
| .github/skills/code-review-skill-main/scripts/pr-analyzer.py | Adds new PR complexity analyzer script (new file). |
| .github/skills/code-review-skill-main/reference/typescript.md | Adds TypeScript review guide reference (new file). |
| .github/skills/code-review-skill-main/reference/security-review-guide.md | Adds security review guide (new file). |
| .github/skills/code-review-skill-main/reference/performance-review-guide.md | Adds performance review guide (new file). |
| .github/skills/code-review-skill-main/reference/java.md | Adds Java review guide (new file). |
| .github/skills/code-review-skill-main/reference/css-less-sass.md | Adds CSS/Less/Sass review guide (new file). |
| .github/skills/code-review-skill-main/reference/cpp.md | Adds C++ review guide (new file). |
| .github/skills/code-review-skill-main/reference/code-review-best-practices.md | Adds review best-practices reference (new file). |
| .github/skills/code-review-skill-main/reference/c.md | Adds C review guide (new file). |
| .github/skills/code-review-skill-main/reference/architecture-review-guide.md | Adds architecture review guide (new file). |
| .github/skills/code-review-skill-main/assets/review-checklist.md | Adds quick checklist asset (new file). |
| .github/skills/code-review-skill-main/assets/pr-review-template.md | Adds PR review template asset (new file). |
| .github/skills/code-review-skill-main/SKILL.md | Adds “code-review-excellence” skill definition (new file). |
| .github/skills/code-review-skill-main/README.md | Adds skill documentation/readme (new file). |
| .github/skills/code-review-skill-main/LICENSE | Adds MIT license for imported skill content (new file). |
| .github/skills/code-review-skill-main/CONTRIBUTING.md | Adds contributing guide for skill content (new file). |
| .github/skills/code-review-skill-main/.gitignore | Adds gitignore for skill subtree (new file). |
| .github/prompts/opsx-onboard.prompt.md | Mirrors onboard skill updates in prompt form. |
| .github/prompts/opsx-ff.prompt.md | Clarifies template vs context/rules handling in ff prompt. |
| .github/prompts/opsx-explore.prompt.md | Updates explore prompt wording to steer toward proposals. |
| .github/prompts/opsx-bulk-archive.prompt.md | Updates “no changes” message. |
| .github/prompts/opsx-archive.prompt.md | Updates sync invocation instructions to tool/skill-based flow. |
| .github/instructions/terminal.instructions.md | Removes “task-first for CI parity” rule text. |
| .github/instructions/repo.instructions.md | Removes explicit VS Code CI task requirements. |
| .github/instructions/dotnet-framework.instructions.md | Updates guidance for SDK-style net48 projects and Windows-only build policy. |
| .github/instructions/debugging.instructions.md | Updates tracing docs to match -EnableTracing switch. |
| .github/instructions/csharp.instructions.md | Adds new C# coding guidance specific to FieldWorks (new file). |
| .github/instructions/build.instructions.md | Documents non-Windows host limitations and fail-fast behavior. |
Files not reviewed (1)
- Src/xWorks/HeadWordNumbersDlg.designer.cs: Language not supported
Comments suppressed due to low confidence (5)
Src/xWorks/DictionaryConfigurationModel.cs:1
- The serialization delimiter is inconsistent with how the new/updated tests populate
CustomHomographNumbers(several tests use;separators, but this setter only splits on,). This will collapse\"0;1;...\"into a single list item and can break downstream behavior that expects 10 digits. Recommendation: standardize on one delimiter repo-wide and enforce it in both tests and serialization; or accept both delimiters in the setter (e.g., split on,and;) while emitting a single canonical delimiter from the getter. Also,WebUtility.HtmlDecode(value)will throw ifvalueis null; handle null as empty.
Src/xWorks/HeadWordNumbersController.cs:1 _view.CustomDigitsis treated as non-null, but the interface allows implementations to return null (and this controller itself sets_view.CustomDigitsfrom_homographConfig.CustomHomographNumberList, which may be null). If_view.CustomDigitsis null at runtime, this handler will throw. Suggest normalizing to an empty sequence inside the handler (e.g.,var digits = _view.CustomDigits ?? Enumerable.Empty<string>();) and then computingOkButtonEnabledfromdigits.
Src/xWorks/xWorksTests/HeadwordNumbersControllerTests.cs:1TriggerCustomDigitsChanged()will throwNullReferenceExceptionifCustomDigitsChangedhas no subscribers (or if a test constructs the view without creating a controller). Use a null-safe invoke and passEventArgs.Emptyrather than null to better match typical .NET event patterns.
Build/Agent/fix-whitespace.ps1:1- This changes file-write semantics in a way that can be non-deterministic across hosts and PowerShell versions:
-Encoding utf8means UTF-8 with BOM in Windows PowerShell 5.1 but typically UTF-8 without BOM in PowerShell 7+, and the script no longer preserves the original BOM state (the previous implementation explicitly did). Recommendation: restore explicit BOM preservation (or explicitly standardize on BOM/no-BOM) by writing with a knownSystem.Text.UTF8Encodingchoice, or by using the explicitutf8BOM/utf8NoBOMencodings where available, so whitespace-fix does not churn file encodings unexpectedly.
Src/xWorks/DictionaryPublicationDecorator.cs:1 - The dictionary comparison uses
OrderBy(...).SequenceEqual(...), which allocates and adds O(n log n) cost on everyRefresh()call. SinceRefresh()can be frequent, prefer an O(n) comparison without sorting (e.g., compareCount, then iterate one dictionary andTryGetValueinto the other for value equality). This keeps the change-detection logic while reducing per-refresh overhead.
|
Copilot rationale note 1/4: why this PR exists This PR is intentionally the non-render half of the split from the render-speedup branch. I reviewed it as real code only and ignored the agent/skills/prompt updates. The theme is not a single product feature. It is stabilization work around several recent FieldWorks changes that were previously riding along with the rendering branch:
The xWorks dictionary/delete-routing changes that were previously discussed here have been removed from this PR. The underlying DeleteRecord architecture issue on main is now tracked separately as LT-22514. So the main value of PR #865 is: keep the render PR focused, while preserving the non-render build/test/installer/TonePars fixes that make current mainline development and tests less brittle. |
|
Copilot rationale note 2/4: build/test reliability changes The build/test pieces are here because FieldWorks is a mixed native/managed Windows repo, and simple existence checks are not enough for generated native artifacts.
|
|
Copilot rationale note 3/4: installer and TonePars changes The installer changes are here because FLExInstaller does not behave like an ordinary app project. WiX custom actions, DTF packages, and patch installer versioning have constraints that should not be accidentally governed by the same package-management assumptions as the main FieldWorks projects.
The TonePars change in |
Keep build/test helper cleanup out of the render commit. Keep agent, installer, and xWorks cleanup separate. Normalize TonePars generated-file comparisons for Windows paths.
b9e003b to
fe6f187
Compare
|
Copilot update: the xWorks dictionary/DeleteRecord routing changes have been removed from this PR. The DeleteRecord pub/sub architecture issue on |
Summary
This pulls the non-render cleanup out of #724 so the render-speedup PR can stay focused on Views rendering and render verification.
Changes include:
The xWorks dictionary/DeleteRecord routing changes that were previously part of this branch have been removed from PR #865. The underlying DeleteRecord command-routing issue on
mainis now tracked separately in LT-22514.Validation
origin/main..cleanup/non-render-pr-splitnow has noSrc/xWorksdiff.shell: Buildpassed on this branch.shell: Testpassed on this branch: 4150 total, 4090 passed, 60 skipped.This change is