Activity redesign 1/8: resolve change subjects server-side and generate the change-type list#2435
Activity redesign 1/8: resolve change subjects server-side and generate the change-type list#2435myieye wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesProject activity now returns materialized pages enriched with resolved subjects and targets. Backend context handling, generated TypeScript contracts, invokable callers, viewer data construction, and extensive naming tests were updated accordingly. Project Activity Enrichment
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Note for the Argos review: this part is backend-only and intends zero visual change — the deterministic Generated by Claude Code |
myieye
changed the title
myieye
changed the title
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
looks good, I glossed over the change info resolver.
| string[]? changeTypeKeys = null, | ||
| ActivitySort sort = ActivitySort.NewestFirst) | ||
| { | ||
| return await historyService.ProjectActivity(skip, take, |
There was a problem hiding this comment.
I want to keep this awaiting as it preserves the stack trace, we should only do this where we don't await a task as an optimization on a hot path
There was a problem hiding this comment.
| var array = string.Concat(typeNames.Select(name => $"\n '{name}',")); | ||
|
|
||
| Context.Location.CurrentNamespace.CompilationUnits.Add(new RtRaw( | ||
| $"export type ChangeType ={union};\n\nexport const knownChangeTypes = [{array}\n] as const satisfies readonly ChangeType[];\n")); |
There was a problem hiding this comment.
are these new lines going to add noise when git rewrites them on windows checkout?
There was a problem hiding this comment.
No — the root .gitattributes normalizes every text file to LF on both commit and checkout (* text=auto eol=lf), Windows included. So these \n literals match the rest of this generated file and every other checked-in file; a Windows checkout gets LF too, so there's no CRLF churn and the stale-types check stays stable.
Generated by Claude Code
| } | ||
|
|
||
| [Fact] | ||
| public async Task ProjectActivity_ChangeInfo_AddsHomographSubscriptToSubject_OnlyWhenAssigned() |
There was a problem hiding this comment.
let's split these subject specific tests out into their own test class as they're much lower importance.
There was a problem hiding this comment.
Done in cba8373 — moved the ChangeInfo subject/target-naming tests into a new HistoryServiceActivitySubjectTests, with the fixture + helpers factored into a shared HistoryServiceActivityTestsBase. The core paging/filter/sort tests stay in HistoryServiceActivityTests.
Generated by Claude Code
| .ToListAsyncLinqToDB(); | ||
| var index = affectingCommitIds.IndexOf(commitId); | ||
| if (index < 0 || index + 1 >= affectingCommitIds.Count) return null; | ||
| return await dataModel.GetAtCommit<object>(affectingCommitIds[index + 1], entityId) as IObjectWithId; |
There was a problem hiding this comment.
this seems overly complicated, can't we just dataModel.GetAtTime(commitTime - one tick, entityId)? I guess that would throw if there's no commits before. We should probably add an API in harmony for this.
There was a problem hiding this comment.
I've left this as-is for now. GetAtTime(commitTime - 1 tick) goes through linq2db, which truncates every SQLite timestamp comparison to millisecond precision (strftime('…%f')) — see the landmine note in backend/FwLite/AGENTS.md and SnapshotAtCommitService. A sub-millisecond - 1 tick can't reliably exclude the current commit when two commits land in the same millisecond, so this walks commits by their deterministic order rather than by timestamp. I agree a dedicated Harmony API (previous-snapshot-before-a-commit, ordered by commit order not time) is the clean fix — happy to file that upstream and switch to it as a follow-up.
Generated by Claude Code
| } | ||
|
|
||
| private static string Subscript(int number) => | ||
| new(number.ToString().Select(digit => (char)('₀' + (digit - '0'))).ToArray()); |
There was a problem hiding this comment.
use this instead for less allocations and handling negative (not likely):
private static string Subscript(int number)
{
// A standard int32 will never exceed 11 characters (e.g., "-2147483648")
Span<char> buffer = stackalloc char[11];
number.TryFormat(buffer, out int charsWritten);
for (int i = 0; i < charsWritten; i++)
{
// Handle negative signs cleanly with the subscript minus character (U+208B)
buffer[i] = buffer[i] == '-' ? '₋' : (char)(buffer[i] - '0' + '₀');
}
return buffer[..charsWritten].ToString();
}There was a problem hiding this comment.
Done in cba8373 — switched to the stackalloc + TryFormat version, keeping the subscript-minus handling for negatives.
Generated by Claude Code
…type list Adds ActivityChangeInfoResolver: one batched pass per activity page that labels each change with the entity it's about (entry headword with homograph subscript, 'headword › gloss' for senses, vocab object names), the referenced item it names only by id (part of speech set, semantic domain removed, component linked), and the root entry so the frontend can group a commit's changes. Deleted objects are recovered from their latest snapshot so a delete can still name its subject. ProjectActivity returns a materialized page (Task<ProjectActivity[]>) of enriched record copies since the resolver batch-loads across it. ChangeContext gains PreviousSnapshot (the entity's state just before the commit) so the frontend can render true before/after diffs (#2170). A Reinforced.Typings generator emits a ChangeType union + knownChangeTypes array from the registered CRDT change types via the same discriminator helper the runtime uses, giving the frontend a generated, exhaustive list to guard summary coverage with. Part 1/6 of the activity redesign (#2434 shows the assembled result). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
b532dcf to
cba8373
Compare
3 participants
Part 1 of 8 of the activity redesign (readable summaries + before/after diffs, #2170). The full assembled result of parts 1–6 — reviewed end-to-end by CodeRabbit and verified green — lived in #2434 (now closed); it was too large to merge in one piece, so it's split into a stacked sequence where each part is independently green and reviewable. This PR is the only one open; each next part gets its PR when its predecessor merges.
The sequence (each branch builds on the previous; part 6's tree is byte-identical to the reviewed #2434 head):
claude/activity-1-backend-change-info(this PR) — backend enrichment + change-type codegen. No user-visible change.claude/activity-2-ws-code-chips— writing-system codes render as monospace chips app-wide (editor field labels, dictionary preview) for parity with the upcoming read-only diffs.claude/activity-3-diff-components— the read-only diff-view component library (field-level before/after, inline text diff via@sanity/diff-match-patch, audio diffs) + storybook gallery + field-coverage guard tests. No consumer yet.claude/activity-4-list-summaries— the activity list rows become readable summaries ("Apfel › apple · Set Definition (en) to …") grouped by entry, with per-author colours, badges, and bulk-create collapsing; exhaustiveness tests fail if a new change type lacks a summary.claude/activity-5-detail-previews— the detail panel renders real before/after diffs through the diff components; single-entry create commits collapse to an assembled read-only view; per-card failure containment.claude/activity-6-list-detail-layout— extracts Browse's responsive list↔detail shell into a sharedListDetailLayoutand adopts it in Activity: resizable split panes on desktop, full-width list on mobile with full-screen detail + back-button support.claude/activity-7-change-type-filter— replaces the flat ~65-row activity-type multi-select with a grouped, searchable, tri-state filter (sections by what a change touches, collapsed by default, hover "Only" shortcuts, empty selection = unfiltered, debounced instant apply).claude/activity-8-author-filter— unifies the author facet onto the same shell and semantics (sharedFacetFiltercomponent, empty = unfiltered, ×-clear, "Only"), deleting the old 'all'-sentinel machinery and the "No authors → empty feed" state.UI previews for the filter parts (7 and 8) — open in a browser:
https://raw.githubusercontent.com/sillsdev/languageforge-lexbox/ff896559192ae3bb4f406ecde6272963a5de1142/docs/img/pr-activity-filter/filter-collapsed-groups.pnghttps://raw.githubusercontent.com/sillsdev/languageforge-lexbox/ff896559192ae3bb4f406ecde6272963a5de1142/docs/img/pr-activity-filter/filter-group-expanded.pnghttps://raw.githubusercontent.com/sillsdev/languageforge-lexbox/ff896559192ae3bb4f406ecde6272963a5de1142/docs/img/pr-activity-filter/filter-active-trigger.pngThis part
Adds
ActivityChangeInfoResolver: one batched pass per activity page that labels each change with the entity it's about (entry headword with homograph subscript, "headword › gloss" for senses, vocab object names), any item the change references only by id (the part of speech set, the semantic domain removed, the component linked), and the change's root entry so the frontend can group a commit's changes. Deleted objects are recovered from their latest snapshot (oneTake(1)query per missing id) so a delete can still name its subject.ProjectActivityreturns a materialized page (Task<ProjectActivity[]>) of enriched record copies (ChangeInfois init-only) since the resolver batch-loads across the page — every consumer already materialized.ChangeContextgainsPreviousSnapshot(the entity's state just before the commit), the data needed for true before/after diffs (#2170).A Reinforced.Typings generator emits a
ChangeTypestring-literal union +knownChangeTypesarray from the registered CRDT change types, using the same discriminator helper the runtime uses — parts 4 and 7's coverage tests are built on it, so a new change type can't silently ship without a summary or a filter group.Tests:
HistoryServiceActivityTestscovers subject/target naming (homograph only-when-assigned, morph-type markers, sense numbering, deleted-object recovery, component endpoints, reorders) — 36 tests, all passing. Frontend impact is limited to regenerated types (additive) and a one-lineHistoryViewfix for the new requiredchangeInfoproperty.🤖 Generated with Claude Code
https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj