Skip to content

fix: copy current session share URL#2324

Open
AjTheSpidey wants to merge 7 commits into
hyperdxio:mainfrom
AjTheSpidey:codex/share-session-current-url
Open

fix: copy current session share URL#2324
AjTheSpidey wants to merge 7 commits into
hyperdxio:mainfrom
AjTheSpidey:codex/share-session-current-url

Conversation

@AjTheSpidey
Copy link
Copy Markdown

@AjTheSpidey AjTheSpidey commented May 21, 2026

Fixes #2313.

Found the share button could copy an older URL if the side panel rendered before the session params finished landing in the address bar.

Changed it so the button reads window.location.href when you click it, not during render. I also kept the success/error toast behavior, blocked duplicate clicks while the copy is running, and added tests around the fresh URL + retry cases.

Checked locally:

  • yarn workspace @hyperdx/app jest src/tests/SessionSidePanel.test.tsx --runInBand
  • yarn prettier --check packages/app/src/SessionSidePanel.tsx packages/app/src/tests/SessionSidePanel.test.tsx
  • yarn exec eslint --flag v10_config_lookup_from_file --quiet packages/app/src/SessionSidePanel.tsx packages/app/src/tests/SessionSidePanel.test.tsx
  • yarn workspace @hyperdx/app exec tsc --noEmit --pretty false

@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

@AjTheSpidey is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 21, 2026

🦋 Changeset detected

Latest commit: 68d9d8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

Deep Review

✅ No critical issues found. The fix replaces render-time URL capture with a click-time read, adds a synchronous ref-based duplicate-click guard, and tracks mount state so async resolution after unmount is a no-op. Tests cover the URL-freshness, error, rejection, duplicate-click, unmount, and StrictMode paths.

🔵 P3 nitpicks (7)
  • packages/app/src/SessionSidePanel.tsx:86 — the bare catch { copied = false; } is effectively dead because copyTextToClipboard catches internally and resolves to boolean, and it would also discard any unexpected error with no logging breadcrumb.
    • Fix: bind and log the error (console.error('share copy failed', err)) or remove the catch entirely and rely on the typed contract.
    • maintainability, reliability
  • packages/app/src/__tests__/SessionSidePanel.test.tsx:251 — flushing microtasks via await new Promise(resolve => setTimeout(resolve, 0)) after finishCopy(true) will silently turn into a false negative if any future await is added between the copy resolution and notifications.show.
    • Fix: drain microtasks deterministically with await Promise.resolve(); await Promise.resolve(); or assert with await waitFor(...) against a stable signal.
    • kieran-typescript, testing
  • packages/app/src/SessionSidePanel.tsx:94 — when the panel unmounts mid-copy, the clipboard write still completes (or fails) globally but the user gets no toast either way; this is the explicit "skip stale notifications" behavior but is worth documenting since notifications.show is a global portal.
    • Fix: add a one-line comment naming the intentional silence, or, if surfacing is preferred, drop the post-finally mount guard only on the failure path so the red toast still fires.
    • correctness, julik-frontend-races, reliability
  • packages/app/src/__tests__/SessionSidePanel.test.tsx:194 — no assertion exercises the Share button's loading={isSharingSession} prop while the copy is pending, so a regression that hard-codes the prop or forgets to flip the state would not be caught even though the duplicate-click test passes via the ref guard.
    • Fix: during the in-flight window assert shareButton has data-loading="true" (or the Mantine spinner is present) and re-assert it is cleared after the promise resolves.
  • packages/app/src/__tests__/SessionSidePanel.test.tsx:137 — URL assertions hard-code http://localhost/sessions?..., coupling tests to jsdom's default origin; a future testEnvironmentOptions.url change would break every URL assertion in the file even though production behavior is correct.
    • Fix: assert against window.location.href directly, or use expect.stringContaining('?sessionSource=...&sid=session-1') for the path-and-query portion.
  • packages/app/src/__tests__/SessionSidePanel.test.tsx:40notificationsShowSpy is installed at module scope and never restored, so the mutation of notifications.show can leak to later tests in the same Jest worker unless restoreMocks: true is set globally.
    • Fix: move jest.spyOn(...) into beforeEach, pair it with spy.mockRestore() in afterEach, or add an afterAll(() => notificationsShowSpy.mockRestore()).
  • packages/app/src/__tests__/SessionSidePanel.test.tsx:177 — the rejection-path test verifies the error toast but never fires a second click to confirm the finally block actually released isSharingSessionRef.current, which is the invariant the try/catch/finally shape exists to protect.
    • Fix: after the error notification assertion, dispatch a second click and assert copyTextToClipboardMock is called a second time.

Reviewers (6): correctness, kieran-typescript, julik-frontend-races, testing, maintainability, reliability.

Testing gaps: the new loading={isSharingSession} UI prop is not directly asserted; the reject path does not verify the duplicate-click guard reset.

@AjTheSpidey AjTheSpidey force-pushed the codex/share-session-current-url branch 2 times, most recently from 130d313 to b1fe133 Compare May 22, 2026 18:40
@AjTheSpidey AjTheSpidey force-pushed the codex/share-session-current-url branch 2 times, most recently from 016f943 to a62b13e Compare May 22, 2026 19:27
@AjTheSpidey AjTheSpidey force-pushed the codex/share-session-current-url branch from a62b13e to be021e5 Compare May 22, 2026 19:45
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.

Share Session button copies stale URL on first click (missing sid/sfrom/sto)

1 participant