Skip to content

Conversation

@stnguyen90
Copy link
Contributor

@stnguyen90 stnguyen90 commented Oct 10, 2025

What does this PR do?

At the moment, if a cookie isn't set, only the local storage cookie fallback is used for realtime authentication. This means sessions using client.setSession() will be unauthenticated when using realtime.

This PR makes realtime use this.config.session (which is set with client.setSession()) if it exists.

Test Plan

This needs to be tested still.

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

None

Summary by CodeRabbit

  • Bug Fixes

    • Session resolution now prefers the configured session and falls back to stored cookies only when needed, improving sign-in reliability and reducing unexpected reconnect logouts.
  • New Features

    • API spec adds a Session authentication option using the X-Appwrite-Session header to authenticate requests with a user session.

At the moment, if a cookie isn't set, only the local storage cookie fallback is used for realtime authentication. This means sessions using client.setSession() will be unauthenticated when using realtime.

This PR makes realtime use this.config.session (which is set with client.setSession()) if it exists.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

Walkthrough

In the client template, session resolution in the "connected" onMessage path now prefers this.config.session and falls back to reading a_session_${project} from cookieFallback in localStorage if absent. The session binding was changed from const to let to permit reassignment. messageData extraction and the authentication message payload structure remain the same, but the payload's session value is sourced from the resolved session variable. Separately, the Swagger spec tests/resources/spec.json was updated to add a new Session security definition (X-Appwrite-Session header API key).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 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(web): use session from setSession() for realtime" accurately reflects the primary change in the changeset. The main modification in templates/web/src/client.ts.twig prioritizes this.config.session (set by client.setSession()) for realtime authentication before falling back to localStorage, which directly matches the title's stated intent. The title is concise, specific, and uses clear terminology that would allow teammates scanning history to immediately understand the change. The secondary modification to spec.json (adding a Session security definition) is a supporting change and doesn't override the focus of the primary change.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-8925-realtime-session-web-sdk

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4be7e1d and 21acb98.

📒 Files selected for processing (2)
  • templates/web/src/client.ts.twig (1 hunks)
  • tests/resources/spec.json (1 hunks)
🔇 Additional comments (2)
tests/resources/spec.json (1)

56-61: Session security definition looks good.

The new Session security definition is properly structured and aligns with the PR objective to support session-based authentication for realtime connections.

Note: Unlike JWT, Key, and Project definitions, this doesn't include an x-appwrite.demo field. This appears intentional since session tokens shouldn't have static demo values, but verify this is the desired behavior.

templates/web/src/client.ts.twig (1)

481-487: The original review comment contains incorrect assumptions and suggestions unsupported by the actual codebase.

The code at lines 481-487 is correct as-is:

  1. Error handling is already adequate: The outer try-catch block at line 476 already wraps the JSON.parse at line 483, catching any exceptions and logging them at line 517. The suggested nested try-catch would be redundant.

  2. No setSession() method exists: The review's suggestion to verify setSession() is unfounded—this method does not exist in the codebase.

  3. Session resolution is sound: The code properly prioritizes this.config.session over the localStorage fallback and correctly handles the undefined case.

No changes are required. The code adequately handles errors and properly implements the session resolution logic.

Likely an incorrect or invalid review comment.


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.

@stnguyen90 stnguyen90 merged commit 405ed73 into master Oct 23, 2025
52 checks passed
@stnguyen90
Copy link
Contributor Author

Thanks @hmacr for testing this:

image image

@stnguyen90 stnguyen90 deleted the feat-8925-realtime-session-web-sdk branch October 23, 2025 17:04
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.

5 participants