feat(ui): mobile UI overhaul — tab bar, design tokens, layout fixes#190
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 8 issue(s) (2 warning).
frontend/src/components/MobileShell.tsx
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🔵 style (L11): Uses
React.ReactNodewithout importing React. Other new components in this PR (EmptyState,PageHeader) useimport type { ReactNode } from 'react'— this should be consistent. Works due toreact-jsxtransform but inconsistent with PR's own convention.[fixable] - 🔵 missing_tests: No tests for
MobileShell— specifically theshouldHideTabBarlogic (hides on/login,/chat,/chat/:id, shows on other routes). This is the core routing logic for tab bar visibility.
frontend/src/components/TabBar.tsx
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🔵 style (L20): Redundant
useIsDesktopguard —MobileShellalready prevents renderingTabBaron desktop. The double-check is harmless but unnecessary sinceTabBaris only ever rendered insideMobileShell.[fixable]
frontend/src/hooks/useSessionList.ts
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🟡 regressions:
useSessionListstill fetches inbox/todo counts and subscribes toinbox_updatedWS events, butSessionList.tsxno longer destructuresinboxCount/todoCount. This duplicates the same fetch+subscribe logic now inuseTabBadges. Every page usinguseSessionListpays for two parallel fetches to/api/inboxand/api/todos(one fromuseSessionList, one fromuseTabBadgesviaTabBar). Consider removing the dead badge logic fromuseSessionList.[fixable]
frontend/src/styles/global.css
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🔵 style: New
.session-status-dot,.session-status-dot.attached, and.session-status-dot.detachedCSS classes are added but not referenced by any component in the diff or elsewhere in the codebase. These appear to be orphaned styles shipped ahead of usage.[fixable] - 🔵 style:
.session-status-dot.attacheduses#34c759and.detacheduses#ff9f0a— these are hardcoded colors not using design token variables, inconsistent with this PR's goal of replacing hardcoded colors with CSS vars. Consider adding--success-alt/--warning-alttokens or reusing--success/--warning.[fixable]
frontend/src/hooks/useTabBadges.ts
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🟡 unsafe_assumptions (L24):
todoCountis set totodoData.items.lengthwhich counts all items returned by/api/todos, including completed ones. If the intent is to badge only active/pending todos, this may over-count. Verify whether the API already filters or whether the hook should filter by status.[fixable]
frontend/src/components/__tests__/TabBar.test.tsx
Solid UI refactor — design tokens, tab bar, and EmptyState are well-structured with good tests. Main concerns: duplicated badge-fetching logic between useTabBadges and useSessionList, orphaned CSS for session-status-dot, and missing MobileShell tests.
- 🔵 missing_tests: No test for the 'More' menu behavior — opening the overlay, navigating to Calendar/Files, and closing on overlay click. The More menu has non-trivial state (toggle open, stopPropagation, navigate+close).
| return HIDE_TAB_BAR.some((p) => pathname === p || pathname.startsWith(p + '/')); | ||
| } | ||
|
|
||
| export function MobileShell({ children }: { children: React.ReactNode }) { |
There was a problem hiding this comment.
🔵 style: Uses React.ReactNode without importing React. Other new components in this PR (EmptyState, PageHeader) use import type { ReactNode } from 'react' — this should be consistent. Works due to react-jsx transform but inconsistent with PR's own convention. [fixable]
| const isDesktop = useIsDesktop(); | ||
| const [moreOpen, setMoreOpen] = useState(false); | ||
|
|
||
| if (isDesktop) return null; |
There was a problem hiding this comment.
🔵 style: Redundant useIsDesktop guard — MobileShell already prevents rendering TabBar on desktop. The double-check is harmless but unnecessary since TabBar is only ever rendered inside MobileShell. [fixable]
| .catch(() => ({ items: [] })), | ||
| ]).then(([inboxData, todoData]) => { | ||
| if (Array.isArray(inboxData)) setInboxCount(inboxData.length); | ||
| if (todoData?.items) setTodoCount(todoData.items.length); |
There was a problem hiding this comment.
🟡 unsafe_assumptions: todoCount is set to todoData.items.length which counts all items returned by /api/todos, including completed ones. If the intent is to badge only active/pending todos, this may over-count. Verify whether the API already filters or whether the hook should filter by status. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (2 warning).
frontend/src/styles/global.css
Clean extraction of badge logic and solid new components; main issues are orphaned CSS from the removed inbox nav buttons and missing response.ok validation in useTabBadges.
- 🟡 regressions: Orphaned CSS:
.inbox-nav-btn,.inbox-nav-btn:active,.inbox-nav-label, and.inbox-nav-badgerules remain in global.css but the HTML elements that used them were removed from SessionList.tsx in this PR. These should be cleaned up.[fixable]
frontend/src/components/MobileShell.tsx
Clean extraction of badge logic and solid new components; main issues are orphaned CSS from the removed inbox nav buttons and missing response.ok validation in useTabBadges.
- 🔵 missing_tests: MobileShell has no test file. It contains routing-aware logic (
shouldHideTabBar) and the desktop guard — worth a small test to verify the tab bar is hidden on /login, /chat/*, and on desktop viewports.[fixable]
frontend/src/hooks/useTabBadges.ts
Clean extraction of badge logic and solid new components; main issues are orphaned CSS from the removed inbox nav buttons and missing response.ok validation in useTabBadges.
- 🟡 bugs (L16): Neither the inbox nor todos fetch checks
response.okbefore calling.json(). A 4xx/5xx response will attempt JSON parse on an error body, which could set incorrect counts (e.g. an HTML error page would throw). The old code in useSessionList had the same pattern, but since this is a fresh extraction it's a good time to add.then(r => { if (!r.ok) throw new Error(); return r.json(); }).[fixable]
frontend/src/components/TabBar.tsx
Clean extraction of badge logic and solid new components; main issues are orphaned CSS from the removed inbox nav buttons and missing response.ok validation in useTabBadges.
- 🔵 style (L20): TabBar already has an
if (isDesktop) return nullguard, and MobileShell also checks!isDesktopbefore rendering TabBar. The double-guard is harmless but redundant — consider removing one to keep a single source of truth for the mobile-only constraint.[fixable]
frontend/src/hooks/__tests__/useTabBadges.test.ts
Clean extraction of badge logic and solid new components; main issues are orphaned CSS from the removed inbox nav buttons and missing response.ok validation in useTabBadges.
- 🔵 missing_tests: The test doesn't cover the WebSocket
inbox_updatedsubscription path (debounced re-fetch). A test confirming that a WS message triggers a re-fetch with the 300ms debounce would strengthen coverage of the moved logic.[fixable]
| useEffect(() => { | ||
| const fetchCounts = () => | ||
| Promise.all([ | ||
| fetch('/api/inbox') |
There was a problem hiding this comment.
🟡 bugs: Neither the inbox nor todos fetch checks response.ok before calling .json(). A 4xx/5xx response will attempt JSON parse on an error body, which could set incorrect counts (e.g. an HTML error page would throw). The old code in useSessionList had the same pattern, but since this is a fresh extraction it's a good time to add .then(r => { if (!r.ok) throw new Error(); return r.json(); }). [fixable]
| const isDesktop = useIsDesktop(); | ||
| const [moreOpen, setMoreOpen] = useState(false); | ||
|
|
||
| if (isDesktop) return null; |
There was a problem hiding this comment.
🔵 style: TabBar already has an if (isDesktop) return null guard, and MobileShell also checks !isDesktop before rendering TabBar. The double-guard is harmless but redundant — consider removing one to keep a single source of truth for the mobile-only constraint. [fixable]
…ce hardcoded colors Five CSS variables were referenced but never defined (--bg-secondary, --text-secondary, --hover, --active, --warning). Adds type scale (--text-xs through --text-xl) and spacing scale (--space-1 through --space-6). Replaces all hardcoded #e53935 and #ff9800 with var refs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…colors - Define missing CSS vars (--bg-secondary, --text-secondary, --hover, --active, --warning) - Add type scale (--text-xs through --text-xl) and spacing scale (--space-1 through --space-6) - Replace hardcoded #e53935 and #ff9800 with var(--danger) and var(--warning) - Add reusable PageHeader component with back button, title, badge, center slot, and action slot Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract useTabBadges hook for shared inbox/todo counts - Add TabBar component with Chat, Tasks, Inbox, Todos, More tabs - Badge counts on Inbox and Todos tabs - More menu opens slide-up sheet with Calendar and Files - Hidden on desktop and chat routes via MobileShell wrapper - Remove old inline Inbox/Todos nav buttons from SessionList - Add bottom padding to page containers for tab bar clearance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Stack loop controls vertically on mobile (goal select on row 1, spec toggle + Start on row 2) to prevent overlap - Add reusable EmptyState component (icon + title + subtitle) - Adopt EmptyState in TaskBoard, InboxView, TodoView, SessionList Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The tab bar now handles inbox/todo badge counts via useTabBadges, so useSessionList no longer needs to fetch those counts or subscribe to inbox_updated WebSocket events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
useTabBadges was counting all todo items including completed ones. Now only non-completed items contribute to the badge count. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
force-pushed
the
feat/mobile-ui-overhaul
branch
from
721d466 to
0f0d2b3
Compare
1 participant
Summary
--bg-secondary,--text-secondary,--hover,--active,--warning), added type scale (--text-xsthrough--text-xl) and spacing scale (--space-1through--space-6). Replaced all hardcoded#e53935and#ff9800with CSS var refs.Remaining (follow-up PRs)
var(--space-N), map font sizes to type scale)Test plan
🤖 Generated with Claude Code