-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: Makes RooTips appear in home again #7986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… an issue where <RooTips /> would be incorrectly hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the changes and found some issues that need attention. The implementation adds useful utilities for accessing DismissibleUpsell state, but there are a few critical issues to address.
|
|
||
| export const DismissedUpsellsProvider: React.FC<DismissedUpsellsProviderProps> = ({ children }) => { | ||
| const [dismissedUpsells, setDismissedUpsells] = useState<string[]>([]) | ||
| const [isLoading, setIsLoading] = useState(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The context starts with isLoading: true but components might render before the dismissed list is fetched. This could cause a flash of upsells that should be hidden.
Consider either:
- Starting with
isLoading: falseand an empty list - Or ensuring components handle the loading state properly to prevent flashing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's switch to isLoading false by default
| // Wrapper component that provides the context | ||
| const DismissibleUpsell = memo((props: DismissibleUpsellProps) => { | ||
| return ( | ||
| <DismissedUpsellsProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consider lifting the DismissedUpsellsProvider to a higher level? Each DismissibleUpsell instance wrapping itself with a provider could cause issues if multiple upsells are rendered, as each would have its own provider instance and separate state.
The provider should ideally be at the app root level to share state across all upsells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, but this isn't used often enough to warrant being at the top level. If other reviewers agree with you, I can make that change, but I'll refrain from it for now.
| import DismissibleUpsell from "../common/DismissibleUpsell" | ||
| import { useCloudUpsell } from "@src/hooks/useCloudUpsell" | ||
| import { Cloud } from "lucide-react" | ||
| import { useUpsellVisibility, UPSELL_IDS } from "@/hooks/useUpsellVisibility" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Import path inconsistency. Some imports use @/ alias while others use @src/. Consider standardizing on one approach for consistency.
|
@roomote-agent read through the comments and implement the relevant feedback. |
|
Hi @brunobergher, I've reviewed the PR comments and I'm now implementing the requested changes. I'll address the following:
I'll update you once the changes are complete. |
- Change isLoading default to false in DismissedUpsellsContext to prevent flash of upsells - Fix import path inconsistency in ChatView.tsx (use @src/ instead of @/)
|
Hi @brunobergher, I've successfully implemented the requested changes from the PR review: ✅ Changed
✅ Fixed import path inconsistency in
The changes have been pushed to the PR branch. The relevant CI checks (compile, linting, code analysis) are passing. The test failures appear to be pre-existing issues unrelated to these changes. Let me know if you need any additional adjustments! |
But because this is dependent on knowing the state of a DismissibleUpsell, it adds some utilities to be able to access their state from the outside. No visual changes.