Skip to content

docs: fix README feature list and RefreshCookieName comment#254

Merged
veverkap merged 1 commit into
mainfrom
docs/fix-readme-oauth2-and-refresh-cookie-06709c5f1aa5865f
May 11, 2026
Merged

docs: fix README feature list and RefreshCookieName comment#254
veverkap merged 1 commit into
mainfrom
docs/fix-readme-oauth2-and-refresh-cookie-06709c5f1aa5865f

Conversation

@veverkap
Copy link
Copy Markdown
Contributor

@veverkap veverkap commented May 11, 2026

  • Add 'generic OAuth2 login (GitHub, Discord, Slack, and any custom provider)' to the intro feature list; the OAuth2Handler and built-in providers (GitHubProvider, GoogleOAuth2Provider) exist in the codebase but were absent from the README description (present in docs/index.md)
  • Correct the RefreshCookieName quick-start comment from 'optional' to 'required when Sessions is set'; AuthHandler.Validate() enforces this constraint and returns an error when Sessions is non-nil but RefreshCookieName is empty

Greptile Summary

This PR makes two documentation-only corrections to README.md: it adds generic OAuth2 login to the intro feature list (matching the existing OAuth2Handler and built-in providers already present in the codebase), and updates the RefreshCookieName quick-start comment from "optional" to "required when Sessions is set" (matching AuthHandler.Validate() enforcement and the auth.go struct comment).

  • The OAuth2 addition accurately reflects handler/oauth2.go and the built-in GitHubProvider/GoogleOAuth2Provider implementations.
  • The RefreshCookieName correction aligns the quick-start comment with actual runtime behaviour, but the equivalent comment in the detailed AuthHandler reference block (line 520) still reads // optional and was not updated in this PR.

Confidence Score: 4/5

Safe to merge — documentation-only change with no runtime impact.

Both changes are accurate relative to the source code. The only remaining gap is a parallel "optional" label for RefreshCookieName in the detailed AuthHandler reference block that this PR did not touch, leaving the README internally inconsistent on that point.

README.md line 520 — the detailed AuthHandler struct example still labels RefreshCookieName as optional.

Important Files Changed

Filename Overview
README.md Two doc-only changes: adds generic OAuth2 to the intro feature list (accurate, matching handler/oauth2.go), and corrects the RefreshCookieName quick-start comment from "optional" to "required when Sessions is set" (matches AuthHandler.Validate() enforcement). A parallel occurrence of the old "optional" label remains in the detailed AuthHandler reference block at line 520.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[AuthHandler configured] --> B{Sessions set?}
    B -- No --> C[RefreshCookieName optional\nRefresh tokens disabled]
    B -- Yes --> D{RefreshCookieName set?}
    D -- No --> E[Validate returns error\ntoken issuance HTTP 500]
    D -- Yes --> F[Refresh token issued\nstored in HttpOnly cookie]
Loading

Comments Outside Diff (1)

  1. README.md, line 520 (link)

    P2 The RefreshCookieName field is still labelled // optional in the detailed AuthHandler reference block further down in the README. This is now inconsistent with the quick-start comment fixed in this PR and with the auth.go source comment ("Required when Sessions is non-nil"). AuthHandler.Validate() returns an error when Sessions is set and RefreshCookieName is empty, so calling it optional is misleading.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: README.md
    Line: 520
    
    Comment:
    The `RefreshCookieName` field is still labelled `// optional` in the detailed `AuthHandler` reference block further down in the README. This is now inconsistent with the quick-start comment fixed in this PR and with the `auth.go` source comment ("Required when Sessions is non-nil"). `AuthHandler.Validate()` returns an error when `Sessions` is set and `RefreshCookieName` is empty, so calling it optional is misleading.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Codex

Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
README.md:520
The `RefreshCookieName` field is still labelled `// optional` in the detailed `AuthHandler` reference block further down in the README. This is now inconsistent with the quick-start comment fixed in this PR and with the `auth.go` source comment ("Required when Sessions is non-nil"). `AuthHandler.Validate()` returns an error when `Sessions` is set and `RefreshCookieName` is empty, so calling it optional is misleading.

```suggestion
    RefreshCookieName: "refresh",  // required when Sessions is set; stores refresh token in an HttpOnly cookie
```

Reviews (1): Last reviewed commit: "docs: fix README feature list and Refres..." | Re-trigger Greptile

- Add 'generic OAuth2 login (GitHub, Discord, Slack, and any custom
  provider)' to the intro feature list; the OAuth2Handler and built-in
  providers (GitHubProvider, GoogleOAuth2Provider) exist in the codebase
  but were absent from the README description (present in docs/index.md)
- Correct the RefreshCookieName quick-start comment from 'optional' to
  'required when Sessions is set'; AuthHandler.Validate() enforces this
  constraint and returns an error when Sessions is non-nil but
  RefreshCookieName is empty

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@veverkap veverkap requested review from a team and Copilot May 11, 2026 16:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates top-level documentation to accurately reflect existing OAuth2 support and handler configuration requirements.

Changes:

  • Add generic OAuth2 login (e.g., GitHub/Discord/Slack/custom providers) to the README feature list to match the existing OAuth2Handler functionality documented elsewhere.
  • Correct the Quick Start RefreshCookieName comment to indicate it’s required when Sessions is set (consistent with AuthHandler.Validate() behavior).
Show a summary per file
File Description
README.md Aligns the feature list with existing OAuth2 capabilities and fixes Quick Start guidance for RefreshCookieName when Sessions is enabled.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@veverkap veverkap merged commit a1ef754 into main May 11, 2026
25 checks passed
@veverkap veverkap deleted the docs/fix-readme-oauth2-and-refresh-cookie-06709c5f1aa5865f branch May 11, 2026 17:58
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.

2 participants