feat(ourlogs): fetch not-yet-available pinned logs and invalidate on filter changes#116614
feat(ourlogs): fetch not-yet-available pinned logs and invalidate on filter changes#116614JoshuaKGoldberg wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📊 Type Coverage Diff✅ no issues found |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JoshuaKGoldberg
force-pushed
the
feat/logs-781-fetch-not-yet-available-pinned-logs
branch
from
ebdd212 to
24d8526
Compare
JoshuaKGoldberg
force-pushed
the
feat/logs-781-fetch-not-yet-available-pinned-logs
branch
from
24d8526 to
00fd77e
Compare
…filter changes Adds data fetching for pinned log rows that are in the URL but not yet loaded in the infinite scroll table (e.g. the virtualizer hasn't reached them yet). Also removes pinned IDs from the URL when they can't be found within the current page filters (date range, project, environment). - Add removePinnedRow to LogsPinning interface (idempotent removal) - New usePinnedLogsQuery hook: fetches missing pinned log rows via the events API, then removes any IDs that aren't returned (invalidation) - PinnedLogs now accepts fetchedPinnedRows/isFetchingPinnedRows props, merges fetched rows with existing allRows, and shows a loading indicator while rows are being fetched - logsInfiniteTable calls usePinnedLogsQuery and passes results down Refs LOGS-781 Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
JoshuaKGoldberg
force-pushed
the
feat/logs-781-fetch-not-yet-available-pinned-logs
branch
from
00fd77e to
394b9bc
Compare
Replace removePinnedRow with removePinnedRows so invalidation removes all not-yet-available pinned ids in a single state update. Calling the single-id setter in a loop dropped removals: nuqs runs setState updaters at render, so each synchronous call saw the same committed state and only the last write survived. Include the utc flag in the pinned-log fetch for absolute date ranges so it matches the main logs query's time window and avoids spurious pin invalidation. Also fix pre-existing typecheck failures (LogFixture missing ORGANIZATION_ID, PinnedLogs wrapper row type) and a require-await lint error in the specs. Refs LOGS-781 Co-Authored-By: Claude <noreply@anthropic.com>
…-not-yet-available-pinned-logs
JoshuaKGoldberg
changed the title
|
@cursor review |
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 90715c1. Configure here.
narsaynorath
left a comment
narsaynorath
left a comment
There was a problem hiding this comment.
lgtm! One thing I noticed when testing it was that the floating button to move to the top of the table occupies the same space as the pinned logs. Not sure if this is already known or is fine to ignore, but just wanted to raise it 👍
| interface Props { | ||
| allRows: LogTableRowItem[]; | ||
| logsPinning: LogsPinning; | ||
| query: ReturnType<typeof usePinnedLogsQuery>; |
There was a problem hiding this comment.
nit: I think query here is too generic of a name for what this parameter holds. I thought it was a string that represented a query to get the pinned logs. I think a better name might just be pinnedLogs
Alternatively, I was also wondering if we should just move the hook and this content into this component. It doesn't seem to be used by parent except to pass it down here.
There was a problem hiding this comment.
Re the naming: yeah makes sense! I'll stew on this.
Re moving down: I tried that locally, but then it doesn't fire until the component renders - which adds a good chunk of delay.
Great spot! No, I hadn't noticed 🙈. Filed: LOGS-837 |
2 participants
Summary
Part 2 of 2 for log pinning implementation (part 1: #115102). Adds a new
usePinnedLogsQueryhook that sets up a query, which is then used in the existingPinnedLogscomponent.Closes LOGS-781