[COMMS-889] Fix the work package URL if it does not use the canoncial identifier of a work package by judithroth · Pull Request #23818 · opf/openproject

judithroth · 2026-06-19T12:10:02Z

https://community.openproject.org/wp/COMMS-889

Since the correct page is loaded already and only the URL in the browser uses an outdated identifier, the URL is rewritten by a little javascript helper.

This has the added benefit of also "auto-fixing" bookmarks of old identifiers or fixing the URL if a numeric work package ID was used while the instance is in semantic mode or vice versa. No redirects needed.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

Copilot

Pull request overview

This PR addresses STC-835 by ensuring that when a work package page is already correctly loaded but the browser URL contains an outdated work package identifier, the URL is rewritten client-side to match the canonical identifier (avoiding redirects and helping “auto-fix” old bookmarks).

Changes:

Adds a Turbo global listener helper that compares the current /work_packages/:id segment with the page’s <link rel="canonical"> and rewrites the URL via history.replaceState.
Adds a new unit spec covering several rewrite/no-rewrite cases, including preserving query/hash.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
frontend/src/turbo/turbo-global-listeners.ts	Adds the canonicalization helper and invokes it on Turbo render / DOMContentLoaded.
frontend/src/turbo/turbo-global-listeners.spec.ts	Adds unit tests validating URL rewrite behavior against the canonical link.

github-actions · 2026-06-19T13:50:25Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/backlogs/create_spec.rb[1:3:2]

🤖 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 #23818, linked for reference only):

- `rspec ./modules/backlogs/spec/features/backlogs/create_spec.rb[1:3:2]`

Treat this as a standalone task, unrelated to PR #23818. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23818 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 @judithroth 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 @judithroth, and request a review from @judithroth.
On every commit, set @judithroth 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.

github-actions · 2026-06-19T14:52:27Z

Warning

Flaky specs

rspec ./modules/team_planner/spec/features/team_planner_overview_spec.rb[1:4:4: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 #23818, linked for reference only):

- `rspec ./modules/team_planner/spec/features/team_planner_overview_spec.rb[1:4:4:1]`

Treat this as a standalone task, unrelated to PR #23818. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23818 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 @judithroth 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 @judithroth, and request a review from @judithroth.
On every commit, set @judithroth 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.

judithroth · 2026-06-19T14:58:00Z

+      expect(page).to have_current_path(semantic_path("relations"))
+    end
+  end
+end


@myabc if you have an idea on what should be tested additionally, please let me know 🙂 The calendar service turned out to be independent of the routes that are rewritten (as far as I understand it)

github-actions · 2026-06-19T15:45:57Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/backlogs/create_spec.rb[1:3:2]

🤖 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 #23818, linked for reference only):

- `rspec ./modules/backlogs/spec/features/backlogs/create_spec.rb[1:3:2]`

Treat this as a standalone task, unrelated to PR #23818. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23818 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 @judithroth 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 @judithroth, and request a review from @judithroth.
On every commit, set @judithroth 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

@judithroth I'm sorry for the glacial pace of my review. I've since introduced some basic spec coverage for the existing global listeners (#23980), which will unfortunately necessitate rebasing/updating this PR.

myabc · 2026-07-02T21:56:44Z

+
+export function canonicalizeWorkPackageIdInUrl():void {
+  const currentPath = window.location.pathname;
+  const wpIdPattern = /\/work_packages\/([^/]+)/;


nitpick: This could make use of the new URLPattern API.

Also consider reusing WP_ID_URL_PATTERN from core-app/shared/helpers/work-package-id-pattern..

Good catch, thanks for pointing it out!

myabc · 2026-07-02T22:05:11Z

+  describe "on initial page load (DOMContentLoaded)" do
+    it "rewrites a numeric work package ID to the canonical semantic ID" do
+      visit numeric_path
+      wait_for_network_idle


You should be able to use the more granular wait_for_turbo if this is a Turbo Drive visit.

myabc · 2026-07-02T22:10:33Z

+    project_work_package_path(project, work_package.reload.identifier, tab)
+  end
+
+  describe "on initial page load (DOMContentLoaded)" do


👍🏻 for covering these different scenarios!

myabc

I haven't finalised my review, but this is the crux of it.

I may need to verify a few things with @HDinger . Thanks again for your patience @judithroth !

myabc · 2026-07-02T22:11:39Z

+      visit project_path(project)
+      wait_for_reload
+
+      page.execute_script("Turbo.visit('#{numeric_path}')")


nit: can this be made more realistic, e.g. by following a numeric link? (perhaps in user-generated content?)

myabc · 2026-07-02T22:37:16Z

+  window.history.replaceState(
+    window.history.state,
+    '',
+    newPath + window.location.search + window.location.hash,
+  );


I've dug into this a bit more and my concerns about calling plain replaceState were somewhat valid. While passing window.history.state through does preserve the restorationIdentifier, Turbo's Session.history.location and view.lastRenderedLocation still end up holding the pre-rewrite URL.

The consequences of this are nuanced and complex (and if weren't for Claude, I doubt I would fully understand them myself). However they can lead to real broken behaviour. In detail:

Snapshot cache is keyed by lastRenderedLocation (the old numeric URL), while back/forward popstate resolves the rewritten canonical URL, leading to cache lookup misses and a full network re-fetch instead of instant restore. The feature spec only asserts the URL and doesn't observe the refresh itself.

Action misclassification at three sites (Visit.changeHistory, #getDefaultAction, PageView.isPageRefresh): links/forms targeting the now-canonical URL get "advance" instead of "replace" (duplicate history entry), and refresh-morph detection breaks.

Suggested change

window.history.replaceState(

window.history.state,

'',

newPath + window.location.search + window.location.hash,

);

const newUrl = new URL(

newPath + window.location.search + window.location.hash,

window.location.origin,

);

// Go through Turbo's history API so Session.history.location stays in

// sync; pass the current restorationIdentifier, as replace() would

// otherwise mint a fresh one and orphan the recorded scroll position.

Turbo.session.history.replace(newUrl, Turbo.session.history.restorationIdentifier);

// turbo:render fires after Turbo has already stamped lastRenderedLocation,

// so correct it too — it keys the snapshot cache and refresh detection.

(Turbo.session as unknown as { view:{ lastRenderedLocation:URL } }).view.lastRenderedLocation = newUrl;

N.B. the Turbo typings look like they are missing lastRenderedLocation - will try to fix upstream...

FYI, typings fix upstream: DefinitelyTyped/DefinitelyTyped#75208

Split into smaller PRs: DefinitelyTyped/DefinitelyTyped#75214

myabc · 2026-07-02T22:39:32Z

+    window.history.pushState({}, '', '/');
+    document.querySelectorAll('link[rel="canonical"]').forEach((element) => element.remove());
+    /* eslint-disable @typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-empty-function */
+    replaceStateSpy = vi.spyOn(window.history, 'replaceState').mockImplementation(() => {});


This should be changed to spy on Turbo.session.history.replace.

myabc · 2026-07-02T22:43:43Z

+    window.history.pushState({}, '', '/');
+    document.querySelectorAll('link[rel="canonical"]').forEach((element) => element.remove());


⚠️ this mutates real browser state and has the potential to cause flaky specs, depending on the order specs are run. We should restore the location and remove the links in an afterEach.

myabc · 2026-07-02T22:46:24Z

+      page.go_back
+      wait_for_network_idle
+
+      expect(page).to have_current_path(semantic_path)


I suspect that this will still pass, even if you remove the canonicalizeWorkPackageIdInUrl() call from runOnRenderAndLoad.. since we're only asserting the URL, this can't detect that the cache has been missed.

https://community.openproject.org/wp/COMMS-889 Since the correct page is loaded already and only the URL in the browser uses an outdated identifier, the URL is rewritten by a little javascript helper. This has the added benefit of also "auto-fixing" bookmarks of old identifiers or fixing the URL if a numeric work package ID was used while the instance is in semantic mode or vice versa. No redirects needed.

https://community.openproject.org/wp/COMMS-889 Turbo Drive calls history.pushState({ turbo: { restorationIdentifier, visitId } }, '', url) before firing turbo:render. Passing null as the state discards that metadata. Turbo uses the restorationIdentifier to restore a cached snapshot on back/forward navigation — without it, Turbo falls back to a full server fetch. The page still loads correctly, but cache-based back/forward performance is lost. Everything else about the implementation is compatible with Turbo navigation: - turbo:render fires after <head> is merged, so the canonical link is already up to date - replaceState (not pushState) corrects the current history entry without adding a new one - Angular's UI Router is restarted on turbo:load (which fires after turbo:render), so it sees the already-corrected URL

https://community.openproject.org/wp/COMMS-889

Copilot AI review requested due to automatic review settings June 19, 2026 12:10

judithroth self-assigned this Jun 19, 2026

Copilot started reviewing on behalf of judithroth June 19, 2026 12:10 View session

Copilot AI reviewed Jun 19, 2026

View reviewed changes

Comment thread frontend/src/turbo/turbo-global-listeners.ts Outdated

Comment thread frontend/src/turbo/turbo-global-listeners.ts Outdated

Comment thread frontend/src/turbo/turbo-global-listeners.ts Outdated

Comment thread frontend/src/turbo/turbo-global-listeners.spec.ts Outdated

judithroth force-pushed the bug/stc-835-short-wp-link-doesn-t-redirect-to-the-current-canonical-url branch from 1fce673 to d85edf1 Compare June 19, 2026 12:23

judithroth changed the title ~~[STC-835] Short WP link doesn't redirect to the current canonical URL~~ [STC-835] Short WP link should open the canonical URL of a work package Jun 19, 2026

judithroth marked this pull request as draft June 19, 2026 13:34

judithroth changed the title ~~[STC-835] Short WP link should open the canonical URL of a work package~~ [STC-835] Correct work package URL if does not use the canoncial identifier of a work package Jun 19, 2026

judithroth requested review from myabc and thykel June 19, 2026 14:55

judithroth commented Jun 19, 2026

View reviewed changes

judithroth marked this pull request as ready for review June 19, 2026 14:58

judithroth marked this pull request as draft June 19, 2026 14:59

judithroth force-pushed the bug/stc-835-short-wp-link-doesn-t-redirect-to-the-current-canonical-url branch from 820957d to d5b4892 Compare June 19, 2026 15:26

judithroth marked this pull request as ready for review June 19, 2026 15:27

judithroth changed the title ~~[STC-835] Correct work package URL if does not use the canoncial identifier of a work package~~ [STC-835] Fix the work package URL if it does not use the canoncial identifier of a work package Jun 22, 2026

judithroth requested a review from a team June 30, 2026 12:10

myabc reviewed Jul 2, 2026

View reviewed changes

myabc requested changes Jul 2, 2026

View reviewed changes

This was referenced Jul 4, 2026

[hotwired__turbo] Align types with the v8.0.23 implementation DefinitelyTyped/DefinitelyTyped#75208

Closed

[hotwired__turbo] Add session.view types DefinitelyTyped/DefinitelyTyped#75214

Merged

judithroth added 3 commits July 10, 2026 20:13

[COMMS-889] Add tests for turbo and angular interactions

dc213ac

https://community.openproject.org/wp/COMMS-889

judithroth added 2 commits July 10, 2026 20:14

[COMMS-889] Adress review feedback

19848d0

https://community.openproject.org/wp/COMMS-889

[COMMS-889] Refactor to match current file structure

e9c5d78

https://community.openproject.org/wp/COMMS-889

judithroth force-pushed the bug/stc-835-short-wp-link-doesn-t-redirect-to-the-current-canonical-url branch from d5b4892 to e9c5d78 Compare July 10, 2026 18:15

[COMMS-889] Add test for reload

8337b62

https://community.openproject.org/wp/COMMS-889

judithroth changed the title ~~[STC-835] Fix the work package URL if it does not use the canoncial identifier of a work package~~ [COMMS-889] Fix the work package URL if it does not use the canoncial identifier of a work package Jul 10, 2026

-  window.history.replaceState(
-    window.history.state,
-    '',
-    newPath + window.location.search + window.location.hash,
-  );
+  const newUrl = new URL(
+    newPath + window.location.search + window.location.hash,
+    window.location.origin,
+  );
+  // Go through Turbo's history API so Session.history.location stays in
+  // sync; pass the current restorationIdentifier, as replace() would
+  // otherwise mint a fresh one and orphan the recorded scroll position.
+  Turbo.session.history.replace(newUrl, Turbo.session.history.restorationIdentifier);
+  // turbo:render fires after Turbo has already stamped lastRenderedLocation,
+  // so correct it too — it keys the snapshot cache and refresh detection.
+  (Turbo.session as unknown as { view:{ lastRenderedLocation:URL } }).view.lastRenderedLocation = newUrl;

		window.history.pushState({}, '', '/');
		document.querySelectorAll('link[rel="canonical"]').forEach((element) => element.remove());

Uh oh!

Conversation

judithroth commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

myabc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myabc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

judithroth commented Jun 19, 2026 •

edited

Loading