Skip to content

FOLLOW-UP: Add skip-setup flag to connect() to eliminate double setup on cross-handler nav (from PR #58) #67

@adnaan

Description

@adnaan

Context

This follow-up task was identified during the review of PR #58.

Source PR: #58
PR Title: fix: cross-handler SPA navigation, navigation edge cases, and Tier 1 file uploads
Suggested by: @claude[bot]
Priority: Low

Task Description

In handleNavigationResponse's cross-handler path, setupEventDelegation() and linkInterceptor.setup() are called twice:

  1. Synchronously, immediately after DOM replacement (so listeners work before the async connect() completes)
  2. Again internally by connect() as part of its normal setup flow

Current behavior is safe because both setup methods are idempotent (they dedupe by wrapper ID — existing listeners for the same key are removed before new ones are registered). This is documented as a comment in handleNavigationResponse.

However, if connect() ever changes its internal ordering (e.g. a refactor that removes or reorders the setup calls), the pre-connect sync calls would silently become the ONLY setup path, and removing them later would silently break things.

Proposed Fix

Add a skipListenerSetup option to connect():

interface ConnectOptions {
  skipListenerSetup?: boolean;
}

async connect(selector: string, options?: ConnectOptions): Promise<void> {
  // ... transport setup ...
  if (!options?.skipListenerSetup) {
    this.eventDelegator.setupEventDelegation();
    this.linkInterceptor.setup(this.wrapperElement);
    // ... other setup that handleNavigationResponse already handled ...
  }
  // ...
}

Then handleNavigationResponse would pass { skipListenerSetup: true } since it handles setup itself.

Alternative: leave current behavior but add a unit test asserting that connect() calls setupEventDelegation and linkInterceptor.setup so a future refactor would fail loudly.

Original Comment

setupEventDelegation() and linkInterceptor.setup() are called synchronously before connect(), then connect() calls them again internally. The comment says both are idempotent (remove existing listener before adding new one), but there's a brief window where the synchronously-registered listener is active. If connect() re-runs setup with a different configuration (e.g. updated rateLimitedHandlers), events fired in that window use stale config. Low risk in practice, but worth being aware of.


This issue was created from PR #58 review comments.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestfollow-upFollow-up task from PR reviewfrom-reviewIssue originated from PR review

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions