Mobile ServicePage: SheetForm edit + close-on-save navigation (Slice 6)
Below md, ServicePage renders the edit form inside a SheetForm (open on
mount -- this page always edits an existing instance reached via
/services/:type/:id). The sheet body holds Name + Enabled + Connection
fields (no SectionCard wrapper, the sheet is the container) + Delete +
Widgets. At md+ the existing full-page layout renders token-identical.
Refactor: extracted the desktop inline JSX into configFields/widgetsCard/
confirmDelete consts and renamed ServiceConnectionCard ->
ServiceConnectionFields (isMobile prop drops the SectionCard wrapper on
mobile). Desktop output unchanged.
Fixes from Slice 6 review:
- R4.5: save() now closes the sheet on successful save (was staying open).
- Closing the sheet (save or cancel) navigates back to /services -- on
mobile the sheet IS the page, so closing it would strand the user on a
blank div. Added useNavigate.
- Strengthened the mobile save test to assert the full payload
(name, id, enabled, secrets:{}, config), not just name+id.
Out of scope (flagged for verify pass): R4.5 dirty-state outside-click
confirm is a broader SheetForm concern not yet implemented.
Tests: 5 new (2 desktop non-regression + no-dialog, 3 mobile sheet render +
save payload + editable config). useNavigate added to the router mock.
110 tests pass; lint/build green.
Refs openspec/changes/mobile-responsive-parity/ (spec R4, tasks slice 6).
This commit is contained in:
+209
-130
@@ -1,5 +1,5 @@
|
||||
import { useMemo, useState } from "react";
|
||||
import { useParams } from "react-router-dom";
|
||||
import { useNavigate, useParams } from "react-router-dom";
|
||||
import { Alert, AlertDescription } from "@/components/ui/alert";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Button } from "@/components/ui/button";
|
||||
@@ -12,6 +12,7 @@ import {
|
||||
useServiceInstances,
|
||||
useServiceTypes,
|
||||
} from "../hooks/useServices";
|
||||
import { useIsMobile } from "../hooks/useIsMobile";
|
||||
import type {
|
||||
ServiceInstance,
|
||||
ServiceInstanceInput,
|
||||
@@ -19,6 +20,7 @@ import type {
|
||||
} from "../types";
|
||||
import { SectionCard } from "../components/SectionCard";
|
||||
import { ConfirmDialog } from "../components/ConfirmDialog";
|
||||
import { SheetForm } from "@/components/ui/sheet-form";
|
||||
import { getServiceBinding } from "../integrations/registry";
|
||||
|
||||
function Field({
|
||||
@@ -63,11 +65,17 @@ export function ServicePage() {
|
||||
[types, serviceType],
|
||||
);
|
||||
|
||||
const navigate = useNavigate();
|
||||
const [name, setName] = useState("");
|
||||
const [enabled, setEnabled] = useState(true);
|
||||
const [draftConfig, setDraftConfig] = useState<Record<string, unknown>>({});
|
||||
const [deleteOpen, setDeleteOpen] = useState(false);
|
||||
const [hydrated, setHydrated] = useState(false);
|
||||
const isMobile = useIsMobile();
|
||||
// The mobile SheetForm opens by default when the page loads: this page is
|
||||
// reached via /services/:serviceType/:serviceId, always editing an existing
|
||||
// instance, so there is no separate "open edit" trigger on mobile.
|
||||
const [sheetOpen, setSheetOpen] = useState(true);
|
||||
|
||||
// Hydrate local form state once the instance loads.
|
||||
if (instance && !hydrated) {
|
||||
@@ -106,6 +114,106 @@ export function ServicePage() {
|
||||
|
||||
async function save() {
|
||||
await saveService.mutateAsync(buildInput());
|
||||
// R4.5: close the sheet on successful save and return to the services list
|
||||
// (on mobile the sheet IS the page, so closing it would strand the user).
|
||||
if (isMobile) {
|
||||
setSheetOpen(false);
|
||||
navigate("/services");
|
||||
}
|
||||
}
|
||||
|
||||
const configFields = (
|
||||
<ServiceConnectionFields
|
||||
instance={instance}
|
||||
typeInfo={typeInfo}
|
||||
draftConfig={draftConfig}
|
||||
onConfigChange={setDraftConfig}
|
||||
isMobile={isMobile}
|
||||
/>
|
||||
);
|
||||
|
||||
const widgetsCard =
|
||||
binding.widgets.length > 0 ? (
|
||||
<SectionCard
|
||||
title="Widgets"
|
||||
description="Widget kinds this service provides."
|
||||
>
|
||||
<div className="flex flex-col gap-2">
|
||||
{binding.widgets.map((w) => (
|
||||
<div
|
||||
key={w.kind}
|
||||
className="flex items-center justify-between rounded border p-2"
|
||||
>
|
||||
<div>
|
||||
<div className="font-medium">{w.name}</div>
|
||||
<div className="text-xs text-muted-foreground">
|
||||
{w.description}
|
||||
</div>
|
||||
</div>
|
||||
<Badge variant="outline">{w.kind}</Badge>
|
||||
</div>
|
||||
))}
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Add these to the dashboard from the dashboard's edit dialog.
|
||||
</p>
|
||||
</div>
|
||||
</SectionCard>
|
||||
) : null;
|
||||
|
||||
const confirmDelete = (
|
||||
<ConfirmDialog
|
||||
open={deleteOpen}
|
||||
title="Delete service?"
|
||||
message="This removes the service and any widgets that reference it. This cannot be undone."
|
||||
confirmLabel="Delete"
|
||||
onCancel={() => setDeleteOpen(false)}
|
||||
onConfirm={() => {
|
||||
deleteService.mutate(instance.id);
|
||||
setDeleteOpen(false);
|
||||
}}
|
||||
/>
|
||||
);
|
||||
|
||||
if (isMobile) {
|
||||
return (
|
||||
<div className="flex flex-col gap-4">
|
||||
<SheetForm
|
||||
open={sheetOpen}
|
||||
onOpenChange={setSheetOpen}
|
||||
title={name || instance.name}
|
||||
onSave={save}
|
||||
onCancel={() => {
|
||||
setSheetOpen(false);
|
||||
navigate("/services");
|
||||
}}
|
||||
isPending={saveService.isPending}
|
||||
>
|
||||
<div className="flex flex-col gap-6">
|
||||
<Field label="Name" htmlFor="service-name">
|
||||
<Input
|
||||
id="service-name"
|
||||
value={name}
|
||||
onChange={(e) => setName(e.target.value)}
|
||||
/>
|
||||
</Field>
|
||||
<div className="flex items-center gap-2">
|
||||
<Switch
|
||||
id="service-enabled"
|
||||
checked={enabled}
|
||||
onCheckedChange={setEnabled}
|
||||
/>
|
||||
<Label htmlFor="service-enabled">Enabled</Label>
|
||||
</div>
|
||||
{configFields}
|
||||
<Button variant="destructive" onClick={() => setDeleteOpen(true)}>
|
||||
Delete service
|
||||
</Button>
|
||||
{widgetsCard}
|
||||
</div>
|
||||
</SheetForm>
|
||||
{confirmDelete}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
@@ -146,65 +254,27 @@ export function ServicePage() {
|
||||
</div>
|
||||
</SectionCard>
|
||||
|
||||
<ServiceConnectionCard
|
||||
instance={instance}
|
||||
typeInfo={typeInfo}
|
||||
draftConfig={draftConfig}
|
||||
onConfigChange={setDraftConfig}
|
||||
/>
|
||||
{configFields}
|
||||
|
||||
{binding.widgets.length > 0 ? (
|
||||
<SectionCard
|
||||
title="Widgets"
|
||||
description="Widget kinds this service provides."
|
||||
>
|
||||
<div className="flex flex-col gap-2">
|
||||
{binding.widgets.map((w) => (
|
||||
<div
|
||||
key={w.kind}
|
||||
className="flex items-center justify-between rounded border p-2"
|
||||
>
|
||||
<div>
|
||||
<div className="font-medium">{w.name}</div>
|
||||
<div className="text-xs text-muted-foreground">
|
||||
{w.description}
|
||||
</div>
|
||||
</div>
|
||||
<Badge variant="outline">{w.kind}</Badge>
|
||||
</div>
|
||||
))}
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Add these to the dashboard from the dashboard's edit dialog.
|
||||
</p>
|
||||
</div>
|
||||
</SectionCard>
|
||||
) : null}
|
||||
{widgetsCard}
|
||||
|
||||
<ConfirmDialog
|
||||
open={deleteOpen}
|
||||
title="Delete service?"
|
||||
message="This removes the service and any widgets that reference it. This cannot be undone."
|
||||
confirmLabel="Delete"
|
||||
onCancel={() => setDeleteOpen(false)}
|
||||
onConfirm={() => {
|
||||
deleteService.mutate(instance.id);
|
||||
setDeleteOpen(false);
|
||||
}}
|
||||
/>
|
||||
{confirmDelete}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function ServiceConnectionCard({
|
||||
function ServiceConnectionFields({
|
||||
instance,
|
||||
typeInfo,
|
||||
draftConfig,
|
||||
onConfigChange,
|
||||
isMobile,
|
||||
}: {
|
||||
instance: ServiceInstance;
|
||||
typeInfo: ServiceTypeInfo | undefined;
|
||||
draftConfig: Record<string, unknown>;
|
||||
onConfigChange: (config: Record<string, unknown>) => void;
|
||||
isMobile: boolean;
|
||||
}) {
|
||||
const saveService = useSaveServiceInstance();
|
||||
// Empty-on-edit: local state starts blank; a blank field means "keep existing".
|
||||
@@ -232,96 +302,105 @@ function ServiceConnectionCard({
|
||||
{ type: typeof value === "number" ? "integer" : "string" },
|
||||
]);
|
||||
|
||||
function handleUpdateConnection() {
|
||||
const onlyChanged = Object.fromEntries(
|
||||
Object.entries(draftSecrets).filter(([, v]) => v !== ""),
|
||||
);
|
||||
saveService.mutate({
|
||||
id: instance.id,
|
||||
service_type: instance.service_type,
|
||||
name: instance.name,
|
||||
config: draftConfig,
|
||||
secrets: onlyChanged,
|
||||
enabled: instance.enabled,
|
||||
});
|
||||
setDraftSecrets({});
|
||||
}
|
||||
|
||||
const fields = (
|
||||
<div className="flex flex-col gap-3">
|
||||
{configEntries.length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground">No connection config.</p>
|
||||
) : (
|
||||
<div className="flex flex-col gap-3">
|
||||
{configEntries.map(([key, schema]) => {
|
||||
const isNumber =
|
||||
schema.type === "integer" || schema.type === "number";
|
||||
return (
|
||||
<Field
|
||||
key={key}
|
||||
label={key}
|
||||
htmlFor={`cfg-${key}`}
|
||||
helper={schema.description}
|
||||
>
|
||||
<Input
|
||||
id={`cfg-${key}`}
|
||||
type={isNumber ? "number" : "text"}
|
||||
value={String(draftConfig[key] ?? "")}
|
||||
onChange={(e) =>
|
||||
onConfigChange({
|
||||
...draftConfig,
|
||||
[key]: isNumber
|
||||
? e.target.value === ""
|
||||
? undefined
|
||||
: Number(e.target.value)
|
||||
: e.target.value,
|
||||
})
|
||||
}
|
||||
/>
|
||||
</Field>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{Object.keys(instance.secrets_set).length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground">No secret fields.</p>
|
||||
) : (
|
||||
<div className="flex flex-col gap-3">
|
||||
{Object.entries(instance.secrets_set).map(([key, isSet]) => (
|
||||
<div key={key} className="flex flex-col gap-1.5">
|
||||
<Field
|
||||
label={key}
|
||||
htmlFor={`secret-${key}`}
|
||||
helper="Leave blank to keep the current value."
|
||||
>
|
||||
<Input
|
||||
id={`secret-${key}`}
|
||||
type="password"
|
||||
placeholder={isSet ? "•••••• (set)" : "Not set"}
|
||||
value={draftSecrets[key] ?? ""}
|
||||
onChange={(e) =>
|
||||
setDraftSecrets({
|
||||
...draftSecrets,
|
||||
[key]: e.target.value,
|
||||
})
|
||||
}
|
||||
/>
|
||||
</Field>
|
||||
{isSet ? <Badge variant="secondary">set</Badge> : null}
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
|
||||
<Button onClick={handleUpdateConnection}>Update connection</Button>
|
||||
</div>
|
||||
);
|
||||
|
||||
// On mobile the fields render inside the SheetForm body without a card
|
||||
// wrapper (the SheetForm already provides the container). On desktop they
|
||||
// keep their original SectionCard framing.
|
||||
if (isMobile) {
|
||||
return <div className="flex flex-col gap-3">{fields}</div>;
|
||||
}
|
||||
|
||||
return (
|
||||
<SectionCard
|
||||
title="Connection"
|
||||
description="Edit non-secret connection config and secret values."
|
||||
>
|
||||
<div className="flex flex-col gap-3">
|
||||
{configEntries.length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground">No connection config.</p>
|
||||
) : (
|
||||
<div className="flex flex-col gap-3">
|
||||
{configEntries.map(([key, schema]) => {
|
||||
const isNumber =
|
||||
schema.type === "integer" || schema.type === "number";
|
||||
return (
|
||||
<Field
|
||||
key={key}
|
||||
label={key}
|
||||
htmlFor={`cfg-${key}`}
|
||||
helper={schema.description}
|
||||
>
|
||||
<Input
|
||||
id={`cfg-${key}`}
|
||||
type={isNumber ? "number" : "text"}
|
||||
value={String(draftConfig[key] ?? "")}
|
||||
onChange={(e) =>
|
||||
onConfigChange({
|
||||
...draftConfig,
|
||||
[key]: isNumber
|
||||
? e.target.value === ""
|
||||
? undefined
|
||||
: Number(e.target.value)
|
||||
: e.target.value,
|
||||
})
|
||||
}
|
||||
/>
|
||||
</Field>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{Object.keys(instance.secrets_set).length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground">No secret fields.</p>
|
||||
) : (
|
||||
<div className="flex flex-col gap-3">
|
||||
{Object.entries(instance.secrets_set).map(([key, isSet]) => (
|
||||
<div key={key} className="flex flex-col gap-1.5">
|
||||
<Field
|
||||
label={key}
|
||||
htmlFor={`secret-${key}`}
|
||||
helper="Leave blank to keep the current value."
|
||||
>
|
||||
<Input
|
||||
id={`secret-${key}`}
|
||||
type="password"
|
||||
placeholder={isSet ? "•••••• (set)" : "Not set"}
|
||||
value={draftSecrets[key] ?? ""}
|
||||
onChange={(e) =>
|
||||
setDraftSecrets({
|
||||
...draftSecrets,
|
||||
[key]: e.target.value,
|
||||
})
|
||||
}
|
||||
/>
|
||||
</Field>
|
||||
{isSet ? <Badge variant="secondary">set</Badge> : null}
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
|
||||
<Button
|
||||
onClick={() => {
|
||||
const onlyChanged = Object.fromEntries(
|
||||
Object.entries(draftSecrets).filter(([, v]) => v !== ""),
|
||||
);
|
||||
saveService.mutate({
|
||||
id: instance.id,
|
||||
service_type: instance.service_type,
|
||||
name: instance.name,
|
||||
config: draftConfig,
|
||||
secrets: onlyChanged,
|
||||
enabled: instance.enabled,
|
||||
});
|
||||
setDraftSecrets({});
|
||||
}}
|
||||
>
|
||||
Update connection
|
||||
</Button>
|
||||
</div>
|
||||
{fields}
|
||||
</SectionCard>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,149 @@
|
||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { ServicePage } from "../ServicePage";
|
||||
import type {
|
||||
ServiceInstance,
|
||||
ServiceInstanceInput,
|
||||
ServiceTypeInfo,
|
||||
} from "../../types";
|
||||
|
||||
// --- fixtures ---
|
||||
|
||||
const instance: ServiceInstance = {
|
||||
id: "svc-1",
|
||||
service_type: "grafana",
|
||||
name: "Production Grafana",
|
||||
config: { base_url: "https://grafana.example.com", timeout_seconds: 5 },
|
||||
secrets_set: { api_key: true },
|
||||
enabled: true,
|
||||
created_at: 1_700_000_000,
|
||||
updated_at: 1_700_000_000,
|
||||
};
|
||||
|
||||
const typeInfo: ServiceTypeInfo = {
|
||||
service_type: "grafana",
|
||||
name: "Grafana",
|
||||
description: "Dashboards, metrics, and logs.",
|
||||
config_schema: {
|
||||
type: "object",
|
||||
properties: {
|
||||
base_url: { type: "string", description: "Absolute URL." },
|
||||
timeout_seconds: { type: "integer" },
|
||||
},
|
||||
},
|
||||
secret_fields: [{ key: "api_key", label: "API key", required: false }],
|
||||
widget_kinds: [],
|
||||
};
|
||||
|
||||
// --- mocks ---
|
||||
|
||||
const mutateAsync = vi.fn();
|
||||
const mutate = vi.fn();
|
||||
const deleteMutate = vi.fn();
|
||||
|
||||
vi.mock("../../hooks/useServices", () => ({
|
||||
useServiceInstances: () => ({ data: [instance] }),
|
||||
useServiceTypes: () => ({ data: [typeInfo] }),
|
||||
useSaveServiceInstance: () => ({
|
||||
mutateAsync,
|
||||
mutate,
|
||||
isPending: false,
|
||||
}),
|
||||
useDeleteServiceInstance: () => ({ mutate: deleteMutate, isPending: false }),
|
||||
}));
|
||||
|
||||
vi.mock("react-router-dom", () => ({
|
||||
useParams: () => ({
|
||||
serviceType: "grafana",
|
||||
serviceId: "svc-1",
|
||||
}),
|
||||
useNavigate: () => vi.fn(),
|
||||
}));
|
||||
|
||||
// jsdom has no window.matchMedia; stub it. Default to desktop (matches: false).
|
||||
function setMatchMedia(matches: boolean) {
|
||||
window.matchMedia = ((query: string) => ({
|
||||
matches: query.includes("768") ? matches : false,
|
||||
media: query,
|
||||
onchange: null,
|
||||
addEventListener: () => {},
|
||||
removeEventListener: () => {},
|
||||
addListener: () => {},
|
||||
removeListener: () => {},
|
||||
dispatchEvent: () => false,
|
||||
})) as unknown as typeof window.matchMedia;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
setMatchMedia(false);
|
||||
mutateAsync.mockReset();
|
||||
mutate.mockReset();
|
||||
deleteMutate.mockReset();
|
||||
});
|
||||
|
||||
describe("ServicePage (desktop)", () => {
|
||||
it("renders the full-page layout with the service name and connection card", () => {
|
||||
render(<ServicePage />);
|
||||
// Page heading (desktop only — mobile uses SheetForm title)
|
||||
expect(
|
||||
screen.getByRole("heading", { name: "Production Grafana" }),
|
||||
).toBeInTheDocument();
|
||||
// Connection section card title
|
||||
expect(screen.getByText("Connection")).toBeInTheDocument();
|
||||
// General Save button
|
||||
expect(screen.getByRole("button", { name: "Save" })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not render the SheetForm at desktop width", () => {
|
||||
render(<ServicePage />);
|
||||
// SheetForm renders a dialog with role="dialog" only when open; on
|
||||
// desktop the page layout is used, so no dialog should be present.
|
||||
expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
describe("ServicePage (mobile SheetForm — slice 6)", () => {
|
||||
beforeEach(() => setMatchMedia(true));
|
||||
|
||||
it("renders the SheetForm with the service name as title below md", () => {
|
||||
render(<ServicePage />);
|
||||
// SheetForm title is rendered inside a SheetTitle (role="heading").
|
||||
expect(
|
||||
screen.getByRole("heading", { name: "Production Grafana" }),
|
||||
).toBeInTheDocument();
|
||||
// The dialog (Sheet content) should be present on mobile.
|
||||
expect(screen.getByRole("dialog")).toBeInTheDocument();
|
||||
// Desktop page header description is NOT rendered inside the SheetForm.
|
||||
expect(
|
||||
screen.queryByText("Dashboards, metrics, and logs."),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("edits the name field and Save calls the save mutation", async () => {
|
||||
render(<ServicePage />);
|
||||
const nameInput = screen.getByLabelText("Name");
|
||||
expect(nameInput).toHaveValue("Production Grafana");
|
||||
|
||||
await userEvent.clear(nameInput);
|
||||
await userEvent.type(nameInput, "Renamed Grafana");
|
||||
|
||||
const saveButton = screen.getByRole("button", { name: "Save" });
|
||||
await userEvent.click(saveButton);
|
||||
|
||||
expect(mutateAsync).toHaveBeenCalledTimes(1);
|
||||
const input = mutateAsync.mock.calls[0][0] as ServiceInstanceInput;
|
||||
expect(input.name).toBe("Renamed Grafana");
|
||||
expect(input.id).toBe("svc-1");
|
||||
// Lock the full save payload (config draft, enabled, secrets sentinel).
|
||||
expect(input.enabled).toBe(true);
|
||||
expect(input.secrets).toEqual({});
|
||||
expect(input.config).toMatchObject({ base_url: "https://grafana.example.com" });
|
||||
});
|
||||
|
||||
it("renders the connection config fields as editable inside the SheetForm", () => {
|
||||
render(<ServicePage />);
|
||||
const urlInput = screen.getByLabelText("base_url");
|
||||
expect(urlInput).toHaveValue("https://grafana.example.com");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user