vuetify icon indicating copy to clipboard operation
vuetify copied to clipboard

fix(nested): Prevent infinite loops when resolving path

Open J-Sek opened this issue 1 year ago • 2 comments

Description

While loops used by components with nesting need some safety check to prevent freezing end-users tab. Performance overhead of the checks is close to none, while it makes a big difference in the field.

Error does not necessarily need to be developers fault. Data from the API may contain duplicates or it may be a result of some drag & copy functionality built on top of Vuetify's components.

I was advised to log a warning, but I just don't think it is better than throwing error

  • warning inside while loop does not help – tabs usually freeze DevTools as well and devs might work with simplified data, leaving potential for errors in the field
  • warning after validating path/parents collection – unnecessary performance overhead if we can have cheaper solutions
  • warning + breaking from the loop – risky... potentially misleading end-users if they click things fast and don't pay attention, and won't get logged in monitoring (like Sentry or Bugsnag)

mitigates #20389

Markup:

<template>
  <v-app theme="dark">
    <v-container>
      <h4>Tree view</h4>
      <v-treeview :items="items" item-value="title" open-on-click />
      <v-divider class="my-6" />
      <h4>List</h4>
      <v-list v-model:opened="opened" v-model:selected="selected" select-strategy="classic">
        <v-list-group value="src-group">
          <template #activator="{ props }">
            <v-list-item v-bind="props" title="src">
              <template #prepend="{ isSelected }">
                <v-list-item-action start>
                  <v-checkbox-btn :model-value="isSelected" />
                </v-list-item-action>
              </template>
            </v-list-item>
          </template>
          <v-list-group value="renderer-group">
            <template #activator="{ props }">
              <v-list-item v-bind="props" title="renderer">
                <template #prepend="{ isSelected }">
                  <v-list-item-action start>
                    <v-checkbox-btn :model-value="isSelected" />
                  </v-list-item-action>
                </template>
              </v-list-item>
            </template>
            <v-list-group value="src-group">
              <template #activator="{ props }">
                <v-list-item v-bind="props">
                  <template #title>src ––– <span class="text-red">toggle selection here on one level below</span></template>
                  <template #prepend="{ isSelected }">
                    <v-list-item-action start>
                      <v-checkbox-btn :model-value="isSelected" />
                    </v-list-item-action>
                  </template>
                </v-list-item>
              </template>
              <v-list-group value="assets-group">
                <template #activator="{ props }">
                  <v-list-item v-bind="props">
                    <template #title>assets ––– <span class="text-red">expand me or the parent</span></template>
                    <template #prepend="{ isSelected }">
                      <v-list-item-action start>
                        <v-checkbox-btn :model-value="isSelected" />
                      </v-list-item-action>
                    </template>
                  </v-list-item>
                </template>
                <v-list-item title="styles.sass" value="4 - Leaf">
                  <template #prepend="{ isSelected }">
                    <v-list-item-action start>
                      <v-checkbox-btn :model-value="isSelected" />
                    </v-list-item-action>
                  </template>
                </v-list-item>
              </v-list-group>
            </v-list-group>
          </v-list-group>
        </v-list-group>
      </v-list>
    </v-container>
  </v-app>
</template>

<script setup>
  import { ref } from 'vue'

  const opened = ref(['src-group', 'renderer-group', 'assets-group'])
  const selected = ref([])

  const items = ref([
    {
      title: 'src',
      children: [
        {
          title: 'renderer',
          children: [
            {
              title: 'src',
              children: [
                {
                  title: 'assets',
                  children: [
                    { title: 'styles.sass', isFileNode: true },
                  ],
                },
              ],
            },
          ],
        },
      ],
    },
  ])
</script>

J-Sek avatar Aug 25 '24 03:08 J-Sek

Note: I searched for while (parent, but some places did not require or even failed when path duplication check was introduced. It looks like a bit of a mess, but I don't feel like I am in a position to make a call for deeper refactoring.

J-Sek avatar Aug 25 '24 03:08 J-Sek

I moved this to register instead which also detects duplicate siblings.

Should we skip this check for leaf nodes? #12286 seems to work fine now if the only duplicates are leaves, getPath will be returning the wrong value for these still though and if only one of the duplicates is unmounted it will break the others because unregister cleans up all the references.

KaelWD avatar Aug 27 '24 15:08 KaelWD