fix(modal): sub-anchor click closes the umbrella modal (E2E regression)#567
Conversation
Sub-anchor links render as <a href="#" data-sub-anchor>, which were also
matched by the AsciiDoc cross-reference handler (a[href^="#"]). Clicking a
sub-anchor therefore fired both its own handler AND a bogus
navigate('/anchor/') with an empty id. Harmless before, but since the
modal now closes when a non-anchor route resolves (LLM-Coding#565), handleRoute
normalizes '/anchor/' → '/anchor' (not an anchor route) and closed the
umbrella modal — breaking the "navigate to sub-anchor / show back button"
E2E tests.
Exclude data-sub-anchor links from the cross-reference handler and ignore
empty ids, so sub-anchor clicks only load the sub-anchor and the modal +
back button stay put.
Verified: full E2E suite (35) green locally, incl. the two umbrella tests
that regressed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDer PR verfeinert die Logik zum Umwandeln interner AsciiDoc-Cross-Reference-Links in Router-Navigation. Der DOM-Selektor wird um einen Filter erweitert, der Sub-Anchor-Elemente ausschließt, und die Validierung wird verschärft, um leere oder Hash-Route-Format-Links zu ignorieren. ÄnderungenAnchor-Link-Verarbeitung
Geschätzter Code-Review-Aufwand🎯 2 (Einfach) | ⏱️ ~8 Minuten Möglicherweise verwandte PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JensGrote
left a comment
JensGrote
left a comment
There was a problem hiding this comment.
LGTM ✅
Nice root-cause analysis in the description. The double-guard (:not([data-sub-anchor]) selector + if (!anchorId) return) is solid defensive coding — even if one mechanism is bypassed, the other catches it.
Verified that data-sub-anchor is set internally in renderSubAnchorList() (line 150) and handled in the same file (line 258), so the coupling is tight and unlikely to break silently.
3 participants
Problem
main's E2E suite is red: the "Umbrella Anchors › navigate to sub-anchor / show back button" tests fail because clicking a sub-anchor inside an umbrella modal closes the whole modal.Root cause (two bugs combine)
<a href="#" data-sub-anchor>, which the AsciiDoc cross-reference handler (a[href^="#"]) also matched. A sub-anchor click thus fired both its own handler and a bogusnavigate('/anchor/')(empty id).handleRoutestrips the trailing slash so'/anchor/'→'/anchor', which is not an anchor route →closeOpenAnchorModal()closed the umbrella modal.Before #565 the bogus navigation was harmless, so this only surfaced now.
Fix
Exclude
[data-sub-anchor]links from the cross-reference handler and ignore empty ids. Sub-anchor clicks now only load the sub-anchor; the modal and its back button stay put.Verification
/anchor/gof-design-patterns, the modal stays open, the back button is visible.Priority: unblocks
main(E2E currently failing) and the open analytics PR #566.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes