chore(frontend): remove the disabled CollabWrapperComponent - #7351
Open
aglinxinyuan wants to merge 2 commits into
Open
chore(frontend): remove the disabled CollabWrapperComponent#7351aglinxinyuan wants to merge 2 commits into
aglinxinyuan wants to merge 2 commits into
Conversation
CollabWrapperComponent cannot be instantiated. It is declared in app.module and registered as the formly wrapper "collab-wrapper", but nothing ever applies that wrapper name to a field: the only code that does is CollabWrapperComponent.setupFieldConfig, inside the component's own file, and its sole caller in operator-property-edit-frame is commented out. That caller was disabled with "TODO: we temporarily disable this due to Yjs update causing issues in Formly". The disabling predates the October 2025 core/gui relocation, so it has been off for well over a year, and every commit touching the component since has been mechanical upkeep -- an Angular 19 upgrade, the standalone-components refactor, and a dependabot y-quill bump keeping a dependency current for code that never runs. This is a proposal rather than an obvious cleanup: the TODO states an intent to restore, so if that is still live, say so and I will close this. The implementation is one `git revert` away either way, and the sibling PresetWrapperComponent.setupFieldConfig remains in use, so the pattern to restore it from is still in the tree. Removed: the component and its template/styles, the app.module import and declaration, the formly-config import and wrapper registration, and the commented-out caller block that referenced the deleted class. One knock-on. operator-property-edit-frame.component.spec.ts imported COLLAB_DEBOUNCE_TIME_MS purely as a tick() duration during fixture setup, unrelated to the debounce its assertions actually wait on (FORM_DEBOUNCE_TIME_MS). It is replaced by a locally named SETUP_FLUSH_MS with the same 10ms value, so behaviour is unchanged. quill, y-quill and quill-cursors are NOT removable: operator-property-edit-frame and port-property-edit-frame both still use them. No package.json change here. Verified: tsc --noEmit clean across the app, formly-config.spec (11) and operator-property-edit-frame.spec (195) green, yarn format:ci passing.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the disabled CollabWrapperComponent wiring from the Angular frontend, cleaning up dead Formly wrapper code that was registered but not reachable/exercised. It also adjusts the affected unit test to stop importing a debounce constant from the removed component.
Changes:
- Removed
CollabWrapperComponent(TS + template + styles) and its registration as thecollab-wrapperFormly wrapper. - Deleted the long-disabled (commented-out) call site in
OperatorPropertyEditFrameComponent. - Updated
operator-property-edit-frameunit tests to use a locally defined setup flush duration instead of importingCOLLAB_DEBOUNCE_TIME_MS.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts | Removes the commented-out CollabWrapperComponent.setupFieldConfig(...) block. |
| frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts | Replaces the removed constant import with a locally defined setup flush constant. |
| frontend/src/app/common/formly/formly-config.ts | Drops collab-wrapper import and wrapper registration from Formly config. |
| frontend/src/app/common/formly/collab-wrapper/collab-wrapper/collab-wrapper.component.ts | Deletes the unused collaborative wrapper component implementation. |
| frontend/src/app/common/formly/collab-wrapper/collab-wrapper/collab-wrapper.component.html | Deletes the wrapper template. |
| frontend/src/app/common/formly/collab-wrapper/collab-wrapper/collab-wrapper.component.css | Deletes the wrapper styles. |
| frontend/src/app/app.module.ts | Removes the component import/declaration from AppModule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The constant landed at the exact line the deleted COLLAB_DEBOUNCE_TIME_MS import used to occupy, which left it wedged between imports with thirty more import statements after it. Move it, with its comment, to just above the other module-level constant so the import block stays contiguous. Pure reorder: the only read is a tick() call inside a test body, far after the declaration, so there is no temporal-dead-zone concern and no behaviour change. Addresses the review comment on apache#7351. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7351 +/- ##
============================================
+ Coverage 83.72% 83.84% +0.12%
Complexity 4137 4137
============================================
Files 1169 1167 -2
Lines 46520 46444 -76
Branches 5179 5164 -15
============================================
- Hits 38947 38941 -6
+ Misses 5860 5791 -69
+ Partials 1713 1712 -1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
CollabWrapperComponentcannot be instantiated. It is wired in, but nothing reaches it:It is currently the largest uncovered frontend file at 6.8% of 73 lines.
Please read this as a proposal, not a routine cleanup. The caller was disabled deliberately:
I am not treating "temporarily" as decisive, for these reasons:
core/guirelocationy-quill0.1.5 → 1.0.0 (#6418)Nobody has worked on the feature itself since it was switched off — it has only been carried along by upgrades. But if the intent to restore is still live, say so and I will close this. The implementation is one
git revertaway, and the siblingPresetWrapperComponent.setupFieldConfigis still in use atoperator-property-edit-frame.component.ts:1084, so the pattern to rebuild from remains in the tree either way.Removed: the component with its template and styles, the
app.moduleimport and declaration, theformly-configimport and wrapper registration, and the commented-out caller that referenced the deleted class.One knock-on, handled explicitly.
operator-property-edit-frame.component.spec.tsimportedCOLLAB_DEBOUNCE_TIME_MSfrom the component — but only as atick()duration during fixture setup, before the change under test, and unrelated to the debounce its assertions actually wait on (FORM_DEBOUNCE_TIME_MS, 150ms). It was an incidental borrow of a convenient10. Replaced with a locally namedSETUP_FLUSH_MS = 10, so behaviour is byte-identical and the intent is now stated where it is used.No
package.jsonchange.quill,y-quillandquill-cursorslook like they should go with it, but they do not:operator-property-edit-frameandport-property-edit-frameboth still use all three.Any related issues, documentation, discussions?
Closes #7350
How was this PR tested?
Deletions need proof that nothing else depended on the removed code, so:
Those two specs are the ones that actually exercise the touched wiring —
formly-config.speccovers the wrapper registry, and the other is the spec whose import changed.yarn format:cipasses; it flaggedformly-config.tson the first attempt, since removing the entry changed the array's wrapping.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)