fix: recalculate stats picks wrong leaf on duplicate node names - #14
Conversation
The fallback in findLastValidPath used validLinks.find() to return the first tree node whose link ended with the last path segment. This was ambiguous when a participant backtracked through multiple branches and two or more sibling leaves shared the same name — the wrong node could be returned, flipping successful from true to false and producing incorrect Task Path Outcomes (Indirect/Direct Success → Fail) after Recalculate Stats. Replace the single-segment heuristic with a progressive multi-segment suffix match: build suffixes of increasing length from the tail of pathTaken until exactly one valid link matches, giving the most context-aware and unambiguous result. Closes #13
Adds a changelog entry prompting owners to re-run Recalculate Stats on previously-recalculated studies, bumps LATEST_UPDATE_DATE so the unread indicator lights up, and drops internal design notes covering the root-cause analysis, a short-term schema fix, and a proposed V2 event-sourced overhaul for tree testing.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR replaces the single-segment fallback in Confidence Score: 5/5Safe to merge — the core algorithm is correct and well-tested against the documented edge cases; only pre-existing P2 gaps remain. No P0 or P1 issues introduced. The progressive suffix algorithm correctly handles duplicate leaf names and backtracking paths. The early-break optimisation is provably safe (if no link ends with suffix S, no link ends with any longer prefix of S). The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolveSelectedLink(validLinks, pathTaken)"] --> B["Sort links by length ↓ (sortedLinks)"]
B --> C{"Step 1: pathTaken.endsWith(link)\nfor each sortedLink"}
C -->|"Exact suffix match"| R1["Return link (longest wins)"]
C -->|"No match"| D{"Step 2: repeat last segment\npathTaken + '/' + lastSegment"}
D -->|"Repeated suffix match"| R2["Return link"]
D -->|"No match"| E["Step 3: progressive suffix loop\nnumSegments = 1 → N"]
E --> F{"suffix = last numSegments\nof pathTaken"}
F --> G["matches = sortedLinks\n.filter(endsWith suffix)"]
G -->|"matches.length === 1"| R3["Return matches[0]"]
G -->|"matches.length === 0"| H["break — longer suffix\nwon't match either"]
G -->|"matches.length > 1"| I{"Is this a smaller\nambiguous set?"}
I -->|"Yes"| J["Update lastResortMatch\n& currentBestMatchCount"]
J --> F
I -->|"No"| F
H --> R4["Return lastResortMatch\n(null if nothing found)"]
Reviews (3): Last reviewed commit: "refactor(treetest): split findLastValidP..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect task outcome flips caused by Recalculate Stats mis-resolving a participant’s selected leaf when path_taken contains backtracking segments and/or duplicate leaf names exist in the tree.
Changes:
- Updates
findLastValidPathto use progressive multi-segment suffix matching to better infer the intended selected link frompathTaken. - Adds a dashboard update/changelog entry notifying owners to re-run Recalculate Stats, and bumps the “latest update” date used for unread-badge logic.
- Adds internal design notes documenting the root cause and proposing schema/model improvements.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lib/treetest/actions.ts |
Improves path resolution heuristic used during stats recalculation. |
src/app/(main)/dashboard/updates/updates-list.tsx |
Adds an updates feed entry about the fix. |
src/app/(main)/dashboard/_components/dashboard-nav.tsx |
Updates the constant used to detect unread updates. |
docs/tree-testing-notes.md |
Adds internal notes on the issue, fix, and longer-term redesign options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR review pointed out that lastResortMatch was only captured on the first ambiguous iteration (numSegments=1), so when a later, more specific suffix narrowed the candidate set and then hit zero matches, the fallback returned a candidate from the least-specific window. Track currentBestMatchCount alongside lastResortMatch and update the fallback whenever a smaller ambiguous set is found, so the returned candidate always reflects the longest suffix that still matched. Also updates tree-testing-notes.md to reflect the actual findLastValidPath(tree, pathTaken) signature and document all four matching steps (the doc previously only mentioned the old broken fallback).
|
Addressing the This one is deliberately out of scope for this PR — the same reasoning is captured in Quick rationale for why I don't want to patch it here: computing The right fix ships with the short-term schema work documented in §2 of the notes: add |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR review noted that the inline comment still described the old single-segment heuristic as the ambiguous-case fallback, which no longer exists — the current code returns the first candidate from the smallest ambiguous suffix set instead. Rewrites the comment to describe what the code actually does.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…stic PR review raised two points on the recalculate path: 1. Performance — findLastValidPath rebuilt validLinks via a full tree walk on every call, and the ambiguous-set filter allocated a fresh array per suffix length. In recalculateStudyResults (and the answer-changed branch of saveStudyData) this ran once per result row, so the tree walk was redundant for every row after the first. 2. Determinism — the lastResortMatch fallback picked matches[0] from validLinks in DFS order, which meant sibling reordering in the tree could flip a result on re-recalculation even though nothing substantive changed. Splits the resolver into collectValidLinks(tree) + resolveSelectedLink(validLinks, pathTaken), with findLastValidPath as a thin wrapper for single-shot callers. Both hot callers now collect the link list once and pass it into the per-row loop. The suffix-match loop now filters from the length-sorted list so ambiguous-set picks are stable under tree reorganization and consistent with the "longest/most-specific wins" heuristic used by step 1. Also updates the design notes to describe the new two-function shape.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up to the previous refactor, addressing two PR review findings: 1. The length-desc sort had no secondary tie-breaker, so same-length links kept their Array.sort-stable input order — i.e., DFS traversal order. Reordering siblings in the tree could still flip the ambiguous-fallback winner. Adds a localeCompare tie-breaker so sort order is fully independent of tree traversal. 2. resolveSelectedLink still cloned+sorted validLinks on every call, wasting O(leaves log leaves) per result row in both hot callers. Extracts sortValidLinks as its own helper so callers can hoist it once outside their per-row loop; resolveSelectedLink now takes the pre-sorted list directly. findLastValidPath keeps working as a single-shot wrapper. Both hot callers (saveStudyData's answer-changed branch, and recalculateStudyResults) now compute sortedLinks once per study.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…h_taken Two follow-ups from PR review: 1. findLastValidPath was dead code after both hot callers were moved to use the three-helper API directly. Dropped the wrapper rather than pinning it with a speculative single-shot call site; adding it back later is trivial if a real caller appears. 2. The design notes wrongly framed recalculateStudyResults not updating direct_path_taken as a "drift out of sync" bug. It isn't one: direct_path_taken is derived from (selectedLink === pathTaken) at submission — it's a directness measure, independent of answer correctness. Editing expected_answer has no bearing on whether the participant backtracked, so recalc shouldn't touch it. The notes now say so, and push the direct_path_taken re-derivation concern into the short-term selected_link backfill section where it actually applies.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tightens the #28 update entry to the two things users actually need (what happened and what to do), and removes the internal design notes from version control — keeping them as a local-only working doc.
Problem / Intent
Addresses #13.
Study owners clicking Recalculate Stats sometimes saw previously-correct task results flip to failures (and occasionally the reverse). The problem was worst on trees with duplicate leaf names, or when participants backtracked between branches before making a final selection.
Root cause: a participant's final leaf selection was never persisted — only a concatenated
path_takenstring. Recalculate had to reverse-engineer the selection from that string, and its fallback matched the first link whose terminal segment matched, regardless of which branch the link actually lived in. When multiple leaves shared a name (e.g. twocontact-usnodes under different parents), matching was effectively arbitrary.This button has been live since January 2026, so any historical results that were previously recalculated on affected studies may be inaccurate.
Approach
Replaced the last-segment fallback with progressive multi-segment suffix matching: the resolver builds progressively longer trailing suffixes from
path_takenand keeps growing the window until exactly one valid link matches. This uses the full navigation context instead of just the last path segment, so the correct branch is identified unambiguously even when multiple leaves share a terminal name.Owners who ran Recalculate before this fix are prompted via the changelog to re-run it and refresh their results.
Also includes internal design notes (
docs/tree-testing-notes.md) capturing:selected_linkso Recalculate doesn't have to guess from a string),