DYN-9970 Fix Documentation Browser breadcrumb collision#17067
DYN-9970 Fix Documentation Browser breadcrumb collision#17067RobertGlobant20 merged 3 commits intomasterfrom
Conversation
Fix Documentation Browser breadcrumb collision for String node Updated breadcrumb resolution in DocumentationBrowserViewModel.cs to prefer exact suffix matches (for example .String) before falling back to broad substring matching. Prevents String node docs from incorrectly resolving to Input/DateTime via DateTime.FromString. Added regression test in DocumentationBrowserViewExtensionTests.cs to verify StringInput shows Input/Basic breadcrumbs and not DateTime. It only changes ambiguous cases where substring collisions existed (like String matching DateTime.FromString). In those cases, it now picks the more precise match.
RobertGlobant20
requested review from
QilongTang,
aparajit-pratap,
Copilot and
jasonstratton
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9970
|
GIF showing the expected behavior for the String node. |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect Documentation Browser breadcrumb resolution for nodes whose names collide as substrings of other node keys (e.g., "String" matching "DateTime.FromString"), by prioritizing exact key-suffix matches before falling back to substring matching.
Changes:
- Update
DocumentationBrowserViewModel.GetBreadCrumbsValueto prefer keys ending in".{OriginalName}"(e.g.,Input.String) before usingContains. - Add a regression test ensuring
CoreNodeModels.Input.StringInputresolves toInput / Basicinstead ofInput / DateTimein an ambiguous dictionary.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/DocumentationBrowserViewExtension/DocumentationBrowserViewModel.cs | Adjusts breadcrumb lookup order to avoid substring collisions by preferring exact suffix matches. |
| test/DynamoCoreWpf2Tests/ViewExtensions/DocumentationBrowserViewExtensionTests.cs | Adds regression coverage for the String node breadcrumb collision scenario. |
| var method = typeof(DocumentationBrowserViewModel).GetMethod( | ||
| "GetBreadCrumbsValue", | ||
| BindingFlags.NonPublic | BindingFlags.Instance); | ||
|
|
||
| // Act | ||
| var breadCrumbs = method.Invoke(docsViewModel, new object[] { nodeAnnotationEventArgs }) as string; | ||
|
|
There was a problem hiding this comment.
The test uses reflection to invoke the private GetBreadCrumbsValue method, but it doesn’t assert that GetMethod actually found it. If the method is renamed/overloaded or its accessibility changes, this will fail with a NullReferenceException at Invoke and be harder to diagnose. Add an explicit Assert.IsNotNull(method, ...) (or Assert.Fail) before invoking.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Address PR review feedback - Added Assert.IsNotNull guard on reflected GetBreadCrumbsValue method before Invoke to produce a clear diagnostic if the method is renamed - Replaced suffix-match foreach loop with LINQ FirstOrDefault and idiomatic KeyValuePair.Key != null check
|
Hi Team, any comments in this PR? |
Purpose
Fix Documentation Browser breadcrumb collision for String node
Updated breadcrumb resolution in DocumentationBrowserViewModel.cs to prefer exact suffix matches (for example .String) before falling back to broad substring matching. Prevents String node docs from incorrectly resolving to Input/DateTime via DateTime.FromString. Added regression test in DocumentationBrowserViewExtensionTests.cs to verify StringInput shows Input/Basic breadcrumbs and not DateTime.
It only changes ambiguous cases where substring collisions existed (like String matching DateTime.FromString). In those cases, it now picks the more precise match.
Declarations
Check these if you believe they are true
Release Notes
Fix Documentation Browser breadcrumb collision for String node
Reviewers
@aparajit-pratap @QilongTang @jasonstratton
FYIs