ronantakizawa/codereview-bench huggingface dataset:
Repository: vuejs/vue-next
File: packages/runtime-vapor/src/apiTemplateRef.ts (TypeScript)

Code before:
  refFor = false,
  refKey?: string,
): NodeRef | undefined {
  if (!instance || instance.isUnmounted) return

  // vdom interop
  const handleVdomRef = (target: any): boolean => {
    if (isInteropEnabled && isFragment(target) && target.setRef) {
      target.setRef(instance, ref, refFor, refKey)
      return true
    }
    return false
  }
  if (handleVdomRef(el)) return

  if (isVaporComponent(el) && isAsyncWrapper(el)) {
    // unresolved: handled in DynamicFragment's updated hook
    if (!el.type.__asyncResolved) return

    // resolved: set ref to the inner component
    el = (el.block as DynamicFragment).nodes as VaporComponentInstance
  }

  const setupState: any = __DEV__ ? instance.setupState || {} : null
  const refValue = getRefValue(el)
  if (handleVdomRef(refValue)) return

  const refs =
    instance.refs === EMPTY_OBJ ? (instance.refs = {}) : instance.refs

  const canSetSetupRef = __DEV__
    ? createCanSetSetupRefChecker(setupState, refs)
    : NO

  const canSetRef = (ref: NodeRef, key?: string) => {
    if (__DEV__ && knownTemplateRefs.has(ref as any)) {
      return false
    }
    if (key && isTemplateRefKey(refs, key)) {
      return false
    }
    return true
  }

  // dynamic ref changed. unset old ref
  if (oldRef != null && oldRef !== ref) {
    if (isString(oldRef)) {
      refs[oldRef] = null
      if (__DEV__ && canSetSetupRef(oldRef)) {
        setupState[oldRef] = null
      }

Review comment (suggestion):
```suggestion
  if (handleVdomRef(refValue)) return ref
```

Code after:
  refFor = false,
  refKey?: string,
): NodeRef | undefined {
  if (!instance || instance.isUnmounted) return

  // vdom interop
  const handleVdomRef = (target: any): boolean => {
    if (isInteropEnabled && isFragment(target) && target.setRef) {
      target.setRef(instance, ref, refFor, refKey)
      return true
    }
    return false
  }
  if (handleVdomRef(el)) return ref

  if (isVaporComponent(el) && isAsyncWrapper(el)) {
    // unresolved: handled in DynamicFragment's updated hook
    if (!el.type.__asyncResolved) return

    // resolved: set ref to the inner component
    el = (el.block as DynamicFragment).nodes as VaporComponentInstance
  }

  const setupState: any = __DEV__ ? instance.setupState || {} : null
  const refValue = getRefValue(el)
  if (handleVdomRef(refValue)) return ref

  const refs =
    instance.refs === EMPTY_OBJ ? (instance.refs = {}) : instance.refs

  const canSetSetupRef = __DEV__
    ? createCanSetSetupRefChecker(setupState, refs)
    : NO

  const canSetRef = (ref: NodeRef, key?: string) => {
    if (__DEV__ && knownTemplateRefs.has(ref as any)) {
      return false
    }
    if (key && isTemplateRefKey(refs, key)) {
      return false
    }
    return true
  }

  // dynamic ref changed. unset old ref
  if (oldRef != null && oldRef !== ref) {
    if (isString(oldRef)) {
      refs[oldRef] = null
      if (__DEV__ && canSetSetupRef(oldRef)) {
        setupState[oldRef] = null
      }
Repository: wagtail/wagtail
File: client/src/components/CommentApp/comments.js (JavaScript)

Code before:
    this.commentAdditionNode.insertAdjacentElement('afterend', annotationNode);
    return new BasicFieldLevelAnnotation(this.fieldNode, annotationNode);
  }
  addComment() {
    this.commentApp.makeComment(
      this.getAnnotationForComment(),
      this.contentPath
    );
  }
}

function initFieldLevelCommentWidget(fieldElement) {
  const widget = new FieldLevelCommentWidget(
    fieldElement,
    fieldElement.querySelector('[data-comment-add]'),
    document.querySelector('#comment-icon'),
    window.commentApp
  );
  widget.register();
}

function prepareFieldLevelCommentInit(fieldSelector) {
  if (window.commentApp) {
    initFieldLevelCommentWidget(fieldSelector);
  } else {
    document.addEventListener('DOMContentLoaded', () => {
      initFieldLevelCommentWidget(fieldSelector);
    });
  }
}

export default {
  initComments,
  FieldLevelCommentWidget,
  initFieldLevelCommentWidget,
  prepareFieldLevelCommentInit,
};


Review comment (suggestion):
**Question:** Would the JS be loaded before `DOMContentLoaded`? I think the full page load is a separate event. I think this event only refers to the DOM, not the actual JS code. Is this an actual issue or are we just prematurely optimising? And frankly, we're repeating the init. This looks like a code smell.

Code after:
      this.commentAdditionNode.classList.add('u-hidden');
    } else {
      this.commentAdditionNode.classList.remove('u-hidden');
    }
  }
  getAnnotationForComment() {
    const annotationNode = this.annotationTemplateNode.cloneNode(true);
    annotationNode.id = '';
    this.commentAdditionNode.insertAdjacentElement('afterend', annotationNode);
    return new BasicFieldLevelAnnotation(this.fieldNode, annotationNode);
  }
}

function initFieldLevelCommentWidget(fieldElement) {
  const widget = new FieldLevelCommentWidget({
    fieldNode: fieldElement,
    commentAdditionNode: fieldElement.querySelector('[data-comment-add]'),
    annotationTemplateNode: document.querySelector('#comment-icon')
  });
  if (widget.contentPath) {
    window.commentApp.registerWidget(widget);
  }
}

export default {
  initComments,
  FieldLevelCommentWidget,
  initFieldLevelCommentWidget
};
Repository: wagtail/wagtail
File: wagtail/admin/utils.py (Python)

Code before:
from django.conf import settings


def get_admin_base_url(context=None):
    """
    Gets the base URL for the wagtail admin site. This is set in `settings.WAGTAILADMIN_BASE_URL`,
    which was previously `settings.BASE_URL`.
    If setting is omitted and this is called in a request context, falls back to
    `request.site.root_url` or next the host_name of the request.
    """

    admin_base_url = getattr(settings, "WAGTAILADMIN_BASE_URL", None)
    if admin_base_url is None:
        admin_base_url = getattr(settings, "BASE_URL", None)

    if admin_base_url is None and context is not None:
        request = context["request"]
        admin_base_url = getattr(request.site, "root_url", None)
        if admin_base_url is None:
            admin_base_url = request.get_host()
            secure_prefix = "http"
            if request.is_secure():
                secure_prefix = "https"
            admin_base_url = secure_prefix + "://" + admin_base_url

    return admin_base_url


Review comment (suggestion):
We may want to add a deprecated warning here, but only if `WAGTAILADMIN_BASE_URL` is `None` and `BASE_URL` is not `None`.

See `wagtail/utils/deprecation.py` for the utils we use for this.

This would probably be a two release warning, so I recommend you use `RemovedInWagtail50Warning`, you can search through the code for other examples.

Code after:
from warnings import warn

from django.conf import settings

from wagtail.utils.deprecation import RemovedInWagtail50Warning


def get_admin_base_url(context=None):
    """
    Gets the base URL for the wagtail admin site. This is set in `settings.WAGTAILADMIN_BASE_URL`,
    which was previously `settings.BASE_URL`.
    If setting is omitted and this is called in a request context, falls back to
    `request.site.root_url` or next the host_name of the request.
    """

    admin_base_url = getattr(settings, "WAGTAILADMIN_BASE_URL", None)
    if admin_base_url is None and hasattr(settings, "BASE_URL"):
        warn(
            "settings.BASE_URL has been renamed to settings.WAGTAILADMIN_BASE_URL",
            category=RemovedInWagtail50Warning,
        )
        admin_base_url = settings.BASE_URL

    if admin_base_url is None and context is not None:
        request = context["request"]
        admin_base_url = getattr(request.site, "root_url", None)
        if admin_base_url is None:
            admin_base_url = request.get_host()
            secure_prefix = "http"
            if request.is_secure():
                secure_prefix = "https"
            admin_base_url = secure_prefix + "://" + admin_base_url

    return admin_base_url
Repository: wagtail/wagtail
File: client/src/includes/tabs.js (JavaScript)

Code before:
        e.preventDefault();
        this.selectTab(tab);
      });
      tab.addEventListener('focusin', () => {
        this.selectTab(tab);
      });
      tab.addEventListener('keydown', this.keydownEventListener);
      // Set index of tab used in keyboard controls
      // eslint-disable-next-line no-param-reassign
      tab.index = index;
    });

    // Select previous or next tab using history
    window.addEventListener('popstate', (e) => {
      if (e.state && e.state.tabContent) {
        const tab = this.tabContainer.querySelector(
          `a[href="#${e.state.tabContent}"][role="tab"]`,
        );
        if (tab) {
          this.selectTab(tab);
        }
      }
    });

    // Dispatch tab-changed on window load
    window.onload = () => {
      if (this.tabContainer) {
        document.dispatchEvent(new CustomEvent('wagtail:tab-changed'));
      }
    };
  }

  /**
   *  Handle keydown on tabs
   * @param {Event}event
   */
  keydownEventListener(event) {
    const keyPressed = event.key;
    const { keys } = this.state;

    switch (keyPressed) {
      case keys.left:
      case keys.right:
        this.switchTabOnArrowPress(event);
        break;
      case keys.end:
        event.preventDefault();
        this.focusLastTab();
        break;
      case keys.home:
        event.preventDefault();

Review comment (suggestion):
```suggestion
    document.addEventListener('DOMContentLoaded', () => {
```

We should be using `addEventListener` for this rather than the global callback. Technically all of the tabs initialisations are meant to be happening after `DOMContentLoaded` anyway, so I don’t see why we’d even want to dispatch the event only on `load`.

Code after:
        e.preventDefault();
        this.selectTab(tab);
      });
      tab.addEventListener('focusin', () => {
        this.selectTab(tab);
      });
      tab.addEventListener('keydown', this.keydownEventListener);
      // Set index of tab used in keyboard controls
      // eslint-disable-next-line no-param-reassign
      tab.index = index;
    });

    // Select previous or next tab using history
    window.addEventListener('popstate', (e) => {
      if (e.state && e.state.tabContent) {
        const tab = this.tabContainer.querySelector(
          `a[href="#${e.state.tabContent}"][role="tab"]`,
        );
        if (tab) {
          this.selectTab(tab);
        }
      }
    });
  }

  /**
   *  Handle keydown on tabs
   * @param {Event}event
   */
  keydownEventListener(event) {
    const keyPressed = event.key;
    const { keys } = this.state;

    switch (keyPressed) {
      case keys.left:
      case keys.right:
        this.switchTabOnArrowPress(event);
        break;
      case keys.end:
        event.preventDefault();
        this.focusLastTab();
        break;
      case keys.home:
        event.preventDefault();
        this.focusFirstTab();
        break;
      default:
        break;
    }
  }
Repository: wagtail/wagtail
File: client/src/includes/tabs.js (JavaScript)

Code before:
        this.selectTab(tab);
      });
      tab.addEventListener('focusin', () => {
        this.selectTab(tab);
      });
      tab.addEventListener('keydown', this.keydownEventListener);
      // Set index of tab used in keyboard controls
      // eslint-disable-next-line no-param-reassign
      tab.index = index;
    });

    // Select previous or next tab using history
    window.addEventListener('popstate', (e) => {
      if (e.state && e.state.tabContent) {
        const tab = this.tabContainer.querySelector(
          `a[href="#${e.state.tabContent}"][role="tab"]`,
        );
        if (tab) {
          this.selectTab(tab);
        }
      }
    });

    // Dispatch tab-changed on window load
    window.onload = () => {
      if (this.tabContainer) {
        document.dispatchEvent(new CustomEvent('wagtail:tab-changed'));
      }
    };
  }

  /**
   *  Handle keydown on tabs
   * @param {Event}event
   */
  keydownEventListener(event) {
    const keyPressed = event.key;
    const { keys } = this.state;

    switch (keyPressed) {
      case keys.left:
      case keys.right:
        this.switchTabOnArrowPress(event);
        break;
      case keys.end:
        event.preventDefault();
        this.focusLastTab();
        break;
      case keys.home:
        event.preventDefault();
        this.focusFirstTab();

Review comment (suggestion):
I don’t see how this could ever not be set? We’re already relying on `this.tabContainer` being defined elsewhere so I don’t see what this protects us from.

Code after:
        this.selectTab(tab);
      });
      tab.addEventListener('focusin', () => {
        this.selectTab(tab);
      });
      tab.addEventListener('keydown', this.keydownEventListener);
      // Set index of tab used in keyboard controls
      // eslint-disable-next-line no-param-reassign
      tab.index = index;
    });

    // Select previous or next tab using history
    window.addEventListener('popstate', (e) => {
      if (e.state && e.state.tabContent) {
        const tab = this.tabContainer.querySelector(
          `a[href="#${e.state.tabContent}"][role="tab"]`,
        );
        if (tab) {
          this.selectTab(tab);
        }
      }
    });
  }

  /**
   *  Handle keydown on tabs
   * @param {Event}event
   */
  keydownEventListener(event) {
    const keyPressed = event.key;
    const { keys } = this.state;

    switch (keyPressed) {
      case keys.left:
      case keys.right:
        this.switchTabOnArrowPress(event);
        break;
      case keys.end:
        event.preventDefault();
        this.focusLastTab();
        break;
      case keys.home:
        event.preventDefault();
        this.focusFirstTab();
        break;
      default:
        break;
    }
  }

  selectTabByURLHash() {
Repository: wagtail/wagtail
File: client/src/entrypoints/admin/page-editor.js (JavaScript)

Code before:
let updateFooterTextTimeout = -1;
window.updateFooterSaveWarning = (formDirty, commentsDirty) => {
  const warningContainer = $('[data-unsaved-warning]');
  const warnings = warningContainer.find('[data-unsaved-type]');
  const anyDirty = formDirty || commentsDirty;
  const typeVisibility = {
    all: formDirty && commentsDirty,
    any: anyDirty,
    comments: commentsDirty && !formDirty,
    edits: formDirty && !commentsDirty,
  };

  let hiding = false;
  if (anyDirty) {
    warningContainer.removeClass('footer__container--hidden');
  } else {
    if (!warningContainer.hasClass('footer__container--hidden')) {
      hiding = true;
    }
    warningContainer.addClass('footer__container--hidden');
  }
  clearTimeout(updateFooterTextTimeout);
  const updateWarnings = () => {
    warnings.forEach((warning) => {
      const visible = typeVisibility[warning.dataset.unsavedType];
      warning.hidden = !visible; /* eslint-disable-line no-param-reassign */
    });
  };
  if (hiding) {
    // If hiding, we want to keep the text as-is before it disappears
    updateFooterTextTimeout = setTimeout(updateWarnings, 1050);
  } else {
    updateWarnings();
  }
};


Review comment (suggestion):
We follow the Airbnb styleguide (https://github.com/airbnb/javascript), which encourages always placing comments on their own separate line, preceding what the comment is for. So in this case:

```suggestion
     // eslint-disable-next-line no-param-reassign
      warning.hidden = !visible;
```

Code after:
let updateFooterTextTimeout = -1;
window.updateFooterSaveWarning = (formDirty, commentsDirty) => {
  const warningContainer = $('[data-unsaved-warning]');
  const warnings = warningContainer.find('[data-unsaved-type]');
  const anyDirty = formDirty || commentsDirty;
  const typeVisibility = {
    all: formDirty && commentsDirty,
    any: anyDirty,
    comments: commentsDirty && !formDirty,
    edits: formDirty && !commentsDirty,
  };

  let hiding = false;
  if (anyDirty) {
    warningContainer.removeClass('footer__container--hidden');
  } else {
    if (!warningContainer.hasClass('footer__container--hidden')) {
      hiding = true;
    }
    warningContainer.addClass('footer__container--hidden');
  }
  clearTimeout(updateFooterTextTimeout);
  const updateWarnings = () => {
    warnings.forEach((warning) => {
      const visible = typeVisibility[warning.dataset.unsavedType];
      // eslint-disable-next-line no-param-reassign
      warning.hidden = !visible;
    });
  };
  if (hiding) {
    // If hiding, we want to keep the text as-is before it disappears
    updateFooterTextTimeout = setTimeout(updateWarnings, 1050);
  } else {
    updateWarnings();
  }
};
Repository: wagtail/wagtail
File: wagtail/admin/urls/__init__.py (Python)

Code before:
        StreamFieldBlockPreview.as_view(),
        name="wagtailadmin_block_preview",
    ),
]


# Import additional urlpatterns from any apps that define a register_admin_urls hook
for fn in hooks.get_hooks("register_admin_urls"):
    urls = fn()
    if urls:
        urlpatterns += urls


# Add "wagtailadmin.access_admin" permission check
urlpatterns = decorate_urlpatterns(urlpatterns, require_admin_access)


def localized_js_catalog():
    js_catalog_view = JavaScriptCatalog.as_view(packages=["wagtail.admin"])

    def view(request, *args, **kwargs):
        return get_localized_response(
            request.user, js_catalog_view, request, *args, **kwargs
        )

    return view


# These url patterns do not require an authenticated admin user
urlpatterns += [
    path("sprite/", home.sprite, name="wagtailadmin_sprite"),
    path("login/", account.LoginView.as_view(), name="wagtailadmin_login"),
    # Password reset
    path("password_reset/", include(wagtailadmin_password_reset_urls)),
    # JS translation catalog
    path(
        "jsi18n/",
        localized_js_catalog(),
        name="wagtailadmin_javascript_catalog",
    ),
]


# Default view (will show 404 page)
# This must be the last URL in this file!

if settings.APPEND_SLASH:
    # Only catch unrecognised patterns with a trailing slash
    # and let CommonMiddleware handle adding a slash to every other pattern
    urlpatterns += [
        re_path(r"^.*/$", home.default),

Review comment (suggestion):
We should probably put this in its own file in `wagtail.admin.views`, just so that we don't pollute the urls file with ad-hoc views.

Code after:
        name="wagtailadmin_block_preview",
    ),
]


# Import additional urlpatterns from any apps that define a register_admin_urls hook
for fn in hooks.get_hooks("register_admin_urls"):
    urls = fn()
    if urls:
        urlpatterns += urls


# Add "wagtailadmin.access_admin" permission check
urlpatterns = decorate_urlpatterns(urlpatterns, require_admin_access)


# These url patterns do not require an authenticated admin user
urlpatterns += [
    path("sprite/", home.sprite, name="wagtailadmin_sprite"),
    path("login/", account.LoginView.as_view(), name="wagtailadmin_login"),
    # Password reset
    path("password_reset/", include(wagtailadmin_password_reset_urls)),
    # JS translation catalog
    path(
        "jsi18n/",
        localized_js_catalog,
        name="wagtailadmin_javascript_catalog",
    ),
]


# Default view (will show 404 page)
# This must be the last URL in this file!

if settings.APPEND_SLASH:
    # Only catch unrecognised patterns with a trailing slash
    # and let CommonMiddleware handle adding a slash to every other pattern
    urlpatterns += [
        re_path(r"^.*/$", home.default),
    ]

else:
    # Catch all unrecognised patterns
    urlpatterns += [
        re_path(r"^", home.default),
    ]


# Hook in our own 404 handler
def display_custom_404(view_func):
    @functools.wraps(view_func)
Repository: wagtail/wagtail
File: wagtail/admin/wagtail_hooks.py (Python)

Code before:
                self.label,
                self.url,
                start_page.id,
                icon_name=self.icon_name,
                classname=self.classname,
            )
        else:
            return super().render_component(request)


@hooks.register("register_help_menu_item")
def register_keyboard_shortcuts_menu_item():
    return MenuItem(
        _("Shortcuts"),
        icon_name="regex",
        order=1200,
        attrs={
            "role": "button",
            "data-a11y-dialog-show": "keyboard-shortcuts-dialog",
            "data-action": "w-action#noop:prevent:stop",
            "data-controller": "w-action",
        },
        name="keyboard-shortcuts-trigger",
        url="#",
    )


@hooks.register("register_admin_menu_item")
def register_explorer_menu_item():
    return ExplorerMenuItem(
        _("Pages"),
        reverse("wagtailadmin_explore_root"),
        name="explorer",
        icon_name="folder-open-inverse",
        order=100,
    )


class SettingsMenuItem(SubmenuMenuItem):
    def render_component(self, request):
        return SubMenuItemComponent(
            self.name,
            self.label,
            self.menu.render_component(request),
            icon_name=self.icon_name,
            classname=self.classname,
            footer_text="Wagtail v" + __version__,
        )


@hooks.register("register_admin_menu_item")

Review comment (suggestion):
Please can this be moved to around line 1016 - after `register_editors_guide_menu_item`. This way the code order will follow the visual rendering ordering the sidebar.


Code after:
                self.label,
                self.url,
                start_page.id,
                icon_name=self.icon_name,
                classname=self.classname,
            )
        else:
            return super().render_component(request)


@hooks.register("register_admin_menu_item")
def register_explorer_menu_item():
    return ExplorerMenuItem(
        _("Pages"),
        reverse("wagtailadmin_explore_root"),
        name="explorer",
        icon_name="folder-open-inverse",
        order=100,
    )


class SettingsMenuItem(SubmenuMenuItem):
    def render_component(self, request):
        return SubMenuItemComponent(
            self.name,
            self.label,
            self.menu.render_component(request),
            icon_name=self.icon_name,
            classname=self.classname,
            footer_text="Wagtail v" + __version__,
        )


@hooks.register("register_admin_menu_item")
def register_settings_menu():
    return SettingsMenuItem(
        _("Settings"),
        settings_menu,
        name="settings",
        icon_name="cogs",
        order=10000,
    )


@hooks.register("register_permissions")
def register_permissions():
    return Permission.objects.filter(
        content_type__app_label="wagtailadmin", codename="access_admin"
    )