Fix #32: Architecture: Deepen the result handoff: one module at the route seam - #33
Conversation
|
Please revise this PR before merge: (1) |
Kingson4Wu
left a comment
There was a problem hiding this comment.
PR Review: Consolidate result storage into useResultStore
Scope: Replaces useHistory.js (localStorage only) and absorbs formatMbtiType.js into a single useResultStore.js that owns the sessionStorage handoff (mbti_result), the localStorage history (mbti_lab_result), type normalization, entry validation, and fresh-then-history precedence. Views are updated; formatMbtiType.test.js is replaced by useResultStore.test.js; one new ResultView regression test is added.
Verification performed
npx vitest run→ 7 files / 78 tests passednpm run build→ succeeded (pre-existing >500 kB chunk warning only)grepacrosssrc/andtests/→ no dangling imports ofuseHistoryorformatMbtiType; no raw reads ofmbti_result/mbti_lab_resultoutside the new module- Backward compatibility: the saved result shape (
{ type, percents }fromuseTest.js) has been stable since the initial commit (dd54182), andHISTORY_KEYkeeps the oldmbti_lab_resultname, so existing users' persisted results pass the new validation and still load. - Refresh behavior on
/resultis preserved:submit()writes both storages, and afterload()consumes the session entry, refresh falls back to an identical history entry.
Findings
Overall this is a clean, well-tested refactor that fixes the reported crash (invalid session entry reaching ResultCard). Findings below are minor / non-blocking.
-
Dead code:
clear()is never called —src/composables/useResultStore.js:89-92
No view or test-other-than-its-own invokesclear(). The oldclearResultwas equally unused, so this is pre-existing API surface carried forward. Either wire it to a "clear history" affordance or drop it. -
submit()performs no validation,load()validates strictly —src/composables/useResultStore.js:73-76vs:36-67
If a caller ever submits a malformed result, it is persisted to history and then silently rejected on every subsequentload()— the "last result" feature stays broken until a valid test overwrites it. Current callers (useTest→TestView.vue:52) always produce valid shapes, so this is not a live bug, but validating insubmit(or returning the parsed entry from it) would fail fast instead. -
Minor test-coverage regression: whitespace trimming — deleted
tests/formatMbtiType.test.js(cases' enfp ','\tistj\n') vstests/useResultStore.test.js:154-158
normalizeTypestill trims, but only the lowercase case ('infp') is re-covered; the trim cases were dropped andnormalizeTypeis not exported, so they can only be exercised throughload(). A one-lineputSession({ type: ' enfp ', ... })case would restore parity. -
Note (intentional, no change requested): HomeView now consumes the fresh handoff —
src/views/HomeView.vue:62
load()is read-and-consume, so a Home visit eats any not-yet-consumed session entry. In current flows both storages hold identical entries aftersubmit, so this is benign and is exactly the mechanism that prevents a stale session entry from shadowing newer history (documented in the module header and covered by the "tab 2" test attests/useResultStore.test.js:100-118). Flagging only so reviewers are aware of the semantic change to HomeView.
Conclusion
Approve. The core fix is correct and regression-tested (tests/ResultView.test.js:100-110 proves a corrupted session entry renders the empty state instead of crashing ResultCard), storage keys are backward compatible, and all 78 tests plus the production build pass. Findings 1-3 are optional polish.
RalphWorks review run: https://github.com/OctopusGarage/mbti-lab/actions/runs/35680692179
|
RalphWorks addressed PR feedback in f9ba9f0. Run: https://github.com/OctopusGarage/mbti-lab/actions/runs/35680753178 |
Closes #32
Generated by RalphWorks. Execution and artifacts. Review the patch and required checks before merging.