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

Code before:
    window.HTMLFormElement.prototype.requestSubmit = null; // mock not being available in a browser
    window.HTMLFormElement.prototype.submit = submit;

    const select = document.querySelector('select');
    select.value = 'Z-A';
    select.dispatchEvent(new CustomEvent('change'));

    expect(submit).toHaveBeenCalled();
    expect(lastFormCalled).toEqual(document.getElementById('form'));
  });

  it('should throw an error if there is no form associated with the controlled element', async () => {
    const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

    document.body.innerHTML = `
    <div id="form">
      <input type="text" data-controller="w-submit" data-action="change->w-submit#submit" />
    </div>`;

    await Promise.resolve(); // wait for the controller to initialize

    expect(errorSpy).not.toHaveBeenCalled();

    const input = document.querySelector('input');

    document.querySelector('input').dispatchEvent(new CustomEvent('change'));

    expect(errorSpy).toHaveBeenCalledTimes(3);
    const [[, message, error]] = errorSpy.mock.calls;

    expect(message).toEqual('Error invoking action "change->w-submit#submit"');
    expect(error.message).toEqual(
      'w-submit controlled element must be part of a <form />',
    );

    errorSpy.mockRestore();
  });
});


Review comment (suggestion):
```suggestion
    input.dispatchEvent(new CustomEvent('change'));
```

Code after:
    window.HTMLFormElement.prototype.requestSubmit = null; // mock not being available in a browser
    window.HTMLFormElement.prototype.submit = submit;

    const select = document.querySelector('select');
    select.value = 'Z-A';
    select.dispatchEvent(new CustomEvent('change'));

    expect(submit).toHaveBeenCalled();
    expect(lastFormCalled).toEqual(document.getElementById('form'));
  });

  it('should throw an error if there is no form associated with the controlled element', async () => {
    const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

    document.body.innerHTML = `
    <div id="form">
      <input type="text" data-controller="w-submit" data-action="change->w-submit#submit" />
    </div>`;

    await Promise.resolve(); // wait for the controller to initialize

    expect(errorSpy).not.toHaveBeenCalled();

    const input = document.querySelector('input');

    input.dispatchEvent(new CustomEvent('change'));

    expect(errorSpy).toHaveBeenCalledTimes(3);
    const [[, message, error]] = errorSpy.mock.calls;

    expect(message).toEqual('Error invoking action "change->w-submit#submit"');
    expect(error.message).toEqual(
      'w-submit controlled element must be part of a <form />',
    );

    errorSpy.mockRestore();
  });
});
Repository: wagtail/wagtail
File: client/src/controllers/AutosizeController.stories.js (JavaScript)

Code before:
import { AutosizeController } from './AutosizeController';

export default {
  title: 'Stimulus / AutosizeController',
  argTypes: {
    children: {
      control: 'text',
      defaultValue: '',
    },
    debug: {
      control: 'boolean',
      defaultValue: false,
    },
  },
};

const definitions = [
  {
    identifier: 'w-autosize',
    controllerConstructor: AutosizeController,
  },
];

const Template = ({ children = '', debug = false }) => (
  <StimulusWrapper debug={debug} definitions={definitions}>
    <textarea data-controller="w-autosize">{children}</textarea>
  </StimulusWrapper>
);

export const EmptyTextarea = Template.bind({});

export const FilledTextarea = Template.bind({});

FilledTextarea.args = {
  children:
    'This is a filled textarea, it should autosize to fit its content. Wagtail ipsum dolor sit flap, perching by the riverside with elegant wiggle. Tiny steps, gentle dips, and endless tail-bobs bring joy to every puddle. Curabitur chirp elit, sed flutter magna finibus et song. Praesent feathers shimmer in early light; nullam breeze dapibus whisper across soft meadow moss. Etiam wagtailus curious, hopping between stones and stories, always following ripples where sunlight dances.',
};


Review comment (suggestion):
I get React warnings that say we should use `defaultValue` or `value` props instead of setting the children of the `<textarea>`:

<img width="959" height="286" alt="Image" src="https://github.com/user-attachments/assets/8d67886b-9cc4-40e1-8f1a-16599610498b" />


```suggestion
    <textarea data-controller="w-autosize" defaultValue={children}></textarea>
```

(We should rename `children` accordingly, I think)

Code after:
import { AutosizeController } from './AutosizeController';

export default {
  title: 'Stimulus / AutosizeController',
  argTypes: {
    value: {
      control: 'text',
      defaultValue: '',
    },
    debug: {
      control: 'boolean',
      defaultValue: false,
    },
  },
};

const definitions = [
  {
    identifier: 'w-autosize',
    controllerConstructor: AutosizeController,
  },
];

const Template = ({ value = '', debug = false }) => (
  <StimulusWrapper debug={debug} definitions={definitions}>
    <textarea data-controller="w-autosize" defaultValue={value} />
  </StimulusWrapper>
);

export const EmptyTextarea = Template.bind({});

export const FilledTextarea = Template.bind({});

FilledTextarea.args = {
  value:
    'This is a filled textarea, it should autosize to fit its content. Wagtail ipsum dolor sit flap, perching by the riverside with elegant wiggle. Tiny steps, gentle dips, and endless tail-bobs bring joy to every puddle. Curabitur chirp elit, sed flutter magna finibus et song. Praesent feathers shimmer in early light; nullam breeze dapibus whisper across soft meadow moss. Etiam wagtailus curious, hopping between stones and stories, always following ripples where sunlight dances.',
};
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,
)

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": len(2),
            "Items": ["/nightly/latest.json", "nightly/dist/" + f.name],
        },
        "CallerReference": str(uuid.uuid4()),
    },
)


Review comment (suggestion):
```suggestion
            "Quantity": 2,
```

Or store the items in a list and do `len(items)`

Code after:

print("Uploading", f.name)  # noqa: T201
s3 = boto3.client("s3")
s3.upload_file(
    str(f),
    "releases.wagtail.io",
    "nightly/dist/" + f.name,
)

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": 1,
            "Items": ["/nightly/latest.json"],
        },
        "CallerReference": str(uuid.uuid4()),
    },
)
Repository: wagtail/wagtail
File: wagtail/test/testapp/models.py (Python)

Code before:


register_snippet(CustomPermissionModel)


class RequiredDatePage(Page):
    deadline = models.DateField()

    content_panels = [
        TitleFieldPanel("title", classname="title"),
        FieldPanel("deadline"),
    ]


class CommentableJSONPage(Page):
    commentable_body = CommentableJSONField()
    uncommentable_body = models.JSONField()
    stream_body = StreamField(
        [
            ("text", CharBlock()),
        ]
    )


@register_snippet
class Book(ClusterableModel):
    name = models.CharField(default="Book Name", max_length=255)
    page = models.ForeignKey(
        SimplePage, on_delete=models.CASCADE, null=True, blank=True, related_name="+"
    )
    category = ParentalKey(
        "BookCategory",
        related_name="books",
        on_delete=models.CASCADE,
        blank=True,
        null=True,
        default=None,
    )


@register_snippet
class BookRelationShip(models.Model):
    name = models.CharField(default="Book Name", max_length=255)
    book = ParentalKey(Book, related_name="book_relationship", on_delete=models.CASCADE)
    page = models.ForeignKey(
        SimplePage, blank=True, null=True, on_delete=models.CASCADE
    )


@register_snippet
class BookCategory(ClusterableModel):

Review comment (suggestion):
I wonder if we can reuse some of the existing models, e.g. `EventPageSpeakerAward`, so that we don't add three new models just for testing this.

Code after:
        permissions = [
            # Starts with can_ and "Can "
            ("can_start_trouble", "Can start trouble"),
            # Doesn't start with can_ and "Can "
            ("cause_chaos", "Cause chaos for advanced permission model"),
            # Starts with an action similar to a built-in permission "change_"
            ("change_text", "Change text"),
            # Without any _ and the label ends with the default verbose_name
            ("control", "Manage custom permission model"),
        ]


register_snippet(CustomPermissionModel)


class RequiredDatePage(Page):
    deadline = models.DateField()

    content_panels = [
        TitleFieldPanel("title", classname="title"),
        FieldPanel("deadline"),
    ]


class CommentableJSONPage(Page):
    commentable_body = CommentableJSONField()
    uncommentable_body = models.JSONField()
    stream_body = StreamField(
        [
            ("text", CharBlock()),
        ]
    )
Repository: webpack/webpack
File: lib/cli.js (JavaScript)

Code before:
		const newPath = path.split("/");

		let schemaPart = schema;

		for (let i = 1; i < newPath.length; i++) {
			const inner = schemaPart[newPath[i]];

			if (!inner) {
				break;
			}

			schemaPart = inner;
		}

		return schemaPart;
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} description
	 */
	const getDescription = path => {
		for (const { schema } of path) {
			if (schema.cli && schema.cli.helper) continue;
			if (schema.cliDescription) return schema.cliDescription;
			if (schema.description) return schema.description;
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} negative description
	 */
	const getNegatedDescription = path => {
		for (const { schema } of path) {
			if (schema.cli && schema.cli.helper) continue;
			if (schema.negatedDescription) return schema.negatedDescription;
		}
	};

	/**
	 *
	 * @param {any} schemaPart schema
	 * @returns {Pick<ArgumentConfig, "type"|"values">} partial argument config
	 */
	const schemaToArgumentConfig = schemaPart => {
		if (schemaPart.enum) {
			return {
				type: "enum",

Review comment (suggestion):
since this is cli specific this should be moved into `schema.cli.description`

Code after:
		const newPath = path.split("/");

		let schemaPart = schema;

		for (let i = 1; i < newPath.length; i++) {
			const inner = schemaPart[newPath[i]];

			if (!inner) {
				break;
			}

			schemaPart = inner;
		}

		return schemaPart;
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} description
	 */
	const getDescription = path => {
		for (const { schema } of path) {
			if (schema.cli) {
				if (schema.cli.helper) continue;
				if (schema.cli.description) return schema.cli.description;
			}
			if (schema.description) return schema.description;
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} negative description
	 */
	const getNegatedDescription = path => {
		for (const { schema } of path) {
			if (schema.cli) {
				if (schema.cli.helper) continue;
				if (schema.cli.negatedDescription) return schema.cli.negatedDescription;
			}
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} reset description
	 */
Repository: webpack/webpack
File: lib/cli.js (JavaScript)

Code before:

		return schemaPart;
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} description
	 */
	const getDescription = path => {
		for (const { schema } of path) {
			if (schema.cli && schema.cli.helper) continue;
			if (schema.cliDescription) return schema.cliDescription;
			if (schema.description) return schema.description;
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} negative description
	 */
	const getNegatedDescription = path => {
		for (const { schema } of path) {
			if (schema.cli && schema.cli.helper) continue;
			if (schema.negatedDescription) return schema.negatedDescription;
		}
	};

	/**
	 *
	 * @param {any} schemaPart schema
	 * @returns {Pick<ArgumentConfig, "type"|"values">} partial argument config
	 */
	const schemaToArgumentConfig = schemaPart => {
		if (schemaPart.enum) {
			return {
				type: "enum",
				values: schemaPart.enum
			};
		}
		switch (schemaPart.type) {
			case "number":
				return {
					type: "number"
				};
			case "string":
				return {
					type: schemaPart.absolutePath ? "path" : "string"
				};
			case "boolean":

Review comment (suggestion):
```suggestion
			if (schema.cli.negatedDescription) return schema.cli.negatedDescription;
```

Code after:

		return schemaPart;
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} description
	 */
	const getDescription = path => {
		for (const { schema } of path) {
			if (schema.cli) {
				if (schema.cli.helper) continue;
				if (schema.cli.description) return schema.cli.description;
			}
			if (schema.description) return schema.description;
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} negative description
	 */
	const getNegatedDescription = path => {
		for (const { schema } of path) {
			if (schema.cli) {
				if (schema.cli.helper) continue;
				if (schema.cli.negatedDescription) return schema.cli.negatedDescription;
			}
		}
	};

	/**
	 *
	 * @param {PathItem[]} path path in the schema
	 * @returns {string | undefined} reset description
	 */
	const getResetDescription = path => {
		for (const { schema } of path) {
			if (schema.cli) {
				if (schema.cli.helper) continue;
				if (schema.cli.resetDescription) return schema.cli.resetDescription;
			}
		}
	};

	/**
	 *
	 * @param {any} schemaPart schema
	 * @returns {Pick<ArgumentConfig, "type"|"values">} partial argument config
Repository: webpack/webpack
File: lib/css/CssGenerator.js (JavaScript)

Code before:
			return JS_AND_CSS_EXPORT_TYPES;
		}
		const sourceTypes = new Set();
		const connections = this._moduleGraph.getIncomingConnections(module);
		for (const connection of connections) {
			if (!connection.originModule) {
				continue;
			}
			sourceTypes.add(connection.originModule.type.split("/")[0]);
		}
		if (sourceTypes.has("javascript")) {
			return JS_AND_CSS_TYPES;
		}
		return CSS_TYPES;
	}

	/**
	 * @param {NormalModule} module the module
	 * @param {string=} type source type
	 * @returns {number} estimate size of the module
	 */
	getSize(module, type) {
		switch (type) {
			case "javascript": {
				const cssData = module.buildInfo.cssData;
				if (!cssData) {
					return 42;
				}
				if (cssData.exports.size === 0) {
					if (module.buildMeta.isCSSModule) {
						return 42;
					}
					return 0;
				}
				const exports = cssData.exports;
				const stringifiedExports = JSON.stringify(
					Array.from(exports).reduce((obj, [key, value]) => {
						obj[key] = value;
						return obj;
					}, {})
				);

				return stringifiedExports.length + 42;
			}
			case "css": {
				const originalSource = module.originalSource();

				if (!originalSource) {
					return 0;
				}


Review comment (suggestion):
How about we detect if all source types are CSS related types, I don't know if lazy proxied module can have connection to css after sideEffectPlugin optimization

Code after:
				return null;
		}
	}

	/**
	 * @param {NormalModule} module fresh module
	 * @returns {SourceTypes} available types (do not mutate)
	 */
	getTypes(module) {
		// TODO, find a better way to prevent the original module from being removed after concatenation, maybe it is a bug
		if (this.exportsOnly) {
			return JS_AND_CSS_EXPORT_TYPES;
		}
		const sourceTypes = new Set();
		const connections = this._moduleGraph.getIncomingConnections(module);
		for (const connection of connections) {
			if (!connection.originModule) {
				continue;
			}
			if (connection.originModule.type.split("/")[0] !== CSS_TYPE)
				sourceTypes.add(JS_TYPE);
		}
		if (sourceTypes.has(JS_TYPE)) {
			return JS_AND_CSS_TYPES;
		}
		return CSS_TYPES;
	}

	/**
	 * @param {NormalModule} module the module
	 * @param {string=} type source type
	 * @returns {number} estimate size of the module
	 */
	getSize(module, type) {
		switch (type) {
			case "javascript": {
				const cssData = /** @type {BuildInfo} */ (module.buildInfo).cssData;
				if (!cssData) {
					return 42;
				}
				if (cssData.exports.size === 0) {
					if (/** @type {BuildMeta} */ (module.buildMeta).isCSSModule) {
						return 42;
					}
					return 0;
				}
				const exports = cssData.exports;
				const stringifiedExports = JSON.stringify(
					Array.from(exports).reduce((obj, [key, value]) => {
						obj[key] = value;
						return obj;
Repository: webpack/webpack
File: lib/ConstPlugin.js (JavaScript)

Code before:
		//         var x, {y, z} = obj
		//       }
		//     } else {
		//       …
		//     }
		//
		// the generated code is:
		//
		//     if (true) f() else {}
		//     if (false) {
		//       var f, x, y, z;   (in loose mode)
		//       var x, y, z;      (in strict mode)
		//     } else {
		//       …
		//     }
		//
		// NOTE: When code runs in strict mode, `var` declarations
		// are hoisted but `function` declarations don't.
		//
		const declarations = parser.scope.isStrict
			? getHoistedDeclarations(statement, false)
			: getHoistedDeclarations(statement, true);
		const replacement =
			declarations.length > 0 ? `{ var ${declarations.join(", ")}; }` : "{}";
		const dep = new ConstDependency(
			replacement,
			/** @type {Range} */ (statement.range)
		);
		dep.loc = /** @type {SourceLocation} */ (statement.loc);
		parser.state.module.addPresentationalDependency(dep);
	}
}

module.exports = ConstPlugin;


Review comment (bug):
Let's add a comment `// removed by dead control flow\n${replacement}`  (maybe better) for debug reasons

Code after:
		//         var x, {y, z} = obj
		//       }
		//     } else {
		//       …
		//     }
		//
		// the generated code is:
		//
		//     if (true) f() else {}
		//     if (false) {
		//       var f, x, y, z;   (in loose mode)
		//       var x, y, z;      (in strict mode)
		//     } else {
		//       …
		//     }
		//
		// NOTE: When code runs in strict mode, `var` declarations
		// are hoisted but `function` declarations don't.
		//
		const declarations = parser.scope.isStrict
			? getHoistedDeclarations(statement, false)
			: getHoistedDeclarations(statement, true);
		const replacement =
			declarations.length > 0 ? `{ var ${declarations.join(", ")}; }` : "{}";
		const dep = new ConstDependency(
			`// removed by dead control flow\n${replacement}`,
			/** @type {Range} */ (statement.range)
		);
		dep.loc = /** @type {SourceLocation} */ (statement.loc);
		parser.state.module.addPresentationalDependency(dep);
	}
}

module.exports = ConstPlugin;
Repository: webpack/webpack
File: lib/CleanPlugin.js (JavaScript)

Code before:
							if (err) {
								oldAssets = undefined;
							} else {
								if (oldAssets) mergeAssets(currentAssets, oldAssets);
								oldAssets = currentAssets;
								if (keptAssets) mergeAssets(oldAssets, keptAssets);
							}
							callback(err);
						}
					);
				};

				if (oldAssets) {
					diffCallback(null, getDiffToOldAssets(currentAssets, oldAssets));
				} else {
					getDiffToFs(fs, outputPath, currentAssets, diffCallback);
				}
			}
		);
	}
}

module.exports = CleanPlugin;
module.exports.getDirectories = getDirectories;
module.exports.hasFile = hasFile;
module.exports.isEqualPath = isEqualPath;


Review comment (suggestion):
Let's implement them as a plugin methods (using `_` before method, i.e. `_isEqualPath` and for other too), don't want to make them as a part of public API

Code after:
							if (err) {
								oldAssets = undefined;
							} else {
								if (oldAssets) mergeAssets(currentAssets, oldAssets);
								oldAssets = currentAssets;
								if (keptAssets) mergeAssets(oldAssets, keptAssets);
							}
							callback(err);
						}
					);
				};

				if (oldAssets) {
					diffCallback(null, getDiffToOldAssets(currentAssets, oldAssets));
				} else {
					getDiffToFs(fs, outputPath, currentAssets, diffCallback);
				}
			}
		);
	}
}

module.exports = CleanPlugin;
module.exports._getDirectories = getDirectories;
module.exports._hasFile = hasFile;
module.exports._isEqualPath = isEqualPath;
Repository: webpack/webpack
File: lib/Module.js (JavaScript)

Code before:
	}

	/**
	 * @returns {string | null} absolute path which should be used for condition matching (usually the resource path)
	 */
	nameForCondition() {
		return null;
	}

	/**
	 * @param {ConcatenationBailoutReasonContext} context context
	 * @returns {string | undefined} reason why this module can't be concatenated, undefined when it can be concatenated
	 */
	getConcatenationBailoutReason(context) {
		return `Module Concatenation is not implemented for ${this.constructor.name}`;
	}

	/**
	 * @param {ModuleGraph} moduleGraph the module graph
	 * @returns {ConnectionState} how this module should be connected to referencing modules when consumed for side-effects only
	 */
	getSideEffectsConnectionState(moduleGraph) {
		if (this.factoryMeta !== undefined) {
			if (this.factoryMeta.sideEffectFree) return false;
			if (this.factoryMeta.sideEffectFree === false) return true;
		}
		return true;
	}

	/**
	 * @param {CodeGenerationContext} context context for code generation
	 * @returns {CodeGenerationResult} result
	 */
	codeGeneration(context) {
		// Best override this method
		const sources = new Map();
		for (const type of this.getSourceTypes()) {
			if (type !== "unknown") {
				sources.set(
					type,
					this.source(
						context.dependencyTemplates,
						context.runtimeTemplate,
						type
					)
				);
			}
		}
		return {
			sources,
			runtimeRequirements: new Set([

Review comment (suggestion):
Please put this login inside `RawModule`, otherwise it is potential breaking changes for plugins

Code after:
	}

	/**
	 * @returns {string | null} absolute path which should be used for condition matching (usually the resource path)
	 */
	nameForCondition() {
		return null;
	}

	/**
	 * @param {ConcatenationBailoutReasonContext} context context
	 * @returns {string | undefined} reason why this module can't be concatenated, undefined when it can be concatenated
	 */
	getConcatenationBailoutReason(context) {
		return `Module Concatenation is not implemented for ${this.constructor.name}`;
	}

	/**
	 * @param {ModuleGraph} moduleGraph the module graph
	 * @returns {ConnectionState} how this module should be connected to referencing modules when consumed for side-effects only
	 */
	getSideEffectsConnectionState(moduleGraph) {
		return true;
	}

	/**
	 * @param {CodeGenerationContext} context context for code generation
	 * @returns {CodeGenerationResult} result
	 */
	codeGeneration(context) {
		// Best override this method
		const sources = new Map();
		for (const type of this.getSourceTypes()) {
			if (type !== "unknown") {
				sources.set(
					type,
					this.source(
						context.dependencyTemplates,
						context.runtimeTemplate,
						type
					)
				);
			}
		}
		return {
			sources,
			runtimeRequirements: new Set([
				RuntimeGlobals.module,
				RuntimeGlobals.exports,
				RuntimeGlobals.require
			])