Skip to content

Conversation

gurjeetsinghvirdee
Copy link
Member

@gurjeetsinghvirdee gurjeetsinghvirdee commented Sep 10, 2025

What does this PR do?

It improves the user experience when authentication is required to access projects
Instead of displaying a technical error message such as:

User (role: guests) missing scopes (["projects.read"])

The CLI will now show a clear, actionable message:

Cannot access projects. Login is required use 'appwrite login' command

Before

image

After

image

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

Summary by CodeRabbit

  • Bug Fixes
    • Improved unauthorized error handling: when a guest user attempts to access projects without the required scope, the CLI now shows a clear, actionable message indicating that login is required and suggests running ‘appwrite login’.
    • Prevents confusing generic errors by providing a specific reason for denied access to projects.
    • Other 401 and error scenarios remain unchanged, ensuring consistent behavior elsewhere.

Copy link

coderabbitai bot commented Sep 10, 2025

Walkthrough

The change updates lib/client.js by adding a new error-handling branch in Client.call for 401 responses where json.message is a string containing 'User (role: guests) missing scopes (["projects.read"])'. In this case, it throws an AppwriteException with the message: Cannot access projects. Login is required use 'appwrite login' command, passing along json.code, json.type, and the raw response text. This check runs before the existing 401 handling for 'user_more_factors_required'. Other error handling remains unchanged.

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the primary change by stating that the error message is improved for scenarios where authentication is required to access projects, making it concise, specific, and directly related to the changeset without extraneous details or vague wording.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
lib/client.js (2)

202-215: Make detection robust and tighten scope; polish the user-facing copy.

  • Exact-phrase matching on json.message is brittle (locale/wording drift) and may silently regress. Also, scoping this only to /projects avoids false positives on unrelated endpoints that might reference projects.read.
  • Copy nit: “Login is required use …” reads awkwardly.

Apply:

-      if (
-        json.code === 401 &&
-        typeof json.message === "string" &&
-        json.message.includes(
-          'User (role: guests) missing scopes (["projects.read"])'
-        )
-      ) {
-        throw new AppwriteException(
-          "Cannot access projects. Login is required use 'appwrite login' command",
-          json.code,
-          json.type,
-          text
-        );
-      }
+      const isGuestProjectsScope =
+        json.code === 401 &&
+        path.startsWith('/projects') &&
+        typeof json.message === 'string' &&
+        (
+          json.type === 'general_unauthorized_scope' ||
+          /role:\s*guests.*missing scopes.*projects\.read/i.test(json.message)
+        );
+      if (isGuestProjectsScope) {
+        throw new AppwriteException(
+          "Cannot access projects. Login required. Run 'appwrite login'.",
+          json.code,
+          json.type,
+          text
+        );
+      }

202-215: Add a quick test/verification for this mapping.

  • Ensure we don’t regress the UX and that the branch is hit as intended.

Manual steps:

  • Run: appwrite logout (or clear session), then appwrite projects list.
  • Expect: “Cannot access projects. Login required. Run 'appwrite login'.”

If you’d like, I can open a follow-up adding a small test that stubs undici.fetch to return a 401 with the error payload and asserts the thrown AppwriteException message.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 71e8c88 and 1848aea.

📒 Files selected for processing (1)
  • lib/client.js (1 hunks)

@gurjeetsinghvirdee
Copy link
Member Author

Hey, @ChiragAgg5k
Can you please review this PR?

@ChiragAgg5k
Copy link
Member

@gurjeetsinghvirdee we need to add this change to sdk-generator first here - https://github.com/appwrite/sdk-generator/blob/6fda9e58b37c9872c1a2a424e5467de8de1bc567/templates/cli/lib/client.js.twig#L150

please read the contribution guidelines in the repository

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.

3 participants