Skip to content

Conversation

urmauur
Copy link
Member

@urmauur urmauur commented Jun 2, 2025

Describe Your Changes

This pull request enhances the scroll handling logic in the ThreadDetail component to improve user experience by adding functionality to detect the presence of a scrollbar and updating related behaviors accordingly.

Scroll Handling Enhancements:

  • Introduced a new state variable hasScrollbar to track whether the scroll container has a scrollbar. (web-app/src/routes/threads/$threadId.tsx, web-app/src/routes/threads/$threadId.tsxR30)
  • Added a checkScrollState function to determine the scroll position and the presence of a scrollbar, updating the isAtBottom and hasScrollbar states dynamically. (web-app/src/routes/threads/$threadId.tsx, web-app/src/routes/threads/$threadId.tsxR49-R61)
  • Integrated checkScrollState into existing logic:
    • Called during initial render and when the thread ID changes. (web-app/src/routes/threads/$threadId.tsx, [1] [2]
    • Periodically invoked via an interval when streamingContent is active. (web-app/src/routes/threads/$threadId.tsx, web-app/src/routes/threads/$threadId.tsxR128-R134)
    • Used in the scroll event handler to update hasScrollbar alongside isAtBottom and isUserScrolling. (web-app/src/routes/threads/$threadId.tsx, [1] [2]

UI Behavior Update:

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Enhance ThreadDetail scroll handling by tracking scrollbar presence and updating UI visibility logic.

  • Scroll Handling Enhancements:
    • Add hasScrollbar state to track scrollbar presence in ThreadDetail.
    • Implement checkScrollState to update isAtBottom and hasScrollbar states.
    • Integrate checkScrollState in initial render, thread ID change, and during streamingContent.
    • Update scroll event handler to set hasScrollbar.
  • UI Behavior Update:
    • Modify UI element visibility logic to consider isAtBottom and hasScrollbar.

This description was created by Ellipsis for 3955703. You can customize this summary. It will automatically update as commits are pushed.

@urmauur urmauur added this to the v0.5.18 milestone Jun 2, 2025
@urmauur urmauur self-assigned this Jun 2, 2025
@urmauur urmauur requested a review from louis-menlo June 2, 2025 14:04
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 3955703 in 1 minute and 38 seconds. Click for details.
  • Reviewed 101 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/threads/$threadId.tsx:49
  • Draft comment:
    Consider wrapping checkScrollState in useCallback to ensure stability when used in setInterval and effects.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While technically correct that useCallback could be used here, the function only uses stable references (setState functions and refs). React guarantees setState functions are stable. The setInterval usage is within a useEffect that only depends on streamingContent, so the interval is already properly cleaned up and recreated when needed. The other useEffect calls are one-time setup effects. The comment points out a legitimate React optimization pattern. Not using useCallback could theoretically cause unnecessary interval cleanup/recreation. The potential benefits are minimal since the function uses stable references and the interval cleanup/recreation is already tied to streamingContent changes. The added complexity of useCallback isn't justified here. The comment suggests a premature optimization that adds complexity without meaningful benefits in this specific case.
2. web-app/src/routes/threads/$threadId.tsx:178
  • Draft comment:
    Specify a debounce wait time (e.g., 50ms) in debounce(handleDOMScroll) to prevent excessive calls.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/routes/threads/$threadId.tsx:144
  • Draft comment:
    handleScroll and handleDOMScroll contain nearly identical logic; consider consolidating them to reduce code duplication.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. web-app/src/routes/threads/$threadId.tsx:55
  • Draft comment:
    Consider extracting the magic number (10) used for bottom detection into a named constant for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_BGnpiOv3H3YjEJnR

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@urmauur urmauur merged commit b0d3d48 into release/v0.5.18 Jun 2, 2025
20 checks passed
@urmauur urmauur deleted the fix/sticky-action-scroll-to-bottom branch June 2, 2025 15:00
@github-project-automation github-project-automation bot moved this to QA in Jan Jun 2, 2025
@github-actions github-actions bot modified the milestones: v0.5.18, v0.5.19 Jun 2, 2025
@david-menloai david-menloai moved this from QA to Done in Jan Jun 20, 2025
@LazyYuuki LazyYuuki removed this from the v0.7.2 milestone Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants