Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “Export State Change Logs” action to the experiment details menu and centralizes CSV downloading into a shared export helper, enabling CSV exports from multiple UI surfaces.
Changes:
- Added a new experiment details menu item + action for exporting state change logs.
- Introduced
downloadValuesAsCSVinCommonExportHelpersServiceand reused it in the private segment list modal. - Added the English i18n string for the new menu item.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/frontend/projects/upgrade/src/assets/i18n/en.json |
Adds translation for the new export menu item label. |
packages/frontend/projects/upgrade/src/app/shared/services/common-export-helpers.service.ts |
Adds a shared CSV download helper using Blob + object URLs. |
packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts |
Refactors CSV export to use the shared export helper service. |
packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-overview-details-section-card/experiment-overview-details-section-card.component.ts |
Adds the new menu item/action and implements CSV export of experiment state change logs. |
packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts |
Extends EXPERIMENT_DETAILS_PAGE_ACTIONS with EXPORT_STATE_CHANGE_LOGS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...periment-overview-details-section-card/experiment-overview-details-section-card.component.ts
Show resolved
Hide resolved
...periment-overview-details-section-card/experiment-overview-details-section-card.component.ts
Outdated
Show resolved
Hide resolved
...periment-overview-details-section-card/experiment-overview-details-section-card.component.ts
Show resolved
Hide resolved
...periment-overview-details-section-card/experiment-overview-details-section-card.component.ts
Outdated
Show resolved
Hide resolved
...periment-overview-details-section-card/experiment-overview-details-section-card.component.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| downloadStateChangeLogs(name: string, stateTimeLogs: ExperimentStateTimeLog[]) { | ||
| const sortedLogs = [...stateTimeLogs].sort((a, b) => new Date(a.timeLog).getTime() - new Date(b.timeLog).getTime()); |
There was a problem hiding this comment.
do we need to sort the logs? would they ever really arrive out of order?
There was a problem hiding this comment.
yess we need to sort them because timelog is not a deafult sorting field. they showed up out of order a couple times while i was testing
| this.commonExportHelpersService.downloadValuesAsCSV(this.objectsToCsvRows(values), fileName); | ||
| } | ||
|
|
||
| private objectsToCsvRows(values: object[]): string[] { |
There was a problem hiding this comment.
object is a type to avoid, like any, it doesn't work as expected... any time any ol' unknown "object" is valid Record<string, unknown> is what I've heard is the best typesafe thing.
however... we do know what the type is here, it'sExperimentStateTimeLog[], this isn't any ol' unknown object, this is doing a lot of work that IMO can be safely simplified.
I think something like this essentially does the same thing? The one difference is that it would allow unexpected values, safely wrapped in quotes to make them strings. IMO that would be preferable to mysterious blank cells via coercing to "", plus it's really clear what's going on.
downloadStateChangeLogs(name: string, stateTimeLogs: ExperimentStateTimeLog[]) {
if (!stateTimeLogs.length) return; // could throw an explicit error, this would be weird?
const headerRow = 'timeLog,fromState,toState';
const sortedLogs = [...stateTimeLogs].sort((a, b) => new Date(a.timeLog).getTime() - new Date(b.timeLog).getTime()); // if sorting is really needed, if not this gets even simpler.
const dataRows = sortedLogs.map(({ timeLog, fromState, toState }) => {
const formattedTime = new Date(timeLog).toLocaleString();
return `"${formattedTime}","${fromState}","${toState}"`;
});
const csvRows = [headerRow, ...dataRows];
this.commonExportHelpersService.downloadValuesAsCSV(
csvRows,
`${name}_state_change_logs_${new Date().toISOString()}`
);
}There was a problem hiding this comment.
I was going to change that to a Record<> type. Missed it. Good catch. Will do now
There was a problem hiding this comment.
Also: a) I wanted to keep the objectsToCsvRows generic so that it could potentially handle any object, and so it's not brittle with respect to possible changes in the log format (or if we want to add other log fields to the download); and b) I think it makes sense to separate the method that performs the conversion from the one that does the downloading.
| this.openConfirmEmailDataModal(experiment.id, experiment.name); | ||
| break; | ||
| case EXPERIMENT_DETAILS_PAGE_ACTIONS.EXPORT_STATE_CHANGE_LOGS: | ||
| this.downloadStateChangeLogs(experiment.name, experiment.stateTimeLogs || []); |
There was a problem hiding this comment.
for what it's worth, downloading the design or emailing data menu functions open a confirm modal. why? i don't know why, but it's what every other menu item in the new design does, they all at least open an are-you-sure, so for consistency we may want to do that here also.
There was a problem hiding this comment.
@amurphy-cl confirmed in the meeting yesterday that we don't want a modal
No description provided.