Google Analytics integration#223
Conversation
WalkthroughImplements a Google Analytics 4 integration: client-side OAuth UI and flows, server-side OAuth handlers and session coordination, DB schema and Vault-backed token storage with migrations, DB API surface and mocks, and dashboard wiring and environment/workflow config. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant API as Backend API
participant Google as Google OAuth
participant DB as Database
participant Vault as Vault
Browser->>API: POST /google/oauth/initiate
API->>Google: Redirect to Google auth endpoint
Google->>Browser: Consent screen / Redirect back with code
Browser->>API: GET /google/oauth/callback?code=...
API->>Google: Exchange code for tokens
Google-->>API: Access + Refresh token
API->>Google: Fetch user info & GA4 accounts/properties
Google-->>API: Accounts & properties
API->>DB: Persist pending session (accounts/properties)
API-->>Browser: Redirect with session context
Browser->>API: POST /google/properties/save (selected property IDs + session)
API->>API: Validate pending session
API->>Vault: store_ga_token(connection_id, refresh_token)
Vault-->>API: secret name
API->>DB: Create/Update google_analytics_connections (vault_secret_name, status)
DB-->>API: OK
API-->>Browser: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Updates to Preview Branch (feat/google-analytics-v2) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
🏷️ Release PreviewWhen this PR is merged, it will automatically create:
The changelog will be updated and committed automatically. 💡 Tip: Update the changelog heading to control release type:
|
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/api/auth_google.go:
- Around line 467-472: The current save logic only stores Google refresh tokens
when status == "active", so properties activated later lack tokens; either
remove the status check in the save flow to always call
h.DB.StoreGoogleToken(r.Context(), conn.ID, session.RefreshToken) when
session.RefreshToken != "" (referencing conn.ID, session.RefreshToken and
h.DB.StoreGoogleToken) or add token persistence to the activation path inside
UpdateGooglePropertyStatus so that when a property is switched to "active" you
retrieve the stored session/refresh token (or the OAuth response available at
creation) and call h.DB.StoreGoogleToken for that conn.ID; update only one of
these places (creation or status update) to ensure every active property ends up
with a stored token.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/api/auth_google.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Write in British (Australian) English, not American English
All code, comments, and strings must use Australian spelling as enforced by themisspelllinter withlocale: AU
Files:
internal/api/auth_google.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use structured logging withzerologincluding contextual fields (job ID, domain, request ID) rather than string concatenation in Go code
Never log secrets, Supabase credentials, JWTs, or end-user content in Go code
UseDebuglog level only in development for noisy instrumentation; keep behindLOG_LEVEL=debugso production stays quiet in Go code
UseInfolog level for expected state changes that help operators follow the happy path in Go code, including job creation, worker pool startup, and external webhooks
UseWarnlog level sparingly for unexpected or degraded behaviour that was recovered from, and always include next action or retry detail in Go code
UseErrorlog level for failures that cannot be automatically corrected; always attach errors via.Err(err)in Go code
Ensure API handlers log therequest_id(injected by middleware) so support can correlate client reports to backend events in Go code
Log once at the boundary that handles errors; downstream callers should usefmt.Errorf("context: %w", err)rather than double logging in Go code
Prefer one high-quality log per meaningful event over streaming repetitive messages; avoid logging inside tight loops in Go code
Capture only high-severity or security-relevant issues in Sentry; do not spam Sentry with transient warnings already handled by retries in Go code
Functions in Go must not exceed a cyclomatic complexity of 35; refactor using Extract + Test + Commit pattern for violations
Always rungo fmt ./...before committing to avoid formatting failures in CI
Active linters includegovet,staticcheck,errcheck,revive,ineffassign,gocyclo, andmisspellin Go code
Usegofmtandgoimportsformatters on Go code before committing
Apply Extract + Test + Commit pattern when refactoring Go functions exceeding 50 lines: analyse structure, extract focused functions, create comprehensive tests, commit each step separately, verify integration
Do not add schema management log...
Files:
internal/api/auth_google.go
🧠 Learnings (7)
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Use structured logging with `zerolog` including contextual fields (job ID, domain, request ID) rather than string concatenation in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Ensure API handlers log the `request_id` (injected by middleware) so support can correlate client reports to backend events in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Log once at the boundary that handles errors; downstream callers should use `fmt.Errorf("context: %w", err)` rather than double logging in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Prefer one high-quality log per meaningful event over streaming repetitive messages; avoid logging inside tight loops in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Use `Debug` log level only in development for noisy instrumentation; keep behind `LOG_LEVEL=debug` so production stays quiet in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Use `Warn` log level sparingly for unexpected or degraded behaviour that was recovered from, and always include next action or retry detail in Go code
Applied to files:
internal/api/auth_google.go
📚 Learning: 2025-12-24T00:38:38.149Z
Learnt from: CR
Repo: Harvey-AU/blue-banded-bee PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T00:38:38.149Z
Learning: Applies to **/*.go : Never log secrets, Supabase credentials, JWTs, or end-user content in Go code
Applied to files:
internal/api/auth_google.go
🧬 Code graph analysis (1)
internal/api/auth_google.go (3)
internal/api/errors.go (4)
MethodNotAllowed(144-146)Unauthorised(129-131)BadRequest(124-126)NotFound(139-141)internal/api/response.go (2)
WriteSuccess(35-46)WriteNoContent(63-65)internal/db/google_connections.go (2)
GoogleAnalyticsConnection(20-34)ErrGoogleConnectionNotFound(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Integration Tests
- GitHub Check: Unit Tests
- GitHub Check: Supabase Preview
- GitHub Check: Deploy Review App
- GitHub Check: Supabase Preview
🔇 Additional comments (1)
internal/api/auth_google.go (1)
926-1006: LGTM!The
listGoogleConnectionsanddeleteGoogleConnectionhandlers follow best practices with proper request-scoped logging, authentication checks, and error handling.
| // Store token only for active properties (saves vault space) | ||
| if status == "active" && session.RefreshToken != "" { | ||
| if err := h.DB.StoreGoogleToken(r.Context(), conn.ID, session.RefreshToken); err != nil { | ||
| logger.Warn().Err(err).Str("connection_id", conn.ID).Msg("Failed to store token in vault") | ||
| } | ||
| } |
There was a problem hiding this comment.
Inactive properties won't have tokens when activated later.
Tokens are only stored for properties marked as active during initial save. If a user later activates an inactive property via UpdateGooglePropertyStatus, no token will be available for API calls.
Consider storing the token for all properties, or implementing token storage in UpdateGooglePropertyStatus when activating.
🤖 Prompt for AI Agents
In @internal/api/auth_google.go around lines 467 - 472, The current save logic
only stores Google refresh tokens when status == "active", so properties
activated later lack tokens; either remove the status check in the save flow to
always call h.DB.StoreGoogleToken(r.Context(), conn.ID, session.RefreshToken)
when session.RefreshToken != "" (referencing conn.ID, session.RefreshToken and
h.DB.StoreGoogleToken) or add token persistence to the activation path inside
UpdateGooglePropertyStatus so that when a property is switched to "active" you
retrieve the stored session/refresh token (or the OAuth response available at
creation) and call h.DB.StoreGoogleToken for that conn.ID; update only one of
these places (creation or status update) to ensure every active property ends up
with a stored token.
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/api/auth_google.go`:
- Around line 673-676: The current error logging reads and writes the entire
resp.Body into logs; change this to cap the amount read and avoid logging the
body contents. In the error branch where resp is checked (the code using
resp.StatusCode, io.ReadAll(resp.Body) and log.Error().Str("body", ...)),
replace the unrestricted ReadAll with a bounded read (e.g.,
io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes))) and remove the body string
from the log; instead log only the body length (len(body)) and a flag if the
body hit the cap (truncated). Keep the same log key names (e.g., "status") and
update the log.Error call to include the length/truncated indicator rather than
the raw body.
♻️ Duplicate comments (8)
internal/api/auth_google.go (8)
22-27: Pending sessions stored in process memory won’t survive multi‑instance OAuth callbacks.
If the callback lands on a different instance, the session lookup will fail and users see “Session expired or not found”. Use a shared session store or signed/encrypted cookie sessions, or document the single‑instance limitation and confirm deployment topology.
42-53: Avoid spawning a cleanup goroutine on every session store.
Each call launches a new goroutine; under load this can accumulate and waste memory. Start a single ticker viasync.Once(and wire it into shutdown when available).🧹 Proposed fix
var ( pendingGASessions = make(map[string]*PendingGASession) pendingGASessionsMu sync.RWMutex + gaCleanupOnce sync.Once ) @@ - // Cleanup old sessions in background - go cleanupExpiredGASessions() + // Cleanup old sessions in background (single worker) + gaCleanupOnce.Do(func() { + go func() { + ticker := time.NewTicker(5 * time.Minute) + defer ticker.Stop() + for range ticker.C { + cleanupExpiredGASessions() + } + }() + })
319-336: Do not accept OAuth tokens or Google user info from the client.
Client‑supplied tokens can be exfiltrated or spoofed; usesession_idand pull tokens/user info from the server‑side pending session.🔒 Proposed fix
var req struct { + SessionID string `json:"session_id"` PropertyID string `json:"property_id"` PropertyName string `json:"property_name"` - RefreshToken string `json:"refresh_token"` - AccessToken string `json:"access_token"` - GoogleEmail string `json:"google_email"` - GoogleUserID string `json:"google_user_id"` } @@ - if req.PropertyID == "" || req.RefreshToken == "" { - BadRequest(w, r, "Property ID and refresh token are required") + if req.SessionID == "" || req.PropertyID == "" { + BadRequest(w, r, "Session ID and property ID are required") return } + + session := getPendingGASession(req.SessionID) + if session == nil || session.RefreshToken == "" { + BadRequest(w, r, "Session expired or missing refresh token. Please reconnect to Google Analytics.") + return + } @@ - GoogleUserID: req.GoogleUserID, - GoogleEmail: req.GoogleEmail, + GoogleUserID: session.UserID, + GoogleEmail: session.Email, @@ - if err := h.DB.StoreGoogleToken(r.Context(), conn.ID, req.RefreshToken); err != nil { + if err := h.DB.StoreGoogleToken(r.Context(), conn.ID, session.RefreshToken); err != nil { logger.Error().Err(err).Msg("Failed to store refresh token in vault") InternalError(w, r, err) return }
467-472: Inactive properties later activated will be missing refresh tokens.
Storing tokens only for “active” means a later activation has no token to use. Persist tokens for all properties up‑front or store them when activating.✅ Minimal fix (store token for all properties)
- // Store token only for active properties (saves vault space) - if status == "active" && session.RefreshToken != "" { + // Store token for all properties so later activation can succeed + if session.RefreshToken != "" { if err := h.DB.StoreGoogleToken(r.Context(), conn.ID, session.RefreshToken); err != nil { logger.Warn().Err(err).Str("connection_id", conn.ID).Msg("Failed to store token in vault") } }
655-660: Use a request‑scoped logger so GA4 property fetch logs include request_id.
The package‑level logger drops request correlation; pass the request logger intofetchPropertiesForAccountand use it for Info/Error logs. Based on learnings, includerequest_idin handler logs.♻️ Example refactor
import ( "context" @@ "github.com/google/uuid" + "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) @@ -func (h *Handler) fetchPropertiesForAccount(ctx context.Context, client *http.Client, accessToken, accountName string) ([]GA4Property, error) { +func (h *Handler) fetchPropertiesForAccount(ctx context.Context, client *http.Client, accessToken, accountName string, logger *zerolog.Logger) ([]GA4Property, error) { // URL-encode the filter value (accountName contains slash like "accounts/123456") apiURL := fmt.Sprintf("https://analyticsadmin.googleapis.com/v1beta/properties?filter=parent:%s", url.QueryEscape(accountName)) - log.Info().Str("account_name", accountName).Str("api_url", apiURL).Msg("Fetching GA4 properties for account") + logger.Info().Str("account_name", accountName).Str("api_url", apiURL).Msg("Fetching GA4 properties for account") @@ - log.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed") + logger.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed") @@ - log.Info().Str("account_name", accountName).Int("property_count", len(properties)).Msg("Fetched GA4 properties") + logger.Info().Str("account_name", accountName).Int("property_count", len(properties)).Msg("Fetched GA4 properties") return properties, nil } @@ - properties, err := h.fetchPropertiesForAccount(r.Context(), &http.Client{Timeout: 30 * time.Second}, tokenResp.AccessToken, accounts[0].AccountID) + properties, err := h.fetchPropertiesForAccount(r.Context(), &http.Client{Timeout: 30 * time.Second}, tokenResp.AccessToken, accounts[0].AccountID, logger) @@ - properties, err := h.fetchPropertiesForAccount(r.Context(), client, session.AccessToken, accountID) + properties, err := h.fetchPropertiesForAccount(r.Context(), client, session.AccessToken, accountID, logger)Also applies to: 701-701
771-810: Remove noisy [GA Debug] path‑parsing logs and avoid logging session IDs.
These Info‑level traces leak internal routing details and session identifiers. Remove them or downgrade to Debug behindLOG_LEVEL=debugusing the request logger. As per coding guidelines, keep debug logging behindLOG_LEVEL=debugand avoid sensitive data.
856-864: Never return access/refresh tokens to the browser.
The pending‑session response exposes high‑value secrets and is vulnerable to XSS. Return only the data the UI needs (e.g., accounts/properties andsession_id).🔒 Proposed fix
WriteSuccess(w, r, map[string]interface{}{ "accounts": session.Accounts, "properties": session.Properties, "state": session.State, "user_id": session.UserID, "email": session.Email, - "access_token": session.AccessToken, - "refresh_token": session.RefreshToken, + "session_id": sessionID, }, "")
871-895: Consolidate verbose GA debug logging infetchAccountProperties.
Per‑account Info logs will be noisy and include session/account IDs. Keep a single Debug log (request‑scoped) or remove the traces. Based on learnings, prefer minimal, request‑correlated logging.
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| log.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed") | ||
| return nil, fmt.Errorf("properties endpoint returned status: %d", resp.StatusCode) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cd /repo && cat -n internal/api/auth_google.go | sed -n '665,685p'Repository: Harvey-AU/blue-banded-bee
Length of output: 124
🏁 Script executed:
# Also check what this endpoint does and if there are similar patterns
rg -A 5 -B 5 "Google API properties" internal/api/auth_google.goRepository: Harvey-AU/blue-banded-bee
Length of output: 428
🏁 Script executed:
# Check if the response body could contain user data - look at function context
cat -n internal/api/auth_google.go | sed -n '650,700p'Repository: Harvey-AU/blue-banded-bee
Length of output: 2313
🏁 Script executed:
# Search for other places in the file where response bodies are logged
rg "io\.ReadAll.*resp\.Body" internal/api/auth_google.go -A 3 -B 1Repository: Harvey-AU/blue-banded-bee
Length of output: 345
Remove uncapped response body from error logs.
External API error responses should not be logged in full. Whilst Google's error responses are unlikely to contain direct PII, logging unbounded external data violates the coding guideline to avoid logging end-user content. Cap the read size and log only the body length instead.
🛡️ Safer logging
- body, _ := io.ReadAll(resp.Body)
- log.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed")
+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
+ log.Error().Int("status", resp.StatusCode).Int("body_bytes", len(body)).Msg("Google API properties request failed")
return nil, fmt.Errorf("properties endpoint returned status: %d", resp.StatusCode)📝 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.
| if resp.StatusCode != http.StatusOK { | |
| body, _ := io.ReadAll(resp.Body) | |
| log.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed") | |
| return nil, fmt.Errorf("properties endpoint returned status: %d", resp.StatusCode) | |
| if resp.StatusCode != http.StatusOK { | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) | |
| log.Error().Int("status", resp.StatusCode).Int("body_bytes", len(body)).Msg("Google API properties request failed") | |
| return nil, fmt.Errorf("properties endpoint returned status: %d", resp.StatusCode) |
🤖 Prompt for AI Agents
In `@internal/api/auth_google.go` around lines 673 - 676, The current error
logging reads and writes the entire resp.Body into logs; change this to cap the
amount read and avoid logging the body contents. In the error branch where resp
is checked (the code using resp.StatusCode, io.ReadAll(resp.Body) and
log.Error().Str("body", ...)), replace the unrestricted ReadAll with a bounded
read (e.g., io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes))) and remove the
body string from the log; instead log only the body length (len(body)) and a
flag if the body hit the cap (truncated). Keep the same log key names (e.g.,
"status") and update the log.Error call to include the length/truncated
indicator rather than the raw body.
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
|
We have accounts (only the acount you select) and properties (All for an account) being connected, then the 'active' status is only set on ones the user picks. There is The plan is to close this PR when that's done, then start a new PR for using these connections to do stuff. (Namely prioritising page crawls based on traffic, and broken links sorted by page views to help prioritise.) |
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@dashboard.html`:
- Around line 3249-3252: Update the inline comment above the Google integration
call to use Australian spelling: change "Initialize Google Analytics
integration" to "Initialise Google Analytics integration" where the comment
precedes the invocation of window.setupGoogleIntegration(); (search for the
comment near the window.setupGoogleIntegration call to locate it).
- Around line 2990-2995: Update the heading and explanatory paragraph text to
indicate multi-select support: change the h4 text "Select a GA4 Property" (the
heading element) to something like "Select GA4 Properties" or "Select one or
more GA4 Properties" and update the paragraph node (the p element with color
`#6b7280`) from "Multiple properties found. Please select which one to connect:"
to wording that explicitly permits multiple selections such as "Multiple
properties found. Please select one or more properties to connect:" so users
understand they can activate more than one property.
- Around line 2899-2916: The template is missing the status element that
loadGoogleConnections() populates: add a span with class "google-status" near
the other Google fields (e.g., next to .google-email or under
.google-connected-date) so the Active/Inactive text can be rendered; ensure the
span uses a clear class name "google-status" so the existing JS can find it, and
optionally add minimal inline styling or a CSS class to match the surrounding
styles.
- Around line 2919-2932: The checkbox with class "google-status-toggle" is
currently hidden with display:none which removes it from the accessibility tree;
replace that with a visually-hidden approach (e.g., CSS that keeps it in the
document flow and focusable: position:absolute; width:1px; height:1px;
margin:-1px; overflow:hidden; clip:rect(0 0 0 0); border:0) and remove
style="display: none". Ensure the input has an accessible name by adding an
aria-label (or aria-labelledby pointing to a visible label text) on the input
(e.g., aria-label="Google status") and keep it inside the interactive
"google-toggle-container" label so keyboard users can toggle it; verify class
"google-toggle-container" contains a visible toggle indicator that reflects the
input's checked state for sighted users.
♻️ Duplicate comments (2)
web/static/js/bb-google.js (2)
320-367: Gate or remove GA debug logging before release.There are multiple
console.logcalls that expose OAuth flow internals. Please wrap them behind a debug flag or remove them.♻️ Example approach
+const DEBUG_GA = false; ... - console.log("[GA Debug] Session ID:", pendingGASessionData.session_id); + if (DEBUG_GA) { + console.log("[GA Debug] Session ID:", pendingGASessionData.session_id); + }Also applies to: 660-672, 742-746, 881-958
766-773: Replace the innerHTML template with DOM nodes.Even with a numeric count, building the UI via DOM APIs keeps the pattern consistent and avoids accidental injection later.
♻️ Suggested refactor
- accountUI.innerHTML = ` - <h3 style="margin: 0 0 8px 0; font-size: 16px; font-weight: 600;">Select Google Analytics Account</h3> - <p style="color: `#6b7280`; font-size: 14px; margin: 0 0 16px 0;"> - You have access to ${accounts.length} accounts. Select one to view its properties. - </p> - <div id="googleAccountList"></div> - `; + accountUI.textContent = ""; + const heading = document.createElement("h3"); + heading.style.cssText = "margin: 0 0 8px 0; font-size: 16px; font-weight: 600;"; + heading.textContent = "Select Google Analytics Account"; + accountUI.appendChild(heading); + + const para = document.createElement("p"); + para.style.cssText = "color: `#6b7280`; font-size: 14px; margin: 0 0 16px 0;"; + para.textContent = `You have access to ${accounts.length} accounts. Select one to view its properties.`; + accountUI.appendChild(para); + + const list = document.createElement("div"); + list.id = "googleAccountList"; + accountUI.appendChild(list);
| <div> | ||
| <strong class="google-name" style="font-size: 16px" | ||
| >Google Analytics Connection</strong | ||
| > | ||
| <span | ||
| class="google-email" | ||
| style="color: #6b7280; font-size: 13px; margin-left: 8px" | ||
| ></span> | ||
| <span | ||
| class="google-connected-date" | ||
| style=" | ||
| color: #6b7280; | ||
| font-size: 13px; | ||
| display: block; | ||
| margin-top: 4px; | ||
| " | ||
| ></span> | ||
| </div> |
There was a problem hiding this comment.
Add a visible status label for GA connections.
loadGoogleConnections() sets .google-status, but the template doesn’t include that element, so “Active/Inactive” never appears. Add a status span (and optionally style it) so the status is actually rendered.
🛠️ Suggested update
<span
class="google-connected-date"
style="
color: `#6b7280`;
font-size: 13px;
display: block;
margin-top: 4px;
"
></span>
+ <span
+ class="google-status"
+ style="
+ display: inline-block;
+ font-size: 12px;
+ padding: 2px 8px;
+ border-radius: 4px;
+ background: `#f3f4f6`;
+ color: `#374151`;
+ margin-top: 6px;
+ "
+ ></span>🤖 Prompt for AI Agents
In `@dashboard.html` around lines 2899 - 2916, The template is missing the status
element that loadGoogleConnections() populates: add a span with class
"google-status" near the other Google fields (e.g., next to .google-email or
under .google-connected-date) so the Active/Inactive text can be rendered;
ensure the span uses a clear class name "google-status" so the existing JS can
find it, and optionally add minimal inline styling or a CSS class to match the
surrounding styles.
| <label | ||
| class="google-toggle-container" | ||
| style=" | ||
| display: inline-flex; | ||
| align-items: center; | ||
| cursor: pointer; | ||
| user-select: none; | ||
| " | ||
| > | ||
| <input | ||
| type="checkbox" | ||
| class="google-status-toggle" | ||
| style="display: none" | ||
| /> |
There was a problem hiding this comment.
Make the status toggle keyboard/screen‑reader accessible.
The checkbox is display: none, which removes it from the accessibility tree and keyboard focus. Use a visually hidden style and add an ARIA label so users can toggle it.
♿ Suggested fix
- <input
- type="checkbox"
- class="google-status-toggle"
- style="display: none"
- />
+ <input
+ type="checkbox"
+ class="google-status-toggle"
+ aria-label="Toggle Google Analytics connection status"
+ style="
+ position: absolute;
+ opacity: 0;
+ width: 1px;
+ height: 1px;
+ "
+ />📝 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.
| <label | |
| class="google-toggle-container" | |
| style=" | |
| display: inline-flex; | |
| align-items: center; | |
| cursor: pointer; | |
| user-select: none; | |
| " | |
| > | |
| <input | |
| type="checkbox" | |
| class="google-status-toggle" | |
| style="display: none" | |
| /> | |
| <label | |
| class="google-toggle-container" | |
| style=" | |
| display: inline-flex; | |
| align-items: center; | |
| cursor: pointer; | |
| user-select: none; | |
| " | |
| > | |
| <input | |
| type="checkbox" | |
| class="google-status-toggle" | |
| aria-label="Toggle Google Analytics connection status" | |
| style=" | |
| position: absolute; | |
| opacity: 0; | |
| width: 1px; | |
| height: 1px; | |
| " | |
| /> |
🤖 Prompt for AI Agents
In `@dashboard.html` around lines 2919 - 2932, The checkbox with class
"google-status-toggle" is currently hidden with display:none which removes it
from the accessibility tree; replace that with a visually-hidden approach (e.g.,
CSS that keeps it in the document flow and focusable: position:absolute;
width:1px; height:1px; margin:-1px; overflow:hidden; clip:rect(0 0 0 0);
border:0) and remove style="display: none". Ensure the input has an accessible
name by adding an aria-label (or aria-labelledby pointing to a visible label
text) on the input (e.g., aria-label="Google status") and keep it inside the
interactive "google-toggle-container" label so keyboard users can toggle it;
verify class "google-toggle-container" contains a visible toggle indicator that
reflects the input's checked state for sighted users.
| <h4 style="margin: 0 0 12px 0; font-size: 14px; color: #374151"> | ||
| Select a GA4 Property | ||
| </h4> | ||
| <p style="color: #6b7280; font-size: 13px; margin-bottom: 12px"> | ||
| Multiple properties found. Please select which one to connect: | ||
| </p> |
There was a problem hiding this comment.
Copy implies single selection, but UI supports multiple toggles.
Update the wording to reflect multi‑select so users understand they can activate more than one property.
✏️ Suggested copy
- <h4 style="margin: 0 0 12px 0; font-size: 14px; color: `#374151`">
- Select a GA4 Property
- </h4>
- <p style="color: `#6b7280`; font-size: 13px; margin-bottom: 12px">
- Multiple properties found. Please select which one to connect:
- </p>
+ <h4 style="margin: 0 0 12px 0; font-size: 14px; color: `#374151`">
+ Select GA4 properties
+ </h4>
+ <p style="color: `#6b7280`; font-size: 13px; margin-bottom: 12px">
+ Multiple properties found. Select one or more to activate:
+ </p>📝 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.
| <h4 style="margin: 0 0 12px 0; font-size: 14px; color: #374151"> | |
| Select a GA4 Property | |
| </h4> | |
| <p style="color: #6b7280; font-size: 13px; margin-bottom: 12px"> | |
| Multiple properties found. Please select which one to connect: | |
| </p> | |
| <h4 style="margin: 0 0 12px 0; font-size: 14px; color: `#374151`"> | |
| Select GA4 properties | |
| </h4> | |
| <p style="color: `#6b7280`; font-size: 13px; margin-bottom: 12px"> | |
| Multiple properties found. Select one or more to activate: | |
| </p> |
🤖 Prompt for AI Agents
In `@dashboard.html` around lines 2990 - 2995, Update the heading and explanatory
paragraph text to indicate multi-select support: change the h4 text "Select a
GA4 Property" (the heading element) to something like "Select GA4 Properties" or
"Select one or more GA4 Properties" and update the paragraph node (the p element
with color `#6b7280`) from "Multiple properties found. Please select which one to
connect:" to wording that explicitly permits multiple selections such as
"Multiple properties found. Please select one or more properties to connect:" so
users understand they can activate more than one property.
| // Initialize Google Analytics integration | ||
| if (window.setupGoogleIntegration) { | ||
| window.setupGoogleIntegration(); | ||
| } |
There was a problem hiding this comment.
Use Australian spelling in the new comment.
The comment uses “Initialize”; please change to “Initialise” to match the AU spelling rule. As per coding guidelines, ...
✏️ Suggested fix
- // Initialize Google Analytics integration
+ // Initialise Google Analytics integration📝 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.
| // Initialize Google Analytics integration | |
| if (window.setupGoogleIntegration) { | |
| window.setupGoogleIntegration(); | |
| } | |
| // Initialise Google Analytics integration | |
| if (window.setupGoogleIntegration) { | |
| window.setupGoogleIntegration(); | |
| } |
🤖 Prompt for AI Agents
In `@dashboard.html` around lines 3249 - 3252, Update the inline comment above the
Google integration call to use Australian spelling: change "Initialize Google
Analytics integration" to "Initialise Google Analytics integration" where the
comment precedes the invocation of window.setupGoogleIntegration(); (search for
the comment near the window.setupGoogleIntegration call to locate it).
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
67-72: Replace invalid input parameter with correct one.The input
skip-config-verificationdoes not exist ingolangci/golangci-lint-action@v7. The correct input to control configuration verification isverify, which should be set tofalseto skip config validation.🔧 Proposed fix
- name: golangci-lint uses: golangci/golangci-lint-action@v7 with: version: v2.5.0 args: --config .golangci.yml --modules-download-mode=mod - skip-config-verification: true + verify: false
This reverts commit 87539a0.
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/static/js/bb-google.js`:
- Around line 577-580: The checkbox created as toggleInput (className
"property-status-toggle") is hidden with display: none which removes it from the
accessibility tree; change the approach to make it visually hidden but still
accessible (e.g., add a "visually-hidden" CSS class or set inline styles like
position: absolute; width:1px; height:1px; margin:-1px; overflow:hidden;
clip:rect(0 0 0 0); white-space:nowrap; border:0) instead of display:none, and
add an aria-label (e.g., toggleInput.setAttribute("aria-label", "Toggle property
status")) so screen readers can identify it; ensure the visible control updates
the checkbox state and keep the element focusable for keyboard users.
- Around line 468-513: Remove or gate the ad-hoc debug console statements inside
toggleConnectionStatus: eliminate the console.log calls that print "[GA Toggle]
Toggling ...", the PATCH request trace, the "Status updated successfully"
message, and the console.error usages that report API or general errors; instead
either remove them entirely for production or wrap them behind a configurable
debug flag (e.g., a global DEBUG or isDev check) so logs only emit when
debugging is enabled, while keeping error handling behavior (showGoogleError,
loadGoogleConnections) and existing fetch/auth logic intact; reference function
toggleConnectionStatus and helper functions showGoogleError and
loadGoogleConnections when making the change.
♻️ Duplicate comments (10)
dashboard.html (4)
2919-2944: Missing.google-statuselement in template.The JavaScript code in
bb-google.js(lines 183-189) attempts to populate.google-status, but the HTML template lacks this element. The status will never be displayed.
2956-2960: Make the status toggle keyboard/screen-reader accessible.The checkbox uses
display: none, which removes it from the accessibility tree and keyboard focus. Use a visually hidden approach withposition: absolute; opacity: 0; width: 1px; height: 1px;and add anaria-labelattribute.
3018-3023: Copy implies single selection, but UI supports multiple toggles.The heading says "Select a GA4 Property" (singular) and the paragraph says "Please select which one to connect". Update the wording to reflect multi-select capability, e.g. "Select GA4 Properties" and "Select one or more to activate".
3253-3259: Use Australian spelling in the comment.The comment uses "Initialize"; please change to "Initialise" to match the AU spelling rule. As per coding guidelines, all code and comments must use Australian spelling.
Suggested fix
- // Initialize Google Analytics integration + // Initialise Google Analytics integrationweb/static/js/bb-google.js (6)
321-382: Remove or gate debug logging statements before production.This function contains numerous
console.logstatements with "[GA Debug]" prefix (lines 322, 332, 338-342, 353, 359, 363, 368) that expose internal OAuth session state to anyone with browser dev tools open.
669-686: Debug logging statements persist in showPropertySelection.Lines 670-674 and 678-681 contain
console.logstatements that should be removed or gated behind a debug flag before production deployment.
750-755: Debug logging in showAccountSelection.Lines 751-755 contain debug logging that should be removed or gated before production deployment.
776-782: Potential XSS via innerHTML assignment.Line 776 uses a template literal with
innerHTMLassignment. Whileaccounts.lengthis numeric and safe, this pattern is risky if extended. Consider using DOM manipulation methods for consistency with the rest of the file (e.g.,renderPropertyList).
878-879: Global mutable state for sensitive data.
pendingGASessionDatastores OAuth session information in a module-level variable. Consider adding defensive cleanup on page unload or after a timeout to reduce exposure window.
890-894: Extensive debug logging in OAuth callback handler.The
handleGoogleOAuthCallbackfunction contains numerousconsole.logstatements (lines 890-894, 906, 911, 917, 925, 933-939, 947, 958, 962-966, 970) that expose OAuth flow internals. Remove or gate these before production.
| async function toggleConnectionStatus(connectionId, active) { | ||
| console.log( | ||
| `[GA Toggle] Toggling ${connectionId} to ${active ? "active" : "inactive"}` | ||
| ); | ||
|
|
||
| try { | ||
| const { data: { session } = {} } = await window.supabase.auth.getSession(); | ||
| const token = session?.access_token; | ||
| if (!token) { | ||
| console.error("[GA Toggle] No auth token available"); | ||
| showGoogleError("Not authenticated. Please sign in."); | ||
| return; | ||
| } | ||
|
|
||
| console.log( | ||
| `[GA Toggle] Making PATCH request to /v1/integrations/google/${connectionId}/status` | ||
| ); | ||
| const response = await fetch( | ||
| `/v1/integrations/google/${encodeURIComponent(connectionId)}/status`, | ||
| { | ||
| method: "PATCH", | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| status: active ? "active" : "inactive", | ||
| }), | ||
| } | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| const text = await response.text(); | ||
| console.error(`[GA Toggle] API error: ${response.status}`, text); | ||
| throw new Error(text || `HTTP ${response.status}`); | ||
| } | ||
|
|
||
| console.log("[GA Toggle] Status updated successfully"); | ||
| // Reload to update UI | ||
| loadGoogleConnections(); | ||
| } catch (error) { | ||
| console.error("[GA Toggle] Failed to toggle connection status:", error); | ||
| showGoogleError("Failed to update status"); | ||
| loadGoogleConnections(); // Reload to reset toggle state | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove debug logging in toggleConnectionStatus.
Lines 469-471, 482-484, 501, and 505 contain console.log and console.error statements with "[GA Toggle]" prefix that should be removed or gated behind a debug flag before production deployment.
Suggested approach
async function toggleConnectionStatus(connectionId, active) {
- console.log(
- `[GA Toggle] Toggling ${connectionId} to ${active ? "active" : "inactive"}`
- );
-
try {
const { data: { session } = {} } = await window.supabase.auth.getSession();
const token = session?.access_token;
if (!token) {
- console.error("[GA Toggle] No auth token available");
showGoogleError("Not authenticated. Please sign in.");
return;
}
- console.log(
- `[GA Toggle] Making PATCH request to /v1/integrations/google/${connectionId}/status`
- );
const response = await fetch(🤖 Prompt for AI Agents
In `@web/static/js/bb-google.js` around lines 468 - 513, Remove or gate the ad-hoc
debug console statements inside toggleConnectionStatus: eliminate the
console.log calls that print "[GA Toggle] Toggling ...", the PATCH request
trace, the "Status updated successfully" message, and the console.error usages
that report API or general errors; instead either remove them entirely for
production or wrap them behind a configurable debug flag (e.g., a global DEBUG
or isDev check) so logs only emit when debugging is enabled, while keeping error
handling behavior (showGoogleError, loadGoogleConnections) and existing
fetch/auth logic intact; reference function toggleConnectionStatus and helper
functions showGoogleError and loadGoogleConnections when making the change.
| const toggleInput = document.createElement("input"); | ||
| toggleInput.type = "checkbox"; | ||
| toggleInput.className = "property-status-toggle"; | ||
| toggleInput.style.display = "none"; |
There was a problem hiding this comment.
Hidden checkbox lacks accessibility attributes.
The dynamically created checkbox for property toggles uses display: none, removing it from the accessibility tree. This mirrors the same issue flagged in the HTML template. Apply a visually hidden approach and add an aria-label.
Suggested fix
const toggleInput = document.createElement("input");
toggleInput.type = "checkbox";
toggleInput.className = "property-status-toggle";
- toggleInput.style.display = "none";
+ toggleInput.style.cssText = "position: absolute; opacity: 0; width: 1px; height: 1px;";
+ toggleInput.setAttribute("aria-label", `Toggle ${prop.display_name} active status`);
toggleInput.setAttribute("data-property-id", prop.property_id);📝 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.
| const toggleInput = document.createElement("input"); | |
| toggleInput.type = "checkbox"; | |
| toggleInput.className = "property-status-toggle"; | |
| toggleInput.style.display = "none"; | |
| const toggleInput = document.createElement("input"); | |
| toggleInput.type = "checkbox"; | |
| toggleInput.className = "property-status-toggle"; | |
| toggleInput.style.cssText = "position: absolute; opacity: 0; width: 1px; height: 1px;"; | |
| toggleInput.setAttribute("aria-label", `Toggle ${prop.display_name} active status`); | |
| toggleInput.setAttribute("data-property-id", prop.property_id); |
🤖 Prompt for AI Agents
In `@web/static/js/bb-google.js` around lines 577 - 580, The checkbox created as
toggleInput (className "property-status-toggle") is hidden with display: none
which removes it from the accessibility tree; change the approach to make it
visually hidden but still accessible (e.g., add a "visually-hidden" CSS class or
set inline styles like position: absolute; width:1px; height:1px; margin:-1px;
overflow:hidden; clip:rect(0 0 0 0); white-space:nowrap; border:0) instead of
display:none, and add an aria-label (e.g.,
toggleInput.setAttribute("aria-label", "Toggle property status")) so screen
readers can identify it; ensure the visible control updates the checkbox state
and keep the element focusable for keyboard users.
- Remove OAuth tokens from API response (getPendingSession) - Document single-instance session storage limitation - Handle rows.Close() error in ListGoogleConnections - Add changelog separator for merge conflict prevention
|
🐝 Review App Deployed! Homepage: https://blue-banded-bee-pr-223.fly.dev 📋 Test Features:
🗄️ Database: Supabase preview branch |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/db/google_connections.go`:
- Around line 131-157: Extract the repeated NULL-to-field assignment logic into
a method on *GoogleAnalyticsConnection (e.g., applyNullableFields) and call it
from both the current scan/population site and ListGoogleConnections to remove
duplication; the helper should accept the sql.NullString / sql.NullTime
parameters (ga4PropertyID, ga4PropertyName, googleAccountID, googleUserID,
googleEmail, vaultSecretName, installingUserID, status, lastSyncedAt) and set
the corresponding struct fields (GA4PropertyID, GA4PropertyName,
GoogleAccountID, GoogleUserID, GoogleEmail, VaultSecretName, InstallingUserID,
Status, LastSyncedAt) when Valid is true. Ensure the new method is defined on
GoogleAnalyticsConnection and replace the duplicated if-blocks in both locations
with a single call to that method.
♻️ Duplicate comments (6)
internal/api/auth_google.go (6)
57-58: Goroutine accumulation risk on every session store.Spawning
cleanupExpiredGASessions()on every call can accumulate goroutines under load. Consider usingsync.Oncewith a background ticker.♻️ Suggested fix using sync.Once
+var cleanupOnce sync.Once + func storePendingGASession(session *PendingGASession) string { sessionID := uuid.New().String() session.ExpiresAt = time.Now().Add(10 * time.Minute) pendingGASessionsMu.Lock() pendingGASessions[sessionID] = session pendingGASessionsMu.Unlock() - // Cleanup old sessions in background - go cleanupExpiredGASessions() + // Start cleanup ticker once + cleanupOnce.Do(func() { + go func() { + ticker := time.NewTicker(5 * time.Minute) + for range ticker.C { + cleanupExpiredGASessions() + } + }() + }) return sessionID }
325-342: Security: OAuth tokens accepted directly from client request body.
SaveGooglePropertyacceptsrefresh_tokenandaccess_tokenfrom the client, exposing them to XSS attacks. TheSaveGooglePropertiesendpoint correctly uses onlysession_idand retrieves tokens server-side—this endpoint should follow the same pattern.🔒 Suggested fix
var req struct { + SessionID string `json:"session_id"` PropertyID string `json:"property_id"` PropertyName string `json:"property_name"` - RefreshToken string `json:"refresh_token"` - AccessToken string `json:"access_token"` - GoogleEmail string `json:"google_email"` - GoogleUserID string `json:"google_user_id"` }Then retrieve tokens from the server-side session using
getPendingGASession(req.SessionID).
473-478: Inactive properties won't have tokens when activated later.Tokens are only stored for properties marked as active during initial save. If a user later activates an inactive property via
UpdateGooglePropertyStatus, no token will be available for API calls.Consider storing the token for all properties, or implementing token storage in
UpdateGooglePropertyStatuswhen activating.
679-682: Cap response body read and remove from error logs.The full response body from Google's API is read without limit and logged. Per coding guidelines, avoid logging end-user content. Cap the read and log only the body length.
🛡️ Safer logging
if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - log.Error().Int("status", resp.StatusCode).Str("body", string(body)).Msg("Google API properties request failed") + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) + log.Error().Int("status", resp.StatusCode).Int("body_bytes", len(body)).Msg("Google API properties request failed") return nil, fmt.Errorf("properties endpoint returned status: %d", resp.StatusCode) }
898-934: Remove debug logging from fetchAccountProperties.This function contains numerous
log.Info()statements with "[GA Debug]" prefix that should be removed or changed toDebuglevel. The per-account comparison loop logging (lines 916-927) is particularly verbose.
778-837: Remove or reduce debug logging before production.Multiple
log.Info()calls with "[GA Debug]" prefix expose internal routing details and don't use the request logger (missingrequest_idcorrelation). Per coding guidelines, useDebuglevel for noisy instrumentation and keep behindLOG_LEVEL=debug.♻️ Suggested approach
Either remove these debug statements entirely, or:
- Change
log.Info()tolog.Debug()- Use the request logger instead of package-level
log- Remove the "[GA Debug]" prefix
- log.Info(). + logger := loggerWithRequest(r) + logger.Debug(). Str("raw_path", path). Str("session_path", sessionPath). Strs("parts", parts). Int("parts_len", len(parts)). - Msg("[GA Debug] Parsing pending-session path") + Msg("Parsing pending-session path")
| if ga4PropertyID.Valid { | ||
| conn.GA4PropertyID = ga4PropertyID.String | ||
| } | ||
| if ga4PropertyName.Valid { | ||
| conn.GA4PropertyName = ga4PropertyName.String | ||
| } | ||
| if googleAccountID.Valid { | ||
| conn.GoogleAccountID = googleAccountID.String | ||
| } | ||
| if googleUserID.Valid { | ||
| conn.GoogleUserID = googleUserID.String | ||
| } | ||
| if googleEmail.Valid { | ||
| conn.GoogleEmail = googleEmail.String | ||
| } | ||
| if vaultSecretName.Valid { | ||
| conn.VaultSecretName = vaultSecretName.String | ||
| } | ||
| if installingUserID.Valid { | ||
| conn.InstallingUserID = installingUserID.String | ||
| } | ||
| if status.Valid { | ||
| conn.Status = status.String | ||
| } | ||
| if lastSyncedAt.Valid { | ||
| conn.LastSyncedAt = lastSyncedAt.Time | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting a helper to reduce NULL handling duplication.
The same NULL-to-field assignment pattern is repeated here and in ListGoogleConnections (lines 200-226). A small helper method on *GoogleAnalyticsConnection could reduce this duplication.
♻️ Optional refactor
// applyNullableFields sets fields from scanned nullable values
func (conn *GoogleAnalyticsConnection) applyNullableFields(
ga4PropertyID, ga4PropertyName, googleAccountID, googleUserID,
googleEmail, vaultSecretName, installingUserID, status sql.NullString,
lastSyncedAt sql.NullTime,
) {
if ga4PropertyID.Valid {
conn.GA4PropertyID = ga4PropertyID.String
}
if ga4PropertyName.Valid {
conn.GA4PropertyName = ga4PropertyName.String
}
// ... remaining fields
}🤖 Prompt for AI Agents
In `@internal/db/google_connections.go` around lines 131 - 157, Extract the
repeated NULL-to-field assignment logic into a method on
*GoogleAnalyticsConnection (e.g., applyNullableFields) and call it from both the
current scan/population site and ListGoogleConnections to remove duplication;
the helper should accept the sql.NullString / sql.NullTime parameters
(ga4PropertyID, ga4PropertyName, googleAccountID, googleUserID, googleEmail,
vaultSecretName, installingUserID, status, lastSyncedAt) and set the
corresponding struct fields (GA4PropertyID, GA4PropertyName, GoogleAccountID,
GoogleUserID, GoogleEmail, VaultSecretName, InstallingUserID, Status,
LastSyncedAt) when Valid is true. Ensure the new method is defined on
GoogleAnalyticsConnection and replace the duplicated if-blocks in both locations
with a single call to that method.
Google Analytics integration
2 participants
Summary
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.