feat: project level themes #8344
Conversation
|
@ericpgreen2 slight change to our Viz for AI. We will pick a theme of it has been defined, you think that's too aggressive? |
I'd advocate for consistency. Since Explore dashboards require a theme be set explicitly, I'd expect the same of inline AI visualizations. I'd expect: # rill.yaml
ai_theme: my_theme
// or refactored to be nested under a top-level "ai" key
ai:
theme: my_theme
connector: my_openai
instructions: ... |
@ericpgreen2 @djbarnwal lets go with |
mindspank
changed the title
| map<string, string> variables = 7; | ||
| map<string, string> annotations = 9; | ||
| string frontend_url = 18; | ||
| string theme = 19; |
There was a problem hiding this comment.
Can you remove the option here? Since the field gets populated from rill.yaml, it should not be possible to pass it over the APIs CreateInstance or EditInstance (because the YAML parser runs continuously and would quickly overwrite anything passed through the API)
| map<string, string> variables = 15; | ||
| map<string, string> annotations = 10; | ||
| optional string frontend_url = 19; | ||
| optional string theme = 20; |
There was a problem hiding this comment.
Same comment as above
| // Set an error without returning to mark if there are parse errors (if not, force error to nil in case there previously were parse errors) | ||
| var parseErrsErr error | ||
| if len(parser.Errors) > 0 { | ||
| parseErrsErr = ErrParserHasParseErrors | ||
| } | ||
| err = r.C.UpdateError(ctx, self.Meta.Name, parseErrsErr) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't think moving this is safe – it will prevent parse errors from showing up in Rill Developer
| // Check if theme referenced theme changed | ||
| themeChanged := false | ||
| if parser.RillYAML.Theme != "" && diff != nil { | ||
| referencedTheme := parserpkg.ResourceName{Kind: parserpkg.ResourceKindTheme, Name: parser.RillYAML.Theme} | ||
| for _, n := range diff.Added { | ||
| if n.Normalized() == referencedTheme.Normalized() { | ||
| themeChanged = true | ||
| break | ||
| } | ||
| } | ||
| if !themeChanged { | ||
| for _, n := range diff.Modified { | ||
| if n.Normalized() == referencedTheme.Normalized() { | ||
| themeChanged = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
| if !themeChanged { | ||
| for _, n := range diff.Deleted { | ||
| if n.Normalized() == referencedTheme.Normalized() { | ||
| themeChanged = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is needed? It does a full re-parse of the project when rill.yaml is changed, so I don't think any further changes here should be necessary (unless I'm missing something?)
| inst.ProjectAIConnector = rillYAML.AIConnector | ||
|
|
||
| // Validate theme references an existing theme resource | ||
| if rillYAML.Theme != "" { | ||
| // Check if theme exists in parsed resources using normalized name (case-insensitive) | ||
| themeName := parser.ResourceName{Kind: parser.ResourceKindTheme, Name: rillYAML.Theme} | ||
| if _, ok := p.Resources[themeName.Normalized()]; !ok { | ||
| return fmt.Errorf("theme references theme %q which does not exist", rillYAML.Theme) | ||
| } | ||
| } | ||
|
|
||
| inst.Theme = rillYAML.Theme |
There was a problem hiding this comment.
It should just assign the expressed theme without doing validation. This is not a safe place to do validation – validation has to be done when parsing or when reconciling/consuming the value.
It'll be difficult to validate this at parse time because rill.yaml is parsed before any other file is parsed, so it won't know if the theme exists or not. Due to unpredictable parse order, we generally don't validate references in the parser – we do that in reconciles. But for AI themes, there is no resource to reconcile.
So I suggest you validate this when consuming the setting in the AI chat UI. I.e. show an error if the theme doesn't exist.
Adds project wide theme.
themein rill.yamlCloses https://linear.app/rilldata/issue/ENG-960/add-theme-support-for-ai-generated-charts
Checklist: