feat: DAH-4291 invite-to logging improvement: add teardown beacon, env snapshot, structured events

[fwextensions]

Description

Adds diagnostic logging for the invite-to bot detection shipped in #2987: a pagehide beacon so a human click in an email in-app webview isn't lost when the page is torn down before the interaction/dwell gate, a passive browser-env snapshot on every signal, and one structured invite_to.response {json} log line per event that names why a response was suppressed. No applicant-facing behavior change — recording still happens server-side on GET, and the shadow endpoint still records nothing to Salesforce.

Jira ticket

https://sfgovdt.jira.com/browse/DAH-4291

Review instructions

Applies to any environment with the temp.webapp.inviteToClientRecording Unleash flag enabled (local or a review app). Flag off = no new client behavior, server logging only.

Mint a link (rails console):

t = JsonWebTokenService.encode_token(
  type: 'I2I', deadline: 30.days.from_now.to_date.to_s, act: 'yes', appId: '<APP_ID>'
)

1. Open /listings/<LISTING_ID>/next-steps?t=<t>, move the mouse, and confirm one log line
   invite_to.response {"...","outcome":"recorded","source":"get","ok":true,...} plus a second
   with "source":"client_shadow","trigger":"interaction" and an env object.
2. Reload and close the tab immediately, before touching anything. Expect
   "trigger":"teardown" — this is the case the PR exists to capture.
3. Load the page in a background tab and close it without ever viewing it. Expect no
   client_shadow line (a hidden unload is not evidence of a human).
4. Re-mint with deadline: 1.day.ago.to_date.to_s and load it. Expect
   "outcome":"suppressed","reason":"deadline_passed" with deadline_date / today / late_by.
5. Switch language on the page. Expect "reason":"language_change" with a referrer that has
   no ?t= token in it.
6. Confirm the request_id on all lines from one page load matches.

PA testing does not apply: there is no user-visible change. Both branches of the flag are exercised by the specs.

Before requesting eng review

Version Control

• branch name begins with angular if it contains updates to Angular code
• branch name contains the Jira ticket number
• PR name follows type: TICKET-NUMBER Description format, use DAH-000 if it does not need a ticket
• PR name follows urgent: Description format if it is urgent and does not need a ticket

Code quality

• the set of changes is small
• all automated code checks pass (linting, tests, coverage, etc.)
• if the PR is a bugfix, there are tests and logs around the bug

Code conventions

• web pages are formatted with .scss stylesheets and ui-seeds tokens, rather than inline styles or Tailwind

Review instructions

• instructions specify which environment(s) it applies to
• instructions work for PA testers
• instructions have already been performed at least once

Request eng review

• PR has needs review label
• Use Housing Eng group to automatically assign reviewers, and/or assign specific engineers
• If time sensitive, notify engineers in Slack

Before merging

Request product acceptance (PA) testing

• PA tested in the review environment (use needs product acceptance label)
• if PA testing cannot be done, changes are behind a feature flag

Details: what each change is for

Client

• Teardown beacon. Once the page arms (paint + visible proven), a pagehide before interaction/dwell sends trigger: "teardown" via navigator.sendBeacon, which survives unload where fetch is cancelled. Confidence ranks interaction > dwell > teardown.
• Disarms on visibilitychange → hidden, so an unload from a hidden page does not report a teardown — indistinguishable from a background prefetch.
• Env snapshot (webdriver, ua, coarse, cores, mem, screen, lang, tz) on every signal. Evidence only, never a gate: fingerprinting is spoofable and a false positive would reject a real applicant. It exists to classify the ambiguous bucket — a teardown with webdriver:false and an Outlook-iOS UA is a confident webview human; one with a headless UA is not.

Server

New InviteToEventLogging concern emits one line per event: invite_to.response {json}, a greppable prefix plus a JSON tail Papertrail can post-process into fields. Values are bounded centrally (256 chars, non-scalars coerced) since these endpoints are unauthenticated, and env is allow-listed to the nine keys the client sends.

Change	Why
reason= names one of no_action / deadline_passed / language_change / test_link	Four causes previously collapsed into one *NOT* recording message
deadline_date / today / late_by	Logs what the code actually compared, in resolved local terms, instead of a UTC timestamp. late_by under a minute is a clock/UX issue, not a late response
referrer on language_change	The one suppression that can silently eat a legitimate first click; scrubbed to scheme/host/path because that referrer carries the invite JWT
request_id on every line	The three log sites correlate directly instead of by timestamp adjacency, which breaks under concurrency
ok=true/false	send_invite_to_response returns nil on both invalid-action and any rescued error, so "recording" previously meant attempted

Deprecated JWT keys (response, applicationNumber) are intentionally omitted from the new payloads.

Two things worth knowing

A bug this caught in test, not review. window.matchMedia?.("…").matches guards the call but still dereferences undefined — it would have thrown in exactly the older/webview environments this feature exists to measure. snapshotEnv is now defensive per-property, so a telemetry read can never throw away the signal it reports.

ok= is weaker than it reads. It's !result.nil?, which separates the rescued/invalid-action path from success, but on the happy path send_invite_to_response returns whatever send_message returns. For ok=true to mean "Salesforce accepted it", send_message needs to return a real success value — a behavior change rather than logging, so it's not in this PR. Follow-up: DAH-XXXX.

Testing

Jest 39/39 and RSpec 57/57 on the touched specs, ESLint and Rubocop clean. New coverage: teardown beacon fired and suppressed-when-hidden, sendBeacon-unavailable fallback, a throwing env property, unparseable and blank deadlines, test_link suppression, each format_duration unit, oversized field truncation, and a referrer assertion that the JWT never reaches the log.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR introduces structured, JSON-based event logging for “invite-to” response flows (server GET recording and client shadow human-verification), and adds client-side environment snapshots plus a teardown beacon signal to improve post-hoc classification.

Changes:

• Added a shared InviteToEventLogging concern and migrated existing invite-to logs to structured invite_to.response {json} lines.
• Enhanced client shadow logging with an environment snapshot (snapshotEnv) and teardown-time delivery via sendBeacon.
• Expanded RSpec/Jest coverage to assert new structured log payloads and teardown behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
spec/controllers/invite_to_controller_spec.rb	Adds expectations for new structured logging and suppression reasons.
spec/controllers/api/v1/invite_to_response_controller_spec.rb	Updates log assertions to structured event format and includes env snapshot checks.
app/javascript/hooks/useAutoRecordInviteToResponse.ts	Attaches env snapshots to logs and sends teardown beacons on pagehide.
app/javascript/api/inviteToApiService.ts	Introduces env snapshot collection and sendBeacon logging API.
app/javascript/tests/pages/invite-to-apply.test.tsx	Updates mocks for new invite-to API exports.
app/javascript/tests/hooks/useAutoRecordInviteToResponse.test.tsx	Adds tests for env payload attachment and teardown beacon behavior.
app/javascript/tests/api/inviteToApiService.test.ts	Adds tests for snapshotEnv and beaconHumanVerifiedClick.
app/controllers/invite_to_controller.rb	Replaces ad-hoc strings with structured logging; adds suppression/deadline helper logic.
app/controllers/concerns/invite_to_event_logging.rb	New concern that emits stable-prefix JSON logs for invite-to events.
app/controllers/api/v1/invite_to_response_controller.rb	Migrates shadow log endpoint to structured logging and logs env snapshot.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

app/controllers/invite_to_controller.rb:106

• Logging the full request.referrer can inadvertently capture query strings/fragments (including tokens/PII, depending on upstream URLs). To reduce exposure, consider logging a scrubbed referrer (e.g., scheme/host/path only, or path + locale segment), or explicitly stripping known sensitive params (like t) before emitting it.

        # The one suppression that can silently eat a legitimate first click; capture what
        # request.referrer actually was so language_change? can be audited.
        referrer: (reason == 'language_change' ? request.referrer : nil),

app/controllers/invite_to_controller.rb:179

• These helper methods are currently public. In Rails controllers, public instance methods can be treated as actions unless hidden, which increases the risk of accidental routing exposure and expands the controller's public surface area. Mark suppression_reason, deadline_terms, and format_duration as private (or move them below an existing private declaration) to keep them as internal helpers.

  # Names the single reason a GET is not recorded instead of collapsing four
  # causes into one branch. Order matches the original `||` precedence: a preview
  # link (act blank) is `no_action`.
  def suppression_reason(invite_action, response, deadline, is_test)
    if invite_action.blank? && response.blank? then 'no_action'
    elsif deadline && deadline_has_passed?(deadline) then 'deadline_passed'
    elsif language_change? then 'language_change'
    elsif is_test then 'test_link'
    end
  end

  # Emits what the code actually compared, in resolved local terms, with a
  # near-miss delta: late_by under an hour is a product conversation, under a
  # minute is likely a clock/UX issue.
  def deadline_terms(deadline, reason)
    return {} unless reason == 'deadline_passed' && deadline.present?

    # Time.zone.parse returns nil (rather than raising) for input it cannot make
    # sense of, e.g. "not-a-date", so nil must be handled explicitly - the rescue
    # below would not catch the resulting NoMethodError.
    deadline_time = Time.zone.parse(deadline)
    return { deadline_raw: deadline } if deadline_time.nil?

    {
      deadline_date: deadline_time.to_date.to_s,
      today: Time.zone.today.to_s,
      late_by: format_duration((Time.zone.now - deadline_time).to_i),
    }
  rescue ArgumentError, TypeError
    { deadline_raw: deadline } # unparseable - surface it rather than crash the log line
  end

  # Compact human duration ("2m", "3h", "5d") without pulling in a gem.
  def format_duration(seconds)
    seconds = seconds.abs
    return "#{seconds}s" if seconds < 60
    return "#{seconds / 60}m" if seconds < 3600
    return "#{seconds / 3600}h" if seconds < 86_400

    "#{seconds / 86_400}d"
  end

app/controllers/invite_to_controller.rb:166

• late_by is computed using a wall-clock Time delta from Time.zone.now to deadline_time. For date-only deadlines (common in this flow), this makes the logged value dependent on time-of-day and can be off by a day around DST transitions (and can make specs like the \"3d\" expectation flaky across DST). If the intent is day-scale reporting for date-only deadlines, compute the delta from Time.zone.today - deadline_time.to_date (calendar days) and format that, while still preserving a seconds/minutes/hours delta only when the input actually includes a time component.

    {
      deadline_date: deadline_time.to_date.to_s,
      today: Time.zone.today.to_s,
      late_by: format_duration((Time.zone.now - deadline_time).to_i),
    }

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

app/controllers/invite_to_controller.rb:149

• Using deadline && deadline_has_passed?(deadline) treats an empty-string deadline as present (truthy), which will now suppress as deadline_passed because deadline_has_passed? returns true on unparseable/nil parses. If blank deadlines should behave like “no deadline provided”, switch this check to deadline.present? && deadline_has_passed?(deadline) to avoid misclassifying empty values.

  def suppression_reason(invite_action, response, deadline, is_test)
    if invite_action.blank? && response.blank? then 'no_action'
    elsif deadline && deadline_has_passed?(deadline) then 'deadline_passed'
    elsif language_change? then 'language_change'
    elsif is_test then 'test_link'
    end
  end

app/javascript/hooks/useAutoRecordInviteToResponse.ts:152

• beaconHumanVerifiedClick returns a boolean indicating whether the browser accepted the beacon, but the result is currently ignored. In browsers/environments without sendBeacon (or if queuing fails), the teardown signal is silently lost; consider checking the return value and adding a fallback (e.g., a best-effort request using fetch(..., { keepalive: true }) if available, or at minimum a debug-level log/metric) so this failure mode is observable.

    const handlePageHide = () => {
      if (fired || !armed) return
      fired = true
      cleanup()
      beaconHumanVerifiedClick(buildRecord("teardown"))
    }

[fwextensions]

This is a local script used for offline processing of logs to analyze the bot detection results.

[jimlin-sfgov]

There appears to be quite a few changes here, and no review instructions to boot. Also, the abundance of multi-line comments with semantic fluff are a slog to get through. I don't see this getting approved and merged quickly, unless reviewers dedicate a good chunk of time to focus on this PR.

I strongly encourage you to strike a better balance of verbosity and clarity, whether that's through tuning your prompts, or editing the LLM output by hand. As a reviewer I want concise human-readable content. Amending and refining LLM output to align with that should be done before a PR is considered ready for review, not during the review process.

🧑 Written by @jimlin-sfgov

[fwextensions]

@jimlin-sfgov shrank the comments by a third and streamlined the PR description. Items 1 and 2 in the review instructions are the more important changes, but the main thing is just making sure the new logging doesn't interfere with the existing server-side response recording. 62% of the PR is tests to get to the 100% qlty threshold.

[amyc-sfds]

If I may, I want to jump in with a question. The associated Jira ticket describes the technical problem and the implementation, but not the impact or risk of leaving it unaddressed—and it is marked Critical in priority. Is this is a "drop what you're doing" situation? If it is, then I think this should be a team conversation rather than a side PR.

I ask because this PR needs a more thorough review. The code is relatively self-contained, but it extends several controllers and introduces new logging logic, so the approach you've taken here should be validated against our existing patterns and architecture. That level of review takes focused time and attention, so it would displace other things currently being worked on.

For next steps, it would be helpful to get alignment on priorities. If moving this forward is critical/time-sensitive, I recommend bringing it to the team and putting it through your standard process. The engineers own the code that goes into prod, and I want them to have the room to look at this properly.

[jimlin-sfgov]

thought: It would be nice to have structured logging throughout our Rails app, instead of building out a little module just to serve the needs of your feature. Options include:

• configure logging behavior in Rails.application.configure do ... config.logger = ...
• use a gem like logstruct
• upgrade to Rails 8.1 which supports structured logging natively

[jimlin-sfgov]

question: So my earlier slack comment about utilizing the navigator object was taken into consideration? Have you also considered the PII implications as well? If we combine this digital fingerprint with listing_id and application_id, does that become a privacy violation?

[jimlin-sfgov]

question: How was this particular set of properties decided on? There are additional ways to get clued in on headless browsers, e.g. navigator.connection.rtt and eval.toString().length

[jimlin-sfgov]

suggestion: Remove from file, this file is for functions that make API calls.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

suggestion: Do not use env, could be confused with environment and environment variables.

[jimlin-sfgov]

There are quite a few review comments. Thoughts on adding a claude.md to steer your LLM work better @fwextensions ?

[jimlin-sfgov]

Another thought: it would be helpful for engineers if you could illustrate what you've been seeing in the logs so far, and how those logs motivated you to make this new PR