feat: enable browser back-button navigation within the SPA dashboard#44
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBrowser back-button navigation is enabled unconditionally across the SPA dashboard: view changes and modal opens now push navigation entries so the back button dismisses the top modal or reverts the prior view instead of leaving the dashboard. ChangesBack-Button Navigation for SPA Dashboard
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant App
participant NavHistory as Navigation History
participant Modal as Modal Manager
User->>App: open modal / change view
App->>NavHistory: pushNav({type, revert})
NavHistory->>Browser: ensure popstate listener registered
User->>Browser: press Back
Browser->>NavHistory: popstate
NavHistory->>NavHistory: lookup entry & run revert
NavHistory->>Modal: invoke revert (close modal or revert view)
Modal->>App: update UI state
App->>User: prior view / modal dismissed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR enables browser back-button navigation across all viewports by flipping
Confidence Score: 5/5Safe to merge — the core navigation change is well-scoped, the hook logic is unchanged, and the updated tests correctly validate the new unified desktop+mobile behavior. The change is a straightforward flag flip with mechanical removal of ternaries. The underlying hook behavior is unmodified,
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant App as App.tsx
participant Nav as useNavigationHistory
participant History as window.history
participant Modal as modalManager
Note over App,Nav: enabled: true (desktop + mobile)
User->>App: clicks "Open Settings"
App->>Modal: openSettings(section)
App->>Nav: pushNav({ type: "modal", close: closeSettings })
Nav->>History: pushState({ navIndex: N })
User->>History: presses Back button
History->>Nav: popstate event (navIndex: N-1)
Nav->>Modal: entry.close() → closeSettings()
Modal-->>User: Settings modal dismissed
Reviews (8): Last reviewed commit: "fix: fix flaky PlanningModeModal test — ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -226,8 +226,8 @@ function AppInner() { | |||
| const viewportMode = useViewportMode(); | |||
| const isMobile = viewportMode === "mobile"; | |||
There was a problem hiding this comment.
isMobile is now a dead variable
All isMobile ? ... : ... ternaries were removed by this PR, leaving isMobile (and potentially viewportMode / the useViewportMode() call) without any remaining consumers. This is dead code and will fail a build with noUnusedLocals: true.
| const isMobile = viewportMode === "mobile"; | |
| const viewportMode = useViewportMode(); |
If viewportMode is also no longer used elsewhere in this component, the useViewportMode() call can be removed entirely as well.
There was a problem hiding this comment.
Good catch to check, but isMobile is still actively used in 4 places: keyboard handling (line 327), mobile keyboard open guard (line 334), MobileNavBar mobileNavEnabled prop (line 1194), and project-content CSS class (line 1256). It's not dead code — only the isMobile ? historyHandler : plainHandler ternaries were removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/app/hooks/useNavigationHistory.ts (1)
18-21: 💤 Low valueStale documentation: interface comment still says "Only active on mobile".
The interface comment at line 19 contradicts the updated function JSDoc (lines 34-36). Consider updating to reflect the new cross-platform behavior.
📝 Suggested documentation fix
export interface UseNavigationHistoryOptions { - /** Only active on mobile. When false, pushNav/replaceCurrent are no-ops. */ + /** When false, pushNav/replaceCurrent are no-ops and no popstate listener is registered. */ enabled: boolean; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/hooks/useNavigationHistory.ts` around lines 18 - 21, The interface comment on UseNavigationHistoryOptions is stale — it currently reads "Only active on mobile" but the hook now works cross-platform; update the JSDoc for the UseNavigationHistoryOptions interface (symbol: UseNavigationHistoryOptions) to reflect the new behavior (e.g., remove "Only active on mobile" and state that enabled toggles history tracking across platforms or similar), keeping wording consistent with the hook's JSDoc at lines ~34-36.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/dashboard/app/hooks/useNavigationHistory.ts`:
- Around line 18-21: The interface comment on UseNavigationHistoryOptions is
stale — it currently reads "Only active on mobile" but the hook now works
cross-platform; update the JSDoc for the UseNavigationHistoryOptions interface
(symbol: UseNavigationHistoryOptions) to reflect the new behavior (e.g., remove
"Only active on mobile" and state that enabled toggles history tracking across
platforms or similar), keeping wording consistent with the hook's JSDoc at lines
~34-36.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97046b9c-7eb2-4f33-9a84-0fd52f30d917
📒 Files selected for processing (3)
.changeset/browser-back-button-spa-integration.mdpackages/dashboard/app/App.tsxpackages/dashboard/app/hooks/useNavigationHistory.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/__tests__/navigation-history.test.tsx (1)
422-424:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid silent-pass branches in integration tests.
if (!agentsTab) return;can let the test pass without validating behavior. Fail fast (orfindByTitle) so missing tab rendering is caught as a regression.Suggested tightening
- const agentsTab = screen.queryByTitle("Agents"); - if (!agentsTab) return; + const agentsTab = await screen.findByTitle("Agents"); fireEvent.click(agentsTab);Also applies to: 442-444
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/__tests__/navigation-history.test.tsx` around lines 422 - 424, The test currently uses queryByTitle("Agents") and returns silently if agentsTab is missing (agentsTab variable), which can hide regressions; change the flow to fail fast by awaiting screen.findByTitle("Agents") (or assert presence with expect(screen.getByTitle("Agents")).toBeInTheDocument()) before firing the click, and apply the same change for the identical pattern at the other occurrence (lines around the second agentsTab usage) so missing tab rendering causes the test to fail rather than pass silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/app/components/__tests__/navigation-history.test.tsx`:
- Around line 389-391: The test currently asserts window.history.pushState was
called at some point which can be triggered by unrelated startup code; before
simulating the user click in the "Back-button nav is enabled on desktop" test
(and likewise in the "Agents view-change" test), record the current call count
of window.history.pushState (e.g., via const before = (window.history.pushState
as jest.Mock).mock.calls.length) then perform the click and assert the call
count increased (expect((window.history.pushState as
jest.Mock).mock.calls.length).toBeGreaterThan(before)); update the assertions in
navigation-history.test.tsx to use this action-specific pattern so you only
detect calls caused by the user action.
---
Outside diff comments:
In `@packages/dashboard/app/components/__tests__/navigation-history.test.tsx`:
- Around line 422-424: The test currently uses queryByTitle("Agents") and
returns silently if agentsTab is missing (agentsTab variable), which can hide
regressions; change the flow to fail fast by awaiting
screen.findByTitle("Agents") (or assert presence with
expect(screen.getByTitle("Agents")).toBeInTheDocument()) before firing the
click, and apply the same change for the identical pattern at the other
occurrence (lines around the second agentsTab usage) so missing tab rendering
causes the test to fail rather than pass silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cb8fc39-812b-426b-8de4-cfcb54c89188
📒 Files selected for processing (1)
packages/dashboard/app/components/__tests__/navigation-history.test.tsx
|
The failing test |
|
@copilot resolve the merge conflicts in this pull request |
|
Typecheck issue - Error: packages/dashboard typecheck: app/App.tsx(879,76): error TS2304: Cannot find name 'openDetailTaskWithHistory'. |
…shboard Previously the back button left the dashboard page entirely because useNavigationHistory was only enabled for mobile viewports. Now it's enabled for all viewports — desktop and mobile — so the browser back button dismisses modals and reverts view changes within the SPA. Changes: - Enable useNavigationHistory for desktop (enabled: true instead of enabled: isMobile) - Remove all isMobile ? historyAwareHandler : plainHandler ternaries - Always use navigation-aware handlers (openDetailTask, openSettingsWithNav, etc.) - Update JSDoc to reflect desktop support
Tests previously asserted desktop does NOT push history entries. Now that back-button nav is enabled for desktop too, flip the assertions.
…ering The test clicked 'Medium' and 'Continue' synchronously without waiting for the scope question options to fully render. Use findByText/findByRole to wait for elements to appear before interacting, and add a timeout to the final waitFor for the second question.
timothyjlaurent
force-pushed
the
timothyjlaurent/browser-back-button-spa-integration
branch
from
298f848 to
5c16e09
Compare
2 participants
Problem
The browser back button in the Fusion dashboard SPA doesn't navigate back within the SPA — instead it leaves the page entirely (goes back to whatever page was visited before the dashboard).
Root Cause
The
useNavigationHistoryhook (which integrates the browser History APIpushState/popstatewith modal and view state) was only enabled for mobile viewports (enabled: isMobile). On desktop, all navigation history operations were no-ops.Solution
Enable
useNavigationHistoryon both desktop and mobile:enabled: isMobile→enabled: truein App.tsxisMobile ? historyAwareHandler : plainHandlerternaries to always use navigation-aware handlersTesting
useNavigationHistoryunit tests pass unchangedWhat this enables
Related: FN-002
Summary by CodeRabbit
New Features
Documentation
Tests