Skip to content

Fix TypeError in cylc message mutation form#2302

Merged
wxtim merged 2 commits into
cylc:masterfrom
MetRonnie:list-null
Feb 2, 2026
Merged

Fix TypeError in cylc message mutation form#2302
wxtim merged 2 commits into
cylc:masterfrom
MetRonnie:list-null

Conversation

@MetRonnie
Copy link
Copy Markdown
Member

@MetRonnie MetRonnie commented Sep 8, 2025

Closes #2410

Bug

image

Results in nothing happening and this error:

TypeError: Cannot read properties of null (reading 'length')

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry not included as minor
  • Docs not needed

@MetRonnie MetRonnie added this to the 2.10.0 milestone Sep 8, 2025
@MetRonnie MetRonnie self-assigned this Sep 8, 2025
@MetRonnie MetRonnie added bug Something isn't working small labels Sep 8, 2025
Comment on lines +87 to +91
this.modelValue.unshift(newInput)
this.model.unshift(newInput)
Copy link
Copy Markdown
Member Author

@MetRonnie MetRonnie Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixins did not make it fun to figure out where modelValue is coming from, but it's here:

props: {
// the GraphQL type this input represents
gqlType: {
type: Object,
required: true
},
// array of all GraphQL types in the schema
types: {
type: Array,
default: () => []
},
// the value (v-model is actually syntactic sugar for this)
modelValue: {
required: true
}

We shouldn't be mutating the prop, instead we should be using the model getter/setter

@MetRonnie

This comment was marked as resolved.

@MetRonnie MetRonnie marked this pull request as draft January 14, 2026 14:59
@MetRonnie MetRonnie marked this pull request as ready for review January 28, 2026 16:45
Copy link
Copy Markdown
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but got some odd behaviour:

Image

@MetRonnie
Copy link
Copy Markdown
Member Author

MetRonnie commented Jan 29, 2026

That's expected, the format of messages is [[severity, message], ...] (see the help text), which admittedly is a little awkward, but would need a schema change to address

@oliver-sanders
Copy link
Copy Markdown
Member

Ach right, that makes sense.

Cool, truth be told, we don't really want to expose the cylc message API via the GUI (accidental side-effect of aotf), so a clunky interface is no biggie (we may want to consider redacting the message command from aotf).

This is a genuine fix to the form generator fixing a bug which could occur in any other mutation, so well worth doing irrespective.

@oliver-sanders oliver-sanders requested a review from wxtim January 30, 2026 10:46
@oliver-sanders
Copy link
Copy Markdown
Member

@wxtim, could you give this a quick once-over.

This PR is basically a composition API refactor (you can actually test the bugfix part with model || [] in the original code), should be no functionality change, other than the bugfix of course.

Don't worry about the confusing message command form (see above).

@MetRonnie
Copy link
Copy Markdown
Member Author

An advantage of the composition API is (since Vue 3.5) it is much easier to define v-models on components. This makes it easier to avoid mutating props, which is flagged up in newer versions of eslint-plugin-vue (#2142 (though I don't think the plugin can tell when a prop is added by a mixin))

import { formElementProps, useFormElement } from '@/components/graphqlFormGenerator/mixins'
import { RULES } from '@/components/graphqlFormGenerator/components/vuetify'

/**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it ought to be in a general utilities module?

@wxtim wxtim merged commit ff316dd into cylc:master Feb 2, 2026
10 of 12 checks passed
@MetRonnie MetRonnie deleted the list-null branch February 2, 2026 11:00
@MetRonnie MetRonnie modified the milestones: 2.x, 2.13.0 Feb 2, 2026
@MetRonnie MetRonnie mentioned this pull request Feb 4, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

commands: cannot add messages to the message command

3 participants