Hide "try demo" on login page (fixes #2106) #3889
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
sammichaels
changed the title
WalkthroughThe changes made in this pull request involve modifications to the Assessment against linked issues
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
packages/desktop-client/src/components/manager/subscribe/Login.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/shared/environment.ts (1)
21-24: Consider improving type safety and documentation.The implementation correctly handles the hide/show logic for the demo login button. However, there are a few suggestions for improvement:
- Add JSDoc documentation to clarify the function's purpose and expected environment variable values
- Consider adding type safety for the environment variable
Here's a suggested improvement:
+/** + * Determines whether to display the demo login button. + * @returns {boolean} Returns true if demo login should be shown, false otherwise + * @example + * // To hide demo login, set in your environment: + * REACT_APP_HIDE_LOGIN_DEMO=true + */ export function shouldDisplayLoginDemo() { - return process.env.REACT_APP_HIDE_LOGIN_DEMO === undefined; + const hideDemo = process.env.REACT_APP_HIDE_LOGIN_DEMO; + return hideDemo === undefined || hideDemo === ''; }packages/desktop-client/src/components/manager/subscribe/Login.tsx (1)
Line range hint
1-189: Consider adding documentation for self-hosted users.While the implementation is solid, it would be helpful to add documentation explaining how to configure the
REACT_APP_HIDE_LOGIN_DEMOenvironment variable for self-hosted instances.Would you like me to help draft the documentation for this feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3889.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/manager/subscribe/Login.tsx(2 hunks)packages/loot-core/src/shared/environment.ts(1 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
10-10: LGTM! Clean import addition.
The import of shouldDisplayLoginDemo is well-placed and follows the project's import structure.
173-189: Clean implementation of the configurable demo button.
The conditional rendering is well-implemented and maintains the original layout while providing the requested configurability for self-hosted instances.
Let's verify the environment variable handling:
| export function shouldDisplayLoginDemo() { | ||
| return process.env.REACT_APP_HIDE_LOGIN_DEMO === undefined; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Environment variable needs documentation
The implementation is correctly used in the Login component to conditionally render a demo section, but the REACT_APP_HIDE_LOGIN_DEMO environment variable is not documented anywhere in the codebase. This could cause confusion for other developers.
- Add documentation for
REACT_APP_HIDE_LOGIN_DEMOenvironment variable inpackages/desktop-client/README.md, explaining its purpose and usage - Consider adding a
.env.examplefile to showcase the available environment variables
🔗 Analysis chain
Verify implementation completeness.
Let's verify the implementation and documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation completeness
# Check if the function is used in the Login component
echo "Checking function usage in Login component..."
rg -l "shouldDisplayLoginDemo" "packages/desktop-client/src/components/manager/subscribe/Login"
# Check for any other usages of the function
echo "Checking for other function usages..."
rg -l "shouldDisplayLoginDemo"
# Check if environment variable is documented
echo "Checking for environment variable documentation..."
rg -l "REACT_APP_HIDE_LOGIN_DEMO" -g "*.md"
rg -l "REACT_APP_HIDE_LOGIN_DEMO" -g "*.env*"
Length of output: 875
Script:
#!/bin/bash
# Let's examine the actual usage of the function in the Login component
echo "Checking function implementation in Login.tsx..."
rg "shouldDisplayLoginDemo" packages/desktop-client/src/components/manager/subscribe/Login.tsx -A 3 -B 3
# Let's also check for any environment variable setup files
echo "Checking for environment setup files..."
fd -g ".env*"
# Check for any documentation files that might contain environment variables
echo "Checking documentation files..."
fd "README.md"
Length of output: 1254
Script:
#!/bin/bash
# Let's check the content of relevant README files for environment variable documentation
echo "Checking README files for environment documentation..."
rg "REACT_APP_HIDE_LOGIN_DEMO|environment|env" packages/desktop-client/README.md -A 3 -B 3
# Let's check with hidden files included for .env files
echo "Checking for .env files with hidden flag..."
fd -H -g ".env*"
# Let's also check the Login component to see the complete context of the demo usage
echo "Checking Login component demo section..."
rg "shouldDisplayLoginDemo" packages/desktop-client/src/components/manager/subscribe/Login.tsx -A 10 -B 3
Length of output: 2007
Fixes #2106. Setting the environment variable REACT_APP_HIDE_LOGIN_DEMO to anything will prevent the "Try Demo" link from displaying on the login page.