Fixes from architecture review (4 critical, 10 warnings):
Critical:
- Fix selectNode/setNode docs to accurately describe prop-key
navigation behavior including array support and prop-key writes
- Document RenderContext/Density exported types in host-config
- Resolve ADR dual status ambiguity with clarifying note in README
(frontmatter status = editorial, body Status = decision)
- Effect types already addressed in prior commit
Warnings addressed:
- Add Fragment re-export note to jsx-runtime section in
build-distribution
- Document childCtx/transformCtx helper functions in transforms.md
- Document render() accepting non-root UNode in host-config
- Add Value.Hash re-entrancy constraint to reconciler.md
- Add true-passthrough constraint and h('root') special case
to element-factory constraints
- Add _idCounter bundling caveat note
Review document added at docs/reviews/architecture-review-2026-05-18.md
with full findings, source verification table, and recommendations.
26 KiB
Architecture Review: @alkdev/ujsx
Date: 2026-05-18
Reviewer: Architecture Reviewer (subagent)
Scope: All docs/architecture/ documents + source verification
Summary
The @alkdev/ujsx architecture documentation is thorough, well-structured, and internally consistent across 15 documents. The docs accurately reflect the current source code implementation in almost all respects. The core architectural story — an HTML-agnostic, signal-driven, host-configurable reactive tree system — is clearly articulated with well-documented design decisions (ADRs). The documentation honestly catalogs known gaps and open questions alongside constraints. Key issues include: an inconsistency in how selectNode handles prop navigation vs. how the docs describe it, missing documentation for childCtx/ctx helper functions in the transform system, an undocumented RenderContext type visible in public exports, and several cross-reference paths that use different conventions (relative vs. bare). No document exceeds 265 lines.
Metrics
- Critical issues: 4
- Warnings: 10
- Suggestions: 8
- Overall: needs revision before stabilization
Critical Issues
1. selectNode prop navigation behavior differs from docs
Location: docs/architecture/pointers.md, lines 66-68
Issue: The docs state that a non-numeric segment resolves a prop by checking typeof propVal === "object", implying that only object-typed prop values can be navigation targets. However, the source code (src/core/pointer.ts, line 41) performs the check:
if (propVal !== undefined && typeof propVal === "object" && propVal !== null)
This means Array values (which are typeof === "object") also qualify as navigation targets, but the docs only mention "object nodes" and UElement-like structures. More critically, the docs state that navigating into props.title (where title is a string) returns undefined, but the example path ["props", "title"] is misleading — the path ["props"] would try to resolve "props" as a children index or prop key, not traverse into the props object. The path resolution algorithm in the docs doesn't match how the source actually works for prop access.
Recommendation: Rewrite the path segment resolution table to accurately describe the source behavior:
- Numeric string →
children[index] - Non-numeric string →
props[segment], and if the value is a non-null object (including arrays), navigate into it; otherwise returnundefined - Remove the confusing
["props", "title"]example, or clarify that"props"is a prop key lookup, not a special accessor.
2. setNode prop-key path behavior undocumented for non-array segments
Location: docs/architecture/pointers.md, lines 79-109
Issue: The docs describe setNode in terms of children array index updates, but the source code (src/core/pointer.ts, lines 51-69) also handles a branch for non-numeric path segments that sets values into the props object:
const props = { ...rootEl.props, [head as string]: value };
return { ...rootEl, props } as UElement;
This means setNode(root, ["title"], someNode) would set root.props.title = someNode, which is a significant behavior not documented at all. The docs' "Edge cases" section mentions empty paths and paths into primitives, but not prop-key navigation for writes.
Recommendation: Add a section documenting non-numeric path segment behavior in setNode, explaining that it shallow-merges a key into props, and note the constraint that value must be a valid PropValue (or UNode) for the result to remain type-safe.
3. RenderContext and Density types are exported but undocumented
Location: src/mod.ts line 12; src/core/context.ts lines 50-54
Issue: The barrel export in mod.ts re-exports Density, Direction, and RenderContext from context.ts. However, render-context.md does not exist, and neither Density nor RenderContext is documented anywhere in the architecture docs. The context.ts source file defines a RenderContext interface that extends ContextValue with a direction field, and a Density type with three string literals. The host-config doc mentions the Context class briefly (line 79), but does not document Density or RenderContext.
Recommendation: Either add documentation for RenderContext and Density in a new or existing architecture doc, or add a constraint note explaining that these are exported for downstream consumer use and are not core to UJSX's internal architecture. At minimum, document how Density is intended to be consumed by hosts.
4. ADR frontmatter status: draft contradicts ADR body Status: Proposed
Location: All ADRs (decisions/001 through decisions/005)
Issue: Each ADR file has YAML frontmatter with status: draft, which follows the document lifecycle convention defined in README.md (lines 104-118). However, each ADR also has an internal **Status**: Proposed field in its body. The README states: "ADR documents use a separate Status field in their body" and lists statuses as Proposed, Accepted, Deprecated, or Superseded. This creates two status fields that could diverge — the frontmatter says draft (a document lifecycle status) while the body says Proposed (an ADR decision status). If an implementer reads status: draft and thinks "this decision isn't finalized," they may ignore the ADR, even though ADRs document decisions that have already been made.
Recommendation: Clarify the relationship between the two status fields. Either: (a) rename one to avoid confusion (e.g., use doc_status in frontmatter), (b) add a note in the README explaining that frontmatter status is the document's editorial status while the body Status is the decision's status, or (c) unify them. This is critical because the frontmatter convention is supposed to govern whether implementation should begin, and the mismatch could cause confusion about whether these decisions are stable enough to implement against.
Warnings
1. Fragment JSX runtime export missing from build-distribution docs
Location: docs/architecture/build-distribution.md, lines 58-59 (sub-path exports list)
Issue: The sub-path exports table lists jsx-runtime but documents it as mapping to src/core/jsx-runtime.ts. The actual jsx-runtime.ts file re-exports Fragment in addition to jsx, jsxs, and jsxDEV. The element-factory.md documents Fragment as a factory function, but build-distribution.md doesn't mention that Fragment is available from the jsx-runtime sub-path. This could confuse consumers who expect Fragment to be importable from the JSX runtime but can't find it documented there.
Recommendation: Add a note to build-distribution.md that jsx-runtime also exports Fragment.
2. childCtx and ctx (as transformCtx) are public exports but undocumented as architecture concepts
Location: src/mod.ts line 22; src/transform/registry.ts lines 44-63
Issue: The barrel export re-exports childCtx, matchesSchema, and ctx as transformCtx from the transform module. matchesSchema is documented in transforms.md, but childCtx and ctx (exported as transformCtx) are not documented anywhere in the architecture docs. These are convenience functions for creating transform contexts and pushing ancestors. Consumers importing from @alkdev/ujsx/transform would encounter them with no architectural guidance.
Recommendation: Add documentation for childCtx and ctx/transformCtx in transforms.md, explaining their purpose as factory functions for creating TransformContext instances.
3. The render() method processes URoot children, but docs and source differ in detail
Location: docs/architecture/host-config.md, lines 96-103; src/host/config.ts, lines 97-101
Issue: The docs describe render() as calling mountNode recursively, but the actual source code does:
const payloadChildren = isURoot(node) ? (node as URoot).children : [node];
for (const child of payloadChildren) { mountNode(child, undefined); }
This means if a non-root node is passed to render(), it wraps it in an array and mounts it as a single top-level node without a URoot container. The documentation in host-config.md's "Mount Pipeline" section describes URoot nodes as transparent containers, but doesn't describe this render() behavior of accepting non-root nodes. The docs describe render(node) as "Mount the tree" without noting that non-root nodes are wrapped.
Recommendation: Document that render() accepts any UNode, and if the node is not a URoot, it mounts the node directly as a single top-level element without a root container.
4. URoot type casting in h() source differs from docs
Location: src/core/h.ts, lines 9-15; docs/architecture/element-factory.md, lines 20-28
Issue: The docs describe h() as returning UElement | URoot depending on the type argument. The source code matches this. However, the source uses as URoot cast for the root case and as UElement for the default case. The docs don't mention this casting, which is fine, but the docs also don't mention that when type === "root", the type argument of h() is widened from UType (which includes string | ComponentFn) to the "root" literal. This means h("root", ...) has special runtime behavior that bypasses the normal string-type element creation path.
Recommendation: Add a constraint or note that h("root", ...) is a special case that produces a URoot rather than a UElement, and that the string "root" is effectively a reserved type string.
5. Research cross-references use inconsistent formats
Location: Multiple docs
Issue: Cross-references to research documents use different path formats across documents:
README.mduses../research/reconciler/01-reactive-host-bridge.md(relative from architecture/)schema.mdusesdocs/research/reconciler/00-KEY-FIELD-DESIGN.md(project-relative)reactive-layer.mdusesdocs/research/reconciler/01-reactive-host-bridge.md(project-relative)build-distribution.mduses forward references without paths (just names)
Since all architecture docs live in docs/architecture/, project-relative paths like docs/research/... will not resolve correctly from the architecture directory. Only the ../research/... style works for relative links from architecture docs.
Recommendation: Standardize all research cross-references to use consistent relative paths from the architecture docs directory (e.g., ../research/reconciler/01-reactive-host-bridge.md).
6. Open questions about Value.Hash non-reentrancy are not addressed in constraints
Location: docs/architecture/reconciler.md, lines 196-199; section "Constraints"
Issue: The reconciler doc documents that Value.Hash uses a global mutable accumulator and is "not re-entrant" — you cannot call it from within a computed or effect that is itself triggered by a hash comparison. This is listed as a constraint on Value.Hash but not in the main "Constraints" section of the document. It's buried in a subsection. This is a significant constraint that could cause subtle bugs if an implementer skims the constraints section.
Recommendation: Add this to the main "Constraints" section of reconciler.md: "Value.Hash is not re-entrant — it must not be called from within a reactive computation (computed/effect)."
7. true not being filtered from children is a potential footgun not highlighted in element-factory constraints
Location: docs/architecture/element-factory.md, lines 36-37
Issue: The docs note that true is not filtered (unlike false), and state: "This is consistent with how React treats true in JSX (it renders nothing in DOM but is not stripped at the factory level)." However, this means <div>{condition}</div> where condition is true will produce a UElement with true as a UPrimitive child, which most hosts will need to handle explicitly. This is only mentioned in the child normalization section, not in the Constraints section. For a universal library, this behavior could surprise hosts that expect true to be treated like false/null.
Recommendation: Add to the Constraints section: "true passes through h() as a UPrimitive child. Hosts that handle true differently from other primitives should filter it in their createTextInstance."
8. createRoot _idCounter is shared across all callers — no module isolation documented
Location: docs/architecture/element-factory.md, lines 56-61
Issue: The docs state: "The counter is a module-level variable (let _idCounter = 0). It is: Not thread-safe — JavaScript is single-threaded, and root creation is not a concurrent operation." While this is technically correct, the more important concern is that any bundler that deduplicates the module will share the counter across all consumers. If two different packages both depend on @alkdev/ujsx, they may or may not share the counter depending on bundler behavior. This could lead to unexpected root_3 from package A conflicting with root_3 from package B.
Recommendation: Expand the constraint note to mention that bundled consumers may or may not share the counter, and that id values should be treated as unique within a single module instance only, not globally unique. Consider suggesting createRoot(uniquePrefix + counter) for disambiguation.
9. Missing performance/scalability discussion
Location: Overall — no document addresses performance characteristics
Issue: No architecture document discusses performance targets, scalability considerations, or benchmarks. The reconciler doc mentions that "90% of updates bypass tree diffing" and that fiber trees are "cheap to update," but there are no quantified performance targets (latency p50/p99, throughput, memory budget). For a library that positions itself as suitable for workflow DAGs and desktop UI rendering, the absence of performance context means implementers can't validate that the architecture meets their needs.
Recommendation: Consider adding a "Performance Model" section to one of the core docs (or the README) that outlines expected performance characteristics: tree sizes the system is designed for, update propagation latency expectations, and memory usage patterns. This doesn't need hard benchmarks yet, but should establish design targets (e.g., "designed for trees of 10-10,000 nodes" or "signal propagation should complete in <1ms for prop updates").
10. No error handling strategy documented
Location: Overall
Issue: No architecture document discusses what happens when things go wrong at the system level. The host-config doc has an open question about function component error propagation (line 206), and the transform doc notes that transform() throws if no rule matches (line 35). But there's no overarching error handling strategy: When should UJSX throw vs. emit an error event vs. silently no-op? What happens if createInstance throws? What happens if a signal computation throws?
Recommendation: Add an "Error Handling" section to the README or create a separate error-handling.md doc that establishes UJSX's error philosophy: which errors are programmer errors (throw), which are operational (emit event), and which are silently handled (no-op with logging).
Suggestions
1. Consider documenting the isUElement null check
Location: docs/architecture/schema.md, line 127
The type guard table documents isUElement as checking typeof === "object" && "type" in node && "props" in node && "children" in node && node.type !== "root". The source code (src/core/schema.ts line 66) adds node !== null to the check. While null would fail the "type" in node check, explicitly documenting the null guard would make the type guard specification complete and help implementers writing host-specific guards.
2. Add a naming convention note for Direction string values
Location: docs/architecture/transforms.md, lines 99-106
The Direction type uses arrow characters (ujsx→mdast) in string literals. These are defined in context.ts and also appear in RenderContext. This is unconventional and could cause issues with IDE autocomplete or linting rules that warn on non-ASCII strings. Consider documenting why Unicode arrows were chosen, or noting that the convention should be maintained consistently.
3. Document the UjsxEnvelope convenience type
Location: src/core/events.ts, lines 28-31; docs/architecture/events.md
The source exports a UjsxEnvelope type alias that parameterizes EventEnvelope with UjsxEventMap — essentially EventEnvelope<TKeyof, UjsxEventMap[TKeyof]>. This convenience type is not documented in events.md. While not critical, it's a public export that consumers will see.
4. Consider adding a "Quick Start" or "Concepts" doc for new readers
Location: Overall
The architecture docs are detailed and reference-heavy. A new reader would benefit from a single-page "Concepts" overview that introduces UNode, h(), signals, and HostConfig in 50 lines before directing to the detailed docs. The README partially serves this role but focuses on current-state/gaps rather than a conceptual introduction.
5. Consider documenting the Context class more fully
Location: docs/architecture/host-config.md (partial); src/core/context.ts
The Context class has get(), set(), subscribe(), fork(), and a signal getter, but these are only briefly mentioned in host-config.md. The class is exported from the barrel and appears in the context export. A short section explaining its reactive context model would help consumers who import it directly.
6. Standardize ADR status tracking
Location: All ADR documents
All five ADRs have Status: Proposed, but the architectural decisions they describe are already driving implementation (e.g., ADR-003's choice of Preact signals is clearly implemented). Consider advancing ADR statuses to Accepted for decisions that are already in production code. The Proposed status on implemented decisions undermines the ADR's authority.
7. Consider a centralized glossary
Location: Overall
Terms like "fiber," "host," "instance," "primitive," and "reconciler" are defined in individual docs but would benefit from a centralized glossary. For example, "instance" means different things in different contexts (host instance vs. UElement instance), and readers may miss the distinction.
8. Add line counts or complexity indicators to file references
Location: All source references
Each architecture doc references its source file (e.g., "Source: src/core/schema.ts"). Adding approximate line counts (e.g., "Source: src/core/schema.ts (~80 lines)") would help readers calibrate their expectations about implementation complexity.
Strengths
-
Honest gap documentation: Every document explicitly catalogs "Known Gaps," "Open Questions," and "Constraints." This is exceptional practice — implementers can immediately see what doesn't work yet and what decisions are still pending. The README's "Known gaps" section with cross-references to relevant docs and ADRs is particularly well done.
-
Decision records (ADRs): All five ADRs follow a consistent format (Context, Alternatives Considered, Decision, Consequences) and document not just what was decided but why. The alternatives sections are substantive — they explain why each rejected option was considered and why it was rejected, not just that it was.
-
Architecture-consistent language: The documents use consistent terminology throughout. A "UNode" always means
UPrimitive | UElement | URoot, a "host" always refers to aHostConfigimplementation, a "fiber" always means the reconciler's internal node type. This consistency makes cross-document references meaningful. -
Clear separation of concerns: The docs clearly delineate what each layer owns. The reactive layer owns signals, the host layer owns instances, the transform layer owns conversion, and the pointer layer owns targeted read/write. No layer bleeds into another's responsibilities without explicit bridging (which is documented as a planned reconciler feature).
-
"The tree is the truth" principle: The README's stated core principle ("The tree is the truth. Hosts are interpreters.") is consistently reflected across all docs. Every feature is motivated in terms of what it means for the tree, not for any specific host.
-
Practical consumer context: The README's "Consumer Context" section (Flowgraph, Desktop UI, OpenCode Plugin) grounds the architecture in real use cases, making it easier to evaluate trade-offs with concrete scenarios in mind.
-
Document size discipline: No document exceeds 265 lines (reconciler.md). Each document covers exactly one topic. The README provides navigation. This makes the documentation approachable and scannable.
Verification: Source vs. Documentation
Accuracies (docs match source)
| Aspect | Doc | Source | Status |
|---|---|---|---|
UJSX TypeBox Module schema |
schema.md | src/core/schema.ts | ✅ Match |
| TypeScript types (UPrimitive, PropValue, etc.) | schema.md | src/core/schema.ts | ✅ Match (except key — see below) |
key not yet on UElement |
schema.md ("Known Gaps") | src/core/schema.ts (no key field) |
✅ Consistently documented as not yet implemented |
isUElement discriminator logic |
schema.md | src/core/schema.ts:66 | ✅ Match (note: null guard in source not in docs — see suggestion) |
h() factory behavior |
element-factory.md | src/core/h.ts | ✅ Match |
createRoot auto-ID counter |
element-factory.md | src/core/h.ts:3,24-29 | ✅ Match |
Fragment returns UNode[] |
element-factory.md | src/core/h.ts:43-44 | ✅ Match |
jsx/jsxs/jsxDEV aliases |
element-factory.md | src/core/h.ts:47-49 | ✅ Match |
ReactiveNode interface |
reactive-layer.md | src/core/reactive.ts:4-8 | ✅ Match |
reactiveComponent and reactiveElement behavior |
reactive-layer.md | src/core/reactive.ts:10-47 | ✅ Match |
ReactiveRoot class |
reactive-layer.md | src/core/reactive.ts:49-89 | ✅ Match |
ReactiveRoot.render() overwriting renderDisposer |
reactive-layer.md ("Known Gaps") | src/core/reactive.ts:74 | ✅ Documented as a known hazard |
dispose: () => {} no-op |
reactive-layer.md ("Known Gaps") | src/core/reactive.ts:25,46 | ✅ Consistently documented |
HostConfig interface |
host-config.md | src/host/config.ts:5-30 | ✅ Match |
Root interface |
host-config.md | src/host/config.ts:32-39 | ✅ Match |
createRoot() behavior |
host-config.md | src/host/config.ts:41-109 | ✅ Match |
| Mount-only rendering | host-config.md ("Known Gaps") | src/host/config.ts (no re-render logic) | ✅ Match |
unmount() stub behavior |
host-config.md, lifecycle.md | src/host/config.ts:105-108 | ✅ Match |
Direction type values |
transforms.md | src/core/context.ts:50 | ✅ Match |
TransformRule interface |
transforms.md | src/transform/registry.ts:14-21 | ✅ Match |
TransformRegistry methods |
transforms.md | src/transform/registry.ts:23-42 | ✅ Match |
EventEnvelope interface |
events.md | src/core/events.ts:1-5 | ✅ Match |
PubSubLike interface |
events.md | src/core/events.ts:7-17 | ✅ Match |
UjsxEventMap type |
events.md | src/core/events.ts:19-26 | ✅ Match |
createPubSubEmitter function |
events.md | src/core/events.ts:33-42 | ✅ Match |
proxyEventEmitter function |
events.md | src/core/events.ts:45-51 | ✅ Match |
ValuePointer class |
pointers.md | src/core/pointer.ts:5-29 | ✅ Match |
selectNode function signature |
pointers.md | src/core/pointer.ts:31-49 | ✅ Match (behavior differs — see critical issue #1) |
setNode function signature |
pointers.md | src/core/pointer.ts:51-69 | ✅ Match (behavior differs — see critical issue #2) |
| Package dependencies | build-distribution.md | package.json | ✅ Match |
| Sub-path exports | build-distribution.md | package.json exports field | ✅ Match (except Fragment in jsx-runtime — see warning #1) |
| Barrel exports in mod.ts | build-distribution.md | src/mod.ts | ⚠️ Partial — see critical issue #3 |
Context class defaults |
host-config.md (line 79, line 194) | src/core/context.ts:11-15 | ✅ Match |
Date.now() event IDs |
host-config.md, events.md | src/host/config.ts:57,75,103 | ✅ Consistently documented as placeholder |
Discrepancies
| Aspect | Doc Says | Source Does | Severity |
|---|---|---|---|
selectNode prop navigation |
Only object-typed props are navigable | Source also navigates arrays (which are typeof === "object") |
Critical — doc is incomplete |
setNode prop-key writing |
Not documented | Source handles non-numeric segments by setting props[segment] = value |
Critical — behavior gap |
RenderContext and Density |
Not documented | Exported from mod.ts and source | Critical — public API undocumented |
| ADR dual status | Frontmatter draft vs body Proposed |
Both present, semantics unclear | Critical — governance confusion |
Fragment in jsx-runtime |
Not in exports doc | Source re-exports Fragment from jsx-runtime | Warning — minor coverage gap |
childCtx, transformCtx |
Not in transforms.md | Public exports from registry.ts | Warning — public API gap |
isUElement null check |
Not in docs | Source includes node !== null |
Suggestion — minor |
Recommendations
-
Address all four critical issues before marking the architecture documents as
stable: fixselectNode/setNodedocs to match source, documentRenderContext/Density, and resolve the ADR status ambiguity. -
Standardize cross-reference formats across all docs to use consistent relative paths. Pick one format and apply it everywhere.
-
Advance ADR statuses from
ProposedtoAcceptedfor decisions that are already driving implementation (particularly ADR-002, ADR-003, and ADR-004 whose decisions are reflected in current source code). -
Add error handling philosophy as a cross-cutting concern. Even a brief statement like "UJSX throws for programmer errors (invalid arguments) and no-ops for operational errors (invalid paths in selectNode)" would help implementers.
-
Consider performance targets for the reconciler design. The current docs describe algorithms without stating what "good enough" means. This will make it hard to evaluate whether the reconciler implementation meets its design goals.