Conversation
…ead expansion (CLUE-428) - Add MobX reaction in NavTabPanel so scroll-to-tile works with chat panel closed - Remove scroll useEffect from ChatThread (was causing hidden coupling) - Fix auto-expand to not open document thread on cross-document tile selection - Add tests for override title fallback paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cused (CLUE-428) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
scytacki
left a comment
scytacki
left a comment
There was a problem hiding this comment.
This is better because it isn't dependent on the comment panel but it still has issues. Here is a summary generated by Claude for me.
The visible problem: The scroll sometimes works and sometimes doesn't. After investigating, there are several underlying issues:
Wrong location for the logic. The scroll-to-tile reaction was placed in NavTabPanel — a component responsible for rendering navigation tabs. It was put there because it's "always mounted," but it has no logical relationship to tile selection or scrolling. This makes the code hard to find, understand, and maintain. If NavTabPanel is ever refactored, the scroll behavior silently breaks.
Incorrect document matching. The current code uses focusDocument (the document open in the resource panel) as the scroll target for every tile selection, even when the selected tile isn't in that document. That can happen if there are different documents open on the right and left of CLUE. And the user is clicking on the right side document. In this case the code sends bogus scroll requests because the focusDocument doesn't contain the selected tile.
Overly complex indirection. The current flow is: tile gets selected → NavTabPanel reaction fires → writes to global scrollTo property on UI store → DocumentContent reaction fires → searches its rows for the tile → scrolls. This is a long chain with multiple points of failure, all to connect a tile selection to a DOM scroll.
Suggested fix: Move the scroll behavior into TileComponent — the component that wraps every tile and already watches for selection changes to render the highlight border. When a tile becomes selected, it can simply call scrollIntoView on its own DOM element. This is better because:
It's self-contained. No store indirection, no cross-component coordination, no guessing which document the tile belongs to.
It's reliable. The component knows exactly when it becomes selected and has direct access to its own DOM element.
It's in the right place. Selection highlighting and scroll-on-selection are closely related concerns — both are visual responses to a tile becoming selected.
It naturally scopes to the right document. Each TileComponent only scrolls itself within its own container, eliminating the focusDocument mismatch bug entirely.
Claude also added the following. I know you don't agree, but I'm just including it to show that I'm not the only that thinks scrolling the editable document can be a problem:
The scrolling should probably be limited to read-only/preview documents so it doesn't disruptively auto-scroll the workspace while a user is clicking around. The existing scrollTo mechanism on the UI store can remain for other use cases (e.g., clicking a comment thread header to jump to a tile).
collaborative-learning
|
||||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-428-comment-scrolling-redux
|
| Run status |
|
| Run duration | 03m 15s |
| Commit |
|
| Committer | lbondaryk |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2780 +/- ##
==========================================
- Coverage 66.83% 66.82% -0.02%
==========================================
Files 843 843
Lines 46357 46367 +10
Branches 12052 12054 +2
==========================================
- Hits 30985 30983 -2
- Misses 14271 14283 +12
Partials 1101 1101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2780 +/- ##
===========================================
- Coverage 86.16% 66.81% -19.36%
===========================================
Files 849 844 -5
Lines 46449 46430 -19
Branches 12072 12071 -1
===========================================
- Hits 40024 31020 -9004
- Misses 6018 14306 +8288
- Partials 407 1104 +697
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ns below comment text (CLUE-428) - Move scroll-to-tile from NavTabPanel reaction into TileComponent.componentDidUpdate so each tile scrolls itself into view when selected (no store indirection needed) - Remove NavTabPanel scroll reaction (setScrollTo mechanism remains for chat thread and toolbar callers) - Move renderRatingButtons below comment text in CommentItem Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Allow comment text clicks to propagate to card handler for tile selection - Delay thread scroll to wait for expanded content to render - Allow inner tiles in read-only documents to be selected individually - Make question prompt text select the question tile in read-only mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Inline hasSelectionModifier(e) to avoid shadowing the outer `append` variable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Moving logic for synchronizing selection of tiles and comments into the document rather than as a by product of the comment panel.