Skip to content

fix(frontend): entry deduplication#5061

Merged
jog1t merged 1 commit into
mainfrom
05-15-fix_frontend_entry_deduplication
May 15, 2026
Merged

fix(frontend): entry deduplication#5061
jog1t merged 1 commit into
mainfrom
05-15-fix_frontend_entry_deduplication

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented May 15, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jog1t jog1t marked this pull request as ready for review May 15, 2026 19:31
Copy link
Copy Markdown
Contributor Author

jog1t commented May 15, 2026

Merge activity

  • May 15, 7:36 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 15, 8:10 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 15, 8:10 PM UTC: @jog1t merged this pull request with Graphite.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 15, 2026

PR Review: fix(frontend): entry deduplication

Overview

This PR fixes duplicate log entries in useDeploymentLogsStream by introducing a seenInsertIdsRef set that tracks seen insertId values across three boundaries: the initial history fetch, the live SSE stream, and load-more pagination. It also fixes a missing setHasMore(false) call for the initial history fetch and adds Sentry observability for a previously silent catch block.

The approach is correct and the problem is real. Timestamp-cursor pagination inherently produces overlaps at page boundaries, and history/stream overlap is an inherent race.


Issues

Unguarded insertId in the live-stream path (medium)

If entry.data.insertId is undefined or null, every subsequent entry without an insertId would be silently dropped - Set.has(undefined) returns true after the first Set.add(undefined). The history path already sidesteps this because it iterates and bulk-adds before any dedup check, but the stream path has a hard gate. A minimal guard keeps it safe.

Load-more can silently no-op when a full page is entirely duplicates (low)

hasMore is set based on the raw items.length (correct, because the API may have more pages), but if fresh.length === 0 the user clicks load more, sees nothing, and the button stays enabled indefinitely. An auto-retry or user-facing message in this case would improve UX, though this edge case is rare in practice.

Sentry context includes filter (low)

filter is a free-text search term entered by the user and could contain sensitive strings. Confirm that Sentry data-scrubbing rules cover this field, or omit filter from the context.


Positive observations

  • Using insertId as the dedup key is the right choice. It is the only stable, per-entry unique identifier the API exposes, and the comment makes the reasoning clear.
  • Resetting seenInsertIdsRef.current = new Set() in the main useEffect on any dep change is correct; the set never leaks across stream sessions.
  • The setHasMore(false) fix for the initial history fetch is a good catch. Without it the load-more button stays visible even when there is no older history.
  • Promoting the previously silent catch block to console.warn + Sentry is solid observability hygiene.
  • The history-before-stream ordering means seenInsertIdsRef is always fully populated before the first SSE entry arrives, so there is no initialization race.

Minor nits

The hasMore reset useEffect is necessary because the main useEffect only calls setHasMore(false) conditionally. Without it hasMore could be stale-false after a filter change. The logic is correct but the relationship between the two effects is not immediately obvious. A brief comment noting why this reset lives in a separate effect would help future readers.


Overall this is a clear, well-scoped fix. The insertId guard in the live-stream path is the only item worth calling a blocking concern before merge.

@jog1t jog1t changed the base branch from 05-15-_slop_feat_frontend_enable_compute_add_historical_logs to graphite-base/5061 May 15, 2026 20:06
@jog1t jog1t changed the base branch from graphite-base/5061 to main May 15, 2026 20:08
@jog1t jog1t force-pushed the 05-15-fix_frontend_entry_deduplication branch from 0401c60 to 17b97a5 Compare May 15, 2026 20:09
@jog1t jog1t merged commit b8df22c into main May 15, 2026
8 of 11 checks passed
@jog1t jog1t deleted the 05-15-fix_frontend_entry_deduplication branch May 15, 2026 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant