Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ describe("CreateSuitePage", () => {

expect(screen.getByTestId("create-suite-page")).toBeTruthy();
expect(screen.queryByRole("dialog")).toBeNull();
expect(screen.getByTestId("required-legend")).toHaveTextContent(
"Required field",
);
expect(
screen.getByRole("heading", { name: "Create a new eval suite" }),
).toBeTruthy();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ import { useComposerResolver } from "@/components/environment-composer/use-compo
import { MAX_SUITE_ENVIRONMENTS } from "@/components/project-environments/environment-picker";
import { useProjectEnvironmentsEnabled } from "@/hooks/useProjectEnvironmentsEnabled";
import { useProjectEnvironments } from "@/hooks/useProjectEnvironments";
import { RequiredMark } from "@/components/shared/required-mark";
import {
RequiredLegend,
RequiredMark,
} from "@/components/shared/required-mark";
import { toast } from "@/lib/toast";
import type { HostAttachmentDraft } from "../evals/client-attachments-editor";
import {
Expand Down Expand Up @@ -371,6 +374,7 @@ export function CreateSuitePage({
<p className="text-sm leading-relaxed text-muted-foreground">
Set up the environment you will be evaluating.
</p>
<RequiredLegend />
</div>

<div className="space-y-6">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
DropdownMenuTrigger,
} from "@mcpjam/design-system/dropdown-menu";
import { EnvironmentComposer } from "@/components/environment-composer/environment-composer";
import { RequiredMark } from "@/components/shared/required-mark";
import {
RequiredLegend,
RequiredMark,
} from "@/components/shared/required-mark";
import {
emptyComposerState,
isComposeMode,
Expand Down Expand Up @@ -345,6 +348,9 @@ export function UserTestingScenarioCreateFlow({
Publish one of your environments, hand them to users, then read what
happened in their sessions.
</p>
<div className="mt-2">
<RequiredLegend />
</div>

<div className="mt-6 space-y-5">
<div className="space-y-2">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,9 @@ describe("UserTestingScenarioCreateFlow", () => {
// Marked on the label too — an asterisk is what a scanning user reads as
// "required" before they try Save.
expect(screen.getAllByText("(required)").length).toBeGreaterThan(0);
expect(screen.getByTestId("required-legend")).toHaveTextContent(
"Required field",
);

fireEvent.change(screen.getByTestId("user-testing-create-environment"), {
target: { value: "env-1" },
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { act, fireEvent, render, screen } from "@testing-library/react";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { RequiredLegend, RequiredMark } from "../required-mark";

describe("RequiredMark", () => {
beforeEach(() => {
vi.useFakeTimers();
});

afterEach(() => {
vi.useRealTimers();
});

it("hides the glyph from assistive tech and names the field required", () => {
render(
<label>
Swarm name
<RequiredMark />
</label>,
);

const glyph = screen.getByTestId("required-mark");
expect(glyph).toHaveTextContent("*");
expect(glyph).toHaveAttribute("aria-hidden", "true");
expect(screen.getByText("(required)")).toHaveClass("sr-only");
});

it("explains itself on hover", () => {
render(
<label>
Swarm name
<RequiredMark />
</label>,
);

expect(screen.queryByRole("tooltip")).not.toBeInTheDocument();

const glyph = screen.getByTestId("required-mark");
act(() => {
fireEvent.pointerMove(glyph, { pointerType: "mouse" });
vi.runAllTimers();
});

expect(screen.getByRole("tooltip")).toHaveTextContent("Required");
});
});

describe("RequiredLegend", () => {
it("spells out what the mark means", () => {
render(<RequiredLegend />);

expect(screen.getByTestId("required-legend")).toHaveTextContent(
"* Required field",
);
});
});
41 changes: 37 additions & 4 deletions mcpjam-inspector/client/src/components/shared/required-mark.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
import {
Tooltip,
TooltipContent,
TooltipTrigger,
} from "@mcpjam/design-system/tooltip";

/**
* The marker that makes a required field LOOK required, for forms whose Save is
* already gated on the field being filled. A gate nobody can see reads as a
Expand All @@ -6,7 +12,8 @@
*
* The glyph is decorative: assistive tech gets the word, and the control itself
* still has to carry `aria-required` — this marks the LABEL, it does not
* annotate the input.
* annotate the input. Hovering the glyph names it for everyone else — a bare
* asterisk with no footnote to point at reads as a typo.
*
* Primary, not destructive: every Production Redesign frame draws the marker in
* the brand colour, and red on a field nobody has touched yet reads as an error
Expand All @@ -15,10 +22,36 @@
export function RequiredMark() {
return (
<>
<span aria-hidden="true" className="font-medium text-primary">
*
</span>
<Tooltip>
<TooltipTrigger asChild>
<span
aria-hidden="true"
className="cursor-help font-medium text-primary"
data-testid="required-mark"
>
*
</span>
</TooltipTrigger>
<TooltipContent side="top" variant="muted">
Required
</TooltipContent>
</Tooltip>
<span className="sr-only">(required)</span>
Comment on lines +33 to 39

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 Keyboard misses required-field explanation

On forms without the legend, keyboard navigation cannot focus the TooltipTrigger span. The required-field explanation remains unavailable.

Prompt for agents
Make RequiredMark's explanation available to sighted keyboard users without exposing duplicate or misleading text to assistive technology. The current TooltipTrigger in mcpjam-inspector/client/src/components/shared/required-mark.tsx wraps an aria-hidden, non-focusable span, so Radix can only open it through pointer hover. Account for the existing sr-only “(required)” text and the fact that most RequiredMark call sites do not render RequiredLegend. Add keyboard-focused test coverage as well as the existing hover coverage.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deliberate trade-off rather than an oversight: making the glyph focusable adds an extra tab stop after every required label (4 on the Swarm form alone), which is worse for keyboard users than the missing hover text. Screen-reader users already get "(required)" via the sr-only span. The legend is the keyboard-visible explanation; extending it to the other forms that use RequiredMark is the cleaner fix, and I've asked Sophie whether she wants that.

</>
);
}

/**
* The legend a form shows once, near the top, so the marks below it need no
* guessing. Only for forms that render at least one {@link RequiredMark}.
*/
export function RequiredLegend() {
return (
<p className="text-xs text-muted-foreground" data-testid="required-legend">
<span aria-hidden="true" className="font-medium text-primary">
*
</span>{" "}
Required field
</p>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,9 @@ describe("SwarmsTab — New swarm create flow", () => {
openDescribe();

expect(screen.getByTestId("new-swarm-create-flow")).toBeInTheDocument();
expect(screen.getByTestId("required-legend")).toHaveTextContent(
"Required field",
);
expect(
screen.getByRole("heading", {
name: /create an agentic swarm/i,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import { Textarea } from "@mcpjam/design-system/textarea";
import { ChevronLeft, Loader2, X } from "lucide-react";
import { PersonaPickerPopover } from "@/components/swarms/persona-picker-popover";
import { ProgressStepper } from "@/components/shared/progress-stepper";
import { RequiredMark } from "@/components/shared/required-mark";
import {
RequiredLegend,
RequiredMark,
} from "@/components/shared/required-mark";
import { ErrorBoundary } from "@/components/ui/error-boundary";
import { SwarmTargetComposer } from "@/components/swarms/swarm-target-composer";
import {
Expand Down Expand Up @@ -1798,6 +1801,7 @@ export function NewSwarmCreateFlow({
<p className="text-sm font-medium leading-relaxed text-foreground">
Set up your environment and then describe your users.
</p>
<RequiredLegend />
</div>

<div className="space-y-2">
Expand Down
Loading