Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nested): Prevent infinite loops when resolving path #20390

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

J-Sek
Copy link
Contributor

@J-Sek J-Sek commented Aug 25, 2024

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
Copy link
Contributor Author

J-Sek commented Aug 25, 2024

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 J-Sek force-pushed the fix-20389 branch 5 times, most recently from caa1cd8 to ab43fc4 Compare August 25, 2024 04:13
@KaelWD KaelWD requested a review from yuwu9145 August 27, 2024 15:04
@KaelWD
Copy link
Member

KaelWD commented Aug 27, 2024

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.

Copy link
Member

@yuwu9145 yuwu9145 left a comment

Choose a reason for hiding this comment

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

I think this PR by this far is good enough, duplicate leaf nodes should be warned as well because it confuses selected/activated values.

Generally speaking, duplicating item-value shouldn't be allowed/encouraged (it's almost impossible to properly handled from our end), if developers don't have confidence of avoiding item-value duplication in items, then using return-object is a graceful way to cope with the issue.

I think we should also make this clear as a "Ceveat" in document in addition to the warning

@KaelWD KaelWD merged commit 970f827 into vuetifyjs:master Aug 31, 2024
10 checks passed
@@ -209,6 +213,15 @@ export const useNested = (props: NestedProps) => {
return arr
}),
register: (id, parentId, isGroup) => {
if (nodeIds.has(id)) {
const path = getPath(id).join(' -> ')

Choose a reason for hiding this comment

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

getPath(id) can be a Symbol, which throws TypeError: Cannot convert a Symbol value to a string.

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants