Model a scroll container in the drag-and-drop spec fixtures - #24622
Conversation
|
Warning This pull request does not link an OpenProject work package. Please add a link to the work package in the description, or reference it in the |
There was a problem hiding this comment.
Pull request overview
This PR updates frontend drag-and-drop spec fixtures to better model production by ensuring Pragmatic’s auto-scroll attaches to an element that appears scrollable (via computed overflow), eliminating noisy Vitest warnings and large DOM dumps in test output.
Changes:
- Adds scroll-container semantics to multiple spec fixtures via
overflow-y:auto/overflow: autowrappers. - Adjusts several Angular test templates to include a
.scroll-host(and in nested-root cases an additional.inner-scroll-host) so ancestor-walk logic resolves a scrollable element. - Updates spec comments around why Pragmatic is not mocked and why these fixtures must model scrollability.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/common/drag-and-drop/sortable-lists-engine.preview.spec.ts | Makes the fixture root look scrollable so engine-level auto-scroll registration doesn’t warn. |
| frontend/src/app/shared/helpers/drag-and-drop/drag-and-drop.service.spec.ts | Ensures the service’s provided container is treated as a scroll container by Pragmatic. |
| frontend/src/app/shared/directives/sortable-lists/sortable-lists.directive.behavior.spec.ts | Wraps multiple directive fixtures in scrollable ancestors so closest-scrollable-ancestor resolution matches production. |
| frontend/src/app/features/work-packages/components/wp-card-view/services/wp-card-drag-and-drop.service.spec.ts | Adds a scrollable wrapper for the two-list card-view directive fixture to avoid autoscroll warnings. |
c24f965 to
76bb224
Compare
529d5c3 to
9d6b6e8
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
76bb224 to
e7a6126
Compare
9d6b6e8 to
2cab1f4
Compare
e7a6126 to
f22e7c6
Compare
2cab1f4 to
8b24961
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
f22e7c6 to
5a2889c
Compare
8b24961 to
177a1bf
Compare
5a2889c to
f1f3f93
Compare
177a1bf to
16506df
Compare
f1f3f93 to
a3f233c
Compare
16506df to
2064209
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
a3f233c to
9577b7a
Compare
2064209 to
75eb667
Compare
9577b7a to
d5bd278
Compare
75eb667 to
de57346
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
d5bd278 to
9f2a00b
Compare
de57346 to
86f4119
Compare
2eba20d to
294f54a
Compare
626653a to
d7e2f31
Compare
294f54a to
2eba20d
Compare
d7e2f31 to
626653a
Compare
2eba20d to
294f54a
Compare
626653a to
d7e2f31
Compare
294f54a to
90c3b50
Compare
d7e2f31 to
bccc6d2
Compare
bccc6d2 to
0899bb2
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
The Angular unit-test builder bundles each spec into a __commonJS wrapper, so Vitest's hoist validator walks that wrapper instead of our source and reports top-level vi.mock calls as nested. Only mock, unmock and hoisted are hoist-checked, so vi.doMock sidesteps it. These specs already import the mocked packages and the controller under test lazily in beforeAll, which is the ordering doMock needs. A note above each call records that, since doMock's position above the imports no longer carries the guarantee vi.mock's hoisting gave it. The root spec also claimed it read back the combine, prevent-unhandled and drag-preview options. It never did; they keep the mounted item controller's side effects inert.
vi.doMock only affects later imports, so the statically imported createSortableRoot would have pulled in the real auto-scroll module first and the mock would have stopped applying silently. A type import plus a lazy binding restores the ordering in both engine specs, and drops vi.hoisted with it. The preview spec also loses the file-split rationale that cited a shared module registry, which isolate:true removed. The split itself stands: the sibling renders previews for real.
Pragmatic warns when auto-scroll attaches to an element whose computed overflow is visible. Production always resolves a real scroll ancestor, so the warning was accurate: the fixtures had none. A wrapper carrying overflow but no height satisfies the check without shifting the geometry these specs measure drop edges from. Where the overflow belongs differs. The directive walks up to the closest scrollable ancestor, so its hosts and the card-view root get a wrapper; the drag service and the preview spec register on the element they are handed, so that element takes it directly. Nested-root fixtures need a second, inner wrapper. Two independent root engines resolving one shared host collide on Pragmatic's element-keyed registry, which dedupes per root and not across them. The opSortableListsScrollContainer suite keeps its unscrollable hosts. It asserts that warning on purpose.
0899bb2 to
e01fca2
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Ticket
No work package — frontend test-output hygiene, noticed while working on AGILE-361.
What are you trying to accomplish?
Remove ~43 auto-scroll warnings and their DOM dumps from Vitest output by giving drag-and-drop fixtures realistic scroll containers.
What approach did you choose and why?
Add
overflow-y: autoto the appropriate fixture elements or wrappers, without height limits, preserving geometry-sensitive assertions. Fixtures testing missing scroll containers or explicit-list fallbacks remain unchanged.Nested roots get separate scroll hosts because sharing one can lose auto-scroll when either root cleans up. This remains a constraint for the scroll-container decision deferred in #24504.
Merge checklist