Skip to content

feat: add stars_count pipe for snapshot-based new-star counts (CM-1785) - #4766

Merged
themarolt merged 10 commits into
mainfrom
feat/CM-1785-stars-count-pipe
Sep 25, 2026
Merged

themarolt merged 10 commits into
mainfrom
feat/CM-1785-stars-count-pipe

Conversation

@themarolt

Copy link
Copy Markdown
Contributor

Summary

  • stars_count.pipe provides "new stars" (period-specific, non-cumulative) analytics for a project or collection, replacing activities_count.pipe for stars only. Response field names match activities_count.pipe so stars.ts can swap the pipe name with no other changes.
  • Granularity mode diffs consecutive buckets of stars_cumulative_count.pipe's running total. No-granularity mode sums GitHub snapshot delta (end minus start) plus a plain GitLab event-count range query.
  • Cross-checked against the review findings fixed on the sibling stars_cumulative_count.pipe (PR feat: add stars_cumulative_count pipe for snapshot-based star totals (CM-1785) #4764) and fixed what applied here:
    • Added missing TOKEN "insights-app-token" READ grant (tb push doesn't auto-grant tokens to new pipes).
    • Guarded stars_count_repo_scope's collectionSlug branch on bucketId (falls back to 1 = 0 until bucketId is resolved), matching this file's own gitlab_new CTE and the sibling pipe's stars_repo_scope.
    • Restricted stars_count_repo_scope to GitHub repos (r.url LIKE 'https://github.com/%'), matching its own docstring.
    • Fixed gitlab_new's endDate cutoff from exclusive to inclusive, so GitLab star events timestamped exactly at endDate aren't dropped.

Test plan

  • tb push to staging, spot-check granularity and no-granularity responses against activities_count.pipe for a known project/collection

… fix GitLab endDate cutoff (CM-1785)

Cross-checked stars_count.pipe against the review findings fixed on the
sibling stars_cumulative_count.pipe (PR #4764):

- Add missing TOKEN "insights-app-token" READ grant - tb push doesn't
  auto-grant tokens to new pipes, same lesson as fac3021.
- stars_count_repo_scope's collectionSlug branch was missing the
  bucketId guard (falls back to 1 = 0 when bucketId isn't resolved yet)
  that stars_repo_scope already has, and that this same file's own
  gitlab_new CTE already has - an internal inconsistency, not just a
  gap versus the sibling pipe.
- stars_count_repo_scope was missing the GitHub-only filter
  (r.url LIKE 'https://github.com/%') its own docstring already
  claimed existed.
- gitlab_new's endDate cutoff was exclusive (<), silently dropping
  GitLab star events timestamped exactly at endDate - same boundary
  bug Copilot flagged and got fixed on the sibling pipe (0897a78).

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI balanced review requested due to automatic review settings September 25, 2026 08:29
@cursor

cursor Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
New analytics query logic affects star metrics correctness when wired in; snapshot backfill and net-unstar semantics can change reported counts versus event-based activities_count.

Overview
Adds stars_count.pipe, a Tinybird endpoint for period-specific “new stars” (non-cumulative) for projects and collections, intended to replace activities_count for star metrics only.

With granularity, bucket counts are derived by differencing consecutive rows from stars_cumulative_count (no separate GitHub/GitLab logic in bucket mode). Without granularity, GitHub uses snapshot totals at endDate minus startDate from repositoryStarSnapshots, plus a GitLab star event count over (startDate, endDate]. Response shapes (activityCount, optional startDate/endDate) match activities_count so the insights client can swap pipe names only.

The pipe mirrors sibling star-pipe contracts: insights-app-token READ, GitHub-only repo scope, collectionSlug paths gated on bucketId, and inclusive endDate for GitLab events. Documented caveats include possible negative buckets (net unstars) and first-bucket backfill inflation when snapshot history starts mid-range.

Reviewed by Cursor Bugbot for commit d8bba13. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The first granular bucket incorrectly includes the entire historical cumulative total.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds snapshot-backed period-specific star analytics compatible with the existing activity-count response contract.

Changes:

  • Derives granular star changes from cumulative totals.
  • Computes summary deltas from GitHub snapshots and GitLab events.
  • Supports project, collection, repository, and date filtering.
File Description
services/​libs/​tinybird/​pipes/​stars_count.pipe Adds the new star-count Tinybird endpoint.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread services/libs/tinybird/pipes/stars_count.pipe
Copilot AI review requested due to automatic review settings September 25, 2026 08:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The first bucket incorrectly includes the cumulative pre-range star baseline, producing inflated counts.

Review effort: Balanced
Findings: 1 High severity

Open (1)

…sion (CM-1785)

Same bug as stars_cumulative_count.pipe (crowd.dev#4768): Tinybird rejects
a node with the exact same name as its pipe, so this pipe would have
failed to deploy on first push. Renamed the terminal node from
stars_count to stars_count_final, matching the convention every other
multi-node pipe in this repo follows. No other file references the node
by name.

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings September 25, 2026 09:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Runtime Tinybird behavior still requires the planned staging validation.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 25, 2026 09:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Dynamic Tinybird SQL behavior still requires the planned staging validation.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread services/libs/tinybird/pipes/stars_count.pipe Outdated
…ct (CM-1785)

Per Copilot review: both collection branches (repo scoping and summary
mode) return no rows unless bucketId is defined, but the pipe's own
DESCRIPTION never mentioned it - a caller reading the docs could mistake
an empty response for a valid zero. stars_cumulative_count.pipe already
documents this same requirement for the same reason; stars_count.pipe
just hadn't caught up.

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings September 25, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is consistent with the sibling pipe; only minor interval documentation needs correction.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Document the implemented date range as (startDate, endDate]

services/​libs/​tinybird/​pipes/​stars_count.pipe:105

The interval notation is inconsistent with the query: both the snapshot subtraction and the GitLab predicate exclude values exactly at startDate, so the implemented range is (startDate, endDate], not [startDate, endDate]. Documenting the actual boundary avoids misleading direct callers.

…endDate] (CM-1785)

Per Copilot review: the SQL excludes a snapshot/event captured exactly at
startDate (github_at_start folds it into the baseline, and the GitLab
predicate is timestamp > startDate), so the actual range is
(startDate, endDate], not [startDate, endDate] as previously documented.

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings September 25, 2026 09:53
@themarolt

Copy link
Copy Markdown
Contributor Author

Re: Copilot review · findings outside review threads

  • Document the implemented date range as (startDate, endDate] · services/libs/tinybird/pipes/stars_count.pipe:105
    Fixed in edc8fd3: summary-mode DESCRIPTION now says (startDate, endDate] and notes that a star at exactly startDate belongs to the prior period's baseline.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Correctness depends on the pending Tinybird staging checks for cross-pipe and date-boundary behavior.

Review effort: Balanced
Findings: None

@themarolt
themarolt requested a review from epipav September 25, 2026 10:25
…CM-1785)

Per review feedback: pipe DESCRIPTION blocks shouldn't carry ticket
numbers that go stale once the work ships. Reworded the same points
without the CM-XXX references.

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings September 25, 2026 10:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Summary and granular modes disagree for values occurring exactly at an end-date bucket boundary.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Align endDate boundary handling between summary and granular modes

services/​libs/​tinybird/​pipes/​stars_count.pipe:174

Summary mode includes an event or snapshot exactly at endDate, but granular mode still excludes that value when endDate is a bucket boundary. generate_timeseries emits only buckets whose startDate < actual_end_date, while stars_cumulative_count assigns values to those buckets; for a daily (Sep 24, Sep 25] request, a value at Sep 25 00:00 is counted here but has no granular bucket. Align the boundary semantics in the cumulative/time-series path so both modes return the same period total.

@themarolt
themarolt requested a review from epipav September 25, 2026 10:42
Copilot AI review requested due to automatic review settings September 25, 2026 10:43
@themarolt
themarolt merged commit 81ac231 into main Sep 25, 2026
14 checks passed
@themarolt
themarolt deleted the feat/CM-1785-stars-count-pipe branch September 25, 2026 10:43
@themarolt

Copy link
Copy Markdown
Contributor Author

Re: Copilot review · findings outside review threads

  • Align endDate boundary handling between summary and granular modes · services/libs/tinybird/pipes/stars_count.pipe:174
    Not a regression introduced here - it's inherent to the shared generate_timeseries pipe's own boundary convention (actual_end_date = toDate(endDate), and buckets require startDate < actual_end_date), which every granularity-mode pipe in this codebase relies on as-is. Fixing it would mean changing generate_timeseries.pipe itself, shared by dozens of unrelated pipes, well out of scope for this PR. Leaving as a known edge case: an event or snapshot landing at the exact instant of endDate (typically a day boundary, e.g. 2025-01-01 00:00:00) is included by summary mode's inclusive <= cutoff but has no granular bucket to land in, since generate_timeseries never emits a bucket starting at actual_end_date itself.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Granular requests without a start date can return incomplete series, and project routing also requires an enforced bucketId.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)

Comment on lines +17 to +22
- Exactly one of `project` / `collectionSlug` must be given, same contract as
`stars_cumulative_count.pipe` / `activities_filtered.pipe`. `bucketId` is required alongside
`collectionSlug` for the same reason `stars_cumulative_count.pipe` needs it - both collection
branches here return no rows unless it's defined. The insights backend's Tinybird client
already resolves and injects `bucketId` transparently for both `project` and `collectionSlug`
on every pipe call, same as it does for every other pipe today.
toInt64(cumulativeActivityCount) - toInt64(
coalesce(lagInFrame(cumulativeActivityCount) OVER (ORDER BY startDate ASC), 0)
) AS activityCount
FROM stars_cumulative_count
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.

3 participants