Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughConverts many Tailwind arbitrary/variable classes to tokenized utilities; adds an extensive Lookahead/programme planning surface (API, hooks, types, normalization, UI components); implements activity-linked multi-date booking flows; extends asset/subcontractor/booking models with inference/planning fields; and makes RootLayout async to fetch an initial server user. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemandHeatmap
participant ActivityDrilldownDialog
participant ActivityContextDialog
participant CreateBookingForm
participant BookingAPI
User->>DemandHeatmap: click heatmap cell
DemandHeatmap->>ActivityDrilldownDialog: open(selectedCell)
ActivityDrilldownDialog->>ActivityDrilldownDialog: fetch activities
ActivityDrilldownDialog-->>User: show activities
User->>ActivityDrilldownDialog: view context
ActivityDrilldownDialog->>ActivityContextDialog: open(activity)
ActivityContextDialog->>ActivityContextDialog: fetch booking context (suggested dates, candidates)
ActivityContextDialog-->>User: show coverage & candidates
User->>ActivityContextDialog: Book remaining
ActivityContextDialog->>CreateBookingForm: open with activityContext
CreateBookingForm->>CreateBookingForm: build multi-date plan, validate
CreateBookingForm->>BookingAPI: submit payload (programme_activity_id, selected_week_start, ...)
BookingAPI-->>User: confirm bookings
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/landing/TopBar.tsx (1)
106-135:⚠️ Potential issue | 🔴 CriticalFix Tailwind v4 arbitrary value syntax—missing bracket notation.
The classes use invalid Tailwind v4 syntax. Arbitrary spacing values and z-index utilities require bracket notation:
gap-[1.25],w-[6.25],z-[100],z-[98],z-[99],max-w-[100],px-[7.5],pb-[7.5].Without brackets, these classes won't apply any styles. Additionally, the z-index values (98, 99, 100) fall outside Tailwind v4's default scale and must use bracket syntax.
Invalid classes requiring fixes
gap-1.25→gap-[1.25]w-6.25→w-[6.25]orw-[25px]z-100,z-98,z-99→z-[100],z-[98],z-[99]max-w-100→max-w-[100]ormax-w-[25rem]px-7.5→px-[7.5]orpx-[1.875rem]pb-7.5→pb-[7.5]orpb-[1.875rem]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/landing/TopBar.tsx` around lines 106 - 135, The Tailwind classes in TopBar's JSX use invalid v4 arbitrary-value syntax; update the class strings where the hamburger button, overlay button, and mobile menu div are defined (refer to the JSX using isMenuOpen, toggleMenu, closeMenu) to use bracket notation for non-standard values — e.g., change gap-1.25 → gap-[1.25], w-6.25 → w-[6.25], z-100/z-99/z-98 → z-[100]/z-[99]/z-[98], max-w-100 → max-w-[100] (or a valid rem/px value), and px-7.5/pb-7.5 → px-[7.5]/pb-[7.5]; ensure all arbitrary spacing/z-index classes in the button elements and the div with transition-[right] use the bracketed form so Tailwind v4 will apply them.src/app/(dashboard)/assets/page.tsx (2)
284-291:⚠️ Potential issue | 🟠 MajorSearch and type sorting still ignore the canonical label shown in the table.
Rows now render
canonicalType || assetType, but the filter and the"assetType"sort path still read onlyasset.assetType. Assets whose inferred canonical type differs from the raw backend type won't be searchable or sortable by the value users actually see.Also applies to: 303-337, 597-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 284 - 291, The filter and sorting read only asset.assetType but the UI displays canonicalType || assetType; update all places that reference asset.assetType for searching and sorting (e.g., the searchTerm filtering block using result.filter and any sort comparator or sort key functions) to use the displayed value like (asset.canonicalType || asset.assetType) so both search and sort operate on the canonicalType when present; apply the same change where sorting logic reads asset.assetType so the comparator uses the canonical fallback consistently.
273-275:⚠️ Potential issue | 🟡 MinorReset pagination when the readiness filter changes.
Only
searchTermresetscurrentPagetoday. If someone is on a later page and switches to "Planning ready" or "Needs review", the table can render empty even though matching rows still exist.Suggested fix
useEffect(() => { setCurrentPage(1); - }, [searchTerm]); + }, [searchTerm, readinessFilter]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 273 - 275, The pagination reset effect only watches searchTerm; update the useEffect that calls setCurrentPage(1) so it also depends on the readiness filter (the state variable controlling "Planning ready"/"Needs review")—either add that readiness state (e.g., readinessFilter or readiness) to the dependency array of the existing useEffect or create a separate useEffect that calls setCurrentPage(1) when readiness changes; ensure you reference the same setCurrentPage and searchTerm identifiers so the currentPage is reset whenever searchTerm or the readiness filter changes.src/components/forms/CreateBookingForm.tsx (1)
793-823:⚠️ Potential issue | 🟠 MajorDon't let
useProjectAssetserrors blank out activity candidate assets.In activity mode the form intentionally prefers
candidateAssets, but this effect returns as soon asassetsSwrErroris set. A transient project-assets failure will therefore hide the backend-provided candidate list and block activity-linked booking even though the dialog already has usable assets.Suggested fix
useEffect(() => { - if (assetsSwrError) { + const usingCandidateAssets = candidateAssets.length > 0; + + if (assetsSwrError && !usingCandidateAssets) { dispatchAsset({ type: "SET_ASSET_ERROR", error: true }); return; } const sourceAssets = - candidateAssets.length > 0 + usingCandidateAssets ? candidateAssets.map((asset) => ({ ...asset, asset_code: asset.asset_code || asset.id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forms/CreateBookingForm.tsx` around lines 793 - 823, The effect currently bails out whenever assetsSwrError is truthy, which wipes out backend-provided candidateAssets; change the early-return to only set the error and return when there are no candidateAssets (e.g. if (assetsSwrError && candidateAssets.length === 0) { dispatchAsset({ type: "SET_ASSET_ERROR", error: true }); return; }), otherwise allow the effect to continue and use candidateAssets as the sourceAssets; keep the rest of the normalization logic (normalizedAssets, dispatchAsset({ type: "SET_ASSETS", ... })) and optionally log the assetsSwrError without blocking candidateAssets when present.
🟡 Minor comments (12)
src/components/ui/checkbox.tsx-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorFix
rounded-1which is not a valid Tailwind v4 utility.The class
ring-3is valid in Tailwind v4 (generates a 12px ring width), butrounded-1does not exist in the standard v4 utilities. The default rounded scale includesrounded-xs(2px),rounded-sm(4px),rounded-md(6px), and others, but notrounded-1. Userounded-sminstead, which provides the intended 4px border radius.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/checkbox.tsx` at line 17, In src/components/ui/checkbox.tsx update the Tailwind class list used for the checkbox (the string in the checkbox component) to replace the invalid utility "rounded-1" with the valid Tailwind v4 utility "rounded-sm"; locate the class string definition in the Checkbox component and change only that token so the classes remain "peer ... rounded-sm border ..." to restore a 4px border radius.src/components/lookahead/WindowSelector.tsx-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorAvoid shadowing the global
windowobject.Using
windowas the loop variable shadows the globalwindowobject, which could cause confusion or subtle bugs ifwindowis referenced inside the callback.🔧 Suggested fix
- {(["2W", "4W", "6W"] as LookaheadWindowSize[]).map((window) => ( + {(["2W", "4W", "6W"] as LookaheadWindowSize[]).map((size) => ( <button - key={window} + key={size} type="button" - aria-pressed={windowSize === window} - onClick={() => onSetWindowSize(window)} + aria-pressed={windowSize === size} + onClick={() => onSetWindowSize(size)} className={`flex-1 rounded-lg px-3 py-2 text-sm font-semibold transition-all ${ - windowSize === window + windowSize === size ? "bg-navy text-white shadow-md shadow-slate-900/10" : "text-slate-500 hover:text-slate-900" }`} > - {window === "2W" + {size === "2W" ? "2 Weeks" - : window === "4W" + : size === "4W" ? "4 Weeks" : "6 Weeks"} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/WindowSelector.tsx` at line 42, In WindowSelector's JSX map (the (["2W","4W","6W"] as LookaheadWindowSize[]).map callback) the loop parameter is named window which shadows the global window object; rename that parameter to a non-conflicting identifier (e.g., win, size, or windowSize) and update any references inside the map callback and its returned JSX (props, keys, labels) accordingly to avoid shadowing and potential bugs.src/components/multicalendar/MulticalendarPage.tsx-67-67 (1)
67-67:⚠️ Potential issue | 🟡 MinorDon’t overload
descriptionwith provenance metadata.
CalendarEvent.descriptionis consumed downstream as notes text, so after this change strings likeSource: …andModified from programme defaultwill render under a “Notes” label. Keep provenance on dedicated event fields and compose it only in the views that intentionally display provenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/multicalendar/MulticalendarPage.tsx` at line 67, The description field for CalendarEvent in MulticalendarPage.tsx is being overloaded with provenance metadata; change the assignment so description is only the user notes (e.g., description: b.notes || "") and stop appending provenanceSummary there, leaving provenanceSummary on a dedicated event property (e.g., provenance or provenanceSummary) so views that need provenance can compose it explicitly.src/components/ui/full-calendar/calendar-views.tsx-423-428 (1)
423-428:⚠️ Potential issue | 🟡 MinorOffset the day-view “now” line by minutes, not just the hour.
Line 426 snaps the marker to the top of the current hour, so at 6:59 it still renders at 6:00.
TimeTablealready includesgetMinutes(), which means the two live indicators drift apart for most of the hour.Suggested fix
- const currentHour = new Date().getHours(); - const isCurrentDay = isSameDay(date, new Date()); + const now = new Date(); + const currentHour = now.getHours(); + const currentMinutes = now.getMinutes(); + const isCurrentDay = isSameDay(date, now); ... - style={{ top: `${(currentHour - 6) * 48}px` }} + style={{ top: `${((currentHour - 6) + currentMinutes / 60) * 48}px` }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/full-calendar/calendar-views.tsx` around lines 423 - 428, The live "now" marker is anchored only to currentHour, causing it to snap to the top of the hour; update the marker placement in the component (the conditional render that uses isCurrentDay and currentHour for the div with className "absolute w-full border-t-2 border-navy") to include minutes as well (use TimeTable.getMinutes() or new Date().getMinutes()) so the style.top calculation adds the fractional hour offset from minutes (minutes/60 * hourHeight) to the existing (currentHour - 6) * hourHeight computation; adjust the top formula accordingly so the marker aligns with the TimeTable minute-based indicator.src/components/multicalendar/BookingDetailDialog.tsx-575-623 (1)
575-623:⚠️ Potential issue | 🟡 MinorAvoid the extra divider when provenance is absent.
Because there is already a separator just above this block, Line 622 leaves two dividers back-to-back when none of the provenance fields are present. Make the trailing
<Separator />part of the same condition, or collapse this into a single divider around the optional section.Suggested structure
- {(data.source || - data.programme_activity_name || - data.expected_asset_type || - data.booking_group_id || - data.is_modified) && ( - <div> - ... - </div> - )} - - <Separator className="bg-slate-100" /> + {(data.source || + data.programme_activity_name || + data.expected_asset_type || + data.booking_group_id || + data.is_modified) ? ( + <> + <div> + ... + </div> + <Separator className="bg-slate-100" /> + </> + ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/multicalendar/BookingDetailDialog.tsx` around lines 575 - 623, The trailing Separator should only render when the Provenance block is present; update the conditional that checks data.source, data.programme_activity_name, data.expected_asset_type, data.booking_group_id, or data.is_modified to also include the <Separator /> so it is rendered inside the same block (or wrap both the provenance <div> and the Separator with the same condition) in the BookingDetailDialog component, ensuring no back-to-back dividers appear when all provenance fields are absent.src/components/bookings/BookingCardDesktop.tsx-76-94 (1)
76-94:⚠️ Potential issue | 🟡 MinorFormat
bookingSourcebefore showing it to users.The normalized source value now includes enums like
programme_activity; rendering that string directly exposes the internal value instead of a readable label. A tiny formatter here would also keep the mobile card and booking details dialog consistent.Suggested tweak
+const formatSourceLabel = (source: string) => + source + .replace(/[_-]+/g, " ") + .replace(/\b\w/g, (char) => char.toUpperCase()); ... - <span className="rounded-full bg-slate-100 px-2 py-0.5 text-[10px] font-semibold text-slate-600 capitalize"> - {booking.bookingSource} + <span className="rounded-full bg-slate-100 px-2 py-0.5 text-[10px] font-semibold text-slate-600"> + {formatSourceLabel(booking.bookingSource)} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingCardDesktop.tsx` around lines 76 - 94, BookingCardDesktop is rendering the raw enum-like booking.bookingSource (and thus exposing values like "programme_activity") — convert the source to a human-friendly label before rendering: create or use a formatter function (e.g., formatBookingSource) that maps/normalizes values to readable labels and call it where booking.bookingSource is displayed in BookingCardDesktop (replace the inline {booking.bookingSource} usage); also apply the same formatter in the mobile card and BookingDetailsDialog to keep labels consistent across views.src/components/lookahead/SnapshotHistoryPanel.tsx-33-50 (1)
33-50:⚠️ Potential issue | 🟡 MinorExpose the expand/collapse state on the toggle button.
This is acting as a disclosure, but the button on Lines 33-47 never reports whether the panel is open. Add
aria-expandedand connect it to the content witharia-controls/idso screen readers announce the current state.Suggested tweak
<button type="button" onClick={() => setIsOpen(!isOpen)} + aria-expanded={isOpen} + aria-controls="lookahead-snapshot-history" className="flex w-full items-center justify-between px-5 py-3.5 text-sm font-bold text-slate-600 hover:text-slate-900" > ... {isOpen && ( - <div className="space-y-2 border-t border-slate-100 p-3"> + <div + id="lookahead-snapshot-history" + className="space-y-2 border-t border-slate-100 p-3" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/SnapshotHistoryPanel.tsx` around lines 33 - 50, The toggle button for the Lookahead Snapshot History disclosure doesn't expose state to assistive tech; update the button element (the one using isOpen and setIsOpen) to include aria-expanded={isOpen} and aria-controls referencing the content container, and give the content div a matching id (e.g., snapshot-history-panel or similar) so screen readers can announce open/closed state and associate the button with the panel.src/components/lookahead/PlanningReadinessCard.tsx-19-20 (1)
19-20:⚠️ Potential issue | 🟡 MinorFormat the status label before rendering it.
The normalized payload already uses snake_case values such as
at_risk, so this card will show raw enum text to users. Convert it to a display label here.Suggested fix
const { counts, actionable_tasks: tasks } = planningCompleteness; const score = Math.round(planningCompleteness.score ?? 0); + const statusLabel = (planningCompleteness.status ?? "").replace(/_/g, " "); return ( <section className="rounded-2xl border border-slate-200 bg-white p-5 shadow-sm"> @@ <div className="rounded-2xl bg-slate-50 px-4 py-3 text-center"> <div className="text-3xl font-extrabold text-slate-900">{score}</div> <div className="text-[11px] font-semibold uppercase tracking-wide text-slate-500"> - {planningCompleteness.status} + {statusLabel} </div> </div> </div>Also applies to: 37-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/PlanningReadinessCard.tsx` around lines 19 - 20, The component PlanningReadinessCard is rendering raw enum text from planningCompleteness (e.g., at_risk); compute a display label before rendering by reading planningCompleteness.status (or similar status field), converting snake_case to a user-friendly form (replace underscores with spaces and title-case / capitalize words or map known enum values to explicit labels), store it in a new variable like displayStatus and use that variable wherever the status is rendered (also update the logic around counts/actionable_tasks/score if they reference the raw status). Ensure you reference the existing symbols planningCompleteness, counts, actionable_tasks (tasks), and score when locating where to add the transformation.src/components/lookahead/DemandHeatmap.tsx-116-124 (1)
116-124:⚠️ Potential issue | 🟡 MinorHide the click hint when drilldown is disabled.
onCellSelectis optional, but this banner is unconditional. In non-interactive usages it tells users to click cells that are rendered as plain containers.Suggested fix
- <span className="inline-flex items-center gap-1 rounded-full bg-teal-gradient px-3 py-1 text-xs font-semibold text-navy"> - <Sparkles className="h-3.5 w-3.5" /> - Click any demand cell to drill into activity - </span> + {onCellSelect && ( + <span className="inline-flex items-center gap-1 rounded-full bg-teal-gradient px-3 py-1 text-xs font-semibold text-navy"> + <Sparkles className="h-3.5 w-3.5" /> + Click any demand cell to drill into activity + </span> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/DemandHeatmap.tsx` around lines 116 - 124, The click hint is always shown even when drilldown is disabled; update the DemandHeatmap rendering so the hint span (the element containing Sparkles and "Click any demand cell to drill into activity") is only rendered when the optional onCellSelect prop is provided (i.e., truthy). Locate the JSX around timezone and the hint span in the DemandHeatmap component and wrap or conditionally render that hint using a check against onCellSelect so non-interactive usages do not display the misleading instruction.src/app/(dashboard)/subcontractors/page.tsx-619-634 (1)
619-634:⚠️ Potential issue | 🟡 MinorDon't render unknown readiness as "No".
selectedContractor.planningReadyis optional, but this branch treatsundefinedthe same asfalse. That will label records with missing readiness data as not ready in the sidebar, even though the table above already handles the missing state separately.Suggested fix
<div className="flex items-center justify-between"> <div className="flex items-center gap-3"> <Briefcase className="h-4 w-4 text-slate-400" /> <span className="text-sm font-medium text-slate-500"> Planning Ready </span> </div> - <span - className={`text-sm font-semibold ${ - selectedContractor.planningReady - ? "text-emerald-700" - : "text-amber-700" - }`} - > - {selectedContractor.planningReady ? "Yes" : "No"} - </span> + {selectedContractor.planningReady == null ? ( + <span className="text-sm font-semibold text-slate-500"> + Unknown + </span> + ) : ( + <span + className={`text-sm font-semibold ${ + selectedContractor.planningReady + ? "text-emerald-700" + : "text-amber-700" + }`} + > + {selectedContractor.planningReady ? "Yes" : "No"} + </span> + )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/subcontractors/page.tsx around lines 619 - 634, The sidebar currently treats undefined planningReady the same as false; change the rendering logic around selectedContractor.planningReady in the component so it distinguishes three states: true => show "Yes" with the emerald class, false => show "No" with the amber class, and undefined/null => show "Unknown" (or a neutral placeholder) with a neutral text class; update both the conditional used for the text and the className mapping (refer to selectedContractor.planningReady in the JSX) to use strict checks (e.g., === true / === false) so missing data is not displayed as "No".src/components/bookings/BookingsPage.tsx-253-260 (1)
253-260:⚠️ Potential issue | 🟡 MinorRefresh the default slot when the dialog opens.
With
useMemo([]),nextHourandendHourare fixed at first render. If this page stays open for a while, opening the booking form later can prefill a past time range.One low-impact option
const { nextHour, endHour } = useMemo(() => { const now = new Date(); const nextHour = startOfHour(addHours(now, 1)); return { nextHour, endHour: addHours(nextHour, 1), }; - }, []); + }, [isBookingFormOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 253 - 260, The values nextHour and endHour are computed once due to useMemo([]) so they become stale; update the logic to recompute when the booking dialog opens instead of only on first render — for example remove the empty dependency useMemo and compute nextHour/endHour inside the dialog open handler or change useMemo to depend on the dialog open state (the dialog open boolean used in the component) so nextHour = startOfHour(addHours(new Date(), 1)) and endHour = addHours(nextHour, 1) are recalculated when opening; look for useMemo, nextHour, endHour and the dialog open handler or state to implement this change, using startOfHour and addHours to build the new values.src/components/lookahead/UploadReviewDialog.tsx-72-75 (1)
72-75:⚠️ Potential issue | 🟡 MinorThe summary count can disagree with the table.
reviewRowsfalls back tomappings, but the "Needs review" card always rendersunclassifiedMappings.length. When there are zero unclassified rows but non-emptymappings, the header says0even though the table still has items.Suggested fix
<p className="mt-1 text-lg font-bold text-amber-700"> - {unclassifiedMappings.length} + {reviewRows.length} </p>Also applies to: 111-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/UploadReviewDialog.tsx` around lines 72 - 75, The "Needs review" count and the table use different sources: reviewRows uses the fallback (unclassifiedMappings.length > 0 ? unclassifiedMappings : mappings) but the card always displays unclassifiedMappings.length; update the card (and the other occurrence at the block around lines 111-115) to derive its count from the same variable reviewRows (or apply the same fallback logic) so the header count matches the rows shown; reference reviewRows, unclassifiedMappings, and mappings when making the change.
🧹 Nitpick comments (12)
src/components/TopNav.tsx (1)
67-69: Link the toggle button to the controlled menu for better SR context.Add
aria-controlson the button and a matchingidon the mobile menu container.♿ Suggested patch
- <Button + <Button variant="ghost" onClick={() => setOpen(!isOpen)} className="p-2 text-navy hover:bg-white/40" aria-expanded={isOpen} + aria-controls="mobile-nav-menu" aria-label="Toggle navigation menu" >- <div className="absolute left-0 top-full w-full border-t border-white/55 bg-[linear-gradient(to_bottom,rgba(255,255,255,0.9),rgba(246,251,255,0.88))] shadow-[inset_0_1px_0_rgba(255,255,255,0.85),0_14px_36px_rgba(15,42,74,0.16)] backdrop-blur-[26px] lg:hidden"> + <div id="mobile-nav-menu" className="absolute left-0 top-full w-full border-t border-white/55 bg-[linear-gradient(to_bottom,rgba(255,255,255,0.9),rgba(246,251,255,0.88))] shadow-[inset_0_1px_0_rgba(255,255,255,0.85),0_14px_36px_rgba(15,42,74,0.16)] backdrop-blur-[26px] lg:hidden">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TopNav.tsx` around lines 67 - 69, Add an aria-controls attribute to the toggle button (the element with aria-expanded={isOpen} / aria-label="Toggle navigation menu") and add a matching id on the mobile menu container element that is shown/hidden by TopNav (the element that renders the mobile menu controlled by isOpen); pick a unique id like "topnav-mobile-menu" and use that exact id value in the button's aria-controls so screen readers can associate the button with the controlled region.src/components/lookahead/WindowSelector.tsx (1)
54-58: Consider a lookup object for cleaner label mapping.The nested ternary can be replaced with a simple object lookup for improved readability.
♻️ Suggested refactor
const WINDOW_LABELS: Record<LookaheadWindowSize, string> = { "2W": "2 Weeks", "4W": "4 Weeks", "6W": "6 Weeks", }; // Then in JSX: {WINDOW_LABELS[size]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/WindowSelector.tsx` around lines 54 - 58, Replace the nested ternary in WindowSelector.tsx with a typed lookup: create a constant like WINDOW_LABELS: Record<LookaheadWindowSize, string> = { "2W":"2 Weeks", "4W":"4 Weeks", "6W":"6 Weeks" } and use WINDOW_LABELS[window] in the JSX (or the prop name used instead of window if different) to map the LookaheadWindowSize to its label; this simplifies the rendering logic and improves readability.src/app/layout.tsx (1)
51-51: Consider logging authentication errors for observability.The
.catch(() => null)silently swallows any errors fromgetServerUser(). While falling back tonullis appropriate for optional authentication, logging the error would aid debugging production issues.📋 Suggested improvement
- const initialUser = await getServerUser().catch(() => null); + const initialUser = await getServerUser().catch((err) => { + console.error("[RootLayout] Failed to fetch server user:", err); + return null; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/layout.tsx` at line 51, The current call that sets initialUser swallows errors from getServerUser() via .catch(() => null); change this to capture the error and log it before returning null (e.g., .catch(err => { /* log err with your app logger or console.error */ return null; })), referencing getServerUser and initialUser so the error is observable; prefer using your existing application logger (processLogger/requestLogger) if available instead of console.error.src/hooks/useResolvedProjectSelection.ts (1)
155-161: Minor: Consider extractingprojectBootstrapLoadingcondition for clarity.This boolean expression has grown complex with multiple conditions. Consider extracting it into named intermediate variables for better readability.
♻️ Suggested refactor
+ const isWaitingForProjects = + projectId === null && !projectsError && (projectsLoading || projectsRaw === undefined); + const projectBootstrapLoading = - Boolean(userKey) && - enabled && - (!isProjectSelectionReady || - (projectId === null && - !projectsError && - (projectsLoading || projectsRaw === undefined))); + Boolean(userKey) && enabled && (!isProjectSelectionReady || isWaitingForProjects);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useResolvedProjectSelection.ts` around lines 155 - 161, The boolean for projectBootstrapLoading is too complex—extract intermediate named booleans (e.g., hasUserKey = Boolean(userKey), needsBootstrap = enabled && !isProjectSelectionReady, noProjectYet = projectId === null && !projectsError && (projectsLoading || projectsRaw === undefined)) and then compute projectBootstrapLoading = hasUserKey && (needsBootstrap || noProjectYet); update references to use these names and add brief comments explaining each variable to improve readability and maintainability (references: projectBootstrapLoading, userKey, enabled, isProjectSelectionReady, projectId, projectsError, projectsLoading, projectsRaw).src/components/lookahead/VersionHistory.tsx (1)
17-23: DefaultSetcreates a new instance on every render.The default value
deletingIds = new Set<string>()creates a newSetinstance on each render when no prop is passed. While this won't cause bugs here since theSetis only read (via.has()), it's a subtle anti-pattern that could cause issues if passed to memoized children or used in dependency arrays.♻️ Consider a module-level constant
+const EMPTY_SET = new Set<string>(); + export function VersionHistory({ title = "Programme History", versions, - deletingIds = new Set<string>(), + deletingIds = EMPTY_SET, onDelete, onReviewUpload, }: Props) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/VersionHistory.tsx` around lines 17 - 23, The default parameter deletingIds = new Set<string>() creates a new Set on every render; define a module-level constant (e.g., const EMPTY_DELETING_IDS = new Set<string>()) and use that instead (either as the default parameter for VersionHistory or as a fallback inside the function via deletingIds ?? EMPTY_DELETING_IDS) so the same empty Set instance is reused across renders; update references to the deletingIds parameter accordingly.src/lib/subcontractorNormalization.ts (1)
33-34: Consider accepting numeric strings inasNumber.If
trade_inference_confidencecomes back as"0.87"—which is common for decimal/score fields—the new branch normalizes it tonulland the UI loses the value. Parsing finite numeric strings here would make this helper as tolerant as the rest of the adapter.Suggested tweak
const asNumber = (value: unknown): number | undefined => - typeof value === "number" && Number.isFinite(value) ? value : undefined; + typeof value === "number" && Number.isFinite(value) + ? value + : typeof value === "string" && value.trim() !== "" && Number.isFinite(Number(value)) + ? Number(value) + : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/subcontractorNormalization.ts` around lines 33 - 34, The asNumber helper currently only returns numbers and drops numeric strings; update asNumber to also accept string values that represent finite numbers by trimming the string, attempting to parse it (e.g., via Number or parseFloat), checking Number.isFinite on the parsed result, and returning that numeric value; keep returning undefined for non-numeric strings, empty strings, NaN, or non-finite results so callers like trade_inference_confidence preserve numeric-string inputs while still rejecting invalid values.src/lib/apiNormalization.ts (1)
187-190: Normalizeproject_idwithasIdfor consistency.This is the only identifier in the new asset mapper that goes through
asOptionalString, so numeric IDs would be discarded even though sibling fields accept them.Suggested fix
- project_id: asOptionalString(record.project_id), + project_id: asId(record.project_id) || undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/apiNormalization.ts` around lines 187 - 190, The project_id mapping currently uses asOptionalString(record.project_id) which will drop numeric IDs; change it to use asId(record.project_id) so IDs (numeric or string) are normalized like sibling identifier fields; update any imports to include asId if missing and keep the surrounding fields (planning_ready, description, status) unchanged.src/hooks/lookahead/api.ts (3)
382-385: Minor: Redundant truthy check.The condition
isRecord(payload.counts) && payload.countsis redundant sinceisRecordalready ensurespayload.countsis a non-null object.Suggested simplification
const countsRecord = - isRecord(payload.counts) && payload.counts + isRecord(payload.counts) ? payload.counts : (payload as Record<string, unknown>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/lookahead/api.ts` around lines 382 - 385, The conditional creating countsRecord redundantly checks truthiness after isRecord; simplify the expression by using isRecord(payload.counts) ? payload.counts : (payload as Record<string, unknown>) so countsRecord is set when isRecord(payload.counts) is true and falls back to payload cast otherwise—update the code around countsRecord and the isRecord(payload.counts) check accordingly.
155-173: Potential ambiguity:remaining_hoursused as fallback for bothhoursandgap_hours.If the API response contains only
remaining_hours, this value will be assigned tohours(line 161), andgap_hourswill benull(since the first??chain already consumedremaining_hours). This is likely the intended behavior, but worth verifying that the API doesn't expect both fields to receive the same fallback value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/lookahead/api.ts` around lines 155 - 173, The mapping currently calls asNumber(entry.remaining_hours) inline so the first null-coalescing chain will consume remaining_hours for hours and leave gap_hours null; to fix, extract remaining once (e.g., const remaining = asNumber(entry.remaining_hours)) before the return and use that same remaining as a fallback in both the hours and gap_hours chains so both fields can receive remaining_hours when other candidate fields are absent; keep using asNumber for other fields and update the hours and gap_hours assignments in the same function in src/hooks/lookahead/api.ts (references: asNumber, entry, hours, gap_hours, demand_hours).
215-227: Type assertion onentry.rowsassumes valid row structure.Line 224 uses a type assertion after the
Array.isArraycheck, but doesn't validate that each element matchesLookaheadRow. If the API returns malformed rows, this could cause runtime issues downstream. Consider adding row normalization if this API is prone to schema drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/lookahead/api.ts` around lines 215 - 227, The fetchLookaheadHistory function trusts entry.rows via a raw type assertion to LookaheadSnapshotResponse["rows"]; instead, validate/normalize each row to guard against schema drift by replacing the assertion with an explicit mapping that checks required fields (e.g., using a new isLookaheadRow type guard or inline checks) and coerces/defaults values (use asOptionalString for string fields, parse/ensure numeric fields, default missing arrays/objects), filtering out or normalizing malformed rows so the returned rows array always matches the LookaheadRow shape; update fetchLookaheadHistory (and introduce a small helper like isLookaheadRow/normalizeLookaheadRow if needed) to perform this validation before returning.src/types/index.ts (1)
442-452: Consider makingPlanningCompletenessTask.idrequired.Line 443 declares
idas optional, butnormalizePlanningTasksinapi.tsalways provides a fallback ID (task-${index}). Makingidrequired would align the type with the actual runtime guarantee.Make id required
export interface PlanningCompletenessTask { - id?: string; + id: string; title: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/index.ts` around lines 442 - 452, The interface PlanningCompletenessTask currently marks id as optional but normalizePlanningTasks in api.ts guarantees an id (using `task-${index}`) for every task; update the PlanningCompletenessTask interface to make id required (remove the optional marker) so the type reflects the runtime guarantee and prevent callers from treating id as possibly undefined.src/hooks/lookahead/useLookaheadQueries.ts (1)
61-78: Themutatewrapper discards parameters.Line 75 wraps
mutateas() => mutate(), losing the ability to passdataoroptionsarguments. This prevents optimistic updates and revalidation with specific data. If this is intentional for simplicity, consider documenting it; otherwise, expose the full signature.Expose full mutate signature
type SwrResult<T> = { data: T | undefined; isLoading: boolean; error: unknown; - mutate: () => Promise<T | undefined>; + mutate: (data?: T | Promise<T | undefined> | ((currentData?: T) => T | Promise<T | undefined>), opts?: { revalidate?: boolean }) => Promise<T | undefined>; key: string | null; }; function useTypedSWR<T>( key: string | null, fetcher: (() => Promise<T>) | null, ): SwrResult<T> { const { data, isLoading, error, mutate } = useSWR<T>( key, fetcher, SWR_CONFIG, ); return { data, isLoading, error, - mutate: () => mutate(), + mutate, key, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/lookahead/useLookaheadQueries.ts` around lines 61 - 78, The wrapper in useTypedSWR currently replaces SWR's mutate with a parameterless () => mutate(), which discards the original mutate signature and prevents passing data/options for optimistic updates or revalidation; update useTypedSWR to forward the full mutate signature (accepting data?: T | Promise<T> | MutatorCallback<T>, shouldRevalidate?: boolean | undefined or the equivalent used by your SWR types) and call the underlying mutate with those arguments, and adjust the returned SwrResult<T> type if necessary so callers can pass through data and options to mutate from useTypedSWR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23fd0592-ade0-4b6a-a99d-0410d1fb5061
📒 Files selected for processing (88)
src/app/(auth)/forgot-password/page.tsxsrc/app/(auth)/login/page.tsxsrc/app/(auth)/register/page.tsxsrc/app/(auth)/reset-password/page.tsxsrc/app/(auth)/set-password/page.tsxsrc/app/(dashboard)/assets/page.tsxsrc/app/(dashboard)/home/page.tsxsrc/app/(dashboard)/layout.tsxsrc/app/(dashboard)/loading.tsxsrc/app/(dashboard)/subcontractors/page.tsxsrc/app/layout.tsxsrc/app/not-found.tsxsrc/components/SideNav.tsxsrc/components/TopNav.tsxsrc/components/auth/ForgotPasswordForm.tsxsrc/components/auth/LoginForm.tsxsrc/components/auth/RegisterForm.tsxsrc/components/auth/ResetPasswordForm.tsxsrc/components/auth/SetPasswordForm.tsxsrc/components/bookings/BookingCardDesktop.tsxsrc/components/bookings/BookingCardDropdown.tsxsrc/components/bookings/BookingCardMobile.tsxsrc/components/bookings/BookingHistorySidebar.tsxsrc/components/bookings/BookingList.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/forms/CreateAssetForm.tsxsrc/components/forms/CreateBookingForm.tsxsrc/components/forms/RescheduleBookingForm.tsxsrc/components/forms/UpdateAssetForm.tsxsrc/components/home/RadioToggle.tsxsrc/components/landing/ClientDynamics.tsxsrc/components/landing/ContactModal.tsxsrc/components/landing/HeroOrbs.tsxsrc/components/landing/HeroParallax.tsxsrc/components/landing/LandingPage.tsxsrc/components/landing/LookaheadPreview.tsxsrc/components/landing/ShowcaseSection.tsxsrc/components/landing/TopBar.tsxsrc/components/landing/WatchVideoButton.tsxsrc/components/lookahead/ActivityContextDialog.tsxsrc/components/lookahead/ActivityDrilldownDialog.tsxsrc/components/lookahead/DemandHeatmap.tsxsrc/components/lookahead/LookaheadDashboard.tsxsrc/components/lookahead/PlanningAlerts.tsxsrc/components/lookahead/PlanningReadinessCard.tsxsrc/components/lookahead/SnapshotHistoryPanel.tsxsrc/components/lookahead/StatCards.tsxsrc/components/lookahead/UploadReviewDialog.tsxsrc/components/lookahead/VersionHistory.tsxsrc/components/lookahead/WindowSelector.tsxsrc/components/lookahead/activityBookingCoverage.tssrc/components/multicalendar/AssetFilter.tsxsrc/components/multicalendar/BookingDetailDialog.tsxsrc/components/multicalendar/CalendarHeader.tsxsrc/components/multicalendar/DesktopView.tsxsrc/components/multicalendar/MobileView.tsxsrc/components/multicalendar/MulticalendarPage.tsxsrc/components/site-plans/SitePlanUploadDialog.tsxsrc/components/site-plans/SitePlanViewerDialog.tsxsrc/components/site-plans/SitePlansSection.tsxsrc/components/ui/GlobalNetworkLoadingBar.tsxsrc/components/ui/badge.tsxsrc/components/ui/button.tsxsrc/components/ui/checkbox.tsxsrc/components/ui/full-calendar/calendar-context.tsxsrc/components/ui/full-calendar/calendar-utils.tsxsrc/components/ui/full-calendar/calendar-views.tsxsrc/components/ui/hero/hero-illustration.tsxsrc/components/ui/hero/logo-carousel.tsxsrc/components/ui/input.tsxsrc/components/ui/navigation-menu.tsxsrc/components/ui/radio-group.tsxsrc/components/ui/select.tsxsrc/components/ui/tabs.tsxsrc/components/ui/textarea.tsxsrc/components/ui/tooltip.tsxsrc/hooks/bookings/api.tssrc/hooks/lookahead/api.test.tssrc/hooks/lookahead/api.tssrc/hooks/lookahead/keys.tssrc/hooks/lookahead/useLookaheadData.tssrc/hooks/lookahead/useLookaheadQueries.tssrc/hooks/useProjectAssets.tssrc/hooks/useResolvedProjectSelection.tssrc/lib/apiNormalization.test.tssrc/lib/apiNormalization.tssrc/lib/subcontractorNormalization.tssrc/types/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/components/multicalendar/MulticalendarPage.tsx (1)
46-54: UseformatBookingSourcewhen buildingprovenanceSummary.This keeps source labels consistent with booking cards/dialogs and avoids raw snake_case values in calendar surfaces.
♻️ Proposed refactor
import { isAssetRetiredOrOutOfService } from "@/lib/assetStatus"; +import { formatBookingSource } from "@/lib/bookingHelpers"; ... - const provenanceSummary = [ + const sourceLabel = formatBookingSource(b.source); + const provenanceSummary = [ b.programme_activity_name ? `Activity: ${b.programme_activity_name}` : null, b.expected_asset_type ? `Expected: ${b.expected_asset_type}` : null, - b.source ? `Source: ${b.source}` : null, + sourceLabel ? `Source: ${sourceLabel}` : null, b.is_modified ? "Modified from programme default" : null, ]Also applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/multicalendar/MulticalendarPage.tsx` around lines 46 - 54, The provenanceSummary currently uses raw b.source values; replace that usage with the shared formatter formatBookingSource so source labels match booking cards/dialogs. In the code that builds provenanceSummary (the array using b.programme_activity_name, b.expected_asset_type, b.source, b.is_modified), call formatBookingSource(b.source) instead of using b.source directly and ensure you handle falsy/undefined returns the same way as other entries (filter(Boolean)). Also apply the same change to the other provenanceSummary instance referenced in the diff (the second occurrence that builds a similar array).src/app/(dashboard)/subcontractors/page.tsx (2)
626-648: Consider simplifying the IIFE to a ternary chain for readability.The IIFE works correctly but adds indentation depth. A ternary chain would be more concise and match React patterns:
♻️ Suggested simplification
- {(() => { - if (selectedContractor.planningReady === true) { - return ( - <span className="text-sm font-semibold text-emerald-700"> - Yes - </span> - ); - } - - if (selectedContractor.planningReady === false) { - return ( - <span className="text-sm font-semibold text-amber-700"> - No - </span> - ); - } - - return ( - <span className="text-sm font-semibold text-slate-500"> - Unknown - </span> - ); - })()} + <span + className={`text-sm font-semibold ${ + selectedContractor.planningReady === true + ? "text-emerald-700" + : selectedContractor.planningReady === false + ? "text-amber-700" + : "text-slate-500" + }`} + > + {selectedContractor.planningReady === true + ? "Yes" + : selectedContractor.planningReady === false + ? "No" + : "Unknown"} + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/subcontractors/page.tsx around lines 626 - 648, The IIFE used to render the planning-ready status is verbose; replace the self-invoking function with a concise ternary chain that checks selectedContractor.planningReady (e.g., selectedContractor.planningReady === true ? ... : selectedContractor.planningReady === false ? ... : ...) and return the same three span variants (text-sm font-semibold with text-emerald-700 for "Yes", text-amber-700 for "No", and text-slate-500 for "Unknown") inside the JSX to reduce nesting and improve readability while keeping the exact classes and labels.
31-45: Consider usingTransformedContractorfromsrc/types/index.tsto avoid type drift.The local
Contractorinterface duplicatesTransformedContractordefined insrc/types/index.ts(lines 232-246). Both interfaces define the same fields (suggestedTradeSpecialty,tradeResolutionStatus,tradeInferenceSource,tradeInferenceConfidence,planningReady).If you need to keep a local type, consider extending or re-exporting the central type:
♻️ Suggested refactor
+import { TransformedContractor } from "@/types"; + -interface Contractor { - contractorKey: string; - contractorName: string; - contractorCompany: string; - contractorTrade: string; - contractorEmail: string; - contractorPhone: string; - isActive: boolean; - suggestedTradeSpecialty?: string | null; - tradeResolutionStatus?: string | null; - tradeInferenceSource?: string | null; - tradeInferenceConfidence?: number | null; - planningReady?: boolean; - _originalData?: NormalizedSubcontractor; -} +type Contractor = TransformedContractor;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/subcontractors/page.tsx around lines 31 - 45, The local Contractor interface duplicates fields from TransformedContractor; replace the local declaration by importing and using TransformedContractor from src/types (or declare interface Contractor extends TransformedContractor { _originalData?: NormalizedSubcontractor } if you need the extra _originalData field) so you avoid type drift—update any references to Contractor in this file to use the imported/extended type and remove the duplicated interface declaration.src/app/(dashboard)/assets/page.tsx (2)
481-498: Addaria-pressedfor accessibility on toggle buttons.These filter buttons function as toggle controls but lack the
aria-pressedattribute to communicate the selected state to assistive technologies.♿ Proposed fix for accessibility
<button key={value} type="button" onClick={() => setReadinessFilter(value as ReadinessFilter)} + aria-pressed={readinessFilter === value} className={`rounded-full px-3 py-1.5 text-xs font-semibold transition ${ readinessFilter === value ? "bg-navy text-white" : "bg-slate-100 text-slate-600 hover:bg-slate-200" }`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 481 - 498, The filter buttons rendered in the map are interactive toggles but miss aria-pressed; update the button elements that call setReadinessFilter and compare readinessFilter to value (the elements in the array mapped to labels) to include an aria-pressed attribute whose value is a boolean expression (readinessFilter === value) so assistive tech can detect the selected state; ensure this change is applied to the button within the map that uses setReadinessFilter and ReadinessFilter.
297-303: "Needs review" filter conflatesfalseandundefinedstates.The
planningReady !== truecondition (Line 302) groups both explicitly-not-ready (false) and unknown (undefined) assets under "Needs review". While this may be intentionally conservative, users might want to distinguish between:
- Assets explicitly marked as not planning-ready
- Assets with unknown/uncomputed readiness status
If this is the intended behavior, consider adding a tooltip or count breakdown to help users understand the mix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 297 - 303, The "review" filter currently uses planningReady !== true which mixes false and undefined; update the logic in the block that checks readinessFilter (symbol: readinessFilter) so "review" only selects assets where asset.planningReady === false by changing result = result.filter((asset) => asset.planningReady !== true) to a strict false check, and add a new branch (e.g., readinessFilter === "unknown") that filters for asset.planningReady === undefined if you want to expose the unknown/uncomputed state separately (adjust UI labels/tooltips/counts accordingly).src/hooks/useProjectAssets.ts (1)
51-70: Pagination fallbacks silently mask malformed API responses.The runtime type checks with fallback defaults (Lines 61-66) are defensive, but if the API consistently returns missing or malformed pagination fields, this will go unnoticed. The fallback
has_more: falseis particularly impactful as it prevents loading additional pages.Consider adding debug/development logging when fallbacks are used, or ensure API contract tests validate the response shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProjectAssets.ts` around lines 51 - 70, The pagination fallback logic in the useSWR fetcher (inside useProjectAssets.ts where swrKey, getAssetEnvelope, normalizeAssetList and payload are used) silently masks malformed/missing pagination fields (total, skip, limit, has_more); update the fetcher to detect when you apply a fallback and emit a development-only warning or debug log that includes the received payload and which fields were defaulted so the issue is visible during dev/testing (use an existing logger or console.debug/console.warn guarded by NODE_ENV !== 'production'), and ensure the log references the swrKey and payload to aid troubleshooting.src/components/lookahead/LookaheadDashboard.tsx (2)
1122-1136: Consider using the passed event data instead of a full workspace refresh.The
CreateBookingFormcomponent passes the newly created events to theonSavecallback (line 1131-1133), but the callback only callsrefreshLookaheadWorkspace()and discards the event data. While the refresh will eventually fetch the new bookings from the server, using the passed events to update the local view would be more efficient and align with the component's intent to make bookings "appear immediately."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 1122 - 1136, The onSave callback currently passed to CreateBookingForm discards the created events and triggers a full refresh via refreshLookaheadWorkspace(); instead, accept the events parameter from CreateBookingForm's onSave and merge them into the local lookahead state so new bookings appear immediately (e.g., call the existing state updater that holds bookings/events or a helper like addEventsToWorkspace), and only call refreshLookaheadWorkspace() as a fallback or background refresh if needed; update the handler passed to CreateBookingForm (the onSave prop) to receive the created events and apply them to the same state that the UI reads from, while keeping closeActivityFlow logic unchanged.
1042-1047: Use theisLoadingflag fromuseLookaheadHistoryinstead of a heuristic.The hook returns
isLoadingvia its result spread (line 125), but the component only destructureshistoryandmutate(line 191). Update the destructuring to includeisLoading: historyLoadingand use it directly instead of thehistory.length === 0heuristic, which cannot distinguish between "still fetching" and "fetch completed with no results." This pattern is already used for other hooks in the same file (snapshotLoading,alertsLoading,versionsLoading).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 1042 - 1047, The SnapshotHistoryPanel is using a heuristic for loading state; update the useLookaheadHistory destructuring to include isLoading (rename to historyLoading) from the hook (useLookaheadHistory) alongside history and mutate, then replace the current isLoading prop expression on SnapshotHistoryPanel with isSnapshotHistoryOpen && enabled && historyLoading so it uses the hook-provided loading flag instead of history.length === 0; keep the other props (history, onOpenChange, isOpen) unchanged.src/components/ui/full-calendar/calendar-views.tsx (1)
1187-1188: Verifyz-1000is valid in Tailwind v4.In Tailwind v4, any z-index is allowed and generated despite not being in a predefined scale, soz-1000is valid syntax. However, a z-index of 1000 for a tooltip is unusually high. Best practice is to layer tooltips atz-50or similar to maintain a manageable stacking context.Consider whether such a high value is necessary, or if
z-50would suffice. Ifz-1000is intentional (e.g., to ensure tooltips appear above modals or other high-z elements), consider defining a named token like--z-index-tooltip: 1000in your theme for consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/full-calendar/calendar-views.tsx` around lines 1187 - 1188, The TooltipContent component is using a very large z-index class "z-1000"; change it to a more reasonable stacking value such as "z-50" (or another lower value your design system uses) unless this high value is intentional; if 1000 is required for cases where tooltips must overlay modals, replace the literal with a named theme token (e.g., a CSS variable like --z-index-tooltip) and reference that token via your Tailwind theme so TooltipContent's className stays consistent across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 164-169: Normalize the bookingSource before storing by trimming
whitespace and treating empty or whitespace-only strings as null: when setting
bookingSource (currently assigned from booking.source) call trim() on the value
and if the result is an empty string set bookingSource to null; keep other
fallback behavior (null) intact. Update the mapping that sets bookingSource so
downstream UI won't receive whitespace-only strings that render empty provenance
badges.
In `@src/components/multicalendar/BookingDetailDialog.tsx`:
- Around line 574-590: In BookingDetailDialog, the "Source:" row is rendered
when data.source is truthy even if formatBookingSource(data.source) returns an
empty string; change the guard so you compute the normalized label (call
formatBookingSource(data.source)) and only render the entire <p> block when that
normalizedLabel is non-empty (truthy); update the conditional around the Source
rendering to use the normalized label variable instead of raw data.source to
avoid blank rows.
In `@src/components/ui/full-calendar/calendar-views.tsx`:
- Line 876: The className on the element in calendar-views.tsx uses an invalid
Tailwind utility `rounded-0.75`; replace that token with a valid Tailwind
arbitrary value to preserve the 0.75rem (3px) radius — for example change
`rounded-0.75` to `rounded-[3px]` in the className string so the component's
styling (the className on the element in calendar-views.tsx) uses a supported
Tailwind v4 utility.
---
Nitpick comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 481-498: The filter buttons rendered in the map are interactive
toggles but miss aria-pressed; update the button elements that call
setReadinessFilter and compare readinessFilter to value (the elements in the
array mapped to labels) to include an aria-pressed attribute whose value is a
boolean expression (readinessFilter === value) so assistive tech can detect the
selected state; ensure this change is applied to the button within the map that
uses setReadinessFilter and ReadinessFilter.
- Around line 297-303: The "review" filter currently uses planningReady !== true
which mixes false and undefined; update the logic in the block that checks
readinessFilter (symbol: readinessFilter) so "review" only selects assets where
asset.planningReady === false by changing result = result.filter((asset) =>
asset.planningReady !== true) to a strict false check, and add a new branch
(e.g., readinessFilter === "unknown") that filters for asset.planningReady ===
undefined if you want to expose the unknown/uncomputed state separately (adjust
UI labels/tooltips/counts accordingly).
In `@src/app/`(dashboard)/subcontractors/page.tsx:
- Around line 626-648: The IIFE used to render the planning-ready status is
verbose; replace the self-invoking function with a concise ternary chain that
checks selectedContractor.planningReady (e.g., selectedContractor.planningReady
=== true ? ... : selectedContractor.planningReady === false ? ... : ...) and
return the same three span variants (text-sm font-semibold with text-emerald-700
for "Yes", text-amber-700 for "No", and text-slate-500 for "Unknown") inside the
JSX to reduce nesting and improve readability while keeping the exact classes
and labels.
- Around line 31-45: The local Contractor interface duplicates fields from
TransformedContractor; replace the local declaration by importing and using
TransformedContractor from src/types (or declare interface Contractor extends
TransformedContractor { _originalData?: NormalizedSubcontractor } if you need
the extra _originalData field) so you avoid type drift—update any references to
Contractor in this file to use the imported/extended type and remove the
duplicated interface declaration.
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 1122-1136: The onSave callback currently passed to
CreateBookingForm discards the created events and triggers a full refresh via
refreshLookaheadWorkspace(); instead, accept the events parameter from
CreateBookingForm's onSave and merge them into the local lookahead state so new
bookings appear immediately (e.g., call the existing state updater that holds
bookings/events or a helper like addEventsToWorkspace), and only call
refreshLookaheadWorkspace() as a fallback or background refresh if needed;
update the handler passed to CreateBookingForm (the onSave prop) to receive the
created events and apply them to the same state that the UI reads from, while
keeping closeActivityFlow logic unchanged.
- Around line 1042-1047: The SnapshotHistoryPanel is using a heuristic for
loading state; update the useLookaheadHistory destructuring to include isLoading
(rename to historyLoading) from the hook (useLookaheadHistory) alongside history
and mutate, then replace the current isLoading prop expression on
SnapshotHistoryPanel with isSnapshotHistoryOpen && enabled && historyLoading so
it uses the hook-provided loading flag instead of history.length === 0; keep the
other props (history, onOpenChange, isOpen) unchanged.
In `@src/components/multicalendar/MulticalendarPage.tsx`:
- Around line 46-54: The provenanceSummary currently uses raw b.source values;
replace that usage with the shared formatter formatBookingSource so source
labels match booking cards/dialogs. In the code that builds provenanceSummary
(the array using b.programme_activity_name, b.expected_asset_type, b.source,
b.is_modified), call formatBookingSource(b.source) instead of using b.source
directly and ensure you handle falsy/undefined returns the same way as other
entries (filter(Boolean)). Also apply the same change to the other
provenanceSummary instance referenced in the diff (the second occurrence that
builds a similar array).
In `@src/components/ui/full-calendar/calendar-views.tsx`:
- Around line 1187-1188: The TooltipContent component is using a very large
z-index class "z-1000"; change it to a more reasonable stacking value such as
"z-50" (or another lower value your design system uses) unless this high value
is intentional; if 1000 is required for cases where tooltips must overlay
modals, replace the literal with a named theme token (e.g., a CSS variable like
--z-index-tooltip) and reference that token via your Tailwind theme so
TooltipContent's className stays consistent across the codebase.
In `@src/hooks/useProjectAssets.ts`:
- Around line 51-70: The pagination fallback logic in the useSWR fetcher (inside
useProjectAssets.ts where swrKey, getAssetEnvelope, normalizeAssetList and
payload are used) silently masks malformed/missing pagination fields (total,
skip, limit, has_more); update the fetcher to detect when you apply a fallback
and emit a development-only warning or debug log that includes the received
payload and which fields were defaulted so the issue is visible during
dev/testing (use an existing logger or console.debug/console.warn guarded by
NODE_ENV !== 'production'), and ensure the log references the swrKey and payload
to aid troubleshooting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4b36e88-141d-436c-aa5f-47e38981f723
📒 Files selected for processing (22)
src/app/(dashboard)/assets/page.tsxsrc/app/(dashboard)/subcontractors/page.tsxsrc/components/bookings/BookingCardDesktop.tsxsrc/components/bookings/BookingCardMobile.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/forms/CreateBookingForm.tsxsrc/components/landing/HeroParallax.tsxsrc/components/landing/LandingPage.tsxsrc/components/lookahead/DemandHeatmap.tsxsrc/components/lookahead/LookaheadDashboard.tsxsrc/components/lookahead/PlanningReadinessCard.tsxsrc/components/lookahead/SnapshotHistoryPanel.tsxsrc/components/lookahead/UploadReviewDialog.tsxsrc/components/lookahead/WindowSelector.tsxsrc/components/lookahead/activityBookingCoverage.tssrc/components/multicalendar/BookingDetailDialog.tsxsrc/components/multicalendar/MulticalendarPage.tsxsrc/components/ui/checkbox.tsxsrc/components/ui/full-calendar/calendar-views.tsxsrc/hooks/useProjectAssets.tssrc/lib/bookingHelpers.tssrc/lib/multicalendarHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/checkbox.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/landing/HeroParallax.tsx
- src/components/lookahead/WindowSelector.tsx
- src/components/lookahead/PlanningReadinessCard.tsx
- src/components/lookahead/DemandHeatmap.tsx
- src/components/bookings/BookingCardDesktop.tsx
- src/components/lookahead/activityBookingCoverage.ts
- src/components/landing/LandingPage.tsx
- src/components/forms/CreateBookingForm.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/bookings/BookingsPage.tsx (1)
223-234:⚠️ Potential issue | 🟡 MinorPotential indefinite loading state if bootstrap never resolves.
The redirect is now suppressed while
projectBootstrapLoadingis true. Per the hook implementation (context snippet 1), this flag can remain true indefinitely if:
- The Zustand store's
hasHydratednever becomes true, or- A fetch error occurs that doesn't set
projectsErrorThis would leave users stuck on this page with an endless loading spinner (line 442) and no redirect to
/home.Consider whether a timeout fallback is warranted in
useResolvedProjectSelectionto prevent this edge case from degrading UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 223 - 234, The redirect suppression using projectBootstrapLoading can hang indefinitely if the bootstrap never resolves; update the useResolvedProjectSelection hook (or the logic that sets projectBootstrapLoading) to include a timeout fallback that forces bootstrap completion after a reasonable delay (e.g., 5–10s) or when hydration fails, and expose a fallback flag (e.g., projectBootstrapTimedOut) so the BookingsPage redirect logic can treat timeout as resolved; specifically modify useResolvedProjectSelection (and any store setter that manages projectBootstrapLoading/hasResolvedProjects) to clear projectBootstrapLoading and set a timed-out state on timeout or on fetch error so window.location.href = "/home" can run instead of leaving the spinner indefinitely.src/app/(dashboard)/assets/page.tsx (1)
782-791:⚠️ Potential issue | 🟡 MinorDon’t present the raw type as canonical.
When
canonicalTypeis missing, this still rendersassetTypeunder the “Canonical Type” label. That makes unresolved assets look normalized and conflicts with the adjacent resolution state.Suggested fix
- {selectedAsset.canonicalType || selectedAsset.assetType} + {selectedAsset.canonicalType || "Unspecified"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 782 - 791, The Canonical Type field currently falls back to selectedAsset.assetType when selectedAsset.canonicalType is falsy; change this so the UI does not present raw assetType as canonical — update the JSX that renders the Canonical Type (the span using selectedAsset.canonicalType || selectedAsset.assetType) to only render selectedAsset.canonicalType and otherwise render a neutral indicator (empty string, placeholder like "—" or "Unresolved") so unresolved assets aren’t shown as normalized; ensure you update the span that references selectedAsset.canonicalType and selectedAsset.assetType accordingly.
♻️ Duplicate comments (1)
src/hooks/useProjectAssets.ts (1)
24-40:⚠️ Potential issue | 🟡 MinorSearch nested
recordsenvelopes too.Line 27 already treats
recordsas a valid list wrapper, butgetAssetEnvelope()never descends into that key. For payloads like{ records: { assets: [...], total: ... } }, the assets can still normalize while pagination falls back again.Suggested fix
- for (const key of ["data", "assets", "results", "items"]) { + for (const key of ["data", "assets", "results", "items", "records"]) { const nestedValue = payload[key]; if (hasAssetList(nestedValue)) { return nestedValue; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProjectAssets.ts` around lines 24 - 40, getAssetEnvelope() currently doesn't descend into a nested "records" envelope even though hasAssetList() recognizes "records"; update the for-loop in getAssetEnvelope to include "records" in the keys array (replace ["data","assets","results","items"] with ["data","assets","results","items","records"]) so the function will detect and return nested payload.records that contain an asset list; keep the existing hasAssetList(payload) check and return behavior unchanged.
🧹 Nitpick comments (4)
src/components/bookings/BookingsPage.tsx (1)
253-260: Clarify the intent of theisBookingFormOpendependency.The memoization recalculates
nextHour/endHouronly whenisBookingFormOpenchanges. This means:
- When the form opens (false → true), fresh times are computed ✓
- While the form stays open, times remain frozen (acceptable for a booking form)
This behavior is reasonable, but the dependency choice is non-obvious. Consider adding a brief comment explaining the intent to prevent future maintainers from "fixing" it.
📝 Suggested clarifying comment
const { nextHour, endHour } = useMemo(() => { + // Recompute default times only when the form opens (not on every render) const now = new Date(); const nextHour = startOfHour(addHours(now, 1)); return { nextHour, endHour: addHours(nextHour, 1), }; }, [isBookingFormOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 253 - 260, Add a short clarifying comment above the useMemo that computes nextHour and endHour in the BookingsPage component explaining that the memo intentionally depends on isBookingFormOpen so that times are recalculated only when the booking form is opened/closed (to freeze times while the form is open), e.g., reference nextHour, endHour, and isBookingFormOpen in the comment to prevent future maintainers from removing or changing this dependency.src/components/multicalendar/MulticalendarPage.tsx (1)
46-55: Potential duplication withBookingDetailDialogprovenance rendering.This
provenanceSummarycomputation mirrors the provenance section inBookingDetailDialog.tsx(lines 575-620), which independently reads the same raw fields (programme_activity_name,expected_asset_type,source,is_modified) and formats them for display.Having two parallel provenance representations risks drift if one is updated without the other. Consider extracting a shared utility (e.g.,
buildProvenanceSummary(booking)) to ensure consistency.♻️ Example shared utility
// src/lib/bookingHelpers.ts export const buildProvenanceSummary = (booking: { programme_activity_name?: string | null; expected_asset_type?: string | null; source?: string | null; is_modified?: boolean; }): string => { const sourceLabel = formatBookingSource(booking.source); return [ booking.programme_activity_name ? `Activity: ${booking.programme_activity_name}` : null, booking.expected_asset_type ? `Expected: ${booking.expected_asset_type}` : null, sourceLabel ? `Source: ${sourceLabel}` : null, booking.is_modified ? "Modified from programme default" : null, ] .filter(Boolean) .join(" · "); };Then use it in both
MulticalendarPage.tsxand potentially inBookingDetailDialog.tsxfor the summary tooltip/badge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/multicalendar/MulticalendarPage.tsx` around lines 46 - 55, The provenance summary logic in MulticalendarPage (provenanceSummary using formatBookingSource and fields programme_activity_name, expected_asset_type, source, is_modified) duplicates the same rendering in BookingDetailDialog; extract this into a shared helper (e.g., buildProvenanceSummary(booking)) that accepts the booking object, calls formatBookingSource(booking.source) and returns the joined string, then replace the inline provenanceSummary in MulticalendarPage and the equivalent block in BookingDetailDialog to call buildProvenanceSummary to ensure a single source of truth.src/components/lookahead/LookaheadDashboard.tsx (1)
270-291: Consider guarding against nullprojectIdin revalidation.
revalidateBookingsForProject(projectId)is called without checking ifprojectIdis non-null. WhilePromise.allSettledhandles failures gracefully and current call sites likely have validprojectId, adding a guard improves defensive coding.🔧 Optional defensive improvement
const refreshLookaheadWorkspace = useCallback(async () => { - await Promise.allSettled([ + const mutations = [ mutateSnapshot(), mutateAlerts(), mutateHistory(), mutateVersions(), mutateUploadStatus(), mutateActivities(), mutateBookingContext(), - revalidateBookingsForProject(projectId), - ]); + ]; + if (projectId) { + mutations.push(revalidateBookingsForProject(projectId)); + } + await Promise.allSettled(mutations); }, [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 270 - 291, In refreshLookaheadWorkspace, guard against a null/undefined projectId before calling revalidateBookingsForProject(projectId): check projectId and only include the revalidation promise when it's truthy (e.g., conditionally push or call revalidateBookingsForProject inside an if) so you don't pass null into revalidateBookingsForProject; update the Promise.allSettled array construction in refreshLookaheadWorkspace to conditionally include the revalidateBookingsForProject call and keep the other mutate* calls unchanged.src/app/(dashboard)/assets/page.tsx (1)
46-67: Reuse the sharedApiAssettype here.
useProjectAssets()already returns normalizedApiAsset[], so keeping a parallelAssetFromBackendcontract in this file means these backend fields now have to stay in sync in two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 46 - 67, AssetFromBackend duplicates the shared ApiAsset contract; replace the local AssetFromBackend type with the exported ApiAsset type and update any references in this file (e.g., where useProjectAssets() consumers expect ApiAsset[]) so the file imports ApiAsset instead of declaring AssetFromBackend; remove the AssetFromBackend interface and ensure any nullable/optional fields align with ApiAsset to avoid type mismatches when using useProjectAssets().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 297-303: The "Needs review" filter currently uses strict === false
and misses assets with planningReady === undefined; update the readinessFilter
handling so the "review" branch selects assets where planningReady is not true
(e.g., asset.planningReady === false || asset.planningReady === undefined or
equivalently asset.planningReady !== true) instead of only === false; modify the
block that checks readinessFilter === "review" (and the lambda asset => ...) to
include undefined values so unmarked assets appear in the Needs review bucket.
- Around line 635-654: The current outer guard hides the whole badge row when
asset.planningReady is undefined, which also suppresses
asset.typeResolutionStatus; change the outer condition to render the wrapper
when either asset.planningReady is defined or asset.typeResolutionStatus exists
(e.g. if (asset.planningReady !== undefined || asset.typeResolutionStatus)),
keep the planning-ready span rendered only when asset.planningReady !==
undefined and keep the type resolution span rendered when
asset.typeResolutionStatus is truthy so the status badge shows independently.
---
Outside diff comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 782-791: The Canonical Type field currently falls back to
selectedAsset.assetType when selectedAsset.canonicalType is falsy; change this
so the UI does not present raw assetType as canonical — update the JSX that
renders the Canonical Type (the span using selectedAsset.canonicalType ||
selectedAsset.assetType) to only render selectedAsset.canonicalType and
otherwise render a neutral indicator (empty string, placeholder like "—" or
"Unresolved") so unresolved assets aren’t shown as normalized; ensure you update
the span that references selectedAsset.canonicalType and selectedAsset.assetType
accordingly.
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 223-234: The redirect suppression using projectBootstrapLoading
can hang indefinitely if the bootstrap never resolves; update the
useResolvedProjectSelection hook (or the logic that sets
projectBootstrapLoading) to include a timeout fallback that forces bootstrap
completion after a reasonable delay (e.g., 5–10s) or when hydration fails, and
expose a fallback flag (e.g., projectBootstrapTimedOut) so the BookingsPage
redirect logic can treat timeout as resolved; specifically modify
useResolvedProjectSelection (and any store setter that manages
projectBootstrapLoading/hasResolvedProjects) to clear projectBootstrapLoading
and set a timed-out state on timeout or on fetch error so window.location.href =
"/home" can run instead of leaving the spinner indefinitely.
---
Duplicate comments:
In `@src/hooks/useProjectAssets.ts`:
- Around line 24-40: getAssetEnvelope() currently doesn't descend into a nested
"records" envelope even though hasAssetList() recognizes "records"; update the
for-loop in getAssetEnvelope to include "records" in the keys array (replace
["data","assets","results","items"] with
["data","assets","results","items","records"]) so the function will detect and
return nested payload.records that contain an asset list; keep the existing
hasAssetList(payload) check and return behavior unchanged.
---
Nitpick comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 46-67: AssetFromBackend duplicates the shared ApiAsset contract;
replace the local AssetFromBackend type with the exported ApiAsset type and
update any references in this file (e.g., where useProjectAssets() consumers
expect ApiAsset[]) so the file imports ApiAsset instead of declaring
AssetFromBackend; remove the AssetFromBackend interface and ensure any
nullable/optional fields align with ApiAsset to avoid type mismatches when using
useProjectAssets().
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 253-260: Add a short clarifying comment above the useMemo that
computes nextHour and endHour in the BookingsPage component explaining that the
memo intentionally depends on isBookingFormOpen so that times are recalculated
only when the booking form is opened/closed (to freeze times while the form is
open), e.g., reference nextHour, endHour, and isBookingFormOpen in the comment
to prevent future maintainers from removing or changing this dependency.
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 270-291: In refreshLookaheadWorkspace, guard against a
null/undefined projectId before calling revalidateBookingsForProject(projectId):
check projectId and only include the revalidation promise when it's truthy
(e.g., conditionally push or call revalidateBookingsForProject inside an if) so
you don't pass null into revalidateBookingsForProject; update the
Promise.allSettled array construction in refreshLookaheadWorkspace to
conditionally include the revalidateBookingsForProject call and keep the other
mutate* calls unchanged.
In `@src/components/multicalendar/MulticalendarPage.tsx`:
- Around line 46-55: The provenance summary logic in MulticalendarPage
(provenanceSummary using formatBookingSource and fields programme_activity_name,
expected_asset_type, source, is_modified) duplicates the same rendering in
BookingDetailDialog; extract this into a shared helper (e.g.,
buildProvenanceSummary(booking)) that accepts the booking object, calls
formatBookingSource(booking.source) and returns the joined string, then replace
the inline provenanceSummary in MulticalendarPage and the equivalent block in
BookingDetailDialog to call buildProvenanceSummary to ensure a single source of
truth.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d2481d8-e370-418a-b72c-a663b03df336
📒 Files selected for processing (10)
src/app/(dashboard)/assets/page.tsxsrc/app/(dashboard)/home/page.tsxsrc/app/(dashboard)/subcontractors/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/LookaheadDashboard.tsxsrc/components/multicalendar/BookingDetailDialog.tsxsrc/components/multicalendar/MulticalendarPage.tsxsrc/components/ui/full-calendar/calendar-views.tsxsrc/components/ui/hero/logo-carousel.tsxsrc/hooks/useProjectAssets.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/ui/hero/logo-carousel.tsx
- src/app/(dashboard)/home/page.tsx
- src/app/(dashboard)/subcontractors/page.tsx
- src/components/multicalendar/BookingDetailDialog.tsx
- src/components/ui/full-calendar/calendar-views.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/lookahead/LookaheadDashboard.tsx (2)
1133-1147: Minor: RedundantisOpenprop.The outer conditional already ensures
selectedActivityis truthy, makingisOpen={Boolean(selectedActivity)}alwaystrue. Consider simplifying toisOpen={true}or removing if the component supports it.♻️ Simplify redundant prop
<CreateBookingForm - isOpen={Boolean(selectedActivity)} + isOpen onClose={closeActivityFlow}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 1133 - 1147, The CreateBookingForm is always rendered only when selectedActivity is truthy, so the isOpen={Boolean(selectedActivity)} prop is redundant; update the JSX around CreateBookingForm (the conditional that checks activityDialogMode, selectedActivity, bookingContext, and bookingContextLoading) to either pass isOpen={true} or remove the isOpen prop entirely if CreateBookingForm treats absence as open—adjust callers/props accordingly (look at CreateBookingForm usage and prop interface) and ensure closeActivityFlow and refreshLookaheadWorkspace behavior remains unchanged.
85-90: Time normalization may not handle edge cases correctly.The normalization assumes time is either
HH:MM(5 chars) orHH:MM:SS(8 chars). If the API returns times with single-digit hours (e.g.,9:30) or other formats, this could produce invalid results.♻️ Consider a more robust normalization
function toDateTime(date?: string | null, time?: string | null): Date | null { if (!date || !time) return null; - const normalizedTime = time.length === 5 ? `${time}:00` : time; + const normalizedTime = time.includes(":") && !time.match(/:\d{2}$/) + ? `${time}:00` + : time; const parsed = new Date(`${date}T${normalizedTime}`); return Number.isNaN(parsed.getTime()) ? null : parsed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 85 - 90, The toDateTime function's time normalization assumes fixed lengths and fails for inputs like "9:30" or variants; update toDateTime to robustly parse the time string by splitting on ":" (or matching with a regex), normalizing each component to two digits (pad hours/minutes and default seconds to "00" if missing), validate numeric ranges, then construct the datetime (e.g., `${date}T${HH}:${MM}:${SS}`) and return null on invalid parse; reference the toDateTime function for locating where to implement this normalization and validation.src/components/bookings/BookingsPage.tsx (1)
224-242: The timeout clause is redundant in this redirect guard.
useResolvedProjectSelectionalready derivesprojectBootstrapLoadingasrawProjectBootstrapLoading && !projectBootstrapTimedOut, so(!projectBootstrapLoading || projectBootstrapTimedOut)simplifies to!projectBootstrapLoading. Dropping the extra term makes the redirect behavior easier to reason about.♻️ Proposed cleanup
useEffect(() => { if ( !isTv && userId && - (!projectBootstrapLoading || projectBootstrapTimedOut) && + !projectBootstrapLoading && hasResolvedProjects && !projectId && typeof window !== "undefined" ) { window.location.href = "/home"; @@ }, [ hasResolvedProjects, isTv, projectBootstrapLoading, - projectBootstrapTimedOut, projectId, userId, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 224 - 242, The redirect guard in the useEffect unnecessarily repeats the timeout check: remove the redundant projectBootstrapTimedOut term and simplify the condition to rely only on projectBootstrapLoading (which is already computed by useResolvedProjectSelection as rawProjectBootstrapLoading && !projectBootstrapTimedOut); update the effect condition inside useEffect that currently checks "(!projectBootstrapLoading || projectBootstrapTimedOut)" to just check "!projectBootstrapLoading" so the redirect logic uses the derived projectBootstrapLoading flag consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 367-377: The summary cards render counts using authLoading ||
isLoading while the BookingList uses projectBootstrapLoading; unify them by
introducing a single surface-loading boolean (e.g., const surfaceLoading =
authLoading || isLoading || projectBootstrapLoading) in BookingsPage.tsx and
replace the ternaries that render the counts ({authLoading || isLoading ? "—" :
allBookings.length} and {authLoading || isLoading ? "—" : pendingCount}) with
checks against surfaceLoading, and use that same surfaceLoading when deciding to
show the BookingList skeleton/real list so both cards and list share the same
bootstrap loading gate.
---
Nitpick comments:
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 224-242: The redirect guard in the useEffect unnecessarily repeats
the timeout check: remove the redundant projectBootstrapTimedOut term and
simplify the condition to rely only on projectBootstrapLoading (which is already
computed by useResolvedProjectSelection as rawProjectBootstrapLoading &&
!projectBootstrapTimedOut); update the effect condition inside useEffect that
currently checks "(!projectBootstrapLoading || projectBootstrapTimedOut)" to
just check "!projectBootstrapLoading" so the redirect logic uses the derived
projectBootstrapLoading flag consistently.
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 1133-1147: The CreateBookingForm is always rendered only when
selectedActivity is truthy, so the isOpen={Boolean(selectedActivity)} prop is
redundant; update the JSX around CreateBookingForm (the conditional that checks
activityDialogMode, selectedActivity, bookingContext, and bookingContextLoading)
to either pass isOpen={true} or remove the isOpen prop entirely if
CreateBookingForm treats absence as open—adjust callers/props accordingly (look
at CreateBookingForm usage and prop interface) and ensure closeActivityFlow and
refreshLookaheadWorkspace behavior remains unchanged.
- Around line 85-90: The toDateTime function's time normalization assumes fixed
lengths and fails for inputs like "9:30" or variants; update toDateTime to
robustly parse the time string by splitting on ":" (or matching with a regex),
normalizing each component to two digits (pad hours/minutes and default seconds
to "00" if missing), validate numeric ranges, then construct the datetime (e.g.,
`${date}T${HH}:${MM}:${SS}`) and return null on invalid parse; reference the
toDateTime function for locating where to implement this normalization and
validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd94aa51-b986-4975-9725-5de8b2be7d0e
📒 Files selected for processing (9)
src/app/(dashboard)/assets/page.tsxsrc/app/(dashboard)/home/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/LookaheadDashboard.tsxsrc/components/multicalendar/BookingDetailDialog.tsxsrc/components/multicalendar/MulticalendarPage.tsxsrc/hooks/useProjectAssets.tssrc/hooks/useResolvedProjectSelection.tssrc/lib/bookingHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- src/hooks/useProjectAssets.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/multicalendar/MulticalendarPage.tsx
- src/app/(dashboard)/assets/page.tsx
- src/lib/bookingHelpers.ts
- src/app/(dashboard)/home/page.tsx
- src/hooks/useResolvedProjectSelection.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/lookahead/LookaheadDashboard.tsx (1)
325-373:⚠️ Potential issue | 🟠 MajorBlock a second upload while one is already in flight.
The button still allows a new file selection during
uploading/polling, and the nextstartPolling()call firststopPolling()s the existing upload. That drops client-side tracking for the first upload while it is still processing server-side.Suggested fix
+ const isUploadInFlight = + uploadPhase.kind === "uploading" || uploadPhase.kind === "polling"; + const handleFileSelected = useCallback( async (file: File | null) => { - if (!file || !projectId) return; + if (!file || !projectId || isUploadInFlight) return; ... - [projectId, startPolling], + [isUploadInFlight, projectId, startPolling], ); ... - disabled={!topActionButtonsReady || !projectId} + disabled={ + !topActionButtonsReady || !projectId || isUploadInFlight + }Also applies to: 375-393, 717-733
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 325 - 373, startPolling currently unconditionally stops any existing polling which lets a second upload override client-side tracking; before calling stopPolling() or scheduling a new polling interval, check the current upload phase and bail out if an upload/polling is already active (e.g., if uploadPhase.kind is "uploading" or "polling"/"processing"); update the startPolling(uploadId) entry guard to return early when uploadPhase indicates an in-flight upload, and also ensure the file-selection/upload UI uses the same uploadPhase check to disable the upload button/input so users cannot start a second upload while polling; refer to startPolling, stopPolling, uploadPhase, pollingRef, and isFetchingRef when making the 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 `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 222-225: The planningCompleteness query returned by
usePlanningCompleteness is never invalidated after actions that change
readiness; update the relevant mutation handlers (the upload completion handler,
version deletion function, booking save mutation, and mapping-correction
routines) to call the React Query invalidation for that query (e.g.,
useQueryClient().invalidateQueries or queryClient.refetchQueries using the same
key/params used by usePlanningCompleteness such as ['planningCompleteness', {
projectId }]); import/useQueryClient where needed and trigger the invalidation
right after each successful mutation so the readiness pill/card reflects the
current planningCompleteness immediately.
- Around line 498-505: persistentUploadMessage is computed from
uploadStatus.completeness_notes only, but unclassifiedCount falls back to
unclassifiedMappings.length so the alert can be inconsistent; update the logic
to derive the message from the resolved unclassifiedCount instead of raw
uploadStatus.completeness_notes. Specifically, change where
persistentUploadMessage is created (and the similar blocks at the other
locations: around lines 546-555, 610-612, 898-914, 1027-1029) to call
getPersistentUploadAlertMessage with the resolved value (e.g., construct a
completeness_notes-like object or overload getPersistentUploadAlertMessage to
accept unclassifiedCount) using unclassifiedCount (which references
uploadStatus?.completeness_notes?.unclassified_mapping_count ??
unclassifiedMappings.length) so the composed alert and detail text remain
consistent.
- Around line 159-160: The dismissedAlerts state (and similarly deletingIds) is
persistent across uploads/snapshots in the same project session causing alerts
like "upload_quality" or "demand_spike" to remain hidden for later uploads; add
a useEffect that watches the snapshot and latest upload identifiers (e.g.,
snapshot.id and latestUpload.id or whatever props/vars represent the current
snapshot/upload) and calls setDismissedAlerts(new Set()) (and setDeletingIds(new
Set()) if applicable) to reset these Sets whenever the snapshot or latest upload
changes; place this effect near the useState declarations referencing
dismissedAlerts/setDismissedAlerts and deletingIds/setDeletingIds so the alert
state is cleared for new snapshots/uploads.
---
Outside diff comments:
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 325-373: startPolling currently unconditionally stops any existing
polling which lets a second upload override client-side tracking; before calling
stopPolling() or scheduling a new polling interval, check the current upload
phase and bail out if an upload/polling is already active (e.g., if
uploadPhase.kind is "uploading" or "polling"/"processing"); update the
startPolling(uploadId) entry guard to return early when uploadPhase indicates an
in-flight upload, and also ensure the file-selection/upload UI uses the same
uploadPhase check to disable the upload button/input so users cannot start a
second upload while polling; refer to startPolling, stopPolling, uploadPhase,
pollingRef, and isFetchingRef when making the changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87c79483-ca7e-4a80-b0b7-b6f16b29b5c4
📒 Files selected for processing (2)
src/components/bookings/BookingsPage.tsxsrc/components/lookahead/LookaheadDashboard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/bookings/BookingsPage.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/lookahead/LookaheadDashboard.tsx (1)
295-298: Consider clearing snapshot-scoped selections when the upload/snapshot changes.This effect resets alerts and delete state, but the active drilldown/booking selection survives a new
latestUploadKeyorsnapshotKey. If the workspace refreshes to a new snapshot mid-flow, the dialogs can keep showing stale row/activity context from the previous snapshot.♻️ Possible follow-up
useEffect(() => { setDismissedAlerts(new Set()); setDeletingIds(new Set()); - }, [latestUploadKey, snapshotKey]); + setSelectedCell(null); + closeActivityFlow(); + }, [closeActivityFlow, latestUploadKey, snapshotKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lookahead/LookaheadDashboard.tsx` around lines 295 - 298, The effect that runs on changes to latestUploadKey and snapshotKey currently clears alerts and deletingIds but leaves snapshot-scoped selections intact; update this effect to also clear the active drilldown/booking selection when those keys change by calling the appropriate setters (e.g., setActiveDrilldown(null) and setActiveBooking(null) or equivalent state-reset functions used in LookaheadDashboard) so dialogs cannot retain stale row/activity context across snapshot/upload refreshes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 241-264: The unclassified count is derived from
unclassifiedMappings but that query is disabled until isUploadReviewOpen,
causing unclassifiedCount to be 0 until the dialog opens; update the
useUnclassifiedMappings call (and the similar block at lines ~516-525) so its
enabled flag uses only the presence of latestVersion?.upload_id (e.g., enabled:
Boolean(latestVersion?.upload_id)) or otherwise fetch the count from
latestVersion.completeness_notes.unclassified_mapping_count before falling back
to unclassifiedMappings.length; locate the useUnclassifiedMappings hook and the
unclassifiedCount calculation and ensure the query runs when an upload exists
(not gated by isUploadReviewOpen) so the dashboard shows the correct count
without opening review.
- Around line 1128-1133: The onBook handler currently clears the drilldown
(calls setSelectedCell(null)) before bookingContext is loaded, which causes the
modal/CTA to disappear on cold or failed fetches; change onBook (and the
equivalent at 1189-1203) to first setActivityContextCell(selectedCell),
setSelectedActivity(activity) and setActivityDialogMode("booking") but do not
clear selectedCell immediately—keep it until bookingContext succeeds or an
explicit cancel occurs, and introduce a loading/error UI path in the booking
modal (driven by bookingContext presence/state) so the modal stays visible with
a spinner and a retry/close action if the context fetch fails; reference
setActivityContextCell, setSelectedActivity, setActivityDialogMode,
setSelectedCell and bookingContext when making these changes.
---
Nitpick comments:
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 295-298: The effect that runs on changes to latestUploadKey and
snapshotKey currently clears alerts and deletingIds but leaves snapshot-scoped
selections intact; update this effect to also clear the active drilldown/booking
selection when those keys change by calling the appropriate setters (e.g.,
setActiveDrilldown(null) and setActiveBooking(null) or equivalent state-reset
functions used in LookaheadDashboard) so dialogs cannot retain stale
row/activity context across snapshot/upload refreshes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d1a6558-a002-4c8f-9979-8328fee9a7f6
📒 Files selected for processing (3)
src/app/(dashboard)/assets/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/LookaheadDashboard.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/bookings/BookingsPage.tsx (1)
259-268: Intentional but unconventional dependency may trigger lint warnings.The
useMemodepends onisBookingFormOpento refresh times when the form opens, butisBookingFormOpenisn't actually read inside the callback. This will trigger anreact-hooks/exhaustive-depswarning. Consider suppressing the lint rule with a comment, or useuseRefto capture the form-open timestamp for clearer intent:💡 Option 1: Add lint suppression (minimal change)
const { nextHour, endHour } = useMemo(() => { const now = new Date(); const nextHour = startOfHour(addHours(now, 1)); return { nextHour, endHour: addHours(nextHour, 1), }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally recalculate only when form opens }, [isBookingFormOpen]);💡 Option 2: Clearer intent with useRef (alternative)
const formOpenedAtRef = useRef<Date | null>(null); useEffect(() => { if (isBookingFormOpen) { formOpenedAtRef.current = new Date(); } }, [isBookingFormOpen]); const { nextHour, endHour } = useMemo(() => { const now = formOpenedAtRef.current ?? new Date(); const next = startOfHour(addHours(now, 1)); return { nextHour: next, endHour: addHours(next, 1) }; }, [isBookingFormOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 259 - 268, The useMemo for computing nextHour and endHour depends on isBookingFormOpen but does not read it inside the callback, which triggers react-hooks/exhaustive-deps; either suppress the lint rule with an inline comment above the useMemo (eslint-disable-next-line react-hooks/exhaustive-deps) to keep the current behavior, or implement a clearer approach by adding a formOpenedAtRef (useRef) and a useEffect that sets formOpenedAtRef.current = new Date() when isBookingFormOpen becomes true, then compute nextHour/endHour inside the existing useMemo using formOpenedAtRef.current (falling back to new Date()) so the dependency on isBookingFormOpen reflects the form-open timestamp intent.src/app/(dashboard)/assets/page.tsx (1)
583-585: ReusegetAssetDisplayType()in the row renderer.Search and sort already centralize this fallback. Using the same helper here keeps the visible type from drifting if the rule changes again.
♻️ Proposed refactor
- <span className="truncate capitalize"> - {asset.canonicalType || asset.assetType} - </span> + <span className="truncate capitalize"> + {getAssetDisplayType(asset)} + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 583 - 585, The row renderer currently uses the inline fallback {asset.canonicalType || asset.assetType}; replace that with the shared helper getAssetDisplayType(asset) so the displayed asset type uses the same logic used for search/sort and won't diverge if the rule changes. Update the span content to call getAssetDisplayType and ensure the helper is imported or available in the module where the row renderer (the component rendering asset.canonicalType/asset.assetType) lives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 454-477: The empty-state message must reflect the current
readinessFilter as well as searchTerm: update the empty-state rendering logic
(the block that currently uses searchTerm to show "No assets found.") to
consider readinessFilter (the same one toggled by the buttons using
setReadinessFilter and typed ReadinessFilter). Change the message branching to:
if searchTerm is present keep the "No assets found matching {searchTerm}"
behavior; otherwise switch on readinessFilter and show "No planning-ready assets
found." for 'ready', "No assets needing review found." for 'review', and the
existing "No assets found." for 'all' (or default). Ensure the message uses
readinessFilter and searchTerm variables used in this file (and leave
planningReadyCount and the filter buttons unchanged).
- Around line 253-255: When resetting pagination in the useEffect that currently
calls setCurrentPage(1) on [readinessFilter, searchTerm], clamp currentPage to
the new max page after the dataset shrinks: compute maxPages from
filteredAndSortedAssets.length and pageSize and call setCurrentPage(prev =>
Math.min(prev, maxPages || 1)) (or set to 1 if empty). Do this before the
component's early return so the UI never shows "Page X of Y" where X > Y, and
remove the later redundant totalPages declaration to avoid conflicting
calculations; reference the useEffect, setCurrentPage, currentPage,
filteredAndSortedAssets and totalPages identifiers when making the change.
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 85-111: The toDateTime function lets empty time parts like "" be
treated as 0 because Number("") === 0; fix by validating the raw time parts
before numeric conversion: in toDateTime, ensure time.trim().split(":") returns
2 or 3 parts and that hoursRaw and minutesRaw are non-empty and consist only of
digits (and if secondsRaw is present it must also be non-empty and digits) (use
a simple /^\d+$/ check), return null for any failure, then convert to numbers
and continue with the existing range checks and normalizedTime/Date parsing.
This change localizes to the toDateTime function.
- Around line 669-671: The Tailwind classes used in LookaheadDashboard.tsx
(e.g., "max-w-screen", "rounded-7", "h-90", "min-w-70", "max-h-75" found in the
className on the outer divs) are not standard — add corresponding theme.extend
entries to your Tailwind config (tailwind.config.ts) to define maxWidth,
borderRadius, height, minWidth and maxHeight keys with the desired values so
those utility classes resolve correctly at build time.
---
Nitpick comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 583-585: The row renderer currently uses the inline fallback
{asset.canonicalType || asset.assetType}; replace that with the shared helper
getAssetDisplayType(asset) so the displayed asset type uses the same logic used
for search/sort and won't diverge if the rule changes. Update the span content
to call getAssetDisplayType and ensure the helper is imported or available in
the module where the row renderer (the component rendering
asset.canonicalType/asset.assetType) lives.
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 259-268: The useMemo for computing nextHour and endHour depends on
isBookingFormOpen but does not read it inside the callback, which triggers
react-hooks/exhaustive-deps; either suppress the lint rule with an inline
comment above the useMemo (eslint-disable-next-line react-hooks/exhaustive-deps)
to keep the current behavior, or implement a clearer approach by adding a
formOpenedAtRef (useRef) and a useEffect that sets formOpenedAtRef.current = new
Date() when isBookingFormOpen becomes true, then compute nextHour/endHour inside
the existing useMemo using formOpenedAtRef.current (falling back to new Date())
so the dependency on isBookingFormOpen reflects the form-open timestamp intent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84496f61-a221-4319-8f83-9cd2e6e000ff
📒 Files selected for processing (4)
src/app/(dashboard)/assets/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/ActivityDrilldownDialog.tsxsrc/components/lookahead/LookaheadDashboard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/lookahead/ActivityDrilldownDialog.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/bookings/BookingsPage.tsx (1)
164-169: Avoid trimmingbooking.sourcetwice.This works, but you can simplify and avoid duplicate
trim()calls for readability.💡 Optional cleanup
+ const normalizedSource = booking.source?.trim(); + return { @@ - bookingSource: booking.source?.trim() ? booking.source.trim() : null, + bookingSource: normalizedSource ? normalizedSource : null, bookingGroupId: booking.booking_group_id ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/bookings/BookingsPage.tsx` around lines 164 - 169, The bookingSource assignment trims booking.source twice; change it to compute the trimmed value once and reuse it when setting bookingSource (e.g., derive a single const like trimmedSource = booking.source?.trim() and then set bookingSource: trimmedSource ?? null) so BookingPage's bookingSource property avoids duplicate trim() calls and is clearer.src/app/(dashboard)/assets/page.tsx (1)
474-492: Filter buttons are well-implemented with proper accessibility.Good use of
aria-pressedfor screen reader support. The type cast on line 483 works, though usingas conston the array would provide automatic type inference:✨ Optional: Use `as const` for type-safe mapping
- {[ - ["all", "All assets"], - ["ready", "Planning ready"], - ["review", "Needs review"], - ].map(([value, label]) => ( + {([ + ["all", "All assets"], + ["ready", "Planning ready"], + ["review", "Needs review"], + ] as const).map(([value, label]) => ( <button key={value} type="button" aria-pressed={readinessFilter === value} - onClick={() => setReadinessFilter(value as ReadinessFilter)} + onClick={() => setReadinessFilter(value)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 474 - 492, Replace the inline array with a const tuple typed via "as const" so TypeScript infers the exact string literals and you can remove the manual cast in the onClick; e.g. create a const like options = [["all","All assets"],["ready","Planning ready"],["review","Needs review"]] as const, then map over options and call setReadinessFilter(value) (where value is inferred as the union matching ReadinessFilter), keeping existing identifiers readinessFilter and setReadinessFilter and preserving the aria-pressed and className logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/lookahead/LookaheadDashboard.tsx`:
- Around line 815-818: The banner retry handler onUploadAnother doesn't clear
the hidden file input value so choosing the same file can noop; update the
onUploadAnother path (where setUploadPhase is called) to also reset the input by
checking fileInputRef.current and setting its value to an empty string before
calling fileInputRef.current.click(), e.g. null-safe clear of
fileInputRef.current.value then trigger click so the input's onChange will fire
for the same file selection.
- Around line 205-209: The current useEffect in LookaheadDashboard blindly casts
persisted?.windowSize to WindowSize and restores it via setWindowSizeLocal,
which can introduce invalid values; update the effect to validate
persisted.windowSize against the allowed WindowSize options (e.g., check
membership in the keys of WINDOW_WEEKS or the WindowSize enum) before calling
setWindowSizeLocal, and if invalid or missing fall back to the default "4W";
reference useEffect, hasUIIntentHydrated, uiScopeKey,
useUIIntentStore.getState().getLookaheadIntent, setWindowSizeLocal,
WINDOW_WEEKS, and WindowSize when making this change.
- Around line 1211-1227: Handlers onCorrectMapping and onPromoteToMemory only
refresh upload-review data; after calling
updateProgrammeMapping/promoteItemClassification and the existing
Promise.allSettled of mutateMappings(), mutateUnclassified(),
mutateUploadStatus(), mutatePlanningCompleteness(), also trigger a dashboard
revalidation by calling the dashboard revalidation function (e.g.,
mutateDashboardSurface() or mutateWorkspace()) so the main
heatmap/coverage/readiness UI updates immediately; add that mutate call (or
include it inside the Promise.allSettled array) in both onCorrectMapping and
onPromoteToMemory.
---
Nitpick comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 474-492: Replace the inline array with a const tuple typed via "as
const" so TypeScript infers the exact string literals and you can remove the
manual cast in the onClick; e.g. create a const like options = [["all","All
assets"],["ready","Planning ready"],["review","Needs review"]] as const, then
map over options and call setReadinessFilter(value) (where value is inferred as
the union matching ReadinessFilter), keeping existing identifiers
readinessFilter and setReadinessFilter and preserving the aria-pressed and
className logic.
In `@src/components/bookings/BookingsPage.tsx`:
- Around line 164-169: The bookingSource assignment trims booking.source twice;
change it to compute the trimmed value once and reuse it when setting
bookingSource (e.g., derive a single const like trimmedSource =
booking.source?.trim() and then set bookingSource: trimmedSource ?? null) so
BookingPage's bookingSource property avoids duplicate trim() calls and is
clearer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458f66be-aa85-473f-8caf-ebe791ada717
📒 Files selected for processing (4)
src/app/(dashboard)/assets/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/ActivityDrilldownDialog.tsxsrc/components/lookahead/LookaheadDashboard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/lookahead/ActivityDrilldownDialog.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/(dashboard)/assets/page.tsx (1)
909-911: Consider clamping confidence display values.The percentage calculation assumes
typeInferenceConfidenceis in the 0–1 range. If the backend ever sends a value outside this range (e.g., already as a percentage), the display could show unexpected results like "150%".💡 Optional defensive fix
{selectedAsset.typeInferenceConfidence != null - ? `${Math.round(selectedAsset.typeInferenceConfidence * 100)}%` + ? `${Math.min(100, Math.max(0, Math.round(selectedAsset.typeInferenceConfidence * 100)))}%` : "n/a"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(dashboard)/assets/page.tsx around lines 909 - 911, The displayed confidence can exceed 100% if the backend returns values outside 0-1; update the rendering for selectedAsset.typeInferenceConfidence to compute a clamped percentage first (convert values <=1 by multiplying by 100, treat values >1 as already percentage), then clamp the result to the 0–100 range and round for display, and use that safePercent in place of the current inline expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`(dashboard)/assets/page.tsx:
- Around line 909-911: The displayed confidence can exceed 100% if the backend
returns values outside 0-1; update the rendering for
selectedAsset.typeInferenceConfidence to compute a clamped percentage first
(convert values <=1 by multiplying by 100, treat values >1 as already
percentage), then clamp the result to the 0–100 range and round for display, and
use that safePercent in place of the current inline expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e36fbd85-e84c-4adc-9a83-015ca99a1a3c
📒 Files selected for processing (4)
src/app/(dashboard)/assets/page.tsxsrc/components/bookings/BookingsPage.tsxsrc/components/lookahead/ActivityDrilldownDialog.tsxsrc/components/lookahead/LookaheadDashboard.tsx
This was referenced May 6, 2026
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
Improvements
Tests