Skip to content

Commit 332435e

Browse files
[cueweb] Address review feedback on the Add Show modal
Harden the show proxy routes and remove duplication, matching the patterns used by the rest of CueWeb. - createshow/route.ts: wrap request.json() in try/catch (400 on malformed JSON instead of an unhandled 500), drop the redundant stringify -> parse round-trip, and pass log=true to handleRoute like the other action routes. - findshow/route.ts: same JSON hardening, and stop masking failures as "not found" - only a genuine not-found returns { notFound: true }; any other gateway/transport error propagates its real status, so the duplicate-name check can't silently pass during an outage. - shows/page.tsx: remove the second <ToastContainer/> (and its imports); toasts render through the global <ToastHost/> already mounted in layout. - create-show-dialog.tsx: use the shared toastSuccess from notify_utils instead of importing react-toastify directly. - Consolidate the Show type: extend the existing get_utils Show with optional bookingEnabled/dispatchEnabled and have show_utils re-export Show + getShows from get_utils, removing the divergent duplicate definitions. createShow stays a throwing helper so the modal can surface errors inline. - Add Jest tests for isValidShowName / findShow / createShow / getShows.
1 parent 145c4ae commit 332435e

7 files changed

Lines changed: 143 additions & 33 deletions

File tree

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/*
2+
* Copyright Contributors to the OpenCue Project
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import {
18+
isValidShowName,
19+
findShow,
20+
createShow,
21+
getShows,
22+
type Show,
23+
} from '@/app/utils/show_utils';
24+
import { accessGetApi } from '@/app/utils/api_utils';
25+
26+
jest.mock('@/app/utils/api_utils', () => ({
27+
accessGetApi: jest.fn(),
28+
}));
29+
30+
const mockShow: Show = {
31+
id: 's1', name: 'myshow', active: true, bookingEnabled: true, dispatchEnabled: true,
32+
};
33+
34+
describe('show_utils', () => {
35+
beforeEach(() => jest.clearAllMocks());
36+
37+
describe('isValidShowName', () => {
38+
it('accepts alphanumeric names', () => {
39+
expect(isValidShowName('myShow123')).toBe(true);
40+
});
41+
it('rejects empty, spaces, and punctuation', () => {
42+
expect(isValidShowName('')).toBe(false);
43+
expect(isValidShowName('my show')).toBe(false);
44+
expect(isValidShowName('my-show')).toBe(false);
45+
expect(isValidShowName('show_1')).toBe(false);
46+
});
47+
});
48+
49+
describe('findShow', () => {
50+
it('posts the name to /api/show/findshow and returns the show', async () => {
51+
(accessGetApi as jest.Mock).mockResolvedValue({ show: mockShow });
52+
53+
await expect(findShow('myshow')).resolves.toEqual(mockShow);
54+
expect(accessGetApi).toHaveBeenCalledWith(
55+
'/api/show/findshow',
56+
JSON.stringify({ name: 'myshow' }),
57+
);
58+
});
59+
60+
it('returns null when the gateway reports the show was not found', async () => {
61+
(accessGetApi as jest.Mock).mockResolvedValue({ notFound: true });
62+
await expect(findShow('nope')).resolves.toBeNull();
63+
});
64+
65+
it('returns null when nothing comes back', async () => {
66+
(accessGetApi as jest.Mock).mockResolvedValue(null);
67+
await expect(findShow('nope')).resolves.toBeNull();
68+
});
69+
});
70+
71+
describe('createShow', () => {
72+
it('posts the name to /api/show/createshow and returns the created show', async () => {
73+
(accessGetApi as jest.Mock).mockResolvedValue({ show: mockShow });
74+
75+
await expect(createShow('myshow')).resolves.toEqual(mockShow);
76+
expect(accessGetApi).toHaveBeenCalledWith(
77+
'/api/show/createshow',
78+
JSON.stringify({ name: 'myshow' }),
79+
);
80+
});
81+
82+
it('throws when creation fails so the form can surface the error', async () => {
83+
(accessGetApi as jest.Mock).mockResolvedValue(null);
84+
await expect(createShow('myshow')).rejects.toThrow('Failed to create show');
85+
});
86+
});
87+
88+
describe('getShows', () => {
89+
it('returns the array of shows', async () => {
90+
(accessGetApi as jest.Mock).mockResolvedValue([mockShow]);
91+
await expect(getShows()).resolves.toEqual([mockShow]);
92+
});
93+
94+
it('returns [] when the gateway returns a non-array', async () => {
95+
(accessGetApi as jest.Mock).mockResolvedValue(null);
96+
await expect(getShows()).resolves.toEqual([]);
97+
});
98+
});
99+
});

cueweb/app/api/show/createshow/route.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,18 @@ export async function POST(request: NextRequest) {
2424
return NextResponse.json({ error: 'Invalid method. Only POST is allowed.' }, { status: 405 });
2525
}
2626

27-
const body = JSON.stringify(await request.json());
28-
const jsonBody = JSON.parse(body);
27+
let jsonBody: any;
28+
try {
29+
jsonBody = await request.json();
30+
} catch {
31+
return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 });
32+
}
2933
if (!jsonBody || typeof jsonBody !== 'object' || !jsonBody.name) {
3034
return NextResponse.json({ error: 'Invalid request body: name is required' }, { status: 400 });
3135
}
3236

33-
const response = await handleRoute(method, endpoint, body);
37+
const body = JSON.stringify(jsonBody);
38+
const response = await handleRoute(method, endpoint, body, true);
3439
const responseData = await response.json();
3540

3641
if (!response.ok) return NextResponse.json({ error: responseData.error }, { status: response.status });

cueweb/app/api/show/findshow/route.ts

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,29 @@ export async function POST(request: NextRequest) {
2424
return NextResponse.json({ error: 'Invalid method. Only POST is allowed.' }, { status: 405 });
2525
}
2626

27-
const body = JSON.stringify(await request.json());
28-
const jsonBody = JSON.parse(body);
27+
let jsonBody: any;
28+
try {
29+
jsonBody = await request.json();
30+
} catch {
31+
return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 });
32+
}
2933
if (!jsonBody || typeof jsonBody !== 'object' || !jsonBody.name) {
3034
return NextResponse.json({ error: 'Invalid request body: name is required' }, { status: 400 });
3135
}
3236

33-
try {
34-
const response = await handleRoute(method, endpoint, body);
35-
const responseData = await response.json();
36-
if (!response.ok) return NextResponse.json({ notFound: true }, { status: 200 });
37-
return NextResponse.json({ data: responseData.data }, { status: response.status });
38-
} catch {
39-
return NextResponse.json({ notFound: true }, { status: 200 });
37+
const body = JSON.stringify(jsonBody);
38+
const response = await handleRoute(method, endpoint, body);
39+
const responseData = await response.json();
40+
41+
if (!response.ok) {
42+
// An unknown show comes back as a not-found error -> report { notFound }.
43+
// Any other error is a real failure, so keep its status instead of
44+
// reporting the name as available.
45+
const message = String(responseData?.error ?? "");
46+
if (/not\s*found/i.test(message)) {
47+
return NextResponse.json({ notFound: true }, { status: 200 });
48+
}
49+
return NextResponse.json({ error: responseData.error }, { status: response.status });
4050
}
51+
return NextResponse.json({ data: responseData.data }, { status: response.status });
4152
}

cueweb/app/shows/page.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,15 @@
1515
*/
1616

1717
import { getShows } from "@/app/utils/show_utils";
18-
import { ToastContainer } from "react-toastify";
19-
import "react-toastify/dist/ReactToastify.css";
2018
import ShowsClient from "./shows-client";
2119

20+
// Toasts render through the global <ToastHost /> mounted in app/layout.tsx,
21+
// so this page does not mount its own ToastContainer.
2222
export default async function ShowsPage() {
2323
const shows = await getShows();
2424

2525
return (
2626
<div className="container mx-auto py-10 max-w-[90%]">
27-
<ToastContainer />
2827
<ShowsClient shows={shows} />
2928
</div>
3029
);

cueweb/app/utils/get_utils.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,15 @@ export type Proc = {
9696
unbooked: boolean;
9797
};
9898

99-
// Minimal Show shape - matches the show.Show proto fields the dashboard cares about.
99+
// Minimal Show shape - matches the show.Show proto fields the dashboard and
100+
// the Shows page care about. booking/dispatch are optional so dashboard
101+
// callers that don't request them still typecheck.
100102
export type Show = {
101103
id: string;
102104
name: string;
103105
active: boolean;
106+
bookingEnabled?: boolean;
107+
dispatchEnabled?: boolean;
104108
showStats?: {
105109
runningFrames: number;
106110
pendingFrames: number;

cueweb/app/utils/show_utils.ts

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,12 @@
1515
*/
1616

1717
import { accessGetApi } from "./api_utils";
18+
import type { Show } from "./get_utils";
1819

19-
// Mirrors the Show proto message at proto/src/show.proto.
20-
export type Show = {
21-
id: string;
22-
name: string;
23-
active: boolean;
24-
bookingEnabled: boolean;
25-
dispatchEnabled: boolean;
26-
};
20+
// Single source of truth: Show and getShows live in get_utils with the other
21+
// object getters; re-exported here so the Shows feature imports from one place.
22+
export type { Show };
23+
export { getShows } from "./get_utils";
2724

2825
// Show names must be alphanumeric only (no spaces, dashes, or punctuation).
2926
export function isValidShowName(name: string): boolean {
@@ -38,13 +35,8 @@ export async function findShow(name: string): Promise<Show | null> {
3835
return response.show ?? null;
3936
}
4037

41-
// Returns all shows.
42-
export async function getShows(): Promise<Show[]> {
43-
const response = await accessGetApi("/api/show/getshows", JSON.stringify({}));
44-
return response ?? [];
45-
}
46-
47-
// Creates a new show with the given name and returns it.
38+
// Creates a new show with the given name and returns it. Throws on failure so
39+
// the modal form can surface the reason inline.
4840
export async function createShow(name: string): Promise<Show> {
4941
const body = JSON.stringify({ name });
5042
const response = await accessGetApi("/api/show/createshow", body);

cueweb/components/ui/create-show-dialog.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ import {
2525
DialogHeader,
2626
DialogTitle,
2727
} from "@/components/ui/dialog";
28+
import { toastSuccess } from "@/app/utils/notify_utils";
2829
import * as React from "react";
29-
import { toast } from "react-toastify";
3030

3131
interface CreateShowDialogProps {
3232
open: boolean;
@@ -71,7 +71,7 @@ export function CreateShowDialog({ open, onOpenChange, onSuccess }: CreateShowDi
7171
}
7272

7373
await createShow(trimmed);
74-
toast.success(`Show "${trimmed}" created successfully.`);
74+
toastSuccess(`Show "${trimmed}" created successfully.`);
7575
handleOpenChange(false);
7676
onSuccess?.(trimmed);
7777
} catch (err) {

0 commit comments

Comments
 (0)