Conversation
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR adds a visitor_email parameter to the rome_app_open GA event so app-open analytics can attribute opens to a caller. The host resolves the caller identity on the manifest fetch (guardian email via a new enrichGuardianActor call, visitor email from the session, guest otherwise), threads it through the caller bootstrap, and the two mount paths (RomeAppHost, AppComponentBlock) pass it to trackAppOpen, which trims and falls back to guest. The implementation is clean, well-tested (guardian-over-visitor precedence, anonymous/invalid-session, per-event isolation, empty-email fallback), and correctly scoped per event so identity cannot leak across opens. The manifest route sets no Cache-Control, so there is no cross-user caching leak.
The dominant concern is not implementation quality but the design decision to send raw email addresses to Google Analytics. Google's Analytics ToS explicitly prohibits uploading PII (email addresses included); doing so risks data deletion or property suspension, and it contradicts the very ADR this PR amends, which went to lengths to keep identity out of what reaches GA. Secondary issues: emails are not lowercase-normalized, which will fragment visitor groups in the admin table, and the guardian branch issues an extra DB query per manifest fetch.
Verdict: REQUEST_CHANGES — The feature ships real user email addresses to Google Analytics/BigQuery, which violates Google's PII policy and undercuts the codebase's own privacy stance; this should be consciously addressed before merge.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security | packages/web/src/lib/analytics.ts |
Raw email addresses are sent to Google Analytics (PII policy violation) |
| P2 | code-quality | packages/web/src/lib/analytics.ts |
visitor_email is not normalized to lowercase, fragmenting visitor groups |
| P3 | performance | packages/core/src/api/routes/apps.ts |
Extra DB query and dead type-narrowing in the guardian caller branch |
| trackEvent("rome_app_open", { | ||
| app_id: appId, | ||
| surface, | ||
| visitor_email: visitorEmail?.trim() || "guest", |
There was a problem hiding this comment.
[P1] security — Raw email addresses are sent to Google Analytics (PII policy violation)
visitor_email carries a real, plaintext email address into GA (and, per docs/observability/app-visits.md, into the BigQuery export). Google's Analytics Terms of Service explicitly prohibit sending PII such as email addresses; GA's automated PII detection can result in data deletion or property suspension. This also directly contradicts the amended ADR (raw-analytics-urls-over-client-side-sanitization.md), which deliberately kept identity out of what reaches GA and stated "Identity ... stay[s] out of scope."
Consider sending a non-reversible pseudonymous identifier instead of the raw email — e.g. a salted hash of the normalized email, or GA4's native user_id / a hashed user property — so the admin table can still group by visitor without shipping PII to Google. At minimum this trade-off should be explicitly acknowledged and signed off by whoever owns the GA property.
| trackEvent("rome_app_open", { | ||
| app_id: appId, | ||
| surface, | ||
| visitor_email: visitorEmail?.trim() || "guest", |
There was a problem hiding this comment.
[P2] code-quality — visitor_email is not normalized to lowercase, fragmenting visitor groups
The value is only trimmed (visitorEmail?.trim() || "guest"), not lowercased. The admin table groups opens by visitor_email, so Owner@Example.com and owner@example.com become distinct visitor groups and inflate the count. The rest of the codebase already lowercases emails for identity comparisons (e.g. viewer.email.toLowerCase() in apps.ts). Normalize here for consistency:
visitor_email: visitorEmail?.trim().toLowerCase() || "guest",| // resolved here, per manifest fetch, and delivered on the bootstrap so the | ||
| // app UI can gate owner-only affordances without probing a route. Advisory | ||
| // only — enforcement stays in the app's API handler. | ||
| const guardianActor = guardianSession |
There was a problem hiding this comment.
[P3] performance — Extra DB query and dead type-narrowing in the guardian caller branch
enrichGuardianActor issues an additional guardian_auth SELECT on every manifest fetch just to read email, after resolveGuardianSession has already run. Since enrichGuardianActor always returns a kind: "guardian" actor, the guardianActor?.kind === "guardian" guard on line 506 is always true and only serves TypeScript narrowing. This is minor, but consider having resolveGuardianSession/the session layer surface the seat email directly (or reusing an already-resolved actor) to avoid the second round-trip per manifest fetch.
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR enriches manifest caller identity for cloud guardians and threads caller emails from embedded, full-page, and inline mounts into each rome_app_open event, with guest as the fallback. The identity precedence and per-event plumbing are coherent, and the tests cover the main identity cases.
The analytics sink is a blocker, however: raw emails cannot safely or reliably serve as GA event dimensions. Local tests and type checks could not be rerun because the provided clone has no installed dependencies.
Verdict: REQUEST_CHANGES — The implementation sends raw email addresses to Google Analytics, where they are either redacted and unusable or collected in violation of GA's PII policy.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security | packages/web/src/lib/analytics.ts |
Do not send visitor emails as GA event parameters |
| trackEvent("rome_app_open", { | ||
| app_id: appId, | ||
| surface, | ||
| visitor_email: visitorEmail?.trim() || "guest", |
There was a problem hiding this comment.
[P1] security — Do not send visitor emails as GA event parameters
visitor_email sends a raw email address as a custom GA event parameter. Google prohibits passing recognizable PII such as email addresses to Analytics, while GA4 email redaction evaluates all event parameters, so this either violates policy when redaction is disabled or collapses visitors to (redacted) when enabled. Keep email attribution in first-party storage and send an opaque visitor/account key to GA—or collect the event through a first-party backend—then join to emails only inside Rome Cloud.
yunfanye
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR enriches app manifests with guardian email identity and threads caller emails through embedded, full-page, and inline app-open events, with tests for precedence and guest handling. The event-local identity flow is otherwise coherent, but the selected GA4 transport cannot reliably or permissibly carry raw emails, and the public app SDK contract was not updated to match the new bootstrap shape.
Verdict: REQUEST_CHANGES — The implementation sends raw email addresses to GA4, violating Analytics policy and conflicting with default email redaction.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security/privacy | packages/web/src/lib/analytics.ts |
Do not send raw email addresses to GA4 |
| P2 | architecture | packages/web/src/components/rome-app-host.tsx |
Keep the public bootstrap contract synchronized |
| trackEvent("rome_app_open", { | ||
| app_id: appId, | ||
| surface, | ||
| visitor_email: visitorEmail?.trim() || "guest", |
There was a problem hiding this comment.
[P1] security/privacy — Do not send raw email addresses to GA4
visitor_email sends a raw email address as a GA4 event parameter. Google prohibits sending emails/PII to Analytics, while GA4 email redaction scans all event parameters and is enabled by default for new properties, so compliant/default properties will export a redacted value instead of the promised attribution. Move identifiable attribution to first-party storage, or send an opaque non-PII identifier and join it to email outside GA.
| /** Host-resolved caller identity for this mount (advisory, UI gating only). */ | ||
| caller?: | ||
| | { kind: "guardian"; userId: string } | ||
| | { kind: "guardian"; userId: string; email?: string } |
There was a problem hiding this comment.
[P2] architecture — Keep the public bootstrap contract synchronized
This changes the runtime bootstrap.caller contract, but the exported RomeAppCaller in packages/app-web-sdk/src/runtime/index.ts still declares guardians without email. Apps therefore receive a field at runtime that supported SDK types reject, while the host maintains another divergent copy of the contract. Update and release the SDK type—ideally centralizing the shape—or keep analytics-only identity outside the app bootstrap.
What this PR does
App-open analytics cannot identify which visitor opened an app. Each
rome_app_openevent now records the authenticated caller email invisitor_email, orguestwhen no email is available.Embedded, full-page, and inline chat mounts use the manifest identity. Cloud guardians include their email, and guardian sessions take precedence over coexisting visitor sessions.
The companion Rome Cloud PR adds expandable visitor counts to the admin App opens table.
Design & Invariants
App visit analytics defines the event and reporting contract. Identity belongs to each event. Anonymous callers and local guardians without an email use
guest. Existing page-view and widget behavior stays intact.Emails apply to new events. Historical events lack visitor attribution and remain in the guest group.
Test plan
pnpm --filter @rome/core test src/api/routes/apps.test.ts— 14 tests, including anonymous, invalid-session, visitor, and guardian precedence cases.pnpm test:unit:web— 1,393 web tests and 559 UI tests passed, including 13 analytics tests.git diff --check.pnpm typecheck— blocked by the existing RslibautoExternaltype mismatch in app-web-sdk.pnpm test:unit— blocked by the incomplete local Electron installation.pnpm dev:alland container startup — Traefik cannot bind the occupied host port 80.pnpm lint:prose— Vale and Nix are unavailable locally. Checks ran with installed Node 24.