Overview

Trying to follow the logic of \Drupal\canvas_ai\Controller\CanvasBuilder::render() it is not clear why this particular set of tools is special cased

      $map = [
        EditComponentJs::class => ['js_structure', 'props_metadata'],
        CreateComponent::class => ['component_structure'],
        CreateFieldContent:: class => ['created_content'],
        EditFieldContent:: class => ['refined_text'],
        AddMetadata::class => ['metadata'],
        SetAIGeneratedComponentStructure::class => ['operations'],
      ];
      if (!empty($tools)) {
        foreach ($tools as $tool) {
          foreach ($map as $class => $keys) {

In #3537422: XB AI: Validate AI generated Components before adding to XB we already had to put this comment in all of the classes listed above

// \Drupal\xb_ai\Controller\XbBuilder::render() expects a YAML parsable
    // string.
    // @see \Drupal\xb_ai\Controller\XbBuilder::render()

But inside CanvasBuilder::render() it is not clear what these classes are for and what strings that will be set to $keys are.

Looking through the classes it seems the $keys will but sub-set of the tools output if the tool does not have any errors

We should make this clearer by making an interface and not having to hardcode these class names

Proposed resolution

  1. Create an new BuilderResponseFunctionCallInterface with getSuccessResponseData
  2. Have all the classes currently hardcoded implement this interface

The names can be updated. Need feedback by those that know why this particular classes are hardcoded

User interface changes

Issue fork canvas-3555464

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Title: CanvasBuilder::render() logic around tool specific tools response is unclear » CanvasBuilder::render() logic around specific tool response is unclear
narendrar’s picture

The changes look good to me. My only suggestion to help simplify the remaining tools is to use setOutput instead of getReadableOutput.
Also instead of doing this can we use functionality added in https://www.drupal.org/project/ai/issues/3529313

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Active » Needs review
narendrar’s picture

Changes looks good to me.
Some small changes to do and may be we could update the Proposed resolution in IS.

akhil babu’s picture

Verified page builder functionality manually with these changes and did not find any issues. +1 RTBC

narendrar’s picture

Status: Needs review » Reviewed & tested by the community

Changes looks good to me. Follow-up needs to be created for removing field_name. This can also be done as part of #3555407: AI Canvas, dead code, nits fixes

  • tedbow committed c78a54e9 on 1.x
    [#3555464] feat(AI): CanvasBuilder::render() logic around specific tool...
tedbow’s picture

narendrar’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update, -Needs follow-up

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.