mengw15 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117027006
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -718,7 +774,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings ||
this.getDefaultSettings();
+ this.hydrateSettings(workflowContent.settings);
Review Comment:
The blank path does not get this treatment: `reloadWorkflow(undefined)`
calls `hydrateFormBinding(undefined)` -- with the comment that a leftover must
not leak into the next save -- but nothing calls `hydrateSettings(undefined)`.
Settings go through `clearWorkflow`'s `setWorkflowSettings(undefined)` instead,
which lands somewhere else:
```
clearWorkflow():
destroySharedModel(); // doc destroyed; the contentMeta
observer is still on its map
setWorkflowMetadata(undefined);
setWorkflowSettings(undefined); // isEqual reads the destroyed map, and
writes when it differs
reloadWorkflow(undefined); // setReloadingWorkflow(true) only
happens here
```
So for a workflow whose settings are not the defaults, this writes
`set("settings", defaults)` into the just-destroyed document, and because the
reloading flag is not set yet the observer announces it on
`workflowSettingsChanged$` -- `clearWorkflow` reports a workflow edit where it
reported none before. Calling `hydrateSettings(undefined)` in the blank branch
beside `hydrateFormBinding(undefined)` would put the settings seed on the same
path as the form one, under the flag.
(The pending-seed half of this is @copilot's point above on the `undefined`
seed having no identity; not repeating it here.)
~~~
~~~comment
`has` reads only the shared map, while the value beside it comes from
`readContentMeta`, which falls back to the copy a seed is still holding. So
during the wait for the first sync the two disagree. For a workflow whose
stored binding is present but empty (`{fields: []}` -- an author who added
fields and then removed them all), `has` is false and `isFormBindingNonEmpty`
is false, so an autosave in that window writes content with no `formBinding` at
all and drops the stored one. The `formBindingLoaded` flag this replaces was
true for that case.
The consequence is confined to content fidelity -- `getFormBinding()`
answers `{fields: []}` either way -- but it is the one thing "an autosave in
that window carries it instead of an empty one" does not cover. A
`hasContentMeta(key)` mirroring `readContentMeta` would close it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -865,11 +1002,12 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or
an author
- // populated it), so a plain workflow's content is unchanged and its
save cuts no
- // needless version.
- ...(this.formBindingLoaded ||
this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or
an author
+ // populated it since), so a plain workflow's content is unchanged and
its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate
sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") ||
this.isFormBindingNonEmpty(formBinding)
Review Comment:
`has` reads only the shared map, while the value beside it comes from
`readContentMeta`, which falls back to the copy a seed is still holding. So
during the wait for the first sync the two disagree. For a workflow whose
stored binding is present but empty (`{fields: []}` -- an author who added
fields and then removed them all), `has` is false and `isFormBindingNonEmpty`
is false, so an autosave in that window writes content with no `formBinding` at
all and drops the stored one. The `formBindingLoaded` flag this replaces was
true for that case.
The consequence is confined to content fidelity -- `getFormBinding()`
answers `{fields: []}` either way -- but it is the one thing "an autosave in
that window carries it instead of an empty one" does not cover. A
`hasContentMeta(key)` mirroring `readContentMeta` would close it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
