Skip to content

feat(telemetry): carry the page through its lifecycle events - #804

Merged
surajshetty3416 merged 2 commits into
frappe:developfrom
surajshetty3416:telemetry-page-lifecycle
Sep 8, 2026
Merged

feat(telemetry): carry the page through its lifecycle events#804
surajshetty3416 merged 2 commits into
frappe:developfrom
surajshetty3416:telemetry-page-lifecycle

Conversation

@surajshetty3416

@surajshetty3416 surajshetty3416 commented Sep 8, 2026

Copy link
Copy Markdown
Member

Refs #800. Covers items 1, 2, 5, the reachable half of 3, and the dead builder_page_unpublished from 7.

What

builder_page_created now carries page, source (template / duplicate / blank / import), template_page and block_count. The two known origins are stamped by their API entry points; anything else with more than the root block came in with content (paste, REST), so it reads as import. The capture moved from before_insert to after_insert: Frappe's naming discards whatever name before_insert saw, so the id the issue expected there was never the real one.

builder_page_published carries page, is_first_publish, seconds_since_created, block_count, has_client_script, has_data_script. First publish is read before publish() sets the flag, and published_at is checked too so an unpublish/republish cycle does not count as a second first.

builder_page_unpublished never fired because the editor unpublishes with a plain set_value on published, never the unpublish doc method. It now fires from on_update on the 1 → 0 transition, whichever path flips it, and carries page.

builder_page_template_used sends page (the new page's name) and builder_template_group_imported sends pages, both values the frontend already had in hand.

New builder_editor_opened event, fired from pageStore.setPage whenever the editor switches to a page other than the last one that loaded, with page, block_count, is_published. Reloads of the same page (restore, revert, unpublish) do not refire; a retry after a failed fetch does. This is the first in-editor trace for the 256 sites whose last event was builder_page_created.

Persona survey answers are attached to the site's Pulse profile through a new builder.api.identify_persona endpoint (builder_role, builder_use_case, builder_source). The existing builder_persona_submitted event stays. Argument types are enforced by frappe.whitelist (Builder sets require_type_annotated_api_methods), and the pulse import is local and guarded because identify only exists on frappe v16+.

Tests in builder/builder/tests/test_telemetry.py cover every property above plus the not-first-publish and not-an-unpublish-on-insert edges.

Left out, and why

  • Item 6 (user= on hook events) changes nothing: the Pulse client already defaults user to frappe.session.user. What it does not do is anonymize Administrator (a standard user), so events published as Administrator carry a user the ingest side apparently drops. Builder cannot fix that from its call sites.
  • Item 4 (failure events) needs a reason taxonomy first; wrapping publish() to emit exception class names felt premature.
  • builder_editor_closed (item 3) is unreliable on tab close and would bias session length, so it is skipped.
  • Item 7: the video and font upload paths are reachable (drop a video or font file on the canvas), so those look like low usage rather than dead code. The schema_version stamp and the dead-event test need Pulse data access, which a repo test does not have.
  • Item 8 renames are left alone to keep dashboard continuity. builder_editor_opened supersedes builder_page_opened, which can be retired once it has data.
  • The source: page.live_url ? "hub" : "local" key on the template events is always hub because every template now comes through the hub; left as is.

Noticed, not touched

set_default_values derives route from a name that naming then throws away, so a page's route hash and its name hash never match. Harmless, but surprising.

Most Builder events recorded that something happened but not to what, so
nothing could be joined across create, publish and template use (frappe#800).

- builder_page_created, builder_page_published and builder_page_unpublished
  now carry the page id, plus source/template_page/block_count on create and
  is_first_publish/seconds_since_created/block_count/has_client_script/
  has_data_script on publish. The created capture moves to after_insert:
  naming discards whatever name before_insert saw.
- builder_page_unpublished never fired: the editor unpublishes with a plain
  field update, not the unpublish method. It now fires from on_update on the
  published 1 -> 0 transition, whichever path flips it.
- builder_page_template_used and builder_template_group_imported send the
  page names they already had in hand.
- New builder_editor_opened event when the editor switches to a page, with
  block_count and is_published, so pages that are created and then abandoned
  leave a trace.
- Persona survey answers are attached to the site's Pulse profile through a
  new identify_persona endpoint, so any metric can be split by persona.

Co-Authored-By: Claude <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The PR appears safe to merge.

Both previous findings are resolved; the retry telemetry issue is fixed, and Frappe boundary validation makes the persona type-check concern inapplicable.

Reviews (3): Last reviewed commit: "fix(telemetry): count a retried editor o..." | Re-trigger Greptile

Comment thread frontend/src/stores/pageStore.ts Outdated
Comment thread builder/api.py
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.55172% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.86%. Comparing base (50c0e0f) to head (cafc096).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
builder/api.py 75.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #804      +/-   ##
===========================================
+ Coverage    66.06%   66.86%   +0.79%     
===========================================
  Files           93       94       +1     
  Lines        10386    10496     +110     
===========================================
+ Hits          6862     7018     +156     
+ Misses        3524     3478      -46     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

setPage moved selectedPage before fetching, so a retry of the same page
after a failed fetch looked like no switch and builder_editor_opened was
dropped. Compare against the last page that actually loaded instead.

Also covers count_blocks on a single legacy root block.

Co-Authored-By: Claude <noreply@anthropic.com>
@surajshetty3416

Copy link
Copy Markdown
Member Author

@greptileai

@surajshetty3416
surajshetty3416 merged commit 51079bd into frappe:develop Sep 8, 2026
11 checks passed
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 1.34.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant