feat(gold): collab_person_counter_daily scaffold + Messaging counters (#1527) - #1567
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f7d323a2ba6b7f421fd07cb9b1149e8dfbf0783b and 952394b. 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a new ClickHouse view ChangesCollab Person Counter Daily Scaffold
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ingestion/scripts/migrations/20260702000000_collab-person-counter-daily.sql (1)
46-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: collapse three
FINALscans of the same table into one pass.The M365/Slack/Zulip branches each do a full
FINALscan ofsilver.class_collab_chat_activityfiltered bydata_source. A single scan with per-vendorCASE/multiIfprojection overdata_source IN (...)would avoid reading the table three times. Given this is a clone ofai_person_counter_daily, keeping the readable UNION shape is defensible — flagging only as a potential optimization if the source grows large.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/scripts/migrations/20260702000000_collab-person-counter-daily.sql` around lines 46 - 92, The current UNION ALL in the raw subquery scans silver.class_collab_chat_activity with FINAL three times for M365, Slack, and Zulip. Consider rewriting the extraction as a single pass over data_source IN (...) and use CASE/multiIf to derive messages_sent and channel_posts per source, while keeping the same person_id and metric_date logic in the raw subquery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/ingestion/scripts/migrations/20260702000000_collab-person-counter-daily.sql`:
- Around line 46-92: The current UNION ALL in the raw subquery scans
silver.class_collab_chat_activity with FINAL three times for M365, Slack, and
Zulip. Consider rewriting the extraction as a single pass over data_source IN
(...) and use CASE/multiIf to derive messages_sent and channel_posts per source,
while keeping the same person_id and metric_date logic in the raw subquery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56a19a8f-be25-4461-ade5-4cfaefbf583e
📥 Commits
Reviewing files that changed from the base of the PR and between 84862c5 and f6e5a110373bfc34f87daeab98c61c51a772af09.
📒 Files selected for processing (4)
src/ingestion/dbt/tests/gold/assert_collab_messaging_bounds.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/scripts/migrations/20260702000000_collab-person-counter-daily.sqlsrc/ingestion/silver/_shared/gold_sources.yml
f6e5a11 to
f7d323a
Compare
…#1527) PR A of the #1527 split — the data-layer foundation. Establishes the shared gold view the other collab modalities (#1528–#1532) extend, and delivers the Messaging modality's two counters. - New gold view `insight.collab_person_counter_daily` (migration 20260702000000): honest-NULL UNION ALL over the per-vendor silver chat classes (M365 Teams · Slack · Zulip), outer GROUP BY (person_id, metric_date) with the `if(countIf(x IS NOT NULL) > 0, sumIf …, NULL)` wrapper so a person with no source on a day is NULL, never a fake 0. LEFT JOIN insight.people for org_unit_id. Clone of #1514's ai_person_counter_daily shape. · messages_sent — Σ total_chat_messages across the three vendors. · channel_posts — Σ (channel_posts + channel_replies) across M365 · Slack for vendor comparability (Slack folds replies in already; M365 splits them); Zulip has no channel split → honest-NULL. - dbt singular test `assert_collab_messaging_bounds.sql`: counters ≥ 0. - Register the view in silver/_shared/gold_sources.yml. Designed for extension: later modalities add columns + UNION branches, not new vendor rows. Backend query_ref + catalog seed (PR B), e2e YAML (PR C) and FE (insight-front) follow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…review) Addresses the PR review of the #1527 gold-view scaffold. - BLOCKER: silver.class_collab_chat_activity placeholder in create-bronze-placeholders.sh lacked `channel_replies`, which the M365 branch of collab_person_counter_daily references. ClickHouse validates columns at CREATE VIEW time, so on a fresh cluster / in e2e (placeholders → migrations, before dbt builds the real silver) the migration would fail. Add the column. - MEDIUM: read class_collab_chat_activity (a ReplacingMergeTree) with FINAL in all three vendor branches so a re-synced day with >1 version per key is not double-summed — matches the class_collab_meeting_activity read in 20260518000000_collab-bullet-rewrite.sql. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
333d99f to
952394b
Compare
…ed (#1527) PR B of the #1527 split — the backend layer over PR A's gold view. Clones the - Query_ref migration (m20260702_000001): `collab_messaging_counters_qr` reads insight.collab_person_counter_daily, re-aggregates per person with the honest-NULL `if(countIf(x IS NOT NULL) > 0, sumIf …, NULL)` wrapper, ARRAY JOIN unpivot (drops NULLs), and LEFT JOINs per-org_unit_id `quantileExact` bands. Stored on metrics id …0053 (next free after AI counters …0052). - Catalog seed (m20260702_000002): product-default metric_catalog rows for `messages_sent` + `channel_posts`, each with an initial product-default metric_threshold (catalog-probe requirement) and a metric_query_catalog junction link to the query. source_tags carry the vendors (m365/slack/zulip). Thresholds are initial defaults, NOT calibrated (per-org_unit calibration is a post-release follow-up). - Register both migrations in mod.rs. Tests: - Rust unit (feeds #1433): asserts the generated peer-counter SQL emits both keys with the honest-NULL wrapper + per-org_unit band join — no DB, runs in CI coverage. - Extend catalog live_tests (`product_default_wins_when_no_tenant_overlay`) to assert both Messaging keys resolve at product-default. Designed for extension: later modalities add keys to the ARRAY JOIN list + seed rows. Depends on the gold view from PR A (#1567) at deploy time. Honest-NULL rendering is #1517. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…tructorfabric#1527) PR D of the constructorfabric#1527 split — the frontend surface for the collaboration Messaging counters (backend: gold view constructorfabric#1567 + query/seed). Mirrors the AI-adoption peer-counters panel (constructorfabric#1514). - metric-registry: add V2_IC_COLLAB_PEER_COUNTERS (…0053), the seeded collab Messaging peer-counter query. - ic-extras: add useIcCollabPeerCounters (clone of useIcAiPeerCounters, same row shape + per-org_unit bands, different metric id). - new CollabMessagingPanel: renders messages_sent + channel_posts vs the person's department under a "Messaging" card via the shared PeerStorySection. Fully catalog-driven — any counter row whose metric_key is in the catalog renders; unknown keys are dropped. - mount it in the collaboration section drilldown (section-drilldown-sheet). - catalog-snapshot mock: add the two collab_person_counter_daily.* metrics. - vitest: renders both under "Messaging"; drops unseeded keys; renders nothing without person/range. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
PR A of the #1527 split (see #1516 · release #1526) — the data-layer foundation. Establishes the shared gold view the other collab modalities (#1528–#1532) extend, and delivers the Messaging modality's two counters.
Split plan: A (this) → B backend query_ref + catalog seed → C e2e YAML → D FE (insight-front).
Changes
insight.collab_person_counter_daily(migration20260702000000), cloning feat: improve AI personal analytics queries #1514'sai_person_counter_daily:UNION ALLover the per-vendor silver chat classes (M365 Teams · Slack · Zulip),GROUP BY (person_id, metric_date)withif(countIf(x IS NOT NULL) > 0, sumIf(x, …), NULL)→ a person with no source on a day is NULL, never a fake 0 (a real 0 row stays 0),LEFT JOIN insight.peoplefororg_unit_id(feeds the per-org_unit peer bands in PR B).messages_senttotal_chat_messagesover M365 · Slack · Zulipchannel_postschannel_posts+channel_replies) over M365 · Slackchannel_postsalready (channel_repliesNULL); M365 splits → add both for comparability; Zulip has no channel split → honest-NULLassert_collab_messaging_bounds.sql: counters ≥ 0.silver/_shared/gold_sources.yml.Design-for-extension
The view/test are the scaffold: later modalities add columns + UNION branches (not new vendor-named rows). Honest-NULL rendering ("No data" / source tooltip) is #1517; thresholds/bands land with PR B; this PR is data-layer only.
Testing / validation
ch.sh --target dev):messages_sent == m365_teams_chats + slack_messages_sent + zulip_messages_sentper QUALITY 5: data + UX: honest-NULL and showing missing data (collaboration counters) #1517 NULL rules.Refs #1527 · #1516
🤖 Generated with Claude Code
Summary by CodeRabbit