Skip to content

refactor(handlers): consolidate client display conversion and remove magic strings#100

Merged
appleboy merged 1 commit intomainfrom
refactor/consolidate-client-display
Mar 12, 2026
Merged

refactor(handlers): consolidate client display conversion and remove magic strings#100
appleboy merged 1 commit intomainfrom
refactor/consolidate-client-display

Conversation

@appleboy
Copy link
Copy Markdown
Member

Summary

  • Extract shared clientToDisplay helper in utils.go, replacing duplicate appToDisplay and 2 inline OAuthApplicationClientDisplay conversions across client.go and user_client.go
  • Export ContextKeyClientIP constant from middleware/context.go, replacing 6 raw "client_ip" string occurrences across handlers, middleware, and tests
  • Merge duplicate error page rendering in authorization.go:redirectWithError into a single conditional

Test plan

  • All existing tests pass (go test ./...)
  • Linter passes (make lint)
  • Build succeeds (go build ./...)
  • Verify login flow works (session fingerprint uses new constant)
  • Verify OAuth callback flow works
  • Verify admin client create/edit pages render correctly

🤖 Generated with Claude Code

…magic strings

- Extract shared clientToDisplay helper in utils.go, replacing duplicate appToDisplay and inline OAuthApplication-to-ClientDisplay conversions
- Export ContextKeyClientIP constant from middleware, replacing raw "client_ip" string across handlers and tests
- Merge duplicate error page rendering in redirectWithError into single conditional

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 11, 2026 15:39
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

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

Refactors handler code to centralize OAuth client-to-template conversion logic and standardize the gin context key used to store the client IP, reducing duplication and removing hard-coded strings across the auth/session fingerprint flows.

Changes:

  • Added a shared clientToDisplay helper in internal/handlers/utils.go and replaced duplicated OAuthApplicationClientDisplay mappings in admin/user client handlers.
  • Introduced middleware.ContextKeyClientIP and replaced raw "client_ip" usages in middleware, handlers, and tests.
  • Simplified AuthorizationHandler.redirectWithError by merging duplicate error-page rendering branches.

Reviewed changes

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

Show a summary per file
File Description
internal/middleware/context.go Exports ContextKeyClientIP and uses it when storing client IP in gin context.
internal/middleware/auth.go Uses ContextKeyClientIP when reading client IP for session fingerprinting.
internal/middleware/auth_test.go Updates tests to set client IP using ContextKeyClientIP.
internal/handlers/utils.go Adds shared clientToDisplay helper for template display mapping.
internal/handlers/client.go Replaces inline client display mapping with clientToDisplay.
internal/handlers/user_client.go Replaces appToDisplay and other conversions with clientToDisplay, removes duplicate helper.
internal/handlers/auth.go Uses middleware.ContextKeyClientIP when generating login session fingerprint.
internal/handlers/oauth_handler.go Uses middleware.ContextKeyClientIP when generating OAuth callback session fingerprint.
internal/handlers/authorization.go Consolidates redirect error handling into a single conditional branch.

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

@appleboy appleboy merged commit 2e5d0ff into main Mar 12, 2026
20 of 21 checks passed
@appleboy appleboy deleted the refactor/consolidate-client-display branch March 12, 2026 03:43
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