Conversation
smw-ms
requested review from
AlitzelMendez,
helen229,
maririos,
praveenkuttappan and
tjprescott
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the APIView ClientSPA review experience to show “current of total” counts for unresolved comment-thread navigation, addressing the usability gap described in #14177.
Changes:
- Adds comment navigation state propagation from
CodePanelComponent→ReviewPageComponent→ReviewToolbarComponentto display “X of Y” in the toolbar. - Adds per-thread “X of Y” display next to the in-thread comment navigation controls.
- Adjusts styling to support always-visible in-thread navigation controls and new pointer UI.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dotnet/APIView/ClientSPA/src/app/_components/shared/comment-thread/comment-thread.component.ts | Adds inputs to receive thread position and total active-thread count. |
| src/dotnet/APIView/ClientSPA/src/app/_components/shared/comment-thread/comment-thread.component.html | Renders the “X of Y” pointer between prev/next buttons for thread navigation UI. |
| src/dotnet/APIView/ClientSPA/src/app/_components/shared/comment-thread/comment-thread.component.scss | Adds styling for the in-thread navigation pointer. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-toolbar/review-toolbar.component.ts | Adds inputs for current/total comment navigation state. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-toolbar/review-toolbar.component.html | Displays “X of Y” in the toolbar comment nav section. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-toolbar/review-toolbar.component.scss | Styles the toolbar comment nav info text. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.ts | Stores comment navigation state and updates it via a new handler. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.html | Wires the new navigation-state output from code panel into the review page and passes state to the toolbar. |
| src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.ts | Computes active unresolved-thread positions/count and emits navigation state. |
| src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.html | Passes computed thread position/count into each comment-thread instance. |
| src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.scss | Makes in-thread comment navigation controls always visible for comment-thread rows. |
| private getActiveCommentThreadPositionData(): { count: number; byRowIndex: Map<number, number> } { | ||
| const byRowIndex = new Map<number, number>(); | ||
| let position = 0; | ||
|
|
||
| for (let i = 0; i < this.codePanelRowData.length; i++) { |
There was a problem hiding this comment.
getActiveCommentThreadPositionData() rebuilds the active-thread index map by scanning the full codePanelRowData array each time it's called. Because getActiveCommentThreadPosition()/getActiveCommentThreadCount() are invoked from the template, this can turn into O(n²) work during change detection on large reviews. Consider caching the computed count/map (and re-computing only when comment-thread resolved state / thread rows change), and avoid indexOf(item) by using the row index already available from *uiScroll.
There was a problem hiding this comment.
Adding caching complicates the code a bit.
Realistic scenario for noticeable lag:
Review with ~3,000-5,000 total code lines + comments
User rapidly navigating between 20+ unresolved comment threads
There was a problem hiding this comment.
can you test the performance on a big review with a ton of comments like:
- https://spa.apiviewstagingtest.com/review/dcb5d1bec6c34bdf9094b8500b51e3e1?activeApiRevisionId=e4d73631345d4c6e883c3e225e52fa03
- https://spa.apiviewstagingtest.com/review/e8d301b10edf4b29b346a87fa4905ea6?activeApiRevisionId=2134bc73a6314381b355f32d9cc2aa4d
?
I know it is not the most common scenario but big reviews, especially with a lot of comments, are always problematic and people is always reaching out to us about them, so just to double check
src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.ts
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.ts
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/ClientSPA/src/app/_components/code-panel/code-panel.component.ts
Show resolved
Hide resolved
Two things:
So instead of "Comment on line 344" it would say "Comment on line 344 (4 of 22)" |
… apiview/show-comment-counts
Resolves: #14177
The comments are numbered relative to the current set of active (unresolved) threads in navigation order, so if you resolve a comment the numbers change accordingly. That’s why I didn’t add a comment number to the comment header or in the Comments tab. Thoughts?