Activity redesign: responsive split view, failure containment, review hardening (follow-on to #2388)#2434
Activity redesign: responsive split view, failure containment, review hardening (follow-on to #2388)#2434myieye wants to merge 65 commits into
Conversation
…r diff The activity list now says what actually happened in each change — e.g. "Apfel › apple · Set Definition (en) to …", "Changed part of speech to Noun", "Removed semantic domain 5.2 Food" — instead of just naming the change type, with Simple and Detailed modes mirroring the browse view. The detail panel shows a real before/after diff of the entry (a read-only reuse of the editor layout) in place of the old current-version toggle. Each summary names its subject (entry headword, "headword › gloss" for a sense, a vocab object's name) and any item a change references only by id (the part of speech set, the domain/component removed or linked). Entry headwords carry their homograph number as a subscript when assigned. Those names are resolved in one batched pass per page, so naming adds no per-row queries. The list of change types is generated from the backend via Reinforced.Typings and a coverage test fails until every type has a summary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two activity-summary wording tweaks, both prompted by review: - A reorder now reads "<entry> · Reordered sense <gloss>" — the parent entry is the subject and the moved item is named — instead of "<entry> › <gloss> · Reordered senses", which put one sense in the subject slot and read as if the whole list moved. Generalizes to examples and components: the parent is the subject, and the moved item is named when it has one. - A field edit on a vocabulary object now names its type, e.g. "part of speech Noun · Set Abbreviation (en) to …", matching how the created/edited/deleted vocab lines already name the type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A commit that only builds one entry (its creation plus that entry's own senses and fields) now renders as the finished entry in a read-only EntryEditor, instead of a stack of per-change diff cards. It reads the backend-resolved affectedEntries, so there's no change-by-change assembly and no dependence on recognising every change type — an unfamiliar change type can't break it. Other commits keep their per-change cards, and the "(N changes)" header stays. Adds a Storybook "activity/previews" gallery covering every preview pane (entry/sense/example diffs, complex-form component, generic JSON, the no-preview fallback) and the collapsed entry view, so these can be checked without a live project. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Large commits no longer render a line per change. A commit is classified from cheap signals it already carries — its change-type keys, per-change root ids, and count — without parsing every change: building one entry collapses to "Created entry X", a batch of one create type collapses to "Created 100 semantic domains", and anything else lists the first changes (capped in Detailed mode) with the rest counted. So a 100-change sync commit is one or a few lines, and Simple mode never parses past the first change. Tests to keep this honest as the model grows — covering both ends and the "don't forget to handle it" cases: - every create change type has a bulk-collapse noun (new guard); - the view config covers every model field — a new field must be configured or explicitly excluded (new guard); - every backend change type still maps to a summary, and diff primitives render exactly the configured fields (existing guards); - backend: the resolver names a vocab subject and the referenced part of speech, alongside the existing homograph and reorder coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-PR review follow-ups: - export the two new generated types from the LcmCrdt barrel (index.ts) - give the multi-key change-type filter test a positive count assertion so it can't pass vacuously on an empty result - order multi-writing-system vocab labels by writing-system code, matching Entry.Headword(), so the shown alternative is deterministic - SenseLabel is never null — drop the misleading nullable return - remove the unused `activity` prop from ActivityItemChangePreview - trim two design-rationale comments to their load-bearing part - add translator context for the activity strings (Detailed, the Set-field lines, the "+N more" overflow) and broaden the shared "List mode" comment Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
change-summary.ts keys are backend change-type discriminators and wire-format property names, all intentionally PascalCase, so disable naming-convention for the file (and the test's PascalCase payloads) rather than quoting ~50 keys. Reuse displayValue for span text instead of String()-ing an unknown, and drop an unused handler arg and an unused derived. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ReplaceComplexFormTypeChange had a summary handler but the backend never emits it — the change class isn't registered, so it's absent from the generated knownChangeTypes. Remove the dead handler and add a reverse-coverage test: every handler must map to a real generated change type. Paired with the existing forward check (every known type is handled), the two force the handler back the moment the backend registers the type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Headwords use the best non-audio writing system + morph-type markers, so created entries show a real headword instead of "(Unknown)"; truly headword-less entries show a clear "(no headword)" placeholder. - Standardized wording (dropped inconsistent articles); data values render as subtle pills so they stand apart from the template text; long rows truncate instead of breaking the layout. - Fixed the activity page blowing out when a row is expanded (detail column min-width:0, content scrolls/wraps). - Homograph: a dedicated summary for the homograph-number patch, and the subscript now shows in the preview header. - Collapse a sense-creation commit (sense + its examples) to one line. - Moved the ChangeTypes generation marker out of a confusing public file into TypeGen (internal). Tests (resolver best-WS/morph/empty; frontend sense-collapse + homograph patch), storybook galleries, and i18n context updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eviews, sense numbers
Builds on the merge of develop (new Sense.pictures field + 5 new change types) and addresses the round-2 review feedback + CodeRabbit:
- Handle the 5 develop-added change types (sense pictures ×4 + set-main-publication) with proper summaries; configure the new Sense.pictures field.
- Create-entry commit no longer blows out the page when expanded (the collapse-to-editor wrapper gets min-w-0/min-h-0 — the spot the earlier grid fix missed).
- VLists lazy-load properly (itemSize set) so a 19-change commit no longer loads all 19 at once.
- Resolver: project only the columns it reads (no heavy JSONB hydration), load morph types lazily/projected; sense subjects disambiguate by number ("… › sense 2" / "gloss (2)") for empty/duplicate glosses.
- Styled before/after previews for all 7 vocab object types (was raw JSON), guarded by a preview-entityType coverage test.
- View-aware activity nouns (Lite "word/meaning" vs Classic), matching the field labels.
- Sense-creation collapse only fires on a pure sense tree (won't hide a mixed edit).
- CodeRabbit: created-semantic-domain summary keeps its name (not just code); homograph subscript shows with the Untitled placeholder too; clarifying comment on the ChangeTypes discriminator fallback (it intentionally mirrors Harmony — failing fast there breaks SetPartOfSpeechChange).
Tests: resolver targets/reorders/vocab subjects/sense numbers; frontend new facts, collapse purity, semantic-domain label; preview-entityType guard. i18n extracted + context added.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-91bf15 # Conflicts: # frontend/viewer/src/locales/en.po # frontend/viewer/src/locales/es.po # frontend/viewer/src/locales/fr.po # frontend/viewer/src/locales/id.po # frontend/viewer/src/locales/ko.po # frontend/viewer/src/locales/ms.po # frontend/viewer/src/locales/sw.po # frontend/viewer/src/locales/vi.po
A delete change has a null snapshot, so the History button's context.snapshot?.id was undefined and clicking it did nothing. Fall back to context.previousSnapshot?.id (the pre-delete entity) so History works there too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e preview, layout Baseline checkpoint of the activity-view iteration before the design-review pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n dark mode Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…boxed data Subject renders as a bold borderless token (the scannable element); the verb phrase inherits a muted container colour; data values stay as bordered chips. Drops the middot separators — the bold subject plus its trailing margin is enough separation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Detailed now groups a commit's facts by subject — the headword shows once as a bold header, its changes listed beneath along an indent rail — instead of repeating the subject on every line. Gives the two list modes distinct jobs (skim vs inspect) rather than just different lengths. Adds groupBySubject + a hideSubject prop on ChangeSummary; keyed by subject string so distinct senses of one entry don't wrongly fold together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Multi-select diffs merge before/after into one sorted list, so a removed item keeps its sorted position instead of trailing the kept ones (e.g. a removed semantic domain by code). - Audio-only example creates read as "with audio" instead of leaking the sil-media:// URI; firstAlternative now skips audio writing systems everywhere. - The example Reference field is framed like the text input it is (DiffShell) rather than rendered as bare text. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Any media value (audio-WS field or a sil-media:// value on any ws) now shows an audio marker — an i-mdi-volume-high icon plus "audio" — instead of leaking the media URI. Covers Set/Cleared field edits and audio-only example creates, and drops the unreadable audio ws code. First use of iconography in the summaries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The collapsed create-entry preview rendered the entry's CURRENT (possibly since-edited) state. Now it assembles the entry as-of the commit from that commit's own change snapshots: entry + its senses + their examples, in commit order. Part of speech is resolved from the id when the snapshot carries only that; semantic-domain names come denormalized from the sense snapshot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eal editor The collapsed create-entry preview used the real EntryEditor, so it looked unlike every other activity preview. Now a CollapsedEntryDiff composes the diff primitives (entry + senses + examples, nested like the editor via ObjectHeader) with no 'before', so a create reads as all-added — matching how a lone create-entry change already renders and unifying the panel's visual language. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resource changes were the confusing 'Create remote resource' (present tense, zero info). Now: create/upload/delete of a resource read as past-tense 'Added/Uploaded/Deleted audio recording' with the audio marker (kind inferred by correlating the resource id with a sibling audio-ws field-set in the commit), and the preview plays the resource via an audio player. The generic type fallback also past-tenses its leading verb so any future type reads consistently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DiffRichText/DiffMultiString filtered out audio writing systems entirely, so audio fields vanished from previews. They now render each audio ws through a new DiffAudio that plays the recording (via AudioInput), marking a replaced recording as removed. Also baseline-align the summary 'audio' chip: a plain inline span sits on the text baseline, where the previous inline-flex chip rode low. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The resource-create/upload/delete commit now reads 'Added audio recording' with the audio marker, and its preview plays the recording. Two constraints drove this: the resource change carries no kind, and the field-set that would identify it as audio lives in a SEPARATE commit — so kind can't be derived cheaply (would need a file fetch). Audio is the only resource kind today, so we assume it (documented). The preview couldn't find the resource because RemoteResource isn't an IObjectWithId (null snapshot, entityType 'Unknown'); we now read the resource id off the raw change instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/remove Empty audio delegates to AudioInput's own 'No audio' placeholder (matches the editor) instead of a blank. A replaced/removed recording now sits in a destructive-bordered surface above the added one in an emerald-bordered surface — a diff gutter reads better on a player than a line-through would. New surface-only palette tokens (bg tint, no text styling) for non-text diff content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A deleted component link is gone from the projection, so 'Removed component' had no headwords. Recover its endpoints from the create change (AddEntryComponentChange carries both entry ids) — two steps: load the CFC's changes, read the ids off the create — enough to name the complex form (subject) and component (target). Frontend renders the component target on the remove branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Detailed rows now carry a coloured gutter glyph per change (added +, removed −, changed ✎, reordered ↕) for fast visual grepping down a commit — Simple mode stays a plain-sentence skim. The per-row change cap goes 10→50 since Detailed is the inspect view; still bounded because a row isn't internally virtualised. New pure factCategory() (tested) keeps the mapping out of the template. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Custom-view previews now diff their field lists (entry/sense/example) and writing-system lists (vernacular/analysis) plus base — not just the name. A new vocab-diff-coverage test parses every vocab model's generated interface and asserts each property is either handled by DiffVocabPrimitive or explicitly ignored (vocab-diff-fields.ts) — so a new backend field can't silently vanish from the diff. The guard immediately caught WritingSystem.isAudio, now shown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DiffSelect drew the transition with a literal Unicode → ; use i-mdi-arrow-right like the rest of the icon system for consistent sizing and alignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CustomView.base and WritingSystem.type rendered raw enum values ('FwLite', '0'). Map them to the app's usual names — 'FieldWorks Lite'/'FieldWorks Classic' and 'Vernacular'/'Analysis' — keyed by both the string name and numeric value so it works whichever the wire sends; unknown values still fall through to the raw text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sense numbers are the sense's 1-based position, shown when an entry has more than one sense — independent of the gloss (FieldWorks' rule; homograph numbers, which key off identical headwords, stay separate). Replaces the old empty/duplicate-gloss subscript logic: >1 sense → "N gloss" (or "N" when gloss-less); a lone sense → its gloss or nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The ws-code chip/preview text used text-muted-foreground, which on the muted chip fill and muted dictionary-preview surface measured ~4.4:1 — below the 4.5:1 AA floor for this 12px text (light theme). Switch to foreground/60: near-identical light-grey look, but AA on every surface in both themes (worst case ~5.1:1 on the light muted fill). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
modelFields() regexes property names out of the generated interface; if that parse ever returns nothing (type-gen format drift), the coverage assertion would pass vacuously and stop guarding new fields. Assert a non-empty parse, matching the sibling guard in vocab-diff-coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…design-review-vpaqmr
…ne dead code The activity page now uses the same responsive primary-detail pattern as Browse: resizable list/detail panes on desktop (list keeps a usable minimum instead of crushing to 8rem), and on mobile a full-width list whose detail opens full-screen with a close button and back-button support. Auto-select of the newest commit is desktop-only; mobile never auto-navigates into a detail. A change whose context fails to load now renders an inline error card instead of leaving a permanently blank row. HistoryService.ProjectActivity returns a materialized page (Task<ProjectActivity[]>) since the label resolver already forces materialization and every consumer did too. The resolver no longer emits an untranslated 'sense' subject fallback; the subject degrades to null like other unresolved labels. Also: rename ActivityListViewOptions to ActivityViewPicker (it only picks the view), delete the storybook-only DiffValue component, fold DiffRichText into DiffMultiString, add screen-reader added/removed labels to the audio diff, and drop stale references to the removed Detailed list mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
…r context
vocab-diff-coverage now parses DiffVocabPrimitive's own present('…') rows instead of a
hand-maintained handled-set, so dropping a row fails the test just like adding a model
field does (and a stale-row check runs the other direction). change-summary gains
table-driven cases for the payload-picking handlers (Replace/Add publication, semantic
domain replace, translations, component links, vocab patch clears) and a pin on the
entry-tree recognizer accepting mixed create+edit commits.
ChangeTypesCodeGenerator now calls HistoryService.GetChangeTypeKeyFromType instead of
duplicating the TypeName reflection, so the generated ChangeType union can't drift from
the runtime discriminators. Fixed two test files still calling the old streaming
ProjectActivity shape.
New activity-log msgids get translator-context comments per I18N_CONTEXT_GUIDE.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
…by SHA from the PR)
…erenced by SHA from the PR)" This reverts commit cc47568.
|
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:
📝 WalkthroughWalkthroughChangesBackend activity enrichment
Frontend activity and diff experience
Validation and localization
Estimated code review effort: 5 (Critical) | ~120 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 |
|
Merge interactions with neighboring open PRs (for whoever merges second):
Landmine note: Generated by Claude Code |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
svelte-check (CI workspace mode) rejected the IMultiString built from asString's string|undefined results; missing alternatives now map to ''. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
Browse and Activity used near-identical ResizablePaneGroup + IsMobile + IfOnce plumbing for the same behavior: split panes on desktop, full-width list on mobile with the detail replacing it full-screen while open. Both views now render through one ListDetailLayout component; callers keep their own selection/open state and pass the combined detailVisible flag. Verified: browse-related Playwright UI suite (19 tests incl. back-navigation and mobile open/close flows) and full viewer unit tests pass; svelte-check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
|
@coderabbitai review Generated by Claude Code |
|
The PR description image (mobile list) shows the activity feed in the narrow/mobile layout: a full-width list of activity cards with filter dropdowns ("All authors", "All activity types") and a sort control ("Newest first") at the top. Cards show entries like "Deleted word Apfel", grouped changes for "Created word wulu₂" (with nested "Added meaning a flying insect", "Added publication Main Dictionary"), homograph note "wulu₁ Set homograph number to 1", and other field-edit rows, each with an "Unknown" author and relative timestamp. The PR description image (mobile detail) shows the full-screen detail view opened after tapping a row: a close (X) button, author/timestamp header, "Not synced" badge, a "History" button, Preview/Details tabs, and a red-washed "Deleted word Apfel" panel with fields like Word, Display as, Uses components as, Literal meaning, Note, etc. Thanks for the context on merge interactions with ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
frontend/viewer/src/lib/activity/assemble-entry.test.ts (1)
17-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the "latest wins" deduplication behavior.
The
latestByIdhelper takes the latest snapshot when an entity id appears multiple times in the commit, but no test exercises this path. A duplicate-sense test would guard against regressions in the deduplication logic.🧪 Suggested test
it('ignores senses that belong to a different entry', () => { const contexts = [ctx('Entry', entrySnap), ctx('Sense', {id: 's1', entryId: 'other'})]; expect(assembleEntryAtCommit(entryId, contexts, [])?.senses).toEqual([]); }); + + it('uses the latest snapshot when a sense appears multiple times in the commit', () => { + const contexts = [ + ctx('Entry', entrySnap), + ctx('Sense', {id: 's1', entryId, gloss: {en: 'old gloss'}}), + ctx('Sense', {id: 's1', entryId, gloss: {en: 'new gloss'}}), + ]; + const entry = assembleEntryAtCommit(entryId, contexts, []); + expect(entry?.senses).toHaveLength(1); + expect(entry?.senses[0].gloss).toEqual({en: 'new gloss'}); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/viewer/src/lib/activity/assemble-entry.test.ts` around lines 17 - 28, The test covering assembleEntryAtCommit should also exercise latestById deduplication by including duplicate Sense contexts with the same id and different snapshots. Assert that the assembled entry contains one instance of that sense using the latest snapshot’s values, while preserving the existing commit-order and example-sentence assertions.frontend/viewer/src/lib/entry-editor/diff-view/vocab-diff-coverage.test.ts (1)
13-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer resolving generated paths relative to the test file, not
process.cwd().The sibling coverage test (
entity-config-coverage.test.ts) resolves the same generated directory viafileURLToPath(import.meta.url), which is independent of the working directory. Usingprocess.cwd()here couples the test to being run fromfrontend/viewer; aligning both tests removes that hidden assumption.♻️ Suggested alignment
-import {describe, it, expect} from 'vitest'; -import {readFileSync} from 'node:fs'; -import {resolve} from 'node:path'; +import {describe, it, expect} from 'vitest'; +import {readFileSync} from 'node:fs'; +import {fileURLToPath} from 'node:url'; +import {dirname, join} from 'node:path'; import {IGNORED_VOCAB_FIELDS} from './vocab-diff-fields'; + +const here = dirname(fileURLToPath(import.meta.url));Then derive the model/component paths from
hereinstead ofprocess.cwd().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/viewer/src/lib/entry-editor/diff-view/vocab-diff-coverage.test.ts` around lines 13 - 26, Update propertyNames and handledFields to resolve paths from the test file location using the existing fileURLToPath(import.meta.url)-based here pattern from entity-config-coverage.test.ts, rather than process.cwd(). Derive both generated model and DiffVocabPrimitive.svelte paths relative to here while preserving the current file parsing behavior.frontend/viewer/src/lib/activity/change-summary.ts (1)
98-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
??fallback conflates a camelCasenullwith an absent value.
propreturnsrecord[camel] ?? record[pascal], so a legitimately-nullcamelCase property falls through to the (usuallyundefined) PascalCase variant. Today this is harmless — the only null-sensitive caller (SetPartOfSpeechChange) treatsnullandundefinedidentically — but any future handler that needs to distinguish "explicitly cleared to null" from "absent" would silently break. If you want to future-proof, prefer key presence over??.♻️ Presence-based lookup
- const camel = key[0].toLowerCase() + key.slice(1); - const pascal = key[0].toUpperCase() + key.slice(1); - return record[camel] ?? record[pascal]; + const camel = key[0].toLowerCase() + key.slice(1); + const pascal = key[0].toUpperCase() + key.slice(1); + return camel in record ? record[camel] : record[pascal];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/viewer/src/lib/activity/change-summary.ts` around lines 98 - 104, Update the prop function’s camelCase/PascalCase lookup to use property presence rather than nullish coalescing, so an explicitly null camelCase value is returned as null while the PascalCase variant is used only when the camelCase key is absent.frontend/viewer/src/lib/activity/ChangeSummary.svelte (1)
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the dense
editObjectFieldone-liner for readability.Line 222 nests object-type label, subject token, cleared/ws/value branches, and two more nested conditionals all on a single line. Since whitespace at
{#if}/{:else}/snippet boundaries is trimmed (per this file's own documented spacing contract), reformatting across multiple lines is behavior-preserving and would make this branch much easier to scan/debug.♻️ Suggested reformat (behavior-preserving)
-{:else if fact.kind === 'editObjectField'} - {`@const` objectType = capitalize(objectNoun(fact.object))} - {objectType}{`#if` subject}{`@render` subjectToken(subject, true)}{/if}{`#if` fact.cleared}{$t`Cleared ${fact.field}`}{`#if` fact.ws}{`@render` wsCode(fact.ws)}{/if}{:else if fact.value !== undefined}{`@const` value = fact.value}{`#if` fact.ws}<T msg="Set # to #" cmt="First # is a field label plus writing-system code, second # is the new value">{fact.field}{`@render` wsCode(fact.ws)}{`#snippet` second()}{`@render` chip(value, true)}{/snippet}</T>{:else}{$t`Set ${fact.field} to`}{`@render` chip(value)}{/if}{:else}{$t`Changed ${fact.field}`}{/if} +{:else if fact.kind === 'editObjectField'} + {`@const` objectType = capitalize(objectNoun(fact.object))} + {objectType} + {`#if` subject}{`@render` subjectToken(subject, true)}{/if} + {`#if` fact.cleared} + {$t`Cleared ${fact.field}`}{`#if` fact.ws}{`@render` wsCode(fact.ws)}{/if} + {:else if fact.value !== undefined} + {`@const` value = fact.value} + {`#if` fact.ws} + <T msg="Set # to #" cmt="First # is a field label plus writing-system code, second # is the new value">{fact.field}{`@render` wsCode(fact.ws)}{`#snippet` second()}{`@render` chip(value, true)}{/snippet}</T> + {:else} + {$t`Set ${fact.field} to`}{`@render` chip(value)} + {/if} + {:else} + {$t`Changed ${fact.field}`} + {/if}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/viewer/src/lib/activity/ChangeSummary.svelte` around lines 220 - 222, Reformat the `editObjectField` branch in `ChangeSummary.svelte` into readable multiline markup, separating the `objectType`/`subjectToken` output from the `cleared`, `value`, writing-system, and fallback conditionals. Preserve the existing rendering order, conditions, translations, snippets, and whitespace behavior; make no functional changes.backend/FwLite/LcmCrdt/HistoryService.cs (1)
336-348: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
LoadPreviousSnapshotmaterializes the entity's full commit history to find one adjacent commit.
affectingCommitIdsloads every commit ID that ever touched the entity, then does an in-memoryIndexOfto locate the adjacent one. For heavily-edited entries this is an unbounded list load on every "view change details" call, where a targeted query (e.g., filter by ordering key relative tocommitId,OrderByDescending().Take(1)) would return the same result with a bounded query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/LcmCrdt/HistoryService.cs` around lines 336 - 348, Update LoadPreviousSnapshot to query only the single most recent commit affecting entityId that precedes commitId, using the existing commit ordering and a descending filter with Take(1). Remove the full affectingCommitIds materialization and in-memory IndexOf logic, preserve the null result when no earlier affecting commit exists, and pass the selected commit ID to dataModel.GetAtCommit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/FwLite/LcmCrdt/ActivityChangeInfoResolver.cs`:
- Around line 254-269: Update RecoverDeleted to select only the latest snapshot
per missing EntityId within the database query, rather than materializing all
snapshots and grouping with group.Last() in memory. Apply the same query-level
latest-row filtering in LoadComponentEndpointsFromCreate for
ChangeEntity<IChange>, before narrowing results to AddEntryComponentChange, and
remove the redundant in-memory grouping.
In `@backend/FwLite/LcmCrdt/HistoryService.cs`:
- Around line 57-59: Make ProjectActivity immutable by changing ChangeInfo from
a mutable setter to an init-only property, then update
ActivityChangeInfoResolver.ResolveAsync to return new ProjectActivity instances
using with expressions populated with the resolved ChangeInfo instead of
mutating existing activities or their arrays. Update callers to use the returned
activities while preserving the existing ordering and enrichment behavior.
- Around line 324-334: Update the previous-snapshot lookup in the change-context
construction to use the selected change’s identity, including its changeIndex,
rather than only commitId and entityId. Adjust LoadPreviousSnapshot and its
caller as needed so later same-entity changes in an AddChanges commit compare
against the state immediately before that specific change; keep snapshot
resolution and affected-entry handling unchanged.
In `@frontend/viewer/src/lib/history/HistoryView.svelte`:
- Line 106: Update the ActivityItem invocation in HistoryView to pass
record.changeInfo for the changeInfo field instead of an empty array, while
preserving the existing record change and changeTypes values.
In `@frontend/viewer/src/locales/en.po`:
- Around line 222-224: Rerun the locale catalog extraction to regenerate
frontend/viewer/src/locales/en.po, ensuring the {0} activity-log translator note
attaches to the intended activity-log string rather than “Add your first entry
to start building your dictionary.” and the Code field-label note attaches to
“Code” rather than “Closed.”
In `@frontend/viewer/src/locales/es.po`:
- Around line 227-230: Move the extractor comments from the generated es.po
entry to their corresponding source strings in DashboardView.svelte: place the
fallback activity-log comment on the matching list-field fallback string and the
translation-context comment on “Total entries.” Remove the misleading comments
from the empty-state prompt and Total entries entries in the generated PO
output.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt/HistoryService.cs`:
- Around line 336-348: Update LoadPreviousSnapshot to query only the single most
recent commit affecting entityId that precedes commitId, using the existing
commit ordering and a descending filter with Take(1). Remove the full
affectingCommitIds materialization and in-memory IndexOf logic, preserve the
null result when no earlier affecting commit exists, and pass the selected
commit ID to dataModel.GetAtCommit.
In `@frontend/viewer/src/lib/activity/assemble-entry.test.ts`:
- Around line 17-28: The test covering assembleEntryAtCommit should also
exercise latestById deduplication by including duplicate Sense contexts with the
same id and different snapshots. Assert that the assembled entry contains one
instance of that sense using the latest snapshot’s values, while preserving the
existing commit-order and example-sentence assertions.
In `@frontend/viewer/src/lib/activity/change-summary.ts`:
- Around line 98-104: Update the prop function’s camelCase/PascalCase lookup to
use property presence rather than nullish coalescing, so an explicitly null
camelCase value is returned as null while the PascalCase variant is used only
when the camelCase key is absent.
In `@frontend/viewer/src/lib/activity/ChangeSummary.svelte`:
- Around line 220-222: Reformat the `editObjectField` branch in
`ChangeSummary.svelte` into readable multiline markup, separating the
`objectType`/`subjectToken` output from the `cleared`, `value`, writing-system,
and fallback conditionals. Preserve the existing rendering order, conditions,
translations, snippets, and whitespace behavior; make no functional changes.
In `@frontend/viewer/src/lib/entry-editor/diff-view/vocab-diff-coverage.test.ts`:
- Around line 13-26: Update propertyNames and handledFields to resolve paths
from the test file location using the existing
fileURLToPath(import.meta.url)-based here pattern from
entity-config-coverage.test.ts, rather than process.cwd(). Derive both generated
model and DiffVocabPrimitive.svelte paths relative to here while preserving the
current file parsing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78fc9bdc-1307-4fe4-80c9-7ff6cd7629c5
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (75)
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.csbackend/FwLite/FwLiteShared/TypeGen/ChangeTypesCodeGenerator.csbackend/FwLite/FwLiteShared/TypeGen/ChangeTypesMarker.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/DateTimeOffsetOrmParityTests.csbackend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.csbackend/FwLite/LcmCrdt.Tests/OpenProjectTests.csbackend/FwLite/LcmCrdt/ActivityChangeInfoResolver.csbackend/FwLite/LcmCrdt/HistoryService.csfrontend/viewer/package.jsonfrontend/viewer/src/lib/activity/ActivityFilter.sveltefrontend/viewer/src/lib/activity/ActivityItem.sveltefrontend/viewer/src/lib/activity/ActivityItemChangePreview.sveltefrontend/viewer/src/lib/activity/ActivityView.sveltefrontend/viewer/src/lib/activity/ActivityViewPicker.sveltefrontend/viewer/src/lib/activity/AuthorLabel.sveltefrontend/viewer/src/lib/activity/ChangeSummary.sveltefrontend/viewer/src/lib/activity/CollapsedEntryDiff.sveltefrontend/viewer/src/lib/activity/CollapsedSenseDiff.sveltefrontend/viewer/src/lib/activity/EntryHeadwordButton.sveltefrontend/viewer/src/lib/activity/PreviewViewScope.sveltefrontend/viewer/src/lib/activity/assemble-entry.test.tsfrontend/viewer/src/lib/activity/assemble-entry.tsfrontend/viewer/src/lib/activity/author-color-service.svelte.tsfrontend/viewer/src/lib/activity/author-color.test.tsfrontend/viewer/src/lib/activity/author-color.tsfrontend/viewer/src/lib/activity/change-summary.test.tsfrontend/viewer/src/lib/activity/change-summary.tsfrontend/viewer/src/lib/activity/fact-glyph.tsfrontend/viewer/src/lib/activity/preview-entity-coverage.test.tsfrontend/viewer/src/lib/components/ListItem.sveltefrontend/viewer/src/lib/components/dictionary/DictionaryEntry.sveltefrontend/viewer/src/lib/components/dictionary/Headwords.sveltefrontend/viewer/src/lib/components/field-editors/multi-ws-input.sveltefrontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.sveltefrontend/viewer/src/lib/components/writing-system/WsCode.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/ChangeTypes.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityChangeInfo.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IChangeContext.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectActivity.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/index.tsfrontend/viewer/src/lib/entry-editor/diff-view/DiffAudio.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffEntryLinkList.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffEntryPrimitive.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffExamplePrimitive.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffMultiSelect.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffMultiString.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffRichText.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffSelect.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffSensePrimitive.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffShell.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffText.sveltefrontend/viewer/src/lib/entry-editor/diff-view/DiffVocabPrimitive.sveltefrontend/viewer/src/lib/entry-editor/diff-view/diff-classes.tsfrontend/viewer/src/lib/entry-editor/diff-view/diff-field-coverage.test.tsfrontend/viewer/src/lib/entry-editor/diff-view/inline-diff.test.tsfrontend/viewer/src/lib/entry-editor/diff-view/inline-diff.tsfrontend/viewer/src/lib/entry-editor/diff-view/vocab-diff-coverage.test.tsfrontend/viewer/src/lib/entry-editor/diff-view/vocab-diff-fields.tsfrontend/viewer/src/lib/history/HistoryView.sveltefrontend/viewer/src/lib/layout/ListDetailLayout.sveltefrontend/viewer/src/lib/utils/search-params.tsfrontend/viewer/src/lib/views/entity-config-coverage.test.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/browse/BrowseView.sveltefrontend/viewer/src/stories/activity/previews.stories.sveltefrontend/viewer/src/stories/activity/summaries.stories.sveltefrontend/viewer/src/stories/editor/diff/diff-overview.stories.svelte
| private static async Task RecoverDeleted<T>(ICrdtDbContext db, HashSet<Guid> ids, Dictionary<Guid, T> loaded) | ||
| where T : class, IObjectWithId | ||
| { | ||
| var missing = ids.Where(id => !loaded.ContainsKey(id)).ToHashSet(); | ||
| if (missing.Count == 0) return; | ||
| var snapshots = await ( | ||
| from commit in db.Commits.DefaultOrder() | ||
| from snapshot in db.Snapshots.InnerJoin(s => s.CommitId == commit.Id) | ||
| where missing.Contains(snapshot.EntityId) | ||
| select snapshot | ||
| ).ToListAsyncLinqToDB(); | ||
| foreach (var group in snapshots.GroupBy(s => s.EntityId)) | ||
| { | ||
| if (group.Last().Entity.DbObject is T entity) loaded[group.Key] = entity; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any existing "latest snapshot per entity" helper in SIL.Harmony usage across the backend,
# which could be reused here instead of loading full history.
rg -n -C3 --type=cs 'Snapshots\.' backend/FwLite | rg -v 'ActivityChangeInfoResolver'
rg -n -C3 --type=cs 'DefaultOrder\(' backend/FwLiteRepository: sillsdev/languageforge-lexbox
Length of output: 6329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the resolver around the two relevant methods.
FILE=backend/FwLite/LcmCrdt/ActivityChangeInfoResolver.cs
wc -l "$FILE"
sed -n '240,350p' "$FILE"
# Inspect the related history query for any reusable patterns.
FILE=backend/FwLite/LcmCrdt/HistoryService.cs
sed -n '273,320p' "$FILE"Repository: sillsdev/languageforge-lexbox
Length of output: 8157
RecoverDeleted and LoadComponentEndpointsFromCreate should filter earlier
RecoverDeletedloads every snapshot for each missing id and then doesGroupBy(...).Last()in memory, even though only the latest snapshot is needed.LoadComponentEndpointsFromCreatedoes the same forChangeEntity<IChange>before narrowing toAddEntryComponentChange.- Push the “latest/create row only” selection into the query to avoid avoidable I/O and memory pressure as history grows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/FwLite/LcmCrdt/ActivityChangeInfoResolver.cs` around lines 254 - 269,
Update RecoverDeleted to select only the latest snapshot per missing EntityId
within the database query, rather than materializing all snapshots and grouping
with group.Last() in memory. Apply the same query-level latest-row filtering in
LoadComponentEndpointsFromCreate for ChangeEntity<IChange>, before narrowing
results to AddEntryComponentChange, and remove the redundant in-memory grouping.
Source: Coding guidelines
| /// <summary>Resolved display info per change, parallel to <see cref="Changes"/> by index. Set during enrichment.</summary> | ||
| public IReadOnlyList<ActivityChangeInfo> ChangeInfo { get; set; } = []; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mutable setter on ProjectActivity breaks record immutability.
ChangeInfo uses { get; set; } and is mutated post-construction by the resolver, which conflicts with the repo guideline to prefer immutable records for DTOs. Consider having ActivityChangeInfoResolver.ResolveAsync return the resolved list (or use with expressions) instead of mutating the array elements in place.
As per coding guidelines: "Prefer records for DTOs and immutable data structures".
♻️ Suggested direction
- /// <summary>Resolved display info per change, parallel to <see cref="Changes"/> by index. Set during enrichment.</summary>
- public IReadOnlyList<ActivityChangeInfo> ChangeInfo { get; set; } = [];
+ /// <summary>Resolved display info per change, parallel to <see cref="Changes"/> by index. Set during enrichment.</summary>
+ public IReadOnlyList<ActivityChangeInfo> ChangeInfo { get; init; } = [];This would require ActivityChangeInfoResolver.ResolveAsync to return new ProjectActivity instances (e.g. via activity with { ChangeInfo = ... }) rather than mutating the passed-in array in place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/FwLite/LcmCrdt/HistoryService.cs` around lines 57 - 59, Make
ProjectActivity immutable by changing ChangeInfo from a mutable setter to an
init-only property, then update ActivityChangeInfoResolver.ResolveAsync to
return new ProjectActivity instances using with expressions populated with the
resolved ChangeInfo instead of mutating existing activities or their arrays.
Update callers to use the returned activities while preserving the existing
ordering and enrichment behavior.
Source: Coding guidelines
| var previousSnapshot = await LoadPreviousSnapshot(crdtDbContext, commitId, change.EntityId); | ||
|
|
||
| await ResolveSensePartOfSpeech(snapshot); | ||
| await ResolveSensePartOfSpeech(previousSnapshot); | ||
|
|
||
| var affectedEntries = await GetAffectedEntryIds(change) | ||
| .Select(async (Guid entryId, CancellationToken _) => await GetCurrentOrLatestEntry(entryId)) | ||
| .ToArrayAsync(); | ||
|
|
||
| return new ChangeContext(change, snapshot, previousSnapshot, affectedEntries); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether commits can bundle multiple ChangeEntities against the same entity,
# and whether existing tests cover per-changeIndex before/after within a multi-change commit.
rg -n 'ChangeEntities' backend/FwLite/LcmCrdt -g '*.cs' -B2 -A8
fd -HI 'HistoryServiceActivityTests.cs' backend/FwLite/LcmCrdt.Tests -x cat {}Repository: sillsdev/languageforge-lexbox
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant methods and surrounding history logic.
sed -n '300,380p' backend/FwLite/LcmCrdt/HistoryService.cs
# Look for any per-changeIndex snapshot handling or same-entity multi-change handling.
rg -n 'changeIndex|LoadPreviousSnapshot|GetAtCommit<|ChangeEntities.*Index|Index == changeIndex|previousSnapshot' backend/FwLite/LcmCrdt -g '*.cs' -A3 -B3
# Inspect the commit/change entity model to see whether multiple changes per entity in one commit are represented anywhere.
rg -n 'class ChangeEntity|record ChangeEntity|EntityId|Index' backend/FwLite/LcmCrdt -g '*.cs' -A4 -B4Repository: sillsdev/languageforge-lexbox
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- HistoryService.cs (relevant section) ---'
sed -n '300,370p' backend/FwLite/LcmCrdt/HistoryService.cs
echo
echo '--- LoadPreviousSnapshot definition ---'
rg -n 'LoadPreviousSnapshot\(' backend/FwLite/LcmCrdt/HistoryService.cs -A30 -B5
echo
echo '--- Activity/history tests mentioning changeIndex, previousSnapshot, or multiple changes ---'
rg -n 'changeIndex|previousSnapshot|LoadPreviousSnapshot|GetChangeContext|Activity' backend/FwLite/LcmCrdt.Tests -g '*Tests.cs' -A4 -B4 --max-count 80Repository: sillsdev/languageforge-lexbox
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Search for multi-change commit creation helpers and overloads ---'
rg -n 'AddChange\(|AddChanges\(|ChangesNameHelper|CreateCommit|CommitMetadata' backend/FwLite/LcmCrdt -g '*.cs' -A4 -B4 --max-count 120
echo
echo '--- Inspect HistoryService change-context callers/usages ---'
rg -n 'LoadChangeContext\(|ChangeContext\(' backend/FwLite/LcmCrdt -g '*.cs' -A4 -B4 --max-count 60
echo
echo '--- Inspect HistoryServiceActivityTests for any commit with multiple changes in one commit ---'
rg -n 'AddChange\(ClientId, .*Meta\(\)\)|AddChange\(ClientId, .*CommitMetadata|await DataModel.AddChange' backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs -A3 -B3 --max-count 120Repository: sillsdev/languageforge-lexbox
Length of output: 50385
Compute previousSnapshot from the selected change, not just the commit
LoadPreviousSnapshot(commitId, entityId) returns the state before the last commit that touched the entity. For commits built with AddChanges(...), a later changeIndex on the same entity will compare against the pre-commit state instead of the state before that specific change, so the Activity diff can attribute earlier same-commit edits to the wrong change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/FwLite/LcmCrdt/HistoryService.cs` around lines 324 - 334, Update the
previous-snapshot lookup in the change-context construction to use the selected
change’s identity, including its changeIndex, rather than only commitId and
entityId. Adjust LoadPreviousSnapshot and its caller as needed so later
same-entity changes in an AddChanges commit compare against the state
immediately before that specific change; keep snapshot resolution and
affected-entry handling unchanged.
| <div class="w-full"> | ||
| {#if record} | ||
| <ActivityItem activity={{...record, changes: [record.change], changeTypes: []}}/> | ||
| <ActivityItem activity={{...record, changes: [record.change], changeTypes: [], changeInfo: []}}/> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how ActivityItem.svelte consumes activity.changes vs activity.changeInfo
fd -a 'ActivityItem.svelte' | xargs -I{} sh -c 'echo "== {} =="; cat -n "{}"'Repository: sillsdev/languageforge-lexbox
Length of output: 21074
Pass the record’s changeInfo through here
ActivityItem looks up changeInfo by change.change.index, so hardcoding [] drops the subject/root metadata for every rendered change. Pass the existing record.changeInfo instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/viewer/src/lib/history/HistoryView.svelte` at line 106, Update the
ActivityItem invocation in HistoryView to pass record.changeInfo for the
changeInfo field instead of an empty array, while preserving the existing record
change and changeTypes values.
| #. Activity-log line (fallback for list fields without a singular noun). {0} = plural field label (e.g. a field label from entity config). A chip with the added item follows. | ||
| #: src/project/dashboard/DashboardView.svelte | ||
| msgid "Add your first entry to start building your dictionary." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Flag PO entries whose #. comment references {0}/{1} but the msgid has no placeholder,
# which indicates a comment attached to the wrong msgid.
fd -e po . frontend/viewer/src/locales --exec python3 - {} <<'PY'
import sys, re
path = sys.argv[1]
block = []
def check(block, path):
comments = [l for l in block if l.startswith('#.')]
msgid = next((l for l in block if l.startswith('msgid ')), '')
ctext = ' '.join(comments)
if re.search(r'\{[01]\}', ctext) and not re.search(r'\{[01]\}|#', msgid):
print(f"{path}: comment mentions placeholder but msgid has none -> {msgid.strip()}")
with open(path, encoding='utf-8') as f:
for line in f:
if line.strip() == '':
check(block, path); block = []
else:
block.append(line)
check(block, path)
PYRepository: sillsdev/languageforge-lexbox
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='frontend/viewer/src/locales/en.po'
python3 - <<'PY'
from pathlib import Path
import re
path = Path('frontend/viewer/src/locales/en.po')
lines = path.read_text(encoding='utf-8').splitlines()
targets = [33, 222, 535]
for t in targets:
start = max(1, t - 6)
end = min(len(lines), t + 10)
print(f"\n=== lines {start}-{end} around {t} ===")
for i in range(start, end + 1):
print(f"{i:4}: {lines[i-1]}")
PYRepository: sillsdev/languageforge-lexbox
Length of output: 2757
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('frontend/viewer/src/locales/en.po')
lines = path.read_text(encoding='utf-8').splitlines()
start, end = 545, 556
for i in range(start, end + 1):
print(f"{i:4}: {lines[i-1]}")
PYRepository: sillsdev/languageforge-lexbox
Length of output: 623
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
path = Path('frontend/viewer/src/locales/en.po')
lines = path.read_text(encoding='utf-8').splitlines()
for needle in ['msgid "Code"', 'msgid "Closed"', 'msgid "(unknown)"', 'msgid "Add your first entry to start building your dictionary."']:
print(f"\n=== {needle} ===")
for i, line in enumerate(lines, 1):
if line == needle:
start = max(1, i-3)
end = min(len(lines), i+3)
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
print()
PYRepository: sillsdev/languageforge-lexbox
Length of output: 1869
Realign the misplaced translator comments
frontend/viewer/src/locales/en.po:222-224, 535-537 has two stale #. comments attached to the wrong msgid:
- the
{0}activity-log context sits aboveAdd your first entry to start building your dictionary. - the
Codefield-label context sits aboveClosed
Rerun the catalog extraction so the translator notes line up with the intended strings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/viewer/src/locales/en.po` around lines 222 - 224, Rerun the locale
catalog extraction to regenerate frontend/viewer/src/locales/en.po, ensuring the
{0} activity-log translator note attaches to the intended activity-log string
rather than “Add your first entry to start building your dictionary.” and the
Code field-label note attaches to “Code” rather than “Closed.”
| #. Activity-log line (fallback for list fields without a singular noun). {0} = plural field label (e.g. a field label from entity config). A chip with the added item follows. | ||
| #: src/project/dashboard/DashboardView.svelte | ||
| msgid "Add your first entry to start building your dictionary." | ||
| msgstr "" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the source annotations to confirm the misattributed extractor comments.
rg -nP -C3 'Add your first entry to start building your dictionary' -g '*.svelte'
rg -nP -C3 'fallback for list fields without a singular noun' -g '*.svelte' -g '*.ts'
rg -nP -C3 'Total entries' -g '*.svelte'Repository: sillsdev/languageforge-lexbox
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- es.po context around the flagged entry ---'
python3 - <<'PY'
from pathlib import Path
p = Path('frontend/viewer/src/locales/es.po')
lines = p.read_text(encoding='utf-8').splitlines()
for start in [220, 2668]:
print(f'\n### {p}:{start}-{start+20}')
for i in range(start-1, min(len(lines), start+20)):
print(f'{i+1}:{lines[i]}')
PY
echo
echo '--- search for the exact msgids and nearby source comments ---'
rg -n -C 2 -F 'Add your first entry to start building your dictionary.' .
rg -n -C 2 -F 'Total entries' .
rg -n -C 2 -F 'fallback for list fields without a singular noun' .
rg -n -C 2 -F 'A chip with the added item follows.' .
rg -n -C 2 -F 'translation' frontend/viewer/src frontend .Repository: sillsdev/languageforge-lexbox
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inventory of locale-related source files ---'
git ls-files 'frontend/viewer/src/**/*' | rg 'locale|locales|svelte|ts|js|po|json'
echo
echo '--- candidate extractor/source annotation search across frontend/viewer/src ---'
rg -n -C 2 -F '{0} = plural field label' frontend/viewer/src || true
rg -n -C 2 -F 'plural field label' frontend/viewer/src || true
rg -n -C 2 -F 'A chip with the added item follows.' frontend/viewer/src || true
rg -n -C 2 -F 'Add your first entry to start building your dictionary.' frontend/viewer/src || true
rg -n -C 2 -F 'Total entries' frontend/viewer/src || trueRepository: sillsdev/languageforge-lexbox
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect neighboring entries in frontend/viewer/src/locales/es.po ---'
python3 - <<'PY'
from pathlib import Path
p = Path('frontend/viewer/src/locales/es.po')
lines = p.read_text(encoding='utf-8').splitlines()
for start in [220, 2668]:
print(f'\n### {p}:{start}-{start+18}')
for i in range(start-1, min(len(lines), start+18)):
print(f'{i+1}:{lines[i]}')
PYRepository: sillsdev/languageforge-lexbox
Length of output: 1943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- DashboardView.svelte context around the dashboard prompt and totals ---'
ast-grep outline frontend/viewer/src/project/dashboard/DashboardView.svelte --view expanded || true
echo
rg -n -C 4 -F 'Add your first entry to start building your dictionary.' frontend/viewer/src/project/dashboard/DashboardView.svelte
echo
rg -n -C 4 -F 'Total entries' frontend/viewer/src/project/dashboard/DashboardView.svelte
echo
rg -n -C 4 -F 'A chip with the added item follows.' frontend/viewer/src/project/dashboard/DashboardView.svelte
echo
rg -n -C 4 -F 'fallback for list fields without a singular noun' frontend/viewer/src/project/dashboard/DashboardView.svelteRepository: sillsdev/languageforge-lexbox
Length of output: 2403
Move the extractor comments to the matching strings in frontend/viewer/src/project/dashboard/DashboardView.svelte. The fallback comment is attached to the empty-state prompt, and the translation context comment is attached to Total entries; both are misleading in the generated .po.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/viewer/src/locales/es.po` around lines 227 - 230, Move the extractor
comments from the generated es.po entry to their corresponding source strings in
DashboardView.svelte: place the fallback activity-log comment on the matching
list-field fallback string and the translation-context comment on “Total
entries.” Remove the misleading comments from the empty-state prompt and Total
entries entries in the generated PO output.
Source: Learnings
…eries, po comment fixes ProjectActivity.ChangeInfo is init-only; the resolver returns enriched record copies instead of mutating in place. RecoverDeleted fetches one latest snapshot per missing id (Take(1) descending) instead of each entity's whole snapshot history, and the deleted- component endpoint recovery filters to AddEntryComponentChange rows in SQL. prop() uses key presence so an explicit camelCase null isn't conflated with absent. Added the latest-wins dedup test for assemble-entry and made the vocab coverage test resolve paths from import.meta.url like its siblings. Removed five stale translator comments that had stuck to alphabetically-adjacent msgids (Add your first entry…/Closed/Total entries/Write a reply…/(unknown)) — the comment- preserving extraction keyed them by msgid, so they survived every re-extract since. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
|
Review absorbed in 89ef1e6. Fixed: init-only Skipped, with reasons:
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
Extracts a WsCode component and uses it in the editor's multi-writing-system field labels and the dictionary preview (which opts out of the chip chrome via plain). Chip text uses foreground/60 to clear WCAG AA on the muted fill. This gives live editor fields visual parity with the read-only diff views that later parts of the activity redesign introduce (part 2/6, assembled result reviewed in #2434). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
Field-level before/after components mirroring the editor's layout without its editable widgets: multi-string and rich-text diffs (per writing system, inline text diff via @sanity/diff-match-patch), single- and multi-selects, entry-link lists, audio diffs with screen-reader added/removed labels, and one loose-field component covering all seven vocab object types. Coverage tests parse the components' own rendered fields so a model field can't silently vanish from a diff, plus an entity-config completeness guard and a storybook gallery. No app consumer yet — the activity detail panel adopts these in part 5. Part 3/6 of the activity redesign (assembled result reviewed in #2434). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
Activity rows now say what happened — 'Apfel › apple · Set Definition (en) to …', 'Removed semantic domain 5.2 Food' — instead of naming the change type. A pure describeChange decoder turns each CRDT change into localization-free facts that ChangeSummary renders as localized sentences (bold subject, muted verb, boxed data), grouped under the entry tree they touch with an indent rail; bulk creates collapse to one counted line; rows are capped and CSS-clamped so large commits stay one scannable unit. Authors get stable per-author coloured icons across the project's author set. Coverage tests are built on the generated knownChangeTypes list: they fail until every change type has a summary (or an explicit intentionally-generic allowance) and every create type has a bulk-collapse noun, with a reverse dead-handler guard. Part 4/6 of the activity redesign (assembled result reviewed in #2434). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
Each change card's Preview now renders a real diff of the affected entity through the diff-view components (previous snapshot vs at-commit snapshot, per #2170) instead of comparing with the current version: field-level marking for edits, plain self-diff rendering with a destructive wash for deletions, resource playback for audio commits, and perspective-aware link-list synthesis for complex-form component changes. A commit that only builds one entry (or one sense with its examples) collapses to a single assembled read-only card. Card headers reuse the list's summary sentences and carry entry navigation and History actions; a change whose context fails to load shows an inline error card instead of a blank row. Part 5/6 of the activity redesign (assembled result reviewed in #2434). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
Extracts Browse's split-pane plumbing into ListDetailLayout — resizable panes on desktop, full-width list on mobile with the detail replacing it full-screen while open, list kept mounted so its scroll survives — and renders both Browse and the activity view through it. Activity gains the same navigation model as Browse: the newest commit auto-selects only on desktop; on mobile a tapped row opens the detail full-screen with a close button, driven by a detailOpen query param so the platform back button closes it. Part 6/6 of the activity redesign (assembled result reviewed in #2434). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj
|
Closing in favour of the stacked merge sequence starting at #2435 — this PR was too large to merge in one piece. Its final head ( Generated by Claude Code |
This branch is #2388 (the activity view rework — readable summaries + before/after diff, resolves #2170) plus a deep review-and-polish pass over the whole feature, with
developmerged in. Merging this supersedes #2388; nothing here changes the feature's direction.The layout question
@myieye wasn't fully convinced by the list+detail panels, and @hahn-kev showed the layout crushing the list to 8rem at narrow widths. I researched how version-history UIs handle this before touching it: tools where each item is a rich before/after of one artifact (Google Docs version history, Figma, Notion page history, VS Code Timeline) all converge on list + persistent diff pane, while list-only feeds (GitHub commits, Jira) fit events that are individually small or need their own URL. NN/g's accordion guidance argues against inline expansion for complex, comparison-heavy content, and Material 3 / Microsoft / PatternFly all say the same thing about narrow widths: don't shrink both panes, switch to single-pane at a breakpoint.
So the panel stays, and the narrow-width problem is fixed the way this app already fixes it in Browse:
ResizablePaneGroupas Browse, 35/65 default) instead of the fixedminmax(8rem,25%)grid. Newest commit auto-selects, so the page opens answering "what changed recently?".detailOpenis a query param like Browse'sentryOpen, so the platform back button closes it. Nothing auto-selects on mobile.Robustness
The "opening this row breaks the whole activity page" report is now contained twice over: the view is wrapped in the app's
ViewErrorBoundary, and each change card's{#await}has a{:catch}that renders an inline "Failed to load this change" card instead of leaving a blank 700px row (template catch, not try/catch — the global-handler rule is respected). I could not reproduce the original crash on this branch (deleting an entry and opening its row works — screenshot below), so the root cause was likely fixed during #2388's iteration; the containment is there for whatever the next bad row is. The homograph-patch summary from review now reads "wulu₁ · Set homograph number to 1".API / backend changes
HistoryService.ProjectActivityreturnsTask<ProjectActivity[]>instead ofIAsyncEnumerable— the label resolver already materializes the page and every consumer did too (flagged in Rework the activity view: readable summaries and a before/after diff #2388's own notes). Wire shape is unchanged; the JS interop signature changed with regenerated types.ChangeTypesCodeGeneratornow callsHistoryService.GetChangeTypeKeyFromType(made public) instead of duplicating theTypeNamereflection, so the generatedChangeTypeunion can't drift from the runtime$typediscriminators."sense"subject for a label-less sense; it degrades to null like every other unresolved label and the frontend omits the token (new backend test pins this).Cleanups
Deleted the storybook-only
DiffValuecomponent;DiffRichTextnow delegates toDiffMultiStringinstead of duplicating its layout;ActivityListViewOptionsrenamed toActivityViewPicker(it only picks the view — the Simple/Detailed list mode it was named for no longer exists, and the two stale references to that mode are gone); the audio diff's added/removed boxes carry screen-reader labels (colour was the only cue);changeType()is exported from change-summary instead of re-implemented in ActivityItem.Test hardening
vocab-diff-coveragenow parsesDiffVocabPrimitive's actualpresent('…')rows instead of a hand-maintained set — dropping a row now fails the test (the direction that would have caught the historical MorphType-description bug), with a reverse stale-row check.describeChangecases for the payload-picking handlers that had none (ReplacePublicationChangereadsnewPublicationwhereAddreadspublication— exactly the kind of thing that deserved a pin), translations, component links, vocab patch clears.Test evidence:
LcmCrdt.TestsHistoryServiceActivityTests 36/36 pass; viewer vitest for activity + diff-view + coverage suites 90+ tests pass;svelte-checkand eslint clean;FwLiteSharedbuilds with regenerated types committed (no drift); i18n extraction round-trips cleanly, and new msgids got translator-context comments perI18N_CONTEXT_GUIDE(30 added, 6 judged self-explanatory).E2E: ran FwLiteWeb + vite against the example project, made real edits (field edits, new sense, homograph pair, entry deletion) and exercised the activity view in both layouts with zero page errors.
Screenshots
Desktop split view (delete-commit selected, deletion wash, grouped rows with the homograph line):
Collapsed create-entry tree card:
Mobile: full-width list → tap → full-screen detail with close:
History dialog from a change card:
(Screenshots were committed at
cc47568and reverted in the next commit so they don't live on the branch.)Notes for reviewers
QueryParamStateBoolconfig snapshotsIsMobile.valueat construction (back-button semantics won't retrack if the viewport crosses the breakpoint mid-session). This faithfully mirrors Browse'sentryOpen— a pre-existing pattern now shared by two views; worth a small shared fix someday, not in this PR.IHistoryServiceJsInvokable.@sanity/diff-match-patchdependency, sentence-frame i18n tradeoff, WS-code chips app-wide).🤖 Generated with Claude Code
https://claude.ai/code/session_01Vm9mEUzcfgX4oUPAbhcfsj