Fix page dedup cache preventing retry after failed API calls

[JeroenDeDauw]

Summary by CodeRabbit

• Bug Fixes
  • Corrected page connection handling to ensure pages are marked as added only after API confirmation is received, preventing potential data inconsistencies.

[coderabbitai]

📝 Walkthrough

Walkthrough

The addConnections method in ApiPageConnectionRepo.js has been modified to move the state update of this._addedPages from before the API request to after the response completes, ensuring pages are marked as added only upon successful API response.

Changes

Cohort / File(s)	Summary
State Update Timing resources/js/ApiPageConnectionRepo.js	Moved this._addedPages concatenation from pre-request to inside the .done callback of _queryLinks, ensuring state update occurs only after API response is received and processed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Fix page dedup cache, redirect title icon lookup, and missing isRedirect #93: Directly modifies the same addConnections behavior in ApiPageConnectionRepo by moving the _addedPages state update into the API response callback, making identical control flow changes.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix page dedup cache preventing retry after failed API calls' directly relates to the core change: moving page-added state updates to after API responses succeed, which prevents the dedup cache from blocking retries when API calls fail.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-addedPages-retry

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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)

21-33: ⚠️ Potential issue | 🟠 Major

Handle API failure paths so addConnections always settles.

If _queryLinks fails, this Promise never resolves/rejects because only .done is wired. That can stall callers and retry flows.

Suggested fix

 ApiPageConnectionRepo.prototype.addConnections = function(pageNames) {
 	return new Promise(
-		function(resolve) {
+		function(resolve, reject) {
 			let pagesToAdd = pageNames.filter(page => !this._addedPages.includes(page));

 			if (pagesToAdd.length === 0) {
 				resolve({pages: [], links: []});
 			} else {
 				this._queryLinks(pagesToAdd).done(
 					function(apiResponse) {
-						this._addedPages = this._addedPages.concat(pagesToAdd);
-						this._apiResponseToPagesAndLinks(apiResponse).then(connections => resolve(connections))
+						this._apiResponseToPagesAndLinks(apiResponse)
+							.then(connections => {
+								this._addedPages = this._addedPages.concat(pagesToAdd);
+								resolve(connections);
+							})
+							.catch(reject);
 					}.bind(this)
-				);
+				).fail(reject);
 			}
 		}.bind(this)
 	);
 };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@resources/js/ApiPageConnectionRepo.js` around lines 21 - 33, The Promise
returned in addConnections (the new Promise executor using pageNames and
this._queryLinks) never settles when this._queryLinks fails because only .done
is used; update the call to handle error paths (e.g., use .then/.catch or attach
.fail/.always) so that failures either reject the outer Promise or resolve it
with a safe value, and ensure the same this binding is preserved for the success
and error handlers on this._queryLinks; specifically, modify the block around
this._queryLinks(pagesToAdd).done(...) to add an error handler that calls
reject(...) or resolve({pages:[],links:[]}) as appropriate and keep using
.bind(this) (or arrow functions) so this._addedPages and
this._apiResponseToPagesAndLinks are available.

🤖 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 21-33: The Promise returned in addConnections (the new Promise
executor using pageNames and this._queryLinks) never settles when
this._queryLinks fails because only .done is used; update the call to handle
error paths (e.g., use .then/.catch or attach .fail/.always) so that failures
either reject the outer Promise or resolve it with a safe value, and ensure the
same this binding is preserved for the success and error handlers on
this._queryLinks; specifically, modify the block around
this._queryLinks(pagesToAdd).done(...) to add an error handler that calls
reject(...) or resolve({pages:[],links:[]}) as appropriate and keep using
.bind(this) (or arrow functions) so this._addedPages and
this._apiResponseToPagesAndLinks are available.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 434e7db and 8ab3195.

📒 Files selected for processing (1)

• resources/js/ApiPageConnectionRepo.js