Open
Conversation
📝 WalkthroughWalkthroughAdds explicit redirect handling by building a redirect map from API responses and using it to resolve redirect targets and mark redirect pages; also marks outgoing link entries as non-redirects when building the page map. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/ApiPageConnectionRepo.js (1)
73-77:⚠️ Potential issue | 🟡 MinorTitle icon lookup misses for redirected pages
titleIcons.images/titleIcons.textare keyed by resolved titles (built frompageInfoResponse.query.pagesin_getTitleIcons), but the lookup here usespage.title, which is still the redirect-source title for outgoing-link pages (e.g."Maps for MediaWiki"while the icon is stored under"Maps"). The icon will silently be absent for any redirected page.Apply the same
redirectMapresolution used fordisplayTitle:🔧 Proposed fix
- if (titleIcons.images[page.title]) { - page.image = titleIcons.images[page.title]; - } else if (titleIcons.text[page.title]) { - page.text = titleIcons.text[page.title]; + let iconKey = redirectMap[page.title] || page.title; + if (titleIcons.images[iconKey]) { + page.image = titleIcons.images[iconKey]; + } else if (titleIcons.text[iconKey]) { + page.text = titleIcons.text[iconKey];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/ApiPageConnectionRepo.js` around lines 73 - 77, The title icon lookup uses the original redirect-source title (page.title) so icons keyed by resolved titles are missed; change the lookup in the block that sets page.image/page.text to first resolve the title via the same redirectMap used for displayTitle (e.g. let resolved = redirectMap[page.title] || page.title) and then check titleIcons.images[resolved] and titleIcons.text[resolved] instead of titleIcons.*[page.title], ensuring redirected pages find their icons; reference the redirectMap, displayTitle logic and _getTitleIcons/titleIcons when making this change.
🧹 Nitpick comments (2)
resources/js/ApiPageConnectionRepo.js (1)
28-28: Pre-existing:concatresult is discarded —_addedPagesnever grows
Array.prototype.concatis non-mutating, sothis._addedPagesstays[]after every call, making the deduplication filter on line 23 permanently a no-op. EveryaddConnectionscall re-fetches all pages.🔧 Proposed fix
- this._addedPages.concat(pagesToAdd); + this._addedPages = this._addedPages.concat(pagesToAdd);or equivalently:
- this._addedPages.concat(pagesToAdd); + this._addedPages.push(...pagesToAdd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/ApiPageConnectionRepo.js` at line 28, The concat call on this._addedPages is non-mutating so this._addedPages never updates; update addConnections to either assign the concat result back to this._addedPages (this._addedPages = this._addedPages.concat(pagesToAdd)) or push the new items into the array (this._addedPages.push(...pagesToAdd)) so the deduplication using _addedPages actually works; locate the concat call and replace it with one of these two approaches in the ApiPageConnectionRepo class so subsequent calls skip already-added pages.resources/js/ApiConnectionsBuilder.js (1)
60-78: Pre-existing: external link entries omitisRedirect, yieldingundefinedin_buildPageListAll three code paths here produce page objects without an
isRedirectfield._buildPageList(line 39) copiespage.isRedirectunconditionally, so external pages end up withisRedirect: undefinedrather thanfalse. Not a functional problem (external pages are skipped inApiPageConnectionRepodisplay-title logic via theisExternalcheck), but worth aligning for consistency.🔧 Proposed fix
- pages[titleStr] = { title: titleStr, external: false } + pages[titleStr] = { title: titleStr, external: false, isRedirect: false } } else { - pages[page['*']] = { title: page['*'], external: true } + pages[page['*']] = { title: page['*'], external: true, isRedirect: false } } } else { - pages[page['*']] = { title: page['*'], external: true } + pages[page['*']] = { title: page['*'], external: true, isRedirect: false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/ApiConnectionsBuilder.js` around lines 60 - 78, The externalLinks loop in centralPage.externalLinks.forEach assigns page objects without an isRedirect field so _buildPageList ends up copying undefined; update every pages[...] assignment inside that loop (both the branch where mw.Title.newFromText succeeds and the two external branches) to include isRedirect: false (e.g., pages[titleStr] = { title: titleStr, external: false, isRedirect: false } and pages[page['*']] = { title: page['*'], external: true, isRedirect: false }) so external entries consistently have isRedirect set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@resources/js/ApiPageConnectionRepo.js`:
- Around line 73-77: The title icon lookup uses the original redirect-source
title (page.title) so icons keyed by resolved titles are missed; change the
lookup in the block that sets page.image/page.text to first resolve the title
via the same redirectMap used for displayTitle (e.g. let resolved =
redirectMap[page.title] || page.title) and then check
titleIcons.images[resolved] and titleIcons.text[resolved] instead of
titleIcons.*[page.title], ensuring redirected pages find their icons; reference
the redirectMap, displayTitle logic and _getTitleIcons/titleIcons when making
this change.
---
Nitpick comments:
In `@resources/js/ApiConnectionsBuilder.js`:
- Around line 60-78: The externalLinks loop in centralPage.externalLinks.forEach
assigns page objects without an isRedirect field so _buildPageList ends up
copying undefined; update every pages[...] assignment inside that loop (both the
branch where mw.Title.newFromText succeeds and the two external branches) to
include isRedirect: false (e.g., pages[titleStr] = { title: titleStr, external:
false, isRedirect: false } and pages[page['*']] = { title: page['*'], external:
true, isRedirect: false }) so external entries consistently have isRedirect set.
In `@resources/js/ApiPageConnectionRepo.js`:
- Line 28: The concat call on this._addedPages is non-mutating so
this._addedPages never updates; update addConnections to either assign the
concat result back to this._addedPages (this._addedPages =
this._addedPages.concat(pagesToAdd)) or push the new items into the array
(this._addedPages.push(...pagesToAdd)) so the deduplication using _addedPages
actually works; locate the concat call and replace it with one of these two
approaches in the ApiPageConnectionRepo class so subsequent calls skip
already-added pages.
Collaborator
Author
|
The CI failure appears to be unrelated. I'll handle the unrelated CodeRabbit suggestions in another PR, as they are unrelated. |
cicalese
added a commit
that referenced
this pull request
Feb 26, 2026
Fix issues found by CodeRabbit in #92 - Assign concat result back to _addedPages so the dedup cache actually grows across addConnections calls - Build a redirect map from the API response and use it to resolve title icon lookups for redirected pages - Add isRedirect: false to outgoing and external link page entries for consistency with central page and incoming link entries
Collaborator
Author
|
The unrelated CodeRabbit suggestions are handled in #93. |
When a page has no custom display title, fall back to the resolved redirect target title instead of the original redirect source title.
cicalese
added a commit
that referenced
this pull request
Feb 26, 2026
Fix issues found by CodeRabbit in #92 - Assign concat result back to _addedPages so the dedup cache actually grows across addConnections calls - Build a redirect map from the API response and use it to resolve title icon lookups for redirected pages - Add isRedirect: false to outgoing and external link page entries for consistency with central page and incoming link entries
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
resources/js/ApiPageConnectionRepo.js (1)
155-163: LGTM! Clean helper method with safe null handling.The defensive check for
pageInfoResponse.query.redirectsbefore iteration prevents the TypeError reported in issue#90when the redirects array is absent.For extra defensive programming, consider using optional chaining to guard against edge cases where
querymight be undefined (though currently protected by prior access at line 47):🔧 Optional: Add optional chaining
ApiPageConnectionRepo.prototype._buildRedirectMap = function(pageInfoResponse) { let redirectMap = {}; - if (pageInfoResponse.query.redirects) { - pageInfoResponse.query.redirects.forEach(function(redirect) { + if (pageInfoResponse.query?.redirects) { + pageInfoResponse.query.redirects.forEach(function(redirect) { redirectMap[redirect.from] = redirect.to; }); } return redirectMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/ApiPageConnectionRepo.js` around lines 155 - 163, The helper _buildRedirectMap should guard against pageInfoResponse.query being undefined before accessing redirects to avoid potential runtime errors; update the check for redirects to use optional chaining on pageInfoResponse.query (i.e., verify pageInfoResponse.query?.redirects) and iterate only if that value is present, keeping the current redirectMap population logic in ApiPageConnectionRepo.prototype._buildRedirectMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@resources/js/ApiPageConnectionRepo.js`:
- Around line 155-163: The helper _buildRedirectMap should guard against
pageInfoResponse.query being undefined before accessing redirects to avoid
potential runtime errors; update the check for redirects to use optional
chaining on pageInfoResponse.query (i.e., verify
pageInfoResponse.query?.redirects) and iterate only if that value is present,
keeping the current redirectMap population logic in
ApiPageConnectionRepo.prototype._buildRedirectMap.
Collaborator
Author
|
@JeroenDeDauw This should be ready to merge pending your review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #90
Fixes #58
Summary by CodeRabbit