Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions apps/mobile/src/app/inbox/[...id].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
formatSignalReportSummaryMarkdown,
inboxStatusLabel,
} from "@/features/inbox/utils";
import { PrStatusBadge } from "@/features/tasks/components/PrStatusBadge";
import {
computeReportAgeHours,
type InboxReportActionType,
Expand Down Expand Up @@ -472,6 +473,15 @@ export default function ReportDetailScreen() {
</Text>
</View>
<Text className="text-[12px] text-gray-9">Updated {timeDisplay}</Text>
{report.implementation_pr_url ? (
<View className="ml-auto">
<PrStatusBadge
prUrl={report.implementation_pr_url}
hideWhenUnresolved
size="sm"
/>
</View>
) : null}
</View>

{/* Failed warning */}
Expand Down
11 changes: 11 additions & 0 deletions apps/mobile/src/features/inbox/components/ReportListRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Text } from "@components/text";
import { differenceInHours, format, formatDistanceToNow } from "date-fns";
import { memo } from "react";
import { Pressable, View } from "react-native";
import { PrStatusBadge } from "@/features/tasks/components/PrStatusBadge";
import { useThemeColors } from "@/lib/theme";
import type { SignalReport } from "../types";

Expand Down Expand Up @@ -88,6 +89,16 @@ function ReportListRowComponent({ report, onPress }: ReportListRowProps) {
</Text>
</View>
</View>

{report.implementation_pr_url ? (
<View className="self-center">
<PrStatusBadge
prUrl={report.implementation_pr_url}
hideWhenUnresolved
size="sm"
/>
</View>
) : null}
</Pressable>
);
}
Expand Down
10 changes: 10 additions & 0 deletions apps/mobile/src/features/inbox/components/SwipeableReportCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { GithubLogo, Lightning } from "phosphor-react-native";
import { useRef } from "react";
import { Animated, PanResponder, Pressable, View } from "react-native";
import { MarkdownText } from "@/features/chat/components/MarkdownText";
import { PrStatusBadge } from "@/features/tasks/components/PrStatusBadge";
import { useThemeColors } from "@/lib/theme";
import type {
SignalReport,
Expand Down Expand Up @@ -290,6 +291,15 @@ function CardContent({
</View>
</>
)}
{report.implementation_pr_url ? (
<View className="ml-auto">
<PrStatusBadge
prUrl={report.implementation_pr_url}
hideWhenUnresolved
size="sm"
/>
</View>
) : null}
</View>
</View>
);
Expand Down
111 changes: 111 additions & 0 deletions apps/mobile/src/features/tasks/components/PrStatusBadge.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import { createElement } from "react";
import { act, create } from "react-test-renderer";
import { describe, expect, it, vi } from "vitest";
import type { PrStatus } from "../hooks/usePrStatus";
import { PrStatusBadge } from "./PrStatusBadge";

vi.mock("phosphor-react-native", () => ({
GitMerge: (props: Record<string, unknown>) =>
createElement("GitMerge", props),
GitPullRequest: (props: Record<string, unknown>) =>
createElement("GitPullRequest", props),
}));

vi.mock("@/lib/theme", () => ({
useThemeColors: () => ({
gray: { 11: "#444444" },
status: { success: "#00aa00", error: "#cc0000" },
}),
toRgba: (hex: string, alpha: number) => `${hex}/${alpha}`,
}));

vi.mock("@/lib/openExternalUrl", () => ({ openExternalUrl: vi.fn() }));

vi.mock("../hooks/usePrStatus", () => ({ usePrStatus: vi.fn() }));

import { usePrStatus } from "../hooks/usePrStatus";

const mockUsePrStatus = vi.mocked(usePrStatus);

function setStatus(data: PrStatus | null | undefined) {
mockUsePrStatus.mockReturnValue({ data } as ReturnType<typeof usePrStatus>);
}

function render(props: Parameters<typeof PrStatusBadge>[0]) {
let renderer: ReturnType<typeof create> | null = null;
act(() => {
renderer = create(createElement(PrStatusBadge, props));
});
if (!renderer) throw new Error("Renderer not created");
return renderer as ReturnType<typeof create>;
}

function label(renderer: ReturnType<typeof create>): string | undefined {
const node = renderer.root.findAll(
(n) => typeof n.props?.accessibilityLabel === "string",
)[0];
return node?.props.accessibilityLabel as string | undefined;
}

function iconCount(renderer: ReturnType<typeof create>, type: string): number {
return renderer.root.findAll((n) => n.type === type).length;
}

const base: PrStatus = {
state: "open",
merged: false,
draft: false,
additions: 0,
deletions: 0,
};

describe("PrStatusBadge", () => {
it("renders an open PR badge", () => {
setStatus({ ...base, state: "open" });
const r = render({ prUrl: "https://github.com/a/b/pull/1" });
expect(label(r)).toBe("Open PR");
expect(iconCount(r, "GitPullRequest")).toBe(1);
});

it("renders a merged PR badge with the merge icon", () => {
setStatus({ ...base, state: "closed", merged: true });
const r = render({ prUrl: "https://github.com/a/b/pull/1" });
expect(label(r)).toBe("Open merged PR");
expect(iconCount(r, "GitMerge")).toBe(1);
expect(iconCount(r, "GitPullRequest")).toBe(0);
});

it("renders a closed PR badge", () => {
setStatus({ ...base, state: "closed" });
const r = render({ prUrl: "https://github.com/a/b/pull/1" });
expect(label(r)).toBe("Open closed PR");
});

it("renders a draft PR badge", () => {
setStatus({ ...base, state: "open", draft: true });
const r = render({ prUrl: "https://github.com/a/b/pull/1" });
expect(label(r)).toBe("Open draft PR");
Comment on lines +55 to +87

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.

P2 Non-parameterised state rendering tests

The four individual state cases (open, merged, closed, draft) repeat the same structure — call setStatus, render with an identical prUrl, assert on label(). Per the team's convention we always prefer parameterised tests; these are a natural fit for it.each. The merged case has one extra icon assertion that can be captured as an optional field in the test-case object (e.g. expectedIcon?: string defaulting to "GitPullRequest"), keeping the whole suite in one table.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/src/features/tasks/components/PrStatusBadge.test.tsx
Line: 55-87

Comment:
**Non-parameterised state rendering tests**

The four individual state cases (open, merged, closed, draft) repeat the same structure — call `setStatus`, render with an identical `prUrl`, assert on `label()`. Per the team's convention we always prefer parameterised tests; these are a natural fit for `it.each`. The merged case has one extra icon assertion that can be captured as an optional field in the test-case object (e.g. `expectedIcon?: string` defaulting to `"GitPullRequest"`), keeping the whole suite in one table.

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

});

it.each([
{ data: undefined, label: "loading" },
{ data: null, label: "unresolved (private/404/non-GitHub)" },
])(
"renders nothing when hideWhenUnresolved is set and status is $label",
({ data }) => {
setStatus(data);
const r = render({
prUrl: "https://github.com/a/b/pull/1",
hideWhenUnresolved: true,
});
expect(r.toJSON()).toBeNull();
},
);

it("still renders a neutral badge for an unresolved PR by default", () => {
setStatus(null);
const r = render({ prUrl: "https://github.com/a/b/pull/1" });
expect(r.toJSON()).not.toBeNull();
expect(label(r)).toBe("Open PR");
});
});
Comment on lines +55 to +111

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.

P2 New size prop is untested

The PR adds a size prop with distinct box class and iconSize values for "sm" vs "md", but no test asserts on either. The iconSize logic (16 vs 20) is passed as a prop to the icon, so it is observable in the rendered tree. Without a test, a future refactor could silently regress the compact badge on list rows.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/src/features/tasks/components/PrStatusBadge.test.tsx
Line: 55-111

Comment:
**New `size` prop is untested**

The PR adds a `size` prop with distinct `box` class and `iconSize` values for `"sm"` vs `"md"`, but no test asserts on either. The `iconSize` logic (`16` vs `20`) is passed as a prop to the icon, so it is observable in the rendered tree. Without a test, a future refactor could silently regress the compact badge on list rows.

How can I resolve this? If you propose a fix, please make it concise.

20 changes: 17 additions & 3 deletions apps/mobile/src/features/tasks/components/PrStatusBadge.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,28 @@ import { usePrStatus } from "../hooks/usePrStatus";

interface PrStatusBadgeProps {
prUrl: string;
// Render nothing until the PR state resolves, and only for a canonical
// GitHub PR URL. Inbox surfaces use this so an always-on neutral icon never
// implies a status we couldn't confirm (private repo, 404, unparseable URL).
hideWhenUnresolved?: boolean;
size?: "sm" | "md";
}

// Mirrors the desktop "merged" PR color (Radix purple-9 family). Theme tokens
// don't include a purple, and merged-PR purple is recognisable enough that a
// fixed value works in both light and dark.
const MERGED_COLOR = "#8e4ec6";

export function PrStatusBadge({ prUrl }: PrStatusBadgeProps) {
export function PrStatusBadge({
prUrl,
hideWhenUnresolved = false,
size = "md",
}: PrStatusBadgeProps) {
const themeColors = useThemeColors();
const { data: status } = usePrStatus(prUrl);

if (hideWhenUnresolved && !status) return null;

const handlePress = () => {
openExternalUrl(prUrl);
};
Expand All @@ -40,19 +51,22 @@ export function PrStatusBadge({ prUrl }: PrStatusBadgeProps) {
label = "Open PR";
}

const box = size === "sm" ? "h-7 w-7" : "h-9 w-9";
const iconSize = size === "sm" ? 16 : 20;

return (
<Pressable
onPress={handlePress}
hitSlop={10}
className="h-9 w-9 items-center justify-center rounded-lg border active:opacity-60"
className={`${box} items-center justify-center rounded-lg border active:opacity-60`}
style={{
backgroundColor: toRgba(color, 0.12),
borderColor: toRgba(color, 0.35),
}}
accessibilityRole="link"
accessibilityLabel={label}
>
<Icon size={20} weight="bold" color={color} />
<Icon size={iconSize} weight="bold" color={color} />
</Pressable>
);
}
Loading