docs: fix RefreshCookieName described as optional in quick start#202
Merged
veverkap merged 1 commit intoMay 3, 2026
Merged
Conversation
The inline comment in docs/index.md and the struct comment in handler/auth.go both implied that RefreshCookieName is optional when Sessions is set. In reality, issueTokens returns HTTP 500 'server misconfiguration' when Sessions is non-nil but RefreshCookieName is empty (fast-fail guard in handler/helpers.go). - docs/index.md: change '// optional: deliver refresh token via cookie' to '// required when Sessions is set' - handler/auth.go: replace the misleading 'When empty the refresh token is only returned in the response body' struct comment with accurate language matching the documented and implemented behaviour Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates top-level docs and AuthHandler field documentation to accurately reflect the runtime requirement that RefreshCookieName must be set when Sessions is configured (since token issuance otherwise fails fast with HTTP 500 "server misconfiguration").
Changes:
- Adjust quick start snippet in
docs/index.mdto markRefreshCookieNameas required whenSessionsis set. - Update
handler/auth.go’sAuthHandler.RefreshCookieNamecomment to describe the real failure mode when omitted.
Show a summary per file
| File | Description |
|---|---|
| docs/index.md | Fixes quick start comment to match issueTokens misconfiguration behavior. |
| handler/auth.go | Corrects RefreshCookieName struct field comment to reflect the runtime guard (HTTP 500 on misconfig). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The inline comment in
docs/index.mdand the struct field comment inhandler/auth.goboth implied thatRefreshCookieNameis optional whenSessionsis set. This conflicts with the actual runtime behaviour:issueTokens(inhandler/helpers.go) returns HTTP 500"server misconfiguration"whenSessionsis non-nil butRefreshCookieNameis empty.This same inaccuracy was corrected in the handler-specific docs (
auth.md,passkeys.md,magic-links.md) by the previous PR (#199), but the top-level quick start example and the struct comment were not updated at that time.Changes
docs/index.md— Quick start code snippet: changedto
handler/auth.go—AuthHandler.RefreshCookieNamestruct comment: replacedwith accurate language:
Why this matters
A developer reading the quick start or hovering over the struct field in their IDE would conclude that omitting
RefreshCookieNameis safe. In practice it causes a silent HTTP 500 at runtime on the first login attempt. The fix ensures the quick start and field documentation agree with both the runtime guard and the handler-level docs updated by #199.Testing
No logic was changed. The existing test suite covers the
issueTokensmisconfiguration path.Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Greptile Summary
This PR corrects two documentation inaccuracies: the inline comment in
docs/index.mdand the struct-field GoDoc comment inhandler/auth.goboth previously impliedRefreshCookieNamewas optional whenSessionsis set, when in factissueTokensinhandler/helpers.goreturns HTTP 500"server misconfiguration"ifSessions != nilandRefreshCookieName == "". The updated comments now accurately reflect the enforced runtime requirement, consistent with the handler-specific docs fixed in PR #199.Confidence Score: 5/5
Safe to merge — documentation-only fix with no logic changes.
Both changed lines are comments/GoDoc. The correction is verified accurate against the runtime guard in handler/helpers.go lines 38–42. No code behaviour is altered.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Handler participant issueTokens participant SessionStore Client->>Handler: POST /login Handler->>issueTokens: sessions, refreshCookieName, ... alt Sessions != nil && RefreshCookieName == "" issueTokens-->>Handler: HTTP 500 "server misconfiguration" Handler-->>Client: 500 Internal Server Error else Sessions != nil && RefreshCookieName set issueTokens->>SessionStore: CreateSession(...) SessionStore-->>issueTokens: session issueTokens-->>Handler: accessToken, refreshToken, ok=true Handler-->>Client: 200 OK + cookies endReviews (1): Last reviewed commit: "docs: fix RefreshCookieName described as..." | Re-trigger Greptile