Explain Following changes before acknowledging review #1323

Merged
rockachopa merged 1 commits from timmy/1322-explain-following-changes into main 2026-08-23 21:39:32 +00:00
8 changed files with 174 additions and 18 deletions

View File

@ -118,7 +118,10 @@ Following is a read-first, account-scoped collection: it is encrypted at rest, r
pull-request Search Preview; confirmed **Stop watching** removes an open item. When watched work
closes or merges, the sequential review exposes **Stop watching & next** so the completed item can be
retired without leaving the preview; the next captured change opens immediately, and retiring the
final item completes the Following phase. For open work that still needs thought, **Keep for later & next**
final item completes the Following phase. Changed cards explain whether work closed, reopened, changed title, or
received other activity. During review, conversation messages newer than the prior reviewed revision are highlighted,
and the revision is acknowledged only after both detail and conversation context load; a failed conversation load keeps
that exact change unseen for Retry or **Keep for later & next**. For open work that still needs thought, **Keep for later & next**
restores only the loaded revision to the unseen queue and continues the captured pass without changing
Gitea state, ownership, planning, or watch status. Failed or unconfirmed Gitea mutations leave the collection
and current review position unchanged. Following counts never influence the recommended Work queue. Set

View File

@ -1040,6 +1040,7 @@ textarea { resize: vertical; min-height: 120px; }
.search-preview-conversation { display:grid; gap:8px; padding-top:8px; border-top:1px solid #2a496e; }
.search-preview-conversation h2 { margin:0; font-size:1rem; }
.search-preview-comment { min-width:0; padding:10px 0; border-bottom:1px solid #1b2d45; overflow-wrap:anywhere; }
.search-preview-comment.new-since-review { padding-left:10px; border-left:3px solid var(--accent); background:#10233a; }
.search-preview-conversation button { min-height:44px; width:100%; }
.search-preview-reply { display:grid; gap:8px; padding-top:8px; border-top:1px solid #2a496e; }
.search-preview-reply[hidden] { display:none; }

View File

@ -184,7 +184,7 @@
list.innerHTML = state.items.map((item, index) =>
'<button class="following-card' + (item.has_unseen_change ? ' has-unseen-change' : '') +
'" type="button" data-following-index="' + index + '"><span>' +
(item.has_unseen_change ? '<em>New activity</em>' : '') + '<strong>' +
(item.has_unseen_change ? '<em>' + escapeHtml(item.change_summary || 'New activity') + '</em>' : '') + '<strong>' +
escapeHtml(item.title) + '</strong><small>' + escapeHtml(
(item.kind === 'pull' ? 'Pull request' : 'Issue') + ' · ' + item.repository + ' #' + item.number +
' · ' + item.state + ' · ' + formatTime(item.updated_at)) +

View File

@ -119,10 +119,15 @@
retry.hidden = older.hidden = true;
return;
}
const isNew = comment => Boolean(conversation.reviewedAt && comment.created_at &&
new Date(comment.created_at).getTime() > new Date(conversation.reviewedAt).getTime());
const newCount = (conversation.comments || []).filter(isNew).length;
comments.innerHTML = (conversation.comments || []).map(comment =>
'<article class="search-preview-comment" data-comment-id="' + Number(comment.id || 0) + '">' +
'<article class="search-preview-comment' + (isNew(comment) ? ' new-since-review' : '') +
'" data-comment-id="' + Number(comment.id || 0) + '">' +
'<div class="small">' + escapeHtml(comment.author || 'Unknown author') +
(comment.created_at ? ' · ' + escapeHtml(formatTime(comment.created_at)) : '') + '</div>' +
(comment.created_at ? ' · ' + escapeHtml(formatTime(comment.created_at)) : '') +
(isNew(comment) ? ' · <strong>New since last review</strong>' : '') + '</div>' +
'<div class="markdown-content">' + renderMarkdown(comment.body || '') + '</div></article>'
).join('');
retry.hidden = conversation.status !== 'error';
@ -134,7 +139,8 @@
'Conversation unavailable. Preview and planning actions still work.';
else if (!conversation.comments?.length) status.textContent = 'No conversation yet.';
else status.textContent = conversation.comments.length +
(conversation.comments.length === 1 ? ' message.' : ' messages.');
(conversation.comments.length === 1 ? ' message' : ' messages') +
(newCount ? ' · ' + newCount + ' new since last review' : '') + '.';
};
root.renderSearchPreviewReply = (state, detail, preview, document) => {
const section = document.querySelector('.search-preview-reply');
@ -176,12 +182,21 @@
let replyRequest = null;
let watchRequest = null;
let conversation = null;
let openedRevision = null;
function sameItem(left, right) {
return left && right && left.kind === right.kind && left.repository === right.repository &&
Number(left.number) === Number(right.number);
}
async function notifyOpened(item, requestGeneration) {
const revision = [item?.kind, item?.repository, item?.number, item?.updated_at].join(':');
if (requestGeneration !== generation || openedRevision === revision) return false;
await onOpened?.({...item});
if (requestGeneration === generation) openedRevision = revision;
return true;
}
function replyKey(item, suffix) {
return 'stackchain.search-reply.' + [item?.kind, item?.repository, item?.number]
.map(value => encodeURIComponent(String(value || ''))).join('.') + '.' + suffix;
@ -246,7 +261,10 @@
if (typeof fetchConversation !== 'function') return Promise.resolve(null);
const previousComments = page && Array.isArray(conversation?.comments)
? conversation.comments : [];
conversation = { status:'loading', comments:previousComments, olderPage:page ?? null };
conversation = {
status:'loading', comments:previousComments, olderPage:page ?? null,
reviewedAt:detail?.following === true ? detail.reviewed_at : null,
};
publish({ status:'ready', item:current, detail, conversation });
return fetchConversation(detail, page).then(result => {
if (requestGeneration !== generation) return result;
@ -258,6 +276,7 @@
status:'ready',
comments,
olderPage:result?.older_page ?? null,
reviewedAt:detail?.following === true ? detail.reviewed_at : null,
};
publish({ status:'ready', item:current, detail:current, conversation });
return result;
@ -266,6 +285,7 @@
conversation = {
status:'error', comments:previousComments,
olderPage:page ?? conversation?.olderPage ?? null, error,
reviewedAt:detail?.following === true ? detail.reviewed_at : null,
};
publish({ status:'ready', item:current, detail:current, conversation });
}
@ -283,16 +303,23 @@
const requestGeneration = generation;
current = { ...item };
conversation = null;
openedRevision = null;
publish({ status: 'loading', item: current });
return fetchJson(current).then(async detail => {
if (requestGeneration === generation) {
current = { ...current, ...detail };
if (typeof fetchConversation === 'function') {
loadConversation(current, requestGeneration);
const context = loadConversation(current, requestGeneration);
if (current.following === true) {
await context;
if (conversation?.status === 'ready') await notifyOpened(current, requestGeneration);
} else {
await notifyOpened(current, requestGeneration);
}
} else {
publish({ status: 'ready', item: current, detail });
await notifyOpened(current, requestGeneration);
}
await onOpened?.({...current});
}
return detail;
}).catch(error => {
@ -340,9 +367,14 @@
})().finally(() => { moveRequest = null; });
return moveRequest;
},
retryConversation() {
if (!current) return Promise.resolve(null);
return loadConversation(current, generation);
async retryConversation() {
if (!current) return null;
const requestGeneration = generation;
const result = await loadConversation(current, requestGeneration);
if (current?.following === true && conversation?.status === 'ready') {
await notifyOpened(current, requestGeneration);
}
return result;
},
loadOlderConversation() {
if (!current || !conversation?.olderPage) return Promise.resolve(null);

View File

@ -112,6 +112,16 @@ class FollowingStore:
if not isinstance(kept_updated_at, str) or not kept_updated_at or len(kept_updated_at) > 64:
raise ValueError("kept update is invalid")
item["kept_updated_at"] = kept_updated_at
reviewed_title = raw.get("reviewed_title")
if reviewed_title is not None:
if not isinstance(reviewed_title, str) or not reviewed_title.strip() or len(reviewed_title.strip()) > 300:
raise ValueError("reviewed title is invalid")
item["reviewed_title"] = reviewed_title.strip()
reviewed_state = raw.get("reviewed_state")
if reviewed_state is not None:
if reviewed_state not in _STATES:
raise ValueError("reviewed state is invalid")
item["reviewed_state"] = reviewed_state
return item
@classmethod
@ -123,8 +133,19 @@ class FollowingStore:
unseen = (stored["updated_at"] != stored["last_seen_updated_at"] or
stored.get("kept_updated_at") == stored["updated_at"])
item = {key: value for key, value in stored.items()
if key not in {"last_seen_updated_at", "kept_updated_at"}}
if key not in {"last_seen_updated_at", "kept_updated_at",
"reviewed_title", "reviewed_state"}}
item["has_unseen_change"] = unseen
if unseen:
item["reviewed_at"] = stored["last_seen_updated_at"]
if stored.get("reviewed_state") == "open" and stored["state"] == "closed":
item["change_summary"] = "Closed since last review"
elif stored.get("reviewed_state") == "closed" and stored["state"] == "open":
item["change_summary"] = "Reopened since last review"
elif stored.get("reviewed_title") not in {None, stored["title"]}:
item["change_summary"] = "Title changed since last review"
else:
item["change_summary"] = "New activity"
(changed if unseen else unchanged).append(item)
changed.sort(key=lambda item: item["updated_at"], reverse=True)
return {"revision": snapshot["revision"], "items": changed + unchanged}
@ -194,8 +215,9 @@ class FollowingStore:
items.insert(0, item)
else:
item["last_seen_updated_at"] = items[index]["last_seen_updated_at"]
if "kept_updated_at" in items[index]:
item["kept_updated_at"] = items[index]["kept_updated_at"]
for key in ("kept_updated_at", "reviewed_title", "reviewed_state"):
if key in items[index]:
item[key] = items[index][key]
if items[index] == item:
return self._present({"revision": current["revision"], "items": items})
items.pop(index)
@ -229,8 +251,12 @@ class FollowingStore:
if update is None:
continue
update["last_seen_updated_at"] = item["last_seen_updated_at"]
if "kept_updated_at" in item:
update["kept_updated_at"] = item["kept_updated_at"]
for key in ("kept_updated_at", "reviewed_title", "reviewed_state"):
if key in item:
update[key] = item[key]
if update["updated_at"] != item["updated_at"]:
update.setdefault("reviewed_title", item["title"])
update.setdefault("reviewed_state", item["state"])
if update != item:
items[index] = update
changed = True
@ -272,6 +298,8 @@ class FollowingStore:
item.get("kept_updated_at") == updated_at):
item["last_seen_updated_at"] = updated_at
item.pop("kept_updated_at", None)
item.pop("reviewed_title", None)
item.pop("reviewed_state", None)
revision += 1
connection.execute(
"UPDATE following_issues SET revision = ?, items = ? WHERE login = ?",

View File

@ -41,7 +41,8 @@ def test_following_queue_is_phone_usable_at_narrow_viewport(viewport):
json:async () => ({revision:1,items:[{
repository:'stackchain/api', kind:'pull', number:42,
title:'Changed pull request with a long mobile title', state:'open',
updated_at:'2026-08-23T05:00:00Z', has_unseen_change:true
updated_at:'2026-08-23T05:00:00Z', has_unseen_change:true,
change_summary:'Closed since last review'
}]})
});
globalThis.followingReleaseQueue = attachFollowing(() => {});
@ -50,7 +51,7 @@ def test_following_queue_is_phone_usable_at_narrow_viewport(viewport):
expect(page.locator("#following-sheet")).to_be_visible()
expect(page.locator(".following-card")).to_be_visible()
expect(page.locator(".following-card")).to_contain_text("New activity")
expect(page.locator(".following-card")).to_contain_text("Closed since last review")
expect(page.locator(".following-card")).to_contain_text("Pull request")
expect(page.locator("#review-following")).to_have_text("Review new activity")
assert page.locator("#review-following").bounding_box()["height"] >= 44

View File

@ -113,6 +113,69 @@ def test_following_preview_wires_keep_for_later_action_and_retryable_failure():
assert "keepForLater:feature.keepForLater" in following
def test_following_conversation_marks_messages_newer_than_last_review():
preview_module = ROOT / "frontend" / "search-preview.js"
script = f"""
require({json.dumps(str(preview_module))});
const nodes={{
'#search-preview-comments':{{innerHTML:'',textContent:''}},
'#search-preview-conversation-status':{{textContent:''}},
'#retry-search-preview-conversation':{{hidden:true}},
'#load-older-search-preview-comments':{{hidden:true,disabled:false}},
}};
const document={{querySelector:selector=>nodes[selector]}};
global.renderSearchPreviewConversation({{
status:'ready', reviewedAt:'2026-08-23T03:00:00Z', comments:[
{{id:1,author:'Timmy',created_at:'2026-08-23T02:00:00Z',body:'Earlier'}},
{{id:2,author:'Alex',created_at:'2026-08-23T04:00:00Z',body:'New decision'}},
]
}},document,String,String,String);
process.stdout.write(JSON.stringify({{html:nodes['#search-preview-comments'].innerHTML,status:nodes['#search-preview-conversation-status'].textContent}}));
"""
result = json.loads(subprocess.run(
["node", "-e", script], text=True, capture_output=True, check=True
).stdout)
assert result["html"].count("new-since-review") == 1
assert "New since last review" in result["html"]
assert result["status"] == "2 messages · 1 new since last review."
def test_following_revision_is_acknowledged_only_after_conversation_context_loads():
preview_module = ROOT / "frontend" / "search-preview.js"
script = f"""
const createSearchPreview=require({json.dumps(str(preview_module))});
let unavailable=true;
const opened=[];
const preview=createSearchPreview({{
fetchJson:async item=>({{...item,title:'Changed issue'}}),
fetchConversation:async()=>{{if(unavailable)throw new Error('offline');return {{comments:[]}};}},
onOpened:async item=>opened.push(item.updated_at),
onState:()=>{{}},
}});
(async()=>{{
await preview.open({{
repository:'stackchain/api',kind:'issue',number:42,following:true,
reviewed_at:'2026-08-23T03:00:00Z',updated_at:'2026-08-23T04:00:00Z'
}});
const afterFailure=[...opened];
unavailable=false;
await preview.retryConversation();
process.stdout.write(JSON.stringify({{afterFailure,opened}}));
}})().catch(error=>{{console.error(error);process.exit(1);}});
"""
result = json.loads(subprocess.run(
["node", "-e", script], text=True, capture_output=True, check=True
).stdout)
assert result == {
"afterFailure": [],
"opened": ["2026-08-23T04:00:00Z"],
}
def test_following_typed_identity_prevents_issue_pull_collisions():
script = f"""
const createFollowing = require({json.dumps(str(MODULE))});

View File

@ -51,6 +51,34 @@ def test_refresh_marks_changed_items_unseen_and_sorts_them_first(tmp_path):
assert snapshot["items"][0]["state"] == "closed"
def test_refresh_explains_change_from_last_reviewed_snapshot(tmp_path):
store = FollowingStore(tmp_path / "following.sqlite3", encryption_key=KEY)
store.set_watching("timmy", ITEM, True)
closed = store.refresh("timmy", [{
**ITEM,
"title": "Make mobile review trustworthy",
"state": "closed",
"updated_at": "2026-08-23T04:00:00Z",
}])
assert closed["items"][0]["reviewed_at"] == ITEM["updated_at"]
assert closed["items"][0]["change_summary"] == "Closed since last review"
store.acknowledge(
"timmy", ITEM["repository"], ITEM["number"], "2026-08-23T04:00:00Z"
)
title_change = store.refresh("timmy", [{
**ITEM,
"title": "Make mobile review understandable",
"state": "closed",
"updated_at": "2026-08-23T05:00:00Z",
}])
assert title_change["items"][0]["reviewed_at"] == "2026-08-23T04:00:00Z"
assert title_change["items"][0]["change_summary"] == "Title changed since last review"
def test_acknowledgement_is_revision_conditional_and_later_change_is_unseen(tmp_path):
store = FollowingStore(tmp_path / "following.sqlite3", encryption_key=KEY)
store.set_watching("timmy", ITEM, True)