fix: keep component formatting failures from hiding state tree errors - #25459
Open
totally-not-ai[bot] wants to merge 3 commits into
Open
fix: keep component formatting failures from hiding state tree errors#25459totally-not-ai[bot] wants to merge 3 commits into
totally-not-ai[bot] wants to merge 3 commits into
Conversation
formatOwnerComponentToString calls application code, e.g. the component's toString() and, via the ComponentTracker weak maps, its hashCode(). When that code throws, its exception replaced the IllegalStateException about moving a node between state trees, hiding the actual problem. Catch runtime exceptions and describe the failure instead, logging the exception at debug level.
describe() reaches into application code too: getRouteComponent walks getParent(), and the ComponentTracker lookup uses the component's hashCode(). Since the description is meant for log messages, catch runtime exceptions and append a note about the failure to the details gathered so far instead of propagating it.
Legioth
requested changes
Sep 3, 2026
Matching the full message made the tests break on any wording change, while what matters is that the description names the component class and the exception that was thrown.
Legioth
approved these changes
Sep 3, 2026
|
Contributor
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
1 participant
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.
StateNodebuilds human-readable descriptions of a node and its owner component for error and log messages. Both code paths call back into application code, which can throw and mask the message that was actually being reported.The problem
formatOwnerComponentToString()is used when constructing theIllegalStateExceptionabout moving a node from one state tree to another. It calls the component'stoString()and — through theComponentTrackerweak maps — itshashCode(). If either throws, that exception propagated out instead of theIllegalStateException, so the user saw the misbehavingtoString()rather than the real problem (a component moved between UIs).describe()has the same exposure:ComponentUtil.getRouteComponent()walksgetParent(), and theComponentTrackerlookup goes through the component'shashCode(). Its javadoc already promised a description "for a log message", so throwing from it turns a log message into an error.The fix
Both methods now catch
RuntimeExceptionand describe the failure instead of propagating it:formatOwnerComponentToString()returnsunavailable, describing the component of type <class> threw <exception class>, so the surroundingIllegalStateExceptionstill reaches the user with the component type and the reason the description is missing.describe()appends, describing it further threw <exception class>to the details it has already gathered (node id, tag, component class, …) rather than losing them.In both cases the swallowed exception is logged at debug level so it is still recoverable when needed. The
describe()javadoc now states explicitly that the method never throws.Tests
ComponentTest.cannotMoveComponentsToOtherUI_componentToStringThrows_originalErrorIsReported— adds a component whosetoString()throws and asserts the resultingIllegalStateExceptionnames both the component class and the thrown exception type.StateNodeTest.describe_applicationCodeThrows_failureDescribedWithDetailsSoFar— a component whosegetParent()throws;describe()still returns a description naming the component class and the exception.The existing assertions were also relaxed to check for the essential parts (component class, exception type) instead of matching the full message, so they no longer break on wording changes.