Make spotlight session caching consistent with server

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: e4e6200

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Pull Request Review

Summary

This PR addresses a critical bug in session management by scoping session IDs to individual forms. Previously, all forms shared a single form_session_id key in both localStorage (client-side) and cookies (server-side), causing session collisions when users interact with multiple forms.

Code Quality ✅

Strengths:

• Clean, focused change with minimal diff (3 additions, 3 deletions across 3 files)
• Consistent implementation across both client-side (spotlight app) and server-side (server package)
• Test coverage updated appropriately in [id].test.ts:238

Style & Conventions:

• Follows the monorepo structure and testing patterns outlined in CLAUDE.md
• Uses template literals appropriately for the dynamic key construction

Correctness ✅

The changes correctly address the session collision issue:

Client-side (spotlight):

• initialize.ts:12: Retrieves form-specific session from localStorage
• on-submit-form.ts:40: Stores form-specific session to localStorage

Server-side:

• [id].astro:18,22,24: Cookie operations now scoped to formId
• [id].test.ts:238: Test properly simulates form-specific cookie header

All occurrences of form_session_id have been updated - grep confirms no remaining unscoped references.

Potential Issues ⚠️

1. Migration/Backward Compatibility
Users with existing sessions stored under the old form_session_id key will lose their session state when this deploys. Consider:

• Adding migration logic to copy form_session_id → form_session_id_${formId} on first load
• Documenting this as a breaking change if session continuity matters

Example migration approach:

// In initialize.ts
const legacySessionId = window.localStorage.getItem('form_session_id');
const sessionId = window.localStorage.getItem(`form_session_id_${opts.formId}`) || legacySessionId;
if (legacySessionId && !window.localStorage.getItem(`form_session_id_${opts.formId}`)) {
  window.localStorage.setItem(`form_session_id_${opts.formId}`, legacySessionId);
  window.localStorage.removeItem('form_session_id');
}

2. Cookie Storage Growth
Each form now creates a separate cookie. If users interact with many forms, cookie storage could grow. This is likely acceptable but worth monitoring.

Security Considerations 🔒

No new security issues introduced:

• Session IDs remain opaque and server-generated
• No XSS risk from template literals (formId comes from trusted route params)
• Cookie handling remains consistent with existing patterns

Minor consideration:

• Ensure formId is validated/sanitized before use in cookie names (appears to be handled at the route level via getAstroRouteParams)

Performance Considerations ⚡

Positive:

• No performance degradation - same number of operations, just with dynamic keys
• Actually prevents unnecessary session resets, which could improve perceived performance

Neutral:

• Minimal impact on localStorage/cookie size (additional characters per form)

Test Coverage ✅

Updated:

• [id].test.ts:238 correctly updated to use form-specific cookie header

Note from existing code:
The test file has a comment (lines 67-69) about Astro container renderer limitations preventing cookie validation. This PR's test update is correct, but full cookie behavior can't be validated in tests due to this limitation.

Recommendation:
Consider adding a comment in the test explaining why the cookie format changed, to help future maintainers understand the scoping decision.

Recommendations

1. High Priority: Add migration logic for existing sessions (see example above)
2. Medium Priority: Add a test case that verifies different forms maintain separate sessions
3. Low Priority: Document cookie naming convention in CLAUDE.md or technical docs

Verdict

Approve with suggestions ✅

This is a well-executed bug fix that solves a real problem. The implementation is clean and consistent. The main concern is the breaking change for existing sessions, which should be addressed with migration logic or at minimum documented as a known impact.

---

🤖 Review generated by Claude Code