feat(analytics): use new analytics events#2105
Merged
Conversation
…k-up-new-analytics-events
…articipants tracking
…k-up-new-analytics-events
… posthog to use analytics to get instance
Contributor
WalkthroughReplaces HOC-based analytics with a hook and a centralized analytics instance ( Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
js/app/packages/app/component/command/CommandMenu.tsx (1)
133-180:⚠️ Potential issue | 🟡 MinorTrack
command_menu_useonly after an action actually fires.Line 136 runs before the
activateCommandScopeIdearly return and before the entityblockNameguard. That records a "use" for submenu navigation and non-openable entity rows, which will overcount this metric.📉 Narrow the tracking to real executions/open actions
function handleItemAction(item: CommandMenuItem, openInNewSplit = false) { if (!item) return; - - analytics.track('command_menu_use', { itemType: item.bucket }); if (isCommandItem(item)) { const command = item.data; @@ CommandState.setCommandScopeCommands(nestedCommands); CommandState.setSelectedIndex(0); return; } + + analytics.track('command_menu_use', { itemType: item.bucket }); // Regular command - close and run CommandState.close(); @@ if (isEntityItem(item)) { const blockName = getBlockNameForEntity(item); if (blockName) { + analytics.track('command_menu_use', { itemType: item.bucket }); openWithSplit( { type: blockName, id: item.id },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/command/CommandMenu.tsx` around lines 133 - 180, The analytics.track('command_menu_use', { itemType: item.bucket }) is called too early in handleItemAction and records submenu navigation or non-openable rows; move the track call so it only fires when an actual action executes: for command items, call analytics.track right before runCommand(...) after confirming command.activateCommandScopeId is falsy; for entity items, call analytics.track only after blockName is non-empty and immediately before calling openWithSplit(...). Ensure you remove the original pre-check analytics.track in handleItemAction so submenu early-returns and non-openable entities are not counted.js/app/packages/app/component/SoupChatInput.tsx (1)
165-167:⚠️ Potential issue | 🟡 MinorRemove debug event listeners.
These global
focusin,focusout, andfocusevent listeners withconsole.logappear to be debugging artifacts that should not be committed to production code.🗑️ Suggested removal
-document.addEventListener('focusin', (f) => console.log('focus changed', f)); -document.addEventListener('focusout', (f) => console.log('focus changed', f)); -document.addEventListener('focus', (f) => console.log('focus changed', f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/SoupChatInput.tsx` around lines 165 - 167, Remove the debug global event listeners in the SoupChatInput component that call console.log for 'focusin', 'focusout', and 'focus' (the document.addEventListener lines); either delete these three document.addEventListener(...) statements or guard them behind a development-only flag and ensure any listeners are cleaned up on unmount, so no console logging or stray global handlers remain in production.js/app/packages/block-md/comments/commentOperations.ts (1)
103-111: 🧹 Nitpick | 🔵 TrivialAnalytics tracked before validation, including early-exit paths.
The
comment_deleteevent is tracked at Line 104 before thecommentId === -1check at Line 108. This means deleting "new" (unsaved) comments will also trigger the analytics event, which may not be the intended behavior. Consider moving the tracking after the early return, or only tracking when actually deleting a persisted comment.♻️ Proposed fix to track only persisted comment deletions
return createCallback(async (info: DeleteCommentInfo) => { - analytics.track('comment_delete', { blockType: 'md' }); editor?.dispatchCommand(DISCARD_DRAFT_COMMENT_COMMAND, undefined); const commentId = info.commentId; if (commentId === -1) { deleteNewComments(); return true; } const comment = comments[commentId]; // this can happen when deleting the thread -> // comment mark deleted -> comment server delete re-attempted if (!comment) return true; const deleteInfo = await deleteComment(commentId, { removeAnchorThreadOnly: info.removeAnchorThreadOnly, }); + if (deleteInfo) { + analytics.track('comment_delete', { blockType: 'md' }); + } + if (deleteInfo?.threadDeleted) { editor?.dispatchCommand(DELETE_COMMENT_COMMAND, [comment.anchorId, true]); } return !!deleteInfo; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-md/comments/commentOperations.ts` around lines 103 - 111, The analytics.track('comment_delete', { blockType: 'md' }) call is executed before validating the delete target (commentId) and thus fires for unsaved comments; move the tracking so it only runs for persisted deletions — either relocate the analytics.track call to after the if (commentId === -1) early-return block or wrap it in a conditional (if (commentId !== -1) ...) inside the createCallback handler that returns DeleteCommentInfo; ensure editor?.dispatchCommand(DISCARD_DRAFT_COMMENT_COMMAND, undefined) and deleteNewComments() behavior remain unchanged.js/app/packages/block-pdf/store/comments/commentOperations.ts (1)
118-123: 🧹 Nitpick | 🔵 TrivialAnalytics tracked regardless of delete outcome.
The
comment_deleteevent is emitted unconditionally afterdeleteCommentis called, even if the operation fails. Consider tracking only on successful deletions for more accurate analytics.♻️ Proposed fix to track only successful deletes
const success = deleteComment(commentId, { removeAnchorThreadOnly: info.removeAnchorThreadOnly, }); - analytics.track('comment_delete', { blockType: 'pdf' }); + if (success) { + analytics.track('comment_delete', { blockType: 'pdf' }); + } return success;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-pdf/store/comments/commentOperations.ts` around lines 118 - 123, The analytics event is emitted unconditionally; change the logic in commentOperations.ts so that analytics.track('comment_delete', { blockType: 'pdf' }) is only called when deleteComment(...) succeeds: check the returned value (the local variable success) and, if deleteComment returns a Promise, await it first and then track only when the resolved value is truthy/indicates success (preserving the removeAnchorThreadOnly option passed to deleteComment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/app-sidebar/sidebar.tsx`:
- Around line 246-249: handleCommandPaletteClick currently always emits
analytics.track before toggling, causing closes to be counted as opens; read the
command menu's pre-toggle state (e.g., CommandState.isOpen or CommandState.open)
and only call analytics.track('command_menu_open', { from: 'sidebar' }) when the
menu is about to open (pre-toggle is false), then call CommandState.toggle()
unconditionally so the UI still toggles.
In `@js/app/packages/app/component/GlobalHotkeys.tsx`:
- Around line 78-84: The analytics call is using the current state from
createMenuOpen(), so it fires when the menu is closing instead of when it opens;
change the logic to compute the next state (e.g., const willOpen =
!createMenuOpen()) and then call analytics.track('create_menu_open', { from:
'global_hotkey' }) only when willOpen is true before calling
setCreateMenuOpen(prev => !prev) so tracking reflects the menu opening.
In `@js/app/packages/app/component/paywall/PaywallComponent.tsx`:
- Around line 55-59: The analytics.track call in PaywallComponent.tsx sends
props.customType and props.errorKey which can be undefined or null; update the
subscription_start payload construction to exclude keys with null/undefined
values (e.g., build a payload object, add type: 'early_member', then
conditionally set customType and errorKey only when props.customType != null and
props.errorKey != null) and pass that filtered payload to analytics.track to
avoid sending null/undefined values.
In `@js/app/packages/app/component/settings/Account.tsx`:
- Around line 354-359: The handleToggle function (and the analytics constant
usage) contains missing semicolons causing style inconsistency; update the
handleToggle implementation by adding semicolons after the
analytics.track('notifications_toggled') statement and after the
props.settings.toggle(!props.settings.isEnabled()) call so that function
analytics, handleToggle, props.settings.toggle, and props.settings.isEnabled
follow the project's semicolon style.
In `@js/app/packages/app/lib/analytics/analytics.ts`:
- Around line 50-56: The current try/catch groups multiple analytics providers
so a failure in one (e.g., initializeGoogleAnalytics or initializeMetaPixel)
prevents PostHog from being called; change the calls to
initializeGoogleAnalytics(), initializeMetaPixel(), and
initializePosthog(posthog) so each is wrapped in its own try/catch and logs
provider-specific errors, and do the same for provider lifecycle methods
(identify and reset) so functions like initializeGoogleAnalytics,
initializeMetaPixel, initializePosthog, posthog.identify, and posthog.reset are
invoked independently and failures in one do not stop the others.
- Around line 155-157: The analytics singleton is being fully disabled in dev by
passing disabled: import.meta.env.DEV to createAnalytics which prevents
posthog.init() and breaks downstream feature-flag checks; instead remove or stop
passing the DEV-based disabled flag so createAnalytics/posthog are initialized
in dev, and implement a dev-only short-circuit that no-ops emission methods
(e.g., wrap or override analytics.track, analytics.identify, analytics.reset to
return early when import.meta.env.DEV) so initialization and analytics.posthog
are available but events are not sent from local builds.
In `@js/app/packages/app/lib/analytics/app-events.ts`:
- Around line 33-37: The analytics event schema entry upload_error currently
exposes upload_error.error: string which encourages sending raw exception text;
change the contract to use a sanitized taxonomy such as upload_error.errorCode?:
string or upload_error.reason?: 'network' | 'auth' | 'validation' | 'timeout'
(and optionally upload_error.sanitizedMessage?: string with strict
length/truncation) and remove/avoid any raw error text, then update all
references/serializers that populate upload_error.error to map exceptions to the
new errorCode/reason values (e.g., in upload handling functions and telemetry
emitters) so no raw exception messages are sent.
- Line 64: Update the analytics payload type for the channel_reaction field:
replace the overly-broad Record<string, unknown> declaration for
channel_reaction with a concrete object shape matching usage (an emoji string
and an action union 'add' | 'remove') so callers like any analytics event
constructors or type references that expect channel_reaction use the stricter
type; locate the channel_reaction property in the events/type definitions
(symbol: channel_reaction) and change its type accordingly to enforce the {
emoji: string; action: 'add' | 'remove' } shape throughout the codebase.
- Around line 39-45: Narrow the event payload types by replacing the broad
string types for the "from" and "itemType" fields in this analytics map: change
the "from" fields on command_menu_open, create_menu_open, split_created (and any
other entries using { from: string }) to the literal union
'soup_view_selection_toolbar' | 'soup_view_entity_action' | 'soup_view' |
'soup_view_entity_actions_menu' | 'sidebar' | 'mobile_dock' | 'global_hotkey';
change command_menu_use.itemType to the existing Bucket union type from
quickAccess; and change mentions_menu_use.itemType to the specific union (use
item.kind or define 'chat' | 'entity' | 'user' | 'date') so command_menu_open,
command_menu_use, create_menu_open, mentions_menu_use, split_created have
precise literal types for compile-time checks.
In `@js/app/packages/app/lib/analytics/posthog.tsx`:
- Around line 13-29: PosthogProvider's factory calls useAnalytics(), so the
AnalyticsContextProvider must be mounted above it; locate where the providers
are composed in the root component (where PosthogProvider currently wraps
AnalyticsContextProvider) and swap their order so AnalyticsContextProvider is
the parent and PosthogProvider is its child; after the change ensure
PosthogProvider still imports the same createAssertedContextProvider factory and
that no other provider ordering is violated.
In `@js/app/packages/block-pdf/component/PageOverlay.tsx`:
- Line 28: The file imports analytics directly which is inconsistent; update
PageOverlay to use the useAnalytics() hook instead of the direct analytics
import: remove the direct import from '@app/lib/analytics', import and call
useAnalytics() at the top of the PageOverlay component (e.g., const analytics =
useAnalytics()), and replace any direct usages of the imported analytics symbol
in PageOverlay with the hook-provided analytics instance.
In `@js/app/packages/block-theme/components/ThemeList.tsx`:
- Line 5: The import for useAnalytics in ThemeList.tsx uses a
relative/non-aliased path ("app/component/analytics-context") while the rest of
the PR expects the alias "@app/component/analytics-context"; update the import
to use the `@app` alias to match other files and avoid duplicate module resolution
(look for the import statement referencing useAnalytics and replace its path
with "@app/component/analytics-context").
In `@js/app/packages/channel/Channel/create-channel-message-actions.ts`:
- Around line 80-84: The hook useAnalytics() is being called inside the factory
function createChannelMessageActions, which violates hook rules; change
createChannelMessageActions to accept an analytics parameter (e.g., add an
analytics field to CreateChannelMessageActionsOptions or an explicit analytics
argument) and remove the internal useAnalytics() call, then update all call
sites to pass analytics: useAnalytics() from top-level components; look for
createChannelMessageActions, CreateChannelMessageActionsOptions, MessageData and
MessageActions to update the signature and usages accordingly.
In `@js/app/packages/core/component/AI/component/input/ChatInput.tsx`:
- Line 1: ChatInput.tsx currently imports the analytics singleton directly from
'@app/lib/analytics' instead of using the context hook; update the file to
import and call useAnalytics() from '@app/component/analytics-context' inside
the ChatInput component (replace the existing analytics import and remove direct
usage), i.e., inside the ChatInput function call const analytics =
useAnalytics() and use that instance wherever analytics was referenced.
In `@js/app/packages/core/component/AI/component/message/EditableChatMessage.tsx`:
- Line 11: This file incorrectly imports analytics directly from
`@app/lib/analytics` (causing an inverted dependency and inconsistency); remove
that direct import and instead obtain analytics via the existing hook used
elsewhere: call useAnalytics() inside the EditableChatMessageInner component (or
its parent in this file) and use the returned analytics object/function the same
way other components do (e.g., Chat.tsx, MentionsMenu.tsx, Top.tsx), ensuring
the import comes from the local hook module (not from packages/app) so core no
longer depends on app.
In `@js/app/packages/core/component/DocumentPropertiesModal.tsx`:
- Around line 42-45: The current sequence calls props.onOpenChange before
performing drawerControl.toggle, so if the caller callback throws the toggle
never runs; change the order to call drawerControl.toggle() first (compute the
new open state via drawerControl.isOpen() after toggling or derive newState =
!drawerControl.isOpen() before calling toggle) and then invoke
props.onOpenChange with the new state, wrapping the callback invocation in a
try/catch to swallow/log errors so caller exceptions cannot block the drawer
toggle; reference drawerControl.toggle, drawerControl.isOpen, and
props.onOpenChange when making this change.
In
`@js/app/packages/core/component/LexicalMarkdown/component/menu/MentionsMenu/MentionsMenu.tsx`:
- Line 51: The MentionsMenu component imports useAnalytics from the app layer
which inverts package dependencies (see the import of useAnalytics in
MentionsMenu.tsx and similar usage in EditableChatMessage.tsx); to fix, stop
importing from the app package — either move the analytics hook into the core
package or introduce an interface/prop to inject analytics into MentionsMenu
(e.g., accept an analytics object/function prop used where useAnalytics() is
currently called), update MentionsMenu and EditableChatMessage to obtain
analytics via the new core-exported hook or via dependency injection, and remove
the cross-package import so core no longer depends on app.
- Around line 302-305: The itemAction function calls the async handler
itemActionHandler without awaiting it, which can lead to actions running after
unmount; update itemAction to be async (e.g., const itemAction = async (item:
MentionItem) => { ... }) and await itemActionHandler(item) after the
analytics.track call, and consider adding a try/catch around the await to
surface/handle errors from itemActionHandler; reference the itemAction and
itemActionHandler symbols so you update the correct function.
In `@js/app/packages/core/component/ReferencesModal.tsx`:
- Around line 44-47: The onClick handler currently invokes props.onOpenChange
(caller-supplied) before drawerControl.toggle, so an exception in the callback
can prevent the drawer from toggling; change the order to call
drawerControl.toggle() first (using drawerControl.isOpen() to compute the new
state if needed) and then invoke props.onOpenChange?.(newState) inside a
try/catch to prevent caller errors from blocking the toggle; update the onClick
block that references props.onOpenChange, drawerControl.toggle, and
drawerControl.isOpen accordingly.
In `@js/app/packages/queries/email/thread.ts`:
- Line 283: The analytics.track('email_message_sent') call currently sends no
payload; update the call to include a payload with contentLength (compute from
the outgoing message body/content string length), attachmentsLength (compute
from the outgoing message's attachments array length), and isThreadReply (true
if the message has a parent/thread id or is being sent into an existing thread).
Locate the analytics.track call in js/app/packages/queries/email/thread.ts (the
line with analytics.track('email_message_sent')) and compute those values from
the message object used to send the email (e.g., message.body or
message.content, message.attachments, and message.threadId/parentId) before
passing them as the event properties.
---
Outside diff comments:
In `@js/app/packages/app/component/command/CommandMenu.tsx`:
- Around line 133-180: The analytics.track('command_menu_use', { itemType:
item.bucket }) is called too early in handleItemAction and records submenu
navigation or non-openable rows; move the track call so it only fires when an
actual action executes: for command items, call analytics.track right before
runCommand(...) after confirming command.activateCommandScopeId is falsy; for
entity items, call analytics.track only after blockName is non-empty and
immediately before calling openWithSplit(...). Ensure you remove the original
pre-check analytics.track in handleItemAction so submenu early-returns and
non-openable entities are not counted.
In `@js/app/packages/app/component/SoupChatInput.tsx`:
- Around line 165-167: Remove the debug global event listeners in the
SoupChatInput component that call console.log for 'focusin', 'focusout', and
'focus' (the document.addEventListener lines); either delete these three
document.addEventListener(...) statements or guard them behind a
development-only flag and ensure any listeners are cleaned up on unmount, so no
console logging or stray global handlers remain in production.
In `@js/app/packages/block-md/comments/commentOperations.ts`:
- Around line 103-111: The analytics.track('comment_delete', { blockType: 'md'
}) call is executed before validating the delete target (commentId) and thus
fires for unsaved comments; move the tracking so it only runs for persisted
deletions — either relocate the analytics.track call to after the if (commentId
=== -1) early-return block or wrap it in a conditional (if (commentId !== -1)
...) inside the createCallback handler that returns DeleteCommentInfo; ensure
editor?.dispatchCommand(DISCARD_DRAFT_COMMENT_COMMAND, undefined) and
deleteNewComments() behavior remain unchanged.
In `@js/app/packages/block-pdf/store/comments/commentOperations.ts`:
- Around line 118-123: The analytics event is emitted unconditionally; change
the logic in commentOperations.ts so that analytics.track('comment_delete', {
blockType: 'pdf' }) is only called when deleteComment(...) succeeds: check the
returned value (the local variable success) and, if deleteComment returns a
Promise, await it first and then track only when the resolved value is
truthy/indicates success (preserving the removeAnchorThreadOnly option passed to
deleteComment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d067d86-5a81-4036-bd9b-542190180749
📒 Files selected for processing (50)
js/app/packages/app/component/GlobalHotkeys.tsxjs/app/packages/app/component/Launcher.tsxjs/app/packages/app/component/Onboarding.tsxjs/app/packages/app/component/SoupChatInput.tsxjs/app/packages/app/component/analytics-context.tsxjs/app/packages/app/component/app-sidebar/sidebar.tsxjs/app/packages/app/component/command/CommandMenu.tsxjs/app/packages/app/component/mobile/MobileDock.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/soup-filters-bar.tsxjs/app/packages/app/component/next-soup/soup-view/soup-entity-actions-menu.tsxjs/app/packages/app/component/next-soup/soup-view/soup-entity-selection-toolbar.tsxjs/app/packages/app/component/next-soup/soup-view/use-soup-view-hotkeys.tsjs/app/packages/app/component/paywall/PaywallComponent.tsxjs/app/packages/app/component/settings/Account.tsxjs/app/packages/app/lib/analytics/analytics.tsjs/app/packages/app/lib/analytics/app-events.tsjs/app/packages/app/lib/analytics/index.tsjs/app/packages/app/lib/analytics/posthog.tsxjs/app/packages/block-canvas/component/TopBar.tsxjs/app/packages/block-channel/component/Top.tsxjs/app/packages/block-channel/hooks/participants.tsxjs/app/packages/block-channel/hooks/reactions.tsxjs/app/packages/block-chat/component/Chat.tsxjs/app/packages/block-code/component/TopBar.tsxjs/app/packages/block-md/comments/commentOperations.tsjs/app/packages/block-md/component/TopBar.tsxjs/app/packages/block-pdf/component/PageOverlay.tsxjs/app/packages/block-pdf/component/TopBar.tsxjs/app/packages/block-pdf/store/comments/commentOperations.tsjs/app/packages/block-theme/components/ThemeList.tsxjs/app/packages/channel/Channel/create-channel-message-actions.tsjs/app/packages/channel/Channel/tests/create-channel-message-actions.test.tsjs/app/packages/core/auth/logout.tsjs/app/packages/core/component/AI/component/DragDrop.tsxjs/app/packages/core/component/AI/component/input/ChatInput.tsxjs/app/packages/core/component/AI/component/input/buildRequest.tsjs/app/packages/core/component/AI/component/message/EditableChatMessage.tsxjs/app/packages/core/component/DocumentPropertiesModal.tsxjs/app/packages/core/component/FileList/itemOperations.tsjs/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/LexicalMarkdown/component/menu/MentionsMenu/MentionsMenu.tsxjs/app/packages/core/component/NotificationsModal.tsxjs/app/packages/core/component/ReferencesModal.tsxjs/app/packages/core/component/TopBar/ShareButton.tsxjs/app/packages/core/constant/featureFlags.tsjs/app/packages/core/util/branchName.tsjs/app/packages/core/util/upload.tsjs/app/packages/queries/channel/message.tsjs/app/packages/queries/email/thread.tsjs/app/packages/service-clients/service-storage/util/upload.ts
Comment on lines
+55
to
+59
| analytics.track('subscription_start', { | ||
| type: 'early_member', | ||
| customType: props.customType, | ||
| errorKey: props.errorKey, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential null values in analytics payload.
The errorKey prop can be null (per line 28: errorKey?: PaywallKey | null), and customType is optional. This means the analytics payload may contain undefined or null values. Consider filtering these out if your analytics backend doesn't handle them well:
Suggested improvement
analytics.track('subscription_start', {
type: 'early_member',
- customType: props.customType,
- errorKey: props.errorKey,
+ ...(props.customType && { customType: props.customType }),
+ ...(props.errorKey && { errorKey: props.errorKey }),
});📝 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.
Suggested change
| analytics.track('subscription_start', { | |
| type: 'early_member', | |
| customType: props.customType, | |
| errorKey: props.errorKey, | |
| }); | |
| analytics.track('subscription_start', { | |
| type: 'early_member', | |
| ...(props.customType && { customType: props.customType }), | |
| ...(props.errorKey && { errorKey: props.errorKey }), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/paywall/PaywallComponent.tsx` around lines 55 -
59, The analytics.track call in PaywallComponent.tsx sends props.customType and
props.errorKey which can be undefined or null; update the subscription_start
payload construction to exclude keys with null/undefined values (e.g., build a
payload object, add type: 'early_member', then conditionally set customType and
errorKey only when props.customType != null and props.errorKey != null) and pass
that filtered payload to analytics.track to avoid sending null/undefined values.
Comment on lines
+354
to
+359
| const analytics = useAnalytics() | ||
|
|
||
| const handleToggle = () => { | ||
| analytics.track('notifications_toggled') | ||
| props.settings.toggle(!props.settings.isEnabled()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style inconsistency: missing semicolons.
The statements on lines 354 and 357 are missing semicolons, which is inconsistent with the rest of the codebase.
✏️ Suggested fix
- const analytics = useAnalytics()
+ const analytics = useAnalytics();
const handleToggle = () => {
- analytics.track('notifications_toggled')
- props.settings.toggle(!props.settings.isEnabled())
+ analytics.track('notifications_toggled');
+ props.settings.toggle(!props.settings.isEnabled());
}📝 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.
Suggested change
| const analytics = useAnalytics() | |
| const handleToggle = () => { | |
| analytics.track('notifications_toggled') | |
| props.settings.toggle(!props.settings.isEnabled()) | |
| } | |
| const analytics = useAnalytics(); | |
| const handleToggle = () => { | |
| analytics.track('notifications_toggled'); | |
| props.settings.toggle(!props.settings.isEnabled()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/settings/Account.tsx` around lines 354 - 359,
The handleToggle function (and the analytics constant usage) contains missing
semicolons causing style inconsistency; update the handleToggle implementation
by adding semicolons after the analytics.track('notifications_toggled')
statement and after the props.settings.toggle(!props.settings.isEnabled()) call
so that function analytics, handleToggle, props.settings.toggle, and
props.settings.isEnabled follow the project's semicolon style.
Comment on lines
50
to
56
| try { | ||
| initializeGoogleAnalytics(); | ||
| initializeMetaPixel(); | ||
| initializePosthog(posthog); | ||
| } catch (e) { | ||
| console.error('[Analytics] Failed to initialize providers:', e); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle each analytics provider independently.
Line 53, Line 114, and Line 126 are all inside try blocks that also wrap GA/Meta calls. If one legacy vendor throws during init, identify, or reset, PostHog never receives that lifecycle call even though it is now the default analytics sink.
Also applies to: 102-117, 123-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/lib/analytics/analytics.ts` around lines 50 - 56, The
current try/catch groups multiple analytics providers so a failure in one (e.g.,
initializeGoogleAnalytics or initializeMetaPixel) prevents PostHog from being
called; change the calls to initializeGoogleAnalytics(), initializeMetaPixel(),
and initializePosthog(posthog) so each is wrapped in its own try/catch and logs
provider-specific errors, and do the same for provider lifecycle methods
(identify and reset) so functions like initializeGoogleAnalytics,
initializeMetaPixel, initializePosthog, posthog.identify, and posthog.reset are
invoked independently and failures in one do not stop the others.
Comment on lines
+42
to
+45
| onClick={() => { | ||
| props.onOpenChange?.(!drawerControl.isOpen()); | ||
| drawerControl.toggle(); | ||
| }} |
Contributor
There was a problem hiding this comment.
Don't let onOpenChange block the drawer toggle.
Line 43 now runs arbitrary caller code before Line 44 performs the actual toggle. If that callback throws, the Properties drawer stops opening/closing entirely.
🛠️ Safer ordering
onClick={() => {
- props.onOpenChange?.(!drawerControl.isOpen());
- drawerControl.toggle();
+ const nextOpen = !drawerControl.isOpen();
+ try {
+ props.onOpenChange?.(nextOpen);
+ } finally {
+ drawerControl.toggle();
+ }
}}📝 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.
Suggested change
| onClick={() => { | |
| props.onOpenChange?.(!drawerControl.isOpen()); | |
| drawerControl.toggle(); | |
| }} | |
| onClick={() => { | |
| const nextOpen = !drawerControl.isOpen(); | |
| try { | |
| props.onOpenChange?.(nextOpen); | |
| } finally { | |
| drawerControl.toggle(); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/DocumentPropertiesModal.tsx` around lines 42 -
45, The current sequence calls props.onOpenChange before performing
drawerControl.toggle, so if the caller callback throws the toggle never runs;
change the order to call drawerControl.toggle() first (compute the new open
state via drawerControl.isOpen() after toggling or derive newState =
!drawerControl.isOpen() before calling toggle) and then invoke
props.onOpenChange with the new state, wrapping the callback invocation in a
try/catch to swallow/log errors so caller exceptions cannot block the drawer
toggle; reference drawerControl.toggle, drawerControl.isOpen, and
props.onOpenChange when making this change.
| } from './hooks/useEntityMention'; | ||
| import { useEmailSearchMention } from './hooks/useEmailSearchMention'; | ||
| import { isMobile } from '@core/mobile/isMobile'; | ||
| import { useAnalytics } from '@app/component/analytics-context'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same dependency inversion concern as EditableChatMessage.tsx.
packages/core importing from packages/app inverts the expected dependency direction. This creates coupling where the core package depends on the app package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/core/component/LexicalMarkdown/component/menu/MentionsMenu/MentionsMenu.tsx`
at line 51, The MentionsMenu component imports useAnalytics from the app layer
which inverts package dependencies (see the import of useAnalytics in
MentionsMenu.tsx and similar usage in EditableChatMessage.tsx); to fix, stop
importing from the app package — either move the analytics hook into the core
package or introduce an interface/prop to inject analytics into MentionsMenu
(e.g., accept an analytics object/function prop used where useAnalytics() is
currently called), update MentionsMenu and EditableChatMessage to obtain
analytics via the new core-exported hook or via dependency injection, and remove
the cross-package import so core no longer depends on app.
js/app/packages/core/component/LexicalMarkdown/component/menu/MentionsMenu/MentionsMenu.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+44
to
+47
| onClick={() => { | ||
| props.onOpenChange?.(!drawerControl.isOpen()); | ||
| drawerControl.toggle(); | ||
| }} |
Contributor
There was a problem hiding this comment.
Don't let onOpenChange block the drawer toggle.
Line 45 calls arbitrary caller code before Line 46 toggles the drawer, so any exception in the analytics callback leaves the References drawer stuck in its current state.
🛠️ Safer ordering
onClick={() => {
- props.onOpenChange?.(!drawerControl.isOpen());
- drawerControl.toggle();
+ const nextOpen = !drawerControl.isOpen();
+ try {
+ props.onOpenChange?.(nextOpen);
+ } finally {
+ drawerControl.toggle();
+ }
}}📝 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.
Suggested change
| onClick={() => { | |
| props.onOpenChange?.(!drawerControl.isOpen()); | |
| drawerControl.toggle(); | |
| }} | |
| onClick={() => { | |
| const nextOpen = !drawerControl.isOpen(); | |
| try { | |
| props.onOpenChange?.(nextOpen); | |
| } finally { | |
| drawerControl.toggle(); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/ReferencesModal.tsx` around lines 44 - 47, The
onClick handler currently invokes props.onOpenChange (caller-supplied) before
drawerControl.toggle, so an exception in the callback can prevent the drawer
from toggling; change the order to call drawerControl.toggle() first (using
drawerControl.isOpen() to compute the new state if needed) and then invoke
props.onOpenChange?.(newState) inside a try/catch to prevent caller errors from
blocking the toggle; update the onClick block that references
props.onOpenChange, drawerControl.toggle, and drawerControl.isOpen accordingly.
| ...withCallbacks<SendMessageResponse, Error, SendMessageParams>( | ||
| { | ||
| onSuccess: (data) => { | ||
| analytics.track('email_message_sent'); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding payload properties for parity with channel messages.
The email_message_sent event has no payload, while channel_message_sent includes contentLength, attachmentsLength, and isThreadReply. Consider adding similar properties for consistent analytics:
Suggested enhancement
- analytics.track('email_message_sent');
+ analytics.track('email_message_sent', {
+ hasAttachments: !!vars.message.attachments?.length,
+ isReply: !!vars.message.thread_db_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.
Suggested change
| analytics.track('email_message_sent'); | |
| analytics.track('email_message_sent', { | |
| hasAttachments: !!vars.message.attachments?.length, | |
| isReply: !!vars.message.thread_db_id, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/queries/email/thread.ts` at line 283, The
analytics.track('email_message_sent') call currently sends no payload; update
the call to include a payload with contentLength (compute from the outgoing
message body/content string length), attachmentsLength (compute from the
outgoing message's attachments array length), and isThreadReply (true if the
message has a parent/thread id or is being sent into an existing thread). Locate
the analytics.track call in js/app/packages/queries/email/thread.ts (the line
with analytics.track('email_message_sent')) and compute those values from the
message object used to send the email (e.g., message.body or message.content,
message.attachments, and message.threadId/parentId) before passing them as the
event properties.
…k-up-new-analytics-events
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/app/packages/block-pdf/store/comments/commentOperations.ts (1)
27-38:⚠️ Potential issue | 🟡 MinorAnalytics tracked before operation completes for
comment_create.The
comment_createevent is emitted at the start of the callback (Line 38) before the async operation completes. If the create operation fails, an analytics event will have been recorded for a comment that doesn't exist. Consider moving the tracking to after successful creation, similar to howcomment_deleteonly tracks on success.Suggested approach
return createCallback( async (info: CreateCommentRequest & { threadId: number }) => { - analytics.track('comment_create', { blockType: 'pdf' }); const { threadId, text } = info; // ... existing logic ... if (response) { + analytics.track('comment_create', { blockType: 'pdf' }); deleteNewComments(); } return response; } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-pdf/store/comments/commentOperations.ts` around lines 27 - 38, The analytics.track('comment_create', { blockType: 'pdf' }) call in useCreateComment is executed before the async create operation completes; move the tracking to run only after the create succeeds inside the createCallback async flow (i.e., await the appropriate creation path invoked via createFreeComment, createHighlightComment, attachHighlightComment, or createThreadReply and only then call analytics.track), ensuring you preserve relevant context (e.g., threadId or returned comment id) and do not emit the event on failures.js/app/packages/app/component/GlobalHotkeys.tsx (1)
205-262: 🧹 Nitpick | 🔵 TrivialConsider distinguishing theme event contexts.
All three theme handlers (apply theme, set light preference, set dark preference) track
'theme_changed'with only{ themeId }. Analytics consumers won't be able to distinguish between an immediate theme change versus setting a light/dark mode preference.♻️ Optional: Add context to differentiate theme events
// Line 211 - immediate apply - analytics.track('theme_changed', { themeId: theme.id }); + analytics.track('theme_changed', { themeId: theme.id, context: 'apply' }); // Line 234 - preferred light - analytics.track('theme_changed', { themeId: theme.id }); + analytics.track('theme_changed', { themeId: theme.id, context: 'preferred_light' }); // Line 257 - preferred dark - analytics.track('theme_changed', { themeId: theme.id }); + analytics.track('theme_changed', { themeId: theme.id, context: 'preferred_dark' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/GlobalHotkeys.tsx` around lines 205 - 262, The three hotkey handlers that call registerHotkey (the apply-theme block using setThemeScope.commandScopeId and applyTheme, the set-preferred-light block using setPreferredLightScope.commandScopeId and setLightModeTheme, and the set-preferred-dark block using setPreferredDarkScope.commandScopeId and setDarkModeTheme) all track the same analytics event 'theme_changed' with only { themeId }; update each keyDownHandler to include a distinguishing context field (e.g. { themeId, context: 'apply' } for applyTheme, { themeId, context: 'set_preferred_light' } for setLightModeTheme, and { themeId, context: 'set_preferred_dark' } for setDarkModeTheme) so analytics consumers can tell immediate changes from preference updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/GlobalHotkeys.tsx`:
- Around line 36-54: The analytics payload can contain undefined tokens because
some hotkeys omit hotkeyToken; update the useHotkeyAnalytics hook to guard or
normalize command.hotkeyToken before sending: inside useHotkeyAnalytics (the
function using useSubscribeToKeypress and reading context.commandCaptured),
replace passing command.hotkeyToken directly with a normalized value (e.g.,
fallback to a string like 'unspecified' or skip tracking when token is missing)
so analytics.track('hotkey_use', { action: description, token: /* normalized
token or skip */ }) never sends undefined.
- Around line 67-70: The handler currently always fires
analytics.track('command_menu_open') then calls CommandState.toggle(), which can
actually close the menu; change it to check the menu's current state (e.g.,
CommandState.isOpen or similar) and only call
analytics.track('command_menu_open', { from: 'global_hotkey' }) when the menu is
transitioning from closed to open, then call CommandState.toggle();
alternatively call an explicit open method (e.g., CommandState.open()) when
closed to avoid tracking on close. Use the handleCommandMenu, analytics.track
and CommandState.toggle/open identifiers to locate and update the logic.
In `@js/app/packages/block-pdf/store/comments/commentOperations.ts`:
- Around line 93-101: The useUpdateComment hook currently calls
analytics.track('comment_update', { blockType: 'pdf' }) before the edit
completes; update the createCallback returned by useUpdateComment so it awaits
the editComment(commentId, info) call (make the callback async or return the
Promise and chain .then) and only call analytics.track after editComment
resolves successfully, leaving failures untracked; refer to useUpdateComment,
editComment (from useEditCommentResource), analytics.track, and createCallback
when making the change to ensure parity with comment_delete behavior.
---
Outside diff comments:
In `@js/app/packages/app/component/GlobalHotkeys.tsx`:
- Around line 205-262: The three hotkey handlers that call registerHotkey (the
apply-theme block using setThemeScope.commandScopeId and applyTheme, the
set-preferred-light block using setPreferredLightScope.commandScopeId and
setLightModeTheme, and the set-preferred-dark block using
setPreferredDarkScope.commandScopeId and setDarkModeTheme) all track the same
analytics event 'theme_changed' with only { themeId }; update each
keyDownHandler to include a distinguishing context field (e.g. { themeId,
context: 'apply' } for applyTheme, { themeId, context: 'set_preferred_light' }
for setLightModeTheme, and { themeId, context: 'set_preferred_dark' } for
setDarkModeTheme) so analytics consumers can tell immediate changes from
preference updates.
In `@js/app/packages/block-pdf/store/comments/commentOperations.ts`:
- Around line 27-38: The analytics.track('comment_create', { blockType: 'pdf' })
call in useCreateComment is executed before the async create operation
completes; move the tracking to run only after the create succeeds inside the
createCallback async flow (i.e., await the appropriate creation path invoked via
createFreeComment, createHighlightComment, attachHighlightComment, or
createThreadReply and only then call analytics.track), ensuring you preserve
relevant context (e.g., threadId or returned comment id) and do not emit the
event on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae47c4b2-255f-4cae-afe4-52f86e9973d1
📒 Files selected for processing (10)
js/app/packages/app/component/GlobalHotkeys.tsxjs/app/packages/app/component/Root.tsxjs/app/packages/app/lib/analytics/analytics.tsjs/app/packages/app/lib/analytics/app-events.tsjs/app/packages/block-pdf/component/PageOverlay.tsxjs/app/packages/block-pdf/store/comments/commentOperations.tsjs/app/packages/core/component/AI/component/input/ChatInput.tsxjs/app/packages/core/component/AI/component/message/EditableChatMessage.tsxjs/app/packages/core/component/LexicalMarkdown/component/menu/MentionsMenu/MentionsMenu.tsxjs/app/packages/core/util/upload.ts
Comment on lines
+36
to
+54
| function useHotkeyAnalytics(): void { | ||
| const analytics = useAnalytics(); | ||
|
|
||
| useSubscribeToKeypress((context) => { | ||
| // Only track when a command was actually executed | ||
| if (!context.commandCaptured) return; | ||
|
|
||
| const command = context.commandCaptured; | ||
| const description = | ||
| typeof command.description === 'function' | ||
| ? command.description() | ||
| : command.description; | ||
|
|
||
| analytics.track('hotkey_use', { | ||
| action: description, | ||
| token: command.hotkeyToken, | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
hotkeyToken may be undefined for some hotkeys.
Several hotkeys in this file don't define hotkeyToken (e.g., lines 162, 206, 229, 252), so command.hotkeyToken will be undefined in the tracked event payload. Consider either:
- Explicitly handling undefined (e.g., fallback to
'unspecified') - Filtering out events without a token if they're not useful for analytics
♻️ Optional: Handle undefined token explicitly
analytics.track('hotkey_use', {
action: description,
- token: command.hotkeyToken,
+ token: command.hotkeyToken ?? 'unspecified',
});📝 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.
Suggested change
| function useHotkeyAnalytics(): void { | |
| const analytics = useAnalytics(); | |
| useSubscribeToKeypress((context) => { | |
| // Only track when a command was actually executed | |
| if (!context.commandCaptured) return; | |
| const command = context.commandCaptured; | |
| const description = | |
| typeof command.description === 'function' | |
| ? command.description() | |
| : command.description; | |
| analytics.track('hotkey_use', { | |
| action: description, | |
| token: command.hotkeyToken, | |
| }); | |
| }); | |
| } | |
| function useHotkeyAnalytics(): void { | |
| const analytics = useAnalytics(); | |
| useSubscribeToKeypress((context) => { | |
| // Only track when a command was actually executed | |
| if (!context.commandCaptured) return; | |
| const command = context.commandCaptured; | |
| const description = | |
| typeof command.description === 'function' | |
| ? command.description() | |
| : command.description; | |
| analytics.track('hotkey_use', { | |
| action: description, | |
| token: command.hotkeyToken ?? 'unspecified', | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/GlobalHotkeys.tsx` around lines 36 - 54, The
analytics payload can contain undefined tokens because some hotkeys omit
hotkeyToken; update the useHotkeyAnalytics hook to guard or normalize
command.hotkeyToken before sending: inside useHotkeyAnalytics (the function
using useSubscribeToKeypress and reading context.commandCaptured), replace
passing command.hotkeyToken directly with a normalized value (e.g., fallback to
a string like 'unspecified' or skip tracking when token is missing) so
analytics.track('hotkey_use', { action: description, token: /* normalized token
or skip */ }) never sends undefined.
Comment on lines
93
to
101
| export function useUpdateComment() { | ||
| const analytics = useAnalytics(); | ||
|
|
||
| const editComment = useEditCommentResource(); | ||
|
|
||
| return createCallback((commentId: number, info: EditCommentRequest) => { | ||
| track(TrackingEvents.BLOCKPDF.COMMENT.UPDATE); | ||
| analytics.track('comment_update', { blockType: 'pdf' }); | ||
| return editComment(commentId, info); | ||
| }); |
Contributor
There was a problem hiding this comment.
comment_update also tracked before operation completes.
Same concern as comment_create - the event is tracked before the edit operation succeeds. For consistency with comment_delete, consider tracking after successful completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-pdf/store/comments/commentOperations.ts` around lines
93 - 101, The useUpdateComment hook currently calls
analytics.track('comment_update', { blockType: 'pdf' }) before the edit
completes; update the createCallback returned by useUpdateComment so it awaits
the editComment(commentId, info) call (make the callback async or return the
Promise and chain .then) and only call analytics.track after editComment
resolves successfully, leaving failures untracked; refer to useUpdateComment,
editComment (from useEditCommentResource), analytics.track, and createCallback
when making the change to ensure parity with comment_delete behavior.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
js/app/packages/app/component/GlobalHotkeys.tsx (2)
52-55:⚠️ Potential issue | 🟡 MinorGuard
hotkey_usewhenhotkeyTokenis absent.Several commands in this file still register without a token—for example Line 166—so this subscriber can emit
token: undefinedin the new payload.♻️ Skip malformed events here, or backfill tokens at registration sites
- analytics.track('hotkey_use', { - action: description, - token: command.hotkeyToken, - }); + if (!command.hotkeyToken) return; + + analytics.track('hotkey_use', { + action: description, + token: command.hotkeyToken, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/GlobalHotkeys.tsx` around lines 52 - 55, The analytics.track call for the 'hotkey_use' event can emit token: undefined when a command lacks hotkeyToken; update the subscriber in GlobalHotkeys.tsx to skip or bail out when command.hotkeyToken is falsy (e.g., if (!command.hotkeyToken) return) before calling analytics.track('hotkey_use', {...}), or alternatively provide a clear fallback value (e.g., token: 'unknown') so analytics.track never receives undefined; make the change around the analytics.track invocation referencing analytics.track, 'hotkey_use', and command.hotkeyToken.
71-74:⚠️ Potential issue | 🟠 MajorTrack
command_menu_openonly on transitions to open.
CommandState.toggle()can close the palette too, so this currently records closes as opens.🐛 Gate the event on the pre-toggle state
const handleCommandMenu = () => { - analytics.track('command_menu_open', { from: 'global_hotkey' }); + if (!CommandState.isOpen()) { + analytics.track('command_menu_open', { from: 'global_hotkey' }); + } CommandState.toggle(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/GlobalHotkeys.tsx` around lines 71 - 74, The handler handleCommandMenu currently calls CommandState.toggle() and always tracks 'command_menu_open', which records closes as opens; change it to check the current open state (e.g., CommandState.value or CommandState.isOpen) before toggling and only call analytics.track('command_menu_open', { from: 'global_hotkey' }) when the palette is currently closed (i.e., the toggle will open it) — perform the pre-toggle check, then call CommandState.toggle() so only transitions to open are tracked.js/app/packages/app/component/app-sidebar/sidebar.tsx (1)
246-249:⚠️ Potential issue | 🟠 MajorOnly emit
command_menu_openwhen the sidebar actually opens it.This button toggles the palette, so clicking it while the palette is already open records a false “open”.
🐛 Gate the event on the current palette state
const handleCommandPaletteClick = () => { - analytics.track('command_menu_open', { from: 'sidebar' }); + if (!CommandState.isOpen()) { + analytics.track('command_menu_open', { from: 'sidebar' }); + } CommandState.toggle(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/app-sidebar/sidebar.tsx` around lines 246 - 249, The handler currently emits analytics unconditionally; change handleCommandPaletteClick to only send analytics.track('command_menu_open', { from: 'sidebar' }) when the palette is actually being opened by the click — e.g., check the current CommandState open flag (like CommandState.isOpen or similar) and only call analytics.track when it is currently closed before calling CommandState.toggle(), or alternatively call CommandState.toggle() and emit the event only when the toggle result indicates the palette is now open; reference handleCommandPaletteClick, analytics.track, and CommandState.toggle/isOpen to implement this gating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/app/packages/app/component/app-sidebar/sidebar.tsx`:
- Around line 246-249: The handler currently emits analytics unconditionally;
change handleCommandPaletteClick to only send
analytics.track('command_menu_open', { from: 'sidebar' }) when the palette is
actually being opened by the click — e.g., check the current CommandState open
flag (like CommandState.isOpen or similar) and only call analytics.track when it
is currently closed before calling CommandState.toggle(), or alternatively call
CommandState.toggle() and emit the event only when the toggle result indicates
the palette is now open; reference handleCommandPaletteClick, analytics.track,
and CommandState.toggle/isOpen to implement this gating.
In `@js/app/packages/app/component/GlobalHotkeys.tsx`:
- Around line 52-55: The analytics.track call for the 'hotkey_use' event can
emit token: undefined when a command lacks hotkeyToken; update the subscriber in
GlobalHotkeys.tsx to skip or bail out when command.hotkeyToken is falsy (e.g.,
if (!command.hotkeyToken) return) before calling analytics.track('hotkey_use',
{...}), or alternatively provide a clear fallback value (e.g., token: 'unknown')
so analytics.track never receives undefined; make the change around the
analytics.track invocation referencing analytics.track, 'hotkey_use', and
command.hotkeyToken.
- Around line 71-74: The handler handleCommandMenu currently calls
CommandState.toggle() and always tracks 'command_menu_open', which records
closes as opens; change it to check the current open state (e.g.,
CommandState.value or CommandState.isOpen) before toggling and only call
analytics.track('command_menu_open', { from: 'global_hotkey' }) when the palette
is currently closed (i.e., the toggle will open it) — perform the pre-toggle
check, then call CommandState.toggle() so only transitions to open are tracked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b00345fe-184c-486f-b526-444a57f4097d
📒 Files selected for processing (3)
js/app/packages/app/component/GlobalHotkeys.tsxjs/app/packages/app/component/app-sidebar/sidebar.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/soup-filters-bar.tsx
…k-up-new-analytics-events
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/app/component/app-sidebar/sidebar.tsx (1)
497-511:⚠️ Potential issue | 🟡 Minor
sidebar_clickis tracked even for middle-clicks that do nothing.The analytics event fires at line 498 before the early return for middle mouse at line 502. This means middle-clicks are tracked but don't result in any navigation action.
If middle-click tracking is intentional (e.g., to measure click attempts), this is fine. Otherwise, move the analytics call after the middle-mouse check.
♻️ Optional: track only actionable clicks
onClick={(e) => { - analytics.track('sidebar_click', { - view: props.id, - }); // Middle mouse handling if (e.button === 1) return; e.preventDefault(); + analytics.track('sidebar_click', { + view: props.id, + }); layout.openWithSplit(content(), {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/app-sidebar/sidebar.tsx` around lines 497 - 511, The analytics.track('sidebar_click', ...) call is executed before the middle-click early return in the onClick handler, causing middle mouse clicks to be tracked even though they perform no action; move the analytics.track invocation to after the middle-button check (i.e., after the if (e.button === 1) return;) inside the same onClick handler so only actionable clicks call analytics.track, while keeping the existing payload (view: props.id) and preserving the subsequent layout.openWithSplit call and its options.
♻️ Duplicate comments (4)
js/app/packages/app/lib/analytics/analytics.ts (3)
96-114:⚠️ Potential issue | 🟠 MajorProviders in
identify()are still grouped in a single try/catch.If
gtag()orfbq()throws,posthog.identify()will be skipped. ThetryInitializepattern used for initialization should be applied here as well to ensure each provider's lifecycle method runs independently.🐛 Proposed fix: isolate each provider call
const identify = (userID: string, info: Partial<UserIdentifyInfo>) => { if (disabled) return; - try { + try { gtag('config', GA_ID, { user_id: userID, ...(info.email && { email: info.email }), ...(info.os && { os: info.os }), }); + } catch (e) { + console.error('[Analytics] Failed to identify user (GA):', e); + } + try { fbq('init', '639142540393286', { external_id: userID, em: info.email, }); + } catch (e) { + console.error('[Analytics] Failed to identify user (Meta):', e); + } + try { posthog.identify(userID, { ...info }); } catch (e) { - console.error('[Analytics] Failed to identify user:', e); + console.error('[Analytics] Failed to identify user (PostHog):', e); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/lib/analytics/analytics.ts` around lines 96 - 114, The identify function groups gtag, fbq and posthog.identify calls inside one try/catch so a thrown error from one provider (gtag or fbq) prevents subsequent providers from running; refactor identify to call each provider separately and wrap each call in its own try/catch (or reuse the existing tryInitialize pattern) so that gtag(...), fbq(...), and posthog.identify(...) execute independently and failures in one are logged but do not skip the others.
117-127:⚠️ Potential issue | 🟠 MajorProviders in
reset()are still grouped in a single try/catch.Same issue as
identify()— ifgtag()throws,posthog.reset()won't execute.🐛 Proposed fix: isolate each provider call
const reset = () => { if (disabled) return; - try { + try { gtag('config', GA_ID, { user_id: undefined }); + } catch (e) { + console.error('[Analytics] Failed to reset (GA):', e); + } + try { posthog.reset(); } catch (e) { - console.error('[Analytics] Failed to reset:', e); + console.error('[Analytics] Failed to reset (PostHog):', e); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/lib/analytics/analytics.ts` around lines 117 - 127, The reset() function groups both provider calls in one try/catch so a thrown error from gtag prevents posthog.reset() from running; split them into separate guarded calls: keep the early disabled check, wrap the gtag('config', GA_ID, { user_id: undefined }) call in its own try/catch and log errors, then call posthog.reset() in a separate try/catch (also logging errors) so failure in one provider does not stop the other.
47-57:⚠️ Potential issue | 🟠 MajorPostHog instance is never initialized in dev, breaking feature-flag access.
Setting
disabled = import.meta.env.DEVand returning early frominitializeProviders()meansposthog.init()is never called in local builds. Sinceanalytics.posthogis exported for feature-flag checks elsewhere, those checks will returnundefined/falseand flagged UI cannot be exercised locally.Consider initializing PostHog in all environments but short-circuiting emission methods (
track,identify,reset) in dev, so feature flags remain functional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/lib/analytics/analytics.ts` around lines 47 - 57, The PostHog client is never initialized in dev because initializeProviders returns early when disabled; update createAnalytics so the PostHog instance (posthog created in createAnalytics) is always initialized via initializePosthog(posthog) regardless of import.meta.env.DEV, but when disabled replace or wrap its emission methods (track, identify, reset) with no-op functions to prevent network calls; locate initializeProviders, the disabled flag, the PostHog instance, tryInitialize, and initializePosthog to implement calling posthog.init in all environments while short-circuiting track/identify/reset in dev.js/app/packages/app/component/GlobalHotkeys.tsx (1)
52-55: 🧹 Nitpick | 🔵 Trivial
hotkeyTokenmay beundefinedin tracked payload.Several hotkeys in this file don't define
hotkeyToken, socommand.hotkeyTokenwill beundefinedin the analytics event. Consider providing a fallback value for cleaner analytics data.♻️ Optional: Handle undefined token explicitly
analytics.track('hotkey_use', { action: description, - token: command.hotkeyToken, + token: command.hotkeyToken ?? 'unspecified', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/GlobalHotkeys.tsx` around lines 52 - 55, The analytics payload sometimes sends undefined for hotkeyToken because some commands lack command.hotkeyToken; update the analytics.track call in GlobalHotkeys.tsx to supply a deterministic fallback (e.g., use command.hotkeyToken if present, otherwise a fixed string like 'unknown' or a derived identifier such as command.id or description) so analytics.track('hotkey_use', { action: description, token: ... }) never receives undefined; change the token expression where analytics.track is called to use the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/app/packages/app/component/app-sidebar/sidebar.tsx`:
- Around line 497-511: The analytics.track('sidebar_click', ...) call is
executed before the middle-click early return in the onClick handler, causing
middle mouse clicks to be tracked even though they perform no action; move the
analytics.track invocation to after the middle-button check (i.e., after the if
(e.button === 1) return;) inside the same onClick handler so only actionable
clicks call analytics.track, while keeping the existing payload (view: props.id)
and preserving the subsequent layout.openWithSplit call and its options.
---
Duplicate comments:
In `@js/app/packages/app/component/GlobalHotkeys.tsx`:
- Around line 52-55: The analytics payload sometimes sends undefined for
hotkeyToken because some commands lack command.hotkeyToken; update the
analytics.track call in GlobalHotkeys.tsx to supply a deterministic fallback
(e.g., use command.hotkeyToken if present, otherwise a fixed string like
'unknown' or a derived identifier such as command.id or description) so
analytics.track('hotkey_use', { action: description, token: ... }) never
receives undefined; change the token expression where analytics.track is called
to use the fallback.
In `@js/app/packages/app/lib/analytics/analytics.ts`:
- Around line 96-114: The identify function groups gtag, fbq and
posthog.identify calls inside one try/catch so a thrown error from one provider
(gtag or fbq) prevents subsequent providers from running; refactor identify to
call each provider separately and wrap each call in its own try/catch (or reuse
the existing tryInitialize pattern) so that gtag(...), fbq(...), and
posthog.identify(...) execute independently and failures in one are logged but
do not skip the others.
- Around line 117-127: The reset() function groups both provider calls in one
try/catch so a thrown error from gtag prevents posthog.reset() from running;
split them into separate guarded calls: keep the early disabled check, wrap the
gtag('config', GA_ID, { user_id: undefined }) call in its own try/catch and log
errors, then call posthog.reset() in a separate try/catch (also logging errors)
so failure in one provider does not stop the other.
- Around line 47-57: The PostHog client is never initialized in dev because
initializeProviders returns early when disabled; update createAnalytics so the
PostHog instance (posthog created in createAnalytics) is always initialized via
initializePosthog(posthog) regardless of import.meta.env.DEV, but when disabled
replace or wrap its emission methods (track, identify, reset) with no-op
functions to prevent network calls; locate initializeProviders, the disabled
flag, the PostHog instance, tryInitialize, and initializePosthog to implement
calling posthog.init in all environments while short-circuiting
track/identify/reset in dev.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: acfdff9f-ee88-4e5d-8a74-ea4e81bbc888
📒 Files selected for processing (3)
js/app/packages/app/component/GlobalHotkeys.tsxjs/app/packages/app/component/app-sidebar/sidebar.tsxjs/app/packages/app/lib/analytics/analytics.ts
This was referenced Mar 30, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.