[OP-19666] Consolidate custom-element morph policy#24039
Draft
myabc wants to merge 7 commits into
Draft
Conversation
The Turbo integration listeners bound to the global `document` and returned `void`, so the most Turbo-upgrade-fragile frontend code had no unit coverage beyond a single Selenium navigation spec. Threads an optional event target and `AbortSignal` through the installers so vitest can drive synthetic `turbo:*` events and tear the listeners down between cases. Adds specs for the seamed installers and all five custom stream actions; callers stay unchanged through the defaults.
The frame-visit monkeypatch reached into Turbo internals behind `@ts-nocheck` and a blanket eslint-disable, so neither the compiler nor a test would notice a Turbo upgrade renaming the method or fixing hotwired/turbo#1300 underneath it. Documents the single divergence from upstream -- sourcing the history snapshot from the live document rather than the fetch response -- pins it to Turbo 8.0.x, and adds a tripwire spec that fails on either upgrade so the patch is re-evaluated instead of silently breaking.
The vitest tripwire guards the patched method's upstream shape but not its runtime effect. This adds the behavioural half on the departments page, whose content area is a `data-turbo-action="advance"` frame: advancing through the tree and pressing back must restore the parent frame. Without `turbo-navigation-patch.ts` the URL reverts while the frame keeps the child content, so the exact breadcrumb match fails. The bug only reproduces with Rails-rendered frames, which is why it cannot live in the vitest suite.
The wrapper destroys and re-bootstraps the whole Angular root on every Turbo navigation -- the most consequential frontend/src/turbo/ installer yet the only one left without coverage. Threads the same (target, signal) seam and injects the plugin-context lookup and bootstrap call so vitest can drive synthetic turbo:load events. The RxJS skip(1) pipeline and destroy-then-bootstrap contract are unchanged; callers stay on the defaults.
The OPCE-* tag-name check used to decide whether Turbo's idiomorph may morph an element was duplicated in turbo-global-listeners.ts and dialog/preview.controller.ts, with no test covering either. Gives both a single, tested source: isOpenProjectCustomElement(node). Additive only -- nothing calls this yet.
Replaces the inline OPCE-* tag check with isOpenProjectCustomElement, dropping a redundant toUpperCase() call (Element.tagName is already uppercase). Behaviour is unchanged -- the existing morph-guard specs are the regression net for this swap.
Replaces DialogPreviewController's inline OPCE-* tag check with isOpenProjectCustomElement, shared with turbo-global-listeners.ts. The scheduling-changed replace exception is untouched -- it is date-picker domain logic, not generic morph policy. Wraps the date-picker preview spec's fixture in a <turbo-frame> and adds the coverage this morph path never had: an OPCE-* node is skipped or replaced correctly, a plain node always morphs when not adjacent to a replaced OPCE-* node. A pre-existing bug where a sibling following a replaced OPCE-* node is left stale is tracked separately as OP-19669, not fixed here.
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates the “OPCE-* custom element” detection used by Turbo/Idiomorph morph guards into a single shared predicate, then routes the existing global and dialog-level morph policies through it, adding unit coverage for the new predicate and for the dialog morph behavior (date-picker preview).
Changes:
- Added
isOpenProjectCustomElement(node: Node)as the single predicate for OP custom elements. - Updated Turbo global
turbo:before-morph-elementprevention logic to use the shared predicate. - Updated dialog preview morph callback to use the shared predicate and added date-picker controller spec coverage for the OPCE morph behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/turbo/turbo-global-listeners.ts | Uses shared predicate for the global Turbo morph preventDefault guard. |
| frontend/src/turbo/openproject-custom-element.ts | Introduces shared isOpenProjectCustomElement(Node) predicate. |
| frontend/src/turbo/openproject-custom-element.spec.ts | Adds vitest coverage for the shared predicate (Element/non-Element cases). |
| frontend/src/stimulus/controllers/dynamic/work-packages/dialog/preview.controller.ts | Uses shared predicate in Idiomorph beforeNodeMorphed callback. |
| frontend/src/stimulus/controllers/dynamic/work-packages/date-picker/preview.controller.spec.ts | Adds coverage for dialog morph behavior (skip vs replace OPCE nodes). |
Comment on lines
+130
to
+132
| const turboFrame = ctx.container.querySelector('turbo-frame')!; | ||
| turboFrame.setAttribute('src', '/work_packages/123/dialog/preview'); | ||
|
|
Comment on lines
+148
to
+150
| const turboFrame = ctx.container.querySelector('turbo-frame')!; | ||
| turboFrame.setAttribute('src', '/work_packages/123/dialog/preview?schedule_manually=true'); | ||
|
|
myabc
force-pushed
the
code-maintenance/OP-19617-turbo-test-seam
branch
from
b660548 to
a6d0e55
Compare
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.
Ticket
https://community.openproject.org/wp/OP-19666
What are you trying to accomplish?
OP-19666is Part B of the Turbo cleanup consolidation started in OP-19618 (Part A, CSP nonce enforcement, already merged into this stack). Two independent files each re-implemented the sameOPCE-*custom-element tag-name check to decide whether Turbo's idiomorph is allowed to morph an element:turbo-global-listeners.ts(a globalpreventDefault()guard) anddialog/preview.controller.ts(a frame-localbeforeNodeMorphedcallback with an extra scheduling-change exception). Neither the shared logic nor, in the controller's case, any of the surrounding morph behaviour had test coverage.Investigation also found two other Turbo morph mechanisms that looked related but solve genuinely different problems —
action-menu-morph-remount.ts(post-morph remount, not a skip) and the not-yet-merged PR #23218'spragmatic-dnd-morph-attributes.ts(attribute-level preservation, different event). Both are intentionally out of scope; forcing them into one shared interface would bend that interface to fit cases it doesn't describe.What approach did you choose and why?
Three commits:
Add an OpenProject custom element predicate. New
frontend/src/turbo/openproject-custom-element.tsexportsisOpenProjectCustomElement(node: Node): boolean— the single source of truth for "is this one of our custom elements." Signature takesNode, notElement, because idiomorph's morph callback can hand it non-Element nodes (e.g. Text nodes); the predicate guards internally withinstanceof Elementso neither call site needs its own defensive check. Purely additive, TDD, 3 tests.Route the morph guard through the shared predicate.
turbo-global-listeners.tsswaps its inline check for the shared predicate, dropping a redundant.toUpperCase()call (Element.tagNameis already uppercase). Behaviour-preserving; the two existing morph-guard tests (from OP-19617) are the regression net.Route the dialog morph guard through the shared predicate.
DialogPreviewController(the abstract base shared bydate-picker/andprogress/preview controllers) swaps its inline check too. The scheduling-changed replace exception is untouched — it's date-picker domain logic, not generic morph policy. Adds coverage this path never had.While building that new coverage, a genuine pre-existing bug surfaced:
oldNode.replaceWith(newNode)inside the scheduling-changed branch detaches the old node from the DOM, which breaks idiomorph's sibling-walk bookkeeping — any DOM sibling following a replacedOPCE-*node is silently left stale. Confirmed pre-existing ondev(unrelated to this consolidation), traced against idiomorph's source, verified empirically. Filed separately as https://community.openproject.org/wp/OP-19669 rather than folded into this branch — the test for that path asserts only the (correct)OPCE-*replacement, with a comment pointing at the ticket for the known sibling-morph gap.Merge checklist