refactor(web): pull request list remembers each answer for the next start#25364

External image Major · Stability & Availability · Quick win

Guard the window.localStorage read in snapshotStorage().

snapshotStorage() reads window.localStorage without a try/catch. When browser storage is blocked, that read throws. Two call sites in this change run the read without a guard:

• The list callback calls snapshotStorage() before rememberPullRequestListAnswer and before its entry-observation loop. A throw stops the callback, so the entry observations for that answer are lost.
• restoredPullRequestList calls snapshotStorage() during PullRequestsPage render. A throw escapes before the restore call returns null. The page cannot reach its fetch fallback, and the render cannot complete for affected users.

Returning undefined from the getter fixes both call sites. The restore and remember helpers already accept undefined, and the server-side branch already returns it.

✅ Confirmed as addressed by @juliusmarminge

Real, fixed in c5e7ee9587. snapshotStorage() now returns undefined when reading window.localStorage throws, so the list callback still records its observations and the page falls back to a fresh read. The new test "falls back to a fresh read when the browser blocks storage" stubs a throwing localStorage getter and fails without the change. I also checked it live: with localStorage made to throw in the page, the pull requests page still loaded its list.

@juliusmarminge The supplied code confirms that snapshotStorage() catches a throwing window.localStorage getter and returns undefined. This addresses the finding and preserves the server-side behavior. Thanks for adding the regression test and checking the page live.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Major · Data Integrity & Integration · Heavy lift

Keep project coverage consistent when replacing a stored answer.

Suppose environment A stores project P1 and environment B stores P2. After reassignment, A can answer for P2 before B answers for P1. This write replaces A’s P1 answer while B still stores P2. A reload then restores P2 twice and omits P1; deduplication cannot recover P1. Stage answers for a complete assignment generation, or retain the previous generation until all affected environments answer.

✅ Confirmed as addressed by @juliusmarminge

Real, fixed in c5e7ee9587. A repository two environments share goes to the first query environment (assignProjectsToEnvironments). When that order flips, the environment losing the repository can answer first and replace its kept answer with one that no longer has the repository's rows. A reload before the environment taking it over answers then restored a list without those rows. Rather than staging answer generations, an answer for some projects (projectIds set) now replaces only those projects' rows and keeps the environment's other kept rows, capped at a page. An answer for all of an environment's projects, which every cold start sends, still replaces everything, so retained rows don't outlive the next cold start. Restore already dedupes the overlap. The test "keeps a shared repository's rows when it moves to an environment that has not answered yet" fails without the change.

@juliusmarminge The supplied code addresses this finding. Partial answers now retain cached rows for projects outside input.projectIds, so the losing environment’s response preserves coverage while the gaining environment’s response is pending. All-project answers still replace the environment’s full cached answer, and restore removes overlapping entries.

Staging answer generations is not needed for this fix.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Performance & Scalability · Quick win

Cap the combined stored entries at 99.

If an earlier answer has 99 rows for one project and a partial-project answer has 99 rows for another, retained keeps the earlier rows. Line 313 then stores 198 rows for one environment. Restoration renders more than the intended first page and increases storage and render work. Apply the cap after combining the rows, with an explicit policy for which older rows to retain.

As per coding guidelines, “Make sure all changes are considerate of performance impact.”

Source: Coding guidelines

✅ Confirmed as addressed by @juliusmarminge

Real, fixed in d9a4a1a9b0. A partial answer of 99 rows plus 99 retained rows from an earlier answer stored 198 rows for one environment. The cap now applies after combining: the fresh answer's rows come first and retained rows only fill what is left of the page ([...answer.entries, ...retained].slice(0, LIST_ANSWER_MAX_ENTRIES)). The test "keeps one page per environment when retained rows join a partial answer" fails without the change (198 rows instead of 99).

@juliusmarminge The combined cap addresses the overflow and keeps fresh rows first. The regression test covers the reported 198-row case. Thanks for the fix.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

✅ Review thread resolved.

_{You are interacting with an AI system.}