Skip to content

feat: admin trip preview + shared proposal UI package#36

Merged
Systemsaholic merged 3 commits intomainfrom
feature/trip-preview-shared-ui
Mar 19, 2026
Merged

feat: admin trip preview + shared proposal UI package#36
Systemsaholic merged 3 commits intomainfrom
feature/trip-preview-shared-ui

Conversation

@Systemsaholic
Copy link
Copy Markdown
Owner

@Systemsaholic Systemsaholic commented Mar 19, 2026

Summary

  • New @tailfire/trip-proposal-ui package — 19 shared presentation components extracted from client app (ProposalHero, DaySection, ActivityCard, ProposalShell, detail components, etc.). Zero drift between admin preview and client published views.
  • Admin preview page at /trips/[id]/preview — authenticated, shows live unpublished data via GET /trips/:id/preview-proposal. Cinzel/Lato fonts for visual parity with client. Amber "Preview Mode" banner.
  • Preview button fixed — navigates within admin app instead of opening broken client URL popup
  • Publish button fixedgetClientOrigin() uses NEXT_PUBLIC_CLIENT_URL env var (set in Doppler for all environments)
  • Doppler configuredNEXT_PUBLIC_CLIENT_URL set for dev/stg/prd

Test plan

  • Admin typecheck passes
  • Admin production build passes (route generated)
  • Codex validated: no blocking issues
  • NEXT_PUBLIC_CLIENT_URL set in Doppler dev/stg/prd
  • Preview button navigates to /trips/[id]/preview on tf-demo
  • Preview page renders live trip data with proposal components
  • Publish button copies correct client URL

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Admin preview mode for trip proposals with a sticky preview banner and back navigation
    • Side-by-side and summary comparison views for evaluating itineraries
    • Rich proposal UI: hero, itinerary navigation, day/activity cards, and pricing summaries
    • Activity detail modal with image gallery, pricing breakdown, and full activity details
    • Agent profile card showing advisor info and contact CTA
    • Activity-level approve/decline interactions and selectable itineraries

Create @tailfire/trip-proposal-ui shared package with presentation
components extracted from the client app (ProposalHero, AgentProfileCard,
ItineraryNav, ActivityCard, DaySection, ProposalShell, etc.). Add admin
preview page at /trips/[id]/preview with sticky amber "Preview Mode"
banner and in-app navigation. Fix getClientOrigin for all environments
and update Preview button to navigate in-app instead of opening popup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 19, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
tailfire-client Ready Ready Preview, Comment Mar 19, 2026 8:42pm
tailfire-ota Ready Ready Preview, Comment Mar 19, 2026 8:42pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 19, 2026

Warning

Rate limit exceeded

@Systemsaholic has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 751128b5-39c9-4aba-99d7-a77b4b93f268

📥 Commits

Reviewing files that changed from the base of the PR and between e4c76f9 and 0753e9f.

📒 Files selected for processing (1)
  • packages/trip-proposal-ui/src/components/ProposalShell.tsx
📝 Walkthrough

Walkthrough

Adds a new private UI package @tailfire/trip-proposal-ui with many proposal-related components and a Tailwind preset; integrates it into the admin app, replaces token-based preview flow with a new /trips/[id]/preview route, and adds a usePreviewProposal hook and server preview behavior change to include all itineraries.

Changes

Cohort / File(s) Summary
Admin config & deps
apps/admin/next.config.ts, apps/admin/package.json, apps/admin/tailwind.config.ts
Transpile new workspace packages, add @tailfire/trip-proposal-ui dependency, extend Tailwind content globs, and include external Tailwind preset.
Admin trip pages & hooks
apps/admin/src/app/trips/[id]/page.tsx, apps/admin/src/app/trips/[id]/preview/page.tsx, apps/admin/src/hooks/use-trips.ts
Removed async token-based preview flow from TripDetailPage; add synchronous navigation to /trips/{id}/preview; new client-side TripPreviewPage with loading/error/data states; added usePreviewProposal hook to fetch preview proposal.
API preview logic
apps/api/src/trips/trips.service.ts
Changed previewProposal to build proposal itineraries for all itineraries (no longer filters by status === 'proposing').
New package manifest & TS config
packages/trip-proposal-ui/package.json, packages/trip-proposal-ui/tsconfig.json, packages/trip-proposal-ui/src/tailwind.preset.ts, packages/trip-proposal-ui/src/index.ts
Create new workspace package with export map (including ./tailwind.preset), set peers/devDeps, add tsconfig, and expose Tailwind preset and public re-exports.
Core proposal UI components
packages/trip-proposal-ui/src/components/...
Add ProposalShell, ProposalHero, ItineraryNav, DaySection, ActivityCard, ActivityDetailModal, PricingSummary, AgentProfileCard, and other orchestration/presentation components for proposals, pricing, itinerary navigation, and activity interactions.
Comparison & selection views
packages/trip-proposal-ui/src/components/SideBySideComparison.tsx, packages/trip-proposal-ui/src/components/SummaryComparison.tsx
Add side-by-side and summary comparison UI for multiple itineraries with selection CTA and selection state handling.
Detail renderers
packages/trip-proposal-ui/src/components/details/*
Add type-specific detail components (Flight, Lodging, Dining, Transport, Cruise, Tour, Package, Generic) used by activity renderers.
Presentation utilities
packages/trip-proposal-ui/src/utils/activity-presentation.tsx
Add icon/status mappings, currency formatting, renderActivityDetail router, and inline PortInfoDetail/OptionsDetail.

Sequence Diagram(s)

sequenceDiagram
  participant Admin as Admin UI (browser)
  participant Hook as usePreviewProposal
  participant API as Admin API (/trips/:id/preview-proposal)
  participant Service as TripsService.previewProposal
  participant DB as Database

  Admin->>Hook: request preview for tripId
  Hook->>API: GET /trips/{tripId}/preview-proposal
  API->>Service: build preview proposal (all itineraries)
  Service->>DB: read trip & itineraries
  DB-->>Service: trip+itineraries
  Service-->>API: preview DTO
  API-->>Hook: preview DTO
  Hook-->>Admin: data resolved
  Admin->>ProposalShell: render ProposalHero, ItineraryNav, DaySection, ActivityDetailModal, etc.
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 A preview sprouted, tabs in line,
Cards and modals, pricing fine,
I hopped through itineraries, two by two,
From flights to ports, each lovely view—
A carrot-toast for code made new!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'feat: admin trip preview + shared proposal UI package' accurately and concisely captures the main changes: adding admin preview functionality and creating a shared proposal UI package.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/trip-preview-shared-ui
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Nitpick comments (9)
apps/admin/src/hooks/use-trips.ts (1)

601-606: Non-null assertion differs from existing hook patterns.

The existing useTrip hook uses tripKeys.detail(id || '') with enabled: !!id, while this new hook uses tripKeys.detail(tripId!). While safe due to the enabled guard, using the empty string fallback would be more consistent with the codebase.

♻️ Proposed fix for consistency
 export function usePreviewProposal(tripId: string | undefined) {
   return useQuery({
-    queryKey: [...tripKeys.detail(tripId!), 'preview-proposal'],
+    queryKey: [...tripKeys.detail(tripId ?? ''), 'preview-proposal'],
     queryFn: () => api.get<SharedTripProposalDto>(`/trips/${tripId}/preview-proposal`),
     enabled: !!tripId,
   })
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/admin/src/hooks/use-trips.ts` around lines 601 - 606, In
usePreviewProposal, replace the non-null assertion tripId! in the queryKey with
the same empty-string fallback pattern used elsewhere (e.g.,
tripKeys.detail(tripId || '')) so the key is stable even when tripId is
undefined; keep enabled: !!tripId and the queryFn as-is
(api.get...`/trips/${tripId}/preview-proposal`) so behavior remains gated by
enabled while matching the existing hook pattern.
packages/trip-proposal-ui/src/components/PricingSummary.tsx (1)

21-28: Duplicate formatCurrency with inconsistent formatting.

This local formatCurrency uses maximumFractionDigits: 2, while the shared utility in packages/trip-proposal-ui/src/utils/activity-presentation.tsx uses maximumFractionDigits: 0. This causes inconsistent price displays (e.g., "$100.00" here vs "$100" elsewhere), undermining the package's goal of zero drift between admin preview and client views.

♻️ Proposed fix: use the shared utility
 'use client'
 
 import { Card, CardContent, Separator } from '@tailfire/ui-public'
 import type { SharedItineraryDto } from '@tailfire/shared-types/api'
+import { formatCurrency } from '../utils/activity-presentation'
 
 const typeLabels: Record<string, string> = {
   flight: 'Flights',
   // ... rest of labels
 }
 
-function formatCurrency(cents: number, currency: string) {
-  return new Intl.NumberFormat('en-US', {
-    style: 'currency',
-    currency,
-    minimumFractionDigits: 0,
-    maximumFractionDigits: 2,
-  }).format(cents / 100)
-}
-
 export function PricingSummary({
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/PricingSummary.tsx` around lines 21
- 28, PricingSummary.tsx defines a local formatCurrency that differs from the
shared utility causing inconsistent displays; remove the duplicate local
function and instead import and use the shared formatCurrency from
activity-presentation (the utility in activity-presentation.tsx) so both admin
preview and client views use the same formatting (ensure you replace calls to
the local formatCurrency with the imported one and delete the local definition).
packages/trip-proposal-ui/tsconfig.json (1)

12-12: Excluding tailwind.preset.ts from typecheck may hide errors.

The preset is excluded from TypeScript compilation, meaning pnpm typecheck on this package won't catch type errors in it. While it's transpiled by Next.js's transpilePackages, consider removing it from the exclude list to ensure type errors are caught during development.

♻️ Proposed fix
-  "exclude": ["node_modules", "dist", "src/tailwind.preset.ts"]
+  "exclude": ["node_modules", "dist"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/tsconfig.json` at line 12, The tsconfig exclude
array currently omits "src/tailwind.preset.ts", preventing TypeScript from
typechecking that file; remove "src/tailwind.preset.ts" from the "exclude" list
in the tsconfig.json so pnpm typecheck will include it (or alternatively add it
explicitly to "include"), ensuring the preset file (src/tailwind.preset.ts) is
compiled and any type errors surface during development.
packages/trip-proposal-ui/src/components/details/TourDetail.tsx (1)

26-32: Consider using day.dayNumber as the React key.

Using dayNumber as the key would be more semantically meaningful and stable than the array index, since itinerary days are inherently ordered by their day number.

♻️ Suggested change
-              <div key={i} className="text-xs text-muted-foreground pl-2 border-l border-border">
+              <div key={day.dayNumber} className="text-xs text-muted-foreground pl-2 border-l border-border">
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/details/TourDetail.tsx` around lines
26 - 32, Replace the unstable array index key in the TourDetail itinerary map
with a stable, semantic identifier: change the element keyed by key={i} in the
JSX inside the detail.itineraryDays.map callback to use day.dayNumber (or a
stringified variant like `${day.dayNumber}`) so each day div uses a consistent
unique key tied to the itinerary item; update the map expression that renders
Day {day.dayNumber} to use that new key.
packages/trip-proposal-ui/src/components/AgentProfileCard.tsx (1)

30-37: Add aria-label to the phone link for accessibility.

The phone link contains only an icon with no visible text. Screen reader users would benefit from an accessible label.

♿ Suggested improvement
           {agent.publicPhone && (
             <a
               href={`tel:${agent.publicPhone}`}
               className="shrink-0 text-primary hover:text-primary/80 transition-colors"
+              aria-label={`Call ${fullName}`}
             >
               <Phone className="h-5 w-5" />
             </a>
           )}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/AgentProfileCard.tsx` around lines
30 - 37, The phone icon link in AgentProfileCard (the anchor using
agent.publicPhone and the Phone component) has no accessible text; add an
aria-label (for example aria-label={`Call ${agent.name}` or a localized
equivalent) to the <a> element so screen readers announce the purpose of the
link, keeping the existing href={`tel:${agent.publicPhone}`} and visual markup
unchanged.
packages/trip-proposal-ui/src/components/ProposalHero.tsx (1)

29-91: Consider extracting shared hero content to reduce duplication.

The two layout branches (with/without cover photo) share identical content structure (badge, title, dates, description). Extracting this into a shared inner component would reduce ~30 lines of duplication and make future changes easier.

♻️ Optional refactor to extract shared content
+function HeroContent({
+  tripType,
+  name,
+  startDate,
+  endDate,
+  description,
+  hasPhoto,
+}: {
+  tripType: string | null
+  name: string
+  startDate: string | null
+  endDate: string | null
+  description: string | null
+  hasPhoto: boolean
+}) {
+  const shadowClass = hasPhoto ? 'text-shadow-hero' : ''
+  return (
+    <div className="max-w-3xl mx-auto w-full">
+      {tripType && (
+        <Badge variant="outline" className="mb-2 bg-primary/10 text-primary border-primary/20">
+          {tripType}
+        </Badge>
+      )}
+      <h1 className={`font-display text-3xl md:text-4xl font-bold text-white mb-2 ${shadowClass}`}>
+        {name}
+      </h1>
+      {(startDate || endDate) && (
+        <p className={`text-white/80 text-lg ${shadowClass}`}>
+          {formatDate(startDate)}
+          {startDate && endDate && ' — '}
+          {formatDate(endDate)}
+        </p>
+      )}
+      {description && (
+        <p className={`text-white/80 mt-3 leading-relaxed ${shadowClass}`}>{description}</p>
+      )}
+    </div>
+  )
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/ProposalHero.tsx` around lines 29 -
91, ProposalHero duplicates the inner hero content (Badge, h1 name, date block
using formatDate, and description) across the coverPhotoUrl branches; extract
that repeated block into a small shared component or render function (e.g.,
HeroContent or renderHeroContent) and replace the duplicated JSX in both
branches with a single call to that helper; reference the existing symbols
ProposalHero, Badge, formatDate, name, startDate, endDate, and description so
the new helper can accept those props/variables and preserve styling and
conditional rendering.
packages/trip-proposal-ui/src/components/SummaryComparison.tsx (1)

15-21: formatCurrency is duplicated — consider importing from shared utility.

This function already exists in utils/activity-presentation.tsx and is exported from the package index. Importing it would reduce duplication.

♻️ Proposed fix
 'use client'
 
 import type { SharedItineraryDto } from '@tailfire/shared-types/api'
 import { Button } from '@tailfire/ui-public'
 import { Check } from 'lucide-react'
+import { formatCurrency } from '../utils/activity-presentation'
 
 interface SummaryComparisonProps {
   itineraries: SharedItineraryDto[]
   currency: string
   pricingVisible: boolean
   clientSelectedId: string | null
   onSelect?: (itineraryId: string) => void
 }
 
-function formatCurrency(cents: number, currency: string) {
-  return new Intl.NumberFormat('en-US', {
-    style: 'currency',
-    currency,
-    minimumFractionDigits: 0,
-  }).format(cents / 100)
-}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/SummaryComparison.tsx` around lines
15 - 21, Duplicate formatCurrency implementation: remove the local
formatCurrency function in SummaryComparison.tsx and import the existing
exported formatCurrency from the shared utility (the implementation in
utils/activity-presentation.tsx that is re-exported from the package index)
instead; update the file to import { formatCurrency } from the package root (or
the same path used by other components) and ensure all uses of formatCurrency in
SummaryComparison refer to the imported symbol.
packages/trip-proposal-ui/src/components/DaySection.tsx (1)

11-18: Consider extracting formatDayDate to a shared utility.

This date formatting logic appears in multiple components (ProposalHero, DaySection, and likely others). Extracting it to utils/ would reduce duplication and ensure consistent formatting across the package.

♻️ Proposed shared utility

Create packages/trip-proposal-ui/src/utils/format-date.ts:

export function formatDayDate(dateStr: string | null) {
  if (!dateStr) return null
  return new Date(dateStr + 'T00:00:00').toLocaleDateString('en-US', {
    weekday: 'long',
    month: 'long',
    day: 'numeric',
  })
}

export function formatShortDate(dateStr: string | null) {
  if (!dateStr) return null
  return new Date(dateStr + 'T00:00:00').toLocaleDateString('en-US', {
    year: 'numeric',
    month: 'long',
    day: 'numeric',
  })
}

Then import from the shared utility in each component.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/DaySection.tsx` around lines 11 -
18, Extract the duplicate date formatting logic from DaySection (formatDayDate)
into a shared utility module (e.g., create a new format-date.ts exporting
formatDayDate and formatShortDate), move the existing implementation there, and
then replace the local formatDayDate in DaySection and the same logic in
ProposalHero (and any other components) with imports from the new format-date
utility; ensure the exported functions keep the same signatures
(formatDayDate(dateStr: string | null) and formatShortDate(dateStr: string |
null)) and update imports so components use these shared functions instead of
their local implementations.
packages/trip-proposal-ui/src/components/ItineraryNav.tsx (1)

31-52: Consider adding accessibility attributes for toggle buttons.

The itinerary tabs function as a tab list but lack ARIA attributes. Adding role="tablist" to the container and aria-selected to buttons would improve screen reader support.

♿ Optional accessibility improvement
-        <div className="flex gap-2 overflow-x-auto">
+        <div className="flex gap-2 overflow-x-auto" role="tablist" aria-label="Itinerary options">
           {itineraries.map((it) => (
             <button
               key={it.id}
+              role="tab"
+              aria-selected={activeTab === it.id}
               onClick={() => {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/ItineraryNav.tsx` around lines 31 -
52, The tab buttons in ItineraryNav.tsx are missing ARIA roles/attributes;
update the container that maps the buttons to include role="tablist" and change
each button to expose role="tab" and aria-selected={activeTab === it.id}; also
ensure keyboard focusability by setting tabIndex={activeTab === it.id ? 0 : -1}
(and keep existing onClick handlers onTabChange and onViewModeChange intact) and
include clientSelectedId visual indicator only—this will improve screen reader
and keyboard accessibility for the tab interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/admin/src/app/trips/`[id]/preview/page.tsx:
- Around line 44-64: The error UI branch in the Preview page doesn't capture
runtime errors to Sentry; add a useEffect at the top of the TripsPreviewPage
component (before any conditional returns) that runs when error or tripId
changes and calls Sentry.captureException(error, { tags: { environment:
process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT }, extra: { tripId } }) so errors are
reported and tagged; ensure the useEffect is placed in the component body (not
inside the error conditional) and only runs when error is truthy.

In `@packages/trip-proposal-ui/src/components/ActivityCard.tsx`:
- Around line 19-37: The ActivityCard currently renders activity.pricing
unconditionally; add a new prop pricingVisible?: boolean to the ActivityCard
signature and use it to conditionally render the pricing block (references:
ActivityCard, activity.pricing). Update any callers (e.g., DaySection) to
forward the trip-level pricingVisible flag down into ActivityCard, and ensure
other components that render ActivityCard (modal invocation in ProposalShell and
any list views) also pass the flag so cards respect pricingVisible = false. Keep
default behavior unchanged by defaulting the prop to true.
- Around line 114-129: The "Change to …" button shows even when its target
handler is unavailable, leading to a no-op click; update the render logic in
ActivityCard (around the response block where response, responseStyles,
handleConfirm and handleDecline are used) to only render the "Change to ..."
button if the corresponding opposite handler exists (i.e., only show the button
to switch to decline when onDecline/handleDecline is defined, and only show the
button to switch to approve when onConfirm/handleConfirm is defined), and keep
the disabled/isSubmitting behavior intact.

In `@packages/trip-proposal-ui/src/components/ActivityDetailModal.tsx`:
- Around line 116-123: The location/address block in ActivityDetailModal.tsx
currently only renders when activity.location exists, causing activities with
only activity.address to be hidden; update the conditional to render when either
activity.location or activity.address is present (e.g., change the gate from
activity.location to (activity.location || activity.address)), keep the MapPin
and span but display activity.location if present otherwise the address, and
still append " — {activity.address}" only when both exist so address-only
records still show location context.

In `@packages/trip-proposal-ui/src/components/details/DiningDetail.tsx`:
- Around line 16-21: The current rendering in DiningDetail.tsx uses
detail.reservationDate and detail.reservationTime together, which produces
"Reservation: at 12:00" when reservationDate is null; update the conditional
rendering around detail.reservationDate and detail.reservationTime so you output
a sensible label depending on which exists (e.g., if both exist render
"Reservation: {reservationDate} at {reservationTime.slice(0,5)}", if only
reservationTime render "Reservation: {reservationTime.slice(0,5)}" or
"Reservation time: {reservationTime.slice(0,5)}", and if only reservationDate
render "Reservation: {reservationDate}"), adjusting the JSX that references
detail.reservationDate and detail.reservationTime to perform these checks and
avoid the stray "at".

In `@packages/trip-proposal-ui/src/components/details/PackageDetail.tsx`:
- Around line 20-26: PackageDetail can recurse indefinitely because
childActivities may include packages; thread a numeric depth prop through
ActivityCard and renderActivityDetail (e.g., add optional prop depth: number,
default 0) and enforce a maxDepth (choose a constant like MAX_PACKAGE_DEPTH =
5). When ActivityCard calls renderActivityDetail, pass depth (or depth + 1 when
rendering a child PackageDetail) and have PackageDetail check depth >=
MAX_PACKAGE_DEPTH and stop recursing (e.g., skip rendering child package details
or render a safe placeholder message) instead of rendering nested PackageDetail;
update ActivityCard, renderActivityDetail, and PackageDetail signatures to
accept and increment depth while preserving the existing nested styling prop.

In `@packages/trip-proposal-ui/src/components/SideBySideComparison.tsx`:
- Around line 84-88: The render logic hides zero-cost items because the
condition uses a truthy check; update the conditional that references
pricingVisible and activity.pricing?.totalPriceCents in SideBySideComparison
(the JSX that calls formatCurrency(activity.pricing.totalPriceCents, currency))
to use a nullish/exists check instead (e.g., ensure you test that
activity.pricing?.totalPriceCents is not null/undefined rather than truthy) so
$0 values still render.

In `@packages/trip-proposal-ui/src/utils/activity-presentation.tsx`:
- Around line 86-90: The conditional should render the row when either dates or
times exist and show the appropriate value for each port; change the guard to
check (detail.arrivalDate || detail.departureDate || detail.arrivalTime ||
detail.departureTime) and inside render "Arrive:" using
detail.arrivalTime.slice(0,5) if arrivalTime exists otherwise show
detail.arrivalDate, and similarly render "Depart:" using
detail.departureTime.slice(0,5) if departureTime exists otherwise
detail.departureDate so you don't produce an empty line for date-only entries or
hide time-only entries.
- Around line 40-46: The formatter formatCurrency currently sets
minimumFractionDigits and maximumFractionDigits to 0 which strips cents for
cent-based input; update formatCurrency to preserve cents by using two decimal
places (set minimumFractionDigits and maximumFractionDigits to 2) so the
returned string reflects cents (e.g., $123.45), ensuring ActivityCard and
ActivityDetailModal that call formatCurrency show consistent pricing like
PricingSummary does.

---

Nitpick comments:
In `@apps/admin/src/hooks/use-trips.ts`:
- Around line 601-606: In usePreviewProposal, replace the non-null assertion
tripId! in the queryKey with the same empty-string fallback pattern used
elsewhere (e.g., tripKeys.detail(tripId || '')) so the key is stable even when
tripId is undefined; keep enabled: !!tripId and the queryFn as-is
(api.get...`/trips/${tripId}/preview-proposal`) so behavior remains gated by
enabled while matching the existing hook pattern.

In `@packages/trip-proposal-ui/src/components/AgentProfileCard.tsx`:
- Around line 30-37: The phone icon link in AgentProfileCard (the anchor using
agent.publicPhone and the Phone component) has no accessible text; add an
aria-label (for example aria-label={`Call ${agent.name}` or a localized
equivalent) to the <a> element so screen readers announce the purpose of the
link, keeping the existing href={`tel:${agent.publicPhone}`} and visual markup
unchanged.

In `@packages/trip-proposal-ui/src/components/DaySection.tsx`:
- Around line 11-18: Extract the duplicate date formatting logic from DaySection
(formatDayDate) into a shared utility module (e.g., create a new format-date.ts
exporting formatDayDate and formatShortDate), move the existing implementation
there, and then replace the local formatDayDate in DaySection and the same logic
in ProposalHero (and any other components) with imports from the new format-date
utility; ensure the exported functions keep the same signatures
(formatDayDate(dateStr: string | null) and formatShortDate(dateStr: string |
null)) and update imports so components use these shared functions instead of
their local implementations.

In `@packages/trip-proposal-ui/src/components/details/TourDetail.tsx`:
- Around line 26-32: Replace the unstable array index key in the TourDetail
itinerary map with a stable, semantic identifier: change the element keyed by
key={i} in the JSX inside the detail.itineraryDays.map callback to use
day.dayNumber (or a stringified variant like `${day.dayNumber}`) so each day div
uses a consistent unique key tied to the itinerary item; update the map
expression that renders Day {day.dayNumber} to use that new key.

In `@packages/trip-proposal-ui/src/components/ItineraryNav.tsx`:
- Around line 31-52: The tab buttons in ItineraryNav.tsx are missing ARIA
roles/attributes; update the container that maps the buttons to include
role="tablist" and change each button to expose role="tab" and
aria-selected={activeTab === it.id}; also ensure keyboard focusability by
setting tabIndex={activeTab === it.id ? 0 : -1} (and keep existing onClick
handlers onTabChange and onViewModeChange intact) and include clientSelectedId
visual indicator only—this will improve screen reader and keyboard accessibility
for the tab interface.

In `@packages/trip-proposal-ui/src/components/PricingSummary.tsx`:
- Around line 21-28: PricingSummary.tsx defines a local formatCurrency that
differs from the shared utility causing inconsistent displays; remove the
duplicate local function and instead import and use the shared formatCurrency
from activity-presentation (the utility in activity-presentation.tsx) so both
admin preview and client views use the same formatting (ensure you replace calls
to the local formatCurrency with the imported one and delete the local
definition).

In `@packages/trip-proposal-ui/src/components/ProposalHero.tsx`:
- Around line 29-91: ProposalHero duplicates the inner hero content (Badge, h1
name, date block using formatDate, and description) across the coverPhotoUrl
branches; extract that repeated block into a small shared component or render
function (e.g., HeroContent or renderHeroContent) and replace the duplicated JSX
in both branches with a single call to that helper; reference the existing
symbols ProposalHero, Badge, formatDate, name, startDate, endDate, and
description so the new helper can accept those props/variables and preserve
styling and conditional rendering.

In `@packages/trip-proposal-ui/src/components/SummaryComparison.tsx`:
- Around line 15-21: Duplicate formatCurrency implementation: remove the local
formatCurrency function in SummaryComparison.tsx and import the existing
exported formatCurrency from the shared utility (the implementation in
utils/activity-presentation.tsx that is re-exported from the package index)
instead; update the file to import { formatCurrency } from the package root (or
the same path used by other components) and ensure all uses of formatCurrency in
SummaryComparison refer to the imported symbol.

In `@packages/trip-proposal-ui/tsconfig.json`:
- Line 12: The tsconfig exclude array currently omits "src/tailwind.preset.ts",
preventing TypeScript from typechecking that file; remove
"src/tailwind.preset.ts" from the "exclude" list in the tsconfig.json so pnpm
typecheck will include it (or alternatively add it explicitly to "include"),
ensuring the preset file (src/tailwind.preset.ts) is compiled and any type
errors surface during development.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 012e6591-2074-431c-9371-f35dc234cc1c

📥 Commits

Reviewing files that changed from the base of the PR and between b4c8ac8 and 6b2bfe3.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (29)
  • apps/admin/next.config.ts
  • apps/admin/package.json
  • apps/admin/src/app/trips/[id]/page.tsx
  • apps/admin/src/app/trips/[id]/preview/page.tsx
  • apps/admin/src/hooks/use-trips.ts
  • apps/admin/tailwind.config.ts
  • packages/trip-proposal-ui/package.json
  • packages/trip-proposal-ui/src/components/ActivityCard.tsx
  • packages/trip-proposal-ui/src/components/ActivityDetailModal.tsx
  • packages/trip-proposal-ui/src/components/AgentProfileCard.tsx
  • packages/trip-proposal-ui/src/components/DaySection.tsx
  • packages/trip-proposal-ui/src/components/ItineraryNav.tsx
  • packages/trip-proposal-ui/src/components/PricingSummary.tsx
  • packages/trip-proposal-ui/src/components/ProposalHero.tsx
  • packages/trip-proposal-ui/src/components/ProposalShell.tsx
  • packages/trip-proposal-ui/src/components/SideBySideComparison.tsx
  • packages/trip-proposal-ui/src/components/SummaryComparison.tsx
  • packages/trip-proposal-ui/src/components/details/CruiseDetail.tsx
  • packages/trip-proposal-ui/src/components/details/DiningDetail.tsx
  • packages/trip-proposal-ui/src/components/details/FlightDetail.tsx
  • packages/trip-proposal-ui/src/components/details/GenericDetail.tsx
  • packages/trip-proposal-ui/src/components/details/LodgingDetail.tsx
  • packages/trip-proposal-ui/src/components/details/PackageDetail.tsx
  • packages/trip-proposal-ui/src/components/details/TourDetail.tsx
  • packages/trip-proposal-ui/src/components/details/TransportDetail.tsx
  • packages/trip-proposal-ui/src/index.ts
  • packages/trip-proposal-ui/src/tailwind.preset.ts
  • packages/trip-proposal-ui/src/utils/activity-presentation.tsx
  • packages/trip-proposal-ui/tsconfig.json

Comment on lines +44 to +64
if (error || !trip) {
return (
<div className={`${cinzel.variable} ${lato.variable} min-h-screen bg-background`}>
<PreviewBanner onBack={() => router.push(`/trips/${tripId}`)} />
<div className="flex items-center justify-center min-h-[60vh]">
<div className="text-center space-y-3 max-w-md mx-auto px-4">
<AlertTriangle className="h-10 w-10 text-amber-500 mx-auto" />
<h2 className="text-lg font-semibold">Unable to load preview</h2>
<p className="text-muted-foreground text-sm">
{error instanceof Error
? error.message
: 'The trip may not have any itineraries yet. Add at least one itinerary to preview.'}
</p>
<Button variant="outline" onClick={() => router.push(`/trips/${tripId}`)}>
Back to Editor
</Button>
</div>
</div>
</div>
)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing Sentry error capture for runtime errors.

The error state renders UI feedback but does not capture the error to Sentry. As per coding guidelines, the admin application must capture runtime errors with Sentry tagged by environment.

🛡️ Proposed fix to add Sentry error capture
 'use client'
 
 import { useParams, useRouter } from 'next/navigation'
 import { Cinzel, Lato } from 'next/font/google'
 import { ArrowLeft, AlertTriangle } from 'lucide-react'
 import { Button } from '@/components/ui/button'
 import { usePreviewProposal } from '@/hooks/use-trips'
 import { ProposalHero, AgentProfileCard, ProposalShell } from '@tailfire/trip-proposal-ui'
+import * as Sentry from '@sentry/nextjs'
+import { useEffect } from 'react'

Then add error capture before the error UI return:

   if (error || !trip) {
+    // Capture error to Sentry
+    useEffect(() => {
+      if (error) {
+        Sentry.captureException(error, {
+          tags: { environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT },
+          extra: { tripId },
+        })
+      }
+    }, [error, tripId])
+
     return (

Note: The useEffect should be moved to the component body before conditional returns for proper hook ordering. A better pattern would be:

// At component level, before any returns:
useEffect(() => {
  if (error) {
    Sentry.captureException(error, {
      tags: { environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT },
      extra: { tripId },
    })
  }
}, [error, tripId])

As per coding guidelines: "Admin application must capture runtime errors with Sentry. Errors must be tagged with NEXT_PUBLIC_SENTRY_ENVIRONMENT."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/admin/src/app/trips/`[id]/preview/page.tsx around lines 44 - 64, The
error UI branch in the Preview page doesn't capture runtime errors to Sentry;
add a useEffect at the top of the TripsPreviewPage component (before any
conditional returns) that runs when error or tripId changes and calls
Sentry.captureException(error, { tags: { environment:
process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT }, extra: { tripId } }) so errors are
reported and tagged; ensure the useEffect is placed in the component body (not
inside the error conditional) and only runs when error is truthy.

Comment on lines +19 to +37
export function ActivityCard({
activity,
currency,
nested = false,
commentButton,
onClickTitle,
response,
onConfirm,
onDecline,
}: {
activity: SharedActivityDto
currency: string
nested?: boolean
commentButton?: React.ReactNode
onClickTitle?: () => void
response?: ClientActivityResponseType | null
onConfirm?: () => void
onDecline?: () => void
}) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Expose pricingVisible on the card.

packages/trip-proposal-ui/src/components/ProposalShell.tsx, Line 27, already models trip-level pricing visibility, and Lines 104-108 pass that flag into the modal. This component still renders activity.pricing unconditionally, so any card-based itinerary view cannot honor pricingVisible = false.

Suggested patch
 export function ActivityCard({
   activity,
   currency,
+  pricingVisible = true,
   nested = false,
   commentButton,
   onClickTitle,
   response,
   onConfirm,
   onDecline,
 }: {
   activity: SharedActivityDto
   currency: string
+  pricingVisible?: boolean
   nested?: boolean
   commentButton?: React.ReactNode
   onClickTitle?: () => void
   response?: ClientActivityResponseType | null
   onConfirm?: () => void
   onDecline?: () => void
@@
-            {activity.pricing && (
+            {pricingVisible && activity.pricing && (
               <span className="text-sm font-medium text-primary whitespace-nowrap">
                 {formatCurrency(activity.pricing.totalPriceCents, activity.pricing.currency)}
               </span>
             )}

Callers like DaySection will also need to forward the trip-level flag.

Also applies to: 78-82

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/ActivityCard.tsx` around lines 19 -
37, The ActivityCard currently renders activity.pricing unconditionally; add a
new prop pricingVisible?: boolean to the ActivityCard signature and use it to
conditionally render the pricing block (references: ActivityCard,
activity.pricing). Update any callers (e.g., DaySection) to forward the
trip-level pricingVisible flag down into ActivityCard, and ensure other
components that render ActivityCard (modal invocation in ProposalShell and any
list views) also pass the flag so cards respect pricingVisible = false. Keep
default behavior unchanged by defaulting the prop to true.

Comment on lines +114 to +129
{(onConfirm || onDecline) && (
<div className="mt-3 pt-3 border-t border-border/50 flex items-center justify-between">
{response ? (
<div className="flex items-center gap-2">
<Badge variant="outline" className={responseStyles[response].className}>
{responseStyles[response].label}
</Badge>
<button
type="button"
onClick={response === 'confirmed' ? handleDecline : handleConfirm}
className="text-xs text-muted-foreground hover:text-foreground transition-colors"
disabled={isSubmitting}
>
Change to {response === 'confirmed' ? 'decline' : 'approve'}
</button>
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Don't render a no-op “Change to …” action.

The API allows only one of onConfirm / onDecline, but this branch always shows the opposite-action button once response exists. In the one-handler case the click silently does nothing because handleConfirm / handleDecline immediately return.

Suggested patch
             {response ? (
               <div className="flex items-center gap-2">
                 <Badge variant="outline" className={responseStyles[response].className}>
                   {responseStyles[response].label}
                 </Badge>
-                <button
-                  type="button"
-                  onClick={response === 'confirmed' ? handleDecline : handleConfirm}
-                  className="text-xs text-muted-foreground hover:text-foreground transition-colors"
-                  disabled={isSubmitting}
-                >
-                  Change to {response === 'confirmed' ? 'decline' : 'approve'}
-                </button>
+                {((response === 'confirmed' && onDecline) ||
+                  (response === 'declined' && onConfirm)) && (
+                  <button
+                    type="button"
+                    onClick={response === 'confirmed' ? handleDecline : handleConfirm}
+                    className="text-xs text-muted-foreground hover:text-foreground transition-colors"
+                    disabled={isSubmitting}
+                  >
+                    Change to {response === 'confirmed' ? 'decline' : 'approve'}
+                  </button>
+                )}
               </div>
             ) : (
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{(onConfirm || onDecline) && (
<div className="mt-3 pt-3 border-t border-border/50 flex items-center justify-between">
{response ? (
<div className="flex items-center gap-2">
<Badge variant="outline" className={responseStyles[response].className}>
{responseStyles[response].label}
</Badge>
<button
type="button"
onClick={response === 'confirmed' ? handleDecline : handleConfirm}
className="text-xs text-muted-foreground hover:text-foreground transition-colors"
disabled={isSubmitting}
>
Change to {response === 'confirmed' ? 'decline' : 'approve'}
</button>
</div>
{(onConfirm || onDecline) && (
<div className="mt-3 pt-3 border-t border-border/50 flex items-center justify-between">
{response ? (
<div className="flex items-center gap-2">
<Badge variant="outline" className={responseStyles[response].className}>
{responseStyles[response].label}
</Badge>
{((response === 'confirmed' && onDecline) ||
(response === 'declined' && onConfirm)) && (
<button
type="button"
onClick={response === 'confirmed' ? handleDecline : handleConfirm}
className="text-xs text-muted-foreground hover:text-foreground transition-colors"
disabled={isSubmitting}
>
Change to {response === 'confirmed' ? 'decline' : 'approve'}
</button>
)}
</div>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/ActivityCard.tsx` around lines 114 -
129, The "Change to …" button shows even when its target handler is unavailable,
leading to a no-op click; update the render logic in ActivityCard (around the
response block where response, responseStyles, handleConfirm and handleDecline
are used) to only render the "Change to ..." button if the corresponding
opposite handler exists (i.e., only show the button to switch to decline when
onDecline/handleDecline is defined, and only show the button to switch to
approve when onConfirm/handleConfirm is defined), and keep the
disabled/isSubmitting behavior intact.

Comment on lines +116 to +123
{activity.location && (
<div className="flex items-center gap-2 text-sm text-muted-foreground">
<MapPin className="h-4 w-4 shrink-0" />
<span>
{activity.location}
{activity.address && ` — ${activity.address}`}
</span>
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Show address-only activities.

This section is gated on activity.location, so records that only carry address lose their location context entirely. Render when either field exists.

Suggested patch
-          {activity.location && (
+          {(activity.location || activity.address) && (
             <div className="flex items-center gap-2 text-sm text-muted-foreground">
               <MapPin className="h-4 w-4 shrink-0" />
               <span>
-                {activity.location}
-                {activity.address && ` — ${activity.address}`}
+                {activity.location || activity.address}
+                {activity.location && activity.address && ` — ${activity.address}`}
               </span>
             </div>
           )}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{activity.location && (
<div className="flex items-center gap-2 text-sm text-muted-foreground">
<MapPin className="h-4 w-4 shrink-0" />
<span>
{activity.location}
{activity.address && ` — ${activity.address}`}
</span>
</div>
{(activity.location || activity.address) && (
<div className="flex items-center gap-2 text-sm text-muted-foreground">
<MapPin className="h-4 w-4 shrink-0" />
<span>
{activity.location || activity.address}
{activity.location && activity.address && ` — ${activity.address}`}
</span>
</div>
)}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/ActivityDetailModal.tsx` around
lines 116 - 123, The location/address block in ActivityDetailModal.tsx currently
only renders when activity.location exists, causing activities with only
activity.address to be hidden; update the conditional to render when either
activity.location or activity.address is present (e.g., change the gate from
activity.location to (activity.location || activity.address)), keep the MapPin
and span but display activity.location if present otherwise the address, and
still append " — {activity.address}" only when both exist so address-only
records still show location context.

Comment on lines +16 to +21
{(detail.reservationDate || detail.reservationTime) && (
<div className="text-muted-foreground">
Reservation: {detail.reservationDate}
{detail.reservationTime && <span> at {detail.reservationTime.slice(0, 5)}</span>}
</div>
)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor display issue when only reservationTime exists.

If reservationDate is null but reservationTime is present, this renders "Reservation: at 12:00" which reads awkwardly. Consider adjusting the text to handle this edge case.

💡 Suggested improvement
         {(detail.reservationDate || detail.reservationTime) && (
           <div className="text-muted-foreground">
-            Reservation: {detail.reservationDate}
+            Reservation:{detail.reservationDate && ` ${detail.reservationDate}`}
             {detail.reservationTime && <span> at {detail.reservationTime.slice(0, 5)}</span>}
           </div>
         )}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/details/DiningDetail.tsx` around
lines 16 - 21, The current rendering in DiningDetail.tsx uses
detail.reservationDate and detail.reservationTime together, which produces
"Reservation: at 12:00" when reservationDate is null; update the conditional
rendering around detail.reservationDate and detail.reservationTime so you output
a sensible label depending on which exists (e.g., if both exist render
"Reservation: {reservationDate} at {reservationTime.slice(0,5)}", if only
reservationTime render "Reservation: {reservationTime.slice(0,5)}" or
"Reservation time: {reservationTime.slice(0,5)}", and if only reservationDate
render "Reservation: {reservationDate}"), adjusting the JSX that references
detail.reservationDate and detail.reservationTime to perform these checks and
avoid the stray "at".

Comment on lines +20 to +26
{detail.childActivities.length > 0 && (
<div className="pl-4 border-l border-border space-y-2">
{detail.childActivities.map((child) => (
<ActivityCard key={child.id} activity={child} currency={currency} nested />
))}
</div>
)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there's any existing recursion guard in ActivityCard or renderActivityDetail
rg -n "depth|maxDepth|MAX_DEPTH|recursion" packages/trip-proposal-ui/src/

Repository: Systemsaholic/tailfire

Length of output: 48


🏁 Script executed:

cat -n packages/trip-proposal-ui/src/components/details/PackageDetail.tsx

Repository: Systemsaholic/tailfire

Length of output: 1146


🏁 Script executed:

cat -n packages/trip-proposal-ui/src/components/ActivityCard.tsx

Repository: Systemsaholic/tailfire

Length of output: 7021


🏁 Script executed:

rg -A 30 "renderActivityDetail" packages/trip-proposal-ui/src/

Repository: Systemsaholic/tailfire

Length of output: 15243


🏁 Script executed:

rg -A 10 "SharedPackageDetailDto" packages/shared-types/src/ --type ts

Repository: Systemsaholic/tailfire

Length of output: 1571


🏁 Script executed:

rg -B 5 -A 5 "childActivities" packages/shared-types/src/ --type ts

Repository: Systemsaholic/tailfire

Length of output: 784


🏁 Script executed:

rg -A 20 "export interface SharedActivityDto" packages/shared-types/src/ --type ts

Repository: Systemsaholic/tailfire

Length of output: 1560


Add a recursion guard to prevent stack overflow when packages contain nested packages.

The type system allows SharedPackageDetailDto.childActivities to contain activities with detail.type === 'package', which creates an infinite recursion path:

  1. PackageDetail renders ActivityCard for each child activity (line 23)
  2. ActivityCard calls renderActivityDetail when activity.detail exists (ActivityCard.tsx:104)
  3. renderActivityDetail renders PackageDetail again when detail.type === 'package'

If the backend provides a package containing a child activity that is also a package, this causes a stack overflow. The nested prop only affects styling, not rendering behavior.

🛡️ Proposed fix: Add depth limit
 export function PackageDetail({
   detail,
   currency,
+  depth = 0,
 }: {
   detail: SharedPackageDetailDto
   currency: string
+  depth?: number
 }) {
+  const MAX_DEPTH = 3
+  if (depth >= MAX_DEPTH) {
+    return (
+      <div className="text-sm text-muted-foreground">
+        Package contains nested items (depth limit reached)
+      </div>
+    )
+  }
+
   return (
     <div className="space-y-2 text-sm">
       {/* ... */}
       {detail.childActivities.length > 0 && (
         <div className="pl-4 border-l border-border space-y-2">
           {detail.childActivities.map((child) => (
-            <ActivityCard key={child.id} activity={child} currency={currency} nested />
+            <ActivityCard key={child.id} activity={child} currency={currency} nested depth={depth + 1} />
           ))}
         </div>
       )}
     </div>
   )
 }

This requires threading the depth prop through ActivityCard and renderActivityDetail.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/details/PackageDetail.tsx` around
lines 20 - 26, PackageDetail can recurse indefinitely because childActivities
may include packages; thread a numeric depth prop through ActivityCard and
renderActivityDetail (e.g., add optional prop depth: number, default 0) and
enforce a maxDepth (choose a constant like MAX_PACKAGE_DEPTH = 5). When
ActivityCard calls renderActivityDetail, pass depth (or depth + 1 when rendering
a child PackageDetail) and have PackageDetail check depth >= MAX_PACKAGE_DEPTH
and stop recursing (e.g., skip rendering child package details or render a safe
placeholder message) instead of rendering nested PackageDetail; update
ActivityCard, renderActivityDetail, and PackageDetail signatures to accept and
increment depth while preserving the existing nested styling prop.

Comment on lines +84 to +88
{pricingVisible && activity.pricing?.totalPriceCents ? (
<span className="text-muted-foreground shrink-0">
{formatCurrency(activity.pricing.totalPriceCents, currency)}
</span>
) : null}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Render zero-cost items instead of hiding them.

activity.pricing?.totalPriceCents treats 0 as falsy, so complimentary/included activities lose their price in the compare view. Use a nullish check so $0 still renders.

Suggested patch
-                            {pricingVisible && activity.pricing?.totalPriceCents ? (
+                            {pricingVisible && activity.pricing?.totalPriceCents != null ? (
                               <span className="text-muted-foreground shrink-0">
                                 {formatCurrency(activity.pricing.totalPriceCents, currency)}
                               </span>
                             ) : null}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/components/SideBySideComparison.tsx` around
lines 84 - 88, The render logic hides zero-cost items because the condition uses
a truthy check; update the conditional that references pricingVisible and
activity.pricing?.totalPriceCents in SideBySideComparison (the JSX that calls
formatCurrency(activity.pricing.totalPriceCents, currency)) to use a
nullish/exists check instead (e.g., ensure you test that
activity.pricing?.totalPriceCents is not null/undefined rather than truthy) so
$0 values still render.

Comment on lines +40 to +46
export function formatCurrency(cents: number, currency: string) {
return new Intl.NumberFormat('en-US', {
style: 'currency',
currency,
minimumFractionDigits: 0,
maximumFractionDigits: 0,
}).format(cents / 100)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep cents in the shared currency formatter.

cents is a cent-based integer, but maximumFractionDigits: 0 turns $123.45 into $123. ActivityCard and ActivityDetailModal both use this helper, so the rounding bug becomes shared UI drift. packages/trip-proposal-ui/src/components/PricingSummary.tsx, Lines 21-28, already keeps up to two decimals.

Suggested patch
 export function formatCurrency(cents: number, currency: string) {
   return new Intl.NumberFormat('en-US', {
     style: 'currency',
     currency,
     minimumFractionDigits: 0,
-    maximumFractionDigits: 0,
+    maximumFractionDigits: 2,
   }).format(cents / 100)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function formatCurrency(cents: number, currency: string) {
return new Intl.NumberFormat('en-US', {
style: 'currency',
currency,
minimumFractionDigits: 0,
maximumFractionDigits: 0,
}).format(cents / 100)
export function formatCurrency(cents: number, currency: string) {
return new Intl.NumberFormat('en-US', {
style: 'currency',
currency,
minimumFractionDigits: 0,
maximumFractionDigits: 2,
}).format(cents / 100)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/utils/activity-presentation.tsx` around lines
40 - 46, The formatter formatCurrency currently sets minimumFractionDigits and
maximumFractionDigits to 0 which strips cents for cent-based input; update
formatCurrency to preserve cents by using two decimal places (set
minimumFractionDigits and maximumFractionDigits to 2) so the returned string
reflects cents (e.g., $123.45), ensuring ActivityCard and ActivityDetailModal
that call formatCurrency show consistent pricing like PricingSummary does.

Comment on lines +86 to +90
{(detail.arrivalDate || detail.departureDate) && (
<div className="text-muted-foreground">
{detail.arrivalTime && <span>Arrive: {detail.arrivalTime.slice(0, 5)}</span>}
{detail.departureTime && <span> · Depart: {detail.departureTime.slice(0, 5)}</span>}
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix the port schedule guard.

This row is guarded by arrivalDate / departureDate, but it only renders arrivalTime / departureTime. That yields an empty line for date-only data and hides time-only data entirely.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/trip-proposal-ui/src/utils/activity-presentation.tsx` around lines
86 - 90, The conditional should render the row when either dates or times exist
and show the appropriate value for each port; change the guard to check
(detail.arrivalDate || detail.departureDate || detail.arrivalTime ||
detail.departureTime) and inside render "Arrive:" using
detail.arrivalTime.slice(0,5) if arrivalTime exists otherwise show
detail.arrivalDate, and similarly render "Depart:" using
detail.departureTime.slice(0,5) if departureTime exists otherwise
detail.departureDate so you don't produce an empty line for date-only entries or
hide time-only entries.

The previewProposal endpoint filtered to only 'proposing' status itineraries,
causing the preview page to show empty when itineraries had other statuses
(draft, approved, etc.). Preview should show everything the agent is working on.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…DaySection

ProposalShell was missing responseMap, onConfirmActivity, onDeclineActivity,
renderCommentButton, renderDayCommentButton, and renderApprovalSection props.
Activities were rendering without action buttons or response states.

Now matches the full ProposalClientShell rendering with optional interaction
callbacks (omitted in preview mode, provided by client app).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Systemsaholic Systemsaholic merged commit 55027ea into main Mar 19, 2026
4 checks passed
@Systemsaholic Systemsaholic deleted the feature/trip-preview-shared-ui branch March 19, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant