feat: migrate shipnoise UI into orcasite monorepo#996
Open
fdsafewa wants to merge 8 commits intoorcasound:mainfrom
Open
feat: migrate shipnoise UI into orcasite monorepo#996fdsafewa wants to merge 8 commits intoorcasound:mainfrom
fdsafewa wants to merge 8 commits intoorcasound:mainfrom
Conversation
- Add Next.js middleware to detect shipnoise.net host and rewrite requests to /shipnoise/* pages (checks both host and x-forwarded-host) - Add Phoenix proxy scope at /api/shipnoise/* forwarding to the separate shipnoise FastAPI backend (SHIPNOISE_API_URL env var) - Create /shipnoise and /shipnoise/report pages using Pages Router with ShipnoiseLayout (Banner + custom layout) - Port all Shipnoise components: Banner, SelectionPanel, AvailableRecordings, ShipnoiseDetectionsPlayer, ShipnoiseLayout - Add useShipnoiseApi hook (vessel search, clips search via React Query) and useDebounce hook - Add Next.js API proxy route (/api/shipnoise/[...path]) for server-side forwarding — avoids CORS and mirrors production Phoenix proxy - Add /api/shipnoise/issues route proxying Google Sheets issue data - Use existing VideoJS component for HLS playback (no mux.js needed) - Add shipnoise static assets (Logo, VesselIcon, SVG icons) - Update .env.development with NEXT_PUBLIC_SHIPNOISE_API_URL and SHIPNOISE_API_URL for local development - Update runtime.exs to document SHIPNOISE_API_URL production env var
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Shipnoise feature: server runtime config and reverse-proxy, a frontend Shipnoise section with pages, layout and playback UI, client-side hooks/middleware, and Next.js API proxies for Shipnoise backend and issue reports. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as SelectionPanel / AvailableRecordings
participant Hooks as useShipnoiseApi
participant NextAPI as /api/shipnoise
participant Backend as Shipnoise Backend
User->>UI: enter vessel & date, click search
UI->>Hooks: call useClipsSearch / useVesselSearch
Hooks->>NextAPI: GET /api/shipnoise/...(proxied)
NextAPI->>Backend: forward request to SHIPNOISE_API_URL
Backend-->>NextAPI: response (JSON/clips/vessels)
NextAPI-->>Hooks: proxied response
Hooks-->>UI: search results
UI->>UI: render AvailableRecordings & players
User->>UI: play recording
UI->>Backend: HLS stream requested by player (direct or proxied per config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
ui/src/pages/api/shipnoise/issues.ts (1)
3-4: Move the sheet URL into runtime config.Hardcoding the Apps Script URL makes rotation and environment-specific overrides a code change. This should follow the same
process.env...pattern already used inui/src/pages/api/shipnoise/[...path].ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/api/shipnoise/issues.ts` around lines 3 - 4, Replace the hardcoded Apps Script URL constant SHEET_API_URL with a runtime-config/environment variable lookup (e.g. process.env.SHEET_API_URL) following the same pattern used in the [...path].ts API handler; update the code to read the env var (with an optional fallback or explicit error if missing) and ensure any callers use the new constant name unchanged so rotation and environment-specific overrides can be done via deployment config rather than code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/orcasite_web/router.ex`:
- Around line 66-74: The broad forward("/", ReverseProxyPlug, ...) inside scope
"/api/shipnoise" is catching all /api/shipnoise/* requests and shadowing the
Next.js endpoint; before the ReverseProxyPlug forward, add a specific route for
"/issues" (e.g., get "/issues", YourController.action or forward "/issues" to
the Sheets proxy plug) so requests to "/api/shipnoise/issues" are handled by the
reserved handler, or alternatively move the issues endpoint out of the
"/api/shipnoise" namespace; update router.ex to place the specific get/forward
for "/issues" above the forward("/", ReverseProxyPlug, ...) (referencing scope
"/api/shipnoise" and the existing ReverseProxyPlug and log_reverse_proxy_error).
- Around line 69-73: Remove the compile-time System.get_env usage (the
shipnoise_url binding) from the router module body and instead set the value in
runtime config (e.g., in config/runtime.exs as :your_app, :shipnoise_api_url).
In the router, stop referencing the shipnoise_url variable; create a small
runtime plug or route handler that calls Application.get_env(:your_app,
:shipnoise_api_url, "http://localhost:5000") and then invokes
ReverseProxyPlug.call/2 with that upstream and the existing error_callback
(&__MODULE__.log_reverse_proxy_error/1). This keeps ReverseProxyPlug and
log_reverse_proxy_error intact but ensures the upstream URL is read at
boot/runtime rather than at compile time.
In `@ui/.env.development`:
- Line 5: The SHIPNOISE_API_URL in .env.development currently points at the live
service; change it to a safe local default (e.g., empty string or
http://localhost:PORT) so local dev does not hit production, and move the live
URL into an uncommitted local override or .env.example with a clear comment;
update any README/dev docs to mention using a local override for
SHIPNOISE_API_URL and not committing production endpoints.
In `@ui/src/components/Shipnoise/AvailableRecordings.tsx`:
- Around line 76-87: The component AvailableRecordings.tsx currently injects the
Mailchimp script in its useEffect (creating element with id "mcjs" and src =
MAILCHIMP_SCRIPT + "?v=" + new Date().getTime()) on every mount, which adds an
unnecessary tracker and defeats caching; remove this automatic injection from
the useEffect and the cache-busting querystring, and instead implement
lazy-loading: create a dedicated handler (e.g., loadMailchimpScript) that
appends the MAILCHIMP_SCRIPT once (skip "?v=" + Date.now()), checks for existing
script by id "mcjs" or window.mc4wp/window.mcpopup before adding, and call that
handler only from an explicit CTA or signup flow in the UI when the popup is
actually required.
- Around line 114-125: preferred and others currently include location entries
with zero recordings so the UI shows multiple "0 recordings" sections; update
the construction of preferred and others to exclude groups with no recordings by
filtering out entries where (grouped[label] ?? []).length === 0 before mapping
(i.e., only map labels that have recordings), then merge and sort that filtered
list by recordings.length as before; refer to PREFERRED_LOCATIONS, grouped,
preferred, others, and sortDesc to locate and modify the logic.
In `@ui/src/components/Shipnoise/Banner.tsx`:
- Around line 60-86: The code nests a Button inside a Link which is invalid
HTML; fix by replacing the Link wrapper with the MUI Button configured as an
anchor: remove the Link import and change the Link element to the existing
Button with props component="a",
href="https://mailchi.mp/7ce0cea69cd0/help-improve-shipnoise", target="_blank",
rel="noreferrer" (move any sx width from the Link to the Button if needed),
keeping all other Button props and styles unchanged so it renders as an
accessible anchor-button.
- Around line 115-176: Replace the custom modal Box block that renders when
showReportForm is true with MUI's Dialog: use <Dialog open={showReportForm}
onClose={closeReportForm}> and move the iframe into <DialogContent> (and
optional <DialogTitle>/<DialogActions> for the "Close" Button) so you get
built-in focus trapping, modal semantics and focus restoration; ensure the
existing closeReportForm handler is passed to Dialog's onClose (so Esc and
backdrop clicks close it) and keep the iframe src and allow attributes intact
while preserving the Close Button (use DialogActions or a positioned Button
inside the Dialog for the same styling/behavior).
In `@ui/src/components/Shipnoise/ShipnoiseDetectionsPlayer.tsx`:
- Around line 38-94: The video.js listeners in handlePlayerReady close over
startOffsetSec/endOffsetSec and become stale; fix by creating a ref (e.g.,
clipBoundsRef) that stores {startOffsetSec, endOffsetSec} and update it in a
useEffect when props change, then inside handlePlayerReady's listeners (the
"playing" and "timeupdate" handlers) read bounds from clipBoundsRef.current
instead of the closed-over variables; keep playerRef usage and ensure
handlePlayerReady does not depend on startOffsetSec/endOffsetSec so the
listeners remain stable and always enforce the current clip bounds.
In `@ui/src/hooks/useShipnoiseApi.ts`:
- Around line 136-138: The forEach callback in useShipnoiseApi currently uses a
concise arrow that implicitly returns the result of searchParams.append(...) and
triggers the useIterableCallbackReturn lint rule; change the callback to a block
body so it does not return a value (e.g., replace (params.sites ??
SITE_VALUES).forEach((site) => searchParams.append("sites", site)) with a
block-bodied callback that calls searchParams.append("sites", site); inside),
locating this inside the function/useShipnoiseApi where searchParams is built.
In `@ui/src/middleware.ts`:
- Around line 4-8: The current host check using includes can match unintended
domains; in middleware.ts normalize and boundary-check host and forwardedHost by
lowercasing and stripping any port (e.g., split on ':' and take first segment),
then replace the includes("shipnoise.net") test with a precise boundary check
such as host === "shipnoise.net" || host.endsWith(".shipnoise.net") (and the
same for forwardedHost) so only the exact domain or its subdomains trigger the
isShipnoiseHost rewrite.
In `@ui/src/pages/api/shipnoise/`[...path].ts:
- Around line 26-39: The proxy currently always calls response.json() and sets
JSON headers, which breaks on 204/empty or non-JSON upstream responses; update
the fetch call to forward incoming headers (instead of hardcoded
Content-Type/Accept) and only stringify the body for non-GET/HEAD when
appropriate, then change the response handling to: if response.status === 204 or
response.headers.get('content-length') === '0' -> return
res.status(response.status).end(); else if response.headers.get('content-type')
includes 'application/json' -> await response.json() and return
res.status(response.status).json(data); otherwise await response.text(), copy
the upstream content-type header to the outgoing response and return
res.status(response.status).send(text). Use the existing upstreamUrl,
req.method, req.body, response and res symbols to locate and update the code
paths.
- Around line 4-5: Remove the hardcoded fallback for SHIPNOISE_API_URL and make
its presence mandatory: stop using the || "https://..." default for the
SHIPNOISE_API_URL constant and instead check for SHIPNOISE_API_URL at the start
of the API route handler (the exported request handler function for
[...path].ts); if it is undefined or empty, immediately return a 500 response
with a JSON error message stating that SHIPNOISE_API_URL is not configured so
local/misconfigured environments do not call the live backend.
In `@ui/src/pages/api/shipnoise/issues.ts`:
- Around line 6-39: The handler in ui/src/pages/api/shipnoise/issues.ts exposes
raw sheet data publicly; modify the exported async function handler to require
moderator/admin authentication before fetching SHEET_API_URL (e.g., validate
session/token and check user role) and return 401/403 for unauthorized callers;
also remove the client-facing debug echo (the debug: rawBody.slice(0, 120) in
the 502 response) and instead log parse errors server-side (console.error or app
logger) and return a generic error message to clients; keep the rest of the
parse/fetch logic but ensure only authorized users can reach it.
- Around line 11-13: The upstream fetch to SHEET_API_URL should be bounded with
an AbortController and timeout to avoid stalled Apps Script requests; modify the
fetch invocation around the existing const response = await fetch(SHEET_API_URL,
{ cache: "no-store" } as RequestInit) to create an AbortController, pass signal
into the RequestInit, start a setTimeout that calls controller.abort() after a
fixed interval (e.g. 5–10s), and clear the timer after the fetch completes;
additionally catch the abort path (check for AbortError or error.name ===
"AbortError") and map it to a 502 or 504 response so the API route returns
quickly on upstream timeouts.
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 450-570: The modal markup around selected currently lacks dialog
semantics, focus trapping, Escape handling and focus restoration; replace the
custom overlay with an accessible dialog primitive (e.g., MUI Dialog/Modal or
Reach Dialog) or implement the same behaviors: render the content only when
selected, add role="dialog" and aria-modal="true", trap focus inside the modal
and autofocus the first focusable element, listen for Escape to call
setSelected(null) and restore focus to the previously focused element when
closing, and ensure background content is inert/aria-hidden while the dialog is
open; update usage points around selected, setSelected and buildDisplayFields to
use the dialog primitive API or the new handlers.
- Around line 282-300: The load() effect currently treats any non-OK HTTP
response as an empty list because it never checks res.ok; update load() to check
res.ok after fetch("/api/shipnoise/issues"), and if !res.ok set an explicit
error state (e.g., setFetchError or setIssuesError) and bail without sorting or
calling setIssues; only parse JSON and sort (using getIssueTimestamp) when
res.ok is true; ensure setLoading(false) still runs in finally but that the
error state is used by the component to show an error instead of an empty
dashboard.
---
Nitpick comments:
In `@ui/src/pages/api/shipnoise/issues.ts`:
- Around line 3-4: Replace the hardcoded Apps Script URL constant SHEET_API_URL
with a runtime-config/environment variable lookup (e.g.
process.env.SHEET_API_URL) following the same pattern used in the [...path].ts
API handler; update the code to read the env var (with an optional fallback or
explicit error if missing) and ensure any callers use the new constant name
unchanged so rotation and environment-specific overrides can be done via
deployment config rather than code changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccd58af4-16b8-4be9-a4e8-160bf3c1867a
⛔ Files ignored due to path filters (7)
ui/public/shipnoise/Logo.pngis excluded by!**/*.pngui/public/shipnoise/Search.svgis excluded by!**/*.svgui/public/shipnoise/VesselIcon.pngis excluded by!**/*.pngui/public/shipnoise/Warning.svgis excluded by!**/*.svgui/public/shipnoise/delete.svgis excluded by!**/*.svgui/public/shipnoise/playbutton.svgis excluded by!**/*.svgui/public/shipnoise/up.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
server/config/runtime.exsserver/lib/orcasite_web/router.exui/.env.developmentui/src/components/Shipnoise/AvailableRecordings.tsxui/src/components/Shipnoise/Banner.tsxui/src/components/Shipnoise/SelectionPanel.tsxui/src/components/Shipnoise/ShipnoiseDetectionsPlayer.tsxui/src/components/Shipnoise/ShipnoiseLayout.tsxui/src/hooks/useDebounce.tsui/src/hooks/useShipnoiseApi.tsui/src/middleware.tsui/src/pages/api/shipnoise/[...path].tsui/src/pages/api/shipnoise/issues.tsui/src/pages/shipnoise/index.tsxui/src/pages/shipnoise/report.tsx
Comment on lines
+6
to
+39
| export default async function handler( | ||
| _req: NextApiRequest, | ||
| res: NextApiResponse, | ||
| ) { | ||
| try { | ||
| const response = await fetch(SHEET_API_URL, { | ||
| cache: "no-store", | ||
| } as RequestInit); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Upstream request failed with ${response.status}`); | ||
| } | ||
|
|
||
| const rawBody = await response.text(); | ||
|
|
||
| try { | ||
| const sanitized = rawBody.replace(/^\)\]\}'/, "").trim(); | ||
| if (!sanitized) { | ||
| return res.status(200).json({ data: [] }); | ||
| } | ||
| const parsed = JSON.parse(sanitized); | ||
| return res.status(200).json(parsed); | ||
| } catch (parseError) { | ||
| console.error("Upstream returned non-JSON payload:", parseError); | ||
| return res.status(502).json({ | ||
| error: "Upstream response was not valid JSON", | ||
| debug: rawBody.slice(0, 120), | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to fetch Sheet data:", error); | ||
| return res.status(500).json({ error: "Failed to fetch Sheet data" }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Protect this endpoint before exposing issue reports.
This handler returns the raw sheet to any caller. ui/src/pages/shipnoise/report.tsx then renders respondent IDs, attachments, and security-related submissions from that response, so the dataset is publicly enumerable unless something upstream blocks it. Gate the route with moderator/admin auth and remove the client-facing debug echo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/api/shipnoise/issues.ts` around lines 6 - 39, The handler in
ui/src/pages/api/shipnoise/issues.ts exposes raw sheet data publicly; modify the
exported async function handler to require moderator/admin authentication before
fetching SHEET_API_URL (e.g., validate session/token and check user role) and
return 401/403 for unauthorized callers; also remove the client-facing debug
echo (the debug: rawBody.slice(0, 120) in the 502 response) and instead log
parse errors server-side (console.error or app logger) and return a generic
error message to clients; keep the rest of the parse/fetch logic but ensure only
authorized users can reach it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ui/src/components/Shipnoise/AvailableRecordings.tsx (2)
114-125:⚠️ Potential issue | 🟡 MinorEmpty location groups still render.
PREFERRED_LOCATIONS.map(...)always includes all four preferred locations regardless of whether they have recordings. A vessel with data at only one site will show multiple "0 recordings" sections.Suggested fix
- const preferred = PREFERRED_LOCATIONS.map((label) => ({ - label, - recordings: sortDesc(grouped[label] ?? []), - })); + const preferred = PREFERRED_LOCATIONS + .filter((label) => (grouped[label]?.length ?? 0) > 0) + .map((label) => ({ + label, + recordings: sortDesc(grouped[label]), + })); const others = Object.keys(grouped) .filter((label) => !PREFERRED_LOCATIONS.includes(label)) .map((label) => ({ label, recordings: sortDesc(grouped[label]) })); - return [...preferred, ...others].sort( - (a, b) => b.recordings.length - a.recordings.length, - ); + return [...preferred, ...others] + .filter(({ recordings }) => recordings.length > 0) + .sort((a, b) => b.recordings.length - a.recordings.length);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Shipnoise/AvailableRecordings.tsx` around lines 114 - 125, The preferred-location blocks are always emitted because PREFERRED_LOCATIONS.map creates entries even when grouped[label] is empty; update the construction of preferred in AvailableRecordings.tsx to filter out labels with no recordings (e.g., where (grouped[label] ?? []).length === 0) before mapping, so only preferred locations that actually have recordings are included; keep the existing sortDesc and the remaining logic for others and final sorting intact.
76-87:⚠️ Potential issue | 🟠 MajorThird-party Mailchimp script still injected without functional use.
This
useEffectloads an external Mailchimp tracking script on every mount, but nothing in the component useswindow.mcpopuporwindow.mc4wp. This adds network overhead and privacy implications for users viewing recordings.If the signup popup is needed elsewhere, lazy-load it from an explicit CTA rather than mounting it unconditionally here.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Shipnoise/AvailableRecordings.tsx` around lines 76 - 87, The component AvailableRecordings currently injects the Mailchimp script unconditionally in the useEffect (script id "mcjs", src MAILCHIMP_SCRIPT) even though the component never references window.mcpopup or window.mc4wp; remove this unconditional injection and instead load the script only when explicitly needed (e.g., on a signup CTA click or when a prop like enableMailchimp is true) by moving the script creation logic out of the mount useEffect and into a lazy-load handler that checks/creates the script element and appends it to document.body only when invoked.ui/src/pages/shipnoise/report.tsx (1)
471-591:⚠️ Potential issue | 🟠 MajorModal needs accessible dialog behavior.
The custom overlay lacks dialog semantics (
role="dialog",aria-modal), focus trapping, and Escape key handling. Keyboard and screen-reader users may get trapped or lost behind the modal.Consider using MUI's
<Dialog>component which provides these behaviors out of the box, or manually add:
role="dialog"andaria-modal="true"to the Paper- Focus trap on mount/unmount
- Escape key listener to close
- Backdrop click already closes via
setSelected(null)(missing from current impl),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/shipnoise/report.tsx` around lines 471 - 591, The modal overlay rendered when selected is truthy (using Box, Paper and setSelected) lacks dialog semantics and keyboard/focus behavior; either replace this custom overlay with MUI's Dialog/DialogTitle/DialogContent/DialogActions (so it handles role/aria-modal, focus trap, Escape, and backdrop clicks) or if keeping the custom implementation: add role="dialog" and aria-modal="true" to the Paper, implement a focus trap that activates on mount/unmount (tie to the selected state), add a keydown handler to listen for Escape and call setSelected(null), and ensure backdrop clicks call setSelected(null); update buildDisplayFields usage as needed to remain accessible within the focus trap.server/lib/orcasite_web/router.ex (1)
66-73:⚠️ Potential issue | 🔴 CriticalPhoenix still shadows the Next.js
/api/shipnoise/issuesroute.This scope's
forward("/", ReverseProxyPlug, ...)captures all/api/shipnoise/*requests before they can reach the Next.js catch-all. Theui/src/pages/api/shipnoise/issues.tsendpoint (which proxies Google Sheets data) will be unreachable in production—requests to/api/shipnoise/issueswill be forwarded to the Shipnoise FastAPI backend instead.Options to fix:
- Add a specific route for
/issuesbefore the forward that passes through to Next.js- Move the issues endpoint out of the
/api/shipnoisenamespace (e.g.,/api/shipnoise-issues),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/orcasite_web/router.ex` around lines 66 - 73, The forward("/", ReverseProxyPlug, ...) inside scope "/api/shipnoise" is catching /api/shipnoise/issues and shadowing the Next.js endpoint; add a specific route for "/issues" before the catch-all forward so requests to issues are not forwarded to the Shipnoise backend. Concretely, in the scope "/api/shipnoise" add an explicit match for "/issues" (e.g., a get/post match or a forward that delegates to your Next.js handler) placed above the forward("/", ReverseProxyPlug, ...) so the /issues path is handled by Next.js instead of ReverseProxyPlug; alternatively rename/move the Next.js endpoint if you prefer the other option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 289-294: Normalize the API response handling in report.tsx so it
accepts both direct-array responses and the { data: [] } shape: inspect the
fetched "data" value and if it's an object with a "data" property, extract that
array; otherwise treat "data" as the array; then apply the existing sort using
getIssueTimestamp to produce "sorted". This ensures the logic in the const data
/ const sorted block handles both formats returned by the endpoint implemented
in issues.ts.
---
Duplicate comments:
In `@server/lib/orcasite_web/router.ex`:
- Around line 66-73: The forward("/", ReverseProxyPlug, ...) inside scope
"/api/shipnoise" is catching /api/shipnoise/issues and shadowing the Next.js
endpoint; add a specific route for "/issues" before the catch-all forward so
requests to issues are not forwarded to the Shipnoise backend. Concretely, in
the scope "/api/shipnoise" add an explicit match for "/issues" (e.g., a get/post
match or a forward that delegates to your Next.js handler) placed above the
forward("/", ReverseProxyPlug, ...) so the /issues path is handled by Next.js
instead of ReverseProxyPlug; alternatively rename/move the Next.js endpoint if
you prefer the other option.
In `@ui/src/components/Shipnoise/AvailableRecordings.tsx`:
- Around line 114-125: The preferred-location blocks are always emitted because
PREFERRED_LOCATIONS.map creates entries even when grouped[label] is empty;
update the construction of preferred in AvailableRecordings.tsx to filter out
labels with no recordings (e.g., where (grouped[label] ?? []).length === 0)
before mapping, so only preferred locations that actually have recordings are
included; keep the existing sortDesc and the remaining logic for others and
final sorting intact.
- Around line 76-87: The component AvailableRecordings currently injects the
Mailchimp script unconditionally in the useEffect (script id "mcjs", src
MAILCHIMP_SCRIPT) even though the component never references window.mcpopup or
window.mc4wp; remove this unconditional injection and instead load the script
only when explicitly needed (e.g., on a signup CTA click or when a prop like
enableMailchimp is true) by moving the script creation logic out of the mount
useEffect and into a lazy-load handler that checks/creates the script element
and appends it to document.body only when invoked.
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 471-591: The modal overlay rendered when selected is truthy (using
Box, Paper and setSelected) lacks dialog semantics and keyboard/focus behavior;
either replace this custom overlay with MUI's
Dialog/DialogTitle/DialogContent/DialogActions (so it handles role/aria-modal,
focus trap, Escape, and backdrop clicks) or if keeping the custom
implementation: add role="dialog" and aria-modal="true" to the Paper, implement
a focus trap that activates on mount/unmount (tie to the selected state), add a
keydown handler to listen for Escape and call setSelected(null), and ensure
backdrop clicks call setSelected(null); update buildDisplayFields usage as
needed to remain accessible within the focus trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 259f5bb2-61d8-42b5-9cc0-812ba233f997
📒 Files selected for processing (6)
server/config/runtime.exsserver/lib/orcasite_web/router.exui/.env.developmentui/src/components/Shipnoise/AvailableRecordings.tsxui/src/pages/api/shipnoise/issues.tsui/src/pages/shipnoise/report.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/pages/api/shipnoise/issues.ts
- server/config/runtime.exs
…oenix proxy conflict
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ui/src/pages/api/shipnoise-issues.ts (3)
22-27:⚠️ Potential issue | 🟡 MinorReturn one success schema consistently.
Line 24 returns
{ data: [] }, while Line 27 returns raw parsed JSON. Keep one shape (prefer plain array) across all success paths to avoid downstream contract branching.Suggested patch
const sanitized = rawBody.replace(/^\)\]\}'/, "").trim(); if (!sanitized) { - return res.status(200).json({ data: [] }); + return res.status(200).json([]); } const parsed = JSON.parse(sanitized); - return res.status(200).json(parsed); + return res.status(200).json(Array.isArray(parsed) ? parsed : []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/api/shipnoise-issues.ts` around lines 22 - 27, The success responses must use a single plain-array shape: change the early return that currently does res.status(200).json({ data: [] }) to return an empty array instead, and ensure the final return normalizes parsed into an array (in the handler around rawBody/sanitized/parsed) by returning parsed if Array.isArray(parsed) else [parsed] so both success paths always return a plain array.
6-9:⚠️ Potential issue | 🔴 CriticalGate this endpoint behind moderator/admin authorization.
This route is publicly callable and returns raw issue-report submissions. Add an auth check before fetching upstream and return 401/403 for unauthorized callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/api/shipnoise-issues.ts` around lines 6 - 9, The handler function (export default async function handler) currently serves raw issue reports to any caller; add an authorization gate at the top of the handler that verifies the caller is a moderator/admin (e.g., validate a session, JWT, or admin flag on the user from _req headers/cookies) before proceeding to fetch upstream. If the auth check fails, immediately return res.status(401 or 403). Only after the auth check passes should you continue with the existing fetch/response logic so unauthorized requests never receive raw submissions.
11-37:⚠️ Potential issue | 🔴 CriticalAdd an upstream timeout and map dependency failures to gateway errors.
The upstream
fetchis unbounded and can pin API workers until platform timeout. AddAbortController(e.g., 10s), handleAbortErroras 504, and return 502 for other upstream failures instead of 500.Suggested patch
export default async function handler( _req: NextApiRequest, res: NextApiResponse, ) { try { - const response = await fetch(SHEET_API_URL, { - cache: "no-store", - } as RequestInit); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10_000); + const response = await fetch(SHEET_API_URL, { + cache: "no-store", + signal: controller.signal, + } as RequestInit).finally(() => clearTimeout(timeoutId)); if (!response.ok) { throw new Error(`Upstream request failed with ${response.status}`); } @@ - } catch (error) { + } catch (error) { + if (error instanceof Error && error.name === "AbortError") { + return res.status(504).json({ error: "Sheet upstream timed out" }); + } console.error("Failed to fetch Sheet data:", error); - return res.status(500).json({ error: "Failed to fetch Sheet data" }); + return res.status(502).json({ error: "Failed to fetch Sheet data" }); } }#!/bin/bash set -euo pipefail file="$(fd '^shipnoise-issues\.ts$' | head -n1)" echo "Inspecting: $file" nl -ba "$file" | sed -n '1,120p' echo echo "Timeout-related symbols (expect at least AbortController/signal/setTimeout):" rg -n 'AbortController|controller\.abort|signal:\s*controller\.signal|setTimeout\(' "$file" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/api/shipnoise-issues.ts` around lines 11 - 37, The upstream fetch is unbounded; wrap the fetch(SHEET_API_URL, ...) call with an AbortController (create controller, pass signal to fetch, and set a 10_000ms timeout via setTimeout that calls controller.abort and is cleared on success) so the request cancels after 10s; then change error handling so an AbortError from fetch results in res.status(504) and other upstream/fetch failures map to res.status(502) (update the catch that currently returns 500 and the console.error messages accordingly), keeping existing JSON-parse handling (parseError) unchanged and ensuring the timeout timer is cleared when response is received or on errors.ui/src/pages/shipnoise/report.tsx (1)
471-591:⚠️ Potential issue | 🟠 MajorModal still lacks accessible dialog behavior (semantics/focus/Escape).
This overlay is still missing robust dialog semantics and focus management (focus trap, Escape close, and focus restore). Please switch to an accessible dialog primitive or implement those behaviors on the custom modal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/shipnoise/report.tsx` around lines 471 - 591, The custom overlay uses plain Boxes and Paper and lacks dialog semantics and focus management; replace it with an accessible dialog primitive (e.g., MUI Dialog) or add proper behaviors: wrap the markup rendered when selected in a focus-trapping dialog with role="dialog" and aria-modal="true", capture and restore the previously focused element when opening/closing (use a ref to store document.activeElement and call focus() on it in cleanup), handle Escape to call setSelected(null), and implement Tab/Shift+Tab trapping inside the Paper (or use a focus-trap hook/library) so focus cannot leave the modal; update the code around selected, setSelected and the Paper/Box rendering (and any onClick/onClose handlers) to wire these behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 420-461: The "View" Button lacks an onClick so keyboard users
can't open details; update the Button component (inside the TableRow loop) to
call setSelected(issue) on click (e.g., onClick={(e) => { e.stopPropagation();
setSelected(issue); }}), add an accessible label like aria-label="View
submission details" and keep the TableRow onClick as optional; no extra handlers
for Enter/Space are required because a native Button handles keyboard
activation, but stopPropagation prevents double-handling of the row click.
---
Duplicate comments:
In `@ui/src/pages/api/shipnoise-issues.ts`:
- Around line 22-27: The success responses must use a single plain-array shape:
change the early return that currently does res.status(200).json({ data: [] })
to return an empty array instead, and ensure the final return normalizes parsed
into an array (in the handler around rawBody/sanitized/parsed) by returning
parsed if Array.isArray(parsed) else [parsed] so both success paths always
return a plain array.
- Around line 6-9: The handler function (export default async function handler)
currently serves raw issue reports to any caller; add an authorization gate at
the top of the handler that verifies the caller is a moderator/admin (e.g.,
validate a session, JWT, or admin flag on the user from _req headers/cookies)
before proceeding to fetch upstream. If the auth check fails, immediately return
res.status(401 or 403). Only after the auth check passes should you continue
with the existing fetch/response logic so unauthorized requests never receive
raw submissions.
- Around line 11-37: The upstream fetch is unbounded; wrap the
fetch(SHEET_API_URL, ...) call with an AbortController (create controller, pass
signal to fetch, and set a 10_000ms timeout via setTimeout that calls
controller.abort and is cleared on success) so the request cancels after 10s;
then change error handling so an AbortError from fetch results in
res.status(504) and other upstream/fetch failures map to res.status(502) (update
the catch that currently returns 500 and the console.error messages
accordingly), keeping existing JSON-parse handling (parseError) unchanged and
ensuring the timeout timer is cleared when response is received or on errors.
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 471-591: The custom overlay uses plain Boxes and Paper and lacks
dialog semantics and focus management; replace it with an accessible dialog
primitive (e.g., MUI Dialog) or add proper behaviors: wrap the markup rendered
when selected in a focus-trapping dialog with role="dialog" and
aria-modal="true", capture and restore the previously focused element when
opening/closing (use a ref to store document.activeElement and call focus() on
it in cleanup), handle Escape to call setSelected(null), and implement
Tab/Shift+Tab trapping inside the Paper (or use a focus-trap hook/library) so
focus cannot leave the modal; update the code around selected, setSelected and
the Paper/Box rendering (and any onClick/onClose handlers) to wire these
behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9651e6b5-194f-4c4f-9fba-12b774a444a3
📒 Files selected for processing (2)
ui/src/pages/api/shipnoise-issues.tsui/src/pages/shipnoise/report.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/src/pages/shipnoise/report.tsx (2)
446-460:⚠️ Potential issue | 🟠 MajorMake the “View” button open details directly (keyboard path).
The button is focusable but has no
onClick; opening depends on row click. BindsetSelected(issue)to the button and stop propagation.✅ Suggested patch
<TableCell align="center" sx={{ px: { xs: 2, sm: 3 }, py: 1.5 }} > <Button + onClick={(e) => { + e.stopPropagation(); + setSelected(issue); + }} + aria-label={`View issue ${toDisplayString( + issue["Submission ID"] ?? issue.ID ?? i + 1, + )}`} variant="contained" sx={{ borderRadius: "999px", bgcolor: "#1f2937",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/shipnoise/report.tsx` around lines 446 - 460, The "View" Button is focusable but doesn't open the details—add an onClick handler on this Button to call event.stopPropagation() and then call setSelected(issue) so the row's details open directly from the button; locate the Button component in report.tsx and wire onClick={(e) => { e.stopPropagation(); setSelected(issue); }} using the existing setSelected and issue identifiers.
470-590:⚠️ Potential issue | 🟠 MajorUse an accessible dialog primitive for the modal.
The current overlay still lacks proper dialog semantics/focus trap/Escape handling, which blocks keyboard and screen-reader workflows. Please switch to an accessible dialog component (e.g., MUI
Dialog) or implement equivalent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/shipnoise/report.tsx` around lines 470 - 590, Replace the custom overlay Box/ Paper modal with an accessible MUI Dialog primitive: use Dialog open={Boolean(selected)} onClose={() => setSelected(null)} (so Escape and backdrop clicks close), move the header into DialogTitle (use aria-labelledby) and the details into DialogContent/ DialogActions (use aria-describedby if needed), preserve existing rendering logic that uses selected, buildDisplayFields(selected) and formatDateValue(...) inside the Dialog, and ensure the Close button calls setSelected(null); this gives proper dialog semantics, focus trap and keyboard handling without changing the display content.ui/src/pages/api/shipnoise-issues.ts (1)
8-11:⚠️ Potential issue | 🔴 CriticalRestrict this endpoint to authorized roles before exposing report data.
This route is still callable without auth checks and returns raw issue-report payloads. Please enforce moderator/admin authorization and return 401/403 for unauthorized callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/api/shipnoise-issues.ts` around lines 8 - 11, The exported API handler function handler currently exposes raw issue-report data with no auth; update handler to authenticate and authorize callers before returning data by verifying the request's session/token (e.g., via your existing auth utility or NextAuth getSession) and checking the user's role includes "moderator" or "admin"; return 401 when unauthenticated and 403 when authenticated but not authorized, and short-circuit so only authorized requests reach the code that reads/returns the report payload in handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/pages/api/shipnoise-issues.ts`:
- Around line 21-27: The timeout is being cleared immediately after fetch
resolves, so downstream body reads (response.text()) can still hang; move the
clearTimeout(timeout) so it runs only after the body is fully read (i.e., after
awaiting response.text()), or better wrap the fetch+body read in a try/finally
and clearTimeout(timeout) in the finally block to ensure the timer is cleared on
success or error; update references to clearTimeout(timeout), response.text(),
fetch(...), response.ok and rawBody accordingly.
---
Duplicate comments:
In `@ui/src/pages/api/shipnoise-issues.ts`:
- Around line 8-11: The exported API handler function handler currently exposes
raw issue-report data with no auth; update handler to authenticate and authorize
callers before returning data by verifying the request's session/token (e.g.,
via your existing auth utility or NextAuth getSession) and checking the user's
role includes "moderator" or "admin"; return 401 when unauthenticated and 403
when authenticated but not authorized, and short-circuit so only authorized
requests reach the code that reads/returns the report payload in handler.
In `@ui/src/pages/shipnoise/report.tsx`:
- Around line 446-460: The "View" Button is focusable but doesn't open the
details—add an onClick handler on this Button to call event.stopPropagation()
and then call setSelected(issue) so the row's details open directly from the
button; locate the Button component in report.tsx and wire onClick={(e) => {
e.stopPropagation(); setSelected(issue); }} using the existing setSelected and
issue identifiers.
- Around line 470-590: Replace the custom overlay Box/ Paper modal with an
accessible MUI Dialog primitive: use Dialog open={Boolean(selected)} onClose={()
=> setSelected(null)} (so Escape and backdrop clicks close), move the header
into DialogTitle (use aria-labelledby) and the details into DialogContent/
DialogActions (use aria-describedby if needed), preserve existing rendering
logic that uses selected, buildDisplayFields(selected) and formatDateValue(...)
inside the Dialog, and ensure the Close button calls setSelected(null); this
gives proper dialog semantics, focus trap and keyboard handling without changing
the display content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3fd4878-0502-4391-bb92-abe4498d4346
📒 Files selected for processing (2)
ui/src/pages/api/shipnoise-issues.tsui/src/pages/shipnoise/report.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Infrastructure
Utilities