Staging

[Gospelmairo]

Pull Request Template

Description

Related Issue (Link to issue ticket)

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate - Postman, etc)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

Summary by CodeRabbit

Release Notes

• New Features

  • Apple Sign-In authentication support
  • Password-based login with reset-code verification
  • Google Meet integration for meeting scheduling
  • Messenger contact mode for discovery calls
  • Trainer availability toggle and profile editing
  • Account deactivation, reactivation, and deletion
  • Admin session management (cancel/reschedule)
  • Real-time in-app notifications via WebSocket
  • Admin notifications for key events

• Bug Fixes

  • Enhanced trainer booking-slot filtering by availability

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds Apple Sign-In handler and JWT verifier, Google Meet OAuth/provider with bootstrap CLI, multi-platform meeting routing, deactivated-user middleware, new admin session ops, extended booking/contact modes, self-account lifecycle endpoints, notifications fan-out, and comprehensive schema/SQL/codegen updates.

Changes

Unified Auth, Meetings, and Admin Ops

Layer / File(s)	Summary
Apple Sign-In handler and verifier internal/auth/apple.go, pkg/apple/*, internal/routes/auth.go, api.yaml	Implements Apple JWT verification, user find/create by sub, session issuance; routes POST /auth/apple.
Google Meet integration pkg/googlemeet/*, docs/MEET_INTEGRATION.md, cmd/meet-bootstrap/main.go, internal/config/config.go	Adds Meet OAuth client, provider for spaces create/end, bootstrap CLI, env/config wiring and runbook.
Meeting platform routing pkg/meeting/*, internal/bookings/*, internal/discovery/handler.go, internal/zoomflow/selector.go	Introduces multi-platform selector and platform-aware create/reschedule; adds messenger handle, meet support.
Deactivation and self-account lifecycle internal/middleware/deactivated.go, internal/routes/profile.go, api.yaml	Middleware blocks deactivated users; adds self deactivate/reactivate/delete endpoints.
Admin sessions and notifications internal/routes/admin_sessions.go, internal/routes/admin.go, internal/notification/*, api.yaml	Adds admin list/cancel/reschedule sessions, active sessions list, and admin broadcast notifications.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant API
  participant MeetingSelector
  participant GoogleMeet as Google Meet API
  participant DB
  Client->>API: POST /bookings {session_platform}
  API->>MeetingSelector: For(trainerID, platform)
  MeetingSelector-->>API: Provider (Meet/Zoom/NoOp)
  alt google_meet
    API->>GoogleMeet: POST /spaces (Bearer AT)
    GoogleMeet-->>API: meetingUri, name
    API->>DB: Insert booking with link/id
  else other/no-op
    API->>DB: Insert booking without link
  end
  API-->>Client: 201 booking payload

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• hngprojects/personal-trainer-be#328 — Prior extension of booking/discovery platforms and messenger handle aligns with this PR’s migration and handler updates.
• hngprojects/personal-trainer-be#331 — Similar conditional FK changes around payments in cascade migrations appear in both PRs.
• hngprojects/personal-trainer-be#266 — Overlaps on notifications WebSocket route and middleware wiring.

Suggested reviewers

• CynthiaWahome
• Taterbro
• ukemeikot

Poem

A rabbit taps in terminal light,
Tokens hop—apple, meet—alight.
Zooms and Meets in platform waltz,
Deactivate, then fix our faults.
Admin winds now softly blow,
Notifications thump—let’s go!
Little paws ship big PRs. 🐇✨

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch staging

[coderabbitai]

Actionable comments posted: 19

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

cmd/seed/main.go (1)

44-50: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the hint message to include "staging".

The guard now accepts both "development" and "staging", but the hint on Line 47 only mentions "development".

📝 Proposed fix to update the hint

 	if cfg.Env != "development" && cfg.Env != "staging" {
 		slog.Error("seed script cannot run in production",
 			"got_env", cfg.Env,
-			"hint", "set APP_ENV=development if you really mean to run this against your local DB",
+			"hint", "set APP_ENV=development or APP_ENV=staging if you really mean to run this against your DB",
 		)
 		os.Exit(1)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/seed/main.go` around lines 44 - 50, The slog.Error call guarding the seed
script checks cfg.Env for both "development" and "staging" but its hint string
only mentions "development"; update the hint in the slog.Error invocation so it
references both environments (e.g., "set APP_ENV=development or APP_ENV=staging
if you really mean to run this against your local DB") to accurately reflect the
allowed values and help users run the script intentionally.

internal/bookings/handlers.go (1)

160-197: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Ignore messenger_handle unless session_platform == "messenger".

The validation comment says non-messenger handles are ignored, but this path still persists any non-empty messenger_handle for Zoom/Google Meet bookings. Because CreateBookingParams is forwarded unchanged into the repository layer, that stale handle will be written to the booking row instead of being dropped.

Suggested fix

 	var messengerNS sql.NullString
-	if messengerHandle != "" {
+	if platformStr == "messenger" && messengerHandle != "" {
 		messengerNS = sql.NullString{Valid: true, String: messengerHandle}
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/bookings/handlers.go` around lines 160 - 197, In
HandleCreateBookingSession the messengerHandle is always included in
db.CreateBookingParams (MessengerHandle), so non-messenger bookings can persist
a stale handle; change the assignment so MessengerHandle is only set when
platformStr == "messenger" (i.e., set messengerNS = sql.NullString{Valid:true,
String: messengerHandle} only for platformStr == "messenger"}, otherwise set
MessengerHandle to sql.NullString{Valid:false}) before constructing the
CreateBookingParams to ensure non-messenger platforms do not store the handle.

🧹 Nitpick comments (9)

internal/discovery/handler.go (1)

847-875: ⚡ Quick win

Update error message to reflect multi-platform meeting creation.

The function createMeetingWithRetry now accepts a platform parameter (line 847) and handles both Zoom and Google Meet via the provider selector. However, the final error message at line 873 still reads "zoom meeting creation failed after all retries", which is misleading when the failure occurs for a Google Meet platform. Consider making the message platform-neutral or including the platform name.

♻️ Proposed fix for a generic error message

-	h.log.Error("zoom meeting creation failed after all retries")
+	h.log.Error("meeting creation failed after all retries", "platform", platform)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/discovery/handler.go` around lines 847 - 875, The final log in
createMeetingWithRetry currently hardcodes "zoom meeting creation failed after
all retries"; update the h.log.Error call to be platform-aware by either using a
generic message like "meeting creation failed after all retries" or include the
platform variable (e.g., "meeting creation failed after all retries",
"platform", platform) so the error accurately reflects the provider being used;
locate the h.log.Error at the end of createMeetingWithRetry and replace the
static Zoom text accordingly.

internal/routes/admin.go (1)

118-130: ⚡ Quick win

Prefer the existing parsePagination helper for consistency.

This handler manually parses page and limit with inline defaults and bounds, but the file already has a parsePagination helper (used at line 196) that centralizes the same logic. Using the helper would ensure consistent validation and error responses across all admin endpoints.

♻️ Proposed refactor to use the helper

-	// Parse page/limit from query string manually since this handler has no
-	// oapi-codegen params struct. Defaults: page=1, limit=10, max limit=100.
-	pageVal := 1
-	limitVal := 10
-	if p := c.Query("page"); p != "" {
-		if n, err := strconv.Atoi(p); err == nil && n >= 1 {
-			pageVal = n
-		}
-	}
-	if l := c.Query("limit"); l != "" {
-		if n, err := strconv.Atoi(l); err == nil && n >= 1 && n <= 100 {
-			limitVal = n
-		}
-	}
-	page, limit := pageVal, limitVal
+	// Parse pagination using the existing helper (returns and writes error response on failure)
+	var pagePtr, limitPtr *int
+	if p := c.Query("page"); p != "" {
+		if n, err := strconv.Atoi(p); err == nil {
+			pagePtr = &n
+		}
+	}
+	if l := c.Query("limit"); l != "" {
+		if n, err := strconv.Atoi(l); err == nil {
+			limitPtr = &n
+		}
+	}
+	page, limit, ok := parsePagination(c, pagePtr, limitPtr, s.logger)
+	if !ok {
+		return
+	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/routes/admin.go` around lines 118 - 130, This handler duplicates
manual pagination parsing (pageVal/limitVal) instead of reusing the existing
parsePagination helper; replace the inline parsing block that sets
pageVal/limitVal and page, limit with a call to parsePagination(c) and use its
returned (page, limit, err), returning the same error response when err != nil;
this ensures consistent validation and bounds logic across handlers and matches
other uses (see parsePagination usage at line ~196).

internal/notification/service.go (1)

108-149: 💤 Low value

Consider standardizing idempotency key base naming.

The suffix pattern "-admin-" + adminID.String() is correct for avoiding UNIQUE constraint collisions. However, some callers pass base keys that already contain "admin" (e.g., "zoom-connected-admin-<userID>-<ts>" in zoom_oauth.go:207), which produces keys like "zoom-connected-admin-<userID>-<ts>-admin-<adminID>". While functionally correct, the redundant "admin" token is confusing.

Consider establishing a naming convention where base keys for admin broadcasts omit role indicators (e.g., "zoom-connected-<userID>-<ts>" instead of "zoom-connected-admin-<userID>-<ts>"), since the per-admin suffix already provides that context.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/notification/service.go` around lines 108 - 149,
SendNotificationToAdmins builds per-admin idempotency keys by appending
"-admin-<userID>" to idempotencyKeyBase, but some callers already include
"admin" in their base (e.g., zoom-connected-admin-...), causing redundant
tokens; update SendNotificationToAdmins to normalize idempotencyKeyBase before
appending by stripping a trailing "-admin" (or any trailing
"-admin-<something>") so the final key is always "<base>-admin-<adminID>";
locate the normalization in SendNotificationToAdmins (which constructs key :=
idempotencyKeyBase + "-admin-" + adminID.String()) and replace it with a small
sanitizer that removes a trailing "-admin" segment from idempotencyKeyBase (or
document and update callers to omit role indicators instead) while preserving
existing duplicate-key handling via ErrDuplicateIdempotencyKey and
SendNotificationToUser.

internal/auth/password_reset.go (2)

414-489: ⚡ Quick win

Consider extracting shared validation and eligibility logic.

HandleVerifyResetCode duplicates validation (email/code format), rate limiting setup, and user eligibility checks from HandleResetPassword. For security-critical password reset flows, keeping these validation rules in sync is important for maintaining consistent behavior.

Extracting the shared logic (email/code validation at lines 427-440, rate limiting at lines 442-456, and eligibility checks at lines 468-480) into helper methods would reduce duplication and ensure consistency when either handler is updated.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/auth/password_reset.go` around lines 414 - 489,
HandleVerifyResetCode duplicates email/code validation, rate-limiter checks, and
user eligibility logic found in HandleResetPassword; extract those into shared
helper methods on PasswordResetHandler (e.g., ValidateResetRequest(email, code)
-> ([]api.FieldError, error), EnforceResetRateLimits(ctx, clientIP, email) ->
(allowed bool, err error) or return api response error, and
CheckResetEligibility(ctx, email) -> (*User, error)) and call them from both
handlers; ensure helpers perform the same checks (email/code format,
resetIPLimiter and resetLimiter Allow/Reset behavior, and user role/IsActive
checks against providerLocal/adminRoleName/superAdminRoleName/trainerRoleName)
and return consistent API errors so both HandleVerifyResetCode and
HandleResetPassword remain behaviorally identical.

---

414-489: ⚖️ Poor tradeoff

Reassess rate-limiter reset impact in /auth/verify-reset-code flow

HandleVerifyResetCode calls h.resetLimiter.Allow(emailAddr) and then—only after VerifyCode(...) succeeds and the same active/admin/trainer role gate passes—resets the per-email bucket via h.resetLimiter.Reset(...) (line ~484). The HandleResetPassword endpoint also consumes the OTP on success (ConsumeCodeAndUpdatePassword), and resets h.resetLimiter only after a successful reset.

Because the reset code is consumed on successful HandleResetPassword, a holder of a valid code effectively gains at most one successful password reset per issued OTP; repeatedly calling verify to “refresh” the limiter doesn’t meaningfully increase the number of successful resets per code. The main concern would be limited edge cases where HandleResetPassword fails without consuming the code—then repeated verify calls could undermine the intended throttle.

Consider using a separate limiter for the verify endpoint (or avoid resetting the reset-password limiter key on verify) if you want the limiter to apply uniformly even across those failure scenarios.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/auth/password_reset.go` around lines 414 - 489,
HandleVerifyResetCode currently calls h.resetLimiter.Allow(...) and then resets
that same limiter via h.resetLimiter.Reset(...) after VerifyCode and role
checks, which can let repeated verifies refresh the allowance and undermine
throttling intended for HandleResetPassword (which consumes codes via
ConsumeCodeAndUpdatePassword). Fix by separating the verify and reset-password
rate limits: introduce and use a distinct limiter for verification (e.g.,
h.verifyLimiter.Allow/Reset) or stop calling h.resetLimiter.Reset in
HandleVerifyResetCode so the password-reset limiter remains authoritative;
update HandleVerifyResetCode to use the new limiter (or remove the reset call)
and ensure HandleResetPassword continues to use h.resetLimiter and calls Reset
only after successful ConsumeCodeAndUpdatePassword.

pkg/googlemeet/oauth.go (1)

134-144: ⚡ Quick win

Parse JSON before checking for "invalid_grant" to avoid false positives.

Line 140 searches for "invalid_grant" in the raw response body string before parsing JSON. This could match the substring in unexpected places (e.g., error messages mentioning "invalid_grant" as an example). Parse the JSON structure first and check the error field properly.

♻️ Proposed fix

 	raw, _ := io.ReadAll(resp.Body)
 	if resp.StatusCode >= 400 {
-		// invalid_grant is the specific failure operators need to act
-		// on (refresh token revoked; bootstrap helper must be re-run).
-		// Distinguish from generic 4xx/5xx so the handler can show a
-		// useful message without parsing JSON.
-		if strings.Contains(string(raw), "invalid_grant") {
-			return "", time.Time{}, ErrTokenRevoked
+		var errResp struct {
+			Error string `json:"error"`
+		}
+		if err := json.Unmarshal(raw, &errResp); err == nil && errResp.Error == "invalid_grant" {
+			return "", time.Time{}, ErrTokenRevoked
 		}
 		return "", time.Time{}, fmt.Errorf("googlemeet: token endpoint %d: %s", resp.StatusCode, string(raw))
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/googlemeet/oauth.go` around lines 134 - 144, The current logic reads
resp.Body into raw and searches the raw string for "invalid_grant" which can
produce false positives; instead, after reading raw (or by decoding resp.Body),
unmarshal the JSON into a small struct (e.g., struct{ Error string
`json:"error"`; ErrorDescription string `json:"error_description"` }) and
inspect the Error field for the value "invalid_grant" before returning
ErrTokenRevoked; if JSON decoding fails, fall back to the existing behavior of
returning a formatted error with resp.StatusCode and the raw body. Update the
error-handling block in pkg/googlemeet/oauth.go where resp.Body is read
(variables raw, resp) and where ErrTokenRevoked is returned.

internal/auth/local.go (1)

266-282: 💤 Low value

Track the codegen synchronization debt.

The local request struct workaround (lines 274-277) avoids codegen drift but bypasses type safety. This pattern should be temporary—consider opening an issue to track re-running codegen after resolving the PR #286 conflicts mentioned in the comment, so future changes benefit from the generated types.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/auth/local.go` around lines 266 - 282, The local inline request
struct (var req struct { Email string `json:"email"`; Password string
`json:"password"` }) used before c.ShouldBindJSON in the sign-in handler avoids
codegen drift but should be temporary; create a tracked issue in your repo
referencing the handler (the function containing c.ShouldBindJSON and
h.log.Warn) to re-run the API code generator and restore use of the generated
api.HandleLocalAuthJSONRequestBody type once PR `#286` conflicts are resolved, and
add a TODO comment linking that issue ID next to the inline req declaration so
future contributors know to replace the local struct with the generated type.

pkg/googlemeet/provider.go (2)

80-82: ⚡ Quick win

Silently discarding io.ReadAll errors can mask I/O problems.

The error returned by io.ReadAll is discarded with _ on Line 80 and Line 150. If the read fails (e.g., connection drop mid-response), the error message will contain partial or empty body text, making troubleshooting harder.

🔍 Proposed fix to log read errors

-	raw, _ := io.ReadAll(resp.Body)
+	raw, readErr := io.ReadAll(resp.Body)
+	if readErr != nil {
+		return "", "", fmt.Errorf("googlemeet: create space read error: %w", readErr)
+	}
	if resp.StatusCode >= 400 {
		return "", "", fmt.Errorf("googlemeet: create space %d: %s", resp.StatusCode, string(raw))
	}

Apply the same pattern to Line 150 in DeleteMeeting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/googlemeet/provider.go` around lines 80 - 82, The code in CreateSpace
(the block returning fmt.Errorf("googlemeet: create space...")) and in
DeleteMeeting is discarding the error from io.ReadAll; change the io.ReadAll
call to capture the error (raw, err := io.ReadAll(resp.Body)), check err and
include that error information in the returned error message (or wrap it) so the
final fmt.Errorf for both CreateSpace and DeleteMeeting contains the response
body and any read error details to avoid silencing I/O failures.

---

151-152: 💤 Low value

String-based error detection is fragile but acceptable for this specific API contract.

The check strings.Contains(string(raw), "no active conference") on Line 151 treats a specific 400 error as success. This is brittle—if Google changes the error message wording, the condition will miss it and return an error instead of silently succeeding. However, since this matches the Zoom provider's defensive DeleteMeeting behavior (comment on Line 114-116), and the consequence of a miss is just a logged warning (no data corruption), this is acceptable.

Consider documenting the exact error string returned by Google in a comment to aid future troubleshooting if the check breaks.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/googlemeet/provider.go` around lines 151 - 152, The fragile string check
in the DeleteMeeting flow (the resp.StatusCode == http.StatusBadRequest &&
strings.Contains(string(raw), "no active conference") branch in provider.go)
should be preserved but explicitly documented: add a comment above that
condition noting the exact Google error text being matched ("no active
conference") and why we accept it (mirrors Zoom provider DeleteMeeting behavior
and only results in a warning). Mention the HTTP 400 status and that this is a
defensive check for the provider's API contract so future readers can
troubleshoot if Google changes the wording.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.env.example:
- Line 71: There are duplicate declarations of the environment variable
APPLE_SIGN_IN_BUNDLE_IDS which causes the later (often blank) entry to silently
override the earlier one; remove the redundant declaration so only a single
canonical APPLE_SIGN_IN_BUNDLE_IDS entry remains in the .env.example, keep the
intended value/comment in the chosen section, and ensure any related
documentation or example comments reference that single declaration to prevent
accidental overrides.

In `@api.yaml`:
- Around line 7463-7483: The docs currently instruct clients to send the real
access_token via the query parameter named "token" (and to reuse the normal
access_token), which leaks credentials; update the OpenAPI parameters so the
"token" query parameter is removed or reworded to forbid using the long-lived
access_token, and replace guidance with recommending a short-lived WebSocket
ticket or same-site cookie for browser/React Native clients while continuing to
prefer the "Authorization" header for server-side callers; ensure the parameter
descriptions for "token" and "Authorization" (the symbols to change) clearly
state not to send bearer tokens in URLs and show the secure alternatives.
- Around line 4439-4443: The spec is inconsistent: the new "verify reset code"
step describes trainer users using a forgot-password flow but the downstream
POST /auth/reset-password remains documented as admin-only; reconcile by either
(A) updating POST /auth/reset-password to accept the trainer/forgot-password
flow (remove or relax admin-only constraints and document parameters/permissions
for user self-service reset) or (B) restrict the new verify-reset-code
description to admin-initiated resets only; update the descriptive text for the
verify-reset-code step and the POST /auth/reset-password operation so both refer
to the same actor/permission model (reference the verify-reset-code description
block and the POST /auth/reset-password operation name/path when applying the
change).
- Around line 1538-1546: The OpenAPI schema's password property incorrectly uses
maxLength: 72 (characters) but bcrypt limits the password to 72 UTF-8 bytes;
update the password schema (the "password" property) to remove or avoid relying
on maxLength for bytes, amend the description to state "bcrypt truncates to 72
bytes of the UTF-8-encoded password" and instruct callers that the server
validates by UTF-8 byte length, and optionally add a vendor extension (e.g.,
x-max-bytes: 72) or note that clients should validate UTF-8 byte length
themselves so generated clients don't assume a 72-character limit.

In `@cmd/createTrainers/main.go`:
- Around line 154-156: The shared HTTP client "client" used with
generateAccessToken and later trainer-create requests has no timeout and can
hang the whole batch; update the client initialization (the "client" variable)
to include a sensible Timeout (e.g. http.Client{Timeout: 10 * time.Second} or a
configurable value) or ensure each request uses a context with deadline, then
pass that client into generateAccessToken and downstream request functions so
all HTTP calls (login and trainer-create) respect the timeout.
- Around line 295-323: The non-201 branch leaks the HTTP response body for
status codes other than 400/409 because res.Body is only closed in some paths;
ensure res.Body.Close() is called on every non-201 path (including the generic
failure branch after appendIntoFailedTrainer and before continue) and handle its
error correctly (use the close error value, not the wrong variable like err).
Update the logic around ValidationErrorResponse handling and the generic failure
branch in the create trainers loop (references: res.Body.Close,
appendIntoFailedTrainer, ValidationErrorResponse) so every early continue/return
closes the body and reports any close error.

In `@internal/auth/auth_service.go`:
- Around line 63-96: GenerateTestTokens currently assigns a uuid.UUID directly
to the "sub" claim in accessClaims and refreshClaims which will JSON-marshal
incorrectly; convert the UUID to its string form before assignment (e.g., use
userId.String()) so "sub" is a StringOrURI like GenerateJWTToken uses, and
ensure both accessClaims and refreshClaims use the string value.

In `@internal/discovery/handler.go`:
- Around line 115-122: The error message returned when validating contact modes
is missing "imessage"; update the validation error in the branch that calls
h.log.Warn and c.JSON(api.NewError(...)) so the message lists all five accepted
modes: "zoom_meeting, google_meet, phone_callback, messenger, or imessage". Keep
the switch on mode as-is (cases: "zoom_meeting", "phone_callback",
"google_meet", "messenger", "imessage") and only change the text passed to
api.NewError (and any user-facing log if desired) to reflect the complete set of
allowed modes.

In `@internal/models/queries/booking_slots.sql`:
- Around line 14-30: The booking-slots query GetTrainersBookingSlots
(internal/models/queries/booking_slots.sql) correctly filters by t.is_available
to control what is bookable, but the 15-minute cache around
HandleGetTrainersBookingSlots (internal/bookings/handlers.go) causes stale
visible slots after toggling a trainer; fix by invalidating or refreshing that
cache when a trainer's is_available flag changes (e.g., emit cache invalidation
on the trainer update path or reduce TTL), and ensure booking listing queries
(ListBookingsForAdmin, ListActiveBookingsForAdmin, ListBookingsByTrainer in
internal/models/queries/bookings.sql) remain unchanged so existing/past bookings
are still returned.

In `@internal/repository/db/users.sql.go`:
- Around line 476-512: UpdateTrainerUserProfile's SQL (updateTrainerUserProfile)
and its scanner are missing apple_user_id/AppleUserID in the RETURNING and Scan,
causing incomplete User results; update the SQL RETURNING clause to include
apple_user_id and regenerate the queries file, then update the
UpdateTrainerUserProfile method to scan the apple_user_id into the
User.AppleUserID field (match the position/order used by other user queries like
CreateUser/UpdateUserOnboarding/CreateAppleUser) so the returned User struct is
populated consistently.

In `@internal/routes/admin_subscriptions.go`:
- Line 32: GetAdminSubscriptions currently ignores the OpenAPI-bound params
parameter and re-reads query values from gin.Context; update the handler to use
params.Status, params.Page and params.Limit (or pass params into the existing
parse/validation helper) instead of calling c.Query or parseAdminPagination(c),
remove the unused params read duplication and ensure any pagination/validation
logic is centralized (e.g., adapt parseAdminPagination to accept
api.GetAdminSubscriptionsParams or validate params before use) so the function
signature and generated binding are actually used.

In `@internal/routes/admin_transactions.go`:
- Line 33: GetAdminTransactions (and similarly GetAdminSubscriptions) currently
ignores the provided api.GetAdminTransactionsParams /
api.GetAdminSubscriptionsParams and instead reads pagination from c.Query;
update the handlers to use params.Page and params.Limit (or change
parseAdminPagination to accept page/limit) so pagination comes from the
oapi-codegen params. Concretely: in GetAdminTransactions and
GetAdminSubscriptions replace calls that parse c.Query("page")/c.Query("limit")
by passing params.Page and params.Limit into the pagination helper (or update
parseAdminPagination signature to accept these values and call it with
params.Page/params.Limit), and remove any unused references to c.Query and the
unused params warning.

In `@internal/routes/profile.go`:
- Around line 201-210: The current handling of sql.ErrNoRows from
s.users.q.DeactivateSelf is ambiguous because DeactivateSelf filters on
is_active and can return no rows for either a non-existent user or an
already-deactivated user; update the flow to first call
s.users.q.GetUserByID(ctx, userID) when DeactivateSelf returns sql.ErrNoRows,
return 404 Not Found if GetUserByID also returns sql.ErrNoRows, otherwise return
409 Conflict with "account is already deactivated"; keep existing 500 handling
and logging for other DB errors and log contextual info (userID, err) when
returning 500.
- Around line 230-239: The sql.ErrNoRows from ReactivateSelf is ambiguous (could
mean user doesn't exist or is already active); update the handler to distinguish
those cases by, on ErrNoRows, querying user existence explicitly (e.g., call
s.users.q.GetByID or a similar lookup with the same userID) and return 404 Not
Found if that lookup returns sql.ErrNoRows, otherwise return 409 Conflict with
"account is already active"; keep existing logging and 500 handling for other
errors.

In `@internal/routes/trainers.go`:
- Around line 976-1001: The two separate DB calls (s.trainers.q.UpdateTrainer
and s.trainers.q.UpdateTrainerUserProfile) must be executed inside a single
transaction so the whole profile patch is atomic: open a transaction via your
DB/query layer (e.g. start tx or use the generated q.WithTx / Store.BeginTx
helper), run the UpdateTrainer call using the tx-scoped queries, then if
body.PhoneNumber != nil run UpdateTrainerUserProfile also on the same tx; on any
error rollback and return the 500, otherwise commit the tx and return success.
Ensure you use the tx-scoped query methods corresponding to UpdateTrainer and
UpdateTrainerUserProfile and reference trainerID/updated.UserID as before.
- Around line 993-998: The patch writes any trimmed phone string to
UpdateTrainerUserProfile without validating, so add the same E.164 validation
used in CreateTrainer: after computing phoneVal from body.PhoneNumber (in the
block that calls s.trainers.q.UpdateTrainerUserProfile), test phoneVal against
trainerPhoneE164Regex and if it does not match return a 400/validation error
(consistent with CreateTrainer's behavior) instead of calling
UpdateTrainerUserProfile; only call s.trainers.q.UpdateTrainerUserProfile when
the regex check passes.
- Around line 871-883: The PATCH handler currently uses a plain bool in the
request body so a missing is_available binds as false; change the anonymous body
struct's IsAvailable to *bool, keep using c.ShouldBindJSON(&body), then validate
that body.IsAvailable != nil and return a 400 (with the same logging/message) if
nil; when calling trainers.q.ToggleTrainerAvailability use the dereferenced
value for IsAvailable (e.g., *body.IsAvailable) so the update only runs when the
field was provided.

In `@migrations/000058_extend_booking_platforms_messenger_meet.sql`:
- Around line 44-48: Before re-adding the old check constraint in the down
migration, update any rows where bookings.session_platform = 'messenger' to a
permitted value (e.g., 'whatsapp' or 'zoom') so they won't violate the restored
constraint; specifically, add an UPDATE on the bookings table to remap
'messenger' values prior to executing ALTER TABLE bookings ADD CONSTRAINT
bookings_session_platform_check CHECK (session_platform IN ('whatsapp',
'google_meet', 'zoom')), and then drop the messenger_handle column/constraint as
currently written.

In `@pkg/googlemeet/oauth.go`:
- Around line 90-111: The AccessToken method currently holds OAuthClient.mu
across the network call to refresh(), causing serialized callers; change to a
double-checked locking pattern in AccessToken: acquire mu and check
accessToken/expiresAt, if expired then release mu, call c.refresh(ctx) (no
lock), then re-acquire mu and re-check accessToken/expiresAt in case another
goroutine refreshed in the meantime; only then assign c.accessToken and
c.expiresAt (or return the already-updated token) and release mu. Ensure you
still return errors from refresh and preserve the 60s skew logic used when
checking expiry.

---

Outside diff comments:
In `@cmd/seed/main.go`:
- Around line 44-50: The slog.Error call guarding the seed script checks cfg.Env
for both "development" and "staging" but its hint string only mentions
"development"; update the hint in the slog.Error invocation so it references
both environments (e.g., "set APP_ENV=development or APP_ENV=staging if you
really mean to run this against your local DB") to accurately reflect the
allowed values and help users run the script intentionally.

In `@internal/bookings/handlers.go`:
- Around line 160-197: In HandleCreateBookingSession the messengerHandle is
always included in db.CreateBookingParams (MessengerHandle), so non-messenger
bookings can persist a stale handle; change the assignment so MessengerHandle is
only set when platformStr == "messenger" (i.e., set messengerNS =
sql.NullString{Valid:true, String: messengerHandle} only for platformStr ==
"messenger"}, otherwise set MessengerHandle to sql.NullString{Valid:false})
before constructing the CreateBookingParams to ensure non-messenger platforms do
not store the handle.

---

Nitpick comments:
In `@internal/auth/local.go`:
- Around line 266-282: The local inline request struct (var req struct { Email
string `json:"email"`; Password string `json:"password"` }) used before
c.ShouldBindJSON in the sign-in handler avoids codegen drift but should be
temporary; create a tracked issue in your repo referencing the handler (the
function containing c.ShouldBindJSON and h.log.Warn) to re-run the API code
generator and restore use of the generated api.HandleLocalAuthJSONRequestBody
type once PR `#286` conflicts are resolved, and add a TODO comment linking that
issue ID next to the inline req declaration so future contributors know to
replace the local struct with the generated type.

In `@internal/auth/password_reset.go`:
- Around line 414-489: HandleVerifyResetCode duplicates email/code validation,
rate-limiter checks, and user eligibility logic found in HandleResetPassword;
extract those into shared helper methods on PasswordResetHandler (e.g.,
ValidateResetRequest(email, code) -> ([]api.FieldError, error),
EnforceResetRateLimits(ctx, clientIP, email) -> (allowed bool, err error) or
return api response error, and CheckResetEligibility(ctx, email) -> (*User,
error)) and call them from both handlers; ensure helpers perform the same checks
(email/code format, resetIPLimiter and resetLimiter Allow/Reset behavior, and
user role/IsActive checks against
providerLocal/adminRoleName/superAdminRoleName/trainerRoleName) and return
consistent API errors so both HandleVerifyResetCode and HandleResetPassword
remain behaviorally identical.
- Around line 414-489: HandleVerifyResetCode currently calls
h.resetLimiter.Allow(...) and then resets that same limiter via
h.resetLimiter.Reset(...) after VerifyCode and role checks, which can let
repeated verifies refresh the allowance and undermine throttling intended for
HandleResetPassword (which consumes codes via ConsumeCodeAndUpdatePassword). Fix
by separating the verify and reset-password rate limits: introduce and use a
distinct limiter for verification (e.g., h.verifyLimiter.Allow/Reset) or stop
calling h.resetLimiter.Reset in HandleVerifyResetCode so the password-reset
limiter remains authoritative; update HandleVerifyResetCode to use the new
limiter (or remove the reset call) and ensure HandleResetPassword continues to
use h.resetLimiter and calls Reset only after successful
ConsumeCodeAndUpdatePassword.

In `@internal/discovery/handler.go`:
- Around line 847-875: The final log in createMeetingWithRetry currently
hardcodes "zoom meeting creation failed after all retries"; update the
h.log.Error call to be platform-aware by either using a generic message like
"meeting creation failed after all retries" or include the platform variable
(e.g., "meeting creation failed after all retries", "platform", platform) so the
error accurately reflects the provider being used; locate the h.log.Error at the
end of createMeetingWithRetry and replace the static Zoom text accordingly.

In `@internal/notification/service.go`:
- Around line 108-149: SendNotificationToAdmins builds per-admin idempotency
keys by appending "-admin-<userID>" to idempotencyKeyBase, but some callers
already include "admin" in their base (e.g., zoom-connected-admin-...), causing
redundant tokens; update SendNotificationToAdmins to normalize
idempotencyKeyBase before appending by stripping a trailing "-admin" (or any
trailing "-admin-<something>") so the final key is always
"<base>-admin-<adminID>"; locate the normalization in SendNotificationToAdmins
(which constructs key := idempotencyKeyBase + "-admin-" + adminID.String()) and
replace it with a small sanitizer that removes a trailing "-admin" segment from
idempotencyKeyBase (or document and update callers to omit role indicators
instead) while preserving existing duplicate-key handling via
ErrDuplicateIdempotencyKey and SendNotificationToUser.

In `@internal/routes/admin.go`:
- Around line 118-130: This handler duplicates manual pagination parsing
(pageVal/limitVal) instead of reusing the existing parsePagination helper;
replace the inline parsing block that sets pageVal/limitVal and page, limit with
a call to parsePagination(c) and use its returned (page, limit, err), returning
the same error response when err != nil; this ensures consistent validation and
bounds logic across handlers and matches other uses (see parsePagination usage
at line ~196).

In `@pkg/googlemeet/oauth.go`:
- Around line 134-144: The current logic reads resp.Body into raw and searches
the raw string for "invalid_grant" which can produce false positives; instead,
after reading raw (or by decoding resp.Body), unmarshal the JSON into a small
struct (e.g., struct{ Error string `json:"error"`; ErrorDescription string
`json:"error_description"` }) and inspect the Error field for the value
"invalid_grant" before returning ErrTokenRevoked; if JSON decoding fails, fall
back to the existing behavior of returning a formatted error with
resp.StatusCode and the raw body. Update the error-handling block in
pkg/googlemeet/oauth.go where resp.Body is read (variables raw, resp) and where
ErrTokenRevoked is returned.

In `@pkg/googlemeet/provider.go`:
- Around line 80-82: The code in CreateSpace (the block returning
fmt.Errorf("googlemeet: create space...")) and in DeleteMeeting is discarding
the error from io.ReadAll; change the io.ReadAll call to capture the error (raw,
err := io.ReadAll(resp.Body)), check err and include that error information in
the returned error message (or wrap it) so the final fmt.Errorf for both
CreateSpace and DeleteMeeting contains the response body and any read error
details to avoid silencing I/O failures.
- Around line 151-152: The fragile string check in the DeleteMeeting flow (the
resp.StatusCode == http.StatusBadRequest && strings.Contains(string(raw), "no
active conference") branch in provider.go) should be preserved but explicitly
documented: add a comment above that condition noting the exact Google error
text being matched ("no active conference") and why we accept it (mirrors Zoom
provider DeleteMeeting behavior and only results in a warning). Mention the HTTP
400 status and that this is a defensive check for the provider's API contract so
future readers can troubleshoot if Google changes the wording.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b0e85f37-5e7d-42fa-b893-394cbc9a1a4e

📥 Commits

Reviewing files that changed from the base of the PR and between f701e7e and af6df8d.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (80)

• .env.example
• .gitignore
• .gitleaksignore
• api.yaml
• cmd/createTrainers/main.go
• cmd/meet-bootstrap/main.go
• cmd/seed/main.go
• docs/MEET_INTEGRATION.md
• go.mod
• internal/api/gen.go
• internal/auth/admin_login_test.go
• internal/auth/apple.go
• internal/auth/apple_test.go
• internal/auth/auth_service.go
• internal/auth/google_test.go
• internal/auth/local.go
• internal/auth/local_test.go
• internal/auth/password_reset.go
• internal/auth/repository.go
• internal/auth/sign_in_test.go
• internal/bookings/handler.go
• internal/bookings/handlers.go
• internal/bookings/service.go
• internal/config/config.go
• internal/dev/handler.go
• internal/discovery/handler.go
• internal/middleware/admin_only.go
• internal/middleware/deactivated.go
• internal/models/queries/booking_slots.sql
• internal/models/queries/bookings.sql
• internal/models/queries/discovery.sql
• internal/models/queries/password_reset.sql
• internal/models/queries/roles.sql
• internal/models/queries/subscriptions.sql
• internal/models/queries/trainers.sql
• internal/models/queries/users.sql
• internal/notification/admin_broadcast_test.go
• internal/notification/helpers_test.go
• internal/notification/repository.go
• internal/notification/service.go
• internal/repository/db/account_setup.sql.go
• internal/repository/db/booking_slots.sql.go
• internal/repository/db/bookings.sql.go
• internal/repository/db/discovery.sql.go
• internal/repository/db/models.go
• internal/repository/db/password_reset.sql.go
• internal/repository/db/reviews.sql.go
• internal/repository/db/roles.sql.go
• internal/repository/db/subscriptions.sql.go
• internal/repository/db/trainers.sql.go
• internal/repository/db/users.sql.go
• internal/routes/admin.go
• internal/routes/admin_sessions.go
• internal/routes/admin_subscriptions.go
• internal/routes/admin_transactions.go
• internal/routes/auth.go
• internal/routes/dev.go
• internal/routes/profile.go
• internal/routes/routes.go
• internal/routes/subscriptions.go
• internal/routes/trainers.go
• internal/routes/zoom_oauth.go
• internal/zoomflow/selector.go
• migrations/000058_extend_booking_platforms_messenger_meet.sql
• migrations/000060_user_account_management.sql
• migrations/000061_cascade_user_hard_delete.sql
• migrations/000062_cascade_booking_hard_delete.sql
• migrations/000063_add_apple_user_id_to_users.sql
• migrations/000064_add_is_available_to_trainers.sql
• oapi-codegen.yaml
• pkg/apple/verifier.go
• pkg/apple/verifier_test.go
• pkg/email/email.go
• pkg/googlemeet/oauth.go
• pkg/googlemeet/oauth_test.go
• pkg/googlemeet/provider.go
• pkg/googlemeet/provider_test.go
• pkg/meeting/multi_platform_test.go
• pkg/meeting/selector.go
• pkg/meeting/selector_test.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove duplicated APPLE_SIGN_IN_BUNDLE_IDS to avoid silent override.

APPLE_SIGN_IN_BUNDLE_IDS is declared twice (Line 71 and Line 200). In .env parsing, the later key wins, so a value set in the first occurrence can be unintentionally overridden by the second (often blank), breaking Apple sign-in audience validation. Keep exactly one declaration and one canonical section.

Also applies to: 200-200

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.env.example at line 71, There are duplicate declarations of the environment
variable APPLE_SIGN_IN_BUNDLE_IDS which causes the later (often blank) entry to
silently override the earlier one; remove the redundant declaration so only a
single canonical APPLE_SIGN_IN_BUNDLE_IDS entry remains in the .env.example,
keep the intended value/comment in the chosen section, and ensure any related
documentation or example comments reference that single declaration to prevent
accidental overrides.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate api.yaml and show the referenced section with line numbers
ls -la
fd -n 'api.yaml' . 2>/dev/null || true
# Use rg to find the specific maxLength: 72 near bcrypt description
rg -n --fixed-string "maxLength: 72" -S . -g'api.yaml' || true
# Show around the match with line numbers
file=$(fd -n 'api.yaml' . 2>/dev/null | head -n 1 || true)
if [ -z "${file:-}" ]; then
  echo "api.yaml not found via fd -n; falling back to rg results."
  file=$(rg -l --fixed-string "maxLength: 72" . -g'*.yaml' | head -n 1 || true)
fi
echo "Using file: ${file:-<none>}"
if [ -n "${file:-}" ]; then
  # print around the assumed lines 1538-1546
  sed -n '1525,1565p' "$file" | cat -n
  # also show exact lines with numbers from grep
  nl -ba "$file" | sed -n '1530,1560p'
fi

Repository: hngprojects/personal-trainer-be

Length of output: 1984

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate api.yaml and show the referenced section with line numbers
ls -la
fd -n 'api.yaml' . 2>/dev/null || true

# Use rg to find the specific maxLength: 72 near bcrypt description
rg -n --fixed-string "maxLength: 72" -S . -g'api.yaml' || true

# Show around the match with line numbers
file=$(fd -n 'api.yaml' . 2>/dev/null | head -n 1 || true)
if [ -z "${file:-}" ]; then
  echo "api.yaml not found via fd -n; falling back to rg results."
  file=$(rg -l --fixed-string "maxLength: 72" . -g'*.yaml' | head -n 1 || true)
fi
echo "Using file: ${file:-<none>}"
if [ -n "${file:-}" ]; then
  # print around the assumed lines 1538-1546
  sed -n '1525,1565p' "$file" | cat -n
  # also show exact lines with numbers from grep
  nl -ba "$file" | sed -n '1530,1560p'
fi

Repository: hngprojects/personal-trainer-be

Length of output: 1984

---

🌐 Web query:

OpenAPI Specification maxLength counts characters not bytes UTF-8

💡 Result:

In the OpenAPI Specification, the maxLength keyword counts characters, not bytes [1][2][3]. Because OpenAPI relies on JSON Schema for its validation keywords, maxLength is defined as the number of Unicode code points (logical characters) in a string instance [1][2]. This means that multibyte characters (such as those in UTF-8 that require more than one byte) are counted as a single character rather than their byte-length equivalent [1]. There is a distinct functional difference between character-based length and byte-based length: 1. Character Count (maxLength): This is the standard behavior in OpenAPI for string types, measuring the number of logical characters [1][2]. 2. Byte Count: OpenAPI does not provide a native property to constrain the byte-length of general strings [1]. For scenarios where byte-length validation is required (such as for binary data or specific database constraints), developers must often rely on custom validation, format-specific logic, or specialized extensions (e.g., custom tags used in frameworks like Kubernetes) [4][3][5]. An exception exists for unencoded binary data, where the specification explicitly notes that length is measured in octets (bytes) [6][7][8]. However, for standard string types, implementations that conflate character length with byte length (e.g., in generated client code) are often considered to be handling the specification incorrectly [3].

Citations:

• 1: https://www.learnjsonschema.com/draft7/validation/maxlength/
• 2: What is meaning of maxLength or minimum OAI/OpenAPI-Specification#1754
• 3: Byte length not calculated correctly in python OpenAPITools/openapi-generator#450
• 4: [Declarative Validation] Bring k8s:maxLength tag in line with OpenAPI maxLength validation semantics kubernetes/kubernetes#137291
• 5: Add +k8s:maxBytes and +k8s:minBytes OpenAPI representation kubernetes/kube-openapi#578
• 6: https://swagger.io/specification/v3/
• 7: https://spec.openapis.org/oas/v3.0.html
• 8: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.4.md

---

🌐 Web query:

bcrypt truncates password at 72 bytes behavior accepts longer inputs first 72 bytes

💡 Result:

The bcrypt algorithm has a hard limit of 72 bytes for passwords, a constraint originating from the underlying Blowfish cipher's P-box size [1][2][3]. The behavior regarding inputs longer than 72 bytes varies by implementation: 1. Traditional/Reference Behavior: The original OpenBSD implementation and many older libraries silently truncate passwords at the 72-byte mark, using only the first 72 bytes for hashing [4][1][5]. 2. Modern/Stricter Behavior: Many modern libraries (such as pyca/bcrypt, golang/crypto, and others) have updated their behavior to explicitly reject inputs exceeding 72 bytes by throwing an error (e.g., ValueError or a specific error object) [4][3][6][5]. This change was introduced to prevent user confusion and ensure that developers are aware that trailing characters are not contributing to the security of the hash [5]. Important Technical Considerations: - Byte vs. Character: The 72-byte limit applies to the number of bytes after UTF-8 encoding, not the number of characters [7]. Multibyte characters (such as emojis or certain non-Latin scripts) can cause a string to exceed 72 bytes even if it contains fewer than 72 characters [7]. - Security Recommendation: To securely handle passwords longer than 72 bytes, a common best practice is to pre-hash the password using a cryptographic hash function (like SHA-256) and then pass the resulting digest (encoded as a string or base64) to bcrypt [4][1][2]. This ensures the entire input is accounted for while remaining within the bcrypt byte limit. Top results: [4][1][7][3][6][5]

Citations:

• 1: https://security.stackexchange.com/questions/39849/does-bcrypt-have-a-maximum-password-length
• 2: https://crypto.stackexchange.com/questions/24993/is-there-a-way-to-use-bcrypt-with-passwords-longer-than-72-bytes-securely
• 3: https://github.com/bcgit/bc-java/blob/master/core/src/main/java/org/bouncycastle/crypto/generators/BCrypt.java
• 4: https://github.com/pyca/bcrypt/blob/main/README.rst
• 5: https://go.googlesource.com/crypto.git/+/bc7d1d1eb54b3530da4f5ec31625c95d7df40231
• 6: https://github.com/golang/crypto/blob/bc7d1d1eb54b3530da4f5ec31625c95d7df40231/bcrypt/bcrypt.go
• 7: https://github.com/kelektiv/node.bcrypt.js/blob/master/README.md

---

maxLength: 72 misrepresents bcrypt’s 72-byte UTF-8 limit

OpenAPI’s maxLength constrains string length by characters (code points), not UTF-8 bytes. bcrypt’s 72-byte cutoff is based on the byte length of the UTF-8-encoded password, so inputs with <72 characters but >72 bytes can still be truncated (or rejected depending on the bcrypt library), while generated clients may accept them—contradicting the behavior described in the schema.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api.yaml` around lines 1538 - 1546, The OpenAPI schema's password property
incorrectly uses maxLength: 72 (characters) but bcrypt limits the password to 72
UTF-8 bytes; update the password schema (the "password" property) to remove or
avoid relying on maxLength for bytes, amend the description to state "bcrypt
truncates to 72 bytes of the UTF-8-encoded password" and instruct callers that
the server validates by UTF-8 byte length, and optionally add a vendor extension
(e.g., x-max-bytes: 72) or note that clients should validate UTF-8 byte length
themselves so generated clients don't assume a 72-character limit.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

The published reset flow is now internally contradictory.

This new step positions trainer users inside the forgot-password flow, but the downstream POST /auth/reset-password docs still describe admin-only resets. As written, the spec tells trainer clients to verify a reset code they apparently cannot use in the final step.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api.yaml` around lines 4439 - 4443, The spec is inconsistent: the new "verify
reset code" step describes trainer users using a forgot-password flow but the
downstream POST /auth/reset-password remains documented as admin-only; reconcile
by either (A) updating POST /auth/reset-password to accept the
trainer/forgot-password flow (remove or relax admin-only constraints and
document parameters/permissions for user self-service reset) or (B) restrict the
new verify-reset-code description to admin-initiated resets only; update the
descriptive text for the verify-reset-code step and the POST
/auth/reset-password operation so both refer to the same actor/permission model
(reference the verify-reset-code description block and the POST
/auth/reset-password operation name/path when applying the change).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't ask clients to send the real access token in the WebSocket URL.

Putting a bearer token in ?token= leaks it into reverse-proxy logs, APM traces, browser history, and crash reports. Because the docs explicitly tell clients to reuse the normal access_token, anyone with log access can replay a live session until expiry. Prefer a short-lived WS ticket or same-site cookie for browser/RN clients, and keep header auth for server-side callers.

Also applies to: 7496-7550

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api.yaml` around lines 7463 - 7483, The docs currently instruct clients to
send the real access_token via the query parameter named "token" (and to reuse
the normal access_token), which leaks credentials; update the OpenAPI parameters
so the "token" query parameter is removed or reworded to forbid using the
long-lived access_token, and replace guidance with recommending a short-lived
WebSocket ticket or same-site cookie for browser/React Native clients while
continuing to prefer the "Authorization" header for server-side callers; ensure
the parameter descriptions for "token" and "Authorization" (the symbols to
change) clearly state not to send bearer tokens in URLs and show the secure
alternatives.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Set a timeout on the shared HTTP client.

This client is reused for login and every trainer-create request, but with no timeout a single stalled connection can hang the whole batch indefinitely.

Suggested fix

-	client := &http.Client{}
+	client := &http.Client{
+		Timeout: 30 * time.Second,
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	client := &http.Client{}
	accessToken, err := generateAccessToken(BASE_URL, client, email, password)
	if err != nil {
	client := &http.Client{
	Timeout: 30 * time.Second,
	}
	accessToken, err := generateAccessToken(BASE_URL, client, email, password)
	if err != nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/createTrainers/main.go` around lines 154 - 156, The shared HTTP client
"client" used with generateAccessToken and later trainer-create requests has no
timeout and can hang the whole batch; update the client initialization (the
"client" variable) to include a sensible Timeout (e.g. http.Client{Timeout: 10 *
time.Second} or a configurable value) or ensure each request uses a context with
deadline, then pass that client into generateAccessToken and downstream request
functions so all HTTP calls (login and trainer-create) respect the timeout.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require is_available to be present in the PATCH body.

With a plain bool, {} binds successfully as false, so a missing field will deactivate the trainer instead of being rejected as an invalid request.

Suggested fix

 	var body struct {
-		IsAvailable bool `json:"is_available"`
+		IsAvailable *bool `json:"is_available"`
 	}
 	if err := c.ShouldBindJSON(&body); err != nil {
 		s.logger.Warn("toggle trainer availability: invalid request body", "err", err)
 		c.JSON(http.StatusBadRequest, api.NewError("invalid request body", api.CodeBadRequest))
 		return
 	}
+	if body.IsAvailable == nil {
+		c.JSON(http.StatusBadRequest, api.NewValidationError([]api.FieldError{
+			{Field: "is_available", Message: "is_available is required"},
+		}))
+		return
+	}
 
 	result, err := s.trainers.q.ToggleTrainerAvailability(c.Request.Context(), db.ToggleTrainerAvailabilityParams{
 		ID:          trainerID,
-		IsAvailable: body.IsAvailable,
+		IsAvailable: *body.IsAvailable,
 	})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/routes/trainers.go` around lines 871 - 883, The PATCH handler
currently uses a plain bool in the request body so a missing is_available binds
as false; change the anonymous body struct's IsAvailable to *bool, keep using
c.ShouldBindJSON(&body), then validate that body.IsAvailable != nil and return a
400 (with the same logging/message) if nil; when calling
trainers.q.ToggleTrainerAvailability use the dereferenced value for IsAvailable
(e.g., *body.IsAvailable) so the update only runs when the field was provided.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Make the trainer/user profile patch atomic.

UpdateTrainer commits before UpdateTrainerUserProfile runs. If the phone-number write fails, this endpoint returns 500 after already persisting the trainer-row edits, so the client sees a failure even though part of the patch landed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/routes/trainers.go` around lines 976 - 1001, The two separate DB
calls (s.trainers.q.UpdateTrainer and s.trainers.q.UpdateTrainerUserProfile)
must be executed inside a single transaction so the whole profile patch is
atomic: open a transaction via your DB/query layer (e.g. start tx or use the
generated q.WithTx / Store.BeginTx helper), run the UpdateTrainer call using the
tx-scoped queries, then if body.PhoneNumber != nil run UpdateTrainerUserProfile
also on the same tx; on any error rollback and return the 500, otherwise commit
the tx and return success. Ensure you use the tx-scoped query methods
corresponding to UpdateTrainer and UpdateTrainerUserProfile and reference
trainerID/updated.UserID as before.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate phone_number with the same E.164 rule used on create.

CreateTrainer rejects invalid numbers via trainerPhoneE164Regex, but this self-service patch writes any trimmed string straight into UpdateTrainerUserProfile. That lets invalid phone data bypass the existing contract.

Suggested fix

 	if body.PhoneNumber != nil {
 		phoneVal := strings.TrimSpace(*body.PhoneNumber)
+		if phoneVal != "" && !trainerPhoneE164Regex.MatchString(phoneVal) {
+			c.JSON(http.StatusBadRequest, api.NewError("phone_number must be in E.164 format (e.g. +2348012345678)", api.CodeBadRequest))
+			return
+		}
 		if _, err := s.trainers.q.UpdateTrainerUserProfile(ctx, db.UpdateTrainerUserProfileParams{
 			ID:          updated.UserID,
 			PhoneNumber: phoneVal,
 		}); err != nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/routes/trainers.go` around lines 993 - 998, The patch writes any
trimmed phone string to UpdateTrainerUserProfile without validating, so add the
same E.164 validation used in CreateTrainer: after computing phoneVal from
body.PhoneNumber (in the block that calls
s.trainers.q.UpdateTrainerUserProfile), test phoneVal against
trainerPhoneE164Regex and if it does not match return a 400/validation error
(consistent with CreateTrainer's behavior) instead of calling
UpdateTrainerUserProfile; only call s.trainers.q.UpdateTrainerUserProfile when
the regex check passes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Down migration will fail if messenger bookings exist.

The down migration drops the messenger_handle column and restores the old session_platform constraint allowing only whatsapp, google_meet, and zoom. However, if any bookings with session_platform = 'messenger' exist in production (created after this migration runs), they will violate the restored constraint and cause the rollback to fail.

The up migration correctly remaps legacy whatsapp rows to zoom (line 10-11) before applying the new constraint. The down migration should similarly remap messenger rows to a valid value before restoring the old constraint.

🔄 Proposed fix to remap messenger rows before restoring constraint

 ALTER TABLE bookings DROP COLUMN IF EXISTS messenger_handle;
 ALTER TABLE bookings DROP CONSTRAINT IF EXISTS bookings_session_platform_check;
+
+-- Remap messenger rows to zoom before applying the old constraint
+UPDATE bookings SET session_platform = 'zoom'
+WHERE session_platform = 'messenger';
+
 ALTER TABLE bookings
     ADD CONSTRAINT bookings_session_platform_check
     CHECK (session_platform IN ('whatsapp', 'google_meet', 'zoom'));

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	ALTER TABLE bookings DROP COLUMN IF EXISTS messenger_handle;
	ALTER TABLE bookings DROP CONSTRAINT IF EXISTS bookings_session_platform_check;
	ALTER TABLE bookings
	ADD CONSTRAINT bookings_session_platform_check
	CHECK (session_platform IN ('whatsapp', 'google_meet', 'zoom'));
	ALTER TABLE bookings DROP COLUMN IF EXISTS messenger_handle;
	ALTER TABLE bookings DROP CONSTRAINT IF EXISTS bookings_session_platform_check;
	-- Remap messenger rows to zoom before applying the old constraint
	UPDATE bookings SET session_platform = 'zoom'
	WHERE session_platform = 'messenger';
	ALTER TABLE bookings
	ADD CONSTRAINT bookings_session_platform_check
	CHECK (session_platform IN ('whatsapp', 'google_meet', 'zoom'));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@migrations/000058_extend_booking_platforms_messenger_meet.sql` around lines
44 - 48, Before re-adding the old check constraint in the down migration, update
any rows where bookings.session_platform = 'messenger' to a permitted value
(e.g., 'whatsapp' or 'zoom') so they won't violate the restored constraint;
specifically, add an UPDATE on the bookings table to remap 'messenger' values
prior to executing ALTER TABLE bookings ADD CONSTRAINT
bookings_session_platform_check CHECK (session_platform IN ('whatsapp',
'google_meet', 'zoom')), and then drop the messenger_handle column/constraint as
currently written.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Release mutex during token refresh HTTP call to prevent lock contention.

The mutex is acquired at line 95 and held through the entire refresh call (line 104), which performs a network request with a 10-second timeout. Concurrent requests for AccessToken will serialize and wait, potentially causing cascading delays. Consider double-checked locking: check expiry, release lock, refresh, re-acquire lock, verify again before storing.

♻️ Suggested refactoring pattern

 func (c *OAuthClient) AccessToken(ctx context.Context) (string, error) {
 	if !c.IsConfigured() {
 		return "", errors.New("googlemeet: OAuthClient not configured")
 	}
 
 	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	// 60s skew so an in-flight request can't race the clock and expire
-	// mid-call (same buffer as the Zoom oauth client uses).
 	if c.accessToken != "" && time.Now().Before(c.expiresAt.Add(-60*time.Second)) {
-		return c.accessToken, nil
+		tok := c.accessToken
+		c.mu.Unlock()
+		return tok, nil
 	}
+	c.mu.Unlock()
 
+	// Refresh without holding lock
 	tok, exp, err := c.refresh(ctx)
 	if err != nil {
 		return "", err
 	}
+	
+	c.mu.Lock()
+	// Double-check: another goroutine may have refreshed while we were unlocked
+	if c.accessToken != "" && time.Now().Before(c.expiresAt.Add(-60*time.Second)) {
+		tok := c.accessToken
+		c.mu.Unlock()
+		return tok, nil
+	}
 	c.accessToken = tok
 	c.expiresAt = exp
+	c.mu.Unlock()
 	return tok, nil
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/googlemeet/oauth.go` around lines 90 - 111, The AccessToken method
currently holds OAuthClient.mu across the network call to refresh(), causing
serialized callers; change to a double-checked locking pattern in AccessToken:
acquire mu and check accessToken/expiresAt, if expired then release mu, call
c.refresh(ctx) (no lock), then re-acquire mu and re-check accessToken/expiresAt
in case another goroutine refreshed in the meantime; only then assign
c.accessToken and c.expiresAt (or return the already-updated token) and release
mu. Ensure you still return errors from refresh and preserve the 60s skew logic
used when checking expiry.