Skip to content

Model a scroll container in the drag-and-drop spec fixtures - #24622

Open
myabc wants to merge 1 commit into
fix/vitest-mock-hoisting-warningsfrom
fix/pragmatic-autoscroll-spec-fixtures
Open

Model a scroll container in the drag-and-drop spec fixtures#24622
myabc wants to merge 1 commit into
fix/vitest-mock-hoisting-warningsfrom
fix/pragmatic-autoscroll-spec-fixtures

Conversation

@myabc

@myabc myabc commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Ticket

No work package — frontend test-output hygiene, noticed while working on #AGILE-361.

Stacked on #24621, which is itself stacked on #24525.

What are you trying to accomplish?

Removes the ~43 Auto scrolling has been attached to an element that appears not to be scrollable lines from the frontend Vitest output, each one dragging a full DOM dump behind it.

Unlike the hoisting warnings in the PR below this one, these are accurate. Pragmatic's warning is dev-only, fires at autoScrollForElements() registration, and tests computed overflow-x/overflow-y for auto/scroll — it never measures real scrollability, and there is no opt-out flag.

In production, sortable-lists.directive.ts:232 resolves scrollContainer() ?? closestScrollableAncestorOf(host) ?? undefined, so a real scroll container is always found. In the specs the fixtures sat in a plain container with no scrollable ancestor, so the engine fell back to scrollContainer ?? element (sortable-lists-engine.ts:459) and attached to the list element itself. Pragmatic was correctly reporting that the fixture did not model a scroll container.

So this is not warning-suppression: the fixtures were wrong, and they are now shaped like production.

What approach did you choose and why?

Give the element auto-scroll actually attaches to a computed overflow-y: auto, with no height.

Because Pragmatic checks computed overflow and nothing else, that is sufficient. It also leaves layout alone apart from establishing a block formatting context — which matters here, because these specs assert drop edges and pointer offsets from element rects. A height cap, or overflow-y: scroll's reserved scrollbar gutter, would have moved the numbers those assertions depend on.

The attachment point differs per spec, so it was resolved file by file rather than blanket-applied:

  • sortable-lists.directive.behavior.spec.ts — the directive walks up to the closest scrollable ancestor, so nine host templates gained a .scroll-host wrapper.
  • drag-and-drop.service.spec.ts — this service registers on the container it is handed, with no ancestor walk, so the container itself takes the overflow.
  • wp-card-drag-and-drop.service.spec.ts — one wrapper covers the root and both card lists; each one's ancestor walk climbs to it.
  • sortable-lists-engine.preview.spec.ts — calls createSortableRoot directly, so the root itself takes the overflow.

What the exempt fixtures are, and why they still warn nothing

sortable-lists.directive.behavior.spec.ts contains it('warns when the collapsed root has neither an input nor a scrollable ancestor'), which spies on console.warn for the literal 'not to be scrollable'. Its three fixtures deliberately have no scrollable ancestor, and they are untouched here — giving them one would have left that test passing while proving nothing. It is silent in the run output because the spy swallows the warning, not because it stopped happening.

The explicit-list scroll-container fallback describe is likewise untouched: its .ancestor and .scrollable-list elements are the subject of its own data-auto-scrollable assertions.

A constraint for DREAM-786's deferred scroll-container decision

The obvious version of this change — one shared wrapper per fixture — collided with a real engine limit, worth recording even though nothing hits it today.

In the two nested-root fixtures a single wrapper made the outer and inner opSortableLists roots resolve the same element. scrollRegistrations is scoped per root (sortable-lists-engine.ts:150) and nothing dedupes across roots, so Pragmatic's element-keyed registry took two registrations on one node — its "You have already registered autoScrolling on the same element" warning, whose second-order effect the engine's own comment at :142-149 spells out: the survivor silently loses autoscroll once either side cleans up. The fixtures resolve it with a nested .inner-scroll-host so each root gets a distinct ancestor.

To be clear about the blast radius: no consumer is exposed. Every surface has exactly one root per page — Backlogs (one root, two distinct sortable-lists--scrollable columns), Boards (addScrollContainer pointing at the root's own element, which is the case the engine comment was written for), and draggable-autocompleter (single instance in all seven call sites). Within one root the dedupe works. So this is not a latent production bug and doesn't warrant a ticket of its own.

It is, however, a constraint on the decision #24504 defers — "wiring sortable-lists--scrollable needs a scroll-container decision". That page is single-root by design (DREAM-786's first acceptance criterion is one root hosting both item types), so it is safe as drawn. Two things to keep in view when the decision is made: it stops being safe if a second root ever lands inside the same scrollable ancestor, and Backlogs' outlet selector — its only precedent — is page-scoped (#backlogs_container [data-controller~='sortable-lists--scrollable']), so a second root anywhere under that container would match both scrollable elements and collide without any nesting involved. Scoping the selector to the root's own subtree costs nothing now.

Merge checklist

  • Added/updated tests — test-only change; all 167 files / 1548 tests still pass
  • Added/updated documentation in Lookbook (patterns, previews, etc) — n/a
  • Tested major browsers (Chrome, Firefox, Edge, ...) — verified on chromium only; see below

Verification

Full suite run three times on chromium.

  • 167 files / 1548 tests passing on every run
  • Auto scrolling has been attached 0 / 0 / 0
  • already registered autoScrolling 0 / 0 / 0
  • Warning: A vi. 0 / 0 / 0 (from the PR below)

The geometry-sensitive assertions are the canary for this change and were left untouched: the preview spec's exact pointer offsets ({ x: 12, y: 18 } and the clamped { x: 100, y: 20 }), and the container-append test's drop-edge maths.

"Warning-free" here means the two targeted families. Still present, unchanged from baseline and out of scope: the stimulus-use useDispatch deprecation (10), NG0912 (1), the Lit dev-mode banner (2), and Error updating activities list (2).

Copilot AI lite review requested due to automatic review settings August 5, 2026 18:49
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

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
title in square brackets, e.g. [SLUG-123] My title here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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: auto wrappers.
  • 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.

@myabc myabc added needs review javascript Pull requests that update Javascript code labels Aug 5, 2026
@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 529d5c3 to 9d6b6e8 Compare August 5, 2026 23:26
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24622, linked for reference only):

- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]`

Treat this as a standalone task, unrelated to PR #24622. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24622 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 9d6b6e8 to 2cab1f4 Compare August 6, 2026 08:22
@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 2cab1f4 to 8b24961 Compare August 6, 2026 08:56
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]
  • rspec ./spec/features/projects/create_spec.rb[1:12:3:2:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24622, linked for reference only):

- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]`
- `rspec ./spec/features/projects/create_spec.rb[1:12:3:2:1]`

Treat this as a standalone task, unrelated to PR #24622. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24622 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 8b24961 to 177a1bf Compare August 6, 2026 09:27
@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 177a1bf to 16506df Compare August 6, 2026 10:20
@myabc myabc added this to the 17.8.x milestone Aug 6, 2026
@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 16506df to 2064209 Compare August 6, 2026 12:00
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24622, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`

Treat this as a standalone task, unrelated to PR #24622. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24622 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

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.
@myabc
myabc force-pushed the fix/pragmatic-autoscroll-spec-fixtures branch from 2064209 to 75eb667 Compare August 6, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code needs review

Development

Successfully merging this pull request may close these issues.

2 participants