refactor(ui): improve flow onboarding with first workspace creation and re-design settings ui #299
refactor(ui): improve flow onboarding with first workspace creation and re-design settings ui #299
Conversation
Summary of ChangesHello, 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! This pull request significantly refactors the application's user interface and core workflows, focusing on improving the onboarding experience for new users and streamlining the settings management. By separating layout concerns, redesigning the settings pages with a grouped navigation, and introducing a mandatory workspace creation step, the changes aim to provide a more intuitive, organized, and efficient user journey. Additionally, API responses for workspaces now include more comprehensive data, supporting the enhanced UI. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the UI, focusing on the user onboarding flow by forcing workspace creation, and redesigning the settings pages. The introduction of RequireWorkspace to protect routes, the new dedicated page for workspace creation, and the modularization of the settings UI into a new SettingsLayout with smaller components are all excellent changes that improve code structure and user experience. I've found a few issues to address: a layout problem in the header for users without a workspace, an invalid HTML structure in the new settings navigation menu, and a potential memory leak from an uncleaned setTimeout. I've provided suggestions to address these points. Overall, this is a great contribution that enhances the application's usability and maintainability.
Note: Security Review did not run due to the size of the PR.
| {settingsTabGroups.map((group, groupIndex) => ( | ||
| <div> | ||
| <li key={group.name} className="p-2"> | ||
| {/* Group header */} | ||
| <div className="text-xs font-semibold text-muted-foreground uppercase mb-2 tracking-wider px-3"> | ||
| {group.name} | ||
| </div> | ||
| {/* Tabs in this group */} | ||
| {group.tabs.map((tab) => ( | ||
| <Link | ||
| key={tab.id} | ||
| to={tab.path} | ||
| className={`block px-3 py-1 rounded-md text-sm font-medium transition-colors mb-1 last:mb-0 ${ | ||
| isActive(tab.path) | ||
| ? 'bg-accent text-accent-foreground' | ||
| : 'text-muted-foreground hover:bg-accent hover:text-accent-foreground' | ||
| }`} | ||
| > | ||
| {tab.label} | ||
| </Link> | ||
| ))} | ||
| </li> | ||
| {/* Group divider - shown after group */} | ||
| {groupIndex < settingsTabGroups.length - 1 && ( | ||
| <div className="border-b my-2" /> | ||
| )} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
The list of setting groups has two issues:
- The root
<div>element inside themapat line 25 is missing akeyprop. - The structure
<ul> > <div> > <li>is invalid HTML. A<li>element must be a direct child of a<ul>. The same applies to the dividerdivat line 48.
This can lead to unexpected rendering behavior and accessibility issues. Refactoring to use React.Fragment and ensuring <li> elements are direct children of <ul> will fix this.
{settingsTabGroups.map((group, groupIndex) => (
<React.Fragment key={group.name}>
<li className="p-2">
{/* Group header */}
<div className="text-xs font-semibold text-muted-foreground uppercase mb-2 tracking-wider px-3">
{group.name}
</div>
{/* Tabs in this group */}
{group.tabs.map((tab) => (
<Link
key={tab.id}
to={tab.path}
className={`block px-3 py-1 rounded-md text-sm font-medium transition-colors mb-1 last:mb-0 ${
isActive(tab.path)
? 'bg-accent text-accent-foreground'
: 'text-muted-foreground hover:bg-accent hover:text-accent-foreground'
}`}
>
{tab.label}
</Link>
))}
</li>
{/* Group divider - shown after group */}
{groupIndex < settingsTabGroups.length - 1 && (
<li className="px-3">
<div className="border-b my-2" />
</li>
)}
</React.Fragment>
))}
| <div className="flex justify-end w-full"> | ||
| <AppLogo type="small" /> | ||
| <div className="ml-auto"> | ||
| <NavUser isOnlyAvatar dropdownSide="bottom" /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The layout for the header when no workspace exists seems incorrect. The combination of justify-end on the parent div and ml-auto on a child div is confusing and will likely result in both the logo and the user avatar being pushed to the right side of the header. For a more standard and consistent layout with the logo on the left and user navigation on the right, you could simplify the structure to be consistent with the other branch of this conditional rendering.
<div className="flex w-full items-center justify-between">
<AppLogo type="small" />
<NavUser isOnlyAvatar dropdownSide="bottom" />
</div>
| await navigator.clipboard.writeText(apiKeyData.apiKey); | ||
| setCopied(true); | ||
| toast.success('API key copied to clipboard'); | ||
| setTimeout(() => setCopied(false), 2000); |
There was a problem hiding this comment.
Using setTimeout without cleanup can lead to memory leaks if the component unmounts before the timeout completes. It's safer to use a useEffect hook to manage the timer's lifecycle. This will ensure the timer is cleared if the component unmounts or if copied state changes again before the timeout finishes.
Here's how you could refactor it:
// At the top level of your component, after state declarations
useEffect(() => {
if (copied) {
const timer = setTimeout(() => {
setCopied(false);
}, 2000);
return () => clearTimeout(timer);
}
}, [copied]);
// And in handleCopy, you would just set the state:
const handleCopy = async () => {
if (apiKeyData?.apiKey) {
await navigator.clipboard.writeText(apiKeyData.apiKey);
setCopied(true);
toast.success('API key copied to clipboard');
}
};…nd re-design settings ui (#299) * feat(console): add all workspaces navigation and improve 404 page UI * refactor(layout): extract header into dedicated HeaderBar component * refactor(console): add workspace-aware header layout * refactor(console): convert workspace creation to page and add route protection * refactor(console): update workspaces UI from table to card layout * feat(workspaces): add member and target counts to workspace list * refactor(settings): reorganize settings page with sidebar layout * feat(settings): add API keys management * refactor(settings): improve API key display layout * fix(screenshot-cell): add type assertion for screenshotPath * refactor(workspaces): use workspace selector hook
* feat(console, admin): add list users for administration * feat(console): redesign user detail sheet * fix(console): fix sorting for server datatable * refactor(console): restyle user-detail-sheet * feat(console): add confirm dialog for action ban * feat(console): add user detail section * fix(console): fix small typo in tls tab (#290) * feat(asset): add tls filter for asset * fix(core): fix asset test * fix(asset): fix based on bot reviews * fix(console): fix small typo in tls tab * fix(console): add missing tls for queryParams in asset context * fix(console): fix tls query hook in dashboard --------- Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * chore(deps): bump multer from 2.0.2 to 2.1.0 (#292) Bumps [multer](https://github.com/expressjs/multer) from 2.0.2 to 2.1.0. - [Release notes](https://github.com/expressjs/multer/releases) - [Changelog](https://github.com/expressjs/multer/blob/main/CHANGELOG.md) - [Commits](expressjs/multer@v2.0.2...v2.1.0) --- updated-dependencies: - dependency-name: multer dependency-version: 2.1.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(targets): create multiple targets (#291) * refactor(targets): combine logic create target in transaction * feat(targets): add bulk target creation endpoint * feat(targets): add bulk creation support * fix(console): move tabList into component to avoid out of context (#293) Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * fix(assets): select asset relations in query (#297) * refactor(ui): improve flow onboarding with first workspace creation and re-design settings ui (#299) * feat(console): add all workspaces navigation and improve 404 page UI * refactor(layout): extract header into dedicated HeaderBar component * refactor(console): add workspace-aware header layout * refactor(console): convert workspace creation to page and add route protection * refactor(console): update workspaces UI from table to card layout * feat(workspaces): add member and target counts to workspace list * refactor(settings): reorganize settings page with sidebar layout * feat(settings): add API keys management * refactor(settings): improve API key display layout * fix(screenshot-cell): add type assertion for screenshotPath * refactor(workspaces): use workspace selector hook * feat(auth): add session retry with exponential backoff * chore(agent): migrate ai agent * feat(router): add admin users route * feat(console): implement create user * feat(console): add change name, email and reset password in user detail * fix(console): fix duplicate tlsHosts in context * fix(console): use loading state of data table and improve client user type * fix(console): add admin route * feat(console): Implement role-based access control for settings tabs and sidebar menu items based on user roles. * style(console): update 'Add User' button to outline variant * refactor(console): move add user button to table toolbar * fix(console): add autoComplete to user detail input --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: l1ttps <l1ttps443@gmail.com>
No description provided.