Skip to content

fix(pages): scroll docs deep links on load#413

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
fjbarrett:fix/docs-deep-link-scroll
Jul 21, 2026
Merged

fix(pages): scroll docs deep links on load#413
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
fjbarrett:fix/docs-deep-link-scroll

Conversation

@fjbarrett

@fjbarrett fjbarrett commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Description

Direct docs links now scroll to their fragment after the rendered markdown heading is available. The effect reads React Router's location hash, decodes percent-encoded fragments, retries briefly while content renders, and cancels stale retries when navigation changes.

Since #407 switched the site to BrowserRouter, verification used the equivalent clean URLs under /docs/....

AI assistance: OpenAI Codex helped implement and verify this change. I reviewed the full diff and take responsibility for it.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing:
    • make check
    • cd pages && npm run typecheck
    • cd pages && npm run build
    • Playwright: direct English fragment
    • Playwright: encoded CJK fragment
    • Playwright: fragment change after mount
    • Playwright: unknown fragment produces no error
    • Playwright: existing cross-page click still scrolls

Screenshots

Before, a directly loaded fragment remains at the top of the FAQ:

Before

After, the same URL scrolls to the requested FAQ section:

After

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works (the frontend currently has no automated test runner; covered with Playwright browser verification above)
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (not applicable)
  • I have signed the CLA

Related Issues

Closes #412

@CLAassistant

CLAassistant commented Jul 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: No comments generated. Looks good to me.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Thanks for the fix — the core deep-link scrolling logic is correct and the retry/cancel handling is solid. A few things to address before merging:

1. HISTORY.md should not be committed

This file is a Codex agent workflow log ("Forked the repository and claimed issue #412", etc.). It's agent-internal scratch state and unrelated to the codebase — this file didn't exist in the repo before. Please remove it from the PR.

2. Screenshots should live in the PR description, not the repo

The two PNGs under screenshots/ are acceptance evidence for this UI change. That's exactly what the PR description is for — and you've already embedded them there via raw.githubusercontent.com. Committing the binaries into the repo is redundant and bloats history. Please drop the screenshots/ additions and keep them as links in the PR body only.

3. De-duplicate the tryScroll retry logic

The new useEffect reimplements almost the same rAF retry loop that already exists in handleContentClick (the tryScroll(attempts) at ~L233-241). Please extract a single shared helper so both paths use one implementation, e.g.:

function scrollToFragmentWhenReady(id: string): () => void {
  let frame = 0;
  let cancelled = false;
  const tryScroll = (attempts: number) => {
    if (cancelled) return;
    const el = document.getElementById(id);
    if (el) {
      el.scrollIntoView({ behavior: 'smooth', block: 'start' });
    } else if (attempts < 10) {
      frame = requestAnimationFrame(() => tryScroll(attempts + 1));
    }
  };
  frame = requestAnimationFrame(() => tryScroll(0));
  return () => {
    cancelled = true;
    cancelAnimationFrame(frame);
  };
}

The useEffect can then just return scrollToFragmentWhenReady(decodeFragment(fragment)), and handleContentClick can reuse it too (which also gives that path the cancellation it currently lacks).

Once these are addressed this looks good to go. Thanks!

Read the routed fragment after markdown renders so direct links and later hash
changes reach their headings. Retry briefly for rendered content and cancel
stale attempts during navigation.

The routed-fragment effect and the in-content link handler share a single
scrollToFragmentWhenReady helper, so the rAF retry loop is defined once and the
click path gains the cancellation it previously lacked.

Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fjbarrett
fjbarrett force-pushed the fix/docs-deep-link-scroll branch from 2e23264 to 714bb79 Compare July 21, 2026 05:14
@fjbarrett

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. All three items are addressed in 714bb79:

  1. Removed HISTORY.md from the PR.
  2. Removed screenshots/ from repository history. The PR description still contains the before/after evidence, hosted from the separate immutable asset commit aca0fd1.
  3. Extracted the retry loop into scrollToFragmentWhenReady. Both initial deep-link scrolling and cross-page fragment clicks now use the shared helper, including cancellation of stale retries.

The PR now changes only pages/src/pages/DocsPage.tsx (+35/-10).

Verified:

  • tsc --noEmit
  • npm run build

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 389a6e3 into alibaba:main Jul 21, 2026
2 checks passed
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.

fix(pages): DocsPage does not scroll to in-page anchor on initial load / deep link

3 participants