Add --all flag to preview list command#3098
Conversation
📊 Performance Test ResultsComparing 7a846f1 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
I think we should consider excluding expired preview sites without an associated local sites. They are basically guaranteed to be deleted on the server, and they risk cluttering the output of this command.
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Nice and clean! 👍 I left a couple of comments that'd be nice to address, but they're all minor.
| for ( const snapshot of snapshots ) { | ||
| const durationUntilExpiry = formatDurationUntilExpiry( snapshot.date ); | ||
| const url = `https://${ snapshot.url }`; | ||
|
|
||
| table.push( [ | ||
| { href: url, content: url }, | ||
| snapshot.name, | ||
| format( snapshot.date, 'yyyy-MM-dd HH:mm' ), | ||
| durationUntilExpiry, | ||
| ] ); | ||
| const siteName = siteNameById.get( snapshot.localSiteId ) ?? unknownSiteLabel; | ||
| const bucket = snapshotsBySiteName.get( siteName ) ?? []; | ||
| bucket.push( snapshot ); | ||
| snapshotsBySiteName.set( siteName, bucket ); | ||
| } |
There was a problem hiding this comment.
Studio doesn't enforce local site name uniqueness. I think it would be smarter to group the preview sites by local site ID instead of by name.
| if ( all && siteNameById ) { | ||
| const unknownSiteLabel = __( 'Unknown site' ); |
There was a problem hiding this comment.
| if ( all && siteNameById ) { | |
| const unknownSiteLabel = __( 'Unknown site' ); | |
| if ( all ) { | |
| const config = await readCliConfig(); | |
| const siteNameById = new Map< string, string >( | |
| config.sites.map( ( site ) => [ site.id, site.name ] ) | |
| ); | |
| const unknownSiteLabel = __( 'Unknown site' ); |
I don't really see a good reason to construct siteNameById outside this if statement. If there are no local sites, then it'd be better to group all existing snapshots under Unknown site
| // Expired orphans are almost certainly already deleted server-side — clean them up. | ||
| if ( all ) { | ||
| await pruneExpiredOrphanedSnapshots( token.id, isSnapshotExpired ); | ||
| } |
There was a problem hiding this comment.
| // Expired orphans are almost certainly already deleted server-side — clean them up. | |
| if ( all ) { | |
| await pruneExpiredOrphanedSnapshots( token.id, isSnapshotExpired ); | |
| } | |
| // Expired orphans are almost certainly already deleted server-side — clean them up. | |
| await pruneExpiredOrphanedSnapshots( token.id, isSnapshotExpired ); |
Not a big deal either way, but we could do this regardless of whether the --all option was passed or not.
| choices: [ 'table', 'json' ], | ||
| default: 'table', | ||
| description: __( 'Output format' ), | ||
| } ) | ||
| .option( 'all', { | ||
| type: 'boolean', | ||
| default: false, | ||
| description: __( 'List preview sites for all local sites, grouped by site' ), | ||
| } ); | ||
| }, | ||
| handler: async ( argv ) => { | ||
| await runCommand( argv.path, argv.format as 'table' | 'json' ); | ||
| await runCommand( argv.path, argv.format as 'table' | 'json', Boolean( argv.all ) ); |
There was a problem hiding this comment.
choices: [ 'table', 'json' ] as const,
default: 'table' as const,
description: __( 'Output format' ),
} )
.option( 'all', {
type: 'boolean',
default: false,
description: __( 'List preview sites for all local sites, grouped by site' ),
} );
},
handler: async ( argv ) => {
await runCommand( argv.path, argv.format, argv.all );Two nits:
- By adding const assertions to the
choicesanddefaultconfig params, TS can correctly infer theargv.formattype, and we can remove the type assertion. argv.allis already a boolean, so there's no need to cast it.
3 participants
Related issues
How AI was used in this PR
Claude Code (Sonnet 4.5) helped scaffold the implementation and tests based on the existing
studio preview listcommand structure. I reviewed each change, iterated on several design decisions (flag vs. subcommand, grouping strategy, sort order, orphan coalescing), and ran the full lint/typecheck/test loop after each revision.Proposed Changes
--allflag tostudio preview listthat lists preview sites across all local Studio sites, grouped by siteUnknown sitegroupbuildSnapshotTable()helper so the single-site and--alltable rendering stay consistent--allpath (happy path, grouped headers, sort order, orphan fallback, orphan coalescing, expired badge, empty state) + 1 regression test for the single-site pathNote: The JSON output path (
--format=json) is unchanged by this PR — it already dumps the fullconfig.snapshotsarray regardless of the current site, so--allhas no effect there. Keeping that behavior consistent with the existing command; happy to tighten it up in a follow-up if desired.Testing Instructions
Setup: Have at least 2 local Studio sites with one or more preview sites each (use
studio preview createin each site folder to generate some).Happy path
npm run cli:buildnode apps/cli/dist/cli/main.mjs preview list --allMy Site (3 preview sites)Regression — existing single-site behavior
cdinto a Studio site foldernode apps/cli/dist/cli/main.mjs preview listEdge cases
preview list --allwith a user that has zero preview sites → friendlyNo preview sites foundmessage--all: runpreview listfrom~/Desktopor similar → clear error that the directory isn't added to Studio (unchanged behavior)Found N preview sites (M expired)in the--alloutputstudio auth logoutthenpreview list --all→Authentication requirederror (unchanged behavior)Automated
npm test -- apps/cli/commands/preview/tests/list.test.ts— 11 tests should passnpx eslint apps/cli/commands/preview/list.ts apps/cli/commands/preview/tests/list.test.ts— cleanPre-merge Checklist