Render work-package mentions with semantic identifiers#113
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns the CKEditor CommonMark processor and mention/quickinfo insertion flow with OpenProject’s new “semantic” work package identifiers (e.g., #PROJ-7, ##PROJ-7, ###PROJ-7) so references round-trip consistently between markdown source, editor model, and rendered elements.
Changes:
- Extend the CommonMark inline WP reference matcher to recognize semantic identifiers in addition to numeric IDs.
- Rename the mention payload field from
idNumbertodisplayIdand propagate it through mention casting and quickinfo insertion. - Update work package autocomplete insertion to use
wp.displayId(with fallback towp.id) and add Jest fixtures for semantic IDs + boundary cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/commonmark/work-package-refs.test.js | Adds coverage for semantic WP reference upcast/round-trip and boundary handling. |
| src/plugins/op-macro-wp-quickinfo/op-macro-wp-quickinfo-plugin.js | Uses mention.displayId when creating quickinfo widgets so data-id carries the user-facing identifier. |
| src/mentions/work-package-mentions.js | Uses wp.displayId (fallback wp.id) when building inserted mention IDs/links for WPs. |
| src/mentions/user-mentions.js | Renames idNumber → displayId for consistency with the mention contract. |
| src/mentions/mentions-caster.js | Treats data-id as displayId end-to-end (upcast + downcast) and uses it for link generation. |
| src/commonmark/commonmarkdataprocessor.js | Expands WP_REF_RE to match semantic IDs and emits them into data-id. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`opf/commonmark-ckeditor-build#113` makes the editor emit `data-id="PROJ-N"` (the displayId) for work-package mentions in semantic mode. The previous resolver path regex-extracted digits from `data-id` and called `find_by(id:)`, which either mis-resolved ("PROJ-1" → id 1) or raised `UnsupportedLookup`. Resolve via `find_by_display_id` instead — the canonical "either shape" resolver per the convention in `app/models/work_package/semantic_identifier/finder_methods.rb`. Semantic shapes are rejected in classic mode so the filter matches `LinkHandlers::WorkPackages`: a `#PROJ-1` reference is only meaningful when the instance is configured for it. User and group mention paths keep the existing `mention_id` regex extraction — they only carry numeric primary keys.
`opf/commonmark-ckeditor-build#113` makes the editor emit `data-id="PROJ-N"` (the displayId) for work-package mentions in semantic mode. The previous resolver path regex-extracted digits from `data-id` and called `find_by(id:)`, which either mis-resolved ("PROJ-1" → id 1) or raised `UnsupportedLookup`. Route the work-package branch through a new `resolve_work_package_mention` that reads the raw `data-id`, gates semantic shapes on the mode setting (matching `LinkHandlers::WorkPackages`), and resolves via `find_by_display_id` — the canonical "either shape" resolver per the convention in `app/models/work_package/semantic_identifier/finder_methods.rb`. User and group mention paths keep the existing `mention_id` regex extraction — they only carry numeric primary keys. Spec coverage extends to: semantic `data-id` in semantic mode renders the link / quickinfo; classic mode falls through to literal text; unresolvable identifiers degrade gracefully; and a classic-mode case where the WP itself carries a semantic identifier pins that labels and URLs key off the mode, not the record state.
`opf/commonmark-ckeditor-build#113` makes the editor emit `data-id="PROJ-N"` (the displayId) for work-package mentions in semantic mode. The previous resolver path regex-extracted digits from `data-id` and called `find_by(id:)`, which either mis-resolved ("PROJ-1" → id 1) or raised `UnsupportedLookup`. Route the work-package branch through a new `resolve_work_package_mention` that reads the raw `data-id`, gates semantic shapes on the mode setting (matching `LinkHandlers::WorkPackages`), and resolves via `find_by_display_id` — the canonical "either shape" resolver per the convention in `app/models/work_package/semantic_identifier/finder_methods.rb`. User and group mention paths keep the existing `mention_id` regex extraction — they only carry numeric primary keys. Spec coverage extends to: semantic `data-id` in semantic mode renders the link / quickinfo; classic mode falls through to literal text; unresolvable identifiers degrade gracefully; and a classic-mode case where the WP itself carries a semantic identifier pins that labels and URLs key off the mode, not the record state.
Picks up the matched changes in opf/commonmark-ckeditor-build#113: work-package mention elements now carry the numeric primary key in `data-id` and the user-facing identifier in a new `data-display-id` sidecar. Upcast reads both; downcast emits `data-display-id` only when the model carries one.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: autocomplete-picked `#`/`##`/`###` work-package mentions now persist as `<mention>` HTML in markdown rather than collapsing `##`/`###` to bare `##ID` / `###ID` strings. The numeric PK in `data-id` is the stable identifier; `data-display-id` records the displayId at write time; backend `MentionFilter` still routes 2x/3x hash text to the quickinfo macro on render. Source-typed `##N` / `###N` still parse to the quickinfo widget and round-trip back to the bare markdown form. Source-typed bare `#N` is no longer promoted by the parser.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the markdown parser no longer rewrites bare `#N` / `##N` / `###N` text into inline view elements, so reloading a stored `<mention>` envelope through the editor preserves the full element on round-trip instead of collapsing it to plain markdown.
Picks up the conditional afterInit in opf/commonmark-ckeditor-build#113: classic-mode `##` / `###` autocomplete picks (pk equal to displayId) insert a quickinfo widget and persist as bare `##ID` / `###ID` markdown; semantic-mode picks fall through to the standard mention command and persist as the structured `<mention>` envelope with the numeric PK in data-id.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: classic- mode `#N` / `##N` / `###N` references go back to master's behaviour (parser promotes, mentions turndown emits bare `data-text`). Semantic- mode references stay structured — the `<mention>` envelope is preserved on save when `data-display-id` diverges from `data-id`.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: `##PROJ-7` and `###PROJ-7` autocomplete picks now render as quickinfo widgets in the editor (matching classic-mode UX) and persist as `<mention>` envelopes so the numeric PK survives a rename. Source-typed semantic shorthand also previews as a widget; storage falls back to bare markdown since no PK is available at parse time.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the quickinfo-envelope predicate now lives in a single helper consumed by both the widget upcast and the mention caster, and the widget editing view carries `data-pk` when the numeric PK differs from the displayed identifier (so devtools surface which widgets will round-trip as `<mention>` envelopes).
Picks up the follow-up in opf/commonmark-ckeditor-build#113: `op-macro-wp-quickinfo` model attributes renamed from `id` / `displayId` to `wpId` / `wpDisplayId`. Mirrors master's `wpId` convention and removes any reader ambiguity around the bare `id` name. Wire attributes (`data-id`, `data-display-id`) unchanged.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the `data-display-id` sidecar is now only emitted on work-package mentions (the type whose displayed identifier can diverge from the record id), so user and group mentions stop accreting a duplicate-id sidecar each time legacy stored markdown is re-saved through the editor.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the inline-rule guard that protects stored `<mention>` envelopes from being re-promoted on reload drops its depth counter (mentions don't nest), switches to `startsWith` over a per-iteration regex, and reads under a name that describes the situation rather than the mechanism.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the mention feed item and mention-attribute object now call the work-package id `wpId` — matching the widget model attribute, the wire `data-id`, and the backend's `id`. The lone `recordId` outlier is gone.
Picks up the follow-up in opf/commonmark-ckeditor-build#113: the mention feed and the mention-attribute object now carry `dataId` / `dataDisplayId` instead of `wpId` / `displayId`. The user-mention feed stops defining an inert `displayId = mention.id`; the caster emits `data-display-id` on presence rather than type.
|
I am confused: This PR mentions a rejected implementation chunk as its WP. |
Looks like @akabiru corrected the link two hours ago. |
thykel
left a comment
There was a problem hiding this comment.
🚀 Cautious soft-approval from me -- I'd love someone more familiar with the component to take a stab at this as well.
Widens the parser's inline reference regex from `\d+` to `\d+|[A-Z][A-Z0-9_]*-\d+`, mirroring the route constraint on the backend side. Boundary checks already in place keep mid-word matches, `#PROJ-1abc`, trailing-dash, and lowercase out of the promotion path. A token-walk guard skips promotion when the match sits inside an unclosed `<mention>` opening tag — stored mention envelopes (`<mention data-text="##KSTP-2">…</mention>`) get re-tokenised as open / inner-text / close, and without the guard the inline rule would rewrite the inner text and break the envelope on reload. The mentions turndown rule preserves a work-package mention's full HTML envelope whenever `data-display-id` is present (autocomplete picks and round-tripped envelopes) and collapses it to bare `data-text` markdown otherwise (parser-emitted single-hash shorthand, legacy single-attribute envelopes). Round-trip jest fixtures cover the semantic regex, the boundary cases, the envelope round-trip for all three marker lengths, and the legacy single-attribute collapse.
Feed items and the mention-attribute object carry two new fields: `dataId` (the record's primary key — `wp.id` for work packages, `mention.id` for users) and `dataDisplayId` (the user-facing identifier, set only for work-package mentions whose displayed form can diverge from the id). The caster passes them through to `data-id` and `data-display-id` on the wire; the `data-display-id` emission keys off field presence, not type, so user/group mentions stop accreting a redundant duplicate-id attribute. The work-package autocomplete uses `wp.displayId || wp.id` for the inserted marker text and the editor-view `<a href>`, giving mode-appropriate URLs that mirror what the backend renders. The user-mention feed drops the inert `displayId = mention.id` it was defining just to satisfy the previous caster contract. The caster's link generation prefers `dataDisplayId || dataId` so the editor-view URL matches the codebase convention for rendered URLs (`display_id` everywhere). Identity stability lives in `data-id`; backend re-resolves `display_id` on every render through `MentionFilter`.
Extends the quickinfo widget plugin so the `##` / `###` autocomplete path lands a widget regardless of mode (matching the UX shipped in #112 for classic mode). The model element gains `wpId` and `markerText` alongside `wpDisplayId` and `detailed`; autocomplete picks set all four, source-typed widgets set the two display-side ones. The data downcast emits a `<mention>` envelope when `wpId` is set (autocomplete-picked or upcast-from-envelope) and a bare `<opce-macro-wp-quickinfo>` otherwise (source-typed shorthand). One invariant across both modes: autocomplete persists as an envelope, source-typed shorthand persists as bare markdown. A second upcast routes stored `<mention>` envelopes whose `data-text` carries `##` or `###` into the same widget model, so reopened comments preview as widgets and round-trip the envelope intact. A shared predicate in `predicate.js` keeps that upcast in sync with the mention caster's deferral path.
Renames the local `id` to `markerText` so it stops shadowing the domain notion of "id" (a record's primary key). The feed item's field stays `id` — CKEditor's mention plugin requires the field to be named `id` and to start with the marker prefix. Replaces the `base = urlRoot + '/work_packages/'` helper with a top-level `urlRoot` constant reused for both the auto-complete URL and the per-WP link, removing the string-concat / template-literal mismatch between the two construction sites. Sets `text` explicitly to `markerText` rather than relying on the plugin's `text || id` auto-default. A one-line comment above the return calls out the plugin contract.
The DOM tag (`opce-macro-wp-quickinfo`) was already a constant (`QUICKINFO_TAG`); the CKEditor model element name (`op-macro-wp-quickinfo`) was still a raw string at six call sites — schema registration, three `createElement` calls, and two `model:` keys on the downcast converters. Add a paired `QUICKINFO_MODEL` constant to remove the typo surface and make the model-name / view-tag relationship visible at a glance.
`getMentionLink(...)` computes the URL the editing downcast writes into the `<a href>`. The data downcast doesn't carry it — backend re-renders URLs from the work-package record on every render. A one-line comment makes the persistence-vs-editing distinction visible at the call site. Also drops a stray trailing blank line at the end of the file.
7dcc287 to
9d89eb3
Compare
Ticket
https://community.openproject.org/wp/74949
Paired with core PR opf/openproject#23204 -- both ship together per the build repo's README.
Contract
data-id= work-package id.data-display-id= user-facing identifier (PROJ-7in semantic mode, the numeric id in classic). Same convention on<mention>envelopes and<opce-macro-wp-quickinfo>. Matchesdata-type="user"/"group"mentions, which already used numericdata-id.Storage rule
One invariant across both modes:
<mention>envelope.The presence of a record-id signal (
wpIdon the widget model,data-display-idon the wire) is the discriminator. Autocomplete picks set it; source-typed parses don't.Behaviour matrix
#X/##X/###Xautocomplete (any mode)<mention data-id="<id>" data-display-id="<displayId>" data-text="…">…</mention>#N/#PROJ-Nsource-typed#N/#PROJ-N##N/###Nsource-typed##N/###N<mention data-id="42">(nodata-display-id)#42Notes
\d+|[A-Z][A-Z0-9_]*-\d+. A token-walk guard skips promotion when the match sits inside an unclosed<mention>opening tag, so stored envelopes round-trip without losing their inner text.data-display-idis emitted only on work-package mentions — user/group mentions don't need a second slot.Merge checklist
#PROJ-7,##PROJ-7,###PROJ-7,MACROPROJ-42,MY_PROJ-1, boundary cases (foo#PROJ-1,#PROJ-1abc, trailing dash, lowercase), stored<mention>envelope round-trips for all three marker lengths, legacy single-attribute envelope.npm ci && OPENPROJECT_CORE=/path/to/openproject npm run build. Commit the regeneratedckeditor.jsand verify the diff mentionsdata-display-id.#PROJ-1,##PROJ-1,###PROJ-1→ renders link / widget. Pick via autocomplete → markdown source contains the envelope. Reload, edit, save → round-trip clean.