Fix xmldom iteration and circular dependency in adt-client#56
Merged
ThePlenkov merged 5 commits intomainfrom Mar 2, 2026
Merged
Fix xmldom iteration and circular dependency in adt-client#56ThePlenkov merged 5 commits intomainfrom
ThePlenkov merged 5 commits intomainfrom
Conversation
… by merge commit) The merge-from-main commit 12e09ef re-introduced `@abapify/adk: "*"` into packages/adt-client/package.json. adt-client never actually imports from adk (only has comments referencing it). Removing the dep permanently breaks the circular dependency: adk→adt-client→adk. Co-authored-by: ThePlenkov <6381507+ThePlenkov@users.noreply.github.com>
…lar dep actions/checkout@v6 for pull_request events (without explicit ref) checks out GitHub's auto-created merge commit (refs/pull/N/merge). That merge commit is a 3-way merge: base has no @abapify/adk dep, main added it, PR branch didn't touch it → git re-applies main's addition → circular dep returns in working directory. Fix: use ref: github.event.pull_request.head.sha || github.sha - PR events: checks out actual PR HEAD (correct package.json, no circular dep) - Push events: github.event.pull_request.head.sha is empty → falls back to github.sha (pushed commit SHA) — same behaviour as before - nrwl/nx-set-shas still computes the correct affected range from git history Also document the 3-way merge pitfall in .claude/skills/nx-ci. Co-authored-by: ThePlenkov <6381507+ThePlenkov@users.noreply.github.com>
The root tsdown.config.ts is a shared config base, not a package entry point. Filtering it out prevents nx-tsdown from creating a spurious build target on the workspace root project which has no src/ directory or package.json exports. https://claude.ai/code/session_019C2n4q24du7seSEKsRvyC4
@xmldom/xmldom's NamedNodeMap and NodeList do not implement Symbol.iterator, so for...of loops over attributes/childNodes throw "is not iterable". Replace with index-based loops throughout dom-utils.ts and parse.ts. Also fix two additional bugs: - xml/parse.ts: guard against null doc from DOMParser (empty string input) to throw "Invalid XML: missing root element" instead of a TypeError - xsd/resolve.ts: treat empty string targetNamespace as "no namespace" so chameleon schema elements (e.g. abapGit from abapgit.xsd) are correctly merged into the resolved output These bugs were surface-level hidden because the affected packages were not "affected" in previous CI runs. Changing nx-tsdown/src/plugin.ts (an NX plugin) marks all projects as affected, running all tests. https://claude.ai/code/session_019C2n4q24du7seSEKsRvyC4
Review Summary by Qodo
WalkthroughsDescription• Fix xmldom compatibility by replacing for...of loops with index-based iteration • Remove circular dependency between adt-client and adk packages • Fix CI checkout to use PR head SHA instead of auto-created merge commit • Exclude workspace root tsdown.config.ts from nx-tsdown plugin • Add comprehensive Nx CI skill documentation for local development Diagramflowchart LR
A["xmldom Iteration Fix"] --> B["Index-based loops in dom-utils.ts"]
C["Circular Dependency"] --> D["Remove @abapify/adk from adt-client"]
E["CI Improvements"] --> F["Checkout PR head SHA"]
E --> G["Exclude root tsdown.config.ts"]
H["Documentation"] --> I["Add nx-ci skill guide"]
File Changes1. packages/ts-xsd/src/xml/dom-utils.ts
|
Code Review by Qodo
1. Fork PR CI checkout
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces documentation for Nx CI workflows, updates CI configuration to use PR-specific history, removes a dependency, and refactors DOM utilities and schema parsing to support xmldom's non-iterable collections while improving namespace resolution and filtering workspace-root configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Contributor
|
View your CI Pipeline Execution ↗ for commit 6af06be
☁️ Nx Cloud last updated this comment at |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+21
to
+25
| # For pull_request events, check out the PR head (not GitHub's auto-created | ||
| # merge commit). This prevents 3-way merge from re-introducing deps that | ||
| # the PR is supposed to remove. nrwl/nx-set-shas still computes the correct | ||
| # affected range from git history. Falls back to github.sha for push events. | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} |
There was a problem hiding this comment.
1. Fork pr ci checkout 🐞 Bug ⛯ Reliability
The CI workflow checks out github.event.pull_request.head.sha but does not set repository to the PR head repo. For fork-based pull requests, that SHA is not present in the base repo, so the checkout step can fail and CI won’t run for external contributors.
Agent Prompt
## Issue description
CI currently checks out `github.event.pull_request.head.sha`, but `actions/checkout` defaults to the base repo unless `repository` is provided. For fork PRs, the head SHA exists only in the fork repo, so checkout can fail and CI won't run.
## Issue Context
Workflow triggers on `pull_request`, so fork PRs are a normal scenario for public repos. The current change explicitly prefers PR head SHA.
## Fix Focus Areas
- .github/workflows/ci.yml[18-26]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes compatibility issues with xmldom's DOM implementation and resolves a circular dependency between
adt-clientandadkpackages. It also includes CI improvements and documentation for the Nx CI workflow.Key Changes
xmldom Compatibility Fixes
for...ofloops with index-based iteration inpackages/ts-xsd/src/xml/dom-utils.tsandpackages/ts-xsd/src/xsd/parse.tsNamedNodeMapandNodeListare not iterable despite being array-likegetAttributeValue(),getChildElements(),getAllChildElements(),getTextContent(), andextractXmlns()to uselength-based loopsCircular Dependency Resolution
@abapify/adkdependency frompackages/adt-client/package.jsonadt-client→adk→adt-clientXML Parsing Improvements
packages/ts-xsd/src/xml/parse.ts:doc.documentElementtodoc?.documentElementfor safer accessXSD Resolution Logic
packages/ts-xsd/src/xsd/resolve.ts:!currentNsinstead ofcurrentNs === undefined)""whentargetNamespaceis absentNx Plugin Formatting
tools/nx-tsdown/src/plugin.ts:.filter()to exclude root-leveltsdown.config.tsfilesCI & Documentation
GitHub Actions workflow (``.github/workflows/ci.yml`):
refcheckout for pull requests to use PR head SHA instead of auto-created merge commitNew Nx CI skill documentation (
.claude/skills/nx-ci):nx affectednx affectedis preferred overrun-manyImplementation Details
https://claude.ai/code/session_019C2n4q24du7seSEKsRvyC4
Summary by CodeRabbit
Documentation
Bug Fixes
Chores