Conversation
appleboy
commented
Mar 3, 2026
- Centralize repeated handler logic by introducing shared utilities for navbar construction, error rendering, and slice-to-pointer conversion
- Refactor audit log handlers to reuse common query parsing for pagination and filters
- Replace inline error page rendering across handlers with a unified helper for consistent behavior
- Simplify session token ownership checks by adding a direct service method instead of iterating over all tokens
- Remove duplicated OAuth fingerprint logic and reuse a shared fingerprint implementation
- Switch cryptographic random byte generation to a common utility in OAuth and SQLite code paths
- Cache and reuse the OIDC issuer URL and simplify userinfo claim construction to accept a user model
- Update OIDC tests to reflect the new user-based claim builder API
- Centralize repeated handler logic by introducing shared utilities for navbar construction, error rendering, and slice-to-pointer conversion - Refactor audit log handlers to reuse common query parsing for pagination and filters - Replace inline error page rendering across handlers with a unified helper for consistent behavior - Simplify session token ownership checks by adding a direct service method instead of iterating over all tokens - Remove duplicated OAuth fingerprint logic and reuse a shared fingerprint implementation - Switch cryptographic random byte generation to a common utility in OAuth and SQLite code paths - Cache and reuse the OIDC issuer URL and simplify userinfo claim construction to accept a user model - Update OIDC tests to reflect the new user-based claim builder API Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR centralizes repeated boilerplate across HTTP handlers and related infrastructure. Instead of each handler independently constructing NavbarProps, rendering error pages, and generating random bytes, these operations are factored into shared utilities in internal/handlers/utils.go and internal/util/crypto.go. Additionally, IsTokenOwnedByUser replaces a manual O(n) token-list scan, the OIDC issuer URL is pre-computed once at construction time, and the buildUserInfoClaims function is simplified to accept a *models.User directly.
Changes:
- New
internal/handlers/utils.gointroducesbuildNavbarProps,renderErrorPage, andtoPointerSlicehelpers used across all handlers IsTokenOwnedByUseradded toTokenServiceto replace inline token-list iteration when checking session ownership- OIDC issuer URL pre-computed and cached in
OIDCHandler;buildUserInfoClaimsnow accepts*models.Userinstead of individual fields; tests updated accordingly
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/handlers/utils.go |
New file introducing shared handler utilities |
internal/handlers/session.go |
Uses renderErrorPage, buildNavbarProps, and the new IsTokenOwnedByUser |
internal/handlers/oauth_handler.go |
Removes duplicate fingerprint function; uses renderErrorPage and util.CryptoRandomBytes |
internal/handlers/oidc.go |
Caches issuer URL; simplifies buildUserInfoClaims to accept *models.User |
internal/handlers/oidc_test.go |
Updates tests to use new user-based buildUserInfoClaims API |
internal/handlers/client.go |
Uses renderErrorPage and buildNavbarProps throughout |
internal/handlers/authorization.go |
Uses renderErrorPage and buildNavbarProps |
internal/handlers/audit.go |
Extracts parseAuditParams; uses renderErrorPage, buildNavbarProps, and toPointerSlice |
internal/services/token.go |
Adds IsTokenOwnedByUser to TokenService |
internal/store/sqlite.go |
Replaces inline crypto/rand call with util.CryptoRandomBytes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Treat missing tokens as not owned by returning false without an error when a record is not found - Add database error handling dependency to support explicit not found checks - Add tests covering owned, not owned, and non-existent token ownership cases Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>