Repository navigation
fix(swift/core): report UNKNOWN_COMPONENT_TYPE and CYCLIC_REFERENCE from NodeResolver - #3786
karolkulesza wants to merge 8 commits into
Conversation
…rom NodeResolver The Swift resolver reported nothing for a component type missing from its catalog or for a child reference that repeats an ancestor. Report both as GenericErrors through the action handler, with web_core's codes and messages, deduplicated per node (component instance and data path) through the same sets as EXPRESSION_ERROR. ComponentNodeView no longer prints on every render of a node it has no view for.
…own-component-type
…ift reports both errors
There was a problem hiding this comment.
Code Review
This pull request introduces error reporting for cyclic references and unknown component types during node resolution in NodeResolver, ensuring errors are reported once and deduplicated. It also cleans up fallback views in ComponentNodeView and updates the test suite accordingly. The review feedback suggests making the unknown component type check more robust by handling cases where the target catalog is nil, and cautions against completely removing fallback logging in ComponentNodeView to avoid silent failures during host setup mismatches.
ComponentNodeView rendered nothing and said nothing when a node's type had no view builder in the SwiftUI catalogs passed to Surface. Log a warning through os.Logger the first time each catalog and type pair is seen, so a host setup mismatch is visible without logging on every render.
|
Hi @karolkulesza, Thank you for raising the pr to fix the issue. We have verified the pr and it does resolve the issue. I will escalate this to team to review. Thank you for your patience and understanding. |
| if visited.contains(childNodeKey) { | ||
| let dataPath = normalizePath(basePath ?? "/") | ||
| reportResolutionError( | ||
| code: "CYCLIC_REFERENCE", | ||
| message: "Component '\(childID)' at '\(dataPath)' is referenced by one of its own " | ||
| + "descendants; omitting the reference.", | ||
| dedupeKey: "CYCLIC_REFERENCE:\(childNodeKey)" | ||
| ) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Because report(_:dedupeKey:) prefixes dedupeKey with currentComponentID (the referencing parent), CYCLIC_REFERENCE can be emitted multiple times for the same (childID, dataPath) when two descendants both reference the same ancestor (for example a -> [b1, b2] where both b1 and b2 reference a), whereas web_core, Dart, and Python deduplicate CYCLIC_REFERENCE per (componentId, dataPath). In addition, if two mutually cyclic nodes are also siblings (for example root -> [b, a] with a <-> b), resolving a a second time in the same pass under root -> a runs reportedExpressionErrors.filter { !$0.hasPrefix("a:") } in resolveNode's defer and erases the "a:CYCLIC_REFERENCE:b" key recorded earlier in that pass, causing subsequent root updates to re-report the cycle on every render. Also consider using guard !visited.contains(childNodeKey) else for the early exit here to match guard let childIdentity else right below.
Details and suggested fix
For the same-pass overwrite on DAG-shared nodes, resolveNode already inserts nodeKey into resolvedInCurrentPass; only clearing the "\(nodeKey):" prefix on the first visit to nodeKey within a pass (let isFirstVisitInPass = resolvedInCurrentPass.insert(nodeKey).inserted) keeps a second visit in the same pass from wiping errors recorded by the first visit. Deduplicating CYCLIC_REFERENCE across referencing descendants in the same pass (or by childNodeKey) would also avoid duplicate identical messages for a -> [b1, b2] -> a.
For the early-exit guard:
guard !visited.contains(childNodeKey) else {
let dataPath = normalizePath(basePath ?? "/")
reportResolutionError(
code: "CYCLIC_REFERENCE",
message: "Component '\(childID)' at '\(dataPath)' is referenced by one of its own "
+ "descendants; omitting the reference.",
dedupeKey: "CYCLIC_REFERENCE:\(childNodeKey)"
)
return nil
}| let isFirstInPass = currentPassExpressionErrors.insert(key).inserted | ||
| guard isFirstInPass && !reportedExpressionErrors.contains(key) else { | ||
| return | ||
| } |
There was a problem hiding this comment.
Prefer comma-separated conditions (guard a, b else) over && in guard statements to match idiomatic Swift style (keeping isFirstInPass bound above the guard is great so currentPassExpressionErrors.insert(key) always runs before checking reportedExpressionErrors).
Details and suggested fix
| let isFirstInPass = currentPassExpressionErrors.insert(key).inserted | |
| guard isFirstInPass && !reportedExpressionErrors.contains(key) else { | |
| return | |
| } | |
| let isFirstInPass = currentPassExpressionErrors.insert(key).inserted | |
| guard isFirstInPass, !reportedExpressionErrors.contains(key) else { | |
| return | |
| } |
| // Re-rendering the root reuses `b`, so the cycle is not reported again. | ||
| components.addComponent( | ||
| ComponentModel(id: "t1", type: "Text", properties: ["text": .string("Hi")]) | ||
| ) | ||
| components.addComponent( | ||
| ComponentModel( | ||
| id: "root", | ||
| type: "Container", | ||
| properties: ["children": .array([.string("a"), .string("t1")])] | ||
| ) | ||
| ) | ||
| _ = resolver.resolveTree() | ||
| #expect(handler.capturedErrors.count == 1) | ||
| } |
There was a problem hiding this comment.
unknownComponentTypeIsReportedOncePerComponent below verifies that fixing u1 and breaking it again re-arms and reports a new error, whereas cyclicReferenceIsReportedOnce stops after re-rendering root. Adding a repair-and-reintroduce assertion here (matching web_core and Dart's cycle diagnostic tests) would directly cover the cross-pass clearing path for CYCLIC_REFERENCE.
Details and suggested fix
// Breaking the cycle on `b` and reintroducing it reports `CYCLIC_REFERENCE` again.
components.addComponent(
ComponentModel(id: "b", type: "Container", properties: ["children": .array([])])
)
_ = resolver.resolveTree()
#expect(handler.capturedErrors.count == 1)
components.addComponent(
ComponentModel(
id: "b",
type: "Container",
properties: ["children": .array([.string("a")])]
)
)
_ = resolver.resolveTree()
#expect(genericErrors(handler).map(\.code) == ["CYCLIC_REFERENCE", "CYCLIC_REFERENCE"])| let _ = Self.logMissingViewBuilder(for: renderNode) | ||
| EmptyView() | ||
| } | ||
| } | ||
|
|
||
| private var fallbackView: some View { | ||
| print("[A2UI] Component view builder not found for type: \(node.type)") | ||
| return EmptyView() | ||
| private static let logger = Logger(subsystem: "A2UISwiftUI", category: "ComponentNodeView") | ||
|
|
||
| /// Catalog and type pairs already logged, so a missing view builder is logged once per | ||
| /// process instead of on every render. | ||
| @MainActor private static var loggedMissingViewBuilders: Set<String> = [] | ||
|
|
||
| /// Logs a node type that has no view builder in the SwiftUI catalogs passed to `Surface`. | ||
| /// | ||
| /// A type missing from the surface's catalog was already reported to the agent by the | ||
| /// resolver as `UNKNOWN_COMPONENT_TYPE`. A catalog type with no view builder is a host setup | ||
| /// mismatch that only this log points at. | ||
| @MainActor private static func logMissingViewBuilder(for node: Node) { | ||
| let catalogID = node.catalogID ?? "default" | ||
| guard loggedMissingViewBuilders.insert("\(catalogID)/\(node.type)").inserted else { | ||
| return | ||
| } | ||
| let message = "No view builder for component type '\(node.type)' in catalog '\(catalogID)'." | ||
| logger.warning("\(message, privacy: .public)") | ||
| } |
There was a problem hiding this comment.
Two small improvements in logMissingViewBuilder: pass defaultCatalogID from the environment as a fallback before "default" so it matches Surface.render(node:using:defaultCatalogID:) when node.catalogID is nil, and interpolate node.type and catalogID directly into logger.warning(...) instead of pre-building let message: String so unified logging preserves a static format string and avoids eager string formatting.
Details and suggested fix
| let _ = Self.logMissingViewBuilder(for: renderNode) | |
| EmptyView() | |
| } | |
| } | |
| private var fallbackView: some View { | |
| print("[A2UI] Component view builder not found for type: \(node.type)") | |
| return EmptyView() | |
| private static let logger = Logger(subsystem: "A2UISwiftUI", category: "ComponentNodeView") | |
| /// Catalog and type pairs already logged, so a missing view builder is logged once per | |
| /// process instead of on every render. | |
| @MainActor private static var loggedMissingViewBuilders: Set<String> = [] | |
| /// Logs a node type that has no view builder in the SwiftUI catalogs passed to `Surface`. | |
| /// | |
| /// A type missing from the surface's catalog was already reported to the agent by the | |
| /// resolver as `UNKNOWN_COMPONENT_TYPE`. A catalog type with no view builder is a host setup | |
| /// mismatch that only this log points at. | |
| @MainActor private static func logMissingViewBuilder(for node: Node) { | |
| let catalogID = node.catalogID ?? "default" | |
| guard loggedMissingViewBuilders.insert("\(catalogID)/\(node.type)").inserted else { | |
| return | |
| } | |
| let message = "No view builder for component type '\(node.type)' in catalog '\(catalogID)'." | |
| logger.warning("\(message, privacy: .public)") | |
| } | |
| let _ = Self.logMissingViewBuilder(for: renderNode, defaultCatalogID: defaultCatalogID) | |
| EmptyView() | |
| } | |
| } | |
| private static let logger = Logger(subsystem: "A2UISwiftUI", category: "ComponentNodeView") | |
| /// Catalog and type pairs already logged, so a missing view builder is logged once per | |
| /// process instead of on every render. | |
| @MainActor private static var loggedMissingViewBuilders: Set<String> = [] | |
| /// Logs a node type that has no view builder in the SwiftUI catalogs passed to `Surface`. | |
| /// | |
| /// A type missing from the surface's catalog was already reported to the agent by the | |
| /// resolver as `UNKNOWN_COMPONENT_TYPE`. A catalog type with no view builder is a host setup | |
| /// mismatch that only this log points at. | |
| @MainActor private static func logMissingViewBuilder( | |
| for node: Node, | |
| defaultCatalogID: String? | |
| ) { | |
| let catalogID = node.catalogID ?? defaultCatalogID ?? "default" | |
| guard loggedMissingViewBuilders.insert("\(catalogID)/\(node.type)").inserted else { | |
| return | |
| } | |
| logger.warning( | |
| """ | |
| No view builder for component type '\(node.type, privacy: .public)' \ | |
| in catalog '\(catalogID, privacy: .public)'. | |
| """ | |
| ) | |
| } |
Description
The Swift
NodeResolverreported nothing when a component's type was missing from its catalog or when a child reference repeated one of its ancestors.MessageProcessordefaults to relaxed validation, which lets unknown types through, so the only trace was aprintinComponentNodeViewthat ran on every render. web_core (node-resolver.ts), Dart (node_resolver.dart) and Python report both to the surface, and the node resolution blueprint requires it. The error fallbacks in #2013 need this signal.Changes:
NodeResolverreportsUNKNOWN_COMPONENT_TYPEwhen the component's catalog has no entry for its type, with web_core's message:Component 'u1' has type 'Widget', which is not in catalog 'test'.The node is still built as before.NodeResolverreportsCYCLIC_REFERENCEat the cycle guard inresolveChildReference. The message matches web_core up to the last clause:Component 'a' at '/' is referenced by one of its own descendants; omitting the reference.Swift drops the cyclic child instead of rendering a placeholder, so "rendering a placeholder instead" would be wrong here.GenericErrors sent through the resolver's action handler, and they use the same deduplication asEXPRESSION_ERROR: the error is keyed to the node being resolved (component instance and data path), reported once, and reported again only after that node resolves without it. To share that path, the tail ofhandleFunctionErrormoved into a privatereport(_:dedupeKey:). Expression-error keys are unchanged.ComponentNodeViewno longer prints when it has no view for a node. A type missing from the surface's catalog is now reported by the resolver, and the framework adapter blueprint says the adapter renders nothing for these nodes and doesn't report them again.One difference from web_core: when the agent updates the component that holds the error, Swift reports it again if it is still broken. Swift already does this for
EXPRESSION_ERROR, so this PR keeps it consistent. web_core doesn't report again in that case.This doesn't add a fallback API or placeholder node states. That's #2013.
Testing:
NodeResolverTests:cyclicReferenceIsReportedOnce: oneCYCLIC_REFERENCE, still one after the cyclic node re-resolves for a data change and after the root re-renders.unknownComponentTypeIsReportedOncePerComponent: one error per unknown component, none added when an unknown node re-resolves or the root re-renders, and a new one after the component is fixed and broken again.unknownComponentTypeIsReportedOncePerDataPath: a template over a list reports once per item, and growing the list reports only the new item.SurfaceViewModelTests.updateComponentsAcceptsUnknownTypeByDefaultasserted that a relaxed processor reports no error for an unknown type. It now expects exactly oneUNKNOWN_COMPONENT_TYPE.swift test: 685 tests pass. With the cross-pass check inreportdisabled, the cycle test and the per-component test fail.swift-format lintis clean on the changed files.SurfaceRedrawTests.leafDataUpdateOnlyRedrawsTargetComponentsometimes fails on the first run after a build. It passed 10 out of 10 prebuilt runs on this branch and on main, and it only renders catalog types, so it never reaches the changed code.With #3583 merged, the adapter case
fallback_states_unknown_type_and_cyclic_diagnosticsstays an expected failure on Swift: it also needs pending and error placeholder nodes, which belong to #2013. I updated its reason string to name only the missing placeholder nodes.Related: #2013, #3734, #3583
Pre-launch Checklist
One time:
For this PR:
If you need help, consider asking for advice on the discussion board.