Repository navigation
[3306 by Claude Opus 5.5] Prevent delayed list search updates from overriding navigation during lazy route loading - #3308
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…on route is loading. With the fix, the issue's journey (Goals search → clear → More → Settings, with the Settings chunk delayed) stays on `/settings` and shows Settings. Cause. React Router keeps the list on screen while the next page's code loads. Tasks already blocks its own late URL writes during this window (its shared setter in `TaskList/hooks.ts`), but Goals and Plans used the router's plain search-param setter. A search write that fired after the user tapped Settings was resolved against `/goals` (or `/plans`), which moved the user back. Goals hit this even after Clear: Clear writes the URL immediately, then the 400ms debounce writes it again. Fix. Goals and Plans now use the same guarded setter as Tasks, so the cause is fixed in the shared helper rather than per page. I renamed it from `useTaskSearchParams` to `useListSearchParams` and updated its three existing callers and one test mock. A late list write is dropped if the route has changed or Back/Forward has moved history. Typing, the debounce timings, URL-backed filters, clear and focus, back/forward, task selection and lazy loading are unchanged. There are no sleeps, reloads or Settings-specific code. Tests - Browser test (`e2e/list-search-lazy-navigation.pw.ts`): uses the production build and holds the real Settings chunk, with no fake API responses for the navigation itself. It runs at 320, 390 (touch, through More → Settings) and 1280 (sidebar link). For Tasks, Plans and Goals it checks two cases, a cleared search and a typed search not yet saved, plus a no-edit control. The test pauses the page clock across the edit and navigation, then runs it past every debounce while the chunk is still held. Without that, 390px passed on the old code only because the menu taps happened to beat the 400ms debounce. It also checks that Back returns to the list without the cleared search. - With the fix: 21/21 pass. - On the original Goals/Plans code: 9 fail at every width, ending on `/goals` (6) or `/plans` (3) instead of `/settings`. Tasks and the controls pass on both, as expected, since Tasks was already guarded and the control involves no edit. - HashRouter: the native HashRouter only exists in the desktop app build, so the browser tests can't reach it. A new unit test (`src/components/TaskList/hooks.lazyRoute.test.tsx`) runs the same case under both BrowserRouter and HashRouter with a held lazy route: 8/8 pass. With the old plain setter, 4 fail with exactly the reported URL. - Existing coverage: 3033/3033 unit tests pass, `tsc` and `eslint .` are clean, and 78/78 related browser tests pass (list search, mobile menu, keyboard viewport, Goals, timeline, follow-up, diff). Previews. I captured Settings shown after a cleared Goals search with synthetic fixture data, at 320px and at desktop width. The red browser-push "HTTP 503" notice on the desktop shot comes from the test fixture, which doesn't fake that endpoint; it isn't a product error.
|
/review codex-astra Please independently review issue #3306's navigation correction at exact head e497b46 against all original acceptance criteria. The observed release blocker is a real authenticated Goals search → clear → More → Settings journey returning to /goals while the real Settings JS chunk is delayed. A no-clear control reaches Settings; the failure persists with tab-only service-worker/cache bypass. This was independently reproduced by the operator before this PR. Assess the shared useListSearchParams boundary, adoption by both Goals and Plans, existing Tasks/inbox selection consumers, BrowserRouter and native HashRouter, lazy transitions and history.Back/Forward. Preserve debounce/URL/filter/selection semantics, same-path selection writes, focus, unsent work and ordinary no-edit navigation. Verify no stale list update can retake navigation after another route begins loading. No hard reload, timer delay or Settings-only special case should hide the issue. Inspect the new browser and unit regressions for a genuine old-code-failing/new-code-passing assertion, including 320/390 trusted touch, desktop, typed and cleared searches, held unchanged real lazy module, clock behavior and no-edit controls. Check all existing tests and modified mocks for waived coverage. The native HashRouter unit proof is not final installed native acceptance. Operator independent full Chromium/WebKit runs, units, old-code negative controls, native and authenticated final-build checks remain separate; do not infer them from author claims or static review. No provider/model calls, permission changes or credential access are needed. Keep the independent mobile task-header overlap #3307 separate; this PR does not resolve it or authorize release publication. Independent exact-head operator validation and green required CI are needed before authorized merge. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationThe implementation is narrowly scoped and appears to fix the reported cross-route navigation race. No PR-introduced code blocker is demonstrated by the supplied diff. Merge readiness remains unverified pending independent exact-head operator validation and required CI completion. ✅ Shared navigation boundary — Goals and Plans now use the existing Tasks guard, which checks live router history before writing. Once history moves to ✅ Selection semantics preserved — Tasks and inbox changes rename the hook without changing its implementation. Same-path replacement writes continue to merge current query parameters, preserving concurrent selection and filter changes. ✅ Focused regression coverage — Added tests cover pending typing, cleared searches, no-edit controls, and both router types. Browser tests hold the unchanged Settings module response and exercise mobile menu taps at 320/390 widths plus desktop navigation. This is a static review only; no commands or tests were run. The supplied original issue description is truncated, so acceptance criteria beyond the visible specification cannot be assessed. The authoritative status reports zero failures and six pending checks. Authenticated final-build reproduction, Chromium/WebKit execution, old-code negative controls, installed native acceptance, and preservation of unsent work are not established by static inspection. Issue #3307 and release publication remain separate. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Expand history transition regressionsThe new browser helper checks Back only after Settings finishes loading; it does not exercise Forward or history traversal while the destination remains suspended. Additional focused tests would strengthen confidence in the live-history guard, particularly for Goals and Plans: their pending callbacks can be rescheduled when the setter changes, whereas Tasks explicitly retains the edit’s original callback. This is a coverage improvement, not a demonstrated PR-introduced defect. S2: 🟢 Distinguish clear-test scheduling guaranteesThe browser tests intentionally await Tasks’ clear debounce before navigating, and Plans clears both its input and debounced state immediately. Consequently, those clear cases do not establish that a pending clear writer was rejected; the pending-typing cases provide that coverage. Clarifying the comments would make the evidence easier to interpret. Goals’ clear case does exercise its queued debounced recommit. The renamed inbox mock preserves its existing refusal test rather than waiving an assertion. ScoreThe code is focused and the regression design addresses the reported failure, but material independent runtime validation remains outstanding. Pending checks alone are not the reason for withholding a merge-ready assessment. Score: 7/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3306
Branch:
3306/claude-opus-5-5-prevent-delayed-list-sear-20261021-0406-ac6Commits: ✅ Changes committed (e497b46)
AI Processing Completed
Execution Details:
Summary:
Goals and Plans no longer pull navigation back while a lazy destination route is loading. With the fix, the issue's journey (Goals search → clear → More → Settings, with the Settings chunk delayed) stays on
/settingsand shows Settings.Cause. React Router keeps the list on screen while the next page's code loads. Tasks already blocks its own late URL writes during this window (its shared setter in
TaskList/hooks.ts), but Goals and Plans used the router's plain search-param setter. A search write that fired after the user tapped Settings was resolved against/goals(or/plans), which moved the user back. Goals hit this even after Clear: Clear writes the URL immediately, then the 400ms debounce writes it again.Fix. Goals and Plans now use the same guarded setter as Tasks, so the cause is fixed in the shared helper rather than per page. I renamed it from
useTaskSearchParamstouseListSearchParamsand updated its three existing callers and one test mock. A late list write is dropped if the route has changed or Back/Forward has moved history. Typing, the debounce timings, URL-backed filters, clear and focus, back/forward, task selection and lazy loading are unchanged. There are no sleeps, reloads or Settings-specific code.Tests
e2e/list-search-lazy-navigation.pw.ts): uses the production build and holds the real Settings chunk, with no fake API responses for the navigation itself. It runs at 320, 390 (touch, through More → Settings) and 1280 (sidebar link). For Tasks, Plans and Goals it checks two cases, a cleared search and a typed search not yet saved, plus a no-edit control. The test pauses the page clock across the edit and navigation, then runs it past every debounce while the chunk is still held. Without that, 390px passed on the old code only because the menu taps happened to beat the 400ms debounce. It also checks that Back returns to the list without the cleared search./goals(6) or/plans(3) instead of/settings. Tasks and the controls pass on both, as expected, since Tasks was already guarded and the control involves no edit.src/components/TaskList/hooks.lazyRoute.test.tsx) runs the same case under both BrowserRouter and HashRouter with a held lazy route: 8/8 pass. With the old plain setter, 4 fail with exactly the reported URL.tscandeslint .are clean, and 78/78 related browser tests pass (list search, mobile menu, keyboard viewport, Goals, timeline, follow-up, diff).Previews. I captured Settings shown after a cleared Goals search with synthetic fixture data, at 320px and at desktop width. The red browser-push "HTTP 503" notice on the desktop shot comes from the test fixture, which doesn't fake that endpoint; it isn't a product error.
Detailed Logs:
e8792833-1c5b-403e-8954-3ca3bbb024f7Log files stored at:
/usr/src/app/logs/executions/issue-3306-2026-10-21T04-40-38-785Z-conversation.json/usr/src/app/logs/executions/issue-3306-2026-10-21T04-40-38-785Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3306.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
320px phone: Settings opens after a cleared Goals search
Production build, trusted touch. Goals search typed, cleared, then More → Settings with the real Settings route chunk held and every search debounce elapsed meanwhile. Settings stays on /settings instead of returning to /goals. Synthetic fixture data.
Original unavailable: managed storage is unavailable or the upload has expired.
Desktop: Settings opens after a cleared Goals search
Same delayed-chunk flow through the sidebar Settings link at 1280px. The browser-push 503 notice comes from the fixture, which does not stub that endpoint.
Original unavailable: managed storage is unavailable or the upload has expired.