Skip to content

Filter problems and summarize recent practice - #108

Open
Danielncku wants to merge 2 commits into
sysprog21:mainfrom
Danielncku:feature/lobby-practice-filters
Open

Danielncku wants to merge 2 commits into
sysprog21:mainfrom
Danielncku:feature/lobby-practice-filters

Conversation

@Danielncku

@Danielncku Danielncku commented Sep 25, 2026 •

Copy link
Copy Markdown

What this does

The lobby already lets candidates choose a difficulty, but it offers no way to narrow the problem bank by topic. This change adds a local topic selector that combines with the existing difficulty checkboxes and limits both the visible cards and the recommendation pool. The selection is remembered in local storage.

Problem topics are generated from the existing problem bank and stored only as card metadata. They are not rendered on the cards, so the scenario does not reveal the underlying problem category.

The lobby also shows a compact snapshot of the latest eight assessed interviews. It reports passes, misses, pass rate, the current pass or miss streak, and topics with more misses than passes. Only dated reports with an explicit HIRE or NO_HIRE decision are counted. The snapshot stays hidden when there is no assessed history or report loading fails.

Not included

This version does not track interview starts, incomplete sessions, account-specific local state, or a Failed Before filter. Those earlier changes were removed to keep the pull request focused on topic selection and report-derived insights.

Validation

Focused unit tests cover topic extraction, combined difficulty and topic filtering, recent-result ordering, pass-rate calculation, unscored reports, and weak-topic detection.

Playwright tests cover filtering and reset behavior, rendering the recent snapshot from assessed reports, and hiding it when report history cannot be loaded.

ESLint, Prettier, and the generated problem-card consistency check pass locally. The lobby and /interview routes both return 200 from the official Windows Rust server with this checkout's web/ override.

The full browser suite is not green on this Windows checkout: 16 failures remain in avatar, vendor, and source-policy checks outside the changed paths, so this description does not claim a fully green local gate.

cubic-dev-ai[bot]

This comment was marked as resolved.

@jserv jserv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check https://cbea.ms/git-commit/ carefully and enforce the rules.

@ColtenOuO ColtenOuO left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you also include a commit body? It will make it much easier to understand the changes when tracking them down in the future.

Comment thread web/practice-insights.js Outdated
@@ -0,0 +1,154 @@
const ASSESSED = new Set(["HIRE", "NO_HIRE"]);
const PRACTICE_ATTEMPTS_KEY = "codetrial.practiceAttempts";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Practice attempts are stored under one origin-wide key, while signed-in reports are scoped to the current GitHub account. If user A starts an interview and user B later signs in on the same browser, B will inherit A's attempted/incomplete state. This affects the practice-status filters, recent-start count, and recommendation pool, and may also expose that another user started a particular problem.

@Danielncku Danielncku Sep 26, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi Colten, long time no see!Fixed in 4d9caf7.(Sorry i didn't review the code well and misunderstood that all record will only store in local memory)Practice starts are now stored under a normalized per-account key (with a separate anonymous scope), and the lobby switches scopes after loading the current session. I also added unit coverage and a real-browser Alice-to-Bob account-switch test to verify that attempted filters do not leak across accounts.

@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch from 058eaec to 4d9caf7 Compare September 26, 2026 01:35
@Danielncku

Danielncku commented Sep 26, 2026 •

Copy link
Copy Markdown
Author

Addressed the requested changes in 4d9caf7:

  • Squashed the branch into one commit following the referenced commit-message rules, including an explanatory body wrapped at 72 columns.
  • Scoped browser-local practice attempts to the current normalized GitHub login, with a separate anonymous scope.
  • Added unit and Playwright coverage for account isolation.
  • Changed small topic metadata from --muted to --sub for sufficient contrast.
  • Preserved the existing behavior that prioritizes due reviews outside the currently suggested difficulty.

Verification:

  • 77/77 focused Node tests passed.
  • 57/57 real-browser lobby tests passed.
  • ESLint, JavaScript syntax checks, generated problem-card check, and git diff --check passed.

The complete Windows run executed 627 tests: 608 passed, 5 skipped, and 14 unrelated pre-existing platform/vendor failures remained (Windows ESM paths and CRLF-sensitive checks, plus missing MediaPipe vendor assets).

cubic-dev-ai[bot]

This comment was marked as resolved.

@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch from 4d9caf7 to eabeacd Compare September 26, 2026 01:40
Comment thread scripts/gen-problem-cards.py Outdated
Comment thread web/app.js Outdated
Comment thread web/app.js Outdated
Comment thread web/app.js Outdated
Comment thread web/app.js Outdated
Comment thread web/app.js Outdated
// real filter that matched zero problems.
if (!nodes.problemTopic.value || !nodes.problemStatus.value) return;
const visible = new Set(filteredCards().map((card) => card.id));
for (const card of cards) card.button.hidden = !visible.has(card.id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hiding a card here does not clear a hand-picked selection, and setProblem owns the rule that a selected card the page has hidden goes back to null. With Not practiced active: hand-pick a card, start it, come back through bfcache, and the start recorded for it makes this line hide the card while manualProblem is still set, so recommend() returns early and settle() re-enables Start over a card nobody can see. Drop the pick when the filters hide it, the way the difficulty handler already does.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

applyProblemFilters now clears a manual selection through setProblem(null) whenever the selected card becomes hidden. A bfcache restore regression test records the start, reapplies Not practiced, and verifies that the hidden card cannot remain startable.

Comment thread web/app.js Outdated
Comment thread web/app.js

function showProgress(entries, suffix) {
renderPracticeFocus();
renderRecentPerformance();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

erasable just below is entries.length > 0 || readDeviceHistory().length > 0, so a candidate whose only local practice data is recorded starts gets #history-header hidden and no Delete control, while this call still prints 1 recent interview start saved locally. That makes the new store the one thing the lobby writes to the device that the candidate cannot erase. Count practiceAttempts.length in erasable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

practiceAttempts.length is now part of the erasable condition. A start-only browser test confirms that Delete is visible, removes the active and anonymous start data, and preserves another signed-in account scope.

Comment thread web/practice-insights.js Outdated
Comment thread web/practice-insights.js Outdated
jserv

This comment was marked as outdated.

@Danielncku

Danielncku commented Sep 27, 2026 •

Copy link
Copy Markdown
Author

Hi @jserv, thank you very much for taking the time to provide such a detailed review. I have replied in each thread with my current understanding and proposed direction. As I am still learning the project’s design conventions, I would like to make sure my interpretation is correct before changing the implementation.

Two details—the deletion boundary across local scopes and the count-based incomplete model—seem especially worth confirming first. I will therefore hold off on pushing another revision for now. Once the intended behavior is clear, I will rebase onto the latest main, resolve the conflicts carefully, implement the agreed changes together, run the full relevant test suite, and then update this pull request. Please feel free to correct any part of my understanding.

@jserv

jserv commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Once those interpretations are confirmed, I will rebase onto the latest main, resolve the conflicts, implement the agreed changes together, run the full relevant test suite, and then update this pull request.

Always rebase onto the latest main and synchronize with the latest changes before making further updates, to minimize potential code duplication and merge conflicts.

Don't have to say greeting. Focus on efficient discussions.

@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch from eabeacd to 62b5bd7 Compare September 27, 2026 03:00
@Danielncku

Copy link
Copy Markdown
Author

Rebased onto upstream/main at 42c3b08 before applying the review changes, resolved the app.js conflicts, and updated this PR with commit 62b5bd7. The ten inline issues have been addressed and replied to individually. Validation completed: 86 focused Node tests passed; 8 targeted Playwright regression scenarios passed; ESLint, JavaScript syntax checks, generated-card consistency, and git diff checks passed. I will use the same sync-first workflow for any further update.

@jserv

jserv commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Rebased onto upstream/main at 42c3b08 before applying the review changes, resolved the app.js conflicts, and updated this PR with commit 62b5bd7.

There is NO need to make statements like this. GitHub already tracks every commit. Speak like a human instead of producing AI slop.

@ColtenOuO

Copy link
Copy Markdown
Collaborator

Please rebase onto the latest main and resolve any conflicts.

jserv

This comment was marked as outdated.

@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch 2 times, most recently from 8e99d68 to a45ce6e Compare September 28, 2026 01:41
@Danielncku

This comment was marked as outdated.

@jserv
jserv requested a review from ColtenOuO September 28, 2026 11:38
Comment thread web/practice-insights.js Outdated
/// Move the origin-wide preview key into the first confirmed account scope.
/// Write the scoped copy before removing the legacy key, so blocked storage
/// leaves the only copy intact and a later page load can retry safely.
export function migrateLegacyPracticeAttempts({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

migrateLegacyPracticeAttempts is back on this head, although the reply on the practice-insights.js:86 thread says the unscoped migration was removed entirely. codetrial.practiceAttempts has never existed on main, so every piece of code that serves it handles data nothing has written: this function, the removeItem of the bare key in clearPracticeAttempts, and the unbased count-pairing branch in incompletePracticeAttempts, whose own comment says those entries "are not part of a shipped key".

It also no longer does what the earlier reply to cubic described. selectPracticeAttemptScope runs it for any confirmed session, including the { signedIn: true, user: { login } } that recordGitHubLogin passes in, and the test at practice-insights.test.js:234 is named "legacy attempts migrate into the first confirmed account scope". So the first account to sign in on the browser claims whatever the key holds, which is the cross-account leak the scoping was added to prevent. Since there is no shipped data to preserve, could all of it go, together with its two tests? That leaves readPracticeAttempts as the only reader of the store.

Comment thread tests/browser/practice-insights.test.js Outdated
Comment on lines +219 to +221
setItem: () => {
writes += 1;
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The count-based rule assumes a problem's assessed count only ever grows, but the per-report Delete now on main lowers it. Take a problem interviewed twice, both assessed: the two starts carry baselines 0 and 1, and a count of 2 marks both as complete. Delete one of those reports and the count drops to 1, so the second start (1 <= 1) becomes Incomplete / unscored again. It then shows up in that filter and adds to "N incomplete interview starts saved locally". The candidate deleted a result they had, and the lobby now reports an interview they never finished. Could a start remember which report resolved it, or could a delete also remove the start it answered? Either way, please pin it with a test that deletes one of two reports.

Comment thread web/app.js Outdated
Comment on lines +1239 to +1243
reports = [];
progressNormalized = [];
renderPracticeFocus();
renderRecentPerformance();
applyProblemFilters();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

showProgressError sets reports = [] and then renders the recent-performance box. With every count at 0, each recorded start satisfies 0 <= assessedCount, so a failed /api/reports shows "N incomplete interview starts saved locally", and Incomplete / unscored lists every problem ever started. That sits right next to "Could not load saved account progress." Hiding the box, and leaving the status filter alone, while history is unavailable would avoid stating something the page cannot know.

@jserv jserv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rebase the current branch onto the upstream default branch and rework the series into functionally minimal commits, folding similar ones and enforcing the project's commit message rules.

@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch from a45ce6e to 763a0ba Compare October 1, 2026 03:10
@Danielncku Danielncku changed the title Add topic and practice-status filters to the lobby Filter problems and summarize recent practice Oct 1, 2026
@Danielncku

Copy link
Copy Markdown
Author

I narrowed this pull request to topic filtering and a recent practice snapshot. The topic selection now combines with the existing difficulty filter and stays local to the browser; the snapshot counts only dated HIRE or NO_HIRE reports and stays hidden when history cannot be loaded.

The earlier incomplete-start and account-scoping changes have been removed. Focused unit and browser tests cover filtering, snapshot calculation, and report-load failure handling.

@ColtenOuO ColtenOuO left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Check the CI error logs and fix the issues.

Expose problem topics as local card metadata so candidates can narrow
the lobby without revealing tags on each card. Keep recommendations
inside the selected topic and cover combined filters in unit and browser
tests.
Show a compact snapshot for the latest assessed interviews with pass
rate, current streak, and topics that have more misses than passes. Hide
the snapshot when history is unavailable and cover both success and
failure paths.
@Danielncku
Danielncku force-pushed the feature/lobby-practice-filters branch from 763a0ba to ac27118 Compare October 1, 2026 14:01
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.

3 participants