Skip to content

feat: add global 'beforepopstate' bus to intercept browser back/forward navigation (#509)#511

Merged
NoopDog merged 3 commits into
mainfrom
fran/509-pop-state-bus
Jun 3, 2025
Merged

feat: add global 'beforepopstate' bus to intercept browser back/forward navigation (#509)#511
NoopDog merged 3 commits into
mainfrom
fran/509-pop-state-bus

Conversation

@frano-m

@frano-m frano-m commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

Closes #509.

This pull request introduces a new navigation handling system that tracks browser back/forward navigation events and provides this state to components via context. The changes include creating a centralized pop-state event bus, a context provider for managing navigation state, and hooks for registering callbacks. These updates improve the application's ability to handle user-initiated navigation and provide consistent state management.

Navigation State Management:

Pop-State Event Bus:

High-Level Provider:

  • src/providers/services/provider.tsx: Added ServicesProvider to initialize and provide access to the navigation handling system by combining usePopStateBus and WasPopProvider. This ensures all components can access the navigation state.

@frano-m frano-m requested review from NoopDog and Copilot June 2, 2025 14:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a global pop-state bus to intercept browser back/forward navigation and exposes a wasPop flag via React context.

  • Introduces a centralized pop-state event bus with registration hooks (usePopStateBus, useOnPopState)
  • Adds WasPopProvider context to track navigation initiated by browser history
  • Exposes a high-level ServicesProvider to wire everything at the app root

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/services/beforePopState/popStateBus.ts Core event bus for registering/unregistering pop-state callbacks
src/services/beforePopState/types.ts Type definitions for pop-state callbacks and state
src/services/beforePopState/usePopStateBus.ts Hook to install the global pop-state interceptor
src/services/beforePopState/useOnPopState.ts Hook to register/unregister individual pop-state callbacks
src/providers/services/wasPop/context.ts Creates WasPopContext with default wasPop: false
src/providers/services/wasPop/types.ts Defines WasPopContextProps interface
src/providers/services/wasPop/provider.tsx Implements WasPopProvider to track and reset wasPop
src/providers/services/wasPop/hook.ts useWasPop hook to consume WasPopContext
src/providers/services/provider.tsx ServicesProvider to wire up pop-state bus and WasPopProvider
Comments suppressed due to low confidence (1)

src/services/beforePopState/useOnPopState.ts:1

  • There are no tests covering the registration/unregistration lifecycle in useOnPopState. Consider adding unit tests to verify that callbacks are registered on mount and removed on unmount.
import { useEffect } from "react";

Comment thread src/providers/services/wasPop/provider.tsx Outdated
Comment thread src/providers/services/wasPop/provider.tsx Outdated
@frano-m frano-m marked this pull request as draft June 2, 2025 14:47
@frano-m frano-m requested a review from Copilot June 2, 2025 23:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a global pop‐state event bus and a context‐based system to track browser back/forward navigation, exposing that state to components via hooks and a top‐level services provider.

  • Introduces a centralized pop‐state event bus (popStateBus.ts, types, and hooks)
  • Adds WasPopProvider, its context, and useWasPop hook for consuming pop‐navigation state
  • Adds ServicesProvider to wire up the pop‐state bus and provider at the application root

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/services/beforePopState/popStateBus.ts Implements centralized pop‐state event bus and callback registry
src/services/beforePopState/types.ts Defines types for beforePop callbacks and history state
src/services/beforePopState/usePopStateBus.ts Hook to install the global pop‐state handler
src/services/beforePopState/useOnPopState.ts Hook to register/unregister individual pop‐state callbacks
src/providers/services/wasPop/context.ts Creates WasPopContext with default placeholder values
src/providers/services/wasPop/types.ts Defines WasPopContextProps interface
src/providers/services/wasPop/provider.tsx Implements WasPopProvider to track and expose pop‐state events
src/providers/services/wasPop/hook.ts Provides useWasPop hook to consume pop‐state context
src/providers/services/provider.tsx Adds ServicesProvider that wires up pop‐state bus and provider
Comments suppressed due to low confidence (2)

src/services/beforePopState/popStateBus.ts:58

  • [nitpick] Include more context in this error log (e.g., callback identity or state) to improve debugging when a listener throws.
console.error("Pop listener failed:", e);

src/services/beforePopState/popStateBus.ts:1

  • There are no tests covering the pop-state bus behavior; consider adding unit tests for registering, invoking, and unregistering callbacks.
import Router from "next/router";

Comment on lines +54 to +60
beforePopCallbacks.forEach((cb) => {
try {
if (cb(state) === false) allAllow = false;
} catch (e: unknown) {
console.error("Pop listener failed:", e);
}
});

Copilot AI Jun 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a for…of loop over the callback set and breaking early when a callback returns false, to avoid invoking all listeners once navigation is already blocked.

Suggested change
beforePopCallbacks.forEach((cb) => {
try {
if (cb(state) === false) allAllow = false;
} catch (e: unknown) {
console.error("Pop listener failed:", e);
}
});
for (const cb of beforePopCallbacks) {
try {
if (cb(state) === false) {
allAllow = false;
break;
}
} catch (e: unknown) {
console.error("Pop listener failed:", e);
}
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might have the case where a "downstream" listeners with an important side-effect never runs... leaving as is!

@@ -0,0 +1,25 @@
import React, { ReactNode } from "react";
import { usePopStateBus } from "../../services/beforePopState/usePopStateBus";
import { WasPopProvider } from "../services/wasPop/provider";

Copilot AI Jun 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The import path can be simplified to "./wasPop/provider" to avoid the extra directory traversal and improve readability.

Suggested change
import { WasPopProvider } from "../services/wasPop/provider";
import { WasPopProvider } from "./wasPop/provider";

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good pick up thank you!

@frano-m frano-m marked this pull request as ready for review June 2, 2025 23:51
@NoopDog NoopDog merged commit 0b851fd into main Jun 3, 2025
2 checks passed
@NoopDog NoopDog deleted the fran/509-pop-state-bus branch July 8, 2025 05:28
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.

Add a global “beforePopState” event bus to intercept browser back/forward navigation

3 participants