-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: tweak login UI #6563
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
feat: tweak login UI #6563
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to 8c13b9d in 2 minutes and 21 seconds. Click for details.
- Reviewed
215lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/components/ui/dropdown-menu.tsx:44
- Draft comment:
Changed z-index from z-50 to z-[51] in DropdownMenuContent. Verify that this stacking adjustment doesn’t interfere with other UI overlays. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/components/ui/dropdown-menu.tsx:232
- Draft comment:
Updated z-index to z-[51] for DropdownMenuSubContent. Ensure the new layering behaves as expected in all cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/containers/LeftPanel.tsx:436
- Draft comment:
A divider element was added in the authentication menu section to improve visual separation. Confirm that its styling is consistent with the design system. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/containers/auth/AuthLoginButton.tsx:31
- Draft comment:
The panel width update logic uses 'document.querySelector("aside[ref]")', which may not reliably select the intended element since React refs aren’t rendered as DOM attributes. Consider using a dedicated data attribute or ref callback for more reliable selection. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment correctly identifies an issue with the first selector, the code already has fallback mechanisms that will work. The 'aside[ref]' selector will simply fail silently and move to the next option. The code will still work reliably through its fallbacks. The comment suggests using a data attribute, but that would just be a style preference since the current code already handles failure cases. The code could be simplified by removing the unreliable selector entirely. Why have a selector that we know won't work? While removing the selector would be cleaner, its presence doesn't affect functionality due to the fallbacks. The comment is suggesting a refactor that isn't strictly necessary. The comment identifies a real issue but the code already handles it gracefully through fallbacks. The suggested change would be a minor optimization at best.
Workflow ID: wflow_qK2RmbP9HaaJsZa5
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 4ba99e1 in 2 minutes and 2 seconds. Click for details.
- Reviewed
171lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/LeftPanel.tsx:141
- Draft comment:
Including 'open' in the useEffect dependency array ensures the latest state is used in handleResize. Double-check that reattaching the resize listener on every 'open' change is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check their intention regarding the useEffect dependency array. This violates the rule against asking the author to confirm their intention or to double-check things. Therefore, this comment should not be approved.
2. web-app/src/containers/auth/UserProfileMenu.tsx:15
- Draft comment:
Unused imports (Button and IconChevronDown) were removed. Confirm that these removals align with the updated UI requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/hooks/useThreadScrolling.tsx:165
- Draft comment:
The debounced scroll handler (debouncedScroll) is added without canceling any pending invocations on unmount. Consider calling debouncedScroll.cancel() in the cleanup to avoid potential state updates after unmount. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. web-app/src/hooks/useThreadScrolling.tsx:144
- Draft comment:
The logic in handleScroll and handleDOMScroll is nearly identical. Consider refactoring them into a single shared scroll handler to reduce duplication and potential maintenance issues. - Reason this comment was not posted:
Comment was on unchanged code.
5. web-app/src/hooks/useThreadScrolling.tsx:167
- Draft comment:
The debouncedScroll event listener is attached with an empty dependency array. Verify that the function’s closure does not capture stale state, which might affect its behavior over time. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. web-app/src/providers/AuthProvider.tsx:31
- Draft comment:
Wrapping fetchUserData and resetAppData in useCallback improves dependency management. Ensure that all necessary dependencies are included and that their stability is maintained. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_NGc2NGtCsoMnpI0c
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
* ✨ feat: Re-arrange docs as needed * 🔧 chore: re-arrange the folder structure * Add server docs Add server docs * enhancement: migrate handbook and janv2 * Update docs/src/components/ui/dropdown-button.tsx Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * Update docs/src/pages/_meta.json Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * chore: update feedback #1 * fix: layout ability model * feat: add azure as first class provider (#6555) * feat: add azure as first class provider * fix: deployment url * Update handbook: restructure content and add new sections - Add betting-on-open-source.mdx and open-superintelligence.mdx - Update handbook index with new structure - Remove outdated handbook sections (growth, happy, history, money, talent, teams, users, why) - Update handbook _meta.json to reflect new structure * chore: fix meta data json * chore: update missing install * fix: Catch local API server various errors (#6548) * fix: Catch local API server various errors * chore: Add tests to cover error catches * fix: LocalAPI server trusted host should accept asterisk (#6551) * feat: support .zip archives for manual backend install (#6534) * feat(llamacpp): support .zip archives for manual backend install * Update Lock Files * Merge pull request #6563 from menloresearch/feat/web-minor-ui-tweak-login feat: tweak login UI --------- Co-authored-by: LazyYuuki <[email protected]> Co-authored-by: nngostuds <[email protected]> Co-authored-by: Faisal Amir <[email protected]> Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Louis <[email protected]> Co-authored-by: eckartal <[email protected]> Co-authored-by: Nghia Doan <[email protected]> Co-authored-by: Roushan Kumar Singh <[email protected]>
Describe Your Changes
Fixes Issues
Self Checklist
Important
Enhance login UI with improved dropdown menu styling and responsive design adjustments.
z-indexinDropdownMenuContentandDropdownMenuSubContenttoz-[51]indropdown-menu.tsxfor better layering.LeftPanelfor authentication menu.ResizeObserverinAuthLoginButtonandUserProfileMenuto adjust dropdown width based onasidepanel width.useEffectinLeftPanel.tsxto handle panel open/close on screen resize.useCallbackforscrollToBottomandhandleScrollinuseThreadScrolling.tsxfor better performance.useCallbackforfetchUserDataandresetAppDatainAuthProvider.tsxto optimize re-renders.This description was created by
for 4ba99e1. You can customize this summary. It will automatically update as commits are pushed.