server/src/api/sessionReplay/recordSessionReplay.ts (1)

39-45: Do not coerce route param to Number; resolve numeric siteId from either id or siteId.

String IDs will become NaN, breaking limits, logging, and ingestion.

-    const siteId = Number(request.params.site);
+    const rawSite = request.params.site; // can be '123' or '3c8487f0427d'
+    // Resolve to canonical numeric siteId (implement resolveSiteId to accept string id or numeric)
+    const siteId = await siteConfig.resolveSiteId(rawSite);
+    if (!Number.isInteger(siteId)) {
+      logger.warn(`[SessionReplay] Unknown site identifier: ${rawSite}`);
+      return reply.status(404).send({ success: false, error: "Unknown site" });
+    }

Follow-ups:

• Update usageService APIs to take numeric siteId, after resolution.
• Apply the same resolution in other handlers using Number(request.params.site).

server/public/script-full.js (1)

68-71: Bug: analyticsHost derivation breaks when loading script-full.js

Splitting by “/script.js” fails if this bundle is served as “/script-full.js”, producing wrong API URLs. Use URL.origin.

-    const analyticsHost = src.split("/script.js")[0];
+    const analyticsHost = new URL(src, window.location.href).origin;

server/src/db/postgres/schema.ts (2)

71-83: TDZ + duplicate FKs on sites: remove extra foreignKey() entries

These explicit foreignKey() calls reference organization before it’s defined (risking a TDZ error) and duplicate the column-level .references() you already set, creating duplicate constraints.

   table => [
-    foreignKey({
-      columns: [table.createdBy],
-      foreignColumns: [user.id],
-      name: "sites_created_by_user_id_fk",
-    }),
-    foreignKey({
-      columns: [table.organizationId],
-      foreignColumns: [organization.id],
-      name: "sites_organization_id_organization_id_fk",
-    }),
   ]

---

54-70: Add migration: enable pgcrypto and backfill existing sites.id before enforcing NOT NULL/UNIQUE

rg returned only the schema (server/src/db/postgres/schema.ts:54–56) — no migration found that creates pgcrypto or backfills ids. Add a migration that, in this order:

• CREATE EXTENSION IF NOT EXISTS pgcrypto;
• UPDATE sites SET id = encode(gen_random_bytes(6), 'hex') WHERE id IS NULL;
• ALTER TABLE sites ALTER COLUMN id SET NOT NULL;
• CREATE UNIQUE INDEX CONCURRENTLY sites_id_unique ON sites(id);

Location: server/src/db/postgres/schema.ts (id uses gen_random_bytes).

server/src/api/uptime/incidents.ts (2)

157-217: Wrap acknowledge handler in try/catch per guidelines

Uncaught DB errors will bubble to Fastify’s global handler; our guidelines require explicit try/catch with typed errors.

-  server.route({
+  server.route({
     method: "PATCH",
     url: "/api/uptime/incidents/:id/acknowledge",
     handler: async (request: FastifyRequest, reply: FastifyReply) => {
+      try {
         const session = await getSessionFromReq(request);
         const userId = session?.user?.id;
         ...
         return reply.send({
           success: true,
           incident: updated,
         });
+      } catch (error) {
+        request.log.error({ err: error }, "acknowledge incident failed");
+        return reply.status(500).send({ error: "Internal server error" });
+      }
     },
   });

---

219-281: Same for resolve handler: add try/catch

Mirror the pattern from GET and the acknowledge fix.

-    handler: async (request: FastifyRequest, reply: FastifyReply) => {
+    handler: async (request: FastifyRequest, reply: FastifyReply) => {
+      try {
         const session = await getSessionFromReq(request);
         ...
         return reply.send({
           success: true,
           incident: updated,
         });
+      } catch (error) {
+        request.log.error({ err: error }, "resolve incident failed");
+        return reply.status(500).send({ error: "Internal server error" });
+      }
     },

server/src/api/uptime/notifications.ts (1)

23-39: Tighten validation for triggerEvents to allowed values and remove duplication.

Currently triggerEvents accepts arbitrary strings; confine to "down" | "recovery" and centralize defaults in Zod.

Apply:

+const triggerEventEnum = z.enum(["down", "recovery"]);
+
 const createChannelSchema = z.object({
   type: channelTypeSchema,
   name: z.string().min(1).max(100),
   config: channelConfigSchema,
   monitorIds: z.array(z.number()).nullable().optional(),
-  triggerEvents: z.array(z.string()).default(["down", "recovery"]).optional(),
+  triggerEvents: z.array(triggerEventEnum).default(["down", "recovery"]).optional(),
   cooldownMinutes: z.number().int().min(0).max(1440).default(5).optional(),
 });

 const updateChannelSchema = z.object({
   name: z.string().min(1).max(100).optional(),
   enabled: z.boolean().optional(),
   config: channelConfigSchema.optional(),
   monitorIds: z.array(z.number()).nullable().optional(),
-  triggerEvents: z.array(z.string()).optional(),
+  triggerEvents: z.array(triggerEventEnum).optional(),
   cooldownMinutes: z.number().int().min(0).max(1440).optional(),
 });

server/src/api/uptime/createMonitor.ts (1)

47-76: Create monitor + status atomically; schedule after commit.

A failure between inserts can leave an orphaned monitor or status. Wrap DB writes in a transaction, and only schedule after commit.

-    // Create the monitor
-    const [newMonitor] = await db
-      .insert(uptimeMonitors)
-      .values({
-        organizationId,
-        name,
-        monitorType,
-        intervalSeconds,
-        enabled,
-        httpConfig: monitorType === "http" ? httpConfig : null,
-        tcpConfig: monitorType === "tcp" ? tcpConfig : null,
-        validationRules,
-        monitoringType: monitoringType || "local",
-        selectedRegions: selectedRegions || ["local"],
-        createdBy: userId,
-      })
-      .returning();
-
-    // Initialize monitor status
-    await db.insert(uptimeMonitorStatus).values({
-      monitorId: newMonitor.id,
-      currentStatus: "unknown",
-      consecutiveFailures: 0,
-      consecutiveSuccesses: 0,
-    });
-
-    // Schedule the monitor if enabled
-    if (enabled) {
-      await uptimeService.onMonitorCreated(newMonitor.id, intervalSeconds);
-    }
+    // Create monitor and initial status atomically
+    const newMonitor = await db.transaction(async tx => {
+      const [created] = await tx
+        .insert(uptimeMonitors)
+        .values({
+          organizationId,
+          name,
+          monitorType,
+          intervalSeconds,
+          enabled,
+          httpConfig: monitorType === "http" ? httpConfig : null,
+          tcpConfig: monitorType === "tcp" ? tcpConfig : null,
+          validationRules,
+          monitoringType: monitoringType || "local",
+          selectedRegions: selectedRegions || ["local"],
+          createdBy: userId,
+        })
+        .returning();
+
+      await tx.insert(uptimeMonitorStatus).values({
+        monitorId: created.id,
+        currentStatus: "unknown",
+        consecutiveFailures: 0,
+        consecutiveSuccesses: 0,
+      });
+      return created;
+    });
+
+    // Schedule the monitor after successful commit
+    if (enabled) {
+      await uptimeService.onMonitorCreated(newMonitor.id, intervalSeconds);
+    }

server/src/api/uptime/getMonitorUptimeBuckets.ts (1)

126-145: Fix hourly bucket generation (repeated setDate causes drift).

setDate inside the hour loop subtracts days repeatedly. Compute day once, then iterate hours.

-    for (let i = 0; i < days; i++) {
-      const date = new Date(nowInTz);
-
-      if (bucket === "hour") {
-        // For hourly buckets, generate 24 buckets per day
-        for (let h = 0; h < 24; h++) {
-          date.setDate(date.getDate() - i);
-          date.setHours(23 - h, 0, 0, 0);
-          if (date <= nowInTz) {
-            allBuckets.push({
-              bucket_time: date.toISOString(),
-              bucket_formatted: date.toISOString().slice(0, 13) + ":00:00",
-              total_checks: 0,
-              successful_checks: 0,
-              failed_checks: 0,
-              timeout_checks: 0,
-              uptime_percentage: 100.0,
-            });
-          }
-        }
+    for (let i = 0; i < days; i++) {
+      if (bucket === "hour") {
+        // For hourly buckets, generate 24 buckets per day
+        const dayStart = new Date(nowInTz);
+        dayStart.setDate(dayStart.getDate() - i);
+        dayStart.setHours(0, 0, 0, 0);
+        for (let h = 0; h < 24; h++) {
+          const hour = new Date(dayStart);
+          hour.setHours(h, 0, 0, 0);
+          if (hour <= nowInTz) {
+            allBuckets.push({
+              bucket_time: hour.toISOString(),
+              bucket_formatted: hour.toISOString().slice(0, 13) + ":00:00",
+              total_checks: 0,
+              successful_checks: 0,
+              failed_checks: 0,
+              timeout_checks: 0,
+              uptime_percentage: 100.0,
+            });
+          }
+        }

server/src/api/sites/getSite.ts (2)

19-22: Lookup must support new random site id (string), not just numeric siteId.

Route param is id: string, but you only query by sites.siteId = Number(id). This breaks when clients pass the new string id.

Apply one of these patterns:

-  const site = await db.query.sites.findFirst({
-    where: eq(sites.siteId, Number(id)),
-  });
+  // Support both: numeric siteId and string id
+  const isNumericId = /^\d+$/.test(id);
+  const site = await db.query.sites.findFirst({
+    where: isNumericId ? eq(sites.siteId, Number(id)) : eq(sites.id, id),
+  });

Optionally normalize the route param name to identifier to avoid confusion.

---

36-49: Prefer the server-provided site.id; update client call sites and API helpers to use id (fallback to siteId)

Search shows many client files still use siteId. Update callers to prefer site.id (use site.id ?? site.siteId) and adapt API helpers to accept string|number where needed.

• client/src/components/SiteSettings/SiteSettings.tsx — ApiKeyManager: change siteId={siteMetadata.siteId} → siteId={siteMetadata.id ?? String(siteMetadata.siteId)} (ScriptBuilder already uses fallback).
• client/src/components/SiteSettings/SiteConfiguration.tsx — changeSiteDomain / deleteSite / changeSitePublic / changeSiteSalt / changeSiteBlockBots / IPExclusionManager all called with siteMetadata.siteId → use siteMetadata.id ?? siteMetadata.siteId.
• client/src/app/signup/page.tsx and client/src/app/components/AddSite.tsx — router.push and setSite currently use response.siteId / site.siteId → use response.id ?? response.siteId and setSite(site.id ?? String(site.siteId)). Update addSite response typing if backend returns id.
• client/src/app/[site]/components/Sidebar/SiteSelector.tsx and Sidebar.tsx — setSite(site.siteId.toString()) and pathname segment handling → prefer site.id (fallback to siteId) when setting route and store.
• client/src/app/[site]/components/Header/NoData.tsx — ensure script snippet uses id when present (already uses id ?? siteId; keep consistent).
• client/src/api/admin/sites.ts — update function signatures that take siteId:number (deleteSite, changeSiteDomain, changeSitePublic, changeSiteSalt, changeSiteBlockBots, useSiteHasData, etc.) to accept siteId: string | number and handle conversion; update useCurrentSite to match pathname against site.id first, then site.siteId.
• Run a global audit for ".siteId" usages and replace call sites with the id-first pattern or adjust types/APIs as appropriate.

server/src/services/tracker/trackEvent.ts (3)

276-305: site_id numeric assumption breaks with new string ids; resolve numeric id before quotas/session.

Number(validatedPayload.site_id) becomes NaN for random string ids, breaking usage limits and potentially session updates.

Apply:

-// Check if the site has exceeded its monthly limit
-if (usageService.isSiteOverLimit(Number(validatedPayload.site_id))) {
+// Resolve numeric siteId for quota/accounting (supports string ids)
+const siteIdentifier = validatedPayload.site_id;
+const isNumericId = /^\d+$/.test(siteIdentifier);
+const resolvedSiteConfig = isNumericId ? null : await siteConfig.getSiteConfig(siteIdentifier);
+const numericSiteId = isNumericId ? Number(siteIdentifier) : resolvedSiteConfig?.siteId;
+if (numericSiteId == null || Number.isNaN(numericSiteId)) {
+  logger.warn({ siteId: siteIdentifier }, "Unknown site identifier");
+  return reply.status(404).send({ success: false, error: "Unknown site" });
+}
+
+// Check if the site has exceeded its monthly limit
+if (usageService.isSiteOverLimit(numericSiteId)) {
   logger.info({ siteId: validatedPayload.site_id }, "Skipping event - site over monthly limit");
   return reply.status(200).send("Site over monthly limit, event not tracked");
 }
@@
-// Update session
-const { sessionId } = await sessionsService.updateSession({
-  userId: payload.userId,
-  site_id: payload.site_id,
-});
+// Update session (service expects string; use normalized numeric)
+const { sessionId } = await sessionsService.updateSession({
+  userId: payload.userId,
+  site_id: String(numericSiteId),
+});

Follow‑ups:

• Ensure downstream (queue workers, ClickHouse writers) use numeric siteId consistently.

---

282-292: IP exclusion can be bypassed via client‑supplied ip_address.

An attacker can set ip_address to a non‑excluded IP and slip through. Use the request’s IP for exclusion, keep payload IP only for geolocation.

Apply:

-// Use custom IP if provided in payload, otherwise get from request
-const requestIP = validatedPayload.ip_address || request.ip || "";
+// Always use the request IP for exclusion checks (trusted), not client-provided
+const requestIP = request.ip || "";
@@
-if (await siteConfig.isIPExcluded(requestIP, validatedPayload.site_id)) {
+if (await siteConfig.isIPExcluded(requestIP, validatedPayload.site_id)) {
   logger.info({ siteId: validatedPayload.site_id, ip: requestIP }, "IP excluded from tracking");
   return reply.status(200).send({
     success: true,
     message: "Event not tracked - IP excluded",
   });
 }

Keep using validatedPayload.ip_address inside createBasePayload strictly for geolocation/userId derivation if you intend to allow overrides.

---

214-216: Centralize site_id numeric conversion; remove ad-hoc Number/parseInt callsites

Convert/validate site_id once at the boundary (store a typed numeric siteId) and update these callsites:

• server/src/services/tracker/trackEvent.ts:277 — Number(validatedPayload.site_id)
• server/src/api/admin/getAdminSites.ts:100,104 — Number(event.site_id) / Number(event.site_id)
• server/src/api/admin/getAdminOrganizations.ts:129 — Number(event.site_id)
• server/src/api/sites/getSitesFromOrg.ts:68 — Number(row.site_id)
• server/src/services/sessions/sessionsService.ts:56 — parseInt(payload.site_id, 10)

Reject or fail fast on non-numeric site_id instead of scattered casts.

server/src/api/sites/addSite.ts (1)

93-98: Use the shared logger, not console.

Aligns with server logging and enables log routing/levels.

+import { logger } from "../../lib/logger/logger.js";
@@
-    console.error("Error adding site:", error);
+    logger.error(error as Error, "Error adding site");

server/src/services/tracker/utils.ts (1)

1-3: UAParser usage likely incorrect.

ua-parser-js is typically used as new UAParser(ua).getResult(). The current import/call may return an instance, not IResult.

-import UAParser, { UAParser as userAgentParser } from "ua-parser-js";
+import UAParser from "ua-parser-js";
@@
-    ua: userAgentParser(userAgent),
+    ua: new UAParser(userAgent).getResult(),

Also applies to: 114-116

server/src/services/shared/requestValidation.test.ts (1)

66-89: Tests assert console logs, but code uses logger — fix mocks and expectations.

Production code logs via logger.; the tests spy on console. with mismatched messages. Mock logger and assert on logger calls instead.

@@
-vi.mock("../../lib/siteConfig.js", () => ({
+vi.mock("../../lib/siteConfig.js", () => ({
   siteConfig: {
     
     getSiteConfig: vi.fn(),
   },
 }));
+
+vi.mock("../../lib/logger/logger.js", () => ({
+  logger: {
+    info: vi.fn(),
+    error: vi.fn(),
+    warn: vi.fn(),
+  },
+}));
@@
-  it("should return success true when API key matches", async () => {
+  it("should return success true when API key matches", async () => {
@@
-    // Mock console.info to avoid output during tests
-    const consoleSpy = vi.spyOn(console, "info").mockImplementation(() => {});
-
     const result = await validateApiKey(1, "valid-api-key");
 
     expect(result).toEqual({ success: true });
-    expect(consoleSpy).toHaveBeenCalledWith("[Validation] Valid API key for site 1");
-
-    consoleSpy.mockRestore();
+    const { logger } = await import("../../lib/logger/logger.js");
+    expect(logger.info).toHaveBeenCalledWith({ siteId: 1 }, "Valid API key for site");
@@
-  it("should handle errors during validation", async () => {
+  it("should handle errors during validation", async () => {
@@
-    const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
-
     const result = await validateApiKey(1, "test-key");
 
     expect(result).toEqual({ success: false, error: "Failed to validate API key" });
-    expect(consoleSpy).toHaveBeenCalledWith("Error validating API key:", expect.any(Error));
-
-    consoleSpy.mockRestore();
+    const { logger } = await import("../../lib/logger/logger.js");
+    expect(logger.error).toHaveBeenCalledWith(expect.any(Error), "Error validating API key");
@@
-    const consoleSpy = vi.spyOn(console, "info").mockImplementation(() => {});
-
     const result = await validateOrigin(1, "https://example.com");
 
     expect(result).toEqual({ success: true });
-    expect(consoleSpy).toHaveBeenCalledWith(
-      "[Validation] Origin check disabled. Allowing request for site 1 from origin: https://example.com"
-    );
-
-    consoleSpy.mockRestore();
+    const { logger } = await import("../../lib/logger/logger.js");
+    expect(logger.info).toHaveBeenCalled(); // message may vary; assert call occurred
@@
-  it("should handle errors during validation", async () => {
+  it("should handle errors during validation", async () => {
@@
-    const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
-
     const result = await validateOrigin(1, "https://example.com");
 
     expect(result).toEqual({ success: false, error: "Internal error validating origin" });
-    expect(consoleSpy).toHaveBeenCalledWith("Error validating origin:", expect.any(Error));
-
-    consoleSpy.mockRestore();
+    const { logger } = await import("../../lib/logger/logger.js");
+    expect(logger.error).toHaveBeenCalledWith(expect.any(Error), "Error validating origin");

Also applies to: 148-159, 168-200, 392-403

server/src/lib/siteConfig.ts (1)

30-48: Ambiguity risk without UNIQUE on sites.id.

getSiteByAnyId relies on id being unique. The current schema doesn’t enforce uniqueness. Add UNIQUE + NOT NULL on sites.id to avoid returning an arbitrary row when duplicates occur.

Also applies to: 49-62