From 98f9404dc9adf2792f334fa4da3cf80bf620fa8d Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Thu, 4 Jun 2026 11:47:37 +0100 Subject: [PATCH 1/2] feat(mobile): edit suggested reviewers on inbox report detail (port #2481) Makes the suggested reviewers on an inbox signal report editable from the report detail screen, porting desktop PR #2481 to the mobile app. - New `updateSignalReportArtefact` API method (PUT-replaces artefact content). - New `useUpdateSuggestedReviewers` mutation hook with an optimistic full-replacement update that patches the cached artefacts and reconciles with server-canonicalized data on settle. - `SuggestedReviewers` becomes editable: removable reviewer chips plus an "Add" affordance opening a searchable picker (`EditReviewersSheet`) over the org's available reviewers. - Extracted a shared `ReviewerOptionRow` reused by the add picker and the existing `ReviewerFilterSheet`, and shared reviewer-option / write-shape helpers in `utils.ts`. - Fires `add_suggested_reviewer` / `remove_suggested_reviewer` analytics with `suggested_reviewer_login` / `suggested_reviewer_uuid`. - Unit tests for the write-shape mapping and reviewer matching helpers. Generated-By: PostHog Code Task-Id: a3560a8b-5ba2-46b2-bfe5-d0a9f93db8e2 --- apps/mobile/src/app/inbox/[...id].tsx | 47 +++- apps/mobile/src/features/inbox/api.ts | 28 ++ .../inbox/components/EditReviewersSheet.tsx | 126 +++++++++ .../inbox/components/ReviewerFilterSheet.tsx | 96 +------ .../inbox/components/ReviewerOptionRow.tsx | 53 ++++ .../inbox/components/SuggestedReviewers.tsx | 241 ++++++++++++++---- .../features/inbox/hooks/useInboxReports.ts | 49 ++++ apps/mobile/src/features/inbox/types.ts | 25 +- apps/mobile/src/features/inbox/utils.test.ts | 102 +++++++- apps/mobile/src/features/inbox/utils.ts | 78 ++++++ apps/mobile/src/lib/analytics.ts | 4 + 11 files changed, 695 insertions(+), 154 deletions(-) create mode 100644 apps/mobile/src/features/inbox/components/EditReviewersSheet.tsx create mode 100644 apps/mobile/src/features/inbox/components/ReviewerOptionRow.tsx diff --git a/apps/mobile/src/app/inbox/[...id].tsx b/apps/mobile/src/app/inbox/[...id].tsx index 2438b71813..031443224f 100644 --- a/apps/mobile/src/app/inbox/[...id].tsx +++ b/apps/mobile/src/app/inbox/[...id].tsx @@ -23,6 +23,7 @@ import { View, } from "react-native"; import { useSafeAreaInsets } from "react-native-safe-area-context"; +import { useUserQuery } from "@/features/auth"; import { MarkdownText } from "@/features/chat/components/MarkdownText"; import { getReportRepository } from "@/features/inbox/api"; import { DiscussReportSheet } from "@/features/inbox/components/DiscussReportSheet"; @@ -31,7 +32,10 @@ import { DismissReportSheet, } from "@/features/inbox/components/DismissReportSheet"; import { SignalCard } from "@/features/inbox/components/SignalCard"; -import { SuggestedReviewers } from "@/features/inbox/components/SuggestedReviewers"; +import { + type ReviewerActionExtra, + SuggestedReviewers, +} from "@/features/inbox/components/SuggestedReviewers"; import { DISMISSAL_REASON_OPTIONS } from "@/features/inbox/constants"; import { useInboxEngagementTracker } from "@/features/inbox/hooks/useInboxEngagementTracker"; import { @@ -45,10 +49,14 @@ import type { SignalFindingContent, SignalReportPriority, SignalReportStatus, - SuggestedReviewer, + SuggestedReviewersArtefact, } from "@/features/inbox/types"; import { inboxStatusLabel } from "@/features/inbox/utils"; -import { computeReportAgeHours, useAnalytics } from "@/lib/analytics"; +import { + computeReportAgeHours, + type InboxReportActionType, + useAnalytics, +} from "@/lib/analytics"; import { useThemeColors } from "@/lib/theme"; const statusColorMap: Record = { @@ -134,6 +142,7 @@ export default function ReportDetailScreen() { const insets = useSafeAreaInsets(); const posthog = usePostHog(); const { data: report, isLoading, error } = useInboxReport(reportId ?? null); + const { data: me } = useUserQuery(); const [reportRepo, setReportRepo] = useState(null); const [dismissOpen, setDismissOpen] = useState(false); const [discussOpen, setDiscussOpen] = useState(false); @@ -177,6 +186,23 @@ export default function ReportDetailScreen() { [tracker], ); + const fireReviewerAction = useCallback( + (action_type: InboxReportActionType, extra?: ReviewerActionExtra) => { + if (!report) return; + tracker.signalAction({ + report_id: report.id, + report_title: report.title ?? null, + report_age_hours: computeReportAgeHours(report.created_at), + action_type, + surface: "detail_pane", + is_bulk: false, + bulk_size: 1, + ...extra, + }); + }, + [report, tracker], + ); + const handleToggleSignals = useCallback(() => { // Fire analytics outside the state updater — Strict Mode double-invokes // updaters in development, which would double-fire the event. @@ -221,13 +247,13 @@ export default function ReportDetailScreen() { return null; }, [artefacts]); - const suggestedReviewers = useMemo((): SuggestedReviewer[] => { + const reviewerArtefact = useMemo((): SuggestedReviewersArtefact | null => { for (const a of artefacts) { if (a.type === "suggested_reviewers") { - return (a.content as SuggestedReviewer[]) ?? []; + return a as SuggestedReviewersArtefact; } } - return []; + return null; }, [artefacts]); const findingsBySignalId = useMemo(() => { @@ -471,7 +497,14 @@ export default function ReportDetailScreen() { )} {/* Suggested reviewers */} - + {reviewerArtefact && ( + + )} {/* Signals */} {signals.length > 0 && ( diff --git a/apps/mobile/src/features/inbox/api.ts b/apps/mobile/src/features/inbox/api.ts index 880253e15f..6c6bd190e5 100644 --- a/apps/mobile/src/features/inbox/api.ts +++ b/apps/mobile/src/features/inbox/api.ts @@ -16,6 +16,7 @@ import type { SignalReportsQueryParams, SignalReportsResponse, SignalReportTask, + SuggestedReviewerWriteEntry, } from "./types"; export async function getSignalReports( @@ -195,6 +196,33 @@ export async function getSignalReportArtefacts( return { results, count: data.count ?? results.length }; } +/** Replace the content of a report artefact (full PUT, not a partial update). */ +export async function updateSignalReportArtefact( + reportId: string, + artefactId: string, + content: SuggestedReviewerWriteEntry[], +): Promise { + const baseUrl = getBaseUrl(); + const projectId = getProjectId(); + + const response = await authedFetch( + `${baseUrl}/api/projects/${projectId}/signals/reports/${reportId}/artefacts/${artefactId}/`, + { + method: "PUT", + body: JSON.stringify({ content }), + }, + ); + + if (!response.ok) { + const errorText = await response.text().catch(() => ""); + throw new HttpError( + response.status, + response.statusText, + errorText || "Failed to update suggested reviewers", + ); + } +} + export async function getSignalReportSignals( reportId: string, ): Promise { diff --git a/apps/mobile/src/features/inbox/components/EditReviewersSheet.tsx b/apps/mobile/src/features/inbox/components/EditReviewersSheet.tsx new file mode 100644 index 0000000000..01738ffb00 --- /dev/null +++ b/apps/mobile/src/features/inbox/components/EditReviewersSheet.tsx @@ -0,0 +1,126 @@ +import { Text } from "@components/text"; +import { MagnifyingGlass } from "phosphor-react-native"; +import { useMemo, useState } from "react"; +import { + ActivityIndicator, + Modal, + Pressable, + ScrollView, + TextInput, + View, +} from "react-native"; +import { useScreenInsets } from "@/hooks/useScreenInsets"; +import { useThemeColors } from "@/lib/theme"; +import { useAvailableSuggestedReviewers } from "../hooks/useInboxReports"; +import type { AvailableSuggestedReviewer, SuggestedReviewer } from "../types"; +import { buildReviewerOptions, reviewerMatchesAvailable } from "../utils"; +import { ReviewerOptionRow } from "./ReviewerOptionRow"; + +interface EditReviewersSheetProps { + visible: boolean; + reviewers: SuggestedReviewer[]; + meUuid?: string | null; + onClose: () => void; + onToggle: (option: AvailableSuggestedReviewer) => void; +} + +export function EditReviewersSheet({ + visible, + reviewers, + meUuid, + onClose, + onToggle, +}: EditReviewersSheetProps) { + const { bottom, sheetContentTop } = useScreenInsets(); + const themeColors = useThemeColors(); + const { data: available, isLoading } = useAvailableSuggestedReviewers({ + enabled: visible, + }); + const [query, setQuery] = useState(""); + + const options = useMemo( + () => buildReviewerOptions(available?.results ?? [], meUuid ?? undefined), + [available?.results, meUuid], + ); + + const filtered = useMemo(() => { + const q = query.trim().toLowerCase(); + if (!q) return options; + return options.filter( + (o) => + o.name.toLowerCase().includes(q) || + o.email.toLowerCase().includes(q) || + o.github_login.toLowerCase().includes(q), + ); + }, [options, query]); + + return ( + + + + + Add reviewer + + + + Done + + + + + + + + + + + + {isLoading && options.length === 0 ? ( + + + + ) : filtered.length === 0 ? ( + + No users found + + ) : ( + + {filtered.map((reviewer) => ( + + reviewerMatchesAvailable(r, reviewer), + )} + onPress={() => onToggle(reviewer)} + /> + ))} + + )} + + + ); +} diff --git a/apps/mobile/src/features/inbox/components/ReviewerFilterSheet.tsx b/apps/mobile/src/features/inbox/components/ReviewerFilterSheet.tsx index 7cdf8b1437..d07faaffa1 100644 --- a/apps/mobile/src/features/inbox/components/ReviewerFilterSheet.tsx +++ b/apps/mobile/src/features/inbox/components/ReviewerFilterSheet.tsx @@ -1,9 +1,7 @@ import { Text } from "@components/text"; -import { Check } from "phosphor-react-native"; import { useMemo } from "react"; import { ActivityIndicator, - Image, Modal, Pressable, ScrollView, @@ -14,55 +12,14 @@ import { useScreenInsets } from "@/hooks/useScreenInsets"; import { useThemeColors } from "@/lib/theme"; import { useAvailableSuggestedReviewers } from "../hooks/useInboxReports"; import { useInboxFilterStore } from "../stores/inboxFilterStore"; -import type { AvailableSuggestedReviewer } from "../types"; +import { buildReviewerOptions } from "../utils"; +import { ReviewerOptionRow } from "./ReviewerOptionRow"; interface ReviewerFilterSheetProps { visible: boolean; onClose: () => void; } -interface ReviewerOption { - uuid: string; - name: string; - email: string; - github_login: string; - isMe: boolean; -} - -function buildReviewerOptions( - reviewers: AvailableSuggestedReviewer[], - currentUserUuid: string | undefined, -): ReviewerOption[] { - const seen = new Set(); - const options: ReviewerOption[] = []; - - for (const r of reviewers) { - if (!r.uuid || seen.has(r.uuid)) continue; - seen.add(r.uuid); - options.push({ - uuid: r.uuid, - name: r.name?.trim() || "", - email: r.email?.trim() || "", - github_login: r.github_login?.trim() || "", - isMe: r.uuid === currentUserUuid, - }); - } - - // Sort: "Me" first, then alphabetical by name - options.sort((a, b) => { - if (a.isMe && !b.isMe) return -1; - if (!a.isMe && b.isMe) return 1; - return (a.name || a.email).localeCompare(b.name || b.email); - }); - - return options; -} - -function displayName(r: ReviewerOption): string { - const base = r.name || r.email || "Unknown user"; - return r.isMe ? `${base} (Me)` : base; -} - export function ReviewerFilterSheet({ visible, onClose, @@ -136,55 +93,14 @@ export function ReviewerFilterSheet({ }} > {options.map((reviewer, index) => { - const isSelected = suggestedReviewerFilter.includes( - reviewer.uuid, - ); const showDivider = reviewer.isMe && index < options.length - 1; - return ( - toggleSuggestedReviewer(reviewer.uuid)} - className="flex-row items-center justify-between rounded-md px-2 py-2.5 active:bg-gray-3" - > - - {reviewer.github_login ? ( - - ) : ( - - - {(reviewer.name || - reviewer.email || - "?")[0].toUpperCase()} - - - )} - - - {displayName(reviewer)} - - {reviewer.email && ( - - {reviewer.email} - - )} - - - {isSelected && ( - - )} - + /> {showDivider && ( )} diff --git a/apps/mobile/src/features/inbox/components/ReviewerOptionRow.tsx b/apps/mobile/src/features/inbox/components/ReviewerOptionRow.tsx new file mode 100644 index 0000000000..a6f9c5c265 --- /dev/null +++ b/apps/mobile/src/features/inbox/components/ReviewerOptionRow.tsx @@ -0,0 +1,53 @@ +import { Text } from "@components/text"; +import { Check } from "phosphor-react-native"; +import { Image, Pressable, View } from "react-native"; +import { useThemeColors } from "@/lib/theme"; +import { type ReviewerOption, reviewerOptionLabel } from "../utils"; + +interface ReviewerOptionRowProps { + reviewer: ReviewerOption; + selected: boolean; + onPress: () => void; +} + +export function ReviewerOptionRow({ + reviewer, + selected, + onPress, +}: ReviewerOptionRowProps) { + const themeColors = useThemeColors(); + return ( + + + {reviewer.github_login ? ( + + ) : ( + + + {(reviewer.name || reviewer.email || "?")[0].toUpperCase()} + + + )} + + + {reviewerOptionLabel(reviewer)} + + {reviewer.email && ( + + {reviewer.email} + + )} + + + {selected && } + + ); +} diff --git a/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx b/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx index ee8e1ead7a..a1665b0ee7 100644 --- a/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx +++ b/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx @@ -1,67 +1,210 @@ import { Text } from "@components/text"; -import { Eye } from "phosphor-react-native"; -import { Image, Linking, Pressable, ScrollView, View } from "react-native"; +import { Eye, Plus, X } from "phosphor-react-native"; +import { useMemo, useState } from "react"; +import { + ActivityIndicator, + Image, + Linking, + Pressable, + ScrollView, + View, +} from "react-native"; +import type { + InboxReportActionProperties, + InboxReportActionType, +} from "@/lib/analytics"; import { useThemeColors } from "@/lib/theme"; -import type { SuggestedReviewer } from "../types"; +import { useUpdateSuggestedReviewers } from "../hooks/useInboxReports"; +import type { + AvailableSuggestedReviewer, + SuggestedReviewer, + SuggestedReviewersArtefact, +} from "../types"; +import { + reviewerMatchesAvailable, + toSuggestedReviewerWriteContent, +} from "../utils"; +import { EditReviewersSheet } from "./EditReviewersSheet"; + +export type ReviewerActionExtra = Pick< + InboxReportActionProperties, + "suggested_reviewer_login" | "suggested_reviewer_uuid" +>; interface SuggestedReviewersProps { - reviewers: SuggestedReviewer[]; + reportId: string; + artefact: SuggestedReviewersArtefact; meUuid?: string | null; + fireAction: ( + action: InboxReportActionType, + extra?: ReviewerActionExtra, + ) => void; } export function SuggestedReviewers({ - reviewers, + reportId, + artefact, meUuid, + fireAction, }: SuggestedReviewersProps) { const themeColors = useThemeColors(); - if (reviewers.length === 0) return null; + const [editOpen, setEditOpen] = useState(false); + const { mutate: updateReviewers, isPending } = + useUpdateSuggestedReviewers(reportId); + + const reviewers = artefact.content; + + const displayReviewers = useMemo(() => { + if (!meUuid) return reviewers; + const meIndex = reviewers.findIndex((r) => r.user?.uuid === meUuid); + if (meIndex <= 0) return reviewers; + return [reviewers[meIndex], ...reviewers.filter((_, i) => i !== meIndex)]; + }, [reviewers, meUuid]); + + const removeReviewer = (target: SuggestedReviewer) => { + const next = reviewers.filter((r) => r !== target); + fireAction("remove_suggested_reviewer", { + suggested_reviewer_login: target.github_login || undefined, + suggested_reviewer_uuid: target.user?.uuid, + }); + updateReviewers({ + artefactId: artefact.id, + content: toSuggestedReviewerWriteContent(next), + optimisticReviewers: next, + }); + }; + + const toggleReviewer = (option: AvailableSuggestedReviewer) => { + const existing = reviewers.find((r) => reviewerMatchesAvailable(r, option)); + if (existing) { + removeReviewer(existing); + return; + } + + const optimisticEntry: SuggestedReviewer = { + github_login: option.github_login, + github_name: option.name || null, + relevant_commits: [], + user: { + id: 0, + uuid: option.uuid, + email: option.email, + first_name: option.name, + last_name: "", + }, + }; + fireAction("add_suggested_reviewer", { + suggested_reviewer_login: option.github_login || undefined, + suggested_reviewer_uuid: option.uuid, + }); + updateReviewers({ + artefactId: artefact.id, + content: [ + ...toSuggestedReviewerWriteContent(reviewers), + { user_uuid: option.uuid }, + ], + optimisticReviewers: [...reviewers, optimisticEntry], + }); + }; return ( - - Suggested reviewers - - - {reviewers.map((reviewer) => { - const isMe = - !!reviewer.user?.uuid && !!meUuid && reviewer.user.uuid === meUuid; - const displayName = - reviewer.user?.first_name ?? - reviewer.github_name ?? - reviewer.github_login; - return ( - - Linking.openURL(`https://github.com/${reviewer.github_login}`) - } - hitSlop={4} - className="flex-row items-center gap-2 rounded-full border border-gray-6 bg-gray-2 py-1.5 pr-3 pl-1.5 active:opacity-70" - > - - {displayName} - {isMe && ( - - + + Suggested reviewers + + {isPending && ( + + )} + + setEditOpen(true)} + accessibilityLabel="Add suggested reviewer" + hitSlop={6} + className="flex-row items-center gap-1 rounded-full border border-gray-6 px-2.5 py-1 active:opacity-70" + > + + Add + + + + {displayReviewers.length === 0 ? ( + + No reviewers assigned. Use “Add” to suggest one. + + ) : ( + + {displayReviewers.map((reviewer) => { + const isMe = + !!reviewer.user?.uuid && + !!meUuid && + reviewer.user.uuid === meUuid; + const displayName = + reviewer.user?.first_name ?? + reviewer.github_name ?? + reviewer.github_login; + return ( + + { + fireAction("click_suggested_reviewer", { + suggested_reviewer_login: reviewer.github_login, + }); + Linking.openURL( + `https://github.com/${reviewer.github_login}`, + ); + }} + hitSlop={4} + className="flex-row items-center gap-2 active:opacity-70" + > + - - )} - - ); - })} - + + {displayName} + + {isMe && ( + + + + )} + + removeReviewer(reviewer)} + disabled={isPending} + accessibilityLabel={`Remove ${displayName}`} + hitSlop={6} + className="h-5 w-5 items-center justify-center rounded-full active:bg-gray-4 disabled:opacity-50" + > + + + + ); + })} + + )} + + setEditOpen(false)} + onToggle={toggleReviewer} + /> ); } diff --git a/apps/mobile/src/features/inbox/hooks/useInboxReports.ts b/apps/mobile/src/features/inbox/hooks/useInboxReports.ts index fc122dd8db..1394ee4eed 100644 --- a/apps/mobile/src/features/inbox/hooks/useInboxReports.ts +++ b/apps/mobile/src/features/inbox/hooks/useInboxReports.ts @@ -9,6 +9,7 @@ import { getSignalReportArtefacts, getSignalReportSignals, getSignalReports, + updateSignalReportArtefact, } from "../api"; import { INBOX_REFETCH_INTERVAL_MS } from "../constants"; import { useInboxFilterStore } from "../stores/inboxFilterStore"; @@ -20,6 +21,8 @@ import type { SignalReportSignalsResponse, SignalReportsQueryParams, SignalReportsResponse, + SuggestedReviewer, + SuggestedReviewerWriteEntry, } from "../types"; import { buildSignalReportListOrdering, @@ -143,6 +146,52 @@ export function useInboxReportSignals(reportId: string | null) { }); } +interface UpdateSuggestedReviewersVariables { + artefactId: string; + content: SuggestedReviewerWriteEntry[]; + optimisticReviewers: SuggestedReviewer[]; +} + +export function useUpdateSuggestedReviewers(reportId: string) { + const queryClient = useQueryClient(); + const queryKey = inboxKeys.artefacts(reportId); + + return useMutation< + void, + Error, + UpdateSuggestedReviewersVariables, + { previous: SignalReportArtefactsResponse | undefined } + >({ + mutationFn: ({ artefactId, content }) => + updateSignalReportArtefact(reportId, artefactId, content), + onMutate: async ({ artefactId, optimisticReviewers }) => { + await queryClient.cancelQueries({ queryKey }); + const previous = + queryClient.getQueryData(queryKey); + if (previous) { + queryClient.setQueryData(queryKey, { + ...previous, + results: previous.results.map((artefact) => + artefact.id === artefactId && + artefact.type === "suggested_reviewers" + ? { ...artefact, content: optimisticReviewers } + : artefact, + ), + }); + } + return { previous }; + }, + onError: (_err, _vars, context) => { + if (context?.previous) { + queryClient.setQueryData(queryKey, context.previous); + } + }, + onSettled: () => { + queryClient.invalidateQueries({ queryKey }); + }, + }); +} + export function useDismissReport(reportId: string) { const queryClient = useQueryClient(); diff --git a/apps/mobile/src/features/inbox/types.ts b/apps/mobile/src/features/inbox/types.ts index c7a0eca732..cef53a7d7d 100644 --- a/apps/mobile/src/features/inbox/types.ts +++ b/apps/mobile/src/features/inbox/types.ts @@ -129,6 +129,24 @@ export interface SuggestedReviewer { user: SuggestedReviewerUser | null; } +export interface SuggestedReviewersArtefact { + id: string; + type: "suggested_reviewers"; + created_at: string; + content: SuggestedReviewer[]; +} + +/** + * Write shape for replacing the suggested_reviewers artefact. The server + * canonicalizes to a lowercase `github_login`, with `user_uuid` winning when + * both are supplied. + */ +export interface SuggestedReviewerWriteEntry { + github_login?: string; + user_uuid?: string; + github_name?: string; +} + export type ReportArtefact = | { id: string; @@ -148,12 +166,7 @@ export type ReportArtefact = created_at: string; content: SignalFindingContent; } - | { - id: string; - type: "suggested_reviewers"; - created_at: string; - content: SuggestedReviewer[]; - } + | SuggestedReviewersArtefact | { id: string; type: string; diff --git a/apps/mobile/src/features/inbox/utils.test.ts b/apps/mobile/src/features/inbox/utils.test.ts index 56c53cb951..7e4825270e 100644 --- a/apps/mobile/src/features/inbox/utils.test.ts +++ b/apps/mobile/src/features/inbox/utils.test.ts @@ -1,6 +1,40 @@ import { describe, expect, it } from "vitest"; -import type { SignalReport, SignalReportStatus } from "./types"; -import { buildInboxViewedProperties } from "./utils"; +import type { + AvailableSuggestedReviewer, + SignalReport, + SignalReportStatus, + SuggestedReviewer, +} from "./types"; +import { + buildInboxViewedProperties, + buildReviewerOptions, + reviewerMatchesAvailable, + toSuggestedReviewerWriteContent, +} from "./utils"; + +function makeReviewer( + partial: Partial = {}, +): SuggestedReviewer { + return { + github_login: "octocat", + github_name: "The Octocat", + relevant_commits: [], + user: null, + ...partial, + }; +} + +function makeAvailable( + partial: Partial = {}, +): AvailableSuggestedReviewer { + return { + uuid: "uuid-1", + name: "Ada Lovelace", + email: "ada@example.com", + github_login: "ada", + ...partial, + }; +} const DEFAULT_STATUS_FILTER: SignalReportStatus[] = [ "ready", @@ -135,3 +169,67 @@ describe("buildInboxViewedProperties", () => { expect(props.has_active_filters).toBe(false); }); }); + +describe("toSuggestedReviewerWriteContent", () => { + it("prefers github_login so the server preserves commits/name", () => { + const content = toSuggestedReviewerWriteContent([ + makeReviewer({ + github_login: "ada", + user: { id: 1, uuid: "u1", email: "", first_name: "", last_name: "" }, + }), + ]); + expect(content).toEqual([{ github_login: "ada" }]); + }); + + it("falls back to user_uuid when there is no github_login", () => { + const content = toSuggestedReviewerWriteContent([ + makeReviewer({ + github_login: "", + user: { id: 1, uuid: "u1", email: "", first_name: "", last_name: "" }, + }), + ]); + expect(content).toEqual([{ user_uuid: "u1" }]); + }); + + it("drops entries with neither a login nor a resolved user", () => { + const content = toSuggestedReviewerWriteContent([ + makeReviewer({ github_login: "", user: null }), + ]); + expect(content).toEqual([]); + }); +}); + +describe("reviewerMatchesAvailable", () => { + it("matches on user uuid", () => { + const reviewer = makeReviewer({ + github_login: "", + user: { id: 1, uuid: "uuid-1", email: "", first_name: "", last_name: "" }, + }); + expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(true); + }); + + it("matches on case-insensitive github login", () => { + const reviewer = makeReviewer({ github_login: "ADA", user: null }); + expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(true); + }); + + it("does not match different people", () => { + const reviewer = makeReviewer({ github_login: "octocat", user: null }); + expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(false); + }); +}); + +describe("buildReviewerOptions", () => { + it("dedupes by uuid and pins the current user first", () => { + const options = buildReviewerOptions( + [ + makeAvailable({ uuid: "b", name: "Bob" }), + makeAvailable({ uuid: "a", name: "Ada" }), + makeAvailable({ uuid: "a", name: "Ada (dupe)" }), + ], + "b", + ); + expect(options.map((o) => o.uuid)).toEqual(["b", "a"]); + expect(options[0].isMe).toBe(true); + }); +}); diff --git a/apps/mobile/src/features/inbox/utils.ts b/apps/mobile/src/features/inbox/utils.ts index 2ac3de63ee..cfcd8aa373 100644 --- a/apps/mobile/src/features/inbox/utils.ts +++ b/apps/mobile/src/features/inbox/utils.ts @@ -1,8 +1,11 @@ import type { InboxViewedProperties } from "@/lib/analytics"; import type { + AvailableSuggestedReviewer, SignalReport, SignalReportOrderingField, SignalReportStatus, + SuggestedReviewer, + SuggestedReviewerWriteEntry, } from "./types"; export function inboxStatusLabel(status: SignalReportStatus): string { @@ -89,6 +92,81 @@ export function getActionableReports(reports: SignalReport[]): SignalReport[] { ); } +export interface ReviewerOption { + uuid: string; + name: string; + email: string; + github_login: string; + isMe: boolean; +} + +/** Deduplicate the available-reviewers list by uuid and sort "Me" first, then by name. */ +export function buildReviewerOptions( + reviewers: AvailableSuggestedReviewer[], + currentUserUuid: string | undefined, +): ReviewerOption[] { + const seen = new Set(); + const options: ReviewerOption[] = []; + + for (const r of reviewers) { + if (!r.uuid || seen.has(r.uuid)) continue; + seen.add(r.uuid); + options.push({ + uuid: r.uuid, + name: r.name?.trim() || "", + email: r.email?.trim() || "", + github_login: r.github_login?.trim() || "", + isMe: r.uuid === currentUserUuid, + }); + } + + options.sort((a, b) => { + if (a.isMe && !b.isMe) return -1; + if (!a.isMe && b.isMe) return 1; + return (a.name || a.email).localeCompare(b.name || b.email); + }); + + return options; +} + +export function reviewerOptionLabel(r: ReviewerOption): string { + const base = r.name || r.email || "Unknown user"; + return r.isMe ? `${base} (Me)` : base; +} + +/** A reviewer in the artefact matches an org member by user uuid or (case-insensitive) login. */ +export function reviewerMatchesAvailable( + reviewer: SuggestedReviewer, + available: AvailableSuggestedReviewer, +): boolean { + if (reviewer.user?.uuid && reviewer.user.uuid === available.uuid) { + return true; + } + return ( + !!reviewer.github_login && + !!available.github_login && + reviewer.github_login.toLowerCase() === available.github_login.toLowerCase() + ); +} + +/** + * Build the full-replacement write payload from a read-shape list. Kept reviewers + * are sent by `github_login` so the server preserves their commits/name; an entry + * with only a resolved user falls back to `user_uuid`. Entries with neither are + * dropped. + */ +export function toSuggestedReviewerWriteContent( + reviewers: SuggestedReviewer[], +): SuggestedReviewerWriteEntry[] { + return reviewers + .map((reviewer): SuggestedReviewerWriteEntry | null => { + if (reviewer.github_login) return { github_login: reviewer.github_login }; + if (reviewer.user?.uuid) return { user_uuid: reviewer.user.uuid }; + return null; + }) + .filter((entry): entry is SuggestedReviewerWriteEntry => entry !== null); +} + interface InboxViewedFilterState { sourceProductFilter: string[]; statusFilter: SignalReportStatus[]; diff --git a/apps/mobile/src/lib/analytics.ts b/apps/mobile/src/lib/analytics.ts index e9ccebdca2..45a149bcbb 100644 --- a/apps/mobile/src/lib/analytics.ts +++ b/apps/mobile/src/lib/analytics.ts @@ -43,6 +43,8 @@ export type InboxReportActionType = | "view_signal_external" | "expand_why" | "click_suggested_reviewer" + | "add_suggested_reviewer" + | "remove_suggested_reviewer" | "expand_task_section" | "play_session_recording"; @@ -131,6 +133,8 @@ export interface InboxReportActionProperties { task_section?: "research" | "implementation"; has_question?: boolean; question_text?: string; + suggested_reviewer_login?: string; + suggested_reviewer_uuid?: string; } export type EventPropertyMap = { From 473c358d0a3d088857d2f579a998e7ab577f539d Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Thu, 4 Jun 2026 11:56:04 +0100 Subject: [PATCH 2/2] refactor(mobile): guard add reviewer while pending, parameterise tests Address review feedback on the suggested reviewers UI: - Disable the "Add" button and short-circuit toggleReviewer while a mutation is in-flight so overlapping optimistic updates can't flicker or roll back to a stale snapshot. - Convert toSuggestedReviewerWriteContent and reviewerMatchesAvailable tests to it.each parameterised cases per project convention. Generated-By: PostHog Code Task-Id: 4b8dd76e-7ba9-453b-8cb8-c9441a47b3dc --- .../inbox/components/SuggestedReviewers.tsx | 4 +- apps/mobile/src/features/inbox/utils.test.ts | 80 +++++++++++-------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx b/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx index a1665b0ee7..4e8de6893c 100644 --- a/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx +++ b/apps/mobile/src/features/inbox/components/SuggestedReviewers.tsx @@ -75,6 +75,7 @@ export function SuggestedReviewers({ }; const toggleReviewer = (option: AvailableSuggestedReviewer) => { + if (isPending) return; const existing = reviewers.find((r) => reviewerMatchesAvailable(r, option)); if (existing) { removeReviewer(existing); @@ -119,9 +120,10 @@ export function SuggestedReviewers({ setEditOpen(true)} + disabled={isPending} accessibilityLabel="Add suggested reviewer" hitSlop={6} - className="flex-row items-center gap-1 rounded-full border border-gray-6 px-2.5 py-1 active:opacity-70" + className="flex-row items-center gap-1 rounded-full border border-gray-6 px-2.5 py-1 active:opacity-70 disabled:opacity-50" > Add diff --git a/apps/mobile/src/features/inbox/utils.test.ts b/apps/mobile/src/features/inbox/utils.test.ts index 7e4825270e..9663998ec6 100644 --- a/apps/mobile/src/features/inbox/utils.test.ts +++ b/apps/mobile/src/features/inbox/utils.test.ts @@ -171,51 +171,61 @@ describe("buildInboxViewedProperties", () => { }); describe("toSuggestedReviewerWriteContent", () => { - it("prefers github_login so the server preserves commits/name", () => { - const content = toSuggestedReviewerWriteContent([ - makeReviewer({ + it.each([ + { + name: "prefers github_login so the server preserves commits/name", + reviewer: makeReviewer({ github_login: "ada", user: { id: 1, uuid: "u1", email: "", first_name: "", last_name: "" }, }), - ]); - expect(content).toEqual([{ github_login: "ada" }]); - }); - - it("falls back to user_uuid when there is no github_login", () => { - const content = toSuggestedReviewerWriteContent([ - makeReviewer({ + expected: [{ github_login: "ada" }], + }, + { + name: "falls back to user_uuid when there is no github_login", + reviewer: makeReviewer({ github_login: "", user: { id: 1, uuid: "u1", email: "", first_name: "", last_name: "" }, }), - ]); - expect(content).toEqual([{ user_uuid: "u1" }]); - }); - - it("drops entries with neither a login nor a resolved user", () => { - const content = toSuggestedReviewerWriteContent([ - makeReviewer({ github_login: "", user: null }), - ]); - expect(content).toEqual([]); + expected: [{ user_uuid: "u1" }], + }, + { + name: "drops entries with neither a login nor a resolved user", + reviewer: makeReviewer({ github_login: "", user: null }), + expected: [], + }, + ])("$name", ({ reviewer, expected }) => { + expect(toSuggestedReviewerWriteContent([reviewer])).toEqual(expected); }); }); describe("reviewerMatchesAvailable", () => { - it("matches on user uuid", () => { - const reviewer = makeReviewer({ - github_login: "", - user: { id: 1, uuid: "uuid-1", email: "", first_name: "", last_name: "" }, - }); - expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(true); - }); - - it("matches on case-insensitive github login", () => { - const reviewer = makeReviewer({ github_login: "ADA", user: null }); - expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(true); - }); - - it("does not match different people", () => { - const reviewer = makeReviewer({ github_login: "octocat", user: null }); - expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(false); + it.each([ + { + name: "matches on user uuid", + reviewer: makeReviewer({ + github_login: "", + user: { + id: 1, + uuid: "uuid-1", + email: "", + first_name: "", + last_name: "", + }, + }), + expected: true, + }, + { + name: "matches on case-insensitive github login", + reviewer: makeReviewer({ github_login: "ADA", user: null }), + expected: true, + }, + { + name: "does not match different people", + reviewer: makeReviewer({ github_login: "octocat", user: null }), + expected: false, + }, + ])("$name", ({ reviewer, expected }) => { + expect(reviewerMatchesAvailable(reviewer, makeAvailable())).toBe(expected); }); });