Skip to content

fix: use Notion People DB for GitHub→Slack mapping in backport checker#195

Merged
snomiao merged 10 commits into
mainfrom
sno-backport-fallback
Apr 10, 2026
Merged

fix: use Notion People DB for GitHub→Slack mapping in backport checker#195
snomiao merged 10 commits into
mainfrom
sno-backport-fallback

Conversation

@snomiao

@snomiao snomiao commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Problem

The backport checker was falling back to tagging the Release Sheriff (Christian Byrne) for the majority of PRs, even when he wasn't the author. This happened because the fuzzy Slack name matching couldn't resolve most GitHub usernames.

Solution

  • Primary lookup via Notion People DB: Added lib/notion/people.ts that fetches explicit GitHub Username → Slack ID mappings from the Notion People database (37 active mappings).
  • Graceful fallback: If Notion lookup fails, falls back to the existing fuzzy Slack name matching.
  • Fallback annotation: When neither Notion nor fuzzy matching works and the release sheriff is tagged instead, the message now shows _(fallback: @sheriff)_ to make it clear this is not the PR author.
  • CLI command: Added prbot notion people command with --github, --missing options to list and verify mappings.

Verification

  • All 76 existing tests pass
  • All 37 Notion-mapped GitHub users resolve correctly (case-insensitive)
  • DrJKL (GitHub) correctly maps to drJKL (Notion) → Slack ID

Copilot AI review requested due to automatic review settings April 8, 2026 02:36
@vercel

vercel Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
comfy-pr Ready Ready Preview, Comment Apr 10, 2026 2:13pm

Request Review

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.

Pull request overview

This PR improves author-to-Slack resolution in the frontend backport checker by introducing an explicit GitHub username → Slack ID mapping sourced from a Notion “People” database, with fallback to the existing Slack fuzzy-match behavior.

Changes:

  • Added lib/notion/people.ts to fetch/cached Notion People mappings and expose GitHub→Slack lookup helpers (plus a standalone CLI entrypoint).
  • Added prbot notion people CLI command to inspect mappings and check missing GitHub usernames.
  • Updated the frontend backport checker to prefer Notion mappings, then fall back to Slack fuzzy matching, and annotate sheriff fallback tagging.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Description
lib/notion/people.ts New Notion-backed People mapping fetch + cached lookup helpers + standalone CLI mode.
bot/cli.ts Adds prbot notion people command to query/print mappings.
app/tasks/gh-frontend-backport-checker/index.ts Uses Notion People mapping as primary Slack ID resolver; annotates sheriff fallback.
bun.lock Lockfile updated for dependency graph changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/notion/people.ts Outdated
Comment thread lib/notion/people.ts Outdated
Comment thread lib/notion/people.ts Outdated
Comment thread lib/notion/people.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/tasks/gh-frontend-backport-checker/index.ts Outdated
Comment thread lib/notion/people.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/notion/people.ts Outdated
Comment thread lib/notion/people.ts Outdated
Comment thread bot/cli.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/notion/people.ts Outdated
Comment thread lib/notion/people.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

app/tasks/gh-frontend-backport-checker/index.ts:463

  • New behavior here (Notion People DB primary lookup + fallback to Slack fuzzy matching, plus logging on Notion failures) isn’t covered by the existing index.spec.ts tests for this task. Adding unit tests around findSlackUserIdByGithubUsername (e.g., Notion hit returns ID; Notion miss falls back to Slack match; Notion error falls back without throwing) would help prevent regressions without requiring real Notion/Slack calls.
export async function findSlackUserIdByGithubUsername(
  githubUsername: string,
): Promise<string | null> {
  try {
    // Primary: Notion People database (explicit GitHub→Slack mapping)
    const notionSlackId = await findSlackIdFromNotion(githubUsername);
    if (notionSlackId) return notionSlackId;
  } catch (e) {
    logger.warn("Failed to look up Notion People mapping, falling back to Slack fuzzy match", {
      githubUsername,
      error: (e as Error)?.message ?? String(e),
      stack: (e as Error)?.stack,
    });
  }

  try {
    // Fallback: fuzzy match against Slack workspace members
    const members = await getAllSlackMembers();
    const lowerGh = githubUsername.toLowerCase();
    const found = members.find((m) => {
      if (m.deleted || m.is_bot) return false;
      const profile = m.profile as Record<string, unknown> | undefined;
      return (
        m.name?.toLowerCase() === lowerGh ||
        (profile?.display_name as string)?.toLowerCase() === lowerGh ||
        ((profile?.real_name as string | undefined) || "")
          .toLowerCase()
          .replace(/\s+/g, "")
          .includes(lowerGh)
      );
    });
    return (found?.id as string) || null;
  } catch (e) {
    logger.warn("Failed to look up Slack user for GitHub username", {
      githubUsername,
      error: e,
    });
    return null;
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/tasks/gh-frontend-backport-checker/index.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/tasks/gh-frontend-backport-checker/index.ts Outdated
Comment thread bot/cli.ts Outdated

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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/notion/people.ts
@snomiao
snomiao force-pushed the sno-backport-fallback branch from ff9eeb2 to 2565be2 Compare April 10, 2026 06:04
@snomiao
snomiao enabled auto-merge (squash) April 10, 2026 06:04
snomiao and others added 9 commits April 10, 2026 23:03
- Add lib/notion/people.ts to fetch explicit GitHub→Slack mappings from Notion People database
- Backport checker now uses Notion mapping first, falls back to fuzzy Slack name matching
- Fallback tags now show '(fallback: @sheriff)' instead of bare '@sheriff'
- Add 'prbot notion people' CLI command to list/verify mappings

Amp-Thread-ID: https://ampcode.com/threads/T-019d6ae4-7d7e-724e-a4de-c5ea815e505d
Co-authored-by: Amp <amp@ampcode.com>
…e on failure

- Switch to the shared `notion` client from @/lib so responses hit Keyv cache.
- Trim rich_text values at parse time; whitespace-only entries are now skipped.
- Reset the module-level cache on rejection so transient failures can retry.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Drops eslint/biome suppressions in favor of a NotionProp type, and caches the GitHub→Slack Map so repeated lookups don't reallocate.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Avoids spamming Notion + logs once per PR author when the People DB call fails (missing token, transient outage). Per-process guard resets each run.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test consistently times out at the default 5s on CI runners.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@snomiao
snomiao requested a review from Copilot April 10, 2026 15:08
@snomiao
snomiao merged commit bcf045a into main Apr 10, 2026
8 checks passed
@snomiao
snomiao deleted the sno-backport-fallback branch April 10, 2026 15:09
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/notion/people.ts
Comment on lines +56 to +62
const person = props?.["Person"]?.people?.map((p) => p.name).join(", ") || "";
const githubUsername = (
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

props?.["Person"]?.people?.map(...).join(", ") can still throw at runtime: if people is undefined, people?.map(...) evaluates to undefined and then .join(...) is invoked on undefined. Use optional chaining on join as well (or default to an empty array) so missing/empty properties don’t crash the Notion sync.

Suggested change
const person = props?.["Person"]?.people?.map((p) => p.name).join(", ") || "";
const githubUsername = (
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const person = (props?.["Person"]?.people ?? []).map((p) => p.name).join(", ");
const githubUsername = (props?.["GitHub Username"]?.rich_text ?? [])
.map((t) => t.plain_text)
.join("")
.trim();
const slackId = (props?.["SlackID"]?.rich_text ?? [])
.map((t) => t.plain_text)
.join("")
.trim();

Copilot uses AI. Check for mistakes.
Comment thread lib/notion/people.ts
Comment on lines +58 to +61
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

props?.["GitHub Username"]?.rich_text?.map(...).join("") has the same issue as above: if the property (or rich_text) is missing, .map(...) returns undefined and .join(...) will throw. Add optional chaining for join (or default to []) before trimming, so pages with missing fields don’t crash the whole fetch.

Suggested change
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text)?.join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text)?.join("") || ""

Copilot uses AI. Check for mistakes.
Comment thread lib/notion/people.ts
Comment on lines +56 to +61
const person = props?.["Person"]?.people?.map((p) => p.name).join(", ") || "";
const githubUsername = (
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

props?.["SlackID"]?.rich_text?.map(...).join("") can throw when SlackID (or its rich_text) is absent for a row because .join(...) is called on a possibly-undefined .map(...) result. Make join optional (or default to []) before trimming to keep the query resilient to incomplete rows.

Suggested change
const person = props?.["Person"]?.people?.map((p) => p.name).join(", ") || "";
const githubUsername = (
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text).join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text).join("") || ""
const person = props?.["Person"]?.people?.map((p) => p.name)?.join(", ") || "";
const githubUsername = (
props?.["GitHub Username"]?.rich_text?.map((t) => t.plain_text)?.join("") || ""
).trim();
const slackId = (
props?.["SlackID"]?.rich_text?.map((t) => t.plain_text)?.join("") || ""

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants