fix(issue-details): Keep sticky event nav below page frame#113401
fix(issue-details): Keep sticky event nav below page frame#113401priscilawebdev merged 5 commits into
Conversation
Use a local issue-details offset helper so the sticky event nav sits below the page frame top bar and keeps section scroll margins aligned. This issue only affects issue details today, so keep the offset logic local instead of introducing a broader navigation abstraction before another page needs it. Refs DE-1160 Co-Authored-By: Codex GPT-5 <noreply@openai.com>
github-actions
Bot
added
the
Scope: Frontend
Drop the focused hook spec added during the sticky nav fix follow-up. Keep the production change only and leave the PR scoped to the issue-details offset behavior update itself. Refs DE-1160 Co-Authored-By: Codex GPT-5 <noreply@openai.com>
TkDodo
left a comment
TkDodo
left a comment
There was a problem hiding this comment.
Just an FYI, @natemoo-re has a PR that does something similar to useTopOffset:
e024de8 to
903ee46
Compare
903ee46 to
6f6239f
Compare
Thank you! I could reuse his changes 🙌 |
ae3f940 to
b9c9e37
Compare
b9c9e37 to
e00e96a
Compare
e00e96a to
2fd8380
Compare
5b7f9cc to
18a3487
Compare
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
Looks good! Could be nice to refactor out some of the logic and use the existing Sticky component, but not a blocker. I'm going to merge #113298 so you should be able to rebase against master.
| }); | ||
| }, [nav, isScreenMedium, dispatch, sidebarHeight]); | ||
| }, [nav, dispatch, stickyTopOffset]); | ||
|
|
||
| return ( | ||
| <FloatingEventNavigation |
There was a problem hiding this comment.
Any chance we could refactor this to wrap it in static/app/components/sticky.tsx? That already encapsulates the pinning logic.
There was a problem hiding this comment.
I can address that in a follow-up PR 😉
Reuse the shared top-offset logic introduced by the page frame navigation work so issue details consumes the same source of truth for sticky positioning. This removes the local sticky offset helper, makes contentTop carry the legacy mobile fallback when page frame is disabled, and lets issue details consume the shared value directly for both sticky position and scroll margin math.
…page-frame-issue-details-sticky-nav # Conflicts: # static/app/views/navigation/useTopOffset.tsx
2fd8380 to
64a8923
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
62c6043 to
64a8923
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit def3b4f. Configure here.
| } | ||
|
|
||
| function StickyEventNav({event, group}: {event: Event; group: Group}) { | ||
| const theme = useTheme(); |
There was a problem hiding this comment.
Stuck detection broken with non-zero sticky top offset
Medium Severity
useIsStuck(nav) is called without an offset option, but the element now sticks at a non-zero top value (stickyTopOffset, e.g. 53px on desktop with page frame). The useIsStuck hook uses an IntersectionObserver with rootMargin: '-1px 0px 0px 0px' (offset defaults to 0), which only detects sticking when the element's top edge crosses 1px from the viewport top. Since the element now sticks at 53px, it never crosses that boundary, so isStuck is always false and data-stuck is never "true". The &[data-stuck='true'] border-radius removal never triggers. The sticky.tsx component correctly passes the offset to useIsStuck — this call needs the same treatment.
Reviewed by Cursor Bugbot for commit def3b4f. Configure here.
Follow-up to [this review comment](#113401 (comment)) on #113401: wrap the sticky event nav in the shared `Sticky` component from `sentry/components/sticky` instead of hand-rolling the pinning logic with `position: sticky` + `useIsStuck`. To make `Sticky` a true self-contained primitive, this also makes `--top-bar-height` unconditional in `TopBar`: the CSS variable is now set from `useTopOffset`'s `contentTop` regardless of the page frame feature, so `Sticky`'s `top: var(--top-bar-height, 0px)` works as a single source of truth across desktop/mobile and page-frame-on/off (including the legacy mobile top bar fallback). Consumers no longer need to pass an inline `top` style, and the inline style here is dropped. `useTopOffset` is still consumed in `StickyEventNav` for the scroll-margin math. Refs DE-1160
3 participants
Keep the issue details sticky event nav below the new page frame top bar.
This PR is stacked on
nm/nav/topbar-offsetand reuses the shareduseTopOffset()logic from that branch instead of duplicating the page-frame offset math in issue details.For issue details specifically, this updates the sticky event nav to:
contentTopfor the page-frame top offsetBefore
Screen.Recording.2026-04-20.at.11.27.43.mov
After
Screen.Recording.2026-04-20.at.11.28.44.mov
Closes DE-1160