Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The importer’s attribution selection and ordering logic introduces a couple of correctness/maintainability issues and the new attribution behavior for image_element/hero_image is not clearly covered by tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the manuscript schema and JATS import/export pipeline to model image attributions as attribution child nodes (instead of a figure_element attribute), and expands attribution support to image_element and hero_image.
Changes:
- Allow
attribution*children onimage_elementandhero_imagenodes. - Remove
figure_element.attrs.attributionand switch JATS export to serializeattributionchild nodes. - Update the JATS importer to parse
<attrib>intoattributionnodes for relevant image contexts and preserve correct child ordering.
File summaries
| File | Description |
|---|---|
| src/schema/nodes/image_element.ts | Updates the node content expression to allow attribution* under image_element. |
| src/schema/nodes/hero_image.ts | Updates the node content expression to allow attribution* under hero_image. |
| src/schema/nodes/figure_element.ts | Removes legacy attribution attribute from figure_element in favor of attribution child nodes. |
| src/jats/importer/jats-dom-parser.ts | Parses <attrib> for image-related nodes and ensures caption ordering relative to attributions. |
| src/jats/exporter/jats-exporter.ts | Exports attribution child nodes as <attrib> (including for image_element graphics). |
| src/jats/tests/snapshots/jats-importer.test.ts.snap | Updates snapshots to reflect the schema change (attribution no longer stored in figure_element attrs). |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| export const heroImage: NodeSpec = { | ||
| content: 'figure? alt_text long_desc', | ||
| content: 'figure? attribution* alt_text long_desc', |
There was a problem hiding this comment.
is "*" what we want here? can't it be "?"?
There was a problem hiding this comment.
yes we can have one, and actually the example in JATS spec shows multiple sources in the same attrib tag. updated and as the JATS Content Model allows us to have multiple attrib so in that case I will merge them if that is ok in one node
There was a problem hiding this comment.
I think you meant that we also need to do that update to figure_element and image_element
| ) | ||
| const fig = this.getFigContent(element).content | ||
| return Fragment.from([...fig.slice(0, 1), caption, ...fig.slice(1)]) | ||
| const attributionIndex = findLastIndex( |
There was a problem hiding this comment.
can you explain what this code does?
There was a problem hiding this comment.
That was to place the attribution in the right place in the PM schema, I reverted it as we moved the attribution to be after the caption. It will be simple now as the placements of the content in image_element and hero_image will be the same
| node: 'attribution', | ||
| // we use closest() instead of `context` as it matches ProseMirror's parse-time node stack, not real DOM ancestry. | ||
| // Non-matching <attrib> (e.g. in <verse-group>) falls to `skip: true` below. | ||
| getAttrs: (node) => { |
There was a problem hiding this comment.
I don't think the complexity is really needed. What would be the problem if we just parse attrib as an attribution node all the time?
There was a problem hiding this comment.
updated with context
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues remain in migration, DOM parsing, and JATS attribution ordering.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/jats/importer/jats-dom-parser.ts:283
- Please add round-trip coverage for an
<attrib>inside a standalone<graphic>and the leading/hero<graphic>. The existing fixtures exercise direct<fig>attributions, but the hero fixture contains onlyalt-textandlong-desc, so the new image/hero parsing and serialization paths can regress without a test failure.
const attribution = element.querySelector('attrib')
if (attribution) {
const attributionNode = this.schema.nodes.attribution.create()
content.push(this.parse(attribution, { topNode: attributionNode }))
}
src/schema/migration/migration-scripts/4.5.7.ts:42
- The reducer prepends the previously accumulated content after each current attribution, reversing multiple legacy credits. For input attributions
[A, B], the migrated node containsBfollowed byA, so the displayed attribution order changes; accumulate the prior content before the current content.
(n, attrib) => ({
...attrib,
content: [...(attrib.content || []), ...(n.content || [])],
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
| const $attrib = this.createElement('attrib') | ||
| $attrib.textContent = node.attrs.attribution.literal | ||
| $element.appendChild($attrib) | ||
| const appendAttribution = ($element: Element, node: ManuscriptNode) => { |
There was a problem hiding this comment.
do we need this, or can we use appendChildNodeOfType?
| ) => { | ||
| const parents = [ | ||
| ...doc.querySelectorAll( | ||
| 'fig:has(> attrib), graphic:not(fig graphic):has(> attrib)' |
There was a problem hiding this comment.
is there a reason why specify the node types? did *:has(> attrib) not work?
There was a problem hiding this comment.
we could have attribution for other node types that we still don't support yet, so I restricted that to the types we need to support. But I think that will be a general behavior to have a single attrib node, so I updated it to get all parents
|
Published version |
Image_elementandhero_imagefigure_elementattribute as we have that already as a node