Add change summaries and grouping to the revision browser#999
Add change summaries and grouping to the revision browser#999ericnewcomer wants to merge 8 commits intomainfrom
Conversation
|
Claude finished @ericnewcomer's task in 3m 49s —— View job Review
Overall the feature is well-scoped and well-tested — Likely bugs
Concerns
Diff hygiene
Test coverage gaps worth adding
Inline comments posted on the relevant lines. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const METADATA_LABELS: Record<string, { noun: string; plural: string }> = { | ||
| name: { noun: 'name', plural: 'name' }, | ||
| type: { noun: 'flow type', plural: 'flow type' }, | ||
| expire_after_minutes: { noun: 'expiration', plural: 'expiration' } | ||
| }; |
There was a problem hiding this comment.
Plurals reuse the singular form here, so when two metadata_changed events share the same field, they group into one phrase with count = 2 and the renderer emits ${verb} ${count} ${plural} — e.g. "Changed 2 name". The grouping key is ${order}:${noun}:${verb}, and field only feeds into noun, so this is reachable any time the backend emits two metadata_changed events for the same field in one revision (or in a 15-min collapsed group). Either give these real plurals (names, flow types, expirations) or set noCount: true on the phrase so the count is suppressed.
| const newRevisionWhileOpen = | ||
| !this.hidden && | ||
| changes.has('liveRevision') && | ||
| changes.get('liveRevision') !== undefined && | ||
| !this.storeViewingRevision; | ||
|
|
There was a problem hiding this comment.
changes.get('liveRevision') !== undefined is intended to skip the first reactive notification, but liveRevision is seeded in connectedCallback (this[name] = selector(...)) before the first render, so by the time updated() runs the previous value is always defined and this check is a no-op. The real guard you want is "the value actually changed and we're not previewing" — changes.has('liveRevision') && !this.storeViewingRevision is enough. Also worth gating on (changes.get('liveRevision') as number) < this.liveRevision so a revert that lowers the revision number doesn't trigger a refetch.
| const flush = () => { | ||
| if (group.length === 0) return; | ||
| const head = group[0]; | ||
| const allChanges: RevisionChange[] = []; | ||
| for (const r of group) { | ||
| if (r.changes) allChanges.push(...r.changes); | ||
| } | ||
| result.push({ ...head, changes: allChanges }); | ||
| group = []; | ||
| }; |
There was a problem hiding this comment.
When a non-significant revision arrives with changes: null (or undefined), it's silently merged into a 15-min group and the resulting summary is computed only from whatever non-null neighbors contributed. The user gets one collapsed row with no visual indicator that an opaque revision happened in that window. Worth deciding whether such revisions should always stand alone (so the user can click in to inspect them) or be tagged in the summary.
| } | ||
| }); | ||
|
|
||
| editor = await fixture( | ||
| html`<temba-flow-editor></temba-flow-editor>` | ||
| ); | ||
| editor = await fixture(html`<temba-flow-editor></temba-flow-editor>`); |
There was a problem hiding this comment.
This file has ~16 of these whitespace-only reformats and no functional changes — they look like Prettier churn unrelated to the revisions work. Consider reverting to keep the diff focused on the feature.
| `/flow/revisions/${this.flow}/?version=${FLOW_SPEC_VERSION}` | ||
| ); | ||
| this.revisions = results.slice(1); | ||
| this.revisions = this.collapseRevisions(results); |
There was a problem hiding this comment.
fetchRevisions is now invoked from two paths (open + auto-refresh on liveRevision change) with no in-flight protection. If a save fires while a previous fetch hasn't returned, results can resolve out of order and the older payload will overwrite the newer one. Consider an AbortController or a request-id guard so the late response is dropped.
📊 Coverage ReportSummary
|
|
Claude finished @ericnewcomer's task in 2m 32s —— View job Re-review after
|
| Prior comment | Fix | Test |
|---|---|---|
metadata_changed plural reuse → "Changed 2 name" (revision-summary.ts:93-97) |
METADATA_LABELS simplified to { noun }; metadata_changed and base_language_changed phrases now carry noCount: true, and the renderer in summarizeChanges (revision-summary.ts:235-239) drops the count when noCount is set |
test/revision-summary.test.ts:127-134 — "Changed name" for two repeats |
liveRevision previous-value guard was a no-op (RevisionsWindow.ts:82-87) |
Replaced with previousRevision !== undefined && this.liveRevision > previousRevision (RevisionsWindow.ts:83-89) — correctly skips initial seeding and reverts that lower the number |
test/temba-flow-editor-revisions.test.ts:306-327 |
No in-flight protection in fetchRevisions (RevisionsWindow.ts:205) |
fetchRequestId request-id guard at RevisionsWindow.ts:202-219; stale resolutions/errors short-circuit and only the latest call resets isLoading |
(covered indirectly; no dedicated test, see below) |
Opaque revisions silently merged into 15-min groups (RevisionsWindow.ts:217-226) |
RevisionsWindow.ts:236-244 — !rev.changes || rev.changes.length === 0 now also forces a flush + standalone push, alongside the existing significant-change branch |
test/temba-flow-editor-revisions.test.ts:198-228 |
Whitespace churn in test/temba-localization.test.ts |
Fully reverted — git diff origin/main...HEAD -- test/temba-localization.test.ts is now empty |
n/a |
15-min strict-< boundary (suggested) |
n/a — behavior was already correct | test/temba-flow-editor-revisions.test.ts:166-193 |
Minor leftovers (non-blocking)
- No direct unit test for the request-id guard. The race is straightforward enough that I'd skip it, but if you want to harden it, two pending stubs resolved out of order would do the job.
previousRevision !== undefinedis technically redundant —7 > undefinedevaluates tofalsevia NaN — but the explicit check reads more clearly, so leaving it is fine.- Errors from
fetchRevisionsremain silent (justconsole.error, no UI signal). Pre-existing behavior, not a regression.
· Branch
|
Claude finished @ericnewcomer's task in 1m 52s —— View job Re-review after
|
Show the current revision at the top of the list (labeled, no Revert action). Render a human-friendly summary for each revision built from the structured `changes` records on the wire, joined verb-first and collapsing into a "Significantly changed ..." form when many things moved at once. Collapse revisions made within a 15-minute window onto their most recent member, merging their changes into a combined summary, and let significant revisions break out as standalone rows. Refresh the list automatically when a new revision is saved while the window is open. Widen the window to 340 and lift its max height to 500.
- Suppress count for metadata and base_language phrases so two events for the same field don't render as "Changed 2 name". - Replace the no-op `previousRevision !== undefined` guard with one that requires the live revision to actually increase, so reverts (which lower the revision number transiently) don't refetch. - Add a request-id guard around fetchRevisions so concurrent calls can't resolve out of order and clobber fresher results. - Always show revisions with no recorded changes as standalone rows so the user can click in to inspect them. - Revert unrelated whitespace reformats in temba-localization.test.ts. - Add regression tests covering each fix.
Reusing a single Response across multiple fetch invocations triggers 'body stream already read' on the second consume — which surfaces as an unhandled promise rejection outside the test and fails the CI runner even though the assertions themselves pass.
ericnewcomer
force-pushed
the
ericnewcomer/revision-browser-current
branch
from
884de35 to
6f4469e
Compare
|
Claude finished @ericnewcomer's task in 1m 19s —— View job Re-review after
|
| Item | Status | Where to confirm |
|---|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed | src/flow/revision-summary.ts:93-97, renderer at :235-239; test test/revision-summary.test.ts:127-134 |
liveRevision previous-value guard was a no-op |
Fixed — also gated to "value increased" | src/flow/RevisionsWindow.ts:83-89; tests test/temba-flow-editor-revisions.test.ts:284-327 |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId |
src/flow/RevisionsWindow.ts:202-219 |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs now stand alone | src/flow/RevisionsWindow.ts:236-244; test test/temba-flow-editor-revisions.test.ts:196-229 |
Whitespace churn in test/temba-localization.test.ts |
Reverted | git diff origin/main...HEAD -- test/temba-localization.test.ts is empty |
15-min strict-< boundary |
Test added | test/temba-flow-editor-revisions.test.ts:166-194 |
Nothing new to flag. No fresh code changes since my last re-review, so the same conclusion holds: this looks ready to merge from my side. Non-blocking leftovers (no dedicated test for the request-id race, silent console.error on fetch failure) are unchanged and pre-existing.
· Branch ·
Auto-refresh on revision changes can fire fetchRevisions during fixture setup or unrelated store mutations, and an unstubbed fetch returns undefined which throws in the .then chain. A safe default keeps unrelated triggers quiet; tests that need a specific payload still override via callsFake/resolves.
|
Claude finished @ericnewcomer's task in 2m 27s —— View job Re-review after
|
| Item | Status |
|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed (src/flow/revision-summary.ts:93-97, :235-239) |
liveRevision previous-value guard was a no-op |
Fixed and gated on increase (src/flow/RevisionsWindow.ts:83-89) |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId (src/flow/RevisionsWindow.ts:202-219) |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs stand alone (src/flow/RevisionsWindow.ts:236-244) |
Whitespace churn in test/temba-localization.test.ts |
Reverted |
15-min strict-< boundary test |
Added (test/temba-flow-editor-revisions.test.ts:173-201) |
Looks ready to merge from my side. Pre-existing non-blocking leftovers (no dedicated unit test for the request-id race, silent console.error on fetch failure) are unchanged.
· Branch ·
Replace the @fromstore decorator on RevisionsWindow with a manual zustand.subscribe that activates only while the window is open and disconnects on close (or on disconnectedCallback). This avoids keeping a live store subscriber on every editor instance regardless of whether the user ever views revisions, and trims cross-test reactive cascades during fixture churn.
|
Claude finished @ericnewcomer's task in 3m 10s —— View job Re-review after
|
| Item | Status |
|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed (src/flow/revision-summary.ts:93,108-117,236) |
liveRevision previous-value guard was a no-op |
Now superseded — manual subscription with current > previous (RevisionsWindow.ts:91-104) |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId (RevisionsWindow.ts:219-236) |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs stand alone (RevisionsWindow.ts:253-261) |
Whitespace churn in test/temba-localization.test.ts |
Reverted (git diff origin/main...HEAD -- test/temba-localization.test.ts is empty) |
15-min strict-< boundary test |
Added (test/temba-flow-editor-revisions.test.ts) |
Looks ready to merge from my side. Pre-existing non-blocking leftovers: no direct unit test for the request-id race, silent console.error on fetch failure, and now the missing close→unwatch test noted inline.
· Branch ·
| if ( | ||
| changes.has('hidden') && | ||
| !this.hidden && | ||
| changes.get('hidden') === true | ||
| ) { | ||
| this.fetchRevisions(); | ||
| this.watchRevisions(); | ||
| } else if (changes.has('hidden') && this.hidden) { | ||
| this.unwatchRevisions(); | ||
| } |
There was a problem hiding this comment.
The headline behavior of this commit — "subscription is torn down on close, so no refetch fires after the user closes the window" — isn't directly tested. The existing auto-refresh tests only cover the open path. A test that toggles hidden back to true and then mutates flowDefinition.revision, asserting fetchStub.callCount stays at zero, would lock in the close→unwatch contract. Non-blocking.
The revisions tests mutate the singleton zustand store (setting viewingRevision and flowDefinition.revision) to exercise auto-refresh paths. Without an explicit reset, leftover state can affect subsequent test files that share the same store, producing intermittent failures in unrelated suites.
|
Claude finished @ericnewcomer's task in 1m 56s —— View job Re-review after
|
| Item | Status |
|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed (src/flow/revision-summary.ts:93,108-117,236) |
liveRevision previous-value guard was a no-op |
Superseded — manual zustand.subscribe with current > previous (RevisionsWindow.ts:91-104) |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId (RevisionsWindow.ts:202-219) |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs stand alone (RevisionsWindow.ts:236-244) |
Whitespace churn in test/temba-localization.test.ts |
Reverted |
15-min strict-< boundary test |
Added |
Looks ready to merge from my side. Pre-existing non-blocking leftovers (no direct unit test for the request-id race, silent console.error on fetch failure, missing close→unwatch test) are unchanged.
· Branch ·
Replace the zustand subscription with an imperative refresh() method on RevisionsWindow that the Editor's saveChanges callback invokes after a successful save. This removes the cross-test reactive cascade risk from a singleton-store subscription and keeps the auto-refresh behavior exactly where it's needed.
|
Claude finished @ericnewcomer's task in 1m 45s —— View job Re-review after
|
| Item | Status |
|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed (src/flow/revision-summary.ts:93,108-117,236) |
liveRevision previous-value guard was a no-op |
Superseded — replaced by imperative call from saveChanges (Editor.ts:1601) |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId (RevisionsWindow.ts:193-209) |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs stand alone (RevisionsWindow.ts:230-235) |
Whitespace churn in test/temba-localization.test.ts |
Reverted |
15-min strict-< boundary test |
Added (test/temba-flow-editor-revisions.test.ts:172-200) |
Looks ready to merge from my side. Pre-existing non-blocking leftovers: silent console.error on fetch failure, and the missing saveChanges → refresh integration test noted above.
· Branch ·
| getStore().getState().setDirtyDate(null); | ||
|
|
||
| // Refresh the revisions list if it's currently open. | ||
| this.getRevisionsWindow()?.refresh(); |
There was a problem hiding this comment.
No test exercises this exact wiring (Editor.saveChanges() → getRevisionsWindow()?.refresh()). The unit tests on refresh() in test/temba-flow-editor-revisions.test.ts:290-317 confirm the method behaves correctly when called, but a future refactor that drops this call site would still pass. An integration-level test that stubs getStore().postJSON to resolve a successful response, opens the revisions window, triggers saveChanges(), and asserts fetchStub.callCount increased after the save would lock in the contract. Non-blocking.
test/utils.test.ts is a shared helper file — it exports test utilities and registers a global mocha before()/after() pair via module-level side effects, but has no describe()/it() blocks of its own. The *.test.ts naming caused wtr to load it as its own zero-test session that never cleanly signaled completion. With one more test file in the suite (revision-summary.test.ts), this pushed wtr past its tolerance and the coverage run silently exited without finishing the last batch of files. Excluding the file from the glob makes the run deterministic; the exported helpers and registered hooks still load via the imports already in every test file that depends on them. This was the consistent CI red on PR #999 — the run reaches 108-120 of 122 files and ELIFECYCLEs without a specific test failure. Locally reproduced and bisected: removing any single file makes the suite pass; removing utils.test.ts specifically is the correct fix because it isn't actually a test file.
|
Claude finished @ericnewcomer's task in 1m 44s —— View job Re-review after
|
| Item | Status |
|---|---|
metadata_changed plural reuse → "Changed 2 name" |
Fixed (src/flow/revision-summary.ts:93,108-117,236) |
liveRevision previous-value guard was a no-op |
Superseded — replaced by imperative call from saveChanges (Editor.ts:1601) |
No in-flight protection in fetchRevisions |
Fixed via fetchRequestId (RevisionsWindow.ts:193-209) |
| Opaque revisions silently merged into 15-min groups | Fixed — opaque revs stand alone (RevisionsWindow.ts:230-235) |
Whitespace churn in test/temba-localization.test.ts |
Reverted |
15-min strict-< boundary test |
Added (test/temba-flow-editor-revisions.test.ts:172-200) |
Looks ready to merge from my side. Pre-existing non-blocking leftovers: silent console.error on fetch failure, missing saveChanges → refresh integration test.
· Branch · ·
1 participant
Summary
changesrecords (verb-first, e.g. "Updated 2 messages and added group"). When >2 distinct change types appear, collapse to a category-level form ("Significantly changed structure and translations").changesinto the combined summary; revisions that are significant on their own break out into a standalone row.Test plan
pnpm test:fast— newrevision-summaryand existingtemba-flow-editor-revisionssuites pass