Skip to content

Fix repeated Question Bank navigation reset - #119

Open
k-undurkhaan-2 wants to merge 9 commits into
opensourcefrom
codex/fix-114-question-bank-reset
Open

Fix repeated Question Bank navigation reset#119
k-undurkhaan-2 wants to merge 9 commits into
opensourcefrom
codex/fix-114-question-bank-reset

Conversation

@k-undurkhaan-2

Copy link
Copy Markdown
Collaborator

Summary

  • reset repeated Question Bank navigation against the active library index through the shared latest-wins loader
  • clear category, type, frequency, path, and search state only for repeated navigation while preserving normal and explicit category navigation
  • carry reset intent safely across lazy Browse startup and prevent stale reset/filter work from overwriting newer interactions
  • keep generated bundles synchronized and add focused regression coverage

Root cause

ExamActions.resetBrowseViewToAll() called its module-local loadExamList() without an index. That loader treats a missing index as [], so repeated Question Bank navigation could render an empty list. Independent lazy-navigation and pending-filter paths could also restore stale scope after a reset.

Validation

  • node --test --test-concurrency=1 "tests/js/**/*.test.js" from developer (82/82)
  • python -m unittest discover -s developer/tests/py -p "test_*.py" (20/20)
  • python developer/tests/ci/run_static_suite.py
  • python developer/tests/ci/check_reading_data_integrity.py
  • python developer/tests/e2e/e2e_runner.py (8/8)
  • node scripts/build-bundles.mjs --check (14 outputs current)

Fixes #114

Resolve the active library before rendering a repeated Question Bank reset through the shared latest-wins adapter. Preserve ordinary and explicit category navigation while clearing stale browse state, and add focused regression coverage.

Fixes #114

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f2b9703b8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread js/app/main-entry.js
Prevent cold Browse continuations and background index refreshes from reclaiming newer reset, filter, or search results. Freeze reset ownership and replay index snapshots safely across asynchronous initialization and failure paths.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING REVIEW - changes are required before merge.

GitHub rejected the formal REQUEST_CHANGES event because the authenticated local gh identity is the PR author. An independent maintainer/reviewer should submit REQUEST_CHANGES after evaluating the findings below.

The GitHub merge state is mechanically clean and both CI verify and GitGuardian succeeded, but the current implementation still has merge-blocking correctness and scope-gate issues:

  1. examIndexLoaded replays reuse the token owned by the operation already in flight, so an older asynchronous load/search can remain authorized and overwrite the fresh index replay.
  2. ordinary Question Bank re-entry preserves the visible search query but unconditionally reloads the unsearched list. A file:// reproduction changed an empty no-match result into the full list while leaving the query unchanged.
  3. legacy/fallback repeat navigation is incomplete: the controller-present branch has no repeat callback, the no-controller branch falls through when the reset helper is unavailable, and ordinary navigation runs before repeat reset in the legacy controller.
  4. a delayed cold browseCategory proxy is not canceled by a newer non-Browse navigation and can navigate the user back after they chose another view.
  5. the PR introduces a new __browseFrequencyFilter global writer and then classifies the collision as historical debt, bypassing the build guard that says new conflicts must fail.

There is also an unsatisfied process gate: issue #114 explicitly says implementation and PR creation must wait for maintainer confirmation. The issue has no confirmation comments or reactions, the PR has no maintainer approval, and the author has write but not maintain/admin permission. Please obtain an explicit maintainer ratification or waiver before merge.

Validation performed against the exact head: full JavaScript suite 82/82, Python unit suite 20/20, focused reset/latest-wins/on-demand suites, bundle freshness check (14 outputs), CI verify, and GitGuardian. The green tests do not exercise the failing timelines described in the inline comments.

Comment thread js/app/main-entry.js Outdated
Comment thread js/app/main-entry.js
Comment thread js/boot-fallbacks.js
Comment thread js/app/main-entry.js
Comment thread scripts/build-bundles.mjs Outdated
Preserve active searches across Browse re-entry, make delayed navigation and index refreshes latest-wins, and complete legacy repeat reset handling. Restore single-file ownership of the frequency filter global and rebuild generated bundles.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FOLLOW-UP BLOCKING REVIEW — changes are still required on head 3aceddd.

GitHub rejected the formal REQUEST_CHANGES event because the authenticated local gh identity is the PR author. An independent maintainer/reviewer must submit REQUEST_CHANGES after evaluating these findings.

The new commit fixes two prior findings: delayed cold browseCategory calls now respect navigation intent, and the newly introduced __browseFrequencyFilter conflict/allowlist entry is gone. However, three independently reproduced correctness blockers remain:

  1. A background examIndexLoaded refresh can invalidate a hot filter/category operation and discard the user's newer explicit selection.
  2. The query-aware showView path is overwritten by the second ExamSystemApp navigation handler on a real nav click, leaving the query visible above an unsearched list.
  3. The helper-free legacy reset updates CSS/category state but leaves the functional mode/path/frequency globals unchanged, so the refreshed results remain filtered.

The issue #114 implementation gate is also still unsatisfied: there is no maintainer confirmation, approval, or waiver. The PR author has write permission but not maintain/admin permission.

Validation on the exact head: focused Browse suites passed, full JavaScript suite 85/85, Python suite 20/20, syntax checks passed, bundle check passed with all 14 outputs current, CI verify passed, and GitGuardian passed. These green checks do not cover the reproduced timelines below.

Comment thread js/app/main-entry.js
Comment thread js/boot-fallbacks.js Outdated
Comment thread js/boot-fallbacks.js Outdated
Prevent background index refreshes from superseding active user filters. Keep Browse navigation query-aware across bubbling handlers and centralize complete reset state ownership. Add focused race, navigation, and helper-free regression coverage, then regenerate the affected bundles.

Refs #114

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking review of exact head 14e02e6. Treat this as REQUEST_CHANGES.

GitHub rejected the formal REQUEST_CHANGES submission because the authenticated gh identity is the PR author; the same five findings are therefore submitted as a blocking COMMENT review.

This commit resolves several earlier findings: default category/type filters now compose with the active search, the duplicate frequency writer/allowlist is gone, generated bundles are current, and the common bubbling-navigation double render is removed. It is still not merge-ready because three P1 regressions and two fallback/filter correctness defects remain; see the inline comments.

The process gate is also still unsatisfied: #114 explicitly requires maintainer confirmation before implementation/PR, but the issue still has zero comments/reactions and this PR has no maintainer approval or waiver. The author has write, not maintain/admin, permission.

Validation on this exact head:

  • scripts/build-bundles.mjs --check: 14/14 outputs current; 8 historical conflicts only.
  • Changed-source/bundle syntax: 10/10.
  • Focused suites: Browse navigation 12/12, legacy repeat 3/3, on-demand entrypoints 30/30, latest-wins pass.
  • Full JS runner: 87/87.
  • Real file:// navigation: default query preservation and default category/type intersections pass.
  • Scope audit: no dependency, workflow, data, docs, redesign, quick-picker, or unrelated UI expansion.

Please address every inline finding, add integrated regressions for the real module compositions, and obtain explicit maintainer ratification before merge.

Comment thread js/app/browseController.js Outdated
Comment thread js/app/main-entry.js
Comment thread js/app.js
Comment thread js/main.js
Comment thread js/boot-fallbacks.js Outdated
Resolve the five remaining PR #119 review findings by terminating frequency-folder render cycles, preserving active folder filters during search, replaying fresh indexes after retained user work, restoring first-activation setup, and failing closed until the functional reset owner is ready.

Regenerate the Browse, UI shell, and legacy app bundles and expand deterministic regression coverage. Refs #114.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking re-review for 8321b28f.

GitHub rejected the formal REQUEST_CHANGES submission because the authenticated account is the PR author, so this COMMENT carries the blocking findings; a separate reviewer or maintainer must apply the formal review state.

The latest-wins index replay, bundle parity, CI, generated-artifact parity, and file scope are clean. However, the six production-path cases below still violate the Browse navigation/reset contract and need regressions before merge. Separately, issue #114 still has no explicit maintainer confirmation or waiver, so its stated process gate also remains unsatisfied.

Comment thread js/main.js Outdated
Comment thread js/boot-fallbacks.js
Comment thread js/boot-fallbacks.js
Comment thread js/app.js Outdated
Comment thread js/boot-fallbacks.js Outdated
Comment thread js/app/main-entry.js Outdated
Apply folder scope before deduplication and serialize hydration-aware reset barriers.

Cancel stale navigation continuations, add focused regressions, and rebuild affected bundles.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking re-review of e82d3c1 — do not merge.

GitHub rejected the formal REQUEST_CHANGES submission because the authenticated account (k-undurkhaan-2) is the PR author. This COMMENT therefore carries the blocking findings; an independent reviewer or maintainer must apply the formal review state.

This head fixes folder-before-deduplication, ordinary warm re-entry, the cold proxy double load, and failed-barrier proxy leakage. The full 90-test JavaScript suite, focused race tests, bundle parity, syntax/diff checks, CI, and GitGuardian pass; the delta remains scoped to Browse reset/navigation handling and matching generated bundles.

Five correctness blockers remain: the three inline findings below, plus two previously reported threads whose acceptance failures remain reproducible after this head: cold query overwrite and cold reset undone by persisted hydration.

Separately, issue #114 still has no maintainer confirmation or waiver, and this PR has no independent approval.

Comment thread js/app.js
Comment thread js/main.js Outdated
Comment thread js/main.js Outdated
Preserve pending Browse intent and defer progress refreshes until active user requests settle.

Fail closed on reset persistence mismatches and prevent stale preference hydration after authoritative library changes.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision

Blocking — do not merge f29779b2. Three P1 correctness defects remain in the new activation/progress coordination.

Findings

See the three inline findings: a stale progress replay can discard a fresher authoritative index, a queued progress snapshot can cross a navigation/library epoch and overwrite a new activation, and a strict-on-demand pre-activation library reset is not made durable after Browse preferences load.

Verified repairs

The five previously reported direct scenarios now pass: superseded pending categories stand down, canceled explicit filters do not consume first hydration, warm pre-activation mutations suppress stale hydration, reset persistence fails closed, and the strict cold Ocean query remains query-aware.

Scope and validation

  • No scope expansion beyond Browse activation/reset coordination, focused tests, and matching generated bundles.
  • Handwritten source and generated bundle deltas match exactly (+221/-61 each); 14/14 bundles are current with only the existing eight allowlisted conflicts.
  • Full JavaScript suite: 96/96. Python unit suite: 20/20. Focused navigation/latest-wins/library/on-demand suites and production file:// Chromium probes pass.
  • CI verify and GitGuardian are green; the commit is signed and linear.

Handoff

Coordinate progress and examIndexLoaded replay through one freshness/priority arbiter, attach navigation and active-library epochs to queued progress work, and persist the authoritative live filter once lazy Browse preferences become available. Add combined production-order regressions for each timeline.

The issue #114 maintainer-confirmation gate also remains unsatisfied, and the PR still has no independent approval.

Comment thread js/main.js
Comment thread js/main.js Outdated
Comment thread js/main.js
Bind background progress snapshots to their source library and queued UI epochs, coalescing replacement syncs after every index publication.

Persist authoritative Browse filters through a durable readback before consuming lazy hydration, with production-order regression coverage.

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision

Blocking — do not merge d86fee32. The three prior P1 scenarios are repaired, but one stale-sync correctness defect and two arbitration regressions remain.

Findings

See the three inline findings: direct practice-record syncs still bypass latest-started arbitration and can restore stale completion/Practice state; identical in-flight joins now force redundant tail syncs; and progress can bypass a cold functional-reset barrier that later fails closed.

Verified repairs

The previously reported index-versus-progress ordering, queued navigation/library/reset epoch, and strict-on-demand durable-filter scenarios now pass production-module and file:// probes, including failure/retry paths.

Scope and validation

  • No unrelated feature or UI expansion was introduced; the broader record-sync scheduler changes are tied to the reported Browse progress race.
  • Handwritten source and generated bundle deltas match exactly (+231/-20 each); all 14 generated outputs are current and no symbol conflict was added.
  • Focused JavaScript checks: 23/23. Full JavaScript suite: 96/96. Python suite: 20/20.
  • CI verify and GitGuardian are green; the new commit is signed, linear, and directly descends from the previously audited head.

Merge gate

Issue #114 still contains an explicit maintainer-confirmation-before-implementation gate, but it remains open with no confirmation recorded and this PR has no approving maintainer review. That gate must be satisfied independently of the code fixes.

Handoff

Put every practice-record refresh behind one monotonic latest-started arbiter and reject stale cycles before any cache or UI mutation. Preserve true in-flight deduplication unless a newer data/library epoch requires a replacement. Connect progress arbitration to the cold functional-reset barrier, then add the overlapping direct-sync, identical-join, and barrier-failure regressions described inline.

Comment thread js/main.js
Comment thread js/main.js Outdated
Comment thread js/main.js Outdated
- arbitrate direct and managed practice syncs by invocation and data/library epochs
- deduplicate equivalent in-flight refreshes while preserving current replacement tails
- include cold functional reset barriers in Browse progress rendering

@k-undurkhaan-2 k-undurkhaan-2 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision

Blocking — do not merge 779dbc4a. The three previously reported scenarios are repaired, but this head introduces four remaining race/lifecycle blockers; see the inline findings.

Verified repairs

  • Direct and managed practice syncs now preserve the newest projection when an older invocation settles last.
  • Equivalent in-flight requests and the fallback Practice navigation share one physical read while genuine newer library/data work still gets one coalesced tail.
  • The intended functional-reset cases now hold: progress stays suppressed while pending, a successful reset emits only its canonical snapshot, overlapping functional barriers are latest-wins, and an explicit second functional reset recovers from failure.

Scope and validation

The delta from d86fee32 is limited to one focused test file, three handwritten runtime files, and their three generated bundles (+1091/-96). No unrelated UI, dependency, workflow, data, or documentation change was found, and all generated bundles match their sources. Validation passed: affected JS 23/23, full JS 92/92, Python 20/20, bundle parity 14/14 (the same 8 historical conflict warnings), syntax checks, and git diff --check. The commit is a verified single-parent descendant; CI verify and GitGuardian are green. The passing suites do not exercise the four reported interleavings.

Merge gate

Issue #114 is still open and still requires maintainer confirmation before implementation; it has no confirming comment. The PR also has no independent approving review. That process gate remains unsatisfied independently of the code findings.

Handoff

The next head should make commit invalidation itself schedule one current-data replacement, retain the data epoch through queued Browse progress, bind functional barriers to navigation/reset ownership, and retire a failed barrier after any successful canonical recovery. Add deterministic coverage for each exact interleaving before requesting another review.

Comment thread js/main.js
return PRACTICE_RECORD_ENTITY_STORES.has(store);
});
if (touchesPracticeRecords) {
practiceRecordsDataGeneration += 1;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Requeue refreshes invalidated by practice-data commits

This listener advances the epoch but does not schedule a replacement. If a summaries/details BroadcastChannel commit lands while syncPracticeRecords() is awaiting its reads, the guard below returns from the only cycle successfully without updating the signature, completion cache, anchors, Browse, or Practice, and no pending request remains. An annotation save hits the same path even though this projection does not read annotations. Production probes observed one physical read and zero projection updates or tail, while the same timing on d86fee32 applied once. Restrict invalidation to consumed stores and coalesce a mandatory current-data follow-up whenever a relevant commit invalidates any active direct or managed cycle.

Comment thread js/main.js
? source.activeLibraryGeneration
: readBrowseProgressGeneration('__getActiveLibraryGeneration'),
resetGeneration: readBrowseProgressGeneration('__getBrowseResetIntentGeneration'),
functionalResetGeneration: functionalResetState.generation,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the practice-data epoch in queued progress work

This epoch captures navigation/library/reset state but drops source.practiceRecordsDataGeneration, even though syncPracticeRecords() supplies it. A pre-commit progress snapshot queued behind a user request therefore remains current after a summary/detail commit. In a production composition, settling the user request replayed A after the commit and consumed the newer results token; when the current-data sync failed, A remained final. Carry and compare the data generation through pendingBrowseProgressRefresh (or clear pending progress on commit), and cover a commit between enqueue and settlement.

Comment thread js/boot-fallbacks.js
if (browseFunctionalResetBarrier
&& window.AppEntry
&& typeof window.AppEntry.isBrowseFunctionalResetBarrierCurrent === 'function'
&& !window.AppEntry.isBrowseFunctionalResetBarrierCurrent(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Cancel a functional reset after newer navigation intent

This currentness check only compares the barrier object. It never binds the continuation to the navigation generation marked by showView, the ordinary reset generation, or active Browse ownership. In the production path, Browse (reset pending) → Overview → Browse(false) → a delayed P2/reading filter allowed the old reset to claim the newer results token; the filter returned false and all/all remained final. The same continuation can render after leaving Browse or overwrite a newer ordinary reset. Capture and check those epochs plus the active view before refreshing, and cancel superseded barriers without leaving a failed global state; add the ABA paths.

Comment thread js/main.js
return;
}
const functionalResetState = readBrowseFunctionalResetState();
if (functionalResetState.status === 'failed') {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not let one failed barrier poison later successful recovery

The failed status is global and only a later showView('browse', true) replaces it. Actual NavigationController recovery uses showView(viewName, false), and repeated Browse uses resetBrowseViewToAll(); both can successfully render the canonical list while this generation stays failed, so every subsequent practice-progress snapshot is cleared here until reload or an atypical true reset. Production probes observed the successful recovery/repeat render followed by zero progress renders. Complete or replace the failed generation on any successful canonical activation/reset, or scope this gate to work owned by the failed activation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant