feat/new edit flow monorepo#274
Draft
dnishiyama wants to merge 12 commits into
Draft
Conversation
…nused lines in test fixtures
dnishiyama
requested review from
evanpetzoldt,
pstaylor-patrick and
taterhead247
as code owners
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR introduces a comprehensive update-request workflow for managing location, AO, event, and region changes. It adds specialized form components for each operation (create, edit, move, delete), new API handlers and validators, a modal-driven UI for request submissions, extensive e2e test coverage, and refactors the modal system to support request-based operations alongside approval/rejection flows. ChangesUpdate Request Workflow Implementation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR is substantial and intricate, spanning the full stack from database schema and API request handling through to frontend form composition and e2e testing. The changes are heterogeneous across many domains (validators, handlers, modals, forms, tests, config), each requiring separate reasoning. The update-request workflow introduces several interdependent patterns and new request types, and the modal/form system refactor is a significant structural change. However, the changes follow coherent patterns within each layer, and the test coverage helps validate the implementation.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 72
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/map/src/app/_components/map/group-marker.tsx (1)
79-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish checks for dragged coordinates.
The guard currently rejects valid
0values (!lat || !lng). This can silently skip updates for legitimate coordinates.Proposed fix
- if (!lat || !lng) return; + if (lat == null || lng == null) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/src/app/_components/map/group-marker.tsx` around lines 79 - 97, The handleDragEnd callback rejects valid zero coordinates because it uses falsy checks; replace the guard so it only returns when lat or lng are null/undefined (e.g., check lat == null || lng == null) before calling mapStore.setState for id and showing the toast.success message, ensuring legitimate 0 values are accepted.packages/shared/src/app/functions.ts (1)
180-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove debug console.log statements.
Lines 180 and 199 contain debug logging that should be removed before merging:
- Line 180:
console.log("val", val);- Line 199:
console.log("val is string");🧹 Proposed fix
z.preprocess( (val) => { if (val === undefined || val === null) return undefined; - console.log("val", val); // If it's an array of strings (happens when query params are split) ... // If it's a string, parse it as JSON if (typeof val === "string") { - console.log("val is string"); try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/app/functions.ts` at line 180, Remove the two leftover debug console.log calls in packages/shared/src/app/functions.ts: the one that logs console.log("val", val) and the one that logs console.log("val is string"); locate them in the function handling value parsing/inspection (the scope referencing the variable val) and delete these statements (or replace with a proper logger call if structured logging is required), ensuring no other transient console.log debug statements remain in that module.apps/map/e2e/no-auth/manage-event-workflow.spec.ts (1)
41-47: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRedundant condition in the OR check.
The condition
!testAoButtonText?.toLowerCase().includes("test workout") || !testAoButtonText?.toLowerCase().includes("workout")has a redundant second clause. If text includes "test workout", it inherently includes "workout" as a substring. The condition simplifies to:if (!testAoButtonText?.toLowerCase().includes("test workout")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/e2e/no-auth/manage-event-workflow.spec.ts` around lines 41 - 47, The if-statement using testAoButtonText contains a redundant OR clause; update the condition in the test to only check for the full substring match by replacing the current conditional with a single check that testAoButtonText?.toLowerCase().includes("test workout") is false (i.e., if (!testAoButtonText?.toLowerCase().includes("test workout"))), keeping the early console.log("Exiting: testAoButtonText", testAoButtonText) and return behavior unchanged; locate and modify the conditional that references testAoButtonText in the manage-event-workflow.spec.ts e2e test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vscode/launch.json:
- Around line 27-30: Update the debug launch configuration to target the correct
app by changing the "cwd" setting from "apps/nextjs" to "apps/map" so the
runtimeExecutable ("pnpm") and runtimeArgs (["dev"]) will run the Next.js 15 map
UI; verify the "serverReadyAction" and any port settings still match the app
(port 3000) after the cwd change.
In `@apps/map/e2e/auth/admin-portal.spec.ts`:
- Around line 13-25: Replace the hard-coded prefix "SUPER TEST" with a
run-unique value and propagate it to the cleanup and test helpers: generate a
suffix (e.g., timestamp or short UUID) at the top of the suite and build prefix
= `SUPER TEST – ${suffix}`, then pass that prefix into cleanupTestData and any
test helpers that create fixtures; update references to the prefix variable in
this file (and any helper calls) so each test run uses a distinct prefix and
avoids cross-run deletions.
- Around line 274-276: The test builds dateString using toISOString() (variables
tomorrow and dateString) which yields UTC and can be off by a day; instead
construct YYYY-MM-DD from local parts: use tomorrow.getFullYear(),
(tomorrow.getMonth() + 1) and tomorrow.getDate(), pad month/day to two digits
(e.g., padStart) and join with '-' so dateString is the local-date string for
the form input.
In `@apps/map/e2e/no-auth/edit.spec.ts`:
- Line 5: Replace all arbitrary waitForTimeout calls in edit.spec.ts with
deterministic waits: use Playwright's locator.waitFor /
expect(locator).toBeVisible(), page.waitForResponse(), or page.waitForSelector()
tied to specific UI elements or network responses instead of timeouts (search
for waitForTimeout usages). If you cannot update all tests now, create a
tracking issue documenting each waitForTimeout occurrence and update the TODO to
reference that issue ID so the debt is tracked.
- Around line 16-21: The suite relies on test ordering causing fragile
interdependencies; make the intent explicit by converting the top-level
test.describe to test.describe.serial (replacing test.describe("Admin Map
Actions", ...) with test.describe.serial("Admin Map Actions", ...)) or
alternatively refactor the dependent tests into isolated fixtures/fixtures-based
setup so "New location, AO, & event" creates and yields its data for the
subsequent "Edit AO details" test; update the setup in test.beforeEach or create
a dedicated fixture to ensure deterministic state for the tests referenced by
their names.
- Line 78: The filePath resolution is fragile because it uses process.cwd();
update the filePath assignment in edit.spec.ts to resolve relative to the test
file instead (referencing the filePath variable and its path.resolve call) by
using __dirname (or for ESM use import.meta.url + fileURLToPath) and the
relative segment "e2e/tests/image.png" so the test finds the image regardless of
the current working directory; if you choose import.meta.url, add the
fileURLToPath import/utility and convert import.meta.url to a directory before
calling path.resolve.
In `@apps/map/e2e/no-auth/nearby-locations.spec.ts`:
- Line 11: The test in nearby-locations.spec.ts uses a hardcoded URL in the
page.goto call which reduces portability; replace await
page.goto("http://localhost:3000") with a relative navigation (e.g., await
page.goto("/") ) so Playwright will resolve against baseURL from configuration,
or alternatively read and use the configured baseURL when constructing the
target URL; update the page.goto call accordingly to remove the hardcoded
host/port.
- Line 19: Replace flaky uses of page.waitForTimeout by waiting for concrete UI
state changes: find each occurrence of page.waitForTimeout and instead wait for
the specific element or condition the test depends on (e.g., use await
page.waitForSelector(selector), await page.locator(<selector>).waitFor(), or
Playwright expect assertions like await
expect(page.locator(<selector>)).toBeVisible()/toHaveText(...)); do this for all
instances of page.waitForTimeout in the nearby-locations spec so the test
proceeds only after the expected element appears or the expected text/state is
reached.
In `@apps/map/e2e/no-auth/update-actions.spec.ts`:
- Around line 115-121: The test defines an unused variable logoPath and logs it
but then calls fileChooser.setFiles with a hardcoded string; either remove the
dead logoPath and its console.log, or use it by passing logoPath into
fileChooser.setFiles instead of the literal "public/f3_logo.png". Update the
lines around page.locator('input[name="aoLogo"]'), the logoPath declaration, and
the fileChooser.setFiles call so the test consistently uses the computed path or
drops the unused variable.
- Around line 20-31: The test.beforeAll hook currently creates a browser context
and page (variables context and _page) but does nothing because the
cleanupTestData call is commented out; remove the entire test.beforeAll block
(or uncomment and implement cleanupTestData if you intended to run cleanup) so
you no longer create unused context/_page and add unnecessary overhead;
specifically delete the test.beforeAll(...) function surrounding the
context/newPage/close calls (or restore the call to cleanupTestData(page, "MAP
TEST") inside it) and ensure no references to _page or context remain in
update-actions.spec.ts.
In `@apps/map/e2e/setup/auth.spec.ts`:
- Around line 25-26: The current check waits for the Settings button which is
visible pre-login; replace or augment that assertion with an auth-specific
selector that only appears after sign-in (e.g., check for a user avatar element
or a "Sign out" / "Log out" button). Locate the existing assertion using
page.getByRole("button", { name: "Settings" }) in the auth test and change it to
await expect(...) for a post-auth indicator such as page.getByRole("button", {
name: "Sign out" }) or page.getByAltText("User avatar") (or a project-specific
test id like getByTestId("user-avatar")) so the test reliably verifies
successful authentication.
In `@apps/map/src/app/_components/forms/form-inputs/ao-details-form.tsx`:
- Around line 61-82: Remove the console.log and wrap the entire image
processing/upload flow in a try/catch inside the onChange handler for the file
input: call scaleAndCropImage(file, 640, 640) and await uploadLogo(...) inside
the try, set the form value via onChange(url640) only on success, then attempt
the 64x64 upload in its own try/await (or fire-and-forget with its own catch) so
its failure doesn't leave the form in a partial state; on any caught error use
the existing logging mechanism to log the error and revert or clear the form
value (e.g., call onChange(null) or leave previous value) to avoid inconsistent
state. Reference symbols: scaleAndCropImage, uploadLogo, onChange,
formOriginalRegionId, formId.
In `@apps/map/src/app/_components/forms/form-inputs/controlled-time-input.tsx`:
- Line 18: Call useFormContext with the form values type to restore type-safety
(e.g., import your form values type, then change const form =
useFormContext<FormValues>();) so form and field.value are properly typed; after
that remove the eslint-disable comments that were suppressing type errors around
field.value usage. Ensure you import the correct FormValues/interface used by
your form and update any places referencing form or field.value accordingly.
In `@apps/map/src/app/_components/forms/form-inputs/delete-ao-form.tsx`:
- Around line 7-9: The DeleteAoForm component declares an unused generic
parameter <_T> but always types useFormContext to DeleteAOType; fix by removing
the unused generic from the DeleteAoForm declaration so it reads a non-generic
function, or alternatively make the component generic and pass that generic into
useFormContext (i.e., replace useFormContext<DeleteAOType>() with
useFormContext<_T>() and ensure callers supply the correct type). Update the
DeleteAoForm signature and any call sites accordingly to keep the type
consistent with useFormContext and eliminate the unused _T symbol.
In `@apps/map/src/app/_components/forms/form-inputs/delete-event-form.tsx`:
- Around line 7-8: The DeleteEventForm declares an unused generic parameter _T
but then calls useFormContext<DeleteEventType>() directly; either remove the
unused generic from the DeleteEventForm signature or make the form context
generic by using _T (e.g., replace explicit DeleteEventType with _T in the
useFormContext call). Locate the DeleteEventForm declaration and adjust the
signature or the useFormContext type parameter to eliminate the unused _T,
following the same fix applied in ExistingLocationPickerForm.
In `@apps/map/src/app/_components/forms/form-inputs/event-details-form.tsx`:
- Around line 179-183: The struck-through old-description preview can render an
empty row when currentValues is missing; update the JSX conditional in
event-details-form.tsx to only render the <p> when
currentValues?.eventDescription exists and is different from
formEventDescription (e.g., change the condition to check
currentValues?.eventDescription && currentValues.eventDescription !==
formEventDescription) so the preview is guarded by an existence check for
currentValues.eventDescription.
In `@apps/map/src/app/_components/forms/form-inputs/event-selector.tsx`:
- Around line 37-48: The selected event value can become stale when
regionId/aoId filters change; detect changes to
form.watch(regionFieldName)/form.watch(aoFieldName) or to the query result
(events) and clear/reset the form fieldName if the current
form.getValue(fieldName) is not present in events?.events. Update the component
around the useQuery/regionId/aoId logic (symbols: regionId, aoId, form.watch,
form.getValues/getValue, useQuery, events, fieldName) to run an effect on filter
or events change that sets form.setValue(fieldName, null/undefined) (or triggers
form.resetField(fieldName)) when the selected event id is missing from the
fetched events; apply the same fix to the analogous block referenced at lines
67-76.
In
`@apps/map/src/app/_components/forms/form-inputs/existing-location-picker-form.tsx`:
- Around line 19-24: The component ExistingLocationPickerForm declares an unused
generic parameter _T while always using ExistingLocationPickerFormValues with
useFormContext; remove the unused generic declaration from the component
signature (i.e., drop "<_T extends ExistingLocationPickerFormValues,>" so the
component is a plain function) and ensure any references still use
ExistingLocationPickerFormValues and
useFormContext<ExistingLocationPickerFormValues>() unchanged.
In `@apps/map/src/app/_components/forms/form-inputs/in-region-form.tsx`:
- Around line 48-52: Remove the `@ts-expect-error` and fix the type/schema
mismatch by making originalRegionId nullable in the request schema (change
originalRegionId: z.number().positive() to originalRegionId:
z.number().positive().nullish() in packages/validators/src/request-schemas.ts),
and update the form control to use the Controller pattern instead of forcing
types: replace the direct form.setValue(...) usage for "originalRegionId" with
the Controller's field.onChange(region?.id ?? null) so the UI can clear the
region and the schema will accept null.
In `@apps/map/src/app/_components/forms/form-inputs/location-details-form.tsx`:
- Around line 29-127: The labels are plain divs so inputs lose semantic
association; replace each label div with a <label htmlFor="..."> (or the shared
label component) and add matching id attributes to the corresponding controls
used by form.register (e.g., inputs registered as "locationAddress",
"locationAddress2", "locationCity", "locationState", "locationZip",
"locationLat", "locationLng", and the textarea "locationDescription") and ensure
the Controller for "locationCountry" renders CountrySelect with an id prop that
matches htmlFor; update CountrySelect usage and Textarea/Input props to accept
and pass the id so screen readers and click-to-focus work.
In `@apps/map/src/app/_components/forms/form-inputs/selection-form.tsx`:
- Around line 28-43: SelectionForm is marked deprecated and should not be added
as a public component; remove the exported SelectionForm (and related types
SelectionFormValues / SelectionFormProps usage) from the public codebase: either
delete this component entirely and remove all imports/usages, or if you must
keep it for transition, move it into a legacy/internal folder, update its JSDoc
to remain deprecated, and ensure it is not re-exported from public modules (so
RegionSelector, AOSelector, EventSelector, etc. are the supported public APIs).
- Around line 41-50: Remove the unused generic parameter by changing the
component signature from SelectionForm = <_T extends SelectionFormValues>(props:
SelectionFormProps) => to a plain SelectionForm(props: SelectionFormProps) =>,
and delete the unused watch variables _formOriginalRegionId, _formOriginalAOId,
_formOriginalEventId and _formNewEventId (the consts that call form.watch and
are prefixed with underscores) to avoid unnecessary re-renders; keep the used
watchers (formNewRegionId and formNewAOId) intact.
In `@apps/map/src/app/_components/forms/submit-section.tsx`:
- Around line 162-178: rejectForm currently toasts and closes the modal after
calling rejectSubmission but doesn't refresh cached request/map data; update
rejectForm so that after a successful rejectSubmission it triggers the same
cache refresh used by the approve flow (e.g., call the same invalidate/refetch
helper or react-query/SWR invalidation used by approveSubmission), then toast
and closeModal; locate rejectForm and add the cache invalidation call (using the
same symbol/function the approve path uses) before closeModal to ensure
tables/details are refreshed.
In `@apps/map/src/app/_components/map/desktop-location-panel-content.tsx`:
- Around line 11-15: Move the pure formatting utilities formatTime and
getShortDayOfWeek out of the LocationEditButtons module into a small dedicated
utils module (e.g., location-utils) and export them from there; then update
references/imports in LocationEditButtons and DesktopLocationPanelContent to
import formatTime and getShortDayOfWeek from the new utils module while keeping
LocationEditButtons as the component export, ensuring no behavior changes.
In `@apps/map/src/app/_components/map/location-edit-buttons.tsx`:
- Around line 68-176: The event dropdown is accidentally nested inside the AO
DropdownMenu causing an invalid trigger/content ordering; extract the entire
conditional block that checks eventId and renders the inner DropdownMenu (the
block using DropdownMenu, DropdownMenuTrigger, DropdownMenuContent and calling
openRequestModal for edit/move/delete actions) and move it so it is a sibling of
the AO dropdown (i.e., place the eventId conditional block before or after the
outer DropdownMenu that contains the AO DropdownMenuTrigger), ensuring each
DropdownMenu's DropdownMenuTrigger and DropdownMenuContent remain paired and
contiguous.
In `@apps/map/src/app/_components/map/update-pane.tsx`:
- Around line 146-152: The close button in update-pane.tsx (the button that
calls clearUpdateLocation and renders the X icon) is missing an accessible name
and a button type; update that button element to include type="button" and an
aria-label (e.g., aria-label="Clear update location") so assistive tech can
announce it while keeping the existing onClick/onTouchEnd handlers and X icon
intact.
- Around line 111-149: The handlers are bound twice for touch devices causing
duplicate invocations; remove the onTouchEnd attributes and keep only the native
onClick handlers on the affected controls (the Button components using
handleCreateNew, handleMoveAO, handleMoveEvent and the plain button using
clearUpdateLocation) so each action is invoked once via the regular click path;
edit the JSX to delete the onTouchEnd={...} occurrences for those functions and
ensure no other touch event bindings remain for these controls.
In `@apps/map/src/app/_components/modal/admin-workouts-modal.tsx`:
- Around line 66-70: The current regex used in the startTime and endTime
refinements accepts invalid times like "29:99"; update the validations in the
schema (the z.string().refine calls for startTime and endTime in
admin-workouts-modal.tsx) to enforce true 24-hour bounds (hours 00–23 and
minutes 00–59) by replacing the permissive pattern with a strict 24h pattern
(e.g., require a regex that matches ([01]\d|2[0-3]):[0-5]\d) or equivalent
logic, and keep the existing error messages.
In `@apps/map/src/app/_components/modal/loading-modal.tsx`:
- Around line 13-17: The Dialog currently calls closeModal in its onOpenChange
handler which allows dismissing the loading modal; to prevent user dismissal,
make the Dialog fully controlled for loading by removing or no-op'ing the
onOpenChange that calls closeModal (or use the Dialog API to set
closeOnOutsideClick/closeOnEscape/disableEscapeKey to false depending on the
Dialog implementation) and ensure open is true while loading; if instead
dismissal should be allowed, add an inline comment next to the Dialog/open or
closeModal call documenting why users may close a loading modal.
In `@apps/map/src/app/_components/modal/modal-switcher.tsx`:
- Line 22: Import name and exported component are inconsistent:
modal-switcher.tsx imports AdminUsersModal but admin-users-modal.tsx
default-exports a function named UserModal; pick one approach and apply it
project-wide: either rename the component function in admin-users-modal.tsx from
"UserModal" to "AdminUsersModal" and keep the default export (update any
internal references), or change the import in modal-switcher.tsx to "UserModal"
to match the existing export; ensure you update any other files that import this
module so names remain consistent.
In
`@apps/map/src/app/_components/modal/update/create-ao-and-location-and-event-modal.tsx`:
- Line 25: Remove the unguarded console.log that prints modal default
values—specifically the console.log("CreateAOAndLocationAndEventModal data",
data) call in CreateAOAndLocationAndEventModal; either delete this line entirely
or wrap it with the project's existing non-production check (e.g., the same
NODE_ENV or isDevelopment guard used elsewhere) so it never logs sensitive
user/location fields in production.
In `@apps/map/src/app/_components/modal/update/delete-ao-modal.tsx`:
- Line 31: The form in the DeleteAoModal component is missing an onSubmit
handler which can cause unintended page submissions; update the <form> in
delete-ao-modal.tsx (in the DeleteAoModal component or enclosing JSX) to include
onSubmit={(e) => e.preventDefault()} so the modal buttons don't trigger a full
form submit — mirror the same pattern used by other modals.
In `@apps/map/src/app/_components/modal/update/edit-ao-and-location-modal.tsx`:
- Line 41: The form in the EditAoAndLocationModal component is missing an
onSubmit handler and can trigger unintended submissions; update the <form
className="w-[inherit] overflow-x-hidden p-0.5"> in
edit-ao-and-location-modal.tsx to include onSubmit={(e) => e.preventDefault()}
(or a named handler that calls e.preventDefault()) so the form won't perform a
full-page submit when Enter is pressed or buttons are clicked.
- Around line 29-36: In handleSubmission, don't throw after calling
form.setError since that can double-report errors; instead stop the submission
by returning early when the badImage check fails (i.e., remove "throw new Error"
and return before calling client.request.submitEditAOAndLocationRequest),
keeping the form.setError call to surface validation to the user.
In
`@apps/map/src/app/_components/modal/update/move-ao-to-different-location-modal.tsx`:
- Around line 34-40: The JSX block in move-ao-to-different-location-modal.tsx is
rendering data?.originalAoId twice; remove the duplicate <p> that displays
originalAoId (the second occurrence) and, if a different property was intended
there, replace it with the correct data property (use the same
data?.<propertyName> pattern) so only the correct fields (e.g.,
data?.originalAoId, data?.originalLocationId, data?.newLocationId,
data?.originalRegionId) are shown.
In
`@apps/map/src/app/_components/modal/update/move-event-to-different-ao-modal.tsx`:
- Line 33: The form in the MoveEventToDifferentAoModal component is missing an
onSubmit handler; update the <form className="w-[inherit] overflow-x-hidden
p-0.5"> in the MoveEventToDifferentAoModal component to include onSubmit={(e) =>
e.preventDefault()} (or a named handler that calls e.preventDefault()) to
prevent unintended page reloads/submissions when buttons inside the modal are
clicked.
- Around line 27-28: In the MoveEventToDifferentAoModal component remove the two
debug console.log statements that print "MoveEventToDifferentAoModal data" and
"MoveEventToDifferentAoModal form" (the lines referencing data and
form.watch()); simply delete those console.log calls so they no longer run on
every render or form change (if you need runtime-only logging for development,
replace with a conditional debug guard like checking NODE_ENV or a debug flag
instead).
- Around line 36-41: The debug div in MoveEventToDifferentAoModal exposing raw
IDs (data?.originalEventId, data?.originalAoId, data?.newAoId,
data?.originalRegionId) should be removed or wrapped in a non-production guard;
either delete the entire <div> block or render it only when !isProductionNodeEnv
(or replace it with the existing FormDebugData component) inside the
MoveEventToDifferentAoModal component so internal IDs are not shown in
production.
In
`@apps/map/src/app/_components/modal/update/move-event-to-new-location-modal.tsx`:
- Line 31: The <form> in move-event-to-new-location-modal.tsx is missing an
onSubmit handler which can cause unintended page reloads when the submit button
is clicked; update the form element in the MoveEventToNewLocationModal component
to handle submit by adding onSubmit={(e) => e.preventDefault()} (or delegate to
an existing submit handler) or alternatively ensure the SubmitSection
component's button is explicitly type="button" so the browser's default form
submission is prevented; reference the form element and the SubmitSection
component when making the change.
In `@apps/map/src/app/_components/modal/utils/handle-submission-error.ts`:
- Around line 32-34: The Object.entries call that builds errorMessages
incorrectly types the input as an array ({ message: string; type: string }[])
even though Object.entries expects an object; update the type for the value
passed to Object.entries (the variable named error in
handle-submission-error.ts) to a suitable record type such as Record<string, {
message?: string; type?: string }> or unknown and narrow it before calling
Object.entries, and update the errorMessages assignment to use that corrected
type so TypeScript correctly infers the entries.
- Around line 48-57: The fallback logic in handleSubmissionError contains a
redundant check '!(error instanceof ORPCError)' that is unreachable because
ORPCError is already handled earlier; simplify by removing that redundant branch
and collapse the final else into a single fallback: if error is an ORPCError
handle accordingly, else if error is an Error use error.message, otherwise log
the non-Error value and set a generic "Failed to submit update request" message
(reference the error variable and ORPCError handling in handleSubmissionError to
locate where to adjust the branches).
In `@apps/map/src/app/_components/modal/workout-details-modal.tsx`:
- Line 53: Remove the debug console.log in the WorkoutDetailsModal component in
workout-details-modal.tsx — specifically delete the line console.log("aoId",
aoId, modalAOIds, results); (or replace it with a proper, gated logger if
needed) so internal data is not emitted to the browser console; locate this
inside the WorkoutDetailsModal render/effect where aoId, modalAOIds, and results
are referenced.
In `@apps/map/src/app/_components/virtualized-combobox.tsx`:
- Around line 71-77: The comparator used to sort options violates antisymmetry
by returning -1 when both items are selected; update the sort callback in
virtualized-combobox.tsx (the comparator that receives a and b and uses
selectedOptions/includes) to implement a proper total order: first compute
booleans isASelected = selectedOptions.includes(a.value) and isBSelected =
selectedOptions.includes(b.value), then if isASelected !== isBSelected return
isASelected ? -1 : 1; otherwise use a deterministic tiebreaker (e.g., compare
a.label or a.value or original index) and return -1/1 accordingly, returning 0
only when they are truly equal. Ensure the comparator now satisfies cmp(a,b) ===
-cmp(b,a).
In `@apps/map/src/app/admin/requests/requests-table.tsx`:
- Around line 95-105: The onRowClick handler calls client.request.byId and can
throw on network failures; wrap the async call in a try/catch inside the
onRowClick callback (the function passed to onRowClick) so thrown errors are
caught, call toast.error with a user-friendly message (and optionally include
error.message) when an exception occurs, and only call openRequestModal when
response.request exists; also consider logging the error for debugging
(reference: onRowClick, client.request.byId, openRequestModal, toast.error).
In `@apps/map/src/utils/open-request-modal.ts`:
- Around line 80-89: In the switch case for "move_event_to_new_location" where
openModal is called with ModalType.MOVE_EVENT_TO_NEW_LOCATION, remove the
duplicate spread of updateLocation (it is spread twice) so the payload merges
formValues, a single updateLocation, requestType, currentValues, and
params.meta; update the openModal call to include only one ...updateLocation to
eliminate the redundancy.
- Around line 406-408: Remove the duplicate loc check in the if condition inside
apps/map/src/utils/open-request-modal.ts where ev is set from loc.events (the
block that tests !ev, loc, req?.originalEventId, loc); simplify the guard to
only check for !ev, loc, and req?.originalEventId before assigning ev from
loc.events.find(e => e.id === req?.originalEventId) so the condition no longer
includes the redundant second loc.
In `@packages/api/src/__test__/mock/db.ts`:
- Around line 28-37: The mock implementation for mockValues uses Date.now() to
generate fallback IDs in the anonymous mockImplementation, which can collide
during rapid test runs; replace the time-based ID with a reliably unique
mechanism by introducing a closure-scoped incremental counter or using a UUID
generator and use that to build the id when (data.id as string) is falsy—update
the mockImplementation inside mockValues so the fallback id is produced from the
new counter/UUID (ensure the counter is preserved across calls in the module
scope or import and call the UUID function where the id is generated).
- Around line 5-13: The module-level variable `mockDatabase` causes shared state
across tests; change `createMockDb` to return a new Map instance (the database)
instead of assigning to a module variable, and update any helper functions
(e.g., `getMockDatabase`, `clearMockDatabase` or other utilities) to accept that
returned Map as a parameter so each test can own its database instance;
alternatively ensure tests call the new `createMockDb()` in beforeEach and pass
the instance into helpers rather than relying on module-level `mockDatabase`.
In `@packages/api/src/__test__/update-request.test.ts`:
- Around line 811-841: The test is incorrectly exercising mock helpers instead
of the real rejection logic; update the test to call the actual rejection
handler (e.g., rejectSubmission or handleRejectRequest) after creating the
pending request with recordUpdateRequest/createEventRequest, then assert that
mockDb.update/mockSet/mockWhere were invoked by that handler and that the
request status in DB became "rejected"; if no handler exists, rename the test to
reflect it only exercises mock plumbing or implement a handler function and
import it into the test before invoking it.
In `@packages/api/src/lib/ao-handlers.ts`:
- Line 31: Remove the debug console.log("createAO", { regionId, locationId,
aoName }) from ao-handlers.ts; either delete it or replace it with the project's
structured logger (e.g., processLogger.info or logger.info) emitting a
structured message and context (regionId, locationId, aoName) inside the
createAO handler so observability uses the standard logger instead of
console.log.
- Around line 34-42: The createAO insertion currently defaults aoName to an
empty string in the .values call (name: aoName ?? ""), which permits creating
AOs with empty names that violate the existing AO name validators; update
createAO to require or validate aoName before insertion: either throw/return an
error when aoName is missing/too short (use the same min-length rules as your
validators) or change the .values call to use a non-null asserted aoName (e.g.,
fail fast) so only valid names reach the database; locate the insertion in
createAO/ao-handlers.ts and apply the validation/guard there, referencing aoName
and the .values({ name: ... }) entry.
In `@packages/api/src/lib/check-update-permissions.ts`:
- Around line 25-31: The unauthenticated branch in check-update-permissions
(when session is falsy) returns an object missing the results property, causing
callers that access permissions.results to break; update that return to include
a consistent results key (e.g., results: [] or whatever the normal results type
is) alongside success, orgId, roleName, and mode so the function always returns
the same shape.
In `@packages/api/src/lib/event-handlers.ts`:
- Around line 35-52: The updateEvent handler is force-setting isActive: true and
highlight: false which unintentionally reactivates soft-deleted events and
clears highlights; change the update logic in updateEvent (the .set block that
uses updateRequest.* fields) to only include isActive and highlight when the
incoming updateRequest explicitly provides those values (e.g., eventIsActive,
eventHighlight) or otherwise omit them so existing DB values are preserved — do
not unconditionally set isActive or highlight from within updateEvent.
- Around line 190-202: The delete-then-insert for schema.eventsXEventTypes is
not atomic: if the insert fails the event loses all type associations; wrap the
delete and the conditional insert into a single database transaction using the
DB transaction API (use ctx.db.transaction or the project's transaction helper)
and run the delete against the transaction handle (e.g.,
tx.delete(schema.eventsXEventTypes).where(eq(...))) and the insert via
tx.insert(schema.eventsXEventTypes).values(...); ensure you still skip the
insert when eventTypeIds is empty but keep both operations inside the same
transaction so replacement is all-or-nothing.
- Around line 88-93: The newEvent.startDate default is using new
Date().toISOString() which yields a timestamp; change it to use
dayjs().format("YYYY-MM-DD") to match the PostgreSQL date column and the
existing handleEvent() behavior. Update the newEvent construction (the object
assigned to newEvent) to use dayjs().format("YYYY-MM-DD") for startDate and
ensure dayjs is imported in this file if it isn't already (refer to newEvent and
schema.events.startDate).
In `@packages/api/src/lib/location-handlers.ts`:
- Around line 76-87: The update path for updateLocation is missing the name
column so renames are ignored; update the .set call in the updateLocation
handler to include the name field by mapping it to updateRequest.locationName
(e.g., add name: updateRequest.locationName ?? undefined) alongside the other
address/coordinate/email properties so the database persists locationName
updates.
In `@packages/api/src/lib/update-request-handlers.ts`:
- Around line 313-338: The code always uses request.originalRegionId when
creating the replacement location and AO; change both calls to honor a
destination region by using request.newRegionId when present (e.g. set regionId
to request.newRegionId || request.originalRegionId) for the insertLocation(...)
call and the createAO(...) call (references: locationId, insertLocation,
createAO, request.newRegionId, request.originalRegionId).
- Around line 132-174: This multi-step creation (insertLocation, createAO,
insertEvent, updateEventTypes) must be executed inside a single DB transaction
so all writes commit or roll back together; update the handler to start a
transaction via your DB/context transaction API (e.g., ctx.transaction or
db.transaction), move the calls to insertLocation, createAO, insertEvent and
updateEventTypes into the transaction callback/closure, capture location.id and
event.id from transactional results, and ensure the transaction rolls back on
any failure; apply the same transactional pattern to the other multi-write
handlers that perform multiple related inserts/updates.
- Around line 101-123: The current logs in recordUpdateRequest (the console.log
after insert/update and the console.error in the catch block) may dump sensitive
fields from updateRequestInsertData (e.g., submittedBy, addresses, request
metadata); before logging, create a sanitized version of updateRequestInsertData
that removes or masks fields like submittedBy, any address fields, and
request-specific metadata, and log only that sanitized object plus stable
identifiers/status (e.g., requestId, updated, error message). Update the two
logging sites that reference updateRequestInsertData (the success log with
updated and the error log with error/requestId/updateRequestInsertData) to use
the sanitized payload instead. Ensure the sanitizer is applied consistently for
both success and error paths and reference the variables
updateRequestInsertData, updated, requestId, and the function handling the DB
operations (insert(...).onConflictDoUpdate(...).returning()) when making the
change.
- Around line 263-295: When moving an AO you currently only update
schema.events.locationId; also update the AO's own record so it points to the
new location. In handleMoveAOToNewLocation, after you obtain location.id
(locationId) add an update to the AO/organization table to set its locationId
where its id equals request.originalAoId; likewise in
handleMoveAOToDifferentLocation update the AO/organization record to set
locationId = request.newLocationId where id = request.originalAoId. Use the same
ctx.db.update(...) pattern you used for schema.events and ensure you reference
request.originalAoId, locationId, and request.newLocationId in the respective
handlers.
In `@packages/api/src/router/map/location.ts`:
- Around line 647-698: The getAOsInRegion protectedProcedure is missing an
explicit .output() schema; add an .output(...) call on the same chain as
getAOsInRegion that returns an object with aos: z.array(...) describing each AO
record (include id: z.number(), parentId: z.number().nullable() or z.number(),
name: z.string().nullable(), orgType: z.string(), defaultLocationId:
z.number().nullable(), description: z.string().nullable(), isActive:
z.boolean(), logoUrl: z.string().nullable(), website: z.string().nullable(),
email: z.string().nullable(), twitter: z.string().nullable(), facebook:
z.string().nullable(), instagram: z.string().nullable(), lastAnnualReview:
z.string().nullable() or z.date().nullable(), meta: z.any(), created:
z.string().or(z.date()), workouts: z.array(z.string())), matching the selected
fields from the SQL select (schema.orgs.* and workouts SQL) so types align with
the ctx.db select in getAOsInRegion and ensure consumers and docs get correct
types.
In `@packages/api/src/router/request.ts`:
- Around line 660-719: These protectedProcedure endpoints
(submitEditEventRequest, submitEditAOAndLocationRequest,
submitMoveAOToDifferentRegionRequest, submitMoveAOToDifferentLocationRequest,
submitMoveAOToNewLocationRequest, submitMoveEventToDifferentAoRequest,
submitMoveEventToNewAoRequest, submitMoveEventToNewLocationRequest,
submitDeleteEventRequest, submitDeleteAORequest) are missing .route() metadata
so they won't be exposed in the OpenAPI REST spec; update each procedure to
mirror the pattern used by submitCreateAOAndLocationAndEventRequest and
submitCreateEventRequest by appending .route({ method: "...", path: "...", tags:
["..."], summary: "...", description: "..." }) (choose appropriate HTTP method
and path for each action), keeping the existing .input(...) and .handler(...)
(e.g., leave handleEditEvent, handleMoveAOToNewLocation, handleDeleteAO, etc.
intact) so the endpoints are properly published to the OpenAPI schema and
accessible via REST.
In `@packages/db/drizzle/0014_lucky_captain_marvel.sql`:
- Line 4: The UPDATE statement updating "public"."update_requests" should place
the WHERE clause on its own line for readability; modify the statement that sets
"request_type" = 'edit' (targeting rows where "request_type" =
'create_location') so the WHERE clause starts on a new line after the SET
clause.
In `@packages/db/src/migrate.ts`:
- Around line 17-20: Currently the await createDatabaseIfNotExists(databaseUrl)
call swallows errors by catching and logging them, letting execution proceed
into reset() and migrator() with unclear failures; remove the .catch or re-throw
the caught error so the process fails fast: update the block that calls
createDatabaseIfNotExists(databaseUrl) to either await it without .catch or, if
keeping the catch, call throw err after logging so the error propagates and
prevents subsequent calls to reset() and migrator() from running on a failed
precondition.
In `@packages/env/src/index.ts`:
- Around line 29-34: The console.log in the block checking
vercelInfo?.isPreviewDeployment and vercelInfo.gitCommitRef prints
vercelInfo.databaseUrl, which can expose credentials; update the log in that
branch (the console.log that references vercelInfo.databaseUrl) to avoid
printing the full connection string—either log a simple message like "Overriding
DATABASE_URL with vercel.databaseUrl" without the value or redact sensitive
parts (e.g., only show hostname or "REDACTED") before logging, keeping the
return value as vercelInfo.databaseUrl.
In `@packages/shared/src/common/constants.ts`:
- Around line 35-45: The module currently mutates process.env at import time by
reading process.argv and setting VERCEL and VERCEL_GIT_COMMIT_REF (logic using
previewBranchDbInitFlagIndex), causing unwanted side effects; refactor this by
extracting that logic into a new exported function (e.g., initPreviewBranchEnv
or setPreviewBranchEnv) which encapsulates the process.argv parsing and only
sets process.env when explicitly called, remove the top-level execution so
importing the module no longer mutates globals, and update any callers/tests to
invoke the new function where the previous implicit behavior was relied upon.
In `@packages/validators/src/request-schemas.ts`:
- Around line 170-180: The schema MoveEventToDifferentAOSchema currently marks
newAoId optional but handleMoveEventToDifferentAO() expects a target AO; change
the schema so newAoId is required (remove .optional()) and keep
newRegionId/newLocationId as optional if allowed, ensuring validation fails when
newAoId is missing; update any tests or callers that relied on it being optional
to pass a newAoId.
- Around line 187-200: MoveEventToNewAOSchema currently merges LocationFields
unconditionally which forces new-location fields even when newLocationId is
provided; change the schema so LocationFields are only required when
newLocationId is absent (i.e., when creating a new location). Implement this by
replacing the direct .merge(LocationFields) with a union or conditional schema:
one branch where newLocationId is a positive nullable number and LocationFields
are optional/partial, and the other branch where newLocationId is null/absent
and LocationFields are required; reference MoveEventToNewAOSchema,
newLocationId, LocationFields and ensure the resulting schema matches
handleMoveEventToNewAO() expectations.
In `@README.md`:
- Line 188: Fix the duplicated word in the WIP bullet "decide which things you
can change region and and which you can't" by removing the extra "and" and
optionally adding a comma for clarity so it reads e.g. "decide which things you
can change region and which you can't" or "decide which things you can change,
and which you can't"; update the sentence in README.md accordingly.
In `@tooling/scripts/src/notify-outstanding-requests.ts`:
- Line 115: The code is normalizing a missing request.eventName to an empty
string (eventName: request.eventName ?? ""), which prevents later fallback logic
(e.g., ?? "Unknown") from running and can render a blank name; change the object
assignment to pass through the original value instead (use eventName:
request.eventName or eventName: request.eventName ?? undefined) so downstream
"Unknown" fallback works; update the object that sets eventName (reference
request.eventName and the eventName property) accordingly.
---
Outside diff comments:
In `@apps/map/e2e/no-auth/manage-event-workflow.spec.ts`:
- Around line 41-47: The if-statement using testAoButtonText contains a
redundant OR clause; update the condition in the test to only check for the full
substring match by replacing the current conditional with a single check that
testAoButtonText?.toLowerCase().includes("test workout") is false (i.e., if
(!testAoButtonText?.toLowerCase().includes("test workout"))), keeping the early
console.log("Exiting: testAoButtonText", testAoButtonText) and return behavior
unchanged; locate and modify the conditional that references testAoButtonText in
the manage-event-workflow.spec.ts e2e test.
In `@apps/map/src/app/_components/map/group-marker.tsx`:
- Around line 79-97: The handleDragEnd callback rejects valid zero coordinates
because it uses falsy checks; replace the guard so it only returns when lat or
lng are null/undefined (e.g., check lat == null || lng == null) before calling
mapStore.setState for id and showing the toast.success message, ensuring
legitimate 0 values are accepted.
In `@packages/shared/src/app/functions.ts`:
- Line 180: Remove the two leftover debug console.log calls in
packages/shared/src/app/functions.ts: the one that logs console.log("val", val)
and the one that logs console.log("val is string"); locate them in the function
handling value parsing/inspection (the scope referencing the variable val) and
delete these statements (or replace with a proper logger call if structured
logging is required), ensuring no other transient console.log debug statements
remain in that module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8e1998a-f7e3-4350-ac42-c0525925561b
⛔ Files ignored due to path filters (2)
apps/map/e2e/tests/image.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (111)
.claude/skills/logs/SKILL.md.vscode/launch.jsonAGENTS.mdREADME.mdapps/api/package.jsonapps/map/e2e/auth/admin-portal.spec.tsapps/map/e2e/helpers.tsapps/map/e2e/no-auth/edit.spec.tsapps/map/e2e/no-auth/initial-load.spec.tsapps/map/e2e/no-auth/manage-event-workflow.spec.tsapps/map/e2e/no-auth/nearby-locations.spec.tsapps/map/e2e/no-auth/update-actions.spec.tsapps/map/e2e/setup/auth.spec.tsapps/map/package.jsonapps/map/playwright.config.tsapps/map/playwright.staging.config.tsapps/map/src/app/_components/forms/dev-debug-component.tsxapps/map/src/app/_components/forms/form-inputs/ao-details-form.tsxapps/map/src/app/_components/forms/form-inputs/ao-selector.tsxapps/map/src/app/_components/forms/form-inputs/contact-details-form.tsxapps/map/src/app/_components/forms/form-inputs/controlled-time-input.tsxapps/map/src/app/_components/forms/form-inputs/delete-ao-form.tsxapps/map/src/app/_components/forms/form-inputs/delete-event-form.tsxapps/map/src/app/_components/forms/form-inputs/event-details-form.tsxapps/map/src/app/_components/forms/form-inputs/event-selector.tsxapps/map/src/app/_components/forms/form-inputs/existing-location-picker-form.tsxapps/map/src/app/_components/forms/form-inputs/in-region-form.tsxapps/map/src/app/_components/forms/form-inputs/location-details-form.tsxapps/map/src/app/_components/forms/form-inputs/region-and-ao-selector.tsxapps/map/src/app/_components/forms/form-inputs/region-and-event-selector.tsxapps/map/src/app/_components/forms/form-inputs/region-ao-event-selector.tsxapps/map/src/app/_components/forms/form-inputs/region-selector.tsxapps/map/src/app/_components/forms/form-inputs/selection-form.tsxapps/map/src/app/_components/forms/location-event-form.tsxapps/map/src/app/_components/forms/submit-section.tsxapps/map/src/app/_components/map/desktop-location-panel-content.tsxapps/map/src/app/_components/map/group-marker.tsxapps/map/src/app/_components/map/initial-location-provider.tsxapps/map/src/app/_components/map/layout-edit-button.tsxapps/map/src/app/_components/map/location-edit-buttons.tsxapps/map/src/app/_components/map/update-pane.tsxapps/map/src/app/_components/marker-clusters/clustered-markers.tsxapps/map/src/app/_components/modal/admin-delete-request-modal.tsxapps/map/src/app/_components/modal/admin-requests-modal.tsxapps/map/src/app/_components/modal/admin-workouts-modal.tsxapps/map/src/app/_components/modal/base-modal.tsxapps/map/src/app/_components/modal/country-select.tsxapps/map/src/app/_components/modal/delete-modal.tsxapps/map/src/app/_components/modal/loading-modal.tsxapps/map/src/app/_components/modal/modal-switcher.tsxapps/map/src/app/_components/modal/update-location-modal.tsxapps/map/src/app/_components/modal/update/create-ao-and-location-and-event-modal.tsxapps/map/src/app/_components/modal/update/create-event-modal.tsxapps/map/src/app/_components/modal/update/delete-ao-modal.tsxapps/map/src/app/_components/modal/update/delete-event-modal.tsxapps/map/src/app/_components/modal/update/edit-ao-and-location-modal.tsxapps/map/src/app/_components/modal/update/edit-event-modal.tsxapps/map/src/app/_components/modal/update/move-ao-to-different-location-modal.tsxapps/map/src/app/_components/modal/update/move-ao-to-different-region-modal.tsxapps/map/src/app/_components/modal/update/move-ao-to-new-location-modal.tsxapps/map/src/app/_components/modal/update/move-event-to-different-ao-modal.tsxapps/map/src/app/_components/modal/update/move-event-to-new-ao-modal.tsxapps/map/src/app/_components/modal/update/move-event-to-new-location-modal.tsxapps/map/src/app/_components/modal/utils/handle-submission-error.tsapps/map/src/app/_components/modal/workout-details-modal.tsxapps/map/src/app/_components/virtualized-combobox.tsxapps/map/src/app/_components/workout/workout-details-content.tsxapps/map/src/app/admin/requests/requests-table.tsxapps/map/src/app/auth/layout.tsxapps/map/src/utils/forms.tsapps/map/src/utils/open-request-modal.tsapps/map/src/utils/secondary-effects-provider.tsxapps/map/src/utils/store/modal.tsapps/map/src/utils/types.tsapps/map/tests/helpers.tsapps/map/vitest.config.tspackages/api/package.jsonpackages/api/src/__test__/fixtures/index.tspackages/api/src/__test__/fixtures/update-request.tspackages/api/src/__test__/mock/context.tspackages/api/src/__test__/mock/db.tspackages/api/src/__test__/mock/index.tspackages/api/src/__test__/mock/session.tspackages/api/src/__test__/update-request.test.tspackages/api/src/__tests__/test-utils.tspackages/api/src/check-has-role-on-org.tspackages/api/src/lib/ao-handlers.tspackages/api/src/lib/check-update-permissions.tspackages/api/src/lib/event-handlers.tspackages/api/src/lib/location-handlers.tspackages/api/src/lib/types.tspackages/api/src/lib/update-request-handlers.tspackages/api/src/router/map/location.tspackages/api/src/router/request.tspackages/api/vitest.config.tspackages/db/drizzle/0014_lucky_captain_marvel.sqlpackages/db/drizzle/meta/0014_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/drizzle/schema.tspackages/db/src/migrate.tspackages/env/package.jsonpackages/env/src/index.tspackages/shared/src/app/constants.tspackages/shared/src/app/enums.tspackages/shared/src/app/functions.tspackages/shared/src/common/constants.tspackages/shared/src/common/enums.tspackages/ui/src/form.tsxpackages/validators/src/index.tspackages/validators/src/request-schemas.tstooling/scripts/src/notify-outstanding-requests.ts
💤 Files with no reviewable changes (7)
- apps/map/src/app/_components/modal/admin-requests-modal.tsx
- apps/map/src/app/_components/modal/delete-modal.tsx
- apps/map/src/utils/forms.ts
- apps/map/src/app/_components/modal/admin-delete-request-modal.tsx
- apps/map/src/app/_components/modal/update-location-modal.tsx
- apps/map/src/app/_components/forms/location-event-form.tsx
- apps/map/tests/helpers.ts
Comment on lines
+27
to
+30
| "cwd": "${workspaceFolder}/apps/nextjs", | ||
| "runtimeExecutable": "pnpm", | ||
| "runtimeArgs": ["dev"], | ||
| "serverReadyAction": { |
There was a problem hiding this comment.
Debug configuration points to the wrong app directory.
At Line 27, cwd is set to apps/nextjs, but this PR’s app context is apps/map. This launch config will likely fail or target the wrong workspace.
Suggested fix
- "cwd": "${workspaceFolder}/apps/nextjs",
+ "cwd": "${workspaceFolder}/apps/map",
"runtimeExecutable": "pnpm",
- "runtimeArgs": ["dev"],
+ "runtimeArgs": ["dev:inspect"],Based on learnings: The apps/map directory contains the Next.js 15 map UI (port 3000).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "cwd": "${workspaceFolder}/apps/nextjs", | |
| "runtimeExecutable": "pnpm", | |
| "runtimeArgs": ["dev"], | |
| "serverReadyAction": { | |
| "cwd": "${workspaceFolder}/apps/map", | |
| "runtimeExecutable": "pnpm", | |
| "runtimeArgs": ["dev:inspect"], | |
| "serverReadyAction": { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/launch.json around lines 27 - 30, Update the debug launch
configuration to target the correct app by changing the "cwd" setting from
"apps/nextjs" to "apps/map" so the runtimeExecutable ("pnpm") and runtimeArgs
(["dev"]) will run the Next.js 15 map UI; verify the "serverReadyAction" and any
port settings still match the app (port 3000) after the cwd change.
Comment on lines
+13
to
+25
| const prefix = "SUPER TEST"; | ||
|
|
||
| // To run this file: | ||
| // 1. pnpm -F nextjs test:e2e:ui --grep="admin" | ||
| // 2. Commend out beforeAll cleanupTestData (~line 15) if running directly | ||
| test.describe("Admin Portal", () => { | ||
| test.beforeAll(async ({ browser }) => { | ||
| // Create a new page for cleanup only | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| // Clean up any leftover test data before starting tests | ||
| await cleanupTestData({ page, prefix }); |
There was a problem hiding this comment.
Make the test-data prefix unique per run.
This suite deletes by prefix in beforeAll and again at the end, so a hard-coded "SUPER TEST" will let concurrent runs wipe each other's fixtures and turn retries flaky. Please generate a run-scoped suffix and pass that through the helpers instead of using a shared literal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/e2e/auth/admin-portal.spec.ts` around lines 13 - 25, Replace the
hard-coded prefix "SUPER TEST" with a run-unique value and propagate it to the
cleanup and test helpers: generate a suffix (e.g., timestamp or short UUID) at
the top of the suite and build prefix = `SUPER TEST – ${suffix}`, then pass that
prefix into cleanupTestData and any test helpers that create fixtures; update
references to the prefix variable in this file (and any helper calls) so each
test run uses a distinct prefix and avoids cross-run deletions.
Comment on lines
+274
to
+276
| const tomorrow = new Date(); | ||
| tomorrow.setDate(tomorrow.getDate() + 1); | ||
| const dateString = tomorrow.toISOString().split("T")[0] ?? ""; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/map/e2e/auth/admin-portal.spec.ts | sed -n '270,280p'Repository: F3-Nation/f3-nation
Length of output: 567
🏁 Script executed:
ls -la apps/map/e2e/auth/admin-portal.spec.ts && wc -l apps/map/e2e/auth/admin-portal.spec.tsRepository: F3-Nation/f3-nation
Length of output: 194
🏁 Script executed:
sed -n '274,300p' apps/map/e2e/auth/admin-portal.spec.tsRepository: F3-Nation/f3-nation
Length of output: 839
Build the form date in local time, not UTC.
toISOString() converts to UTC, so this can submit the wrong day when the test runs near local midnight (particularly in positive UTC offset timezones). For a date input, build YYYY-MM-DD from local date parts instead.
💡 Safer local-date formatting
const tomorrow = new Date();
tomorrow.setDate(tomorrow.getDate() + 1);
- const dateString = tomorrow.toISOString().split("T")[0] ?? "";
+ const dateString = [
+ tomorrow.getFullYear(),
+ String(tomorrow.getMonth() + 1).padStart(2, "0"),
+ String(tomorrow.getDate()).padStart(2, "0"),
+ ].join("-");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const tomorrow = new Date(); | |
| tomorrow.setDate(tomorrow.getDate() + 1); | |
| const dateString = tomorrow.toISOString().split("T")[0] ?? ""; | |
| const tomorrow = new Date(); | |
| tomorrow.setDate(tomorrow.getDate() + 1); | |
| const dateString = [ | |
| tomorrow.getFullYear(), | |
| String(tomorrow.getMonth() + 1).padStart(2, "0"), | |
| String(tomorrow.getDate()).padStart(2, "0"), | |
| ].join("-"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/e2e/auth/admin-portal.spec.ts` around lines 274 - 276, The test
builds dateString using toISOString() (variables tomorrow and dateString) which
yields UTC and can be off by a day; instead construct YYYY-MM-DD from local
parts: use tomorrow.getFullYear(), (tomorrow.getMonth() + 1) and
tomorrow.getDate(), pad month/day to two digits (e.g., padStart) and join with
'-' so dateString is the local-date string for the form input.
| import { expect, test } from "@playwright/test"; | ||
|
|
||
| import { SIDEBAR_WIDTH } from "@acme/shared/app/constants"; | ||
| // TODO: Remove all wait for timeouts |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Acknowledged tech debt: remove waitForTimeout calls.
The TODO is noted. As these arbitrary waits make tests flaky and slow, consider addressing this before merge or creating a tracking issue.
Would you like me to open an issue to track removing the waitForTimeout calls from these e2e tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/e2e/no-auth/edit.spec.ts` at line 5, Replace all arbitrary
waitForTimeout calls in edit.spec.ts with deterministic waits: use Playwright's
locator.waitFor / expect(locator).toBeVisible(), page.waitForResponse(), or
page.waitForSelector() tied to specific UI elements or network responses instead
of timeouts (search for waitForTimeout usages). If you cannot update all tests
now, create a tracking issue documenting each waitForTimeout occurrence and
update the TODO to reference that issue ID so the debt is tracked.
Comment on lines
+16
to
+21
| test.describe("Admin Map Actions", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto("/?lat=31.659308&lng=-132.955330&zoom=8"); | ||
| await turnOnEditMode(page); | ||
| await page.waitForSelector('[aria-label="Map"]', { timeout: 30000 }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Test interdependency creates ordering fragility.
This test suite has sequential dependencies - "Edit AO details" depends on "New location, AO, & event" creating data first. If a test fails midway, subsequent tests will also fail. Consider using test.describe.serial to make the ordering explicit, or restructuring to use setup fixtures for test isolation.
Make ordering explicit
-test.describe("Admin Map Actions", () => {
+test.describe.serial("Admin Map Actions", () => {This documents the intentional ordering dependency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test.describe("Admin Map Actions", () => { | |
| test.beforeEach(async ({ page }) => { | |
| await page.goto("/?lat=31.659308&lng=-132.955330&zoom=8"); | |
| await turnOnEditMode(page); | |
| await page.waitForSelector('[aria-label="Map"]', { timeout: 30000 }); | |
| }); | |
| test.describe.serial("Admin Map Actions", () => { | |
| test.beforeEach(async ({ page }) => { | |
| await page.goto("/?lat=31.659308&lng=-132.955330&zoom=8"); | |
| await turnOnEditMode(page); | |
| await page.waitForSelector('[aria-label="Map"]', { timeout: 30000 }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/e2e/no-auth/edit.spec.ts` around lines 16 - 21, The suite relies on
test ordering causing fragile interdependencies; make the intent explicit by
converting the top-level test.describe to test.describe.serial (replacing
test.describe("Admin Map Actions", ...) with test.describe.serial("Admin Map
Actions", ...)) or alternatively refactor the dependent tests into isolated
fixtures/fixtures-based setup so "New location, AO, & event" creates and yields
its data for the subsequent "Edit AO details" test; update the setup in
test.beforeEach or create a dedicated fixture to ensure deterministic state for
the tests referenced by their names.
| ALTER TABLE "update_requests" ALTER COLUMN "event_name" DROP NOT NULL;--> statement-breakpoint | ||
|
|
||
| -- Convert any legacy 'create_location' values to 'edit' to preserve row compatibility | ||
| UPDATE "public"."update_requests" SET "request_type" = 'edit' WHERE "request_type" = 'create_location';--> statement-breakpoint |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: Consider formatting WHERE on a new line.
Static analysis suggests placing the WHERE clause on a new line for readability (SQLFluff LT14). This is a minor formatting preference.
📝 Suggested format
-- Convert any legacy 'create_location' values to 'edit' to preserve row compatibility
-UPDATE "public"."update_requests" SET "request_type" = 'edit' WHERE "request_type" = 'create_location';--> statement-breakpoint
+UPDATE "public"."update_requests"
+SET "request_type" = 'edit'
+WHERE "request_type" = 'create_location';--> statement-breakpoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| UPDATE "public"."update_requests" SET "request_type" = 'edit' WHERE "request_type" = 'create_location';--> statement-breakpoint | |
| -- Convert any legacy 'create_location' values to 'edit' to preserve row compatibility | |
| UPDATE "public"."update_requests" | |
| SET "request_type" = 'edit' | |
| WHERE "request_type" = 'create_location';--> statement-breakpoint |
🧰 Tools
🪛 SQLFluff (4.1.0)
[error] 4-4: The 'WHERE' keyword should always start a new line.
(LT14)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/drizzle/0014_lucky_captain_marvel.sql` at line 4, The UPDATE
statement updating "public"."update_requests" should place the WHERE clause on
its own line for readability; modify the statement that sets "request_type" =
'edit' (targeting rows where "request_type" = 'create_location') so the WHERE
clause starts on a new line after the SET clause.
Comment on lines
+17
to
+20
| await createDatabaseIfNotExists(databaseUrl) | ||
| .then(() => console.log("Database check/creation completed.")) | ||
| .catch((err) => console.error("Failed to check/create database:", err)); | ||
|
|
There was a problem hiding this comment.
Fail fast when DB creation/check fails.
Line 17–Line 20 currently catches and logs errors, then continues migration. That suppresses the precondition failure and can trigger cascading failures in reset()/migrator() with less-clear diagnostics. Re-throw (or remove the local catch) so the script exits on this failure.
Suggested fix
- await createDatabaseIfNotExists(databaseUrl)
- .then(() => console.log("Database check/creation completed."))
- .catch((err) => console.error("Failed to check/create database:", err));
+ try {
+ await createDatabaseIfNotExists(databaseUrl);
+ console.log("Database check/creation completed.");
+ } catch (err) {
+ console.error("Failed to check/create database:", err);
+ throw err;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await createDatabaseIfNotExists(databaseUrl) | |
| .then(() => console.log("Database check/creation completed.")) | |
| .catch((err) => console.error("Failed to check/create database:", err)); | |
| try { | |
| await createDatabaseIfNotExists(databaseUrl); | |
| console.log("Database check/creation completed."); | |
| } catch (err) { | |
| console.error("Failed to check/create database:", err); | |
| throw err; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/migrate.ts` around lines 17 - 20, Currently the await
createDatabaseIfNotExists(databaseUrl) call swallows errors by catching and
logging them, letting execution proceed into reset() and migrator() with unclear
failures; remove the .catch or re-throw the caught error so the process fails
fast: update the block that calls createDatabaseIfNotExists(databaseUrl) to
either await it without .catch or, if keeping the catch, call throw err after
logging so the error propagates and prevents subsequent calls to reset() and
migrator() from running on a failed precondition.
Comment on lines
+29
to
+34
| if (vercelInfo?.isPreviewDeployment && vercelInfo.gitCommitRef) { | ||
| console.log( | ||
| "Overriding DATABASE_URL with vercel.databaseUrl", | ||
| vercelInfo.databaseUrl, | ||
| ); | ||
| return vercelInfo.databaseUrl; |
There was a problem hiding this comment.
Avoid logging the full database URL.
Logging vercelInfo.databaseUrl exposes the connection string (which may include credentials) in logs. Consider logging only that an override occurred, or redact sensitive parts.
Proposed fix
if (vercelInfo?.isPreviewDeployment && vercelInfo.gitCommitRef) {
console.log(
- "Overriding DATABASE_URL with vercel.databaseUrl",
- vercelInfo.databaseUrl,
+ "Overriding DATABASE_URL with vercel.databaseUrl for preview deployment",
);
return vercelInfo.databaseUrl;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (vercelInfo?.isPreviewDeployment && vercelInfo.gitCommitRef) { | |
| console.log( | |
| "Overriding DATABASE_URL with vercel.databaseUrl", | |
| vercelInfo.databaseUrl, | |
| ); | |
| return vercelInfo.databaseUrl; | |
| if (vercelInfo?.isPreviewDeployment && vercelInfo.gitCommitRef) { | |
| console.log( | |
| "Overriding DATABASE_URL with vercel.databaseUrl for preview deployment", | |
| ); | |
| return vercelInfo.databaseUrl; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/env/src/index.ts` around lines 29 - 34, The console.log in the block
checking vercelInfo?.isPreviewDeployment and vercelInfo.gitCommitRef prints
vercelInfo.databaseUrl, which can expose credentials; update the log in that
branch (the console.log that references vercelInfo.databaseUrl) to avoid
printing the full connection string—either log a simple message like "Overriding
DATABASE_URL with vercel.databaseUrl" without the value or redact sensitive
parts (e.g., only show hostname or "REDACTED") before logging, keeping the
return value as vercelInfo.databaseUrl.
| - add tests for approvals in the admin portal | ||
| - test image issues | ||
| - better text of where we're moving aos and regions | ||
| - decide which things you can change region and and which you can't |
There was a problem hiding this comment.
Fix duplicated wording in WIP bullet.
Line 188 has a typo (and and) that reduces clarity in the action item.
Suggested doc fix
-- decide which things you can change region and and which you can't
+- decide which things you can change regions for and which you can't📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - decide which things you can change region and and which you can't | |
| - decide which things you can change regions for and which you can't |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 188, Fix the duplicated word in the WIP bullet "decide
which things you can change region and and which you can't" by removing the
extra "and" and optionally adding a comma for clarity so it reads e.g. "decide
which things you can change region and which you can't" or "decide which things
you can change, and which you can't"; update the sentence in README.md
accordingly.
| id: request.id, | ||
| regionId: request.regionId, | ||
| regionName: request.regionName, | ||
| eventName: request.eventName ?? "", |
There was a problem hiding this comment.
Don’t normalize missing eventName to empty string here.
At Line 115, eventName: request.eventName ?? "" prevents later ?? "Unknown" fallbacks from working, so notifications can render a blank workout name.
Suggested fix
- eventName: request.eventName ?? "",
+ eventName: request.eventName ?? "Unknown",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| eventName: request.eventName ?? "", | |
| eventName: request.eventName ?? "Unknown", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/scripts/src/notify-outstanding-requests.ts` at line 115, The code is
normalizing a missing request.eventName to an empty string (eventName:
request.eventName ?? ""), which prevents later fallback logic (e.g., ??
"Unknown") from running and can render a blank name; change the object
assignment to pass through the original value instead (use eventName:
request.eventName or eventName: request.eventName ?? undefined) so downstream
"Unknown" fallback works; update the object that sets eventName (reference
request.eventName and the eventName property) accordingly.
Contributor
|
We decided to wait until #280 is done. Also, need to resolve branch conflicts and address Coderabbit (he doesn't like being ignored). |
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
3 participants
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.
👋 TL;DR
(coming soon)
🔎 Details
(coming soon)
✅ How to Test
(coming soon)
🥜 GIF
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests