Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/app.test.ts (1)

158-177: Follow the Map-over-Record guideline.

Coding guidelines ask us to use ES6 Map instead of Record in TS files. Please convert the attribute accumulator to a Map and update the callers to read via .get().

Apply this diff:

-  const parseCookieAttributes = (name: string) => {
+  const parseCookieAttributes = (name: string): Map<string, string> | null => {
     const raw = [...cookieWrites].reverse().find((entry) => entry.trim().toLowerCase().startsWith(`${name.toLowerCase()}=`));
     if (!raw) {
       return null;
     }
     const [, ...attributeParts] = raw.split(";").map((part) => part.trim()).filter(Boolean);
-    const attrs: Record<string, string> = {};
+    const attrs = new Map<string, string>();
     for (const attribute of attributeParts) {
       const [attrName, ...attrValueParts] = attribute.split("=");
-      attrs[attrName.toLowerCase()] = attrValueParts.join("=") || "";
+      attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
     }
     return attrs;
   };

   const refreshAttrs = parseCookieAttributes(`stack-refresh-${clientApp.projectId}`);
-  expect(refreshAttrs?.domain).toBe("example.com");
+  expect(refreshAttrs?.get("domain")).toBe("example.com");

   const accessAttrs = parseCookieAttributes("stack-access");
-  expect(accessAttrs?.domain).toBe("example.com");
+  expect(accessAttrs?.get("domain")).toBe("example.com");

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/app.test.ts (1)

147-166: Restore stubbed globals with try/finally to keep tests isolated.

Globals are stubbed but never restored; wrap body and call vi.unstubAllGlobals() in finally.

-  vi.stubGlobal("window", fakeWindow);
-  vi.stubGlobal("document", fakeDocument);
-  vi.stubGlobal("sessionStorage", fakeSessionStorage);
-
-  const { clientApp } = await createApp(
+  vi.stubGlobal("window", fakeWindow);
+  vi.stubGlobal("document", fakeDocument);
+  vi.stubGlobal("sessionStorage", fakeSessionStorage);
+  try {
+    const { clientApp } = await createApp(
       {
         config: {
           domains: [
             { domain: "https://example.com", handlerPath: "/handler" },
             { domain: "https://*.example.com", handlerPath: "/handler" },
           ],
         }
       },
       {
         client: {
           tokenStore: "cookie",
           noAutomaticPrefetch: true,
         },
       }
-  );
+    );
   ...
-  expect(cookieWrites.some((entry) => entry.toLowerCase().startsWith("stack-refresh=") && entry.toLowerCase().includes("expires="))).toBe(true);
-});
+  expect(cookieWrites.some((entry) => entry.toLowerCase().startsWith("stack-refresh=") && entry.toLowerCase().includes("expires="))).toBe(true);
+  } finally {
+    vi.unstubAllGlobals();
+  }
+});

Also applies to: 168-242

packages/template/src/lib/cookie.ts (1)

204-210: Avoid duplicate cookie deletes; only delete with domain OR without.

Still calls Cookies.remove(name) unconditionally after domain-specific remove. Do one or the other to prevent redundant ops and accidental host-only deletion.

 export function deleteCookieClient(name: string, options: DeleteCookieOptions = {}) {
   ensureClient();
-  if (options.domain !== undefined) {
-    Cookies.remove(name, { domain: options.domain });
-  }
-  Cookies.remove(name);
+  if (options.domain !== undefined) {
+    Cookies.remove(name, { domain: options.domain });
+  } else {
+    Cookies.remove(name);
+  }
 }

packages/template/src/lib/cookie.ts (2)

217-224: Set path=/ explicitly to satisfy __Host- requirements and be explicit.*

Relying on library defaults is brittle; __Host-* cookies must have Path=/ and no Domain. Add path: "/" on all writes.

 export function setCookieClient(name: string, value: string, options: SetCookieOptions = {}) {
   ensureClient();
   Cookies.set(name, value, {
     expires: options.maxAge === undefined ? undefined : new Date(Date.now() + (options.maxAge) * 1000),
     domain: options.domain,
     secure: options.secure,
+    path: "/",
   });
 }

---

173-188: isSecure(): LGTM with a small nit.

Looks good. Optionally check x-forwarded-proto before reading cookies to skip an extra lookup.

apps/e2e/tests/js/app.test.ts (2)

217-229: Prefer Map over Record for parsed cookie attributes.

Aligns with repo guideline to use ES6 Maps in TS. Also avoids accidental proto collisions.

-  const parseCookieAttributes = (name: string) => {
+  const parseCookieAttributes = (name: string): Map<string, string> | null => {
     const raw = [...cookieWrites].reverse().find((entry) => entry.trim().toLowerCase().startsWith(`${name.toLowerCase()}=`));
     if (!raw) {
       return null;
     }
     const [, ...attributeParts] = raw.split(";").map((part) => part.trim()).filter(Boolean);
-    const attrs: Record<string, string> = {};
-    for (const attribute of attributeParts) {
-      const [attrName, ...attrValueParts] = attribute.split("=");
-      attrs[attrName.toLowerCase()] = attrValueParts.join("=") || "";
-    }
-    return attrs;
+    const attrs = new Map<string, string>();
+    for (const attribute of attributeParts) {
+      const [attrName, ...attrValueParts] = attribute.split("=");
+      attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
+    }
+    return attrs;
   };

Update callsites accordingly, e.g.:

-const defaultAttrs = parseCookieAttributes(defaultCookieName);
-expect(defaultAttrs?.domain).toBeUndefined();
-expect(defaultAttrs).not.toBeNull();
-expect(Object.prototype.hasOwnProperty.call(defaultAttrs!, "secure")).toBe(true);
+const defaultAttrs = parseCookieAttributes(defaultCookieName);
+expect(defaultAttrs).not.toBeNull();
+expect(defaultAttrs!.has("domain")).toBe(false);
+expect(defaultAttrs!.has("secure")).toBe(true);

-const customAttrs = parseCookieAttributes(customCookieName);
-expect(customAttrs?.domain).toBe("example.com");
+const customAttrs = parseCookieAttributes(customCookieName);
+expect(customAttrs!.get("domain")).toBe("example.com");

As per coding guidelines.

---

231-238: Strengthen assertions: enforce Path=/ and Secure on both cookies.

Assert __Host-* invariants and that the domain cookie is also Secure.

-  expect(defaultAttrs?.domain).toBeUndefined();
-  expect(defaultAttrs).not.toBeNull();
-  expect(Object.prototype.hasOwnProperty.call(defaultAttrs!, "secure")).toBe(true);
+  expect(defaultAttrs).not.toBeNull();
+  expect(defaultAttrs!.has("domain")).toBe(false);
+  expect(defaultAttrs!.get("path")).toBe("/");
+  expect(defaultAttrs!.has("secure")).toBe(true);

-  const customAttrs = parseCookieAttributes(customCookieName);
-  expect(customAttrs?.domain).toBe("example.com");
+  const customAttrs = parseCookieAttributes(customCookieName);
+  expect(customAttrs!.get("domain")).toBe("example.com");
+  expect(customAttrs!.has("secure")).toBe(true);
+  expect(customAttrs!.get("path")).toBe("/");

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (2)

619-647: Choose the trusted parent domain that matches the current base domain.

_current implementation may pick the wrong candidate when multiple parent domains exist (e.g., example.com vs example.co.uk). Prefer the one matching the current base domain; fall back to the first.

-  private async _fetchTrustedParentDomain(): Promise<string | null> {
+  private async _fetchTrustedParentDomain(): Promise<string | null> {
     const project = Result.orThrow(await this._interface.getClientProject());
     const domains = project.config.domains;
@@
-    const selected = candidates[0] ?? null;
-    return selected;
+    // Prefer the candidate that matches the current base domain (browser or server)
+    let currentBase: string | undefined;
+    if (isBrowserLike()) {
+      currentBase = this._getBrowserBaseDomain();
+    } else {
+      currentBase = await this._getServerBaseDomain();
+    }
+    const selected =
+      (currentBase && candidates.find((c) => c === currentBase)) ??
+      candidates[0] ??
+      null;
+    return selected;

---

539-550: Minor: remove unreachable return.

After the try/catch both branches return; the final return is unreachable in Next builds. Safe to drop.

   private async _getServerBaseDomain(): Promise<string | undefined> {
     // IF_PLATFORM next
     try {
       const resolvedHeaders = typeof nextHeaders === "function" ? await nextHeaders() : nextHeaders;
       const hostHeader = resolvedHeaders?.get("x-forwarded-host") ?? resolvedHeaders?.get("host");
       return hostHeader ? extractBaseDomainFromHost(hostHeader) : undefined;
     } catch {
       return undefined;
     }
     // END_PLATFORM
-    return undefined;
   }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/cookies.test.ts (10)

1-2: Avoid dist import; drop TextEncoder import

• Importing from a package’s dist path is brittle. Prefer the public entrypoint.
• TextEncoder is global in Node ≥18; the explicit import is unnecessary.

Apply one of these (verify export availability):

-import { encodeBase32 } from "@stackframe/stack-shared/dist/utils/bytes";
-import { TextEncoder } from "util";
+import { encodeBase32 } from "@stackframe/stack-shared";
// If not re‑exported:
// import { encodeBase32 } from "@stackframe/stack-shared/utils/bytes";

If you keep encodeBase32 usage, rely on the global TextEncoder (no import needed).

---

56-74: Make cookie deletion semantics closer to browsers

Right now, cookies are deleted only when the value is empty. Real deletions also happen via Max-Age=0 or a past Expires. Aligning the shim avoids stale entries lingering in cookieStore.

Apply:

   set: (value: string) => {
     cookieWrites.push(value);
     const [pair] = value.split(";").map((part) => part.trim()).filter(Boolean);
     if (!pair) {
       return;
     }
     const [rawName, ...rawValueParts] = pair.split("=");
     const name = rawName.trim();
     const storedValue = rawValueParts.join("=");
-    if (storedValue === "") {
-      cookieStore.delete(name);
-    } else {
-      cookieStore.set(name, storedValue);
-    }
+    // Parse attributes for deletion semantics
+    const attrs = new Map<string, string>();
+    for (const part of value.split(";").slice(1)) {
+      const [k, ...v] = part.trim().split("=");
+      attrs.set(k.toLowerCase(), v.join("=") || "");
+    }
+    const maxAge = attrs.get("max-age");
+    const expires = attrs.get("expires");
+    const isExpired =
+      maxAge === "0" ||
+      (expires ? !Number.isNaN(Date.parse(expires)) && Date.parse(expires) <= Date.now() : false);
+    if (storedValue === "" || isExpired) cookieStore.delete(name);
+    else cookieStore.set(name, storedValue);
   },

---

76-79: Restore stubbed globals after each test

Globals are stubbed but never un-stubbed, risking cross-test leakage. Add a teardown.

-import { vi } from "vitest";
+import { vi, afterEach } from "vitest";
...
 vi.stubGlobal("window", fakeWindow);
 vi.stubGlobal("document", fakeDocument);
 vi.stubGlobal("sessionStorage", fakeSessionStorage);
 
 return {
   cookieStore,
   cookieWrites,
   location,
 };
}
+
+// Ensure no pollution across tests
+afterEach(() => {
+  // Vitest >=1.2
+  (vi as any).unstubAllGlobals?.();
+});

If your Vitest version lacks unstubAllGlobals, store previous values in setupBrowserCookieEnv and return a cleanup() you call in tests.

---

98-110: Normalize Domain attribute (handle leading dot)

Some setters write Domain=.example.com. Normalizing avoids brittle assertions.

-  for (const attribute of attributeParts) {
+  for (const attribute of attributeParts) {
     const [attrName, ...attrValueParts] = attribute.split("=");
-    attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
+    const key = attrName.toLowerCase();
+    let val = attrValueParts.join("=") || "";
+    if (key === "domain") val = val.replace(/^\./, "");
+    attrs.set(key, val);
   }

---

163-168: Trim long waits to keep e2e fast

10s waits will slow CI. Unless truly necessary, reduce to 2–3s and tighten polling.

-const customReady = await waitUntil(() => cookieStore.has(customCookieName), 10_000);
+const customReady = await waitUntil(() => cookieStore.has(customCookieName), 3_000);
...
-const valuesEqual = await waitUntil(() => cookieStore.get(customCookieName) === cookieStore.get(defaultCookieName), 10_000);
+const valuesEqual = await waitUntil(() => cookieStore.get(customCookieName) === cookieStore.get(defaultCookieName), 3_000);

Optionally make intervalMs smaller for these checks (e.g., 50ms).

Also applies to: 172-174

---

181-185: Assert full __Host- invariants

For __Host- cookies, browsers require Secure, no Domain, and Path=/. You’re asserting the first two; add Path=/ to catch regressions.

 expect(defaultAttrs).not.toBeNull();
 expect(defaultAttrs?.has("secure")).toBe(true);
 expect(defaultAttrs?.get("domain")).toBeUndefined();
+expect(defaultAttrs?.get("path")).toBe("/");

Confirm the writer sets Path=/ for __Host- cookies.

---

186-188: Relax exact match on Domain to tolerate leading dot

Avoid false negatives if the implementation writes .example.com.

- expect(customAttrs?.get("domain")).toBe("example.com");
+ expect(customAttrs?.get("domain")).toBe("example.com"); // after helper normalization
+ // If you don't normalize in the helper, use:
+ // expect(customAttrs?.get("domain")?.replace(/^\./, "")).toBe("example.com");

---

243-296: Consider snapshotting cookie attribute maps

Where attributes are stable (e.g., default insecure cookie), inline snapshots can surface unintended changes (e.g., SameSite flips) per test guidelines.

// Example:
expect(Object.fromEntries(insecureAttrs!)).toMatchInlineSnapshot(`
  {
    "path": "/",
    // "samesite": "lax", // include if applicable/stable
  }
`);

As per coding guidelines

---

313-320: Prefer Map over Record; convert at call site if needed

Guideline favors ES6 Map. You can still feed an object to the existing API via Object.fromEntries.

-const cookieMap: Record<string, string> = {
-  [defaultCookieName]: staleCookieValue,
-  [customCookieName]: freshCookieValue,
-  "stack-access": JSON.stringify(["fresh-token", "fresh-access-token"]),
-};
-
-const tokens = (clientApp as any)._getTokensFromCookies(cookieMap);
+const cookieMap = new Map<string, string>([
+  [defaultCookieName, staleCookieValue],
+  [customCookieName, freshCookieValue],
+  ["stack-access", JSON.stringify(["fresh-token", "fresh-access-token"])],
+]);
+const tokens = (clientApp as any)._getTokensFromCookies(Object.fromEntries(cookieMap));

If _getTokensFromCookies already accepts a Map, pass it directly.

---

298-320: Avoid coupling to private API

_getTokensFromCookies is an internal method; tests may break on refactors. Prefer a public surface or a small exported helper.

Expose a stable helper (e.g., extractTokensFromCookies(cookies: Map<string,string>|Record<string,string>)) and test that instead.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)

574-589: CRITICAL: Refresh token cookie lacks Secure flag in browser context.

As flagged in existing comments, the browser path writes domain cookies without the Secure flag, exposing refresh tokens to potential downgrade attacks. The Secure flag must be set based on the current protocol.

Per the existing discussion with N2D4, the ideal solution is to make setCookieClient automatically set the Secure flag based on window.location.protocol === "https:". This would fix this issue and prevent similar bugs in the future.

As a temporary fix, apply this diff:

 const setCookie = async (targetDomain: string, value: string) => {
   const name = this._getCustomRefreshCookieName(targetDomain);
-  await setOrDeleteCookieWrapper(name, value, { maxAge: 60 * 60 * 24 * 365, domain: targetDomain, noOpIfServerComponent: true });
+  const baseOptions = { maxAge: 60 * 60 * 24 * 365, domain: targetDomain, noOpIfServerComponent: true } as const;
+  if (context === "browser") {
+    await setOrDeleteCookieWrapper(
+      name,
+      value,
+      { ...baseOptions, secure: typeof window !== "undefined" && window.location.protocol === "https:" }
+    );
+  } else {
+    await setOrDeleteCookieWrapper(name, value, baseOptions);
+  }
 };

However, the better long-term solution is to update setCookieClient in packages/template/src/lib/cookie.ts to automatically set secure based on the protocol, as discussed in the existing comments.

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (3)

437-442: Consider renaming updated_at to updated_at_millis for consistency.

As noted in the past review comment, the field name could be more explicit about the unit. This would align with other timestamp fields in the codebase.

Based on past review comments.

Apply this diff to rename the field:

 private _formatRefreshCookieValue(refreshToken: string, updatedAt: number): string {
   return JSON.stringify({
     refresh_token: refreshToken,
-    updated_at: updatedAt,
+    updated_at_millis: updatedAt,
   });
 }

Also update the corresponding parsing logic in _parseStructuredRefreshCookie to use updated_at_millis.

---

551-573: Consider simplifying legacy cookie deletion per past review comments.

The implementation is correct, but as noted in past review comments, this could be simplified:

1. Line 552: Direct call to deleteCookieClient instead of intermediate variable
2. Lines 561-572: Use await statements instead of Promise.all for cleaner code

The domain-based deletion (lines 554-558, 565-571) is necessary because cookies set with a domain attribute are distinct from those without, so both must be cleaned up.

Based on past review comments.

Apply this diff:

 private _deleteLegacyBrowserRefreshCookies() {
-  deleteCookieClient(this._refreshTokenCookieName);
+  deleteCookieClient(this._refreshTokenCookieName);
   deleteCookieClient("stack-refresh");
   const domain = this._getBrowserBaseDomain();
   if (domain) {
     deleteCookieClient(this._refreshTokenCookieName, { domain });
     deleteCookieClient("stack-refresh", { domain });
   }
 }
 private async _deleteLegacyServerRefreshCookies() {
-  const operations: Promise<unknown>[] = [
-    setOrDeleteCookie(this._refreshTokenCookieName, null, { noOpIfServerComponent: true }),
-    setOrDeleteCookie("stack-refresh", null, { noOpIfServerComponent: true }),
-  ];
+  await setOrDeleteCookie(this._refreshTokenCookieName, null, { noOpIfServerComponent: true });
+  await setOrDeleteCookie("stack-refresh", null, { noOpIfServerComponent: true });
+  
   const domain = await this._getServerBaseDomain();
   if (domain) {
-    operations.push(
-      setOrDeleteCookie(this._refreshTokenCookieName, null, { domain, noOpIfServerComponent: true }),
-      setOrDeleteCookie("stack-refresh", null, { domain, noOpIfServerComponent: true }),
-    );
+    await setOrDeleteCookie(this._refreshTokenCookieName, null, { domain, noOpIfServerComponent: true });
+    await setOrDeleteCookie("stack-refresh", null, { domain, noOpIfServerComponent: true });
   }
-  await Promise.all(operations);
 }

---

574-618: Consider simplifying the queue update logic per past review comments.

Several simplifications were suggested in past review comments:

1. Line 577: previousDomain tracking may be unnecessary if you delete all potential cookie names instead of tracking the previous domain
2. Line 604: Use an index counter instead of timestamp for staleness checking (more resilient to same-timestamp writes)
3. Lines 590-601, 603-616: Simplify by always deleting all potential cookies instead of tracking previous domain

The current implementation is functional but could be more maintainable with these changes.

Based on past review comments.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/template/src/lib/cookie.ts (2)

209-214: Avoid duplicate cookie deletions when domain is provided.

Currently removes both domain-scoped and host-scoped cookies even when a domain is passed. Gate the second call to avoid redundant ops.

[ suggest_essential_refactor ]

 function deleteCookieClientInternal(name: string, options: DeleteCookieOptions = {}) {
   if (options.domain !== undefined) {
     Cookies.remove(name, { domain: options.domain });
-  }
-  Cookies.remove(name);
+  } else {
+    Cookies.remove(name);
+  }
 }

---

157-160: Good dedupe: get() now uses getAll() and client getAll uses js-cookie.

This removes duplication and aligns with prior suggestion to route get through getAll and set the helper cookie on access. Nice.

Also applies to: 162-167

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)

444-449: Nice: structured cookie fields and safe JSON parsing.

• updated_at_millis aligns with API naming.
• parseJson replaces broad try/catch and keeps type checks tight.

Also applies to: 453-472, 507-521

packages/template/src/lib/cookie.ts (1)

16-22: Optional: prefer Map internally for cookie collections.

Guideline prefers ES6 Map over Record. Public API can stay Record<string,string>, but you could convert at boundaries and operate on Map internally for better key handling and iteration.

Happy to provide a small helper: toCookieMap()/fromCookieMap() if you want to try this without changing the external types. As per coding guidelines.

Also applies to: 85-107, 162-167

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (2)

646-659: Also delete domain-scoped refresh cookies (browser).

You delete only host-scoped names. Domain-scoped (parent-domain) cookies linger. Clean both to avoid stale tokens.

           cookieNamesToDelete.forEach((name) => deleteCookieClient(name));
+          runAsynchronously(async () => {
+            const res = await this._trustedParentDomainCache.getOrWait([window.location.hostname], "read-write");
+            if (res.status === "ok" && res.data) {
+              await Promise.all(
+                cookieNamesToDelete.map((name) =>
+                  setOrDeleteCookieClient(name, null, { domain: res.data, noOpIfServerComponent: true })
+                ),
+              );
+            }
+          });

---

709-721: Also delete domain-scoped refresh cookies (server).

Mirror the browser cleanup on server to remove parent-domain copies too.

               if (cookieNamesToDelete.length > 0) {
                 await Promise.all(
                   cookieNamesToDelete.map((name) =>
                     setOrDeleteCookie(name, null, { noOpIfServerComponent: true }),
                   ),
                 );
+                const baseDomain = await this._getServerBaseDomain();
+                if (baseDomain) {
+                  await Promise.all(
+                    cookieNamesToDelete.map((name) =>
+                      setOrDeleteCookie(name, null, { domain: baseDomain, noOpIfServerComponent: true }),
+                    ),
+                  );
+                }
               }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/cookies.test.ts (2)

179-179: Past review comment appears resolved.

The previous review flagged that this line should check updated_at_millis instead of updated_at. The current code correctly uses updated_at_millis, so this concern has been addressed.

---

303-303: Past review comment appears resolved.

The previous review flagged that test data should use updated_at_millis instead of updated_at. The current code correctly uses updated_at_millis on lines 303 and 307, so this concern has been addressed.

apps/e2e/tests/js/cookies.test.ts (5)

59-73: Consider edge cases in cookie parsing logic.

The current cookie parsing splits on ; and = characters, which could fail if cookie values legitimately contain these characters. While this may be sufficient for controlled test scenarios, real cookies can have URL-encoded values or quoted strings with these characters.

If tests will only use simple cookie values, this is acceptable. Otherwise, consider using a more robust cookie parsing library or handling edge cases like quoted values.

---

76-78: Add cleanup to prevent test pollution.

The global stubs for window, document, and sessionStorage are not cleaned up after tests complete. This could cause test pollution if multiple tests run in the same environment or if a test fails partway through.

Consider returning a cleanup function from setupBrowserCookieEnv and calling it in test cleanup:

 function setupBrowserCookieEnv(options: BrowserEnvOptions = {}): BrowserEnv {
   // ... existing setup code ...
   
   vi.stubGlobal("window", fakeWindow);
   vi.stubGlobal("document", fakeDocument);
   vi.stubGlobal("sessionStorage", fakeSessionStorage);

   return {
     cookieStore,
     cookieWrites,
     location,
+    cleanup: () => {
+      vi.unstubAllGlobals();
+    },
   };
 }

Then in each test:

-const { cookieStore, cookieWrites } = setupBrowserCookieEnv({ protocol: "https:" });
+const env = setupBrowserCookieEnv({ protocol: "https:" });
+try {
+  // test code
+} finally {
+  env.cleanup();
+}

---

163-172: Consider extracting timeout constants.

The test uses hardcoded timeout values (2,000 ms and 10,000 ms) directly in waitUntil calls. Different CI environments may have different performance characteristics, making these timeouts potentially flaky.

Consider extracting these as configurable constants:

const COOKIE_READY_TIMEOUT = 2_000;
const COOKIE_SYNC_TIMEOUT = 10_000;

Or make them environment-aware:

const DEFAULT_TIMEOUT = process.env.CI ? 20_000 : 2_000;

---

301-308: Add explanatory constants for timestamp values.

The timestamp values 1700000000000 and 1800000000000 are magic numbers without context. Using named constants would make the test's intent clearer.

Apply this diff:

+  const STALE_TIMESTAMP_MILLIS = 1700000000000;  // Nov 15, 2023
+  const FRESH_TIMESTAMP_MILLIS = 1800000000000;  // Jan 21, 2027
+
   const staleCookieValue = JSON.stringify({
     refresh_token: "stale-token",
-    updated_at_millis: 1700000000000,
+    updated_at_millis: STALE_TIMESTAMP_MILLIS,
   });
   const freshCookieValue = JSON.stringify({
     refresh_token: "fresh-token",
-    updated_at_millis: 1800000000000,
+    updated_at_millis: FRESH_TIMESTAMP_MILLIS,
   });

---

316-316: Avoid accessing private methods in tests.

Casting to any to access _getTokensFromCookies breaks encapsulation and makes the test fragile to implementation changes. If this method needs to be tested, consider making it part of the public API or creating a test-specific export.

Consider one of these approaches:

1. Export the method for testing (in the source file):

   export { _getTokensFromCookies } from './internal';

2. Test through the public API if possible:

   // Instead of directly calling _getTokensFromCookies,
   // set up cookies and verify tokens are retrieved via public methods

3. Create a test utility that doesn't rely on private implementation details.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)

582-614: Fix host resolution and add Secure for browser-set domain cookie.

• Hostname is overwritten by the Next headers path and can become undefined in browsers; also port is not stripped.
• Browser path sets domain cookie without Secure.

Apply:

@@
-      let hostname;
-      if (isBrowserLike()) {
-        hostname = window.location.hostname;
-      }
-      // IF_PLATFORM next
-      hostname = (await sc.headers?.())?.get("host");
-      // END_PLATFORM
-      if (!hostname) {
+      let hostname: string | null = null;
+      if (context === "browser") {
+        hostname = typeof window !== "undefined" ? window.location.hostname : null;
+      } else {
+        // IF_PLATFORM next
+        const hdrs = await sc.headers?.();
+        hostname = (hdrs?.get("x-forwarded-host") ?? hdrs?.get("host")) ?? null;
+        // END_PLATFORM
+      }
+      // strip port if present
+      hostname = hostname?.split(":")[0] ?? null;
+      if (!hostname) {
         return;
       }
@@
-      const setCookie = async (targetDomain: string, value: string | null) => {
+      const setCookie = async (targetDomain: string, value: string | null) => {
         const name = this._getCustomRefreshCookieName(targetDomain);
-        const options = { maxAge: 60 * 60 * 24 * 365, domain: targetDomain, noOpIfServerComponent: true };
-        if (context === "browser") {
-          setOrDeleteCookieClient(name, value, options);
-        } else {
-          await setOrDeleteCookie(name, value, options);
-        }
+        const baseOptions = { maxAge: 60 * 60 * 24 * 365, domain: targetDomain, noOpIfServerComponent: true } as const;
+        if (context === "browser") {
+          setOrDeleteCookieClient(
+            name,
+            value,
+            { ...baseOptions, secure: typeof window !== "undefined" && window.location.protocol === "https:" },
+          );
+        } else {
+          await setOrDeleteCookie(name, value, baseOptions);
+        }
       };

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)

473-502: Consider ES6 Map for cookie collections (optional).

New helpers pass around Record<string,string>. Per guidelines, prefer Map for lookups/iteration clarity and to avoid prototype pitfalls. You can convert once at boundaries (e.g., from cookie.parse) and refactor _collectRefreshTokenCookieNames/_extractRefreshTokenFromCookieMap accordingly. Low priority.

Also applies to: 534-563

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro