feat: improve readability of snapshots table#1664
Merged
Julusian merged 3 commits intoSofie-Automation:mainfrom Mar 2, 2026
Merged
feat: improve readability of snapshots table#1664Julusian merged 3 commits intoSofie-Automation:mainfrom
Julusian merged 3 commits intoSofie-Automation:mainfrom
Conversation
Julusian
added
the
Contribution from BBC
WalkthroughAdded a required Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Web UI"
participant API as "Server API"
participant SnapSvc as "Snapshot Service"
participant DB as "Database"
participant FS as "Filesystem"
UI->>API: Trigger snapshot creation / request
API->>SnapSvc: Build snapshot (produce name, longname, meta)
SnapSvc->>DB: Insert Snapshot document (includes longname)
SnapSvc->>FS: Write snapshot file (filename = longname)
SnapSvc-->>API: Return snapshot object (name + longname)
API-->>UI: Respond with snapshot list/item (name displayed, longname present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meteor/server/api/snapshot.ts (1)
785-789:⚠️ Potential issue | 🟡 MinorDownloaded snapshot filename uses the short
name, which may lack uniqueness.Line 787 sets the Content-Disposition attachment filename to
snapshot.snapshot.name(e.g.,"System.json"or"Debug: studioId.json"). Without a timestamp, multiple downloads could overwrite each other. Previously,nameincluded the timestamp. Consider usinglongnamefor the download filename:- ctx.response.attachment(`${snapshot.snapshot.name}.json`) + ctx.response.attachment(`${snapshot.snapshot.longname}.json`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/api/snapshot.ts` around lines 785 - 789, The download filename uses snapshot.snapshot.name which may not be unique; update the attachment call to use the longer unique identifier (snapshot.snapshot.longname) instead of snapshot.snapshot.name so returned files include the timestamp/unique part; locate the code setting ctx.response.attachment in meteor/server/api/snapshot.ts (the block that also sets ctx.response.type, ctx.response.status, and ctx.response.body) and change the filename argument to snapshot.snapshot.longname to prevent collisions.
🧹 Nitpick comments (1)
meteor/server/api/snapshot.ts (1)
228-234:getCurrentTime()called twice — potential timestamp mismatch betweencreatedandlongname.
getCurrentTime()is called on line 231 forcreatedand again on line 233 for thelongnametimestamp. These could return different values. Consider capturing it once:Suggested fix
+ const now = getCurrentTime() snapshot: { _id: snapshotId, type: SnapshotType.SYSTEM, - created: getCurrentTime(), - name: `System` + (studioId ? `_${studioId}` : ''), - longname: `System` + (studioId ? `_${studioId}` : '') + `_${formatDateTime(getCurrentTime())}`, + created: now, + name: `System` + (studioId ? `_${studioId}` : ''), + longname: `System` + (studioId ? `_${studioId}` : '') + `_${formatDateTime(now)}`, version: CURRENT_SYSTEM_VERSION, },The same pattern applies to
createDebugSnapshot(lines 288-290) andcreateRundownPlaylistSnapshot(lines 412-417).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/api/snapshot.ts` around lines 228 - 234, The code calls getCurrentTime() multiple times when building snapshot objects, causing possible mismatched timestamps; update the snapshot construction in snapshot.ts (and similarly in createDebugSnapshot and createRundownPlaylistSnapshot) to call getCurrentTime() once into a local const (e.g., const now = getCurrentTime()) and then use that variable for the created field and for formatDateTime(now) when composing longname so both fields share the exact same timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/meteor-lib/src/collections/Snapshots.ts`:
- Line 16: The Snapshot schema declares longname: string but old documents may
lack it, causing runtime undefined access (e.g., snapshot.snapshot.longname used
by fixValidPath and title={snapshot.longname}). Make longname optional in the
Snapshots type (longname?: string) and add defensive fallback logic wherever
it's used—replace direct accesses (snapshot.longname,
snapshot.snapshot.longname) with a helper/getter that returns longname || name
|| fileName before passing to fixValidPath or rendering title; alternatively
implement a one-off migration to backfill missing longname from name/fileName in
the database and update the Snapshots type accordingly.
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Around line 164-172: The anchor's title uses snapshot.longname which can be
undefined for older snapshots; update the title to fall back to snapshot.name
(e.g., title={snapshot.longname ?? snapshot.name}) so the tooltip never shows
"undefined" and still provides a useful label; modify the JSX in SnapshotsView
where the <a> element references snapshot.longname to use this fallback.
- Around line 214-225: SnapshotTypeIndicator currently renders hardcoded English
labels; update it to use the app i18n by calling useTranslation() inside
SnapshotTypeIndicator (or accept t as a prop) and replace the literal strings
"Playlist", "System", "Debug" (and the default badge text) with t(...) lookups
using appropriate translation keys (e.g. 'snapshot.type.playlist',
'snapshot.type.system', 'snapshot.type.debug' and a fallback key), ensuring
SnapshotTypeIndicator imports useTranslation from react-i18next and returns
translated badge labels for each SnapshotType case.
- Line 24: Update the inconsistent import in SnapshotsView.tsx: replace the
import of SnapshotItem and SnapshotType from
'@sofie-automation/meteor-lib/src/collections/Snapshots.js' with the matching
built artifact path under 'dist' (i.e. import from
'@sofie-automation/meteor-lib/dist/collections/Snapshots.js') so it aligns with
the other imports in this file and avoids runtime build errors.
---
Outside diff comments:
In `@meteor/server/api/snapshot.ts`:
- Around line 785-789: The download filename uses snapshot.snapshot.name which
may not be unique; update the attachment call to use the longer unique
identifier (snapshot.snapshot.longname) instead of snapshot.snapshot.name so
returned files include the timestamp/unique part; locate the code setting
ctx.response.attachment in meteor/server/api/snapshot.ts (the block that also
sets ctx.response.type, ctx.response.status, and ctx.response.body) and change
the filename argument to snapshot.snapshot.longname to prevent collisions.
---
Nitpick comments:
In `@meteor/server/api/snapshot.ts`:
- Around line 228-234: The code calls getCurrentTime() multiple times when
building snapshot objects, causing possible mismatched timestamps; update the
snapshot construction in snapshot.ts (and similarly in createDebugSnapshot and
createRundownPlaylistSnapshot) to call getCurrentTime() once into a local const
(e.g., const now = getCurrentTime()) and then use that variable for the created
field and for formatDateTime(now) when composing longname so both fields share
the exact same timestamp.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
meteor/server/__tests__/cronjobs.test.tsmeteor/server/api/__tests__/cleanup.test.tsmeteor/server/api/snapshot.tspackages/meteor-lib/src/collections/Snapshots.tspackages/webui/src/client/lib/lib.tsxpackages/webui/src/client/ui/Settings/SnapshotsView.tsxpackages/webui/src/client/ui/util/useToggleExpandHelper.tsx
Julusian
force-pushed
the
feat/snapshot-table
branch
from
3eb0dec to
36962d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
meteor/server/api/snapshot.ts (1)
232-233: Consider capturinggetCurrentTime()once per snapshot to keepcreatedand the timestamp inlongnamein sync.In each of the three snapshot factories the
createdfield and thelongnamesuffix callgetCurrentTime()independently:created: getCurrentTime(), // call 1 longname: `…_${formatDateTime(getCurrentTime())}`, // call 2While the gap is practically negligible (microseconds) and
formatDateTimelikely has second-level precision, the two values are semantically the same instant and should share one call.♻️ Proposed refactor (shown for
createSystemSnapshot; apply the same pattern tocreateDebugSnapshotandcreateRundownPlaylistSnapshot)+ const now = getCurrentTime() return { … snapshot: { _id: snapshotId, type: SnapshotType.SYSTEM, - created: getCurrentTime(), + created: now, name: `System` + (studioId ? `_${studioId}` : ''), - longname: `System` + (studioId ? `_${studioId}` : '') + `_${formatDateTime(getCurrentTime())}`, + longname: `System` + (studioId ? `_${studioId}` : '') + `_${formatDateTime(now)}`, version: CURRENT_SYSTEM_VERSION, },Also applies to: 289-290, 416-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/api/snapshot.ts` around lines 232 - 233, The created timestamp and the timestamp used in longname are each calling getCurrentTime() separately in createSystemSnapshot, createDebugSnapshot, and createRundownPlaylistSnapshot; capture getCurrentTime() once per snapshot (e.g., const now = getCurrentTime()) and use that single value for the created field and for formatDateTime(now) when building longname so both fields represent the identical instant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Around line 190-199: The code in SnapshotsView uses (snapshot.comment + '')
which yields the literal string "undefined" when comment is missing; update the
rendering to use a safe fallback such as (snapshot.comment || '') or the nullish
coalescing operator (snapshot.comment ?? '') before splitting so empty or
undefined comments produce an empty string, e.g., locate the span rendering in
the SnapshotsView component where snapshot.comment is processed and replace the
concatenation fallback with one of these safe fallbacks.
- Line 27: The import for assertNever is using the runtime-incorrect path
'@sofie-automation/corelib/src/lib.js'; update the import to pull from the
compiled package (e.g. '@sofie-automation/corelib/dist/lib' or
'@sofie-automation/corelib/dist/lib.js') so Settings/SnapshotsView.tsx imports
assertNever from the dist build like other files; locate the import line
referencing assertNever and replace the 'src/lib.js' path with the 'dist/lib'
equivalent.
---
Nitpick comments:
In `@meteor/server/api/snapshot.ts`:
- Around line 232-233: The created timestamp and the timestamp used in longname
are each calling getCurrentTime() separately in createSystemSnapshot,
createDebugSnapshot, and createRundownPlaylistSnapshot; capture getCurrentTime()
once per snapshot (e.g., const now = getCurrentTime()) and use that single value
for the created field and for formatDateTime(now) when building longname so both
fields represent the identical instant.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
meteor/server/__tests__/cronjobs.test.tsmeteor/server/api/__tests__/cleanup.test.tsmeteor/server/api/snapshot.tspackages/meteor-lib/src/collections/Snapshots.tspackages/webui/src/client/lib/lib.tsxpackages/webui/src/client/ui/Settings/SnapshotsView.tsxpackages/webui/src/client/ui/util/useToggleExpandHelper.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webui/src/client/lib/lib.tsx
- meteor/server/tests/cronjobs.test.ts
- meteor/server/api/tests/cleanup.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/webui/src/client/ui/Settings/SnapshotsView.tsx (1)
9-9: Two separate imports from the same module — consider merging.Lines 9 and 17 both import from
../../lib/lib.js. They can be combined into a single statement.♻️ Proposed consolidation
-import { fetchFrom } from '../../lib/lib.js' ... -import { hashSingleUseToken } from '../../lib/lib.js' +import { fetchFrom, hashSingleUseToken } from '../../lib/lib.js'Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx` at line 9, There are duplicate imports from ../../lib/lib.js in SnapshotsView.tsx; consolidate them into a single import statement that includes fetchFrom plus the other symbol(s) currently imported from the same module (i.e., replace the separate import lines with one combined import for fetchFrom and the other exported names used in Settings/SnapshotsView).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Around line 225-228: The default branch currently calls
assertNever(snapshotType) which throws at runtime and makes the subsequent
return unreachable; to make renders resilient to unknown persisted snapshot
types, flip the order in the switch's default case so you first render a safe
fallback (e.g., <span className="badge bg-primary">{snapshotType ??
'unknown'}</span>) and then call assertNever(snapshotType) for the compile-time
exhaustiveness check, keeping the assertNever(snapshotType) call present (it
should have signature (x: never) => never) so TypeScript still enforces
exhaustiveness while the UI gracefully handles unexpected values in the render
path.
---
Nitpick comments:
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Line 9: There are duplicate imports from ../../lib/lib.js in
SnapshotsView.tsx; consolidate them into a single import statement that includes
fetchFrom plus the other symbol(s) currently imported from the same module
(i.e., replace the separate import lines with one combined import for fetchFrom
and the other exported names used in Settings/SnapshotsView).
tsorbo
reviewed
Feb 26, 2026
Contributor
tsorbo
left a comment
tsorbo
left a comment
There was a problem hiding this comment.
This is not a code review, but on behalf of NRK I think this looks like a good improvement for the snapshots overview.
jstarpl
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the BBC
Type of Contribution
This is a: Feature
Current Behaviour
New Behavior
The snapshots table is tidied up a bit to make it easier to read:
The visible name of snapshots has been shorted to not be the full filename. Any existing snapshots will not be updated, so it wont immediately look this clean in all installations.
As this is not a user facing change (only visible to those who configure sofie), this has not been run through any UX work.
Testing
Affected areas
Time Frame
Other Information
Status
Implementation Details
Data Model Enhancement
longname: stringfield to theSnapshotBaseinterface (and thus inherited bySnapshotSystem,SnapshotDebug,SnapshotRundownPlaylist) to preserve a detailed internal identifier for each snapshot.Snapshot Name Restructuring
name(e.g. "System"),longnameencodes system/studioId and timestamp (e.g. "System_studioId_timestamp").namelike "Debug: ",longnamepreserves the previous timestamped pattern.name= playlist name,longname= "Rundown_<playlist.name>_<playlist.id>".File Storage & Persistence
snapshot.longnamefor storage; the shorternameis retained for UI display.longnamefield.UI Redesign
MomentFromNow).SnapshotRowItemcomponent for per-row rendering andSnapshotTypeIndicatorto show color-coded type badges.longnameis exposed via tooltip on the name link.isExpanded) introduced, replacing the prior globaleditSnapshotId.Other Code Changes
multilineTextutility export/implementation frompackages/webui/src/client/lib/lib.tsxas it became unused.useToggleExpandHelper.tsx:ToggleSetExpandedandToggleIsExpanded, and updated the hook's return typing to use them.Tests
meteor/server/__tests__/cronjobs.test.ts,meteor/server/api/__tests__/cleanup.test.ts) to include the newlongnamefield in inserted snapshot test data.Public API / Type Changes
longname: stringin addition toname. No other exported API signatures were removed.