Fix blank redirect stubs for unmapped fragments#9067
Conversation
Redirect stub pages only redirected anchors explicitly listed in redirects.py. Any other fragment (e.g. environment-configuration-settings .html#mobile-security) logged "unknown redirect" and left the user on a blank page. Now an unmapped fragment falls back to the page's base redirect, carrying the original fragment over (anchor names are generally preserved across a page move). When no redirect can be resolved at all, send the user to the 404 page instead of leaving a blank stub (resolves the existing TODO). 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe redirect template now uses a helper for fragment-based navigation, falls back to base-page redirects with preserved fragments, and sends unresolved fragments to ChangesRedirect navigation updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/_templates/redirect.html`:
- Around line 31-33: The fragment lookup in redirect.html is using the
fragment_redirects object with an in check, which can match inherited properties
and lead redirectTo() to receive a non-string value. Update the fragment lookup
in the redirect logic to use an own-property check on fragment_redirects before
calling redirectTo(), and apply the same safe pattern to the DEFAULT_PAGE
fallback so only real redirect entries are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eaa314dd-ad17-4381-943a-5cbfa6eeecd3
📒 Files selected for processing (1)
source/_templates/redirect.html
|
Newest code from mattermost has been published to preview environment for Git SHA 888741e |
fragment_redirects is a plain object, so `in` also matches inherited Object.prototype members. A URL like ...#toString passed FRAGMENT_REGEX and matched the inherited toString function, sending a non-string to redirectTo(). Use Object.prototype.hasOwnProperty.call so only real redirect entries are matched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Newest code from mattermost has been published to preview environment for Git SHA c03e13c |
The fallback now also backs the explicit fragment redirects we're about to remove, so downgrade the log from console.warn to console.debug to avoid warning noise on routine redirects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The redirect stub now falls back to a page's base redirect and carries the original fragment over. That makes fragment redirects whose target is exactly <base-target>#<same-fragment> redundant: the fallback produces the identical destination. Remove the 233 such entries. Kept untouched: - fragments on pages with no base entry (the fallback can't reach them), - fragments pointing to a different page, and - fragments whose anchor was renamed at the destination. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Newest code from mattermost has been published to preview environment for Git SHA 3bd9417 |
There was a problem hiding this comment.
These are not needed any longer. The new code handles them automatically.
Why
Visiting a redirect-stub URL with an anchor that wasn't explicitly mapped returns a blank page. For example, http://docs.mattermost.com/configure/environment-configuration-settings.html#database renders nothing — the old
configure/…path is a redirect stub, and#databasewas never added to its fragment map, so the JS loggedunknown redirect: databaseand left the user staring at an empty page.Redirect stubs (generated from
redirects.pyviasource/_templates/redirect.html) only redirected fragments explicitly listed inredirects.py. Any other fragment hit theelsebranch and blanked out. The// TODO: redirect the user to a 404 pagewas also still unimplemented.Changes
source/_templates/redirect.html:-default entry) and carry the original fragment over — anchor names are generally preserved across a page move, so…#database/…#mobile-securitynow land on the real section instead of a blank stub. Fixes the whole class of unmapped-anchor links.// TODO. When no redirect can be resolved at all (no matching fragment and no base entry), the user is sent to/404.htmlinstead of a blank page.fragment_redirectsis a plain object, sofragment in …also matched inherited members (#toString,#constructor, …), passing a non-string toredirectTo(). Switched toObject.prototype.hasOwnProperty.call./→ absolute-origin handling into a smallredirectTo()helper, and log the fallback atconsole.debug(notwarn) so routine redirects stay quiet.source/redirects.py:<base-target>#<same-fragment>is redundant — the fallback produces the identical destination. Kept untouched: fragments on pages with no base entry (the fallback can't reach them → they'd 404), fragments pointing to a different page, and fragments whose anchor was renamed at the destination. (2166 → 1933 entries.)Verification
#databaseexactly 150px below the fixed header (matchingsection[id] { scroll-margin-top }), stable across the full page load. Works in Chrome and Firefox. (There's a pre-existing Edge-only quirk where a long page's late reflow leaves the anchor slightly high — unrelated to this change and out of scope.)redirects.pyre-imports cleanly; 0 redundant entries remain; the 34 no-base fragment entries are preserved.🤖 Generated with Claude Code