Adding email to the menu #566
Conversation
✅ Deploy Preview for fireproof-ai-builder canceled.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| useEffect(() => { | ||
| //Logic to get the Current Account | ||
| setCurrentAccount('Amber@vibes.diy'); | ||
| }, []); |
There was a problem hiding this comment.
The new account menu button always displays Amber@vibes.diy because the effect that initializes currentAccount sets it to that literal string. This means every signed-in user will see the same email regardless of who is actually authenticated, which misrepresents the current account and makes the logout action ambiguous. The value should be derived from the logged-in user’s data or omitted until real data is available.
Useful? React with 👍 / 👎.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Hardcoded email in
VibesPanelintroduces brittle behavior and potential leakage of personal info; also causes a blank label on initial render. - Static
id="invite-status"risks duplicate IDs if multiple panels are mounted. - Minor coupling concern:
ButtonSizeis exported from styles rather than the component. - Small button layout uses
fit-content; considerauto/inline-blockfor broader compatibility. Also, Back button rendering is duplicated across modes and can be centralized.
Additional notes (4)
-
Maintainability |
use-vibes/base/components/VibesPanel/VibesPanel.tsx:120-156
The Back button is duplicated in every non-default mode (accounts,mutate,invite). This repetition is easy to drift and makes layout changes harder. Centralize the Back button so it renders once whenevermode !== 'default'. -
Readability |
use-vibes/base/components/VibesPanel/VibesPanel.tsx:123-125
The email-labeled button opens the account menu, but screen readers won’t know its purpose. Consider anaria-labelthat communicates the action (e.g., “Open account menu for …”). This improves accessibility without changing visuals. -
Compatibility |
use-vibes/base/components/VibesButton/VibesButton.styles.ts:35-41
Usingwidth: 'fit-content'for the small button can be inconsistent across older browsers. For more predictable layout, considerwidth: 'auto'and ensure the button renders asinline-block(or simply omit width for small). -
Maintainability |
use-vibes/base/components/VibesButton/VibesButton.tsx:2-2
Exporting theButtonSizetype from the styles module couples your component API to a styling file. It’s cleaner to define the prop type alongside the component and import it into the styles (type-only) if needed. This keeps the styles module purely presentational and avoids circular ownership of component API.
Summary of changes
- VibesButton: Introduced a new
sizeprop ('default' | 'small') and extendedgetButtonStyleto style small buttons (reduced padding, font size, andfit-contentwidth). - VibesPanel: Extracted inline styles into a new
VibesPanel.styles.tswith named style helpers; refactored rendering via acurrentlyDisplaymap for modes. - Added a new
accountsmode that shows the current email (as a button) and an Accounts screen with Logout + Back. - Minor UI/UX tweaks: consistent Back buttons (small size), input styling cleanup, and centralized container/inner styles.
| useEffect(() => { | ||
| //Logic to get the Current Account | ||
| setCurrentAccount('Amber@vibes.diy'); | ||
| }, []); |
There was a problem hiding this comment.
Hardcoding a specific email in production logic is brittle and leaks personal info. This will render an empty button on first paint (before useEffect runs) and then replace it with the hardcoded address, creating a flash of empty content and coupling the component to a placeholder.
Prefer sourcing the current account from auth state/props and rendering a fallback label. At minimum, remove the hardcoded email and show a safe placeholder when no account is available.
Suggestion
- Remove the hardcoded email from the effect and rely on props/context, with a UI fallback.
Example minimal change to avoid shipping a hardcoded email and blank label:
// Remove this effect entirely
// useEffect(() => {
// //Logic to get the Current Account
// setCurrentAccount('Amber@vibes.diy');
// }, []);
// And ensure the render uses a fallback label:
// <VibesButton variant="primary" onClick={() => setMode('accounts')}>
// {currentAccount || 'Accounts'}
// </VibesButton>Optionally, plumb currentAccount?: string via props instead of local state, or load it from your auth context/store in a separate responsible layer.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| <BrutalistCard | ||
| id="invite-status" | ||
| role="status" | ||
| aria-live="polite" | ||
| size="sm" | ||
| variant={ |
There was a problem hiding this comment.
id="invite-status" is static. If multiple VibesPanel instances are rendered on a page, this produces duplicate IDs, which can break document semantics and assistive tech expectations. Prefer using useId() to generate a stable, unique ID per instance.
Suggestion
Generate a per-instance status ID:
const statusId = useId();
...
<BrutalistCard
id={statusId}
role="status"
aria-live="polite"
...
>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
No description provided.