fix hardcoded urls, add dockerfile.

[prapooskur]

No description provided.

[prapooskur]

@dakshshah03

[suaviloquence]

I now have a comprehensive view of the changes. Let me provide my review.

---

PR Review: Multi-Itinerary Support, Dockerization & Environment Improvements

Summary

This PR adds significant new functionality including multi-itinerary support, Docker containerization, environment variable configuration, and various bug fixes and improvements. It spans 50 files with ~6,400 lines added.

---

Key Changes

1. Docker & Deployment (Good)

• Added backend/Dockerfile and frontend/Dockerfile for containerization
• Extended compose.yaml with frontend/backend services
• Updated .env-template with FRONTEND_URL and NEXT_PUBLIC_API_URL

2. Multi-Itinerary Feature (Good)

• Enabled the itinerary router in backend/main.py:48
• Added CRUD endpoints for itineraries in backend/routers/trip/trip.py
• Activities now support itinerary_id field
• Frontend supports itinerary creation, renaming, and deletion

3. API Route Normalization (Good)

• Changed trailing slashes ("/") to empty strings ("") across routers for consistency

4. Environment Variable Handling (Good)

• Replaced hardcoded localhost:8000 URLs with process.env.NEXT_PUBLIC_API_URL
• Fixed password reset email to use Settings.frontend_url

---

Issues & Concerns

1. Security - ESLint Ignored During Builds

// frontend/next.config.ts
eslint: {
  ignoreDuringBuilds: true,
}

This suppresses all linting errors during production builds. Consider fixing the underlying lint issues instead.

2. Potential SQL Injection in Itinerary Endpoints
In backend/routers/trip/itinerary.py and trip.py, the itinerary operations store JSON in a JSONB column. While parameterized queries are used, the itinerary data is stored in a JSON blob which could grow unbounded. Consider adding validation for itinerary limits per trip.

3. Missing Authorization Check in Itinerary Endpoints
backend/routers/trip/trip.py:290 - The list_itineraries endpoint uses WithTrip but the original code checked authorID = jwt.userID. The new code removed this check, potentially allowing any trip collaborator to list itineraries (which may be intentional for collaboration).

4. Unused Import
In backend/routers/trip/itinerary.py:6:

from pydantic import BaseModel  # Used
# But JSONResponse was removed from imports, good

5. Type Safety Improvements (Good)
The frontend changes replace many any types with proper types like Task, Activity, etc. Good improvement.

6. Namespace to Object Conversion
frontend/src/app/lib/endpoints.ts and navigation.ts convert TypeScript namespaces to plain objects. This is a good modernization as namespaces are discouraged in modern TypeScript.

7. Lock File Duplication
Both package-lock.json and pnpm-lock.yaml are present. Consider using only one package manager.

8. Database Export File
dbexport.pgsql is untracked - make sure this doesn't get accidentally committed as it may contain sensitive data.

---

Minor Issues

1. Typo fixed: opactiy-0 → opacity-0 in TripCard.tsx (good catch)

2. Contractions replaced: don't → do not, we'll → we will - seems intentional for consistency

3. Missing newline at end of file in both Dockerfiles

4. datetime.utcnow() is deprecated in Python 3.12+:

# backend/routers/trip/trip.py:320
now = datetime.utcnow().isoformat()

Consider using datetime.now(timezone.utc) instead.

---

Recommendations

1. Fix ESLint issues rather than ignoring them during builds
2. Add validation for maximum itineraries per trip
3. Use consistent package manager - remove either npm or pnpm lock file
4. Add .gitignore entry for dbexport.pgsql if not already present
5. Update deprecated datetime.utcnow() to datetime.now(timezone.utc)
6. Consider adding tests for the new itinerary endpoints

---

Verdict

The PR implements useful features with generally good code quality. The main concerns are the ESLint bypass and the deprecated datetime usage. Would recommend addressing those before merging.

[suaviloquence]

PR Review: A Masterclass in Technical Debt

---

The Good

(This section intentionally left brief)

You containerized the app. Congratulations on achieving what a tutorial could teach in 20 minutes.

---

The Bad, The Ugly, and The Unforgivable

1. You Disabled ESLint. In Production.

// frontend/next.config.ts
eslint: {
  ignoreDuringBuilds: true,
}

This is the "check engine light? just put tape over it" of software engineering. You have lint errors you couldn't fix, so you told the build to shut up. Every future bug that ESLint would have caught is now YOUR fault.

---

2. Import Placement is Chaotic

// frontend/src/app/auth/forgot-password/page.tsx
import { Button } from "@/components/ui/button"

const API_BASE = process.env.NEXT_PUBLIC_API_URL ?? 'http://localhost:8000'
import {
  Card,
  ...

You shoved a variable declaration between imports. This isn't creative expression, it's syntactic anarchy. Every file that does this: LoginCard.tsx, SignupCard.tsx, reset-password/page.tsx. Did copy-paste feel efficient?

---

3. datetime.utcnow() is Deprecated

# backend/routers/trip/trip.py:320
now = datetime.utcnow().isoformat()

Python has been screaming about this since 3.12. Use datetime.now(timezone.utc). But that would require reading deprecation warnings.

---

4. Two Lock Files

frontend/package-lock.json
frontend/pnpm-lock.yaml

Pick. One. Package. Manager. You have npm AND pnpm lock files. Which one is canonical? Which one will betray you in CI when they drift apart? Flip a coin, delete one, and commit to your choice.

---

5. The any Purge Was Incomplete

You replaced some any types with proper types. Admirable. But then:

// frontend/src/app/ui/dashboard/trip-editor/ItineraryTable.tsx:80
const data = (a as unknown as { data?: unknown }).data as Record<string, unknown> | undefined;

You replaced any with unknown and then immediately cast it. This is type safety theater. You're not safer, you're just more verbose. If you're going to cast anyway, at least define a proper interface.

---

6. Magic Strings Everywhere

// frontend/src/app/dashboard/trips/[tripid]/page.tsx:113
if (selectedItineraryId === "default-itinerary") return undefined;

And:

// Same file, line 224
if (deleted_itinerary_id === "default-itinerary") {

The string "default-itinerary" appears multiple times with no constant. When someone typos it as "defualt-itinerary", have fun debugging.

---

7. Inconsistent Error Handling

# backend/routers/trip/trip.py:330
except Exception as e:
    conn.rollback()
    raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e))

You catch Exception, which includes KeyboardInterrupt and SystemExit. Then you expose the raw exception message to the client. Enjoy your information disclosure vulnerability. This pattern repeats in every itinerary endpoint.

---

8. Unbounded JSONB Growth

data["itineraries"].append(itinerary)
cur.execute("UPDATE Trips SET data = %s::jsonb WHERE tripID = %s", ...)

No limit on how many itineraries can be created. No pagination. Someone could create 10,000 itineraries and your SELECT will happily try to load them all. Add a limit or enjoy your future OOM.

---

9. Unused Variables

// frontend/src/app/dashboard/trips/[tripid]/page.tsx
const { invites, mutateInvites } = useTripInvitations(trip);

Previously you destructured error and isLoading too. You removed them but... did you check if the hook still needs to run? You're calling a hook just to get mutateInvites. If the hook does expensive work, you're wasting cycles.

---

10. Database Export in Repo

?? dbexport.pgsql

An untracked database export sitting in your repo. Does it contain user emails? Passwords? API keys? Add it to .gitignore NOW before someone commits it. Actually, delete it entirely. Why is this here?

---

11. Dockerfile Anti-Patterns

# frontend/Dockerfile
COPY . .
RUN pnpm install
RUN pnpm run build

You copy everything, THEN install, THEN build. Every code change invalidates the npm cache. Flip it:

COPY package.json pnpm-lock.yaml ./
RUN pnpm install
COPY . .
RUN pnpm run build

Your builds are slower than they need to be.

Also, no .dockerignore. You're copying node_modules, .git, and who knows what else into your image.

---

12. The useEffect Dependency Lie

// frontend/src/app/ui/dashboard/trip-editor/LocationPicker.tsx
}, [query, location]);

You added location to the dependency array. But location is a prop that represents the current location for biasing search results. If location changes, should the search really re-run with the old query? This smells like "add it to make the linter shut up" without thinking.

---

13. No Loading States for New Operations

const handleCreateItinerary = async (name?: string) => {
  if (!trip_id) return;
  try {
    const endpoint = `${APIEndpoints.Trips}/${trip_id}/itineraries`;
    // No loading state. User can spam click.
    const created = await auth_fetcher(endpoint, { ... });

User clicks "New Itinerary" five times fast. Five itineraries created. Add a loading/disabled state.

---

14. Hardcoded Fallback URLs

const API_BASE = process.env.NEXT_PUBLIC_API_URL ?? 'http://localhost:8000'

This appears in FOUR separate files. Make a shared constant. When localhost changes to 127.0.0.1 or port 8080, you'll update three files and miss one.

---

15. Silent Failures

// frontend/src/app/ui/dashboard/trip-editor/ItineraryViewer.tsx:145
} catch (e) {
  console.error("Failed to rename itinerary:", e);
}

Rename fails. User sees... nothing. No toast, no error message, no indication anything went wrong. The modal just closes. User thinks it worked. It didn't.

---

16. Inconsistent Null Handling

itinerary_id: selectedItineraryId ?? null,

vs

itinerary_id: string | null = None  // Python side

Sometimes you use null, sometimes undefined, sometimes you check for both. Pick a convention. Your frontend sends null, but does your backend handle undefined from optional fields?

---

17. React Key Anti-Pattern

// frontend/src/app/ui/dashboard/trip-editor/LocationPicker.tsx:132
key={props.osm_id ?? `feature-${idx}`}

Using index as a fallback key. When the list reorders, React will mangle your DOM. If osm_id is truly optional, generate a stable ID client-side.

---

18. No Input Validation on Itinerary Names

class ItineraryCreate(BaseModel):
    name: Optional[str] = None

No max length. Someone sends a 10MB string as a name. Your database accepts it. Your frontend tries to render it. Add max_length=255 or similar.

---

19. Commit Message Poetry

f697fb7 burnt down half the amazon rainforest for a find/replace

Hilarious. Also useless for git bisect. What did you actually change? "Find/replace what?" Future you will hate current you.

---

20. The Phantom pnpm-lock.yaml

5,355 lines added. Did you manually add pnpm to a project that was using npm? Why? The package-lock.json is still there and still being modified. This is chaos.

---

Summary

This PR does too much. It's Docker + itineraries + env vars + type fixes + dependency changes + bug fixes all in one. Each of these should be a separate PR that can be reviewed, tested, and reverted independently.

The itinerary feature works but has no validation, no limits, inconsistent error handling, and will definitely cause production issues at scale.

Recommendation: Split this into 4-5 smaller PRs. Fix the ESLint issues instead of hiding them. Add input validation. Pick one package manager. And for the love of all that is holy, delete that database export.

[prapooskur]

@dakshshah03 lgtm