Skip to content

refactor(cli): deduplicate token form params and renderer state#45

Merged
appleboy merged 1 commit into
mainfrom
worktree-refactor
May 29, 2026
Merged

refactor(cli): deduplicate token form params and renderer state#45
appleboy merged 1 commit into
mainfrom
worktree-refactor

Conversation

@appleboy
Copy link
Copy Markdown
Member

Summary

Behavior-preserving cleanup of the CLI's OAuth/token code. Deduplicates three repeated patterns into shared helpers and removes a redundant boolean in the flow renderer. No functional change — output from the /simplify review pass.

AI Authorship

  • No AI was used in this PR
  • AI was used. Details:
    • Tool / model: Claude Opus 4.8 (1M context) via Claude Code
    • AI-authored files: all six — auth.go, browser_flow.go, device_flow.go, token_cmd.go, tokens.go, tui/flow_renderer.go
    • Human line-by-line reviewed: ⚠️ none yet — line-by-line review still pending at time of opening

Change classification

  • Core code (broad impact — needs line-by-line review)
    • The new setClientSecret / setExtraClaims / expiryFromNow helpers are now on the shared token path used by every OAuth flow (browser, device, refresh, revoke). A defect here propagates system-wide, so it warrants close review despite being a pure refactor.

What changed

  • Extract client_secret (confidential-client) and extra_claims form params into *AppConfig.setClientSecret / setExtraClaims, replacing four hand-inlined copies across the auth, browser, device, and revoke flows
  • Add expiryFromNow(expiresIn int) for token-expiry math, replacing three copies of time.Now().Add(time.Duration(x) * time.Second)
  • Drop the redundant hasInProgressStep bool in FlowRenderer; derive it from the existing inProgressStepIdx using -1 as the "none" sentinel

Verification

  • Existing unit + integration suite passes (go test ./... — both cli and cli/tui packages green)
  • go build ./..., go vet ./..., gofmt all clean
  • New tests: none added — this is a behavior-preserving refactor; correctness is covered by the existing suite that already exercises all four OAuth flows and the renderer
  • Manual verification: none beyond the test suite (no behavior change to exercise)

Verifiability check

  • Helpers have small, documented signatures; reviewer can confirm equivalence by comparing each call site to the inlined original
  • Sentinel change (-1 for no in-progress step) is initialized in NewFlowRenderer and read in two derived sites

Risk & rollback

  • Risk: low. The only subtle point is the renderer sentinel — inProgressStepIdx must be initialized to -1 (done in NewFlowRenderer) so the spinner/redraw guards behave as before. The form-param helpers preserve the exact public/confidential-client and empty-claims conditionals.
  • Rollback: revert the single commit — no migrations, no schema, no API changes.

Reviewer guide

  • Read carefully: tokens.go (the three new helpers and that tokenResponseToCredstore still computes the same expiry) and each call site in auth.go / browser_flow.go / device_flow.go / token_cmd.go to confirm the conditionals match the originals.
  • Spot-check OK: tui/flow_renderer.go — verify inProgressStepIdx init to -1 and the two >= 0 / < 0 guards; the rest is mechanical.

🤖 Generated with Claude Code

- Extract client_secret and extra_claims form helpers shared by auth, browser, device, and revoke flows
- Add a single helper for computing token expiry from a duration in seconds
- Derive in-progress step state from the step index sentinel instead of a redundant boolean flag

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 29, 2026 14:43
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tui/flow_renderer.go 0.00% 7 Missing ⚠️
tokens.go 77.77% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

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

This PR refactors shared OAuth/token request logic in the CLI to reduce duplicated form parameter and expiry calculations, while simplifying flow-renderer state tracking without intended behavior changes.

Changes:

  • Adds shared helpers for token expiry, client_secret, and extra_claims form parameters.
  • Replaces duplicated helper logic across browser, device, refresh, and revoke paths.
  • Removes the redundant renderer boolean by using -1 as the no-active-step sentinel.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tokens.go Adds shared helpers and uses expiry helper in token conversion.
auth.go Uses shared helpers when refreshing access tokens.
browser_flow.go Uses shared helpers during authorization-code exchange.
device_flow.go Uses shared helpers for device-flow expiry and extra claims.
token_cmd.go Uses shared helper for revoke client_secret handling.
tui/flow_renderer.go Derives in-progress renderer state from the step index sentinel.

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

@appleboy appleboy merged commit ad79707 into main May 29, 2026
17 checks passed
@appleboy appleboy deleted the worktree-refactor branch May 29, 2026 14:50
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