Skip to content

Replace dual-bar with queue bridge for flash-free bottom bar#842

Open
marcodejongh wants to merge 3 commits intomainfrom
better_global_queue_control_bar
Open

Replace dual-bar with queue bridge for flash-free bottom bar#842
marcodejongh wants to merge 3 commits intomainfrom
better_global_queue_control_bar

Conversation

@marcodejongh
Copy link
Owner

Summary

  • Replaces the dual-bar architecture (root-level + board-route-level bottom bars with useEffect-based registration toggling) with a single persistent bottom bar at the root level
  • Introduces a "queue bridge" context that reads queue state from whichever provider is active — the board route's GraphQLQueueProvider when on board pages, or a thin PersistentSession adapter on non-board pages
  • Uses useLayoutEffect for synchronous injection/cleanup, eliminating the flash caused by timing gaps during route transitions

Key changes

  • New: queue-bridge-context.tsxQueueBridgeProvider, QueueBridgeInjector, and usePersistentSessionQueueAdapter
  • Modified: persistent-session-wrapper.tsx — always renders bottom bar, wraps with necessary providers
  • Modified: Both board route layouts — removed bottom bar rendering, added QueueBridgeInjector
  • Modified: queue-control-bar.tsx — guarded useParams() fallback for root-level rendering
  • Unified: bottom-bar-wrapper.module.css — merged board-route styling (mobile inset, border-radius)
  • Deleted: board-route-bottom-bar-context.tsx, persistent-queue-control-bar.tsx, both layout.module.css

Test plan

  • Navigate home → board route → home: bar never flashes or unmounts
  • Navigate between board route pages (list → view → play): bar content updates smoothly
  • Navigate between different board configs: bar shell stays, content updates
  • Queue operations from bar (swipe next/prev, clear, mirror): work on and off board routes
  • Party mode: bar shows party queue, share button works
  • Local mode: bar shows local queue, persists after navigation
  • BottomTabBar tabs work correctly on all pages

🤖 Generated with Claude Code

…tion

The bottom bar previously used two separate instances (root-level and
board-route-level) with useEffect-based registration toggling between
them, causing visible flashing during route transitions. This replaces
that with a single persistent bottom bar at the root level that uses a
"bridge" context to read queue state from whichever provider is active.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Feb 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
boardsesh Building Building Preview, Comment Feb 14, 2026 11:12am

Request Review

@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@claude
Copy link

claude bot commented Feb 14, 2026

Claude Review

Ready to merge - Minor issues noted below, but nothing blocking.

Issues

  1. queue-bridge-context.tsx:331 - QueueBridgeInjector reads QueueContext with useContext but at this point in the component tree, the context is from the QueueBridgeProvider above, not the GraphQLQueueProvider from the board route. The QueueBridgeInjector is placed inside UISearchParamsProvider but outside GraphQLQueueProvider would mean it reads the bridge's own context, not the board route's. However, looking at the layouts, QueueBridgeInjector is placed inside the GraphQLQueueProvider (since GraphQLQueueProvider wraps the entire board route content), so this should work correctly. Just verify the provider ordering in testing.

  2. queue-bridge-context.tsx:63-66 - Non-null assertion (ps.activeSession!) is used after a truthy check (isParty). This is technically safe but TypeScript doesn't narrow across the const assignments. Consider using optional chaining for consistency or restructuring to avoid the assertion.

  3. queue-control-bar.tsx:117-120 - The buildClimbUrl function now returns null when params.board_name is falsy, but callers don't always check for this before using the result. The null check at line 119-120 only applies to the inner function logic, but the handlers at lines 143 and 159 correctly check. However, line 180-195 also needs to handle the case, which it does at line 194. This is fine but the control flow is a bit fragmented.

  4. Test coverage - No tests added for queue-bridge-context.tsx. Given this is a significant architectural change with complex context injection logic and useLayoutEffect timing, tests for the bridge behavior would help prevent regressions.

Documentation

No documentation updates required - the bottom bar architecture is not currently documented in docs/.

The backend reads NEXTAUTH_SECRET to validate NextAuth JWTs but had no
checked-in env file providing it. Creates .env.development (not
gitignored) with the same dummy secret used by the web package, and
updates the dev script to load it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 14, 2026

Claude Review

Ready to merge - Minor issues noted below, but nothing blocking.

Issues

  1. Missing dependency in useCallback - queue-bridge-context.tsx:141: The setQueue callback is missing queue in its dependency array. It references queue implicitly via currentClimbQueueItem check but should include it to be safe since the logic does reference the queue for finding current item.

  2. useLayoutEffect SSR warning - queue-bridge-context.tsx:334: useLayoutEffect will emit a warning during SSR. While this component is a client component, consider adding a guard or using a custom useIsomorphicLayoutEffect if SSR warnings become noisy.

  3. QueueBridgeInjector placed before GraphQLQueueProvider - In both layouts (layout.tsx:179 and b/[board_slug]/[angle]/layout.tsx:90), QueueBridgeInjector is rendered inside UISearchParamsProvider but the QueueContext it reads comes from the outer GraphQLQueueProvider. This works but the placement is confusing - consider moving it to immediately after GraphQLQueueProvider for clarity.

Test Coverage

No tests added for the new queue-bridge-context.tsx (349 lines). The queue bridge pattern involves complex state synchronization and would benefit from unit tests for:

  • Context injection/cleanup lifecycle
  • Fallback to persistent session adapter when no board route is active
  • Queue operations through the adapter

The useLayoutEffect cleanup was setting injectedContext to null on every
queue context change (since queueContext was in the dep array), then
the setup would set it back. This cleanup/setup cycle could cause the
bridge to briefly see null state, hiding the bar.

Fix: use a ref for the context value (no cleanup needed on updates),
split injection into initial mount (useLayoutEffect) and context
updates (useEffect), and only clear on unmount.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 14, 2026

Claude Review

Ready to merge - Minor issues noted below, but nothing blocking.

Issues

  1. Security: Hardcoded secret committed - packages/backend/.env.development:9 contains NEXTAUTH_SECRET=68cJgCDE39gaXwi8LTVW4WioyhGxwcAd. While the comment says it's not production, hardcoded secrets in git can accidentally leak to production or be harvested by automated scanners. Consider using a placeholder value or documenting that this must be replaced.

  2. Potential race condition in QueueBridgeInjector - packages/web/app/components/queue-control/queue-bridge-context.tsx:365-377: The useLayoutEffect depends on queueContext which comes from useContext(QueueContext), but the effect's dependency array excludes queueContext. If the initial queueContext is null/different than expected on first render, the injection might not occur. The separate useEffect at line 381 mitigates this, but the initial injection could miss the first context value.

  3. Nullable URL not handled in event handler - packages/web/app/components/queue-control/queue-control-bar.tsx:180-213: The buildClimbUrl function can now return null, but when used in the handleClimbClick event handler (line 227), there's no null check before calling router.push(url). The early returns at lines 170 and 212 help, but the function's return type should be explicitly typed to ensure all call sites are checked.

  4. Missing tests - The new queue-bridge-context.tsx file (388 lines) contains complex state management logic (context bridging, injection lifecycle, adapter pattern) but has no corresponding tests. This is a significant architectural change that would benefit from unit tests for the bridge provider, injector lifecycle, and adapter functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant