feat(ourlogs): implement pinned logs with sticky header#115102
feat(ourlogs): implement pinned logs with sticky header#115102JoshuaKGoldberg wants to merge 18 commits into
Conversation
github-actions
Bot
added
the
Scope: Frontend
JoshuaKGoldberg
force-pushed
the
ourlogs-pinned-logs-sticky
branch
from
4407fa4 to
b93a496
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.51% |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| interface RenderOptions extends rtl.RenderOptions, ProviderOptions { | ||
| export interface RenderOptions extends rtl.RenderOptions, ProviderOptions { |
There was a problem hiding this comment.
[Explanation] Cursor keeps trying to add a Parameters<typeof render>[1], so I just exported this.
| } else { | ||
| expect(actionsButton).toBeInTheDocument(); | ||
| } | ||
| expect(actionsButton).toBeInTheDocument(); |
k-fish
left a comment
k-fish
left a comment
There was a problem hiding this comment.
Perf is the only thing I think we should double check, maybe the comment re: LOGS-781, rest is nits.
| ? `${window.location.pathname}?${newSearch}${window.location.hash}` | ||
| : `${window.location.pathname}${window.location.hash}`; | ||
|
|
||
| window.history.replaceState(window.history.state, '', newUrl); |
There was a problem hiding this comment.
Blurgh. yeah. What is the recommended way to do this in our infra? I was getting lost in a lot of the shared utils.
There was a problem hiding this comment.
Ok I took another stab and ended up with ca0602a. I dislike this very much. It's convincing me that Solid.js-style fine-grained reactivity actually is much better (prior to now I didn't feel strongly one way or another). But if we have a better thing, that'd be nice 🙃.
Edit: also this causes #115102 (comment), so that's unfortunate.
There was a problem hiding this comment.
Taking a quick look at useLogsPinning.tsx looks like it's a bit recursive at this moment, if not actually recursive then doing too much imo. It's reading from location and setting pinnedRows as state, then the useEffect is calling navigate to update the location again...
try to use nuqs for all this, the location should be the source of truth and you probably don't need to stick default values into the url on pageload. it makes the urls messy and next pageload we'll just sort out the defaults again.
There was a problem hiding this comment.
#115427 removes some indirection that is absolutely never needed. i've left stuff like REPLAY_QUERY_KEY in there because i'm trying to automate things and want the bot doing one thing at a time, but it's ripe for removal next.
| data-row-pinned={isPinned} | ||
| data-row-hover-linked={isHoverLinked} |
There was a problem hiding this comment.
Are these just to avoid doing emotion's prop syntax?
There was a problem hiding this comment.
I was going off the existing data-row-highlighted code. I could have sworn there was guidance saying explicitly to do this, but I can't find it now.
Is there a different preferred thing?
There was a problem hiding this comment.
No was just wondering why you were passing down data attributes, we might have a rule like that (I can't recall) but if so it's likely because of perf, so feel free to leave it
There was a problem hiding this comment.
Talked 1:1: we can switch both data-*s to standard props in LogTableRowProps.
The original thought was that there's another pattern of putting data-* props on a parent component for purely stylistic things. That lets us avoid child component rerenders. But we aren't benefitting from that here.
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 ef75c37. Configure here.
| ...location, | ||
| query: { | ||
| ...location.query, | ||
| [LOGS_PINNED_KEY]: Array.from(pinnedRows), | ||
| }, | ||
| }, | ||
| {replace: true} | ||
| ); | ||
| // location is intentionally omitted — we only want to sync pinnedRows to the URL. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [navigate, pinnedRows]); |
There was a problem hiding this comment.
Bug: The useEffect in LogsPinningProvider uses a stale location object, causing user-applied filters to be reverted when a row is pinned.
Severity: HIGH
Suggested Fix
To prevent using a stale location object, you can either include location in the useEffect dependency array, use a useRef to store and access the latest location, or conditionally render the LogsPinningProvider only when its feature flag is enabled. The goal is to ensure the navigate call always uses the most current URL query parameters when updating the URL with pinned row information.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: static/app/views/explore/logs/pinning/useLogsPinning.tsx#L50-L63
Potential issue: The `LogsPinningProvider` component captures the `location` object on
its initial render but intentionally omits it from the dependency array of a `useEffect`
hook. This hook is responsible for synchronizing pinned rows to the URL. If a user
modifies URL query parameters (e.g., by applying a search filter) and then pins a log
row, the `useEffect` triggers. It then calls `navigate` using the stale `location`
object captured on mount, which overwrites the user's current filters and reverts them
to their initial state, leading to incorrect data being displayed.
Also affects:
static/app/views/explore/logs/tables/logsInfiniteTable.tsx:483~489
| // location is intentionally omitted — we only want to sync pinnedRows to the URL. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [navigate, pinnedRows]); |
There was a problem hiding this comment.
I don't really like this hook rule but I understand why it's in place, Tony and I were cheekily using useBox on values that weren't meant to be deps instead of having a eslint ignore, but I'm not sure I like that extra fiber usage either 😆
|
|
||
| const rowId = String(dataRow[OurLogKnownFieldKey.ID]); | ||
| const expansionKey = expansionKeyProp ?? rowId; | ||
| const logsPinning = useLogsPinning(); |
There was a problem hiding this comment.
Okay, so this won't go crazy on hovers, but this WILL make the pinning click still a bit hefty... only way to fix this off the top of my head is to do orchestration one level higher above the rows to not break the memo and maybe containment (pass pinned) versions only if it applies to that row? A bit unwieldly but if this lags food for thought in the future.
|
Talked 1:1: will do the |
3 participants
Part 1 of 2 for implementation: rendering logs that are already available in data.
I have to admit, I don't particularly like this implementation.
LogRowContentdetermines whether to render pin UI by whetheruseLogsPinning()returns a context value, which happens only if a provider is available. I would have preferred to DI / props drill from the parent page ->LogsInfiniteTable. But there's a lot going on and I couldn't quite make it work nicely 😞. As a followup to all this work & the logs table constrained heights (LOGS-664), I'd like even more to refactor a bunch.Note that this doesn't handle fetching pinned logs that aren't yet available in the virtualized list data or removing pinned logs from the query parameters on query changes. That'll be part 2 of 2: LOGS-781.
Fixes LOGS-776.
Takes inspiration and a healthy amount of code from #114922. Therefore:
Co-authored-by: @marthalyndon