ronantakizawa/codereview-bench huggingface dataset:
Repository: wagtail/wagtail
File: client/src/controllers/KeyboardController.test.js (JavaScript)

Code before:
import { Application } from '@hotwired/stimulus';
import Mousetrap from 'mousetrap';

import { KeyboardController } from './KeyboardController';

jest.mock('../config/wagtailConfig', () => ({
  WAGTAIL_CONFIG: {
    KEYBOARD_SHORTCUT_PREFERENCE: true,
  },
}));

describe('KeyboardController', () => {
  let app;
  const buttonClickMock = jest.fn();

  /**
   * Simulates a keydown, keypress, and keyup event for the provided key.
   */
  const simulateKey = (
    { key, which = key.charCodeAt(0), ctrlKey = false, metaKey = false },
    target = document.body,
  ) =>
    Object.fromEntries(
      ['keydown', 'keypress', 'keyup'].map((type) => [
        type,
        target.dispatchEvent(
          new KeyboardEvent(type, {
            bubbles: true,
            cancelable: true,
            key: key,
            which,
            ctrlKey,
            metaKey,

Review comment (suggestion):
It might be better to add the 'default' (true) to the core stubs instead.

`client/tests/stubs.js`

Code after:
import { Application } from '@hotwired/stimulus';
import Mousetrap from 'mousetrap';

import { KeyboardController } from './KeyboardController';

jest.mock('../config/wagtailConfig', () => ({
  WAGTAIL_CONFIG: {
    KEYBOARD_SHORTCUTS_ENABLED: true,
  },
}));

describe('KeyboardController', () => {
  let app;
  const buttonClickMock = jest.fn();

  /**
   * Simulates a keydown, keypress, and keyup event for the provided key.
   */
  const simulateKey = (
    { key, which = key.charCodeAt(0), ctrlKey = false, metaKey = false },
    target = document.body,
  ) =>
    Object.fromEntries(
      ['keydown', 'keypress', 'keyup'].map((type) => [
        type,
        target.dispatchEvent(
          new KeyboardEvent(type, {
            bubbles: true,
            cancelable: true,
            key: key,
            which,
            ctrlKey,
            metaKey,
Repository: wagtail/wagtail
File: wagtail/api/v2/views.py (Python)

Code before:

from wagtail.api import APIField
from wagtail.models import Page, PageViewRestriction, Site

from .filters import (
    AncestorOfFilter,
    ChildOfFilter,
    DescendantOfFilter,
    FieldsFilter,
    LocaleFilter,
    OrderingFilter,
    SearchFilter,
    TranslationOfFilter,
)
from .pagination import WagtailPagination
from .serializers import BaseSerializer, PageSerializer, get_serializer_class
from .utils import (
    BadRequestError,
    get_object_detail_url,
    page_models_from_string,
    parse_fields_parameter,
)


class BaseAPIViewSet(GenericViewSet):
    @property
    def renderer_classes(self):
        """
        Dynamically determine renderer classes based on installed apps.
        If rest_framework is not installed, only use JSONRenderer to avoid
        template/static file errors when accessing endpoints directly in browser.
        """
        renderers = [JSONRenderer]

        # Only add BrowsableAPIRenderer if rest_framework is installed
        # (which provides the necessary templates and static files)
        if apps.is_installed("rest_framework"):
            renderers.append(BrowsableAPIRenderer)

        return renderers

    pagination_class = WagtailPagination
    base_serializer_class = BaseSerializer
    filter_backends = []
    model = None  # Set on subclass

    known_query_parameters = frozenset(
        [
            "limit",
            "offset",
            "fields",

Review comment (suggestion):
Maybe we should use `classproperty` just in case people do `renderer_classes = BaseAPIViewSet.renderer_classes + [...]` in their subclasses

Code after:
from rest_framework.viewsets import GenericViewSet

from wagtail.api import APIField
from wagtail.models import Page, PageViewRestriction, Site

from .filters import (
    AncestorOfFilter,
    ChildOfFilter,
    DescendantOfFilter,
    FieldsFilter,
    LocaleFilter,
    OrderingFilter,
    SearchFilter,
    TranslationOfFilter,
)
from .pagination import WagtailPagination
from .serializers import BaseSerializer, PageSerializer, get_serializer_class
from .utils import (
    BadRequestError,
    get_object_detail_url,
    page_models_from_string,
    parse_fields_parameter,
)


class BaseAPIViewSet(GenericViewSet):
    @classproperty
    def renderer_classes(cls):
        renderers = [JSONRenderer]

        # Only add BrowsableAPIRenderer if rest_framework is installed
        # (which provides the necessary templates and static files)
        if apps.is_installed("rest_framework"):
            renderers.append(BrowsableAPIRenderer)

        return renderers

    pagination_class = WagtailPagination
    base_serializer_class = BaseSerializer
    filter_backends = []
    model = None  # Set on subclass

    known_query_parameters = frozenset(
        [
            "limit",
            "offset",
            "fields",
            "order",
            "search",
            "search_operator",
            # Used by jQuery for cache-busting. See #1671
Repository: wagtail/wagtail
File: wagtail/admin/tests/test_filters.py (Python)

Code before:
import datetime

from django import forms
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Group
from django.test import TestCase
from django.urls import reverse

from wagtail.admin.filters import FilteredModelChoiceField
from wagtail.documents import get_document_model
from wagtail.documents.tests.utils import get_test_document_file
from wagtail.images import get_image_model
from wagtail.images.tests.utils import get_test_image_file
from wagtail.models import Page
from wagtail.test.testapp.models import EventPage, EventPageRelatedLink
from wagtail.test.utils import WagtailTestUtils

User = get_user_model()


class TestFilteredModelChoiceField(WagtailTestUtils, TestCase):
    def setUp(self):
        self.musicians = Group.objects.create(name="Musicians")
        self.actors = Group.objects.create(name="Actors")

        self.david = self.create_user(
            "david",
            "david@example.com",
            "kn1ghtr1der",
            first_name="David",
            last_name="Hasselhoff",
        )
        self.david.groups.set([self.musicians, self.actors])

        self.kevin = self.create_user(
            "kevin",
            "kevin@example.com",
            "6degrees",

Review comment (suggestion):
This test module is in `wagtail.admin` and it should not have a dependency to `wagtail.documents`/`wagtail.images`. The tests for documents and images views should go to those modules (specifically `wagtail.{documents,images}.tests.test_admin_views` like in #13234).

Code after:
from django import forms
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Group
from django.test import TestCase

from wagtail.admin.filters import FilteredModelChoiceField
from wagtail.test.utils import WagtailTestUtils

User = get_user_model()


class TestFilteredModelChoiceField(WagtailTestUtils, TestCase):
    def setUp(self):
        self.musicians = Group.objects.create(name="Musicians")
        self.actors = Group.objects.create(name="Actors")

        self.david = self.create_user(
            "david",
            "david@example.com",
            "kn1ghtr1der",
            first_name="David",
            last_name="Hasselhoff",
        )
        self.david.groups.set([self.musicians, self.actors])

        self.kevin = self.create_user(
            "kevin",
            "kevin@example.com",
            "6degrees",
            first_name="Kevin",
            last_name="Bacon",
        )
        self.kevin.groups.set([self.actors])

        self.morten = self.create_user(
            "morten",
            "morten@example.com",
            "t4ke0nm3",
Repository: wagtail/wagtail
File: client/src/entrypoints/admin/comments.js (JavaScript)

Code before:
import { gettext } from '../../utils/gettext';
import { initCommentApp } from '../../components/CommentApp/main';

import { WAGTAIL_CONFIG } from '../../config/wagtailConfig';

const KEYCODE_M = 77;

/**
 * Entrypoint loaded when the comments system is in use.
 */
window.comments = (() => {
  const commentApp = initCommentApp();

  /**
   * Returns true if the provided keyboard event is using the 'add/focus comment' keyboard
   * shortcut and custom keyboard shortcuts enabled by user in settings
   */
  function isCommentShortcut(e) {
    return (
      (e.ctrlKey || e.metaKey) &&
      e.altKey &&
      e.keyCode === KEYCODE_M &&
      WAGTAIL_CONFIG.KEYBOARD_SHORTCUTS_ENABLED
    );
  }

  function getContentPath(fieldNode) {
    // Return the total contentpath for an element as a string, in the form field.streamfield_uid.block...
    if (!fieldNode || fieldNode.closest('[data-contentpath-disabled]')) {
      return '';
    }
    let element = fieldNode.closest('[data-contentpath]');
    const contentpaths = [];
    while (element !== null) {
      contentpaths.push(element.dataset.contentpath);
      element = element.parentElement.closest('[data-contentpath]');
    }
    contentpaths.reverse();
    return contentpaths.join('.');
  }


Review comment (suggestion):
```suggestion
   * shortcut and custom keyboard shortcuts enabled by user in settings.
```

Just a small nitpick. 

Code after:
import { gettext } from '../../utils/gettext';
import { initCommentApp } from '../../components/CommentApp/main';

import { WAGTAIL_CONFIG } from '../../config/wagtailConfig';

const KEYCODE_M = 77;

/**
 * Entrypoint loaded when the comments system is in use.
 */
window.comments = (() => {
  const commentApp = initCommentApp();

  /**
   * Returns true if the provided keyboard event is using the 'add/focus comment' keyboard
   * shortcut and custom keyboard shortcuts enabled by user in settings.
   */
  function isCommentShortcut(e) {
    return (
      WAGTAIL_CONFIG.KEYBOARD_SHORTCUTS_ENABLED &&
      (e.ctrlKey || e.metaKey) &&
      e.altKey &&
      e.keyCode === KEYCODE_M
    );
  }

  function getContentPath(fieldNode) {
    // Return the total contentpath for an element as a string, in the form field.streamfield_uid.block...
    if (!fieldNode || fieldNode.closest('[data-contentpath-disabled]')) {
      return '';
    }
    let element = fieldNode.closest('[data-contentpath]');
    const contentpaths = [];
    while (element !== null) {
      contentpaths.push(element.dataset.contentpath);
      element = element.parentElement.closest('[data-contentpath]');
    }
    contentpaths.reverse();
    return contentpaths.join('.');
  }
Repository: wagtail/wagtail
File: client/src/controllers/KeyboardController.ts (TypeScript)

Code before:
    }
  }

  /**
   * Handle the key event by preventing the default action and performing the specified action.
   */
  handleKey(event: Event) {
    if (event.preventDefault) event.preventDefault();
    const targetElement =
      this.elementTargets.length > 0 ? this.elementTargets[0] : this.element;
    if (this.actionValue === KeyboardAction.FOCUS) {
      this.handleFocus(targetElement as HTMLInputElement);
    } else {
      this.handleClick(targetElement);
    }
  }

  /**
   * Handle the click action on target element
   */
  handleClick(target: HTMLElement) {
    target.click();
  }

  /**
   * Handle the focus action on target element (focusing on searchbar)
   */
  handleFocus(target: HTMLInputElement) {
    target.focus();

    if (target.value && target.select) {
      target.select();
    }
  }

  /**
   * When a key is set or changed, bind the handler to the keyboard shortcut.
   * This will override the shortcut, if already set.
   * Allow key to be empty, so it can be removed from the attributes to unbind the shortcuts
   */
  keyValueChanged(key: string, previousKey: string) {
    if (previousKey && previousKey !== key) {
      Mousetrap.unbind(previousKey);
    }

    if (!key) return;

    if (this.scopeValue === 'global') {
      Mousetrap.bindGlobal(key, this.handleKey);
    } else {
      Mousetrap.bind(key, this.handleKey);

Review comment (suggestion):
```suggestion
   * Handle the focus action on target element and if possible select the text in the input.
```

Code after:

      if (ariaKeyShortcuts) {
        this.keyValue = ariaKeyShortcuts;
      }
    }
  }

  /**
   * Handle the key event by preventing the default action and performing the specified action.
   */
  handleKey(event: Event) {
    if (event.preventDefault) event.preventDefault();
    const targetElement =
      this.elementTargets.length > 0 ? this.elementTargets[0] : this.element;
    if (this.actionValue === KeyboardAction.FOCUS) {
      this.handleFocus(targetElement);
    } else {
      this.handleClick(targetElement);
    }
  }

  /**
   * Handle the click action on target element.
   */
  handleClick(target: HTMLElement) {
    target.click();
  }

  /**
   * Handle the focus action on target element. If it is an input element containing text,
   * also selects the existing text inside the field.
   */
  handleFocus(target: HTMLElement) {
    forceFocus(target);

    if (target instanceof HTMLInputElement && target.value && target.select) {
      target.select();
    }
  }

  /**
   * When a key is set or changed, bind the handler to the keyboard shortcut.
   * This will override the shortcut, if already set.
   * Allow key to be empty, so it can be removed from the attributes to unbind the shortcuts
   */
  keyValueChanged(key: string, previousKey: string) {
    if (previousKey && previousKey !== key) {
      Mousetrap.unbind(previousKey);
    }

    if (!key) return;
Repository: wagtail/wagtail
File: client/src/controllers/KeyboardController.ts (TypeScript)

Code before:
   * Set up the handleKey binding and check for an aria-keyshortcuts attribute to
   * use as a fallback if the key value is not set.
   */
  initialize() {
    this.handleKey = this.handleKey.bind(this);

    if (!this.keyValue) {
      const ariaKeyShortcuts = [...this.elementTargets, this.element]
        .find((element) => element.hasAttribute('aria-keyshortcuts'))
        ?.getAttribute('aria-keyshortcuts');

      if (ariaKeyShortcuts) {
        this.keyValue = ariaKeyShortcuts;
      }
    }
  }

  /**
   * Handle the key event by preventing the default action and performing the specified action.
   */
  handleKey(event: Event) {
    if (event.preventDefault) event.preventDefault();
    const targetElement =
      this.elementTargets.length > 0 ? this.elementTargets[0] : this.element;
    if (this.actionValue === KeyboardAction.FOCUS) {
      this.handleFocus(targetElement as HTMLElement);
    } else {
      this.handleClick(targetElement);
    }
  }

  /**
   * Handle the click action on target element.
   */
  handleClick(target: HTMLElement) {
    target.click();
  }

  /**
   * Handle the focus action on target element. If it is an input element containing text,
   * also selects the existing text inside the field.
   */
  handleFocus(target: HTMLElement) {
    forceFocus(target);

    if (target instanceof HTMLInputElement && target.value && target.select) {
      target.select();
    }
  }

  /**

Review comment (refactor):
```suggestion
      this.handleFocus(targetElement);
```

If we simplify the types above, this is no longer needed, let's use `HTMLElement` throughout.

Code after:

  /**
   * Set up the handleKey binding and check for an aria-keyshortcuts attribute to
   * use as a fallback if the key value is not set.
   */
  initialize() {
    this.handleKey = this.handleKey.bind(this);

    if (!this.keyValue) {
      const ariaKeyShortcuts = [...this.elementTargets, this.element]
        .find((element) => element.hasAttribute('aria-keyshortcuts'))
        ?.getAttribute('aria-keyshortcuts');

      if (ariaKeyShortcuts) {
        this.keyValue = ariaKeyShortcuts;
      }
    }
  }

  /**
   * Handle the key event by preventing the default action and performing the specified action.
   */
  handleKey(event: Event) {
    if (event.preventDefault) event.preventDefault();
    const targetElement =
      this.elementTargets.length > 0 ? this.elementTargets[0] : this.element;
    if (this.actionValue === KeyboardAction.FOCUS) {
      this.handleFocus(targetElement);
    } else {
      this.handleClick(targetElement);
    }
  }

  /**
   * Handle the click action on target element.
   */
  handleClick(target: HTMLElement) {
    target.click();
  }

  /**
   * Handle the focus action on target element. If it is an input element containing text,
   * also selects the existing text inside the field.
   */
  handleFocus(target: HTMLElement) {
    forceFocus(target);

    if (target instanceof HTMLInputElement && target.value && target.select) {
      target.select();
    }
  }
Repository: wagtail/wagtail
File: wagtail/query.py (Python)

Code before:
            # benefit to splitting the result
            yield tuple(queryset)
        else:
            # Iterate through the queryset, returning the rows in manageable
            # chunks for self.__iter__() to fetch full instances for
            current_chunk = []
            for r in queryset.iterator(self.chunk_size):
                current_chunk.append(r)
                if len(current_chunk) == self.chunk_size:
                    yield tuple(current_chunk)
                    current_chunk.clear()
            # Return any left-overs
            if current_chunk:
                yield tuple(current_chunk)


class DeferredSpecificIterable(ModelIterable):
    def __iter__(self):
        for obj in super().__iter__():
            if obj.specific_class:
                yield obj.specific_deferred
            else:
                warnings.warn(
                    "A specific version of the following object could not be returned "
                    "because the specific model is not present on the active "
                    f"branch: <{obj.__class__.__name__} id='{obj.id}' "
                    f"type='{obj.content_type}'>",
                    category=RuntimeWarning,
                )
                yield obj


Review comment (suggestion):
```suggestion
                    f"branch: <{obj.__class__.__name__} id='{obj.id}' title='{getattr(obj, 'title', str(obj)}'"
```

Code after:

    def _get_chunks(self, queryset) -> Iterable[tuple[dict[str, Any]]]:
        if not self.chunked_fetch:
            # The entire result will be stored in memory, so there is no
            # benefit to splitting the result
            yield tuple(queryset)
        else:
            # Iterate through the queryset, returning the rows in manageable
            # chunks for self.__iter__() to fetch full instances for
            current_chunk = []
            for r in queryset.iterator(self.chunk_size):
                current_chunk.append(r)
                if len(current_chunk) == self.chunk_size:
                    yield tuple(current_chunk)
                    current_chunk.clear()
            # Return any left-overs
            if current_chunk:
                yield tuple(current_chunk)


class DeferredSpecificIterable(ModelIterable):
    def __iter__(self):
        for obj in super().__iter__():
            if obj.specific_class:
                yield obj.specific_deferred
            else:
                warnings.warn(
                    "A specific version of the following object could not be returned "
                    "because the specific model is not present on the active "
                    f"branch: <{obj.__class__.__name__} id='{obj.id}' "
                    f"title='{getattr(obj, 'title', str(obj))}' "
                    f"type='{obj.content_type}'>",
                    category=RuntimeWarning,
                )
                yield obj
Repository: wagtail/wagtail
File: scripts/nightly/upload.py (Python)

Code before:

print("Uploading", f.name)  # noqa: T201
s3 = boto3.client("s3")
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/" + f.name,
    ExtraArgs={"ACL": "public-read"},
)
# Redundant upload to a fixed filename for ease of use with package managers
print("Uploading latest.whl")  # noqa: T201
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/latest.whl",
    ExtraArgs={"ACL": "public-read"},
)

print("Updating latest.json")  # noqa: T201

boto3.resource("s3").Object("releases.wagtail.io", "nightly/latest.json").put(
    ACL="public-read",
    Body=json.dumps(
        {
            "url": "https://releases.wagtail.org/nightly/dist/" + f.name,
            "latestUrl": "https://releases.wagtail.org/nightly/dist/latest.whl",
        }
    ),
)


Review comment (suggestion):
I'm not sure if this is a good idea, I guess this could cause code that uses `grep` or a similar tool that looks for `https://[^"]*`, such as how I did it for [our bakerydemo nightly](https://github.com/wagtail/bakerydemo/blob/6a16f1b7d3c53d424361825abb9507032b5b31a4/Dockerfile#L38-L40), to end up with multiple results.

Edit: reverted.

Code after:
    sys.exit()

print("Uploading", f.name)  # noqa: T201
s3 = boto3.client("s3")
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/" + f.name,
)
# Redundant upload to a fixed filename for ease of use with package managers
print("Uploading latest.whl")  # noqa: T201
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/latest.whl",
)

print("Updating latest.json")  # noqa: T201

boto3.resource("s3").Object("releases.wagtail.io", "nightly/latest.json").put(
    Body=json.dumps(
        {
            "url": "https://releases.wagtail.org/nightly/dist/" + f.name,
        }
    ),
)

print("Purging cache")  # noqa: T201

boto3.client("cloudfront").create_invalidation(
    DistributionId="E283SZ5CB4MDM0",
    InvalidationBatch={
        "Paths": {
            "Quantity": 2,
            "Items": ["/nightly/latest.json", "/nightly/dist/latest.whl"],
        },
        "CallerReference": str(uuid.uuid4()),
    },
)
Repository: wagtail/wagtail
File: scripts/nightly/upload.py (Python)

Code before:

import boto3

dist_folder = pathlib.Path.cwd() / "dist"

try:
    f = next(dist_folder.glob("*.whl"))
except StopIteration:
    print("No .whl files found in ./dist!")  # noqa: T201
    sys.exit()

print("Uploading", f.name)  # noqa: T201
s3 = boto3.client("s3")
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/" + f.name,
    ExtraArgs={"ACL": "public-read"},
)
# Redundant upload to a fixed filename for ease of use with package managers
print("Uploading latest.whl")  # noqa: T201
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/latest.whl",
    ExtraArgs={"ACL": "public-read"},
)

print("Updating latest.json")  # noqa: T201

boto3.resource("s3").Object("releases.wagtail.io", "nightly/latest.json").put(
    ACL="public-read",
    Body=json.dumps(
        {
            "url": "https://releases.wagtail.org/nightly/dist/" + f.name,
        }
    ),
)


Review comment (performance):
**Question:** won't responses be cached by Cloudfront for some amount of time?  That would result in users unexpectedly downloading an older nightly if the Cloudfront cache was refreshed just before we upload a new latest.whl. Or does uploading a new object to S3 clear the cache for that key?

Code after:
import uuid

import boto3

dist_folder = pathlib.Path.cwd() / "dist"

try:
    f = next(dist_folder.glob("*.whl"))
except StopIteration:
    print("No .whl files found in ./dist!")  # noqa: T201
    sys.exit()

print("Uploading", f.name)  # noqa: T201
s3 = boto3.client("s3")
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/" + f.name,
)
# Redundant upload to a fixed filename for ease of use with package managers
print("Uploading latest.whl")  # noqa: T201
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/latest.whl",
)

print("Updating latest.json")  # noqa: T201

boto3.resource("s3").Object("releases.wagtail.io", "nightly/latest.json").put(
    Body=json.dumps(
        {
            "url": "https://releases.wagtail.org/nightly/dist/" + f.name,
        }
    ),
)

print("Purging cache")  # noqa: T201

boto3.client("cloudfront").create_invalidation(
    DistributionId="E283SZ5CB4MDM0",
    InvalidationBatch={
        "Paths": {
            "Quantity": 2,
            "Items": ["/nightly/latest.json", "/nightly/dist/latest.whl"],
        },
        "CallerReference": str(uuid.uuid4()),
    },
)