feat(history): add per-task version history - #37
Conversation
lefarcen
added
size/XL
|
Heads-up: #33 is also open against the same You and @cszhouwei may want to compare scope so the focused bugfix and the larger feature work do not step on each other. The maintainer team will decide which path lands. |
joeylee12629-star
left a comment
joeylee12629-star
left a comment
There was a problem hiding this comment.
Thanks for the thorough write-up and QA evidence — feature itself looks solid. A few things to address before we go deeper:
Blocker: branch is significantly out of date
Merge base is b699e8a and main has moved 9 commits since, so the current diff includes reverts of recent work that would silently undo it on merge:
| Already-merged PR | What gets reverted in #37 |
|---|---|
| #47 in-app community links | <CommunityLinks /> removed from toolbar |
| #24 / #44 one-click Vercel publishing | <DeployControl /> removed from toolbar; onRequestConfigureDeploy deep-link removed from page.tsx; configured-state probe + steer-to-settings flow removed from deploy-control.tsx |
| #44 Publish UX polish | btn-ink styling reverts to inline styles |
| #33 deploy-control empty-selector fix | PR description claims this fix, but it's on main as 6740f27; the 3 lines here are pure conflict source — please drop them and update the description |
Could you rebase onto origin/main and keep only the history-feature changes (db.ts, db.test.ts, history-pane.tsx, the history.* i18n keys, the HistoryToggle button + <HistoryPane /> mount in page.tsx, and the commitBaseFor / deleteTask integration in store.ts)? Once that's in we can review the new code in isolation.
Feature-level feedback (after rebase)
-
history-pane.tsx:217—isCurrent={r.html === activeHtml}does full-string equality every row, every render. With 100KB+ HTML that's expensive. It also produces ambiguous "current" markers after Restore: the snapshot v_{n+1} has the same HTML as the restored row, so both rows rendercurrentand Restore disables on both. Compare by version number againstruns[0]?.versioninstead. -
history-pane.tsx:58-61— refresh keyed onactiveHtml. During a streaming Convert,setHtmlForfires per delta, solistRunshits IDB once per delta. Suggest gating onstatus === "done"transitions, or on a dedicated history-version selector exposed from the store. -
store.tscommitBaseForcaptures the snapshot via a side effect inside theset()updater. Functional but Zustand makes no guarantee about updater invocation count. Cleaner: read viaget()first, thenset(). -
Hook-ordering follow-up. We just landed #28 fixing a "Rendered fewer hooks than expected" crash in
convert-chip.tsxcaused by an early return placed before the hooks (it bricked the app wheneverlayoutModehad been persisted asprevieworeditor).HistoryPane'sif (!open) return null;at line 72 is technically safe — all 18 hooks run before it — but the pattern is fragile to future edits. Safer to gate at the parent: readhistoryPaneOpeninpage.tsxand render{historyPaneOpen && <HistoryPane />}so the component only mounts when needed. -
Persist
versionshould bump 7 → 8. The migrate cascade instore.tshistorically gets one entry per added persisted field, even no-op, for auditability. AddinghistoryPaneOpenwithout a bump works (Zustand shallow-merges), but breaks the convention. -
SourceDiffhas no virtualization. 250KB HTML diff can render thousands of<div>s. PR description notes scroll "remained responsive" — believable for the tested input but worth flagging as follow-up before users hit pathological cases.
Test coverage on db.ts is great. pnpm test (5/5) and pnpm build both pass as-checked-out, so the patch in isolation is healthy — the rebase is the primary blocker.
|
Adding one update now that the public harness PR has landed on Please keep the earlier review guidance, but map the files like this:
Recommended validation after rebase:
I am not pushing directly here because this is a larger feature branch and the existing review asks for scope cleanup before another code pass. |
Resolve conflicts after the workspace move into next/:
- Move src/components/history-pane.tsx -> next/src/components/history-pane.tsx
- Move src/lib/history/{db,db.test}.ts -> next/src/lib/history/
- Keep both HistoryToggle and CommunityLinks in toolbar.tsx
- Merge history.* and community.* strings in both i18n locales
- Add `diff`, `idb`, and `fake-indexeddb` to next/package.json
- Drop top-level vitest.config.mts (next has its own)
- Regenerate pnpm-lock.yaml
|
Resolved conflicts with Conflict resolution
Verification
|
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 Thanks for the carefully built history feature. The IndexedDB store design is solid — the soft-failure fallbacks when IDB is unavailable, the per-task GC cap, the composite by-task-version index for the version lookup, and the fake-indexeddb test suite all look good, and the post-rebase mapping into the next/ workspace matches the maintainer guidance.
I found one non-blocking correctness issue in the History pane's "current version" detection — details inline. It does not block merge.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
| <HistoryCard | ||
| key={r.id} | ||
| run={r} | ||
| isCurrent={r.html === activeHtml} |
There was a problem hiding this comment.
The isCurrent flag is derived purely from HTML-content equality, so it can flag more than one version as "current."
isCurrent={r.html === activeHtml} marks every run whose html byte-matches the editor's current HTML, not just the live version.
This breaks deterministically on restore. onRestore sets the editor HTML to record.html and then calls commitBase(activeTaskId), which appends a brand-new version whose html equals the restored version's html. Both the new run and the restored source run then satisfy r.html === activeHtml, so the list renders two current badges, and — because the Restore button is disabled={isCurrent} (line 287) — the older identical version becomes wrongly un-restorable. The same false positive occurs any time two converts happen to emit byte-identical HTML.
Why it matters: the History pane's core job is to tell the user which version is live and let them roll back to any other one. A duplicated current marker plus a disabled Restore button on a genuinely-historical version is a visible regression in exactly the restore flow this PR adds.
Suggested fix — anchor current to the newest run, keeping the content guard so the badge correctly disappears once the user edits the editor after a commit:
isCurrent={r.id === runs[0]?.id && r.html === activeHtml}listRuns already sorts newest-first (b.version - a.version), so runs[0] is the live version. A small regression test for the restore path — after restoring an older version, only the newest run is flagged current and the older identical run stays restorable — would lock this behavior in.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Adopted — fixed in 4ea928b.
Anchored the predicate to the newest row (r.id === runs[0]?.id && r.html === activeHtml) and extracted it into a pure helper at next/src/lib/history/is-current.ts so the regression is unit-coverable. next/src/lib/history/is-current.test.ts adds 4 cases:
- after a restore, the older byte-identical run is no longer flagged current and stays restorable;
- when the editor html drifts from the newest run, the badge clears;
- empty runs list returns false (guards the
runs[0]?.iddegenerate case); - the baseline newest + matching scenario.
pnpm exec tsx scripts/guard.ts, pnpm -F @html-anything/next typecheck, pnpm -F @html-anything/next test (68/68), and pnpm -F @html-anything/next build all pass.
After Restore, commitBase appends a new version whose html equals the restored row's html, so a pure r.html === activeHtml check flagged both rows as current and disabled Restore on the genuinely-historical one. Anchor on runs[0].id (newest, per listRuns ordering) and keep the content guard so the badge still clears when the editor drifts. Extracted to isCurrentRun helper for unit coverage of the restore path, byte-identical older versions, editor drift, and empty runs.
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 Thanks for the per-task version history feature, and for the focused follow-up on the earlier review.
I reviewed the current head (4ea928b) across all changed files: the IndexedDB store and its tests (next/src/lib/history/db.ts, db.test.ts), the isCurrentRun helper and tests (is-current.ts, is-current.test.ts), the HistoryPane UI (history-pane.tsx), the toolbar toggle, the store.ts wiring (commitBaseFor snapshot/archive, deleteTaskRuns on task delete, persisted historyPaneOpen), page.tsx, and the i18n keys.
The one open thread from the prior pass — isCurrent derived purely from HTML-content equality flagging multiple rows as "current" after a restore — is correctly resolved here. isCurrentRun now anchors on run.id === runs[0]?.id && run.html === activeHtml, and extracting it into a pure helper made the restore regression unit-coverable: the four cases in is-current.test.ts lock in that an older byte-identical run stays restorable, the badge clears on editor drift, and the empty-runs degenerate case is guarded.
The rest holds up well: per-task version numbering is computed atomically inside a single readwrite transaction, the GC cap prunes per task, task histories stay isolated, and every store entry point degrades to a soft no-op when IndexedDB is unavailable so the convert pipeline is never blocked. The history iframes are sandboxed without allow-scripts. I verified locally on this head — pnpm -F @html-anything/next typecheck passes and the src/lib/history suite is green (9/9) — alongside the green CI and the documented browser QA.
Nice, careful work — the soft-failure discipline and the pure-helper extraction for testability are both well done.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
4 participants
Why this branch
This branch is ready to pick because the risky parts were exercised in-browser against the real app shell and are now covered by repeatable unit tests: IndexedDB writes, per-task GC, task deletion cleanup, restore commits, visual iframe compare, and source diff rendering. It also fixes the React update loop found during smoke QA after Convert.
What changed
html-anything-history/runswith a 20-version cap.historyPaneOpenacross refreshes.DeployControl's empty deployments selector so Convert does not trigger a React maximum-update-depth crash.Screenshots
v1 -> v2 visual compare
Empty state, English
Empty state, zh-CN
Large HTML visual compare, 250KB+ input
Automated coverage
pnpm testnow coverssrc/lib/history/db.tswithfake-indexeddb:listRunsreturns newest first.deleteTaskRunsremoves only the selected task's rows.deleteRunremoves a historical version without disturbing newer rows.putRun, reads, deletes, andclearAlldegrade without throwing.Browser QA evidence
runshad 1 row, versions[1],currentVersion=1, pane showedv1 currentand刚刚.runshad 2 rows, versions[2, 1],currentVersion=2, v1 no longer had the current marker.[3, 2, 1],currentVersion=3, preview returned to v1 content.v5throughv24; earlier rows were trimmed.remainingRowsForDeleted=0in IndexedDB.historyPaneOpen=trueand the active task's history remained visible.status=done, generated HTML, and no red error logs when IDB was unavailable.enandzh-CN; captured above.+and-rows and the scroll container remained responsive.Verification
pnpm test— 5 tests passed.pnpm build— passed.pnpm dev, with Application -> IndexedDB inspection forhtml-anything-history.Note: build still emits the existing Turbopack NFT tracing warning for
next.config.ts -> src/app/api/draft/route.ts; no new build failure was introduced.