add edit experiment modal#2754
Conversation
zackcl
left a comment
zackcl
left a comment
There was a problem hiding this comment.
It seems the Metrics table isn’t being cleared after changing the App Context. Also, it might be better to disable the "Include All" toggle when the App Context changes, since that’s the default cleared state.
zackcl
left a comment
zackcl
left a comment
There was a problem hiding this comment.
When we switch the app context with “Include All” enabled, the toggle flips back to “Exclude All,” but the Include Lists section stays collapsed. That feels confusing. Expanding it again would make the change clearer. I asked Copilot for a quick fix and it suggested the diff below, which tracks the previous filter mode and expands the card automatically when we transition from INCLUDE_ALL to EXCLUDE_ALL. Please take a look, and if you prefer another approach, feel free:
@@ -46,6 +46,7 @@ export class ExperimentInclusionsSectionCardComponent implements OnInit, OnDestr
experimentInclusions$ = this.experimentService.selectExperimentInclusions$;
tableRowCount$ = this.experimentService.selectExperimentInclusionsLength$;
subscriptions = new Subscription();
+ private previousFilterMode?: FILTER_MODE;
menuButtonItems: IMenuButtonItem[] = [
{
@@ -80,6 +81,22 @@ export class ExperimentInclusionsSectionCardComponent implements OnInit, OnDestr
ngOnInit() {
this.permissions$ = this.authService.userPermissions$;
+ // Expand section when include-all mode transitions back to exclude-all (e.g., after context change)
+ this.subscriptions.add(
+ this.selectedExperiment$.subscribe((experiment) => {
+ if (!experiment) {
+ this.previousFilterMode = undefined;
+ return;
+ }
+
+ const nextFilterMode = experiment.filterMode;
+ if (this.previousFilterMode === FILTER_MODE.INCLUDE_ALL && nextFilterMode === FILTER_MODE.EXCLUDE_ALL) {
+ this.isSectionCardExpanded = true;
+ }
+
+ this.previousFilterMode = nextFilterMode;
+ })
+ );
}
onAddIncludeListClick(appContext: string, experimentId: string): void {
@@ -123,15 +140,11 @@ export class ExperimentInclusionsSectionCardComponent implements OnInit, OnDestr
experiment: experiment,
filterMode: newFilterMode,
});
- this.updateSectionCardExpansion(newFilterMode);
+ // Expansion state will be updated automatically by the subscription when the experiment updates
}
// If user cancels, the toggle state is already reverted, so no action needed
}
- updateSectionCardExpansion(newFilterMode: FILTER_MODE): void {
- this.isSectionCardExpanded = newFilterMode !== FILTER_MODE.INCLUDE_ALL;
- }
-
onMenuButtonItemClick(event: string, experiment: Experiment): void {
switch (event) {
case EXPERIMENT_BUTTON_ACTION.IMPORT_INCLUDE_LIST:
@zackcl Do we want to have the exclusion and inclusion lists always expanded after editing, even if they were collapsed by the user before editing? |
Only the inclusion card needs to expand after the list gets cleared (when "Include All" was enabled), making it obvious the list is now empty. I don't think it should expand if the "Include All" was already disabled and the section card was collapsed by the user. 
|
|
This might not be related to this PR, but while I was testing updating an experiment in this branch, I've encountered an occasional crash that seems to be related to the access token validation. It looks like the frontend is sending an expired Google token, so auth eventually crashes: Please note that I made the following change to log that details: +++ b/backend/packages/Upgrade/src/auth/AuthService.ts
@@ -35,6 +35,11 @@ export class AuthService {
payload = ticket.getPayload();
request.logger.info({ message: 'ID token validated' });
} catch (error) {
+ request.logger.warn({
+ message: 'ID token validation failed, attempting access token validation',
+ error,
+ requestPath: request.originalUrl ?? request.path,
+ });
// If ID token verification fails, try to verify it as an access token
try {
// env.google.serviceAccountId is an array of service account IDs
@@ -50,7 +55,11 @@ export class AuthService {
};
request.logger.info({ message: 'Access token validated' });
} catch (error) {
- request.logger.error(error);
+ request.logger.error({
+ message: 'Token validation failed',
+ error,
+ requestPath: request.originalUrl ?? request.path,
+ });
throw error;
}
}Here's the video reproducing that error (seems to happen randomly though): Screen.Recording.2025-12-03.at.5.35.30.PM.mov |
|
|
When the Unit of Assignment is updated from "Group" to "Individual", the Group Type (stored as "group") persists in the experiment data, which might create confusion. Experiment data after updating the Unit of Assignment from Group to Individual: {
"assignmentUnit": "individual",
"group": "schoolId"
}To reproduce:
I think this happens because we are not explicitly including For reference, it seems like simply updating the fallback to @@ -536,7 +536,7 @@ export class UpsertExperimentModalComponent implements OnInit, OnDestroy {
conditionOrder: unitOfAssignment === ASSIGNMENT_UNIT.WITHIN_SUBJECTS ? conditionOrder : undefined, // Conditional validation
assignmentAlgorithm: assignmentAlgorithm || undefined, // @IsOptional
stratificationFactor: stratificationFactorObj,
- group: groupType || undefined,
+ group: groupType || null,
tags,
state: EXPERIMENT_STATE.INACTIVE,
filterMode: FILTER_MODE.EXCLUDE_ALL,
@@ -595,7 +595,7 @@ export class UpsertExperimentModalComponent implements OnInit, OnDestroy {
conditionOrder: unitOfAssignment === ASSIGNMENT_UNIT.WITHIN_SUBJECTS ? conditionOrder : undefined,
assignmentAlgorithm: assignmentAlgorithm || undefined,
stratificationFactor: stratificationFactorObj,
- group: groupType || undefined,
+ group: groupType || null,
tags, |
@zackcl yeah this was fixed in the old UI by #2645 so we can do something similar |
2 participants
No description provided.