Skip to content

Code Maintenance/STC-462: Resolve #activity-<journal-sequence> anchors on demand dropping eager journal lateral sequencing#23506

Open
akabiru wants to merge 2 commits into
code-maintenance/68063-optimise-db-queries-move-away-from-slow-array-based-pagination-on-the-work-package-activity-tabfrom
code-maintenance/68063-defer-sequence-version-to-anchor-resolution
Open

Code Maintenance/STC-462: Resolve #activity-<journal-sequence> anchors on demand dropping eager journal lateral sequencing#23506
akabiru wants to merge 2 commits into
code-maintenance/68063-optimise-db-queries-move-away-from-slow-array-based-pagination-on-the-work-package-activity-tabfrom
code-maintenance/68063-defer-sequence-version-to-anchor-resolution

Conversation

@akabiru
Copy link
Copy Markdown
Member

@akabiru akabiru commented Jun 2, 2026

Ticket

https://community.openproject.org/wp/STC-462

This builds on:

What are you trying to accomplish?

Removes a ROW_NUMBER() OVER (...) JOIN LATERAL window function aka .with_sequence_version (~130ms/page on long histories) from the activity-tab's common render path. The legacy #activity-N anchor is now treated as a backwards-compatible alias and resolved lazily: when such a deep link arrives, the server resolves it to a journal id once, emits it as a resolved-comment-id Stimulus value, and the client canonicalises #activity-N → #comment- in the address bar.

On a work package with 702 journals, the page-slice query goes from ~130ms to a few ms:

--- Before — on every render:
  Journal Load (130.5ms)  SELECT * FROM journals
    JOIN LATERAL (
      SELECT journals_rank.id,
             ROW_NUMBER() OVER (ORDER BY version ASC) sequence_version
      FROM journals journals_rank
      WHERE journals_rank.journable_id   = journals.journable_id
        AND journals_rank.journable_type = journals.journable_type
    ) ranked ON ranked.id = journals.id
    WHEREORDER BY journals.created_at DESC, journals.id DESC LIMIT 20

--- After — same render, no window function:
  Journal Load (3.0ms)  SELECT journals.id, journals.journable_id, … FROM journals
    WHEREORDER BY journals.created_at DESC, journals.id DESC LIMIT 20
activity-N.mp4

Merge checklist

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

The work package activity tab computed a per-journal sequence_version on
every render — a ROW_NUMBER() window function over a LATERAL join — only to
stamp the legacy data-anchor-activity-id that #activity-N deep links rely on.
Nothing mints those links anymore; copy and share links use
#comment-<journal id>, which needs no extra query.

The activity number is now resolved on demand. Only a request carrying
?anchor=activity-N runs the window function, mapping the number to a journal
id the paginator exposes as resolved_anchor. The view hands that to the
client, which rewrites #activity-N to the canonical #comment-<id> and scrolls
using the comment anchor already present in the DOM. Default renders no longer
touch the window function.

References WP #68063.
@akabiru akabiru force-pushed the code-maintenance/68063-defer-sequence-version-to-anchor-resolution branch from 8c5ae96 to 0eff428 Compare June 2, 2026 07:32
@akabiru akabiru self-assigned this Jun 2, 2026
@akabiru akabiru changed the title Code Maintenance/68063: Resolve activity anchors on demand to drop a per-render query Code Maintenance/68063: Resolve #activity-<journal-sequence> anchors on demand dropping eager journal lateral sequencing Jun 2, 2026
@akabiru akabiru marked this pull request as ready for review June 2, 2026 11:23
@akabiru akabiru requested review from Copilot, myabc and ulferts June 2, 2026 11:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Drops the ROW_NUMBER() OVER (...) JOIN LATERAL window-function join from the activity-tab's default render path. The legacy #activity-<sequence> URL is now treated as a backwards-compatible alias: when an activity-N anchor arrives, the server resolves it to a journal id once, exposes it as a Stimulus value (resolved-comment-id), and the client rewrites #activity-N to #comment-<id> in the address bar before scrolling. Journal item DOM no longer carries data-anchor-activity-id, only data-anchor-comment-id.

Changes:

  • Paginator no longer applies with_sequence_version to the page slice; it only invokes the window function when an activity-N anchor needs resolution, and exposes the resolved comment id via a new resolved_anchor reader.
  • Controller, lazy index component, and journal item templates are updated to drop the sequence_version join from the default path and surface resolved_anchor as a Stimulus value to the client.
  • Frontend UrlHelpers.canonicalActivityAnchor and AutoScrollingController.canonicalizeActivityAnchor translate the legacy hash to the canonical comment hash via history.replaceState, with corresponding unit, request, and feature specs.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
app/services/work_packages/activities_tab/paginator.rb Removes with_sequence_version from the page slice and records the resolved comment id only when an activity anchor is used.
app/controllers/work_packages/activities_tab_controller.rb Forwards resolved_anchor to the lazy index component; drops with_sequence_version from find_journal and time-based update paths.
app/components/work_packages/activities_tab/lazy_index_component.rb Accepts resolved_anchor and emits it as the resolved-comment-id Stimulus value when set.
app/components/.../journals/item_component(.html.erb, /details.rb) Drops the now-unused data-anchor-activity-id DOM attribute.
frontend/.../services/url-helpers.ts (+ spec) Adds canonicalActivityAnchor to map an unresolved or resolved activity anchor to a comment anchor.
frontend/.../auto-scrolling.controller.ts Reads the new Stimulus value, canonicalises the URL hash via replaceState, and bails out when an activity anchor is unresolved.
spec/services/.../paginator_spec.rb Asserts resolved_anchor semantics and that the window function is only run for activity anchors.
spec/requests/work_packages/activities_tab_spec.rb New request spec covering the rendered Stimulus value attribute under all three anchor cases.
spec/features/.../activities_spec.rb Adds a feature-level assertion that the URL is rewritten to #comment-<id> while preserving the work-package path.

The Paginator.paginate class method bypassed the instance, discarding the
resolved_anchor state the controller reads after .call; it had no callers, so
drop it and keep the single new(...).call entry point. Extract the activity
anchor side effect into record_resolved_anchor so the intent is explicit at the
call site, and pin the server contract with a request spec asserting an
unresolvable activity anchor omits the resolved-comment value.
@akabiru akabiru changed the title Code Maintenance/68063: Resolve #activity-<journal-sequence> anchors on demand dropping eager journal lateral sequencing Code Maintenance/STC-462: Resolve #activity-<journal-sequence> anchors on demand dropping eager journal lateral sequencing Jun 2, 2026
@akabiru akabiru requested a review from a team June 3, 2026 05:34
@akabiru akabiru force-pushed the code-maintenance/68063-defer-sequence-version-to-anchor-resolution branch from a01c952 to 3533fec Compare June 4, 2026 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants