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
6 changes: 6 additions & 0 deletions .server-changes/fix-project-settings-toast.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

Renaming a project now keeps you on the project settings page and tells you what happened, instead of silently moving you to the tasks page or clearing the form with no explanation.
Comment thread
claude[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { resolveOrgIdFromSlug } from "~/models/organization.server";
import { ProjectSettingsService } from "~/services/projectSettings.server";
import { logger } from "~/services/logger.server";
import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder";
import { organizationPath, v3ProjectPath } from "~/utils/pathBuilder";
import { organizationPath, v3ProjectSettingsGeneralPath } from "~/utils/pathBuilder";
import { useState } from "react";

function createSchema(
Expand Down Expand Up @@ -60,9 +60,21 @@ function createSchema(
]);
}

type FormAction = "rename" | "delete";

export function submissionFor(lastSubmission: unknown, formAction: FormAction) {
return lastSubmission &&
typeof lastSubmission === "object" &&
"formAction" in lastSubmission &&
lastSubmission.formAction === formAction
? lastSubmission
: undefined;
}

const Params = z.object({
organizationSlug: z.string(),
projectParam: z.string(),
envParam: z.string(),
});

export const action = dashboardAction(
Expand All @@ -75,9 +87,16 @@ export const action = dashboardAction(
},
async ({ user, ability, request, params }) => {
const userId = user.id;
const { organizationSlug, projectParam } = params;
const { organizationSlug, projectParam, envParam } = params;

const settingsPath = v3ProjectSettingsGeneralPath(
{ slug: organizationSlug },
{ slug: projectParam },
{ slug: envParam }
);

const formData = await request.formData();
const formAction = formData.get("action") as FormAction;

const schema = createSchema({
getSlugMatch: (slug) => {
Expand All @@ -87,7 +106,7 @@ export const action = dashboardAction(
const submission = parseWithZod(formData, { schema });

if (submission.status !== "success") {
return json(submission.reply());
return json({ ...submission.reply(), formAction });
}

const projectSettingsService = new ProjectSettingsService();
Expand All @@ -98,7 +117,10 @@ export const action = dashboardAction(
);

if (membershipResultOrFail.isErr()) {
return json({ errors: { body: membershipResultOrFail.error.type } }, { status: 404 });
return json(
{ ...submission.reply({ formErrors: ["Project not found"] }), formAction },
{ status: 404 }
);
}

const { projectId } = membershipResultOrFail.value;
Expand All @@ -107,7 +129,7 @@ export const action = dashboardAction(
case "rename": {
if (!ability.can("manage", { type: "project" })) {
throw await redirectWithErrorMessage(
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
settingsPath,
request,
"You don't have permission to rename this project"
);
Comment thread
claude[bot] marked this conversation as resolved.
Expand All @@ -126,21 +148,24 @@ export const action = dashboardAction(
logger.error("Failed to rename project", {
error: resultOrFail.error,
});
return json({ errors: { body: "Failed to rename project" } }, { status: 400 });
return json(
{ ...submission.reply({ formErrors: ["Failed to rename project"] }), formAction },
{ status: 400 }
);
}
}
}

return redirectWithSuccessMessage(
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
settingsPath,
request,
`Project renamed to ${submission.value.projectName}`
);
}
case "delete": {
if (!ability.can("manage", { type: "project" })) {
throw await redirectWithErrorMessage(
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
settingsPath,
request,
"You don't have permission to delete this project"
);
Expand All @@ -157,7 +182,7 @@ export const action = dashboardAction(
error: resultOrFail.error,
});
return redirectWithErrorMessage(
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
settingsPath,
request,
`Project ${projectParam} could not be deleted`
);
Expand Down Expand Up @@ -185,7 +210,7 @@ export default function GeneralSettingsPage() {
const [renameForm, { projectName }] = useForm({
id: "rename-project",
// TODO: type this
lastResult: lastSubmission as any,
lastResult: submissionFor(lastSubmission, "rename") as any,
shouldRevalidate: "onSubmit",
onValidate({ formData }) {
return parseWithZod(formData, {
Expand All @@ -201,7 +226,7 @@ export default function GeneralSettingsPage() {
const [deleteForm, { projectSlug }] = useForm({
id: "delete-project",
// TODO: type this
lastResult: lastSubmission as any,
lastResult: submissionFor(lastSubmission, "delete") as any,
shouldValidate: "onInput",
shouldRevalidate: "onSubmit",
onValidate({ formData }) {
Expand Down Expand Up @@ -250,6 +275,7 @@ export default function GeneralSettingsPage() {
}}
/>
<FormError id={projectName.errorId}>{projectName.errors}</FormError>
<FormError>{renameForm.errors}</FormError>
</InputGroup>
<FormButtons
confirmButton={
Expand Down
140 changes: 140 additions & 0 deletions apps/webapp/test/projectSettingsToastRedirect.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// A flashed toast survives exactly one hop: the root loader reads it with `session.get`
// (which deletes the flash) and commits the emptied session, so any hop that runs the root
// loader spends the message — including a hop whose leaf loader only redirects again and
// never renders the toast. The general settings action must therefore redirect to a page
// that renders.

import { errAsync, okAsync } from "neverthrow";
import { describe, expect, it, vi } from "vitest";
import { commitSession, getSession, redirectWithErrorMessage } from "~/models/message.server";
import {
action as generalSettingsAction,
submissionFor,
} from "~/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route";

vi.mock("~/services/routeBuilders/dashboardBuilder", () => ({
dashboardAction: (_options: unknown, handler: unknown) => handler,
dashboardLoader: (_options: unknown, handler: unknown) => handler,
}));

vi.mock("~/models/organization.server", () => ({
resolveOrgIdFromSlug: vi.fn().mockResolvedValue("org_1"),
}));

const renameFails = { value: false };

vi.mock("~/services/projectSettings.server", () => ({
ProjectSettingsService: class {
verifyProjectMembership() {
return okAsync({ projectId: "proj_1" });
}
renameProject() {
return renameFails.value ? errAsync({ type: "other" as const }) : okAsync(undefined);
}
deleteProject() {
return okAsync(undefined);
}
},
}));
Comment thread
carderne marked this conversation as resolved.

const SETTINGS_PATH = "/orgs/o/projects/p/env/prod/settings/general";
const ORG_PATH = "/orgs/o";

// Mirrors the read in app/root.tsx's loader.
async function rootLoaderHop(cookie: string | null) {
const session = await getSession(cookie);
const toastMessage = session.get("toastMessage");
return { toastMessage, setCookie: await commitSession(session) };
}

function asRequestCookie(setCookie: string) {
return setCookie.split(";")[0];
}

async function runAction(action: "rename" | "delete", allowed: boolean) {
const body = new URLSearchParams(
action === "rename" ? { action, projectName: "New name" } : { action, projectSlug: "p" }
);

try {
return (await (generalSettingsAction as any)({
user: { id: "user_1" },
ability: { can: () => allowed },
request: new Request(`https://app.example.com${SETTINGS_PATH}`, { method: "POST", body }),
params: { organizationSlug: "o", projectParam: "p", envParam: "prod" },
context: {},
searchParams: undefined,
})) as Response;
} catch (thrown) {
return thrown as Response;
}
}

async function toastFor(response: Response) {
const hop = await rootLoaderHop(asRequestCookie(response.headers.get("Set-Cookie")!));
return hop.toastMessage?.message;
}

describe("toast flash through a redirect chain", () => {
it("is lost when the redirect target redirects again", async () => {
const request = new Request(`https://app.example.com${SETTINGS_PATH}`, { method: "POST" });
const response = await redirectWithErrorMessage("/orgs/o/projects/p", request, "Denied");

const projectRootHop = await rootLoaderHop(
asRequestCookie(response.headers.get("Set-Cookie")!)
);
expect(projectRootHop.toastMessage?.message).toBe("Denied");

const tasksPageHop = await rootLoaderHop(asRequestCookie(projectRootHop.setCookie));
expect(tasksPageHop.toastMessage).toBeUndefined();
});
});

describe("general settings redirects target a page that renders", () => {
it("sends a denied rename back to the settings page with the message", async () => {
const response = await runAction("rename", false);

expect(response.headers.get("Location")).toBe(SETTINGS_PATH);
expect(await toastFor(response)).toBe("You don't have permission to rename this project");
});

it("sends a denied delete back to the settings page with the message", async () => {
const response = await runAction("delete", false);

expect(response.headers.get("Location")).toBe(SETTINGS_PATH);
expect(await toastFor(response)).toBe("You don't have permission to delete this project");
});

// The deleted project's settings page is gone and no org-level page renders, so a
// successful delete keeps its original destination and its message is not shown.
it("leaves a successful delete pointed at the organization root", async () => {
const response = await runAction("delete", true);

expect(response.headers.get("Location")).toBe(ORG_PATH);
});
});

describe("general settings failures reach the form", () => {
it("returns a form-level error when the rename fails", async () => {
renameFails.value = true;
const response = await runAction("rename", true);
renameFails.value = false;

expect(response.status).toBe(400);
expect(await response.json()).toMatchObject({
error: { "": ["Failed to rename project"] },
});
});

// A SubmissionResult carries no form identity, so both forms would otherwise show it.
it("scopes the rename failure to the rename form", async () => {
renameFails.value = true;
const response = await runAction("rename", true);
renameFails.value = false;

const result = await response.json();

expect(submissionFor(result, "rename")).toEqual(result);
expect(submissionFor(result, "delete")).toBeUndefined();
});
});
Loading