You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The preceding source comment still says this link has “no click handler at all,” but this new analytics handler makes that documentation false. Update it to explain that the handler is observational and does not prevent default, matching the regression test below.
Addressed Copilot’s previously missed summary finding from the latest review. The finding was valid: the People-tab source comment still said the person link had no click handler after this PR added an observational analytics handler. I enumerated the same stale statement in the PR code and corrected both the component comment and the corresponding test-suite comment in 8a72c16. I did not change the handler or navigation behavior: the existing test continues to verify that the handler records analytics while Next owns navigation. I also did not add a source-text or mutation test for a comment-only correction, because that would test prose rather than product behavior and would not follow this repository’s testing patterns. There was no inline thread attached to this summary-only finding, so this PR comment records the decision and resolution.
This is the same keepLooking action that HubQueryState renders at lines 2168-2172. That path uses analyticsSurface="rematch" and emits Debate rematch Keep looking, while this non-empty path emits Rematch keep looking, splitting one action into two labels. Use the shared rematch surface here too so attribution stays stable across empty and non-empty states.
Use popup-specific attribution for shared request overflow menu
RequestOverflowMenu is also rendered by the app-wide IncomingRequestPopup (core/debates/debate-coordinator.tsx:323), not only by hub cards. Using the hub-only helper here makes the popup trigger and actions emit Debate hub ... / hub intents even when the hub is closed. Thread an owning surface through this shared menu and give the popup its own non-hub attribution.
Skip attribution for pending personal-space author IDs
Optimistic comments created before a personal space exists use pending:${walletAddr} as author.spaceId (use-create-comment.tsx:171-185). Clicking that author now records the synthetic pending value as a personal_space entity/profile ID, so graph_relationship_followed contains neither a Person nor personal-space ID. Skip profile attribution until the pending author is replaced with a real space ID.
Addressed all three previously missed findings from Copilot’s latest summary in c731224. Copilot did not create inline threads for these items, so this comment records each decision and resolution.
Keep looking attribution: valid. The empty and non-empty controls invoke the same rematch action but had different labels. The non-empty button now uses the shared rematch surface, producing Debate rematch Keep looking and debate_rematch_action just like the empty-state path. I enumerated production rematch action overrides; no other one-off production label remains. Forcing this call back to the hub surface makes the new regression fail.
Request overflow surface: valid misattribution, but the suggested implication that the intents were hub-specific was not correct. open_debate_request_options and manage_debate_request describe the actions without naming a surface, so renaming them would split an established event family without benefit. I kept those intents and added a shared debate-action surface helper. The hub card now explicitly uses the hub surface and the app-wide popup explicitly uses the request-popup surface, so their stable labels are distinct. Forcing the popup back to hub makes the new trigger/action regression fail.
Pending comment author ID: valid. Rather than patching only one comment link, personProfileOpened now rejects the repository’s canonical pending personal-space sentinel at the shared analytics boundary. This covers all three comment-author links and any other current or future profile-open caller without changing navigation to the intentionally navigable pending page. Removing the guard makes the new analytics regression emit graph_relationship_followed with pending:0x123 and fail.
No visual, navigation, or mutation behavior changed, and no out-of-scope issues were found. Final focused result: 235 tests pass; type-check, lint, formatting, and diff checks pass.
The reason will be displayed to describe this comment to others. Learn more.
Read every non-test hunk across the 66 files. No correctness issues found. The riskier spots all hold up:
use-edit-profile.ts:settleSuccess(recordPublishedEdit = true) has only two callers. The no-op path passes false and the makeProposalonSuccess calls it bare, so no callback leaks an unexpected argument into it. The only other published_edit sender is review-changes.tsx, so a profile publish isn't counted twice.
Email-capture sign-in fallback: the card stays mounted behind Privy's modal (hidden, because isModalOpen counts as an overlay while wantsAccount is true). Dismissing the modal reaches giveUpRef.current() through the new guarded onError in usePrivySignIn. The navbar's PrivyConnectButton now only tracks logins it started, so there's no double-counted sign-in.
usePrivySignInonError: the early return behaves the same as the old unconditional clear, and the caller's callback only fires for a sign-in this hook started.
editComment wrapper: keeps the underlying Promise<boolean> and records the edit only when the save succeeds.
Wiring:
content_edited, signup_completed and graph_relationship_followed are registered in public/geo-analytics-*.js.
SmallButton, Button and PrefetchLink pass data-* and onClick through.
The rematch page passes analyticsSurface="rematch" to every shared control, so no hub labels leak into it.
One optional note, not a bug: the generic entity-page-body.tsx:121 still renders <CommentSection> without targetEntityType, so comments posted on a debate's full entity page get tagged entity, not debate. The PR only promises debate and claim context through the shared panel, so this may be fine as is. Worth a one-line prop if you want debate comments attributed wherever they're posted.
Thanks for the thorough review. I confirmed the main conclusion: I did not find a correctness issue in the paths you called out, and there were no inline threads to address.
I did take the optional comment-attribution note. The normal debate browse view already passes targetEntityType="debate", but the generic footer is also the edit/fallback and side-panel path, where a Debate was still falling back to entity. In 68dce2bc9, the footer now derives the logical comment target from the entity data it already reads. I checked the same class rather than patching only Debate: Claim and Topic custom comment views retain their explicit types, and the shared derivation recognizes Claim, Topic, and Debate whenever one reaches the generic footer.
The regression was written first and failed against the previous behavior. I also deliberately replaced the derivation with the old entity fallback and confirmed the test failed before restoring the fix. This changes only analytics payload context; it does not change rendering, navigation, or publishing.
Rebasing across current master exposed one related new caller that landed after this PR's prior base: the profile Debates tab now uses the shared FilterSwitch, whose owning analytics surface this PR intentionally made required. In a02dcfcfe, that caller now uses a distinct profile-debates surface (Profile debates Show hidden / filter_profile_debates) rather than being mislabeled as hub or rematch activity. Its regression also failed before the new surface existed.
Validation after the rebase: 601 broad focused tests and 25 post-fix tests pass; typecheck, lint, formatting, and diff checks pass. No device-visible behavior changed.
Three conflicts in `rematch-page-client.tsx`, all against the debate-rooms
work (#2543) and the analytics attribution (#2522):
- `sessionEnded` and master's `requestSpaceRef` landed in the same place and are
independent. Both kept.
- `HideMyPositionsSwitch` keeps master's `analyticsSurface`, on this branch's
condition: Positions is a tab now, so `tab === 'explore'` already excludes it
and master's `source !== 'mine'` is dead.
- The source menu keeps master's `analytics` prop and this branch's label
narrowing, which has to agree with the `value` beside it.
One thing the merge broke that neither side got wrong on its own: in a debate
room an expired session is rejoined in place rather than redirected away, so
`sessionEnded` stopped meaning "about to navigate" — and the pair header would
have frozen, permanently, over a session that was coming back. The room case is
excepted, with tests both ways. Converted is not: that redirects out of a room
as much as into one.
Master's new analytics test needed a curated page to have a source menu to
attribute at all, for the same reason the switch lost its source check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ohohoreilly
left a comment
Summary
Audit results
signup_completedonly after the API succeedssigned_up/signed_in, attributed toexplore_email_capturecomment_createdafter successful publish, with entity type, surface, space, and parent IDcontent_editedafter successful publishvote_castpath covers curation, stance, veracity, clearing and switching, and indexed outcomesvote_castoperation path covers submitted and indexed outcomespublished_editonly after a real successful profile publication; no-op saves are excludedgraph_relationship_followedwith personal-space or person IDs and originating surfaceValidation
bun run testbefore the Copilot-review fixes: 572 files, 6,840 tests passedmaster: 10 files, 252 tests passedbun run typecheckinapps/webgit diff --check