Add posthog capture for optimistic actions#1521
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant useOptimisticActions
participant PostHog
User->>useOptimisticActions: Trigger optimistic action (e.g., MOVE, STAR)
useOptimisticActions->>useOptimisticActions: Execute optimistic update
useOptimisticActions->>PostHog: capture(eventName generated from action type and params)
useOptimisticActions-->>User: Confirm action completion
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This can just spam our event logs - we're not tracking complete actions? |
|
let me update. |
ahmetskilinc
force-pushed
the
06-27-add_posthog_capture_for_optimistic_actions
branch
from
2f053c8 to
7b43d59
Compare
…event tracking with posthog captures for various email actions
ahmetskilinc
force-pushed
the
06-27-add_posthog_capture_for_optimistic_actions
branch
from
7b43d59 to
4a4a687
Compare
Merge activity
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const actionEventNames: Record<ActionType, (params: any) => string> = { | ||
| [ActionType.MOVE]: () => 'email_moved', | ||
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | ||
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | ||
| [ActionType.IMPORTANT]: (params) => | ||
| params.important ? 'email_marked_important' : 'email_unmarked_important', | ||
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | ||
| }; |
There was a problem hiding this comment.
The params parameter in actionEventNames functions is typed as any, which bypasses TypeScript's type checking. Since the PR has already defined specific parameter types in the PendingAction type union, these could be leveraged here for better type safety.
Consider refactoring to use the appropriate parameter types for each action type:
const actionEventNames: {
[K in ActionType]: (params: Extract<PendingAction, { type: K }>['params']) => string
} = {
[ActionType.MOVE]: (params) => 'email_moved',
[ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'),
// etc.
};This approach would provide proper type checking and autocompletion for each action type's parameters.
| const actionEventNames: Record<ActionType, (params: any) => string> = { | |
| [ActionType.MOVE]: () => 'email_moved', | |
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | |
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | |
| [ActionType.IMPORTANT]: (params) => | |
| params.important ? 'email_marked_important' : 'email_unmarked_important', | |
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | |
| }; | |
| const actionEventNames: { | |
| [K in ActionType]: (params: Extract<PendingAction, { type: K }>['params']) => string | |
| } = { | |
| [ActionType.MOVE]: (params) => 'email_moved', | |
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | |
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | |
| [ActionType.IMPORTANT]: (params) => | |
| params.important ? 'email_marked_important' : 'email_unmarked_important', | |
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | |
| }; |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| }; | ||
|
|
||
| optimisticActionsManager.pendingActions.set(pendingActionId, pendingAction); | ||
| optimisticActionsManager.pendingActions.set(pendingActionId, pendingAction as PendingAction); |
There was a problem hiding this comment.
The type assertion as PendingAction appears unnecessary here. Consider defining pendingAction with an explicit type annotation instead:
const pendingAction: PendingAction = {
id: pendingActionId,
type,
threadIds,
params,
optimisticId,
execute,
undo,
};This would allow TypeScript to validate the object structure at compile time rather than forcing the type with an assertion, which bypasses type checking and could hide potential errors.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| const actionEventNames: Record<ActionType, (params: any) => string> = { | ||
| [ActionType.MOVE]: () => 'email_moved', | ||
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | ||
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | ||
| [ActionType.IMPORTANT]: (params) => | ||
| params.important ? 'email_marked_important' : 'email_unmarked_important', | ||
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | ||
| }; |
There was a problem hiding this comment.
There appears to be a type inconsistency in how ActionType is defined and used. The actionEventNames object is typed as Record<ActionType, (params: any) => string> where ActionType is an enum (numeric values), but later it's accessed with actionEventNames[type] where type is typed as keyof typeof ActionType (string keys).
To resolve this inconsistency, consider one of these approaches:
- Change the type definition to
Record<keyof typeof ActionType, (params: any) => string>to match how it's being accessed - Or modify the access pattern to
actionEventNames[ActionType[type as keyof typeof ActionType]]
This will ensure type safety and prevent potential runtime errors when accessing event names.
| const actionEventNames: Record<ActionType, (params: any) => string> = { | |
| [ActionType.MOVE]: () => 'email_moved', | |
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | |
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | |
| [ActionType.IMPORTANT]: (params) => | |
| params.important ? 'email_marked_important' : 'email_unmarked_important', | |
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | |
| }; | |
| const actionEventNames: Record<keyof typeof ActionType, (params: any) => string> = { | |
| [ActionType.MOVE]: () => 'email_moved', | |
| [ActionType.STAR]: (params) => (params.starred ? 'email_starred' : 'email_unstarred'), | |
| [ActionType.READ]: (params) => (params.read ? 'email_marked_read' : 'email_marked_unread'), | |
| [ActionType.IMPORTANT]: (params) => | |
| params.important ? 'email_marked_important' : 'email_unmarked_important', | |
| [ActionType.LABEL]: (params) => (params.add ? 'email_label_added' : 'email_label_removed'), | |
| }; | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Add PostHog analytics tracking for email actions
Description
Added PostHog event tracking to various email actions to improve analytics and user behavior insights. Events are now captured when users:
Summary by CodeRabbit