show upgrade dialog to new users #2788
Conversation
Reviewer's Guide by SourceryThis pull request introduces a feature to display an upgrade dialog to new users upon login. It also ensures that users who select a 'pro' plan via a query parameter are shown the upgrade dialog. The implementation involves adding an Sequence diagram for showing upgrade dialog to new userssequenceDiagram
participant User
participant AccountEffects
participant AccountService
participant Store
participant WindowsMetaActions
User->>AccountEffects: Logs in
AccountEffects->>AccountService: getStats()
AccountEffects->>AccountService: getPlan()
AccountEffects->>AccountService: getPlanInfos()
AccountEffects->>AccountService: getAvailableCredits()
AccountService-->>AccountEffects: Returns data
AccountEffects->>AccountEffects: Checks if user is new or selected 'pro' plan
alt isNewUser or selected 'pro' plan
AccountEffects->>Store: Dispatch ShowUpgradeDialogAction
Store-->>AccountEffects: Acknowledged
AccountEffects->>AccountService: getUpgradeProUrl()
AccountService-->>AccountEffects: Returns URL
AccountEffects->>AccountEffects: Open external link
end
AccountEffects->>Store: Dispatch other actions
Store-->>AccountEffects: Acknowledged
Updated class diagram for IUserProfileclassDiagram
class IUserProfile {
firstName: string
lastName?: string
picture?: string
isNewUser?: boolean
tokens: IToken
}
class IToken {
// Assuming IToken is defined elsewhere
}
IUserProfile -- IToken : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @imolorhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a feature that displays an upgrade dialog to new users. It achieves this by adding an isNewUser property to the user profile, which is determined by checking if the user's creation time is within the last 10 minutes. This property is then used in the account effects to conditionally show the upgrade dialog to new users upon login, in addition to the existing logic based on query parameters.
Highlights
- New User Identification: The PR introduces a new
isNewUserflag in the user profile to identify recently created users. - Conditional Upgrade Dialog: The upgrade dialog is now displayed to new users upon login, in addition to the existing logic based on the 'plan_select' query parameter.
- Code Improvements: The PR also includes minor code improvements such as using the nullish coalescing operator (
??) instead of the logical OR operator (||) for default value assignments.
Changelog
Click here to see the changelog
- packages/altair-api-utils/src/user.ts
- Added
isNewUserproperty to theIUserProfileinterface.
- Added
- packages/altair-api/src/auth/auth.service.ts
- Added logic to set the
isNewUserproperty based on the user's creation time (within 10 minutes).
- Added logic to set the
- packages/altair-api/src/auth/strategies/google.strategy.ts
- Replaced logical OR operator (
||) with nullish coalescing operator (??) for defaultfirstNameassignment.
- Replaced logical OR operator (
- packages/altair-app/src/app/modules/altair/effects/account.effect.ts
- Imported
offrom 'rxjs'. - Added
isNewUserproperty to the user object when dispatchingSetAccountAction. - Added
isNewUserproperty to the user object when dispatchingAccountIsLoggedInAction. - Modified the
ACCOUNT_IS_LOGGED_INeffect to display the upgrade dialog to new users if they haven't already seen it via the query parameter logic.
- Imported
- packages/altair-app/src/app/modules/altair/store/account/account.action.ts
- Added
isNewUserproperty to theAccountIsLoggedInActionpayload.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The name Altair comes from the brightest star in the constellation Aquila (the Eagle).
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a new feature to show an upgrade dialog to new users. The changes look good overall, but there are a few areas that could be improved for clarity and efficiency.
Summary of Findings
- Magic number: The use of
1000 * 60 * 10(10 minutes) as a magic number should be replaced with a named constant for better readability and maintainability. - Redundant check: The
plan.canUpgradeProcheck inAccountEffectsseems redundant as the subsequent logic depends on it. Consider simplifying the conditional logic. - Nullish coalescing operator: The usage of the nullish coalescing operator
??is good, but ensure it's consistently used throughout the codebase for similar scenarios.
Merge Readiness
The pull request is almost ready for merging. Addressing the magic number and redundant check would improve the code quality. I am unable to approve the pull request, and other reviewers should review and approve this code before merging.
| @@ -109,6 +109,7 @@ export class AuthService { | |||
| firstName: user.firstName, | |||
| lastName: user.lastName, | |||
| picture: user.picture, | |||
| isNewUser: Date.now() - user.createdAt.getTime() < 1000 * 60 * 10, // 10 minutes | |||
There was a problem hiding this comment.
This calculation determines if a user is new based on their creation time. Using a magic number like 1000 * 60 * 10 (10 minutes) makes the code less readable. Consider defining a constant for this duration to improve clarity and maintainability.
const TEN_MINUTES_IN_MS = 1000 * 60 * 10;
isNewUser: Date.now() - user.createdAt.getTime() < TEN_MINUTES_IN_MS, // 10 minutes| if (plan.canUpgradePro) { | ||
| const selectedPlan = consumeQueryParam('plan_select'); | ||
| if (selectedPlan === 'pro') { | ||
| this.store.dispatch( | ||
| new windowsMetaActions.ShowUpgradeDialogAction({ value: true }) | ||
| ); | ||
| this.accountService.getUpgradeProUrl().then(({ url }) => { | ||
| externalLink(url); | ||
| }); | ||
| } else if (user.isNewUser) { | ||
| this.store.dispatch( | ||
| new windowsMetaActions.ShowUpgradeDialogAction({ value: true }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The plan.canUpgradePro check is performed, and then the selectedPlan === 'pro' check is nested inside. If plan.canUpgradePro is false, the inner block will never execute. Consider simplifying this logic by removing the outer if statement, since the inner logic depends on it anyway. Also, it might be clearer to use an else if instead of two separate if statements.
if (plan.canUpgradePro && selectedPlan === 'pro') {
this.store.dispatch(
new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
);
this.accountService.getUpgradeProUrl().then(({ url }) => {
externalLink(url);
});
} else if (user.isNewUser) {
this.store.dispatch(
new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
);![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining the magic number
1000 * 60 * 10inpackages/altair-api/src/auth/auth.service.ts.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -98,10 +99,11 @@ export class AccountEffects { | |||
| ); | |||
| this.store.dispatch( | |||
There was a problem hiding this comment.
issue (complexity): Consider using guard clauses to flatten the nested if statements and improve readability by reducing complexity and improving code flow.
Consider flattening the nested ifs with guard clauses to improve readability. For example, extract the upgrade logic into its own helper or simply return early to avoid deep nesting. One option:
// Before:
if (plan.canUpgradePro) {
const selectedPlan = consumeQueryParam('plan_select');
if (selectedPlan === 'pro') {
this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
this.accountService.getUpgradeProUrl().then(({ url }) => {
externalLink(url);
});
} else if (user.isNewUser) {
this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
}
}
// After:
if (!plan.canUpgradePro) {
return;
}
const selectedPlan = consumeQueryParam('plan_select');
if (selectedPlan === 'pro') {
this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
this.accountService.getUpgradeProUrl().then(({ url }) => {
externalLink(url);
});
return;
}
if (user.isNewUser) {
this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
}This maintains the original functionality while reducing nesting and clarifying your intent.
|
Visit the preview URL for this PR (updated for commit 0443201): https://altair-gql--pr2788-imolorhe-show-upgrad-ajkw6471.web.app (expires Fri, 14 Mar 2025 15:34:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
imolorhe
force-pushed
the
imolorhe/show-upgrade-to-new-users
branch
from
3036952 to
b973ef3
Compare
Fixes
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
New Features: