Skip to content

perf: coalesce slack user info lookups#20724

Merged
seven332 merged 3 commits into
mainfrom
feat/issue-20718-slack-user-info-coalescing
Jul 9, 2026
Merged

perf: coalesce slack user info lookups#20724
seven332 merged 3 commits into
mainfrom
feat/issue-20718-slack-user-info-coalescing

Conversation

@seven332

@seven332 seven332 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a request-local Slack user info resolver with successful-result caching and in-flight lookup coalescing
  • share the resolver between Slack message enrichment and conversation context building
  • add bounded resolver telemetry and route coverage for duplicate lookup reuse plus partial user-info failure

Closes #20718

Verification

  • pnpm -F api exec vitest run src/signals/routes/tests/test-telegram-state.test.ts
  • pnpm -F api exec vitest run src/signals/routes/tests/test-slack-dispatch-probe.test.ts
  • pnpm -F api exec vitest run src/signals/routes/tests/integrations.bdd.test.ts
  • pnpm -F api check-types
  • pnpm -F api lint

Note

  • Local pre-commit full knip is currently blocked by unrelated existing platform unused exports outside this PR. The related unused exported type introduced during this change was fixed before commit.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.85507% with 7 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rc/signals/services/zero-slack-webhooks.service.ts 72.22% 5 Missing ⚠️
...s/api/src/signals/external/slack-message-client.ts 95.83% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

API preview: https://pr-20724-api.vm6.ai

@seven332 seven332 changed the title perf: coalesce Slack user info lookups perf: coalesce slack user info lookups Jul 8, 2026
@seven332 seven332 enabled auto-merge July 8, 2026 15:23
@seven332 seven332 force-pushed the feat/issue-20718-slack-user-info-coalescing branch from c7d22ac to 3edeb22 Compare July 8, 2026 15:23
@seven332

seven332 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Code Review: PR #20724

Summary

Reviewed the Slack user info resolver changes, route coverage, telemetry dimensions, and async cleanup behavior. I did not find blocking logic, performance, security, testing, or deployment-compatibility issues.

Key Findings

Critical Issues (P0)

  • None found.

High Priority (P1)

  • None found.

Testing Review

Coverage

  • perf: coalesce duplicate Slack users.info lookups between message enrichment and conversation context building -> covered by turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts.
  • partial Slack users.info failure remains non-fatal -> covered by turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts.
  • resolver timing action and privacy of telemetry dimensions -> covered by turbo/apps/api/src/signals/routes/__tests__/integrations.bdd.test.ts and turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts.

Convention Compliance

  • No internal vi.mock() usage or direct fetch mocking found in the PR diff.
  • No fake timers, fixed sleeps, timeout increases, filesystem mocks, DB mocks, or lint/type suppressions found.
  • Tests exercise the API route path and centralized Slack boundary mocks instead of importing/testing the resolver directly.

Testing Verdict: Adequate

Bad Smell Analysis

  • Mock violations: 0
  • Test coverage issues: 0
  • Defensive programming issues: 0
  • Dynamic imports: 0
  • Type safety issues in PR diff: 0
  • Timer/artificial delay issues: 0
  • Deployment compatibility issues: 0

Verification

  • pnpm -F api exec vitest run src/signals/routes/__tests__/test-telegram-state.test.ts src/signals/routes/__tests__/test-slack-dispatch-probe.test.ts src/signals/routes/__tests__/integrations.bdd.test.ts passed locally.
  • pnpm -F api lint passed locally.
  • pnpm -F api check-types was inconclusive locally because this workspace has stale ignored generated connector files under turbo/packages/connectors/src/firewall-metadata/ that are not part of this PR diff. CI lint-types and lint-types-apps are passing on the PR.

Recommendations

  • No changes requested.

Verdict

LGTM


Full review details: codereviews/20260708/pr-20724-*
Testing standards: docs/testing.md

@seven332 seven332 added this pull request to the merge queue Jul 8, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 8, 2026
@seven332 seven332 added this pull request to the merge queue Jul 9, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jul 9, 2026
@seven332 seven332 enabled auto-merge July 9, 2026 02:45
@seven332 seven332 force-pushed the feat/issue-20718-slack-user-info-coalescing branch from 3edeb22 to b564ba1 Compare July 9, 2026 02:49
@seven332 seven332 added this pull request to the merge queue Jul 9, 2026
Merged via the queue into main with commit 7bd020c Jul 9, 2026
91 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in VM0 Kanban Jul 9, 2026
@github-actions github-actions Bot mentioned this pull request Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

perf: coalesce Slack user info lookups during run creation

1 participant