@igor.drozdov Excuse the slowness on this one. I've been swamped with reviews 😅 I will take a look tomorrow 🙏🏻

@lindsey-shelton It has been a crazy day for me excuse me if I cant fully commit to this today. I checked a bit and I think this isnt the way to go here. The refetch is really a weird workaround the cache and I am afraid it will not properly reflect the component's behaviour.

this util should look more like

  const setPanelTitleAndSubtitle = async (title, subtitle) => {
    // Necessary for local resolvers to be activated
    await mockApollo.defaultClient.cache.writeQuery({
      query: panelStateQuery,
      data: { panelTitle: title, panelSubtitle: subtitle, isMaximized: false },
    });
  };

This currently does not work, but I will circle back to it tomorrow 🙏🏻

Although I understand we want to figure out the best prompts, I would strongly push us to act with iteration in mind and commit so some guidelines any guidelines even if we say, 10 lines for each file type max. I have left some thoughts in slack, but the short version here is:

1. We've been discussing some form of other of AGENTS.md for over a year (yes, that long) and the problem is becoming exponentially bad
2. As reviewers, we have to keep repeating ourselves to Agents, which do not learn. There only way to remember right now is something akin to Agents.md, and so we should urgently fix this an reduce the increasing burden on maintainers.
3. What does this need to move forward? There is lot of idea, but no clear DRI.

There are certain guidelines that have NO downside. For example, in frontend, explaining how to structure a vue component will apply to any AI generated MR. And given that AI output is a starting point, in the off chance that whatever it generated is off, if its less often than it's right, it's already a clear win over what we have now. Maybe the Frontend needs this more (I can't speak for Backend) but the state of reviewing right now is repeating the same 5 instructions to engineers, who then go back, send this to their Agent, and circle back. It was not this bad 6 months ago. I don't think the local solution will fix this quick enough.

A mix of both approach is waht I'd recommend:

1. Get a few lines in the Agents.md. The things we notice are the worst offenders in review. We can scope this to 30 lines total, anything really.
2. Deploy. Check if the output is better
3. Adjust

Then anything over the limit we've set for ourselves, we can test with a local.md. There is some uyrgency here otherwise what will happen is that reviewer are going to start putting the 1️⃣ signal and slowing down, effectively nullifying any speed boost we might get out of AI output.

@kushalpandya Seems we have a few failures still here! Let me know when its green!

@michaelangeloio I've been reviewing the MR and I have a bunch of suggestions and restructure for the Vue components and some of them may be repetitives. I haven't deleted them because I think they illustrate a lot of examples, but I think a larger summary is required for this to then become applicable to this MR and future frontend ones for Orbit.

1. Scoped styles is rarely, if ever, the way we built Vue components. There's tons of reasons for this, but the short version is we want to stay aligned with Pajamas, so we really try to use only tailwind. Sometimes rarely now, we will go with a SCSS file and some customs styles. Scoped styled was only ever used to override underlying libraries stylings that we have no control over.
2. The general way in Vue to do data transformation an operations. The goal is always to have as much declarative operations as possible, so when we do an operation like network data fetch, we then want to leverage computed for processing the data to different shapes and watchers for side effects. Leaner methods. This has appeared a few times in the MR so looking at some of my comments should help clarify what I mean.
3. Only use computed properties for reactivity. Constants/hardcoded values should go in $options
4. Data processing on the client side is much safer when we don't hard return out of conditions. It may be required in a few places, but overall preferring (myData || []).map over if(!myData || myData.length === 0) return []
5. data transformation rarely should use forEach.If it's truly "for each" value then .map is better and if we filter then either .map + filter or reduce
6. There's a a lot of data parsing utils. Components weren't too bad, but a reviewer cant realistically get through and fully appreciate all of this data parsing I believe. Can't we really split this up? I get we need all of it for it to work, but currently this is mostly a blackbox of data piping.

I stopped in the JS utils file for now, I haven't been able to get through everything after a few hours of checking. I am abstaining from the specs because the comments I left will reshape the specs significantly as well.

I know I have a lot of comments so far, but I do believe they are required for the p0 of this. It's significantly bigger than the average MR and therefore the review time should be proportional imo, even if its behind a disabled FF. That is to say that I want to confirm that this has to get in this week. If it does, I will do my best to support the reviews here, but I think having this in smaller incremental chunks would have helped a lot.

Back to you !

Not a computed! To Options we go!

Similar here to other comments, w use the default value twice so we could probably drop the second one?

Concern Do we really want to maintain all of this data processing? This one may be all on me so please let me know about it, but I can barely reason about all of this 😅 At the minimum, we should JSDOC each function with a rational of how we parse the data and where in the data pipeline it sits and why. WDYT?

Naive question: couldn't this have been its own MR? downloading the CSV and the introduction of a different library, all contained in a single button would have been nice to discuss separately.

Suggestion lots of duplication in the template, maybe more data driven here with:

computed: {
  tableColumns() {
    return [
      { key: 'name', label: s__('Orbit|Name'), class: 'gl-flex-1' },
      { key: 'data_type', label: s__('Orbit|Type'), class: 'schema-detail-col-type' },
      { 
        key: 'nullable', 
        label: s__('Orbit|Nullable'), 
        class: 'schema-detail-col-nullable',
        format: (val) => val ? 'Yes' : 'No'
      },
    ];
  },
},

<template v-if="description">
  <p class="gl-mb-3 gl-mt-0 gl-text-sm gl-text-subtle">
    {{ description }}
  </p>
</template>

<template v-if="properties.length">
  <p class="gl-mb-2 gl-mt-0 gl-text-sm gl-font-bold">
    {{ s__('Orbit|Properties') }}:
  </p>

  <div class="schema-detail-header gl-flex gl-py-1">
    <span
      v-for="col in tableColumns"
      :key="col.key"
      :class="[col.class, 'gl-text-xs gl-font-bold gl-text-subtle']"
    >
      {{ col.label }}
    </span>
  </div>

  <div
    v-for="prop in properties"
    :key="prop.name"
    class="schema-detail-row gl-flex gl-items-center gl-py-1"
  >
    <span
      v-for="col in tableColumns"
      :key="col.key"
      :class="[col.class, 'gl-text-xs', { 'gl-text-subtle': col.key !== 'name' }]"
    >
      {{ col.format ? col.format(prop[col.key]) : prop[col.key] }}
    </span>
  </div>
</template>

Suggestion we can make this more functional and data driven

const fields = [
  { key: s__('Orbit|Type'), value: this.node.type },
  { key: s__('Orbit|FQN'), value: this.node.fqn },
  { key: s__('Orbit|Location'), value: this.node.location },
  { key: s__('Orbit|Connections'), value: String(this.node.connections?.size || 0) },
];
return fields.filter((field) => field.value);

So this has come up a few times throughout the MR here, but we often just want to use sane defaults as value and let the functional methods like map, reduce, filter to operate either on the whole data or the empty array instead of doing the early returns. We save a lot of lines and a lot of handling in the tends because we have to assume we want to get the same data types for both forks, where the early return could potentially differ from the data branch

Issue the display none can be done with the tailwind utils an the harcoded pixel values should be possible too.

Everything in this scoped block should technically all be possible in tailwind too.

The min height can move to a tailwind css class here

Similar to other places here, what if we dont have a match? Why and how do we handle it here? 🤔

Issue I get that this is REST here behind the hood, but I'd love if all we did here was fetch the data. To follow the declarative nature of Vue:

1. Methods operate with specific arguments
2. Computed derive other data from what we've fetched
3. Watchers act after the arrival of the new data

This has come around a few times in the MR and I don't want to comment on each one so I am marking this one as issue and it's larger, AKA we need to make sure that even if we use REST, we map the data flow properly so that we can more easily track the UI output.

Question seems odd that a listener might not have a defined event? shouldn't node be always defined?

Suggestion we can make this easier to reason with

filterGraph(query) {
  const q = query?.trim();
  if (!q || q.length < 2) return;
  
  const [firstMatch] = this.$refs.graphCanvas?.searchNodes(q) || [];
  if (firstMatch) this.selectedNode = firstMatch;
},

Thought usually side effects like this are really great as watchers. When banner dismissed changes to true, we set the cookie in the watcher so if we ever manually set this property without calling this method, still works.