Ux/thread view improvements#634
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds composable autofocus to a combobox, alters message-form autofocus for forward mode, and implements visibility control for the thread compose input driven by scroll proximity, fast-scroll detection, and shared focus state via an extended ThreadView context; includes CSS transitions and minor backend/logging and DB-delete-query adjustments. Changes
Sequence DiagramsequenceDiagram
participant User
participant ScrollContainer as ThreadView\n(Scroll container)
participant Context as ThreadViewContext
participant ReplyForm as ThreadMessage\n(Reply Form)
participant ComposeInput as ThreadEventInput\n(Compose Input)
User->>ScrollContainer: scroll
activate ScrollContainer
ScrollContainer->>ScrollContainer: RAF-throttled handler -> compute isNearBottom / delta
ScrollContainer->>Context: update visibility inputs (isNearBottom / fastScroll)
Context->>ComposeInput: visibility inputs influence isVisible
deactivate ScrollContainer
User->>ReplyForm: focus reply form
activate ReplyForm
ReplyForm->>Context: setIsMessageFormFocused(true)
Context->>ComposeInput: context change affects isVisible
ComposeInput->>User: show (remove --hidden)
deactivate ReplyForm
User->>ComposeInput: focus / blur
activate ComposeInput
ComposeInput->>ComposeInput: onFocusChange(true/false)
ComposeInput->>Context: may update focus state
deactivate ComposeInput
User->>ScrollContainer: scroll away
activate ScrollContainer
ScrollContainer->>ScrollContainer: recompute -> hide input (apply --hidden)
deactivate ScrollContainer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss`:
- Around line 11-14: The hidden modifier .thread-event-input--hidden currently
prevents mouse interaction but leaves controls tabbable; update the hidden state
so the subtree is removed from keyboard focus by either: add the inert attribute
to the container when hidden (preferred) or ensure you set aria-hidden="true" on
the container and apply tabindex="-1" to all focusable descendants (textarea,
buttons, inputs) so they are removed from the tab order; apply this change where
the class .thread-event-input--hidden is toggled and ensure the submit button
and textarea are included.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 239-260: The scroll listener effect for useEffect should
initialize isNearBottom from the current scroll position so the IM bar reflects
the real state immediately; inside the effect that references rootRef, rafRef,
setIsNearBottom, NEAR_BOTTOM_THRESHOLD and thread.id, compute the current
distanceFromBottom (el.scrollHeight - el.scrollTop - el.clientHeight) and call
setIsNearBottom(distanceFromBottom <= NEAR_BOTTOM_THRESHOLD) once before adding
the 'scroll' event (or simply invoke handleScroll() immediately) so the state is
correct even if scrolling happened earlier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 692bdff4-f9cf-459b-95b6-d785b74fbba5
📒 Files selected for processing (7)
src/frontend/src/features/forms/components/combobox/index.tsxsrc/frontend/src/features/forms/components/message-form/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/layouts/components/thread-view/provider.tsx
jbpenrath
force-pushed
the
ux/thread-view-improvements
branch
from
0829de6 to
23b3c4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/use-im-input-visibility.ts`:
- Line 47: The hook currently initializes isNearBottom to true and only updates
on scroll, so the input can be incorrectly visible on first render; change it to
compute the initial visibility from the current scroll position by either
initializing useState with a function that reads the container/sentinel
positions or by invoking the same visibility-checking routine (e.g., the
existing handleScroll/checkIsNearBottom function used by setIsNearBottom) inside
a useLayoutEffect/useEffect that runs on mount and when the active thread
changes; ensure this effect runs after cleanup/reset so the state is measured
again (reference isNearBottom, setIsNearBottom, and the scroll visibility
checker/handler in use-im-input-visibility.ts).
In `@src/frontend/src/features/layouts/components/thread-view/provider.tsx`:
- Line 27: The focus flag is not cleared when the active thread changes; add a
useEffect in the ThreadView provider that watches the active thread identifier
(e.g., activeThreadId or thread?.id) and calls setIsMessageFormFocused(false)
whenever it changes so a newly mounted thread doesn't inherit the previous focus
state; also apply the same reset logic to the other thread-scoped flags
referenced around lines 68-70 (use their setters to clear on thread change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea3a0d8b-fa85-4d3a-9f1b-e70541e691c6
📒 Files selected for processing (6)
src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/use-im-input-visibility.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/layouts/components/thread-view/provider.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/frontend/src/features/layouts/components/thread-view/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
jbpenrath
force-pushed
the
ux/thread-view-improvements
branch
2 times, most recently
from
6bcb526 to
4aff492
Compare
When user forwards a message, the message form should focus the `to` field not the message composer.
Currently the ThreadEventInput is always showed and sticked to the bottom of the screen. But it can be annoying when the user is writing a message for example as the input is displayed above the message reply form. Then it is not relevant to show the input when the user is at the top of the thread view. So we only show the input when the user reach the end of the view.
jbpenrath
force-pushed
the
ux/thread-view-improvements
branch
from
4aff492 to
b857122
Compare
The select_for_update() was ineffective for two reasons: - .exclude(id=instance.id) caused each concurrent thread to lock a disjoint set of rows, so no serialization occurred - .count()/.exists() generate aggregate SQL (SELECT COUNT(*)) that silently drops the FOR UPDATE clause in Django ORM Now locks ALL editor rows (including self) via values_list() evaluation, forcing concurrent deletes to serialize properly.
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.
Purpose
Two ux feedbacks improvements :
When user forwards a message, the message form should focus the
tofield not the message composer.
Currently the ThreadEventInput is always showed and sticked to the
bottom of the screen. But it can be annoying when the user is writing a
message for example as the input is displayed above the message reply
form. Then it is not relevant to show the input when the user is at the
top of the thread view. So we only show the input when the user reach
the end of the view.
CleanShot.2026-04-16.at.12.35.10.mp4
Summary by CodeRabbit
New Features
UI/UX Improvements
Chores