Shell updates 022626 - Ready for Eng Review#272
Conversation
petervachon
requested review from
0xr3ngar,
alincadariu,
angeloaltamiranom,
frankkluijtmans and
ruudandriessen
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { | ||
| CheckCircle, | ||
| Clock, | ||
| FileText, | ||
| AlertTriangle, | ||
| TrendingUp, | ||
| } from "lucide-react"; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix unused import issues, remove the unused symbol from the import list while keeping the rest of the import intact. This cleans up the code, avoids confusion about intended usage, and can slightly reduce bundle size.
Here, the best fix is to remove AlertTriangle from the destructuring import on line 3 of apps/apollo-vertex/app/(preview)/preview/shell/invoice-dashboard.tsx, leaving the other icons (CheckCircle, Clock, FileText, TrendingUp) unchanged. No other changes, imports, or definitions are required, since we are not altering any runtime functionality—only removing an unused name.
Concretely:
- Edit the import from
"lucide-react"at the top ofinvoice-dashboard.tsx. - Delete
AlertTriangle,from the import list. - Ensure formatting and commas remain valid for the remaining specifiers.
| @@ -4,7 +4,6 @@ | ||
| CheckCircle, | ||
| Clock, | ||
| FileText, | ||
| AlertTriangle, | ||
| TrendingUp, | ||
| } from "lucide-react"; | ||
| import { Badge } from "@/components/ui/badge"; |
| TooltipProvider, | ||
| TooltipTrigger, | ||
| } from "@/components/ui/tooltip"; | ||
| import { cn } from "@/lib/utils"; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, unused imports should be removed to improve readability and avoid confusion. They may also slightly impact build performance and can hide genuine issues if left around after refactors.
The best fix here is to delete the unused cn import from apps/apollo-vertex/registry/shell/shell-company.tsx. This does not change runtime behavior because cn is never referenced in the shown code, and removing an unused import is a no-op semantically. Concretely, you should edit the import section at the top of the file and remove the line import { cn } from "@/lib/utils";, leaving all other imports intact.
No additional methods, imports, or definitions are required to implement this change.
| @@ -10,7 +10,6 @@ | ||
| TooltipProvider, | ||
| TooltipTrigger, | ||
| } from "@/components/ui/tooltip"; | ||
| import { cn } from "@/lib/utils"; | ||
| import { useLocalStorage } from "@/registry/use-local-storage/use-local-storage"; | ||
| import type { CompanyLogo } from "./shell"; | ||
| import { |
| const { t, i18n } = useTranslation(); | ||
| const { user } = useUser(); | ||
| const { logout } = useAuth(); | ||
| const { theme, setTheme } = useTheme(); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix an unused variable from a destructuring assignment, remove the unused binding while keeping the ones that are actually used. This eliminates dead code and avoids future confusion without changing behavior.
Here, the fix is to stop destructuring theme from useTheme() and only destructure setTheme, which is the part that appears to be used. On line 55 in apps/apollo-vertex/registry/shell/shell-user-profile.tsx, change:
const { theme, setTheme } = useTheme();to:
const { setTheme } = useTheme();No additional imports, methods, or definitions are needed, because we’re only narrowing the destructured object to what is actually used.
| @@ -52,7 +52,7 @@ | ||
| const { t, i18n } = useTranslation(); | ||
| const { user } = useUser(); | ||
| const { logout } = useAuth(); | ||
| const { theme, setTheme } = useTheme(); | ||
| const { setTheme } = useTheme(); | ||
| const [language, setLanguageState] = useState(i18n.language); | ||
|
|
||
| const setLanguage = useCallback((code: string) => { |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { | ||
| CheckCircle, | ||
| Clock, | ||
| FileText, | ||
| AlertTriangle, | ||
| TrendingUp, | ||
| } from "lucide-react"; |
| TooltipProvider, | ||
| TooltipTrigger, | ||
| } from "@/components/ui/tooltip"; | ||
| import { cn } from "@/lib/utils"; |
| const { t, i18n } = useTranslation(); | ||
| const { user } = useUser(); | ||
| const { logout } = useAuth(); | ||
| const { theme, setTheme } = useTheme(); |
petervachon
changed the title
|
These shell, card, and color general theme updates are ready for Eng review |
There was a problem hiding this comment.
Other than the linting issues I noticed some small things in the shell UI:
User avatar has some misaligned circle around it, maybe it's an incorrect drop shadow?
Positioning of user menu is off. Would be better to align it at the bottom of the avatar in the simple shell layout.
The KPI cards have some extra vertical space. What's the exact purpose of that? I get that we might want a minimum height, but then let's use a slightly bigger font.
If you need some help looking at any of this, let me know 😄
| const MOCK_AUTH_CONTEXT: AuthContextValue = { | ||
| user: { name: "Dev User", email: "dev@localhost", sub: "dev-user-001" }, | ||
| isAuthenticated: true, | ||
| isLoading: false, | ||
| login: async () => {}, | ||
| logout: () => {}, | ||
| accessToken: "bypass-token", | ||
| }; | ||
|
|
||
| const MockAuthProvider: FC<PropsWithChildren> = ({ children }) => ( | ||
| <AuthContext.Provider value={MOCK_AUTH_CONTEXT}>{children}</AuthContext.Provider> | ||
| ); | ||
|
|
||
| type ApolloShellProps = ApolloShellComponentProps & | ||
| ( | ||
| | { bypassAuth: true; clientId?: string; scope?: string; baseUrl?: string } | ||
| | { bypassAuth?: false; clientId: string; scope: string; baseUrl: string } | ||
| ); |
There was a problem hiding this comment.
do we need to bypass auth? we can add people in the org/app that the auth uses.
i think @ruudandriessen did this already with you @petervachon already 🤔
3 participants
Currently under review