Phoenix bugfix for undefined outputFormat

[cartermak]

See #1555

Possible fix seems to work locally for me, but it's unclear whether this is a "bug" or whether I should fix something in my adaptation.

[mattdailis]

Tracking it back to the source - $outputFormat should be coming from here - and it seems to be designed to always be a list. I suppose that implies that the issue is indeed in SequenceEditor.svelte.

aerie-ui/src/stores/sequence-adaptation.ts

Lines 44 to 47 in 210d9c8

	export const outputFormat = derived(
	[sequenceAdaptation],
	([$sequenceAdaptation]) => $sequenceAdaptation?.outputFormat ?? [],
	);

Somehow outputFormats is ending up... undefined, right? Or is it null? Could this be a phasing/initialization issue?

[cartermak]

Yes, an initialization issue is my best understanding.

It's initialized undefined:

aerie-ui/src/components/sequencing/SequenceEditor.svelte

Line 103 in b99e922

let outputFormats: IOutputFormat[];

...and it seems to be nominally set by loadSequenceAdaptation, which seems to be called when parcel.sequence_adaptation_id changes...:

aerie-ui/src/components/sequencing/SequenceEditor.svelte

Lines 113 to 115 in b99e922

	$: {
	loadSequenceAdaptation(parcel?.sequence_adaptation_id);
	}

...and my understanding is that, even though I don't define outputFormat in my adaptation, the adaptation should fall back to a default value: https://github.com/NASA-AMMOS/aerie-ui/blob/b99e9229b92ff9da1be7e37a9f5c2bf7ecbad7d4/src/stores/sequence-adaptation.ts#L68C58-L68C75

...which is SeqJson, as I'd hope:

aerie-ui/src/stores/sequence-adaptation.ts

Lines 23 to 29 in b99e922

	outputFormat: [
	{
	fileExtension: 'json',
	name: 'Seq JSON',
	toOutputFormat: sequenceToSeqJson,
	},
	],

[cartermak]

That's as far as I dug; I could keep reading through the source, but I expect a Phoenix dev would be better equipped to clarify design intent vs. current behavior.

[duranb]

That's as far as I dug; I could keep reading through the source, but I expect a Phoenix dev would be better equipped to clarify design intent vs. current behavior.

It's intended that outputFormat is always defined, even if the user doesn't provide one in their adaptation explicitly. While adding a check prior to using the length does fix it, it only fixes what's affected and not the source. I think more investigation is needed into how it's being set to undefined if all code paths prior to this is supposedly setting it to at least an empty []

[duranb]

@cartermak It looks like you're working off an older version of the codebase. Would you be able to pull in the latest and see if it's still occurring for you? (I don't think we explicitly had a fix for this)

[cartermak]

@duranb thanks for that clarification. In this case, outputFormat is expected to be defined for a sequence adaptation, but outputFormats in the SequenceEditor component isn't initialized by default until loadSequenceAdaptation is called.

Re: codebase version, I can try on develop; I found the bug trying to get Clipper set up on 3.1.0, so I branched from the tag for now.

[mattdailis]

Would initializing it to the empty list count as addressing the root cause?

-   let outputFormats: IOutputFormat[]; 
+   let outputFormats: IOutputFormat[] = []; 

[cartermak]

@duranb tested and I still see the behavior on develop. Looks like the only reason there's conflict is this change Cody made: v3.1.0...develop#diff-1329a1a0eea656e262443bf11e90d4a2988f6bb3c52b88ba6c2ca7b8fbacf2d3L229-R230

@mattdailis tried that and it works too, but I haven't scanned the code to see whether anything is expecting to find undefined instead of an empty array.

[duranb]

@duranb thanks for that clarification. In this case, outputFormat is expected to be defined for a sequence adaptation, but outputFormats in the SequenceEditor component isn't initialized by default until loadSequenceAdaptation is called.

@cartermak Ohh. Got it. Now I see it. You're correct.

Would initializing it to the empty list count as addressing the root cause?

-   let outputFormats: IOutputFormat[]; 
+   let outputFormats: IOutputFormat[] = []; 

@mattdailis That would be a good fix!

[cohansen]

Sorry I missed this whole conversation, yeah the thought was it should never be undefined if you're providing an adaptation but that still should be a valid code path.

[mattdailis]

Pair-reviewed with @dandelany