fix: restore events tail streaming

[dylantientcheu]

Summary

• wire the requested Insights region into the CLI Insights client so algolia events tail -r us|de targets the correct host
• switch tail polling to overlapping windows with request-ID deduplication so live events are not skipped between polls
• add focused regression tests for region propagation and event deduplication/window handling

Test plan

• go test ./api/insights ./pkg/cmd/events/tail
• go test ./...
• start go run ./cmd/algolia events tail -p askai -r us -o json
• push a synthetic Insights view event to the same app and confirm the event appears in the tail stream

Context

• fixes GROUT-233

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric	Results
Complexity	0
Duplication	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[NatanTechofNY]

🤖 AI-Generated Code Review by Cursor

---

PR #209 Code Review — fix: restore events tail streaming

Overview

Small, focused fix (4 files, +130/-6) that wires the requested Insights region into the CLI's Insights client and rewrites events tail polling to use overlapping windows with request-ID deduplication.

Overall: Solid bug fix with good test coverage. Two correctness concerns in the new polling loop (dedup TTL key and error handling) are worth addressing before merge; the rest are polish.

---

⚙️ MEDIUM CONCERNS

1. Dedup TTL keyed on client-supplied event timestamp (MEDIUM)

• Issue: In pkg/cmd/events/tail/tail.go, seenRequestIDs[requestID] = event.Event.Timestamp.Time stores the event's own payload timestamp, then pruneSeenRequestIDs(seenRequestIDs, windowStart.Add(-Interval)) evicts entries older than the cutoff.
• Risk: Insights event timestamps come from the client SDK/browser. A skewed clock or replayed event can insert a requestID whose timestamp is already past the cutoff, so it's pruned on the very next tick. If the backend then redelivers the same event inside the overlap window, it's printed twice — defeating the dedup this PR adds.
• Required Actions:
  • Store time.Now().UTC() (or windowEnd) in the map instead of event.Event.Timestamp.Time, so TTL is measured against wall-clock arrival, not the event payload.

2. Transient API errors now terminate the tail stream (MEDIUM)

• Issue: The new return err inside the non-region error branch is a correctness win vs. the prior silent-swallow behavior, but it means any single 5xx, timeout, or network blip kills the whole algolia events tail process.
• Risk: Users running long-lived tails (the common case) will have sessions die on any transient hiccup and have to restart.
• Required Actions:
  • Consider a bounded retry/backoff for non-region errors, or at minimum print the error to opts.IO.ErrOut and continue the loop. Keep the hard return only for the region-mismatch branch where retrying cannot help.

---

💡 LOW CONCERNS

3. Brittle reflection-based host assertion (LOW)

• Issue: api/insights/client_test.go does reflect.ValueOf(cfg.Hosts[0]).FieldByName("host").String() against an unexported SDK field.
• Risk: If the upstream SDK renames/retypes host (e.g. pointer, nested struct), this asserts "" at runtime with no compile-time signal. The region-propagation intent is already fully covered by require.Equal(t, algoliaInsights.DE, cfg.Region) + require.NotEmpty(t, cfg.Hosts).
• Required Actions:
  • Drop the reflection block, or swap it for a public accessor if one exists.

4. No error wrapping on return err (LOW)

• Issue: The raw SDK error bubbles up from runTailCmd with no indication it came from the polling loop.
• Required Actions:
  • Wrap with context, e.g. return fmt.Errorf("fetching events: %w", err).

5. Window overlap strategy deserves a comment (LOW)

• Issue: The first window is Interval-wide (windowStart := time.Now().UTC().Add(-Interval)), while subsequent windows are ~2*Interval-wide because of windowStart = windowEnd.Add(-Interval) at the bottom of the loop. The overlap is intentional (dedup backstop for boundary events) but it took careful reading to verify.
• Required Actions:
  • Add a one-line comment above the loop explaining the overlap strategy.

---

✅ STRENGTHS

• Real bug fix: Region was accepted by NewClient but never propagated into clientConfig, so -r us|de was a no-op — the one-line fix in api/insights/client.go restores intended behavior.
• Leak fix: Replacing time.Tick(Interval) with time.NewTicker(Interval) + defer ticker.Stop() closes the goroutine/timer leak time.Tick has when the surrounding function can return.
• Error path corrected: The previous loop silently continued iterating on non-region errors with stale state; the new return err is the right behavior (modulo concern chore(workflows): Add golang-ci github workflow #2).
• Tests: Focused regression coverage for region propagation, duplicate-request-ID skipping, events without request IDs, and prune TTL math.
• Dedup strategy: Overlapping windows + request-ID dedup is the right design for low-latency tailing.

---

📋 BEFORE MERGE

Must Have:

1. Switch the dedup map value to wall-clock time so clients with skewed timestamps cannot evict themselves (concern fix(auth) Fix the application command #1).
2. Decide on retry vs. fail-fast for transient errors and align behavior with user expectations for a tail command (concern chore(workflows): Add golang-ci github workflow #2).

Should Have:
3. Drop or harden the reflection-based host assertion (concern #3).
4. Wrap returned errors with context (concern #4).
5. Add a short comment explaining the window-overlap strategy (concern #5).

---

🎯 RECOMMENDATION

Conditional Approval — core fix is correct and valuable; address the two MEDIUM concerns before merge, then the LOW items as polish.

Scores:

• Code Quality: 8/10
• Security: 10/10
• Performance: 8/10
• Correctness: 7/10

---

Generated by Cursor — automated review focusing on correctness, error handling, and test robustness.