Repository navigation
feat(rendering): establish canonical document pipeline - #19
Conversation
📝 WalkthroughWalkthroughPublishing now persists and delivers cache invalidation jobs. Editor saves and publishes now coordinate asynchronous commits and version checks. List semantics support description lists. Rendering uses canonical documents. Stage drag-and-drop supports wrapped, grid, and cancellable interactions. Studio adds scheduled and modified page filters. ChangesPublishing and cache invalidation
Editor persistence and coordination
Semantic lists
Canonical rendering
Stage and Studio updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aria/actions/publishing.ts (1)
309-327: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the job identifier collision-resistant.
aria_cache_invalidation_jobs.idis the unique primary key, butcreatePagePublicationInvalidationJobonly storesidempotencyKey.slice(0, 160). Two distinct page publication keys with long page IDs or versions can truncate to the sameid. TheINSERT ... ON CONFLICT(idempotency_key) DO NOTHINGguard then does not help here, and the same insert is batched with the page publish/unpublish statements. Deriveidfrom the fullidempotencyKey, use a stable hash, or make the primary key safe against truncated prefixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/actions/publishing.ts` around lines 309 - 327, The createPagePublicationInvalidationJob implementation must ensure aria_cache_invalidation_jobs.id remains collision-resistant instead of relying on idempotencyKey.slice(0, 160). Derive the primary-key identifier from the full idempotencyKey using a stable collision-resistant representation, while preserving idempotencyKey for the existing conflict guard and keeping publication invalidation behavior unchanged.
🟡 Minor comments (6)
aria/tests/rendering-foundation/canonical-document.parity.test.ts-162-162 (1)
162-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe ordering assertion passes when
"Header"is absent.
String.prototype.indexOfreturns-1for a missing substring.-1is less than any valid index, so Line 162 passes even when the header region contributes no output. Assert presence before you compare positions.💚 Proposed fix
+ expect(html).toContain("Header"); + expect(html).toContain("Footer"); expect(html.indexOf("Header")).toBeLessThan(html.indexOf("<dl>")); expect(html.indexOf("<dl>")).toBeLessThan(html.indexOf("Footer"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/rendering-foundation/canonical-document.parity.test.ts` at line 162, Update the ordering assertion around the HTML checks to first assert that the expected "Header" content is present, then compare its index with the <dl> index. Ensure the test fails when the header is absent while preserving the existing ordering requirement.aria/lib/rendering/canonical/compileCanonicalNode.ts-181-181 (1)
181-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fragmentdetection does not match the case handling used elsewhere in this file.Line 94 and Line 122 normalize
node.typewithtoLowerCase(). Line 181 compares the raw value. A node whose type is"fragment"is not flattened.compileCanonicalNodethen emits a<fragment>element or returnsnull.Use the same normalization.
🐛 Proposed fix
- if (node.type === "Fragment") { + if (node.type.toLowerCase() === "fragment") { output.push(...compileCanonicalNodes(node.children, options)); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/rendering/canonical/compileCanonicalNode.ts` at line 181, Update the Fragment branch in compileCanonicalNode to normalize node.type with the same toLowerCase() handling used by the checks near lines 94 and 122, so both "Fragment" and "fragment" are flattened consistently.aria/tests/lib/listNodes.test.ts-129-148 (1)
129-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test locks a
dlthat containsdtelements and nodd.
ordinaryListItemToDescriptionChildinaria/lib/blocks/listNodes.ts(line 359) defaults every plain list item to thetermrole. Converting a plain list to description mode therefore produces<dl><dt>…</dt><dt>…</dt></dl>. HTML requires eachdtgroup to be followed by at least onedd, so the result is invalid and screen readers announce terms with no definitions.Confirm this is the intended authoring step. If it is, consider emitting a paired empty
ddfor each converted item so the document stays valid before the author fills it in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/lib/listNodes.test.ts` around lines 129 - 148, Update ordinaryListItemToDescriptionChild in convertListSemanticMode so each plain list item converted to description mode emits its term together with a paired empty description child, producing valid dt/dd groups. Preserve the existing item content and description-list semantics while updating the test expectations to account for the added dd nodes.aria/admin/features/Inspector/composables/useListSemanticMutation.ts-26-30 (1)
26-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUndo restores a normalized node, not the original node.
convertListSemanticModedoes more than set the element. It also runsnormalizeOrdinaryDescriptionPartsand injectsstyles.listStyleType.basewhen the current base value is not a string. SopreviousNodecan differ fromlistNodeeven when the mode is unchanged. After undo, the list can carry alistStyleTypethat the author never set.Capture a plain clone of the current node for the undo path.
🐛 Proposed fix
- const previousNode = convertListSemanticMode( - listNode, - resolveListSemanticMode(listNode), - ); + const previousNode = structuredClone(listNode); const nextNode = convertListSemanticMode(listNode, nextMode);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Inspector/composables/useListSemanticMutation.ts` around lines 26 - 30, Update the undo snapshot in the list semantic mutation flow to capture a plain clone of the current listNode before calling convertListSemanticMode, rather than deriving previousNode through conversion. Keep convertListSemanticMode for creating nextNode, and ensure undo restores the cloned original node without normalization or injected listStyleType values.aria/admin/features/Stage/dragdrop/useCanvasDrop.ts-361-383 (1)
361-383: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winStop the auto-scroll loop when the iframe cannot scroll further.
autoScrollVelocitydepends on pointer position only. If the iframe already sits at a scroll limit, the velocity stays non-zero andstepkeeps rescheduling. Each iteration callsscheduleDropFeedback, which re-runsresolveLibraryDropIntentwithelementsFromPointhit testing and repeated style and rectangle reads. The pointer only needs to rest inside the 56px edge band for this per-frame work to continue for the whole drag.Compare the scroll offset before and after
scrollBy. If it did not change on both axes, stop the loop.🔧 Proposed guard
const step = () => { if (!state.value.isDragging) { autoScrollFrame = null; return; } const x = autoScrollVelocity(point.frameX, iframeWindow.innerWidth); const y = autoScrollVelocity(point.frameY, iframeWindow.innerHeight); if (x === 0 && y === 0) { autoScrollFrame = null; return; } + const beforeX = iframeWindow.scrollX; + const beforeY = iframeWindow.scrollY; iframeWindow.scrollBy(x, y); + if (iframeWindow.scrollX === beforeX && iframeWindow.scrollY === beforeY) { + autoScrollFrame = null; + return; + } scheduleDropFeedback(point); autoScrollFrame = window.requestAnimationFrame(step); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Stage/dragdrop/useCanvasDrop.ts` around lines 361 - 383, Update the step function inside updateAutoScroll to capture the iframeWindow scroll offsets before calling scrollBy, then compare them afterward; when neither axis changes, clear autoScrollFrame and return without scheduling drop feedback or another animation frame. Preserve the existing velocity-zero and drag-state termination paths.aria/tests/cache/public-page-cache.test.ts-28-34 (1)
28-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
purgeMockbefore each test.
purgeMockis a standalonevi.fn(), and the Vitest config does not enableclearMocksorrestoreMocks. AddpurgeMock.mockReset()tobeforeEachso tests after the mock call retain their expected mock state.💚 Proposed fix
describe("public page cache", () => { beforeEach(() => { + purgeMock.mockReset(); purgeMock.mockResolvedValue({ success: true, errors: [] }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/cache/public-page-cache.test.ts` around lines 28 - 34, Reset the standalone purgeMock state in the beforeEach hook by calling purgeMock.mockReset() before configuring its resolved value. Keep the existing mockResolvedValue setup and afterEach restoration unchanged.
🧹 Nitpick comments (23)
aria/tests/rendering-foundation/canonical-document.parity.test.ts (2)
187-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument how to regenerate the pinned
documentHash.Line 188 pins a SHA-256 over every field of the serialized document. Any change to the contract, the renderer output, or the generated CSS breaks this assertion, and the failure message reports only two hex strings. Line 186 and Line 196 already assert determinism and self-consistency.
Add a comment that states the regeneration command, or move the literal to a named constant with that note. Keep the assertion if cross-runtime byte parity is a required guarantee.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/rendering-foundation/canonical-document.parity.test.ts` around lines 187 - 189, Document how to regenerate the pinned documentHash near the assertion in the canonical document parity test, including the exact regeneration command or a named constant with that note. Preserve the existing hash assertion and the determinism and self-consistency checks.
196-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for hash validation and the
freezeoption.The suite asserts only the passing direction. Add cases that confirm
hasValidRenderDocumentHashreturnsfalsefor a document with a modified field, and thatcompileRenderDocumentwithfreeze: falsereturns a mutable document. Both paths are new public behavior in this PR.Do you want me to generate these test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/rendering-foundation/canonical-document.parity.test.ts` around lines 196 - 199, Add negative test coverage in the canonical render document parity suite: modify a field on the document and assert hasValidRenderDocumentHash returns false, and compile a document with freeze: false then assert the returned document is mutable. Keep the existing valid-hash and frozen-document assertions unchanged.aria/lib/rendering/canonical/compileCanonicalNode.ts (2)
136-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
namespaceis not inherited by SVG descendants.Line 136 assigns
"svg"only whentagNameequals"svg". A compiled child such aspath,g, orusereceives"html". Thread the parent namespace throughCompileCanonicalNodeOptionsso descendants of ansvgelement keep thesvgnamespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/rendering/canonical/compileCanonicalNode.ts` at line 136, Update CompileCanonicalNodeOptions and the compileCanonicalNode flow so the parent namespace is passed into child compilation and inherited by descendants. Ensure SVG descendants such as path, g, and use retain the "svg" namespace instead of deriving "html" solely from their own tagName, while preserving the root svg namespace assignment.
141-167: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the tag lists and reuse the computed class tokens.
Line 141 and Line 158 allocate a new array for every node. Move both to module-level
Setconstants. Line 163 also rescansopening.attributesforclassand re-splits the value, although Line 139 already producedclassTokens. Compute the tokens once and test them foraria-managed-image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/rendering/canonical/compileCanonicalNode.ts` around lines 141 - 167, Update compileCanonicalNode’s tag checks by hoisting the void-element and interactive tag lists into module-level Set constants, then reuse those sets for membership tests instead of allocating arrays per node. In the managedImage capability calculation, reuse the existing classTokens computed near line 139 to detect aria-managed-image rather than rescanning opening.attributes and splitting the class value again.aria/lib/rendering/canonical/compileCanonicalAttributes.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo modules only re-export symbols that
renderDocumentHtml.tsimplements. The canonical compiler API is declared across separate files while one file holds every implementation. Consumers gain two import paths per symbol, and ownership of the compiler surface becomes unclear.compileCanonicalNode.tsLine 21 repeats the pattern forcompileCanonicalNodeHtmlandcompileCanonicalTag.
aria/lib/rendering/canonical/compileCanonicalAttributes.ts#L1-L1: move thecompileCanonicalAttributesimplementation into this file, or delete the file and import fromrenderDocumentHtml.ts.aria/lib/rendering/canonical/compileCanonicalContent.ts#L1-L1: apply the same decision tocompileCanonicalContentso both modules follow one convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/rendering/canonical/compileCanonicalAttributes.ts` at line 1, Consolidate the canonical compiler exports by removing the redundant re-export modules and importing compileCanonicalAttributes and compileCanonicalContent directly from renderDocumentHtml.ts; apply this consistently at aria/lib/rendering/canonical/compileCanonicalAttributes.ts:1-1 and aria/lib/rendering/canonical/compileCanonicalContent.ts:1-1, updating any consumers as needed.aria/lib/rendering/canonical/compileRenderDocument.ts (1)
67-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the icon-resource fallback once.
options.iconResources ?? options.document.iconResourcesis repeated at Lines 70, 76, and 111. A single local keeps the threefreshIconResourcescopies aligned if the fallback rule changes.♻️ Proposed refactor
+ const baseIconResources = + options.iconResources ?? options.document.iconResources; const documentOptions: NodeToHtmlDocumentOptions = { ...options.document, - iconResources: freshIconResources( - options.iconResources ?? options.document.iconResources, - ), + iconResources: freshIconResources(baseIconResources), }; const styleMode = resolvePublishedHtmlRenderStyleMode(documentOptions); const html = nodesToHtmlDocument(roots, documentOptions); - const regionIconResources = freshIconResources( - options.iconResources ?? options.document.iconResources, - ); + const regionIconResources = freshIconResources(baseIconResources);roots: compileCanonicalNodes(roots, { breakpoints: documentOptions.breakpoints, styleMode, - iconResources: freshIconResources( - options.iconResources ?? options.document.iconResources, - ), + iconResources: freshIconResources(baseIconResources), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/rendering/canonical/compileRenderDocument.ts` around lines 67 - 113, Extract options.iconResources ?? options.document.iconResources into a local fallback value near the start of the render flow, then reuse it for the documentOptions, regionIconResources, and compileCanonicalNodes freshIconResources calls. Remove the repeated fallback expressions while preserving the existing resource-refresh behavior.aria/tests/core/useSavePublish.test.ts (1)
404-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that pending delivery triggers polling.
The test proves the success toast and the absence of an error toast. It does not assert that
deliveryStatusMockwas called with the returneddeliveryJobId. Add that assertion so the test protects the polling contract inpollPublishDelivery.💚 Proposed assertion
expect(toastSuccessMock).toHaveBeenCalledWith("Page Published"); expect(toastErrorMock).not.toHaveBeenCalled(); + expect(deliveryStatusMock).toHaveBeenCalledWith({ + jobId: "public-route:publish:page-home:v2", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/core/useSavePublish.test.ts` around lines 404 - 430, Update the test around handleSaveAndPublish to assert that pending delivery invokes deliveryStatusMock with the returned deliveryJobId "public-route:publish:page-home:v2". Keep the existing toast assertions and verify the polling contract implemented by pollPublishDelivery.aria/admin/features/Inspector/composables/useStylePreviewQueue.ts (1)
27-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRegistration is global; unregistration depends on a component instance.
registerEditorCommitFlusheraddsflushto a module-level set. The cleanup runs only insideonBeforeUnmount. If any caller invokesuseStylePreviewQueueoutside a componentsetupscope, Vue skips the hook and the flusher stays registered for the process lifetime. Every latersettleEditorCommitBarriercall then invokes a staleflushand can calloptions.applyPreviewfor a destroyed target.Guard the registration with
getCurrentInstance(), or return the unregister function so non-component callers can release it.Also applies to: 69-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Inspector/composables/useStylePreviewQueue.ts` at line 27, Update useStylePreviewQueue around registerEditorCommitFlusher and its onBeforeUnmount cleanup so registration only occurs when getCurrentInstance() is available, or otherwise expose and require the unregisterEditorCommitFlusher callback for non-component callers. Ensure every registered flush is removable and cannot remain active after its target is destroyed.aria/tests/storage/surface-normalization.test.ts (1)
38-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a test that pins the trusted hash to the canonical hash.
This test uses an arbitrary 64-character value, so it only proves pass-through. It does not prove that a marked
content_hashequalsnormalizeSurfaceForPersistence(kind, dsl).sourceHash. That equality is the propertysavePageDSLrelies on for conflict reconciliation and forskipIfContentUnchanged.Add a case that computes the canonical hash for
page, stores it incontent_hashwith the marker, and asserts the resolved value equals that canonical hash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/storage/surface-normalization.test.ts` around lines 38 - 56, Add a test alongside the existing resolveStoredSemanticSourceHash case that derives the expected hash via normalizeSurfaceForPersistence("page", dsl).sourceHash, stores that value as content_hash with the renderSourceHashVersion marker, and asserts the resolver returns the canonical hash without invoking fallback. Use valid DSL input so the assertion verifies canonicalization rather than arbitrary hash pass-through.aria/admin/features/Core/composables/useSavePublish.ts (1)
201-213: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDelivery polling has no delay between attempts.
The loop issues the second
deliveryStatusrequest immediately after the first. A job that needs a few hundred milliseconds is almost always stillpending, so the second attempt adds a request without improving the result. Add a short delay with backoff between attempts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Core/composables/useSavePublish.ts` around lines 201 - 213, Update pollPublishDelivery to wait between deliveryStatus attempts, applying a short increasing backoff before each retry while preserving the existing two-attempt limit and ready/pending outcomes.aria/tests/rendering-parity/p0CanvasFixture.ts (1)
142-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the contradictory
ordered: trueon thedlfixture node.The node sets
element: "dl"andordered: true.resolveListSemanticModereturnsdescriptionbecauseelementwins, so the flag has no effect today. Either remove the flag, or add a short comment stating that it intentionally guards against tag resolution that prefersorderedoverelement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/rendering-parity/p0CanvasFixture.ts` around lines 142 - 149, Clarify the intentional `ordered: true` setting on the `parity-description-list` fixture in the `parity-description-list` node: either remove the unused flag or add a brief comment explaining that it guards against future tag resolution preferring `ordered` over `element`.aria/admin/features/Inspector/inputs/ListProperty.vue (1)
249-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
isDescriptionListfromlistSemanticMode.
listSemanticModealready resolves description semantics throughresolveListSemanticMode. The separateprops.element === "dl"check duplicates that rule in the component. If the resolver changes, the two checks can diverge.♻️ Proposed refactor
-const isDescriptionList = computed( - () => targetNode.value?.props?.element === "dl", -); +const isDescriptionList = computed( + () => listSemanticMode.value === "description", +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Inspector/inputs/ListProperty.vue` around lines 249 - 258, Update the isDescriptionList computed property to derive its value from the existing listSemanticMode computed state, which resolves semantics through resolveListSemanticMode, instead of reading targetNode.value?.props?.element directly. Leave showMarkerControls and the other computed properties unchanged.aria/tests/rendering-parity/canvas-parity.spec.ts (1)
442-442: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert list semantics on the public frame.
expectMountedListSemanticsruns againststageFrameonly.openSurfacereturns aFramefor both surfaces, so the same helper can validate the published HTML. Public-side coverage is what catches Stage-versus-canonical divergence, such as thedlhandling difference inaria/lib/rendering/canonical/renderDocumentHtml.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/rendering-parity/canvas-parity.spec.ts` at line 442, Extend the parity test after the existing expectMountedListSemantics(stageFrame) call to run the same helper against the public Frame returned by openSurface. Keep the stageFrame assertion and validate both surfaces so canonical HTML list semantics are covered.aria/admin/features/Inspector/composables/useListSemanticMutation.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocalize the operator-facing error strings.
"The editor is saving"and"Failed to change list type"are hardcoded English.ListProperty.vueassignsresult.errordirectly tovalidationError, and it usest("inspector.validation.invalidList")only whenresult.erroris undefined. So these strings reach the inspector untranslated. Return message keys, or resolve the text in the component throughuseStudioI18n.Also applies to: 45-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Inspector/composables/useListSemanticMutation.ts` around lines 22 - 24, Update the error handling in useListSemanticMutation, including the isEditorMutationLocked branch and the failed list-type mutation path, to return localization message keys instead of hardcoded English strings. Ensure ListProperty.vue resolves result.error through useStudioI18n before assigning validationError, while preserving the existing fallback to inspector.validation.invalidList when no error is provided.aria/tests/inputs/ListProperty.test.ts (1)
608-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract one shared Tabs stub factory.
The two stub blocks are near-duplicates. The second block omits
modelValuefrom the provided context, so the two tests exercise slightly different stub behavior. A shared factory keeps them aligned.Also applies to: 700-728
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/inputs/ListProperty.test.ts` around lines 608 - 637, Extract the duplicated Tabs, TabsList, and TabsTrigger definitions into one shared stub factory used by both test blocks around the existing definitions and the corresponding block near the second location. Ensure the factory consistently provides both the reactive modelValue and select through tabsSelectionKey, while preserving the existing props, emits, rendering, and click behavior.aria/admin/features/Stage/dragdrop/canvasDropInsertion.ts (3)
259-328: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache child rectangles in the grid index pass.
The function reads
getBoundingClientRectfor each child during row grouping, row sorting, row-bound computation, and the visual scan. It also callschildren.indexOf(child), which is O(n²) for the row. Each rectangle read can force layout, and drag feedback runs this function per animation frame.Measure each child once into an array of
{ element, index, rect }entries, then group, sort, and scan over that array. The insertion-index results stay identical.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Stage/dragdrop/canvasDropInsertion.ts` around lines 259 - 328, Update calculateGridInsertionIndex to measure each child once into cached entries containing its element, original index, and DOMRect, then use those entries for row grouping, sorting, bounds calculation, and visual scanning. Replace children.indexOf lookups with the cached index while preserving the existing insertion-index behavior and ordering.
387-401: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the row tolerance and simplify the direction test.
sameVisualRowrepeats the 8px tolerance thatcalculateGridInsertionIndexdefines asrowTolerance. Extract one module constant so both row checks stay aligned.!(a !== b)is equivalent toa === b; the direct comparison reads clearer.♻️ Proposed cleanup
+const ROW_TOLERANCE_PX = 8; + function sameVisualRow(a: ViewportRect, b: ViewportRect): boolean { - return Math.abs(a.top - b.top) <= 8; + return Math.abs(a.top - b.top) <= ROW_TOLERANCE_PX; } @@ - const domAscendsVisually = !(reversedByFlex !== reversedByDirection); + const domAscendsVisually = reversedByFlex === reversedByDirection;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Stage/dragdrop/canvasDropInsertion.ts` around lines 387 - 401, Extract the 8px row tolerance into a shared module-level constant and use it in both sameVisualRow and calculateGridInsertionIndex so their thresholds remain aligned. In computeHorizontalFlowPlaceholder, simplify the domAscendsVisually direction logic by directly comparing reversedByFlex and reversedByDirection with equality.
217-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve the drop-zone style once per layout pass.
readDropStylecallsgetComputedStyleon each invocation. OnecomputeAddElementsDropLayoutcall reaches it throughisGridDropZone,isWrappedHorizontalDropZone,readDropAxis, andcomputeHorizontalFlowPlaceholder, so the same element is measured six or more times.useCanvasDrop.updateAutoScrollrepeats this path on every animation frame while the pointer stays in the edge zone, so each forced style read lands on the drag hot path.Resolve
DropZoneStyleonce incomputeAddElementsDropLayoutand pass it tocalculateInsertionIndexMidline,computePlaceholderViewportRect, andcomputeHorizontalFlowPlaceholder. Keep the current defaults so behavior does not change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/admin/features/Stage/dragdrop/canvasDropInsertion.ts` around lines 217 - 257, Update computeAddElementsDropLayout to call readDropStyle once per layout pass and reuse the resulting DropZoneStyle throughout the calculation. Pass the cached style into calculateInsertionIndexMidline, computePlaceholderViewportRect, and computeHorizontalFlowPlaceholder, adjusting their related helpers such as readDropAxis, isGridDropZone, and isWrappedHorizontalDropZone to consume it without additional computed-style reads. Preserve all existing fallback defaults and behavior.aria/tests/features/stage/canvasDropInsertion.test.ts (1)
26-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the
defaultViewstub per element.
parent.ownerDocumentis the shared globaldocument, so eachmockFlexParentcall replacesdocument.defaultViewfor every element in the file. The last call wins. The test at lines 269-299 therefore depends on statement order: it must compute the row layout before it creates the column parent, otherwise the row reads column styles. No hook restores the realdefaultView, so the stub also leaks into later tests.Return the style descriptor per element and stub
getComputedStyleso it dispatches on the element it receives. Restoredocument.defaultViewin anafterEachhook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/features/stage/canvasDropInsertion.test.ts` around lines 26 - 51, Update mockFlexParent so style configuration is associated with each created parent and document.defaultView.getComputedStyle dispatches based on the element argument, rather than replacing shared defaultView per call. Preserve the existing display, flex, direction, and grid style values for each mock element, and add an afterEach cleanup that restores the original document.defaultView after every test.aria/tests/features/stage/useCanvasDrop.test.ts (1)
79-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the feedback frame explicitly and remove test listeners.
[...animationFrames.values()][0]relies on the composable scheduling the feedback frame before the auto-scroll frame.handleDragOvercallsscheduleDropFeedbackand thenupdateAutoScroll, so the order holds today, but a reorder in production code would make this test drain the auto-scroll callback instead and fail for the wrong reason. Run every queued callback, or record the callback identity you expect.The
canvas:add-elements-insertionandcanvas:droplisteners are never removed. They stay registered for the second test. Register them with anAbortControllersignal and abort inafterEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/features/stage/useCanvasDrop.test.ts` around lines 79 - 108, Make the useCanvasDrop test select the expected feedback animation callback without depending on Map insertion order, by recording or otherwise identifying the callback scheduled by scheduleDropFeedback and invoking that callback explicitly. Also register the canvas:add-elements-insertion and canvas:drop listeners with an AbortController signal, then abort it in afterEach so listeners do not leak into the second test.aria/lib/cloudflare/workers-shim.ts (1)
37-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the shim signature with the declared module type.
aria/lib/types/cloudflare-workers-module.d.tsdeclarespurge(options: CachePurgeOptions), andCachePurgeResultis declared in both files. The shimpurge()accepts no parameter. Accept and ignore the options argument so the shim matches the contract, and reuse one result type to avoid drift.♻️ Proposed alignment
-export interface CachePurgeResult { - success: boolean; - errors: Array<{ code: number; message: string }>; -} +import type { + CachePurgeOptions, + CachePurgeResult, +} from "cloudflare:workers"; + +export type { CachePurgeResult }; /** Node has no Workers edge cache; a purge is therefore already satisfied. */ export const cache = { - async purge(): Promise<CachePurgeResult> { + async purge(_options: CachePurgeOptions): Promise<CachePurgeResult> { return { success: true, errors: [] }; }, };If the shim itself provides the
cloudflare:workersmodule at build time, declare the types locally instead of importing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/lib/cloudflare/workers-shim.ts` around lines 37 - 47, Update the cache.purge method in workers-shim.ts to accept and ignore the required CachePurgeOptions argument, matching the cloudflare-workers-module declaration. Consolidate CachePurgeResult so the shim reuses the existing declared type rather than defining a duplicate; if the shim supplies the module itself, keep the necessary type declarations local instead of importing them.aria/tests/cache/public-page-cache.test.ts (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale Cloudflare credential mock.
The purge path no longer reads API tokens or zone identifiers. This mock now only injects unused fake credentials. Delete it, or keep it only if another import in this file still calls
getCloudflareEnv.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/cache/public-page-cache.test.ts` around lines 9 - 14, Remove the vi.mock for getCloudflareEnv from the cache test, since the purge path no longer consumes Cloudflare credentials. Retain the mock only if another import in this file still invokes getCloudflareEnv.aria/tests/integration/publishing-snapshot-injection.test.ts (1)
99-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for a concurrent draft save during publish.
The
getPageVersionPinsmock always returns matching draft/published/current versions, so this suite never exercises the case wheredraftVersiondiffers frompublishedVersionright after a successful publish (see the reconciliation race raised onaria/actions/publishing.tslines 340-354). Add a test wheremockGetPageVersionPinsreturns adraftVersiondifferent from the just-published version, to confirm the publish action's behavior in that case.Also applies to: 266-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aria/tests/integration/publishing-snapshot-injection.test.ts` around lines 99 - 104, Add a regression test in the publishing snapshot injection suite for a concurrent draft save during publish. Configure mockGetPageVersionPins to return a draftVersion different from the just-published version while preserving the published and current versions, then assert the publishing action’s reconciliation behavior for that mismatch. Cover the existing setup shared by the referenced mock configuration without changing matching-version scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aria/actions/publishing.ts`:
- Around line 413-428: Update the deliveryStatus handler to stop unconditionally
forcing delivery via the true argument to deliverCommittedInvalidation. Respect
the job’s nextAttemptAt backoff by default, and only bypass it when the API
explicitly accepts and validates an immediate-retry request; otherwise preserve
normal delivery behavior.
- Around line 340-354: The post-publish reconciliation in publishPageDSL must
not fail when draftVersion differs from publishedVersion because of a concurrent
autosave. Remove draftVersion from the fatal condition or make that divergence
non-fatal, while retaining validation that publishedVersion matches the expected
publishedVersion and avoiding conversion of a successful publish into
PUBLISH_FAILED.
In `@aria/admin/features/Core/composables/editorCommitCoordinator.ts`:
- Around line 74-76: Update the module-scoped commit failure handling used by
settleEditorCommitBarrier so failures cannot survive an empty tracked commit set
and abort a later unrelated save. Clear or scope failures when the tracked
commits become empty, and preserve all relevant failures instead of discarding
entries after the first; ensure the barrier only throws failures associated with
commits pending for that barrier.
- Around line 47-52: Bound the loop in awaitTrackedCommits so repeatedly
enqueued follow-up commits cannot keep the barrier running indefinitely. Track
iterations and stop after a finite limit, using the existing error or
continuation behavior appropriate for save and publish so loadingState.isSaving
can resolve instead of remaining stuck.
In `@aria/admin/features/Core/composables/usePropertySave.ts`:
- Around line 1857-1874: Update the commit-tracking flow around saveProperty,
saveProperties, and saveNodeUpdates so benign false results from validation or
missing context do not become commit failures. Adjust trackEditorCommit or these
wrappers to track only successful real commits, while preserving thrown errors
as failures and keeping the existing lock behavior unchanged.
In `@aria/admin/features/Core/composables/useSavePublish.ts`:
- Around line 492-526: Update createDocumentFingerprint to serialize its payload
with a stable, recursively sorted-key serializer instead of direct
JSON.stringify, so authoredDocument and linkedLayoutSlots produce identical
fingerprints regardless of property insertion order. Preserve the existing
fingerprint inputs and values while ensuring nested objects are canonicalized
before serialization.
In `@aria/admin/features/Stage/dragdrop/useCanvasDrop.ts`:
- Around line 477-493: Update handleCanvasExit to verify that the drag has
actually left the canvas/drop-zone before calling endDrag(), rather than relying
only on relatedTarget and viewport coordinates. Track valid drag-enter/leave
depth or use current-target containment, and preserve active drops during normal
intra-zone drag movement.
In `@aria/lib/cache/invalidationJobs.ts`:
- Around line 80-102: Update the page and layout tag construction in the
invalidation job’s else branch to use the job’s stored route/slug, locale, and
version fields rather than treating resourceId as the slug. Preserve locale and
version scoping for localization jobs, and ensure page tags are generated with
the published route while layout tags retain their localization metadata.
In `@aria/lib/localization/invalidationDrain.ts`:
- Around line 63-77: Update drainCacheInvalidations to enforce a bounded timeout
for every input.deliver(job) attempt, including retries, so a hanging cache
purge cannot block the request path; use the existing delivery flow and preserve
retry/error propagation while ensuring each attempt fails fast.
In `@aria/lib/rendering/canonical/compileCanonicalNode.ts`:
- Around line 111-121: Update compileCanonicalNode and compileCanonicalNodes to
avoid calling compileCanonicalNodeHtml for each node’s full subtree solely to
obtain its opening element. Use an opening-tag-only compilation path or
serialize and walk the tree once, while preserving the existing opening parsing
and recursive output behavior.
- Around line 29-48: Update compileCanonicalNode to derive the
CanonicalRenderNode attributes from the original node/prop model using
compileCanonicalAttributes, rather than parsing the opening tag produced by
compileCanonicalNodeHtml. Remove the rendered-HTML attribute reparsing path
while preserving the existing canonical attribute values and hashing behavior.
- Around line 99-101: Update the normalizedType === "svg" branch in
compileCanonicalNode to sanitize content before constructing the trusted-svg
child. Reuse the project’s existing SVG sanitization utility if available,
ensuring script elements, event-handler attributes, and javascript: URLs are
removed while preserving valid SVG markup.
In `@aria/lib/rendering/canonical/compileRenderDocument.ts`:
- Around line 37-46: Replace the regex-based extraction in
aria/lib/rendering/canonical/compileRenderDocument.ts lines 37-46 by returning
and assigning the head nodes and generated stylesheet directly from
nodesToHtmlDocument, preserving the compiled structure. In
aria/lib/rendering/renderPageHtml.ts lines 156-160, expose the compiled
RenderDocumentV1 or its head/body fields from renderPageDslToHtml and construct
headHtml and bodyHtml from those fields instead of matching tags in
rendered.html.
In `@aria/lib/rendering/canonical/renderDocumentHtml.ts`:
- Around line 1147-1161: The canonical list-item generation condition in
aria/lib/rendering/canonical/renderDocumentHtml.ts lines 1147-1161 must also
require the resolved tag not be "dl", preventing li children inside description
lists. Keep the existing dl guard in
aria/admin/features/Stage/components/StageFrame.vue lines 1865-1868 unchanged;
it already matches the corrected canonical behavior.
In `@aria/tests/rendering-parity/p0CanvasFixture.ts`:
- Around line 214-249: Add the new fixture IDs—parity-description-list,
parity-unordered-list, and parity-ordered-list—to P0_CANVAS_PARITY_NODE_IDS so
collectBrowserSurfaceSnapshot includes all three list nodes in P0 parity
collection.
---
Outside diff comments:
In `@aria/actions/publishing.ts`:
- Around line 309-327: The createPagePublicationInvalidationJob implementation
must ensure aria_cache_invalidation_jobs.id remains collision-resistant instead
of relying on idempotencyKey.slice(0, 160). Derive the primary-key identifier
from the full idempotencyKey using a stable collision-resistant representation,
while preserving idempotencyKey for the existing conflict guard and keeping
publication invalidation behavior unchanged.
---
Minor comments:
In `@aria/admin/features/Inspector/composables/useListSemanticMutation.ts`:
- Around line 26-30: Update the undo snapshot in the list semantic mutation flow
to capture a plain clone of the current listNode before calling
convertListSemanticMode, rather than deriving previousNode through conversion.
Keep convertListSemanticMode for creating nextNode, and ensure undo restores the
cloned original node without normalization or injected listStyleType values.
In `@aria/admin/features/Stage/dragdrop/useCanvasDrop.ts`:
- Around line 361-383: Update the step function inside updateAutoScroll to
capture the iframeWindow scroll offsets before calling scrollBy, then compare
them afterward; when neither axis changes, clear autoScrollFrame and return
without scheduling drop feedback or another animation frame. Preserve the
existing velocity-zero and drag-state termination paths.
In `@aria/lib/rendering/canonical/compileCanonicalNode.ts`:
- Line 181: Update the Fragment branch in compileCanonicalNode to normalize
node.type with the same toLowerCase() handling used by the checks near lines 94
and 122, so both "Fragment" and "fragment" are flattened consistently.
In `@aria/tests/cache/public-page-cache.test.ts`:
- Around line 28-34: Reset the standalone purgeMock state in the beforeEach hook
by calling purgeMock.mockReset() before configuring its resolved value. Keep the
existing mockResolvedValue setup and afterEach restoration unchanged.
In `@aria/tests/lib/listNodes.test.ts`:
- Around line 129-148: Update ordinaryListItemToDescriptionChild in
convertListSemanticMode so each plain list item converted to description mode
emits its term together with a paired empty description child, producing valid
dt/dd groups. Preserve the existing item content and description-list semantics
while updating the test expectations to account for the added dd nodes.
In `@aria/tests/rendering-foundation/canonical-document.parity.test.ts`:
- Line 162: Update the ordering assertion around the HTML checks to first assert
that the expected "Header" content is present, then compare its index with the
<dl> index. Ensure the test fails when the header is absent while preserving the
existing ordering requirement.
---
Nitpick comments:
In `@aria/admin/features/Core/composables/useSavePublish.ts`:
- Around line 201-213: Update pollPublishDelivery to wait between deliveryStatus
attempts, applying a short increasing backoff before each retry while preserving
the existing two-attempt limit and ready/pending outcomes.
In `@aria/admin/features/Inspector/composables/useListSemanticMutation.ts`:
- Around line 22-24: Update the error handling in useListSemanticMutation,
including the isEditorMutationLocked branch and the failed list-type mutation
path, to return localization message keys instead of hardcoded English strings.
Ensure ListProperty.vue resolves result.error through useStudioI18n before
assigning validationError, while preserving the existing fallback to
inspector.validation.invalidList when no error is provided.
In `@aria/admin/features/Inspector/composables/useStylePreviewQueue.ts`:
- Line 27: Update useStylePreviewQueue around registerEditorCommitFlusher and
its onBeforeUnmount cleanup so registration only occurs when
getCurrentInstance() is available, or otherwise expose and require the
unregisterEditorCommitFlusher callback for non-component callers. Ensure every
registered flush is removable and cannot remain active after its target is
destroyed.
In `@aria/admin/features/Inspector/inputs/ListProperty.vue`:
- Around line 249-258: Update the isDescriptionList computed property to derive
its value from the existing listSemanticMode computed state, which resolves
semantics through resolveListSemanticMode, instead of reading
targetNode.value?.props?.element directly. Leave showMarkerControls and the
other computed properties unchanged.
In `@aria/admin/features/Stage/dragdrop/canvasDropInsertion.ts`:
- Around line 259-328: Update calculateGridInsertionIndex to measure each child
once into cached entries containing its element, original index, and DOMRect,
then use those entries for row grouping, sorting, bounds calculation, and visual
scanning. Replace children.indexOf lookups with the cached index while
preserving the existing insertion-index behavior and ordering.
- Around line 387-401: Extract the 8px row tolerance into a shared module-level
constant and use it in both sameVisualRow and calculateGridInsertionIndex so
their thresholds remain aligned. In computeHorizontalFlowPlaceholder, simplify
the domAscendsVisually direction logic by directly comparing reversedByFlex and
reversedByDirection with equality.
- Around line 217-257: Update computeAddElementsDropLayout to call readDropStyle
once per layout pass and reuse the resulting DropZoneStyle throughout the
calculation. Pass the cached style into calculateInsertionIndexMidline,
computePlaceholderViewportRect, and computeHorizontalFlowPlaceholder, adjusting
their related helpers such as readDropAxis, isGridDropZone, and
isWrappedHorizontalDropZone to consume it without additional computed-style
reads. Preserve all existing fallback defaults and behavior.
In `@aria/lib/cloudflare/workers-shim.ts`:
- Around line 37-47: Update the cache.purge method in workers-shim.ts to accept
and ignore the required CachePurgeOptions argument, matching the
cloudflare-workers-module declaration. Consolidate CachePurgeResult so the shim
reuses the existing declared type rather than defining a duplicate; if the shim
supplies the module itself, keep the necessary type declarations local instead
of importing them.
In `@aria/lib/rendering/canonical/compileCanonicalAttributes.ts`:
- Line 1: Consolidate the canonical compiler exports by removing the redundant
re-export modules and importing compileCanonicalAttributes and
compileCanonicalContent directly from renderDocumentHtml.ts; apply this
consistently at aria/lib/rendering/canonical/compileCanonicalAttributes.ts:1-1
and aria/lib/rendering/canonical/compileCanonicalContent.ts:1-1, updating any
consumers as needed.
In `@aria/lib/rendering/canonical/compileCanonicalNode.ts`:
- Line 136: Update CompileCanonicalNodeOptions and the compileCanonicalNode flow
so the parent namespace is passed into child compilation and inherited by
descendants. Ensure SVG descendants such as path, g, and use retain the "svg"
namespace instead of deriving "html" solely from their own tagName, while
preserving the root svg namespace assignment.
- Around line 141-167: Update compileCanonicalNode’s tag checks by hoisting the
void-element and interactive tag lists into module-level Set constants, then
reuse those sets for membership tests instead of allocating arrays per node. In
the managedImage capability calculation, reuse the existing classTokens computed
near line 139 to detect aria-managed-image rather than rescanning
opening.attributes and splitting the class value again.
In `@aria/lib/rendering/canonical/compileRenderDocument.ts`:
- Around line 67-113: Extract options.iconResources ??
options.document.iconResources into a local fallback value near the start of the
render flow, then reuse it for the documentOptions, regionIconResources, and
compileCanonicalNodes freshIconResources calls. Remove the repeated fallback
expressions while preserving the existing resource-refresh behavior.
In `@aria/tests/cache/public-page-cache.test.ts`:
- Around line 9-14: Remove the vi.mock for getCloudflareEnv from the cache test,
since the purge path no longer consumes Cloudflare credentials. Retain the mock
only if another import in this file still invokes getCloudflareEnv.
In `@aria/tests/core/useSavePublish.test.ts`:
- Around line 404-430: Update the test around handleSaveAndPublish to assert
that pending delivery invokes deliveryStatusMock with the returned deliveryJobId
"public-route:publish:page-home:v2". Keep the existing toast assertions and
verify the polling contract implemented by pollPublishDelivery.
In `@aria/tests/features/stage/canvasDropInsertion.test.ts`:
- Around line 26-51: Update mockFlexParent so style configuration is associated
with each created parent and document.defaultView.getComputedStyle dispatches
based on the element argument, rather than replacing shared defaultView per
call. Preserve the existing display, flex, direction, and grid style values for
each mock element, and add an afterEach cleanup that restores the original
document.defaultView after every test.
In `@aria/tests/features/stage/useCanvasDrop.test.ts`:
- Around line 79-108: Make the useCanvasDrop test select the expected feedback
animation callback without depending on Map insertion order, by recording or
otherwise identifying the callback scheduled by scheduleDropFeedback and
invoking that callback explicitly. Also register the
canvas:add-elements-insertion and canvas:drop listeners with an AbortController
signal, then abort it in afterEach so listeners do not leak into the second
test.
In `@aria/tests/inputs/ListProperty.test.ts`:
- Around line 608-637: Extract the duplicated Tabs, TabsList, and TabsTrigger
definitions into one shared stub factory used by both test blocks around the
existing definitions and the corresponding block near the second location.
Ensure the factory consistently provides both the reactive modelValue and select
through tabsSelectionKey, while preserving the existing props, emits, rendering,
and click behavior.
In `@aria/tests/integration/publishing-snapshot-injection.test.ts`:
- Around line 99-104: Add a regression test in the publishing snapshot injection
suite for a concurrent draft save during publish. Configure
mockGetPageVersionPins to return a draftVersion different from the
just-published version while preserving the published and current versions, then
assert the publishing action’s reconciliation behavior for that mismatch. Cover
the existing setup shared by the referenced mock configuration without changing
matching-version scenarios.
In `@aria/tests/rendering-foundation/canonical-document.parity.test.ts`:
- Around line 187-189: Document how to regenerate the pinned documentHash near
the assertion in the canonical document parity test, including the exact
regeneration command or a named constant with that note. Preserve the existing
hash assertion and the determinism and self-consistency checks.
- Around line 196-199: Add negative test coverage in the canonical render
document parity suite: modify a field on the document and assert
hasValidRenderDocumentHash returns false, and compile a document with freeze:
false then assert the returned document is mutable. Keep the existing valid-hash
and frozen-document assertions unchanged.
In `@aria/tests/rendering-parity/canvas-parity.spec.ts`:
- Line 442: Extend the parity test after the existing
expectMountedListSemantics(stageFrame) call to run the same helper against the
public Frame returned by openSurface. Keep the stageFrame assertion and validate
both surfaces so canonical HTML list semantics are covered.
In `@aria/tests/rendering-parity/p0CanvasFixture.ts`:
- Around line 142-149: Clarify the intentional `ordered: true` setting on the
`parity-description-list` fixture in the `parity-description-list` node: either
remove the unused flag or add a brief comment explaining that it guards against
future tag resolution preferring `ordered` over `element`.
In `@aria/tests/storage/surface-normalization.test.ts`:
- Around line 38-56: Add a test alongside the existing
resolveStoredSemanticSourceHash case that derives the expected hash via
normalizeSurfaceForPersistence("page", dsl).sourceHash, stores that value as
content_hash with the renderSourceHashVersion marker, and asserts the resolver
returns the canonical hash without invoking fallback. Use valid DSL input so the
assertion verifies canonicalization rather than arbitrary hash pass-through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const delivery = await deliverCommittedInvalidation( | ||
| adapter, | ||
| context, | ||
| invalidationJob.id, | ||
| ); | ||
| const versionPins = await adapter.getPageVersionPins(currentPage.id); | ||
| if ( | ||
| versionPins?.draftVersion !== publishedVersion || | ||
| versionPins.publishedVersion !== publishedVersion | ||
| ) { | ||
| throw new Error( | ||
| "Published page revision could not be reconciled with its saved draft.", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^|/)aria/(actions/publishing\.(ts|tsx)|lib/storage/internal/domains/pageLifecycle\.(ts|tsx))$' || true
echo
echo "Locate symbols:"
rg -n "publishMetaStatement|getPageVersionPins|publishPageDSL|savePageDSL|publishMetaStatement|current_version|published_version|draft_version" -S .Repository: ariabuilder/aria
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "publishing.ts relevant section:"
sed -n '280,370p' aria/actions/publishing.ts
echo
echo "pageLifecycle publishPageDSL/publishMetaStatement relevant section:"
sed -n '750,910p' aria/lib/storage/internal/domains/pageLifecycle.ts
echo
echo "pageLifecycle savePageDSL relevant section:"
sed -n '260,370p' aria/lib/storage/internal/domains/pageLifecycle.ts
echo
echo "pageLifecycle getPageVersionPins relevant section:"
sed -n '1120,1260p' aria/lib/storage/internal/domains/pageLifecycle.tsRepository: ariabuilder/aria
Length of output: 17170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "actions/pages savePageDSL with expectedVersion:"
sed -n '230,295p' aria/actions/pages.ts
echo
echo "publishing.ts post-publish catch flow:"
sed -n '360,410p' aria/actions/publishing.ts
echo
echo "PageSaveOptions expectedVersion usages in actions:"
rg -n "expectedVersion|savePageDSL\\(" aria/actions/pages.ts aria/actions/_shared.ts -C 3
echo
echo "getVersionPins full query/read behavior:"
rg -n "async getPageVersionPins|resolvePageIdentity|publishVersionTableInsert|draft_version|published_version|current_version" aria/lib/storage/internal/domains/pageLifecycle.ts -C 4Repository: ariabuilder/aria
Length of output: 22450
Post-publish draftVersion reconciliation should not fail publish.
publishPageDSL sets published_version while savePageDSL updates draft_version and current_version. A concurrent autosave can commit a newer draft after publishing succeeds, so comparing versionPins.draftVersion !== publishedVersion here does not show an inconsistent publish state. This branch throws while the page is already published in storage, and the catch converts it to success: false with PUBLISH_FAILED. Use a non-fatal check for draft divergence or remove the draft-version comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/actions/publishing.ts` around lines 340 - 354, The post-publish
reconciliation in publishPageDSL must not fail when draftVersion differs from
publishedVersion because of a concurrent autosave. Remove draftVersion from the
fatal condition or make that divergence non-fatal, while retaining validation
that publishedVersion matches the expected publishedVersion and avoiding
conversion of a successful publish into PUBLISH_FAILED.
| deliveryStatus: defineAction({ | ||
| accept: "json", | ||
| input: z.object({ jobId: z.string().trim().min(1).max(160) }).strict(), | ||
| handler: async ({ jobId }, context) => { | ||
| await requireOperation(context, "publishing.publish"); | ||
| const adapter = await getStorageAdapterAsync(context.locals); | ||
| const delivery = await deliverCommittedInvalidation( | ||
| adapter, | ||
| context, | ||
| jobId, | ||
| true, | ||
| ); | ||
| return { success: true, data: { jobId, delivery } }; | ||
| }, | ||
| }), | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
deliveryStatus bypasses backoff on every poll.
This handler always calls deliverCommittedInvalidation(..., true), so claimDueCacheInvalidationJobs skips the next_attempt_at <= ? gate on every invocation. After any failed attempt clears its lease, the very next poll immediately re-attempts delivery (up to 3 sub-attempts in drainCacheInvalidations) with no respect for the computed backoff window. If a client polls this action while the underlying cache-purge endpoint is degraded, this can turn into a retry storm against that endpoint instead of backing off.
Respect the job's nextAttemptAt unless a caller explicitly requests an immediate retry, or apply a minimum interval between forced attempts for the same job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/actions/publishing.ts` around lines 413 - 428, Update the deliveryStatus
handler to stop unconditionally forcing delivery via the true argument to
deliverCommittedInvalidation. Respect the job’s nextAttemptAt backoff by
default, and only bypass it when the API explicitly accepts and validates an
immediate-retry request; otherwise preserve normal delivery behavior.
| async function awaitTrackedCommits(): Promise<void> { | ||
| // Commits may enqueue follow-up mutations, so wait until the set is stable. | ||
| while (pendingCommits.size > 0) { | ||
| await Promise.all([...pendingCommits]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the settle loop.
awaitTrackedCommits repeats while pendingCommits is non-empty. A commit that enqueues a follow-up commit on every settle keeps the set non-empty forever. Save and publish then never resolve, and loadingState.isSaving stays true. Add an iteration bound so the barrier fails or continues instead of spinning.
🛡️ Proposed bound
async function awaitTrackedCommits(): Promise<void> {
// Commits may enqueue follow-up mutations, so wait until the set is stable.
- while (pendingCommits.size > 0) {
- await Promise.all([...pendingCommits]);
- }
+ for (let pass = 0; pass < 10 && pendingCommits.size > 0; pass += 1) {
+ await Promise.all([...pendingCommits]);
+ }
+ if (pendingCommits.size > 0) {
+ throw new Error("Editor commits did not settle");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function awaitTrackedCommits(): Promise<void> { | |
| // Commits may enqueue follow-up mutations, so wait until the set is stable. | |
| while (pendingCommits.size > 0) { | |
| await Promise.all([...pendingCommits]); | |
| } | |
| } | |
| async function awaitTrackedCommits(): Promise<void> { | |
| // Commits may enqueue follow-up mutations, so wait until the set is stable. | |
| for (let pass = 0; pass < 10 && pendingCommits.size > 0; pass += 1) { | |
| await Promise.all([...pendingCommits]); | |
| } | |
| if (pendingCommits.size > 0) { | |
| throw new Error("Editor commits did not settle"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/admin/features/Core/composables/editorCommitCoordinator.ts` around lines
47 - 52, Bound the loop in awaitTrackedCommits so repeatedly enqueued follow-up
commits cannot keep the barrier running indefinitely. Track iterations and stop
after a finite limit, using the existing error or continuation behavior
appropriate for save and publish so loadingState.isSaving can resolve instead of
remaining stuck.
| const failure = commitFailures.shift(); | ||
| commitFailures.length = 0; | ||
| if (failure) throw failure; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale commit failures can abort an unrelated later save.
commitFailures is module scoped and only drained inside the barrier. A commit that fails while no save or publish runs leaves an error in the array. The next settleEditorCommitBarrier call throws that old error, even for a different document or after the user already recovered. The barrier also discards every failure after the first.
Consider clearing failures when the tracked commit set becomes empty, or attach a sequence token so the barrier only consumes failures from commits that were pending during that barrier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/admin/features/Core/composables/editorCommitCoordinator.ts` around lines
74 - 76, Update the module-scoped commit failure handling used by
settleEditorCommitBarrier so failures cannot survive an empty tracked commit set
and abort a later unrelated save. Clear or scope failures when the tracked
commits become empty, and preserve all relevant failures instead of discarding
entries after the first; ensure the barrier only throws failures associated with
commits pending for that barrier.
| saveProperty: (...args) => | ||
| isEditorMutationLocked() | ||
| ? Promise.resolve(false) | ||
| : trackEditorCommit(saveProperty(...args), "Property change"), | ||
| saveProperties: (...args) => | ||
| isEditorMutationLocked() | ||
| ? Promise.resolve(false) | ||
| : trackEditorCommit(saveProperties(...args), "Property changes"), | ||
| previewStyleProperties: (...args) => | ||
| isEditorMutationLocked() ? false : previewStyleProperties(...args), | ||
| previewResponsiveStyleUpdates: (...args) => | ||
| isEditorMutationLocked() ? false : previewResponsiveStyleUpdates(...args), | ||
| previewProps: (...args) => | ||
| isEditorMutationLocked() ? false : previewProps(...args), | ||
| saveNodeUpdates: (...args) => | ||
| isEditorMutationLocked() | ||
| ? Promise.resolve(false) | ||
| : trackEditorCommit(saveNodeUpdates(...args), "Node property change"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Benign false results become commit failures.
trackEditorCommit records a failure whenever the tracked promise resolves to false (see aria/admin/features/Core/composables/editorCommitCoordinator.ts lines 25-27). saveProperty, saveProperties, and saveNodeUpdates also return false for non-error conditions: no node selected (Line 1294), missing itemType/itemSlug (Line 1303), and rejected input validation (Line 1318). After any of those, the next settleEditorCommitBarrier call throws "Property change could not be committed" and blocks the save or publish.
Track only real commits, or make the coordinator treat false as a no-op and rely on thrown errors for failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/admin/features/Core/composables/usePropertySave.ts` around lines 1857 -
1874, Update the commit-tracking flow around saveProperty, saveProperties, and
saveNodeUpdates so benign false results from validation or missing context do
not become commit failures. Adjust trackEditorCommit or these wrappers to track
only successful real commits, while preserving thrown errors as failures and
keeping the existing lock behavior unchanged.
| if (normalizedType === "svg") { | ||
| return [{ type: "trusted-svg", value: content }]; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every consumer of trusted-svg children.
rg -n -C5 "trusted-svg" --type=ts --type=vueRepository: ariabuilder/aria
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | sed -n '1,200p'
echo
echo "Locate compileCanonicalNode.ts:"
fd -a 'compileCanonicalNode\.ts$' . || true
echo
echo "Search canonical-related files:"
git ls-files | rg 'canonical|render' | sed -n '1,200p'Repository: ariabuilder/aria
Length of output: 13554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "compileCanonicalNode.ts outline:"
ast-grep outline aria/lib/rendering/canonical/compileCanonicalNode.ts --view expanded || true
echo
echo "Relevant compileCanonicalNode.ts lines 1-180:"
sed -n '1,180p' aria/lib/rendering/canonical/compileCanonicalNode.ts | cat -n
echo
echo "Search all files for trusted-svg and CanonicalRenderChild occurrences:"
rg -n -C4 "trusted-svg|CanonicalRenderChild" aria || trueRepository: ariabuilder/aria
Length of output: 11936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "contract.ts relevant lines:"
sed -n '220,275p' aria/lib/rendering/canonical/contract.ts | cat -n
echo
echo "index.ts relevant lines around canonical rendering contract:"
sed -n '70,115p' aria/lib/rendering/canonical/index.ts | cat -n
echo
echo "Search raw content sinks and CanonicalRenderChild consumers across all tracked files:"
rg -n -C3 "trusted-svg|CanonicalRenderChild|innerHTML|insertAdjacentHTML|setHTML|innerHTML:" . || trueRepository: ariabuilder/aria
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "svg raw sanitizer implementation:"
fd -a '.*svg.*sanitize.*|sanitizeSvg|svgSanitizer' aria . | sed -n '1,120p'
rg -n -C4 "sanitizeSvg|svg sanitizer|svgSanitizer|trusted-svg|trusted.*svg|rawContent\\(.*svg|svg.*innerHTML" aria --type=ts --type=vue --type=js | sed -n '1,240p'
echo
echo "focused nodeChildren call sites in canonical:"
rg -n -C4 "nodeChildren\\(|compileCanonicalNode\\(|compileCanonicalNodes\\(" aria/lib/rendering/canonical --type=tsRepository: ariabuilder/aria
Length of output: 9430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "SvgProperty stripUnsafeSvg implementation:"
sed -n '220,390p' aria/admin/features/Inspector/inputs/SvgProperty.vue | cat -n
echo
echo "StageFrame svg handling:"
sed -n '1920,1980p' aria/admin/features/Stage/components/StageFrame.vue | cat -n
echo
echo "Render html-to-svg parsing:"
rg -n -C4 "type: \"svg\"|type: 'svg'|SvgNodePropsSchema|parseSvgMarkup|svg:|is svg" aria/lib aria/admin --type=ts --type=vue | sed -n '1,260p'Repository: ariabuilder/aria
Length of output: 28389
Sanitize trusted-svg before exporting it.
nodeChildren assigns ScalarText from props.content, props.text, or props.label to trusted-svg; if the node is svg, this content includes unvalidated SVG markup. Sanitize SVG content such as script elements, event-handler attributes, and javascript: URLs before returning the child, instead of encoding it as trusted markup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/lib/rendering/canonical/compileCanonicalNode.ts` around lines 99 - 101,
Update the normalizedType === "svg" branch in compileCanonicalNode to sanitize
content before constructing the trusted-svg child. Reuse the project’s existing
SVG sanitization utility if available, ensuring script elements, event-handler
attributes, and javascript: URLs are removed while preserving valid SVG markup.
| const html = compileCanonicalNodeHtml( | ||
| node, | ||
| 0, | ||
| breakpoints, | ||
| styleMode, | ||
| null, | ||
| {}, | ||
| options.iconResources, | ||
| ); | ||
| const opening = parseOpeningElement(html); | ||
| if (!opening) return null; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether compileCanonicalNodeHtml can emit an opening tag only.
fd -t f 'renderDocumentHtml.ts' aria/lib/rendering | while IFS= read -r f; do
ast-grep outline "$f" --items all
doneRepository: ariabuilder/aria
Length of output: 4773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the canonical compile path and related caller.
sed -n '70,180p' aria/lib/rendering/canonical/compileCanonicalNode.ts
printf '\n--- compileCanonicalNodeHtml signature/body ---\n'
sed -n '870,1210p' aria/lib/rendering/canonical/renderDocumentHtml.ts | sed -n '1,220p'
printf '\n--- compileCanonicalNodes references ---\n'
rg -n "function compileCanonicalNodes|compileCanonicalNodes\\(|compileCanonicalNode\\(|parseOpeningElement" aria/lib/rendering/canonicalRepository: ariabuilder/aria
Length of output: 11726
Avoid full-subtree HTML serialization per canonical node.
compileCanonicalNode serializes node through the full compileCanonicalNodeHtml path just to parse the opening tag, then compileCanonicalNodes recurses into node.children and repeats that serialization for every descendant. Use an opening-tag-only path or a single tree serialization/walk to avoid quadratic compileCanonicalNodes work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/lib/rendering/canonical/compileCanonicalNode.ts` around lines 111 - 121,
Update compileCanonicalNode and compileCanonicalNodes to avoid calling
compileCanonicalNodeHtml for each node’s full subtree solely to obtain its
opening element. Use an opening-tag-only compilation path or serialize and walk
the tree once, while preserving the existing opening parsing and recursive
output behavior.
| function extractHeadHtml(html: string): string { | ||
| return html.match(/<head>([\s\S]*?)<\/head>/iu)?.[1]?.trim() ?? ""; | ||
| } | ||
|
|
||
| function extractStylesheet(html: string): string { | ||
| return Array.from(html.matchAll(/<style(?:\s[^>]*)?>([\s\S]*?)<\/style>/giu)) | ||
| .map((match) => match[1]?.trim() ?? "") | ||
| .filter(Boolean) | ||
| .join("\n\n"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Structure is discarded into an HTML string and then recovered by regex in two places. The canonical pipeline holds the head nodes, the stylesheet, and the per-region body markup, serializes them into one HTML string, and both sites then parse that string back with tag patterns. Both patterns assume a bare <head>, stop at the first closing tag, and cannot distinguish generated content from author-supplied content.
aria/lib/rendering/canonical/compileRenderDocument.ts#L37-L46: return the head nodes and the generated stylesheet fromnodesToHtmlDocumentand assign them directly, instead of callingextractHeadHtmlandextractStylesheeton the serialized output.aria/lib/rendering/renderPageHtml.ts#L156-L160: expose the compiledRenderDocumentV1(or its head and body parts) fromrenderPageDslToHtml, then buildheadHtmlandbodyHtmlfrom those fields instead of matching<head>and<body>inrendered.html.
📍 Affects 2 files
aria/lib/rendering/canonical/compileRenderDocument.ts#L37-L46(this comment)aria/lib/rendering/renderPageHtml.ts#L156-L160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/lib/rendering/canonical/compileRenderDocument.ts` around lines 37 - 46,
Replace the regex-based extraction in
aria/lib/rendering/canonical/compileRenderDocument.ts lines 37-46 by returning
and assigning the head nodes and generated stylesheet directly from
nodesToHtmlDocument, preserving the compiled structure. In
aria/lib/rendering/renderPageHtml.ts lines 156-160, expose the compiled
RenderDocumentV1 or its head/body fields from renderPageDslToHtml and construct
headHtml and bodyHtml from those fields instead of matching tags in
rendered.html.
| if ( | ||
| normalizedNodeType === "list" && | ||
| (!node.children || node.children.length === 0) && | ||
| Array.isArray(renderProps.items) && | ||
| renderProps.items.length > 0 | ||
| ) { | ||
| const listItemsHtml = renderProps.items | ||
| .map( | ||
| (item) => | ||
| `${indentStr} <li>${escapeHTML(typeof item === "string" ? item : String(item ?? ""))}</li>`, | ||
| ) | ||
| .join("\n"); | ||
|
|
||
| return `${indentStr}${openTag(tag, attrsStr)}\n${listItemsHtml}\n${indentStr}</${tag}>`; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
props.items handling for dl lists differs between the canonical renderer and Stage. Only Stage skips item generation when the resolved tag is dl. The canonical renderer still emits li children, so a description list that carries props.items renders invalid <dl><li>…</li></dl> in published HTML while Stage renders an empty dl.
aria/lib/rendering/canonical/renderDocumentHtml.ts#L1147-L1161: addtag !== "dl"to the condition soliitems are not emitted inside adl.aria/admin/features/Stage/components/StageFrame.vue#L1865-L1868: keep the existingdlguard and confirm it matches the canonical rule after the renderer fix.
📍 Affects 2 files
aria/lib/rendering/canonical/renderDocumentHtml.ts#L1147-L1161(this comment)aria/admin/features/Stage/components/StageFrame.vue#L1865-L1868
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/lib/rendering/canonical/renderDocumentHtml.ts` around lines 1147 - 1161,
The canonical list-item generation condition in
aria/lib/rendering/canonical/renderDocumentHtml.ts lines 1147-1161 must also
require the resolved tag not be "dl", preventing li children inside description
lists. Keep the existing dl guard in
aria/admin/features/Stage/components/StageFrame.vue lines 1865-1868 unchanged;
it already matches the corrected canonical behavior.
| { | ||
| id: "parity-unordered-list", | ||
| type: "List", | ||
| props: { | ||
| "data-parity-id": "unordered-list", | ||
| element: "ul", | ||
| }, | ||
| styles: {}, | ||
| children: [ | ||
| { | ||
| id: "parity-unordered-list-item", | ||
| type: "ListItem", | ||
| props: { element: "li", content: "Unordered item" }, | ||
| styles: {}, | ||
| children: [], | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| id: "parity-ordered-list", | ||
| type: "List", | ||
| props: { | ||
| "data-parity-id": "ordered-list", | ||
| ordered: true, | ||
| }, | ||
| styles: {}, | ||
| children: [ | ||
| { | ||
| id: "parity-ordered-list-item", | ||
| type: "ListItem", | ||
| props: { element: "li", content: "Ordered item" }, | ||
| styles: {}, | ||
| children: [], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 'P0_CANVAS_PARITY_NODE_IDS' ariaRepository: ariabuilder/aria
Length of output: 3221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== p0CanvasFixture around exports =="
sed -n '240,290p' aria/tests/rendering-parity/p0CanvasFixture.ts
echo
echo "== browserSnapshotCollector outline/usages =="
rg -n -C4 'parityIds|data-parity-id|collectBrowserSurfaceSnapshot|parityIds:' aria/tests/rendering-parity aria -g "*.ts"
echo
echo "== candidate collector file size =="
wc -l aria/tests/rendering-parity/browserSnapshotCollector.ts aria/tests/rendering-parity/canvas-parity.spec.ts aria/tests/rendering-parity/p0CanvasFixture.tsRepository: ariabuilder/aria
Length of output: 36753
Include the new parity node ids in P0_CANVAS_PARITY_NODE_IDS.
P0_CANVAS_PARITY_NODE_IDS is passed to collectBrowserSurfaceSnapshot and drives per-node parity collection. Add description-list, unordered-list, and ordered-list so the new list fixtures are covered by the P0 parity checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aria/tests/rendering-parity/p0CanvasFixture.ts` around lines 214 - 249, Add
the new fixture IDs—parity-description-list, parity-unordered-list, and
parity-ordered-list—to P0_CANVAS_PARITY_NODE_IDS so
collectBrowserSurfaceSnapshot includes all three list nodes in P0 parity
collection.
1 participant
Summary
Validation
No release, version bump, migration, bulk rewrite, or Phase 6 implementation is included.
Summary by CodeRabbit