ronantakizawa/codereview-bench huggingface dataset:
Repository: fastify/fastify
File: lib/contentTypeParser.js (JavaScript)

Code before:
  const resource = new AsyncResource('content-type-parser:run', request)
  const done = resource.bind(onDone)

  if (parser.asString === true || parser.asBuffer === true) {
    rawBody(
      request,
      reply,
      reply[kRouteContext]._parserOptions,
      parser,
      done
    )
    return
  }

  const result = parser.fn(request, request[kRequestPayloadStream], done)
  if (result && typeof result.then === 'function') {
    result.then(body => { done(null, body) }, done)
  }

  function onDone (error, body) {
    resource.emitDestroy()
    if (error != null) {
      // We must close the connection as the client may
      // send more data
      reply.header('connection', 'close')
      reply[kReplyIsError] = true
      reply.send(error)
      return
    }
    request.body = body
    handler(request, reply)
  }
}

function rawBody (request, reply, options, parser, done) {
  const asString = parser.asString === true
  const limit = options.limit === null ? parser.bodyLimit : options.limit
  const contentLength = Number(request.headers['content-length'])

  if (contentLength > limit) {
    done(new FST_ERR_CTP_BODY_TOO_LARGE(), undefined)
    return
  }

  let receivedLength = 0
  let body = asString ? '' : []
  const payload = request[kRequestPayloadStream] || request.raw

  if (asString) {
    payload.setEncoding('utf8')
  }

Review comment (suggestion):
I need a review for this line

I think we should send the `Connection: close` header in all exceptions during parsing

My reasoning is the same as in the `FST_ERR_CTP_BODY_TOO_LARGE` case: the client might continue to send data after we reject

Code after:

  const resource = new AsyncResource('content-type-parser:run', request)
  const done = resource.bind(onDone)

  if (parser.asString === true || parser.asBuffer === true) {
    rawBody(
      request,
      reply,
      reply[kRouteContext]._parserOptions,
      parser,
      done
    )
    return
  }

  const result = parser.fn(request, request[kRequestPayloadStream], done)
  if (result && typeof result.then === 'function') {
    result.then(body => { done(null, body) }, done)
  }

  function onDone (error, body) {
    resource.emitDestroy()
    if (error != null) {
      // We must close the connection as the client may
      // send more data
      reply.header('connection', 'close')
      reply[kReplyIsError] = true
      reply.send(error)
      return
    }
    request.body = body
    handler(request, reply)
  }
}

function rawBody (request, reply, options, parser, done) {
  const asString = parser.asString === true
  const limit = options.limit === null ? parser.bodyLimit : options.limit
  const contentLength = Number(request.headers['content-length'])

  if (contentLength > limit) {
    done(new FST_ERR_CTP_BODY_TOO_LARGE(), undefined)
    return
  }

  let receivedLength = 0
  let body = asString ? '' : []
  const payload = request[kRequestPayloadStream] || request.raw

  if (asString) {
    payload.setEncoding('utf8')
Repository: fastify/fastify
File: test/issue-4959.test.js (JavaScript)

Code before:

  const req = http.request({
    ...reqOptions,
    headers: {
      'Content-Type': 'application/json',
      'Content-Length': Buffer.byteLength(postData)
    }
  }, () => {
    innerReject(new Error('Request should have failed'))
  })

  // Kill the socket immediately (before sending data)
  req.on('socket', (socket) => {
    socket.on('connect', () => {
      setTimeout(() => { socket.destroy() }, 0)
    })
  })
  req.on('error', innerResolve)
  req.write(postData)
  req.end()

  return promise
}

test('should handle a socket error', async (t) => {
  const fastify = Fastify()

  function shouldNotHappen () {
    t.assert.fail('This should not happen')
  }
  process.on('unhandledRejection', shouldNotHappen)

  t.after(() => {
    fastify.close()
    process.removeListener('unhandledRejection', shouldNotHappen)
  })

  fastify.addHook('onRequest', async (request, reply) => {
    t.assert.ok('onRequest hook called')
  })

  fastify.addHook('onSend', async (request, reply, payload) => {
    if (request.onSendCalled) {
      t.assert.fail('onSend hook called more than once')
      return
    }

    t.assert.ok('onSend hook called')
    request.onSendCalled = true

    // Introduce a delay

Review comment (bug):
Removed `t.plan(4)` because a race condition makes assertion counts inconsistent
This dropped the strict plan keeps the test reliable while still validating error handling


Code after:

  const req = http.request({
    ...reqOptions,
    headers: {
      'Content-Type': 'application/json',
      'Content-Length': Buffer.byteLength(postData)
    }
  }, () => {
    innerReject(new Error('Request should have failed'))
  })

  // Kill the socket after the request has been fully written.
  // Destroying it on `connect` can race before any bytes are sent, making the
  // server-side assertions (hooks/handler) non-deterministic.
  //
  // To keep the test deterministic, we optionally wait for a server-side signal
  // (e.g. onSend entered) before aborting the client.
  let socket
  req.on('socket', (s) => { socket = s })
  req.on('finish', () => {
    if (waitBeforeDestroy && typeof waitBeforeDestroy.then === 'function') {
      Promise.race([
        waitBeforeDestroy,
        new Promise(resolve => setTimeout(resolve, 200))
      ]).then(() => {
        if (socket) socket.destroy()
      }, innerResolve)
      return
    }
    setTimeout(() => { socket.destroy() }, 0)
  })
  req.on('error', innerResolve)
  req.write(postData)
  req.end()

  return promise
}

test('should handle a socket error', async (t) => {
  t.plan(4)
  const fastify = Fastify()

  let resolveOnSendEntered
  const onSendEntered = new Promise((resolve) => {
    resolveOnSendEntered = resolve
  })

  function shouldNotHappen () {
    t.assert.fail('This should not happen')
  }
  process.on('unhandledRejection', shouldNotHappen)
Repository: fastify/fastify
File: test/router-options.test.js (JavaScript)

Code before:
  // Support semicolon delimiter
  let res = await fastify.inject({
    method: 'GET',
    url: '/test;foo=bar&baz=faz'
  })
  t.assert.strictEqual(res.statusCode, 200)

  // Support query string `?` delimiter
  res = await fastify.inject({
    method: 'GET',
    url: '/test?foo=bar&baz=faz'
  })
  t.assert.strictEqual(res.statusCode, 200)
})

test('Should support extra find-my-way options', t => {
  t.plan(1)
  // allowHeaderParam: example upstream router option
  const fastify = Fastify({
    routerOptions: {
      allowHeaderParam: true
    }
  })
  // Ensure the option is preserved after validation
  t.assert.strictEqual(fastify.initialConfig.routerOptions.allowHeaderParam, true)
})


Review comment (suggestion):
The `allowHeaderParam` does not exist

```suggestion
test('Should support extra find-my-way options', async t => {
  t.plan(1)
  // allowHeaderParam: example upstream router option
  const fastify = Fastify({
    routerOptions: {
      buildPrettyMeta: (route) => {
        const cleanMeta = Object.assign({}, route.store)
        return cleanMeta
      }
    }
  })

  await fastify.ready()

  // Ensure the option is preserved after validation
  t.assert.strictEqual(typeof fastify.initialConfig.routerOptions.buildPrettyMeta, 'function')
})
```

Code after:
  // Support semicolon delimiter
  let res = await fastify.inject({
    method: 'GET',
    url: '/test;foo=bar&baz=faz'
  })
  t.assert.strictEqual(res.statusCode, 200)

  // Support query string `?` delimiter
  res = await fastify.inject({
    method: 'GET',
    url: '/test?foo=bar&baz=faz'
  })
  t.assert.strictEqual(res.statusCode, 200)
})

test('Should support extra find-my-way options', async t => {
  t.plan(1)
  // Use a real upstream option from find-my-way
  const fastify = Fastify({
    routerOptions: {
      buildPrettyMeta: (route) => {
        const cleanMeta = Object.assign({}, route.store)
        return cleanMeta
      }
    }
  })

  await fastify.ready()

  // Ensure the option is preserved after validation
  t.assert.strictEqual(typeof fastify.initialConfig.routerOptions.buildPrettyMeta, 'function')
})
Repository: fastify/fastify
File: lib/handle-request.js (JavaScript)

Code before:
    const store = {
      request,
      reply,
      async: false,
      route: {
        url: context.config.url,
        method: context.config.method
      }
    }
    channels.start.runStores(store, preHandlerCallbackInner, undefined, err, request, reply, store)
  }
}

function preHandlerCallbackInner (err, request, reply, store) {
  const context = request[kRouteContext]

  try {
    if (err != null) {
      reply[kReplyIsError] = true
      if (store) {
        store.error = err
        // Set status code before publishing so subscribers see the correct value
        if (!reply[kReplyHasStatusCode] || reply.statusCode === 200) {
          const statusCode = err.statusCode || err.status
          reply.code(statusCode >= 400 ? statusCode : 500)
        }
        channels.error.publish(store)
      }
      reply.send(err)
      return
    }

    let result

    try {
      result = context.handler(request, reply)
    } catch (err) {
      if (store) {
        store.error = err
        // Set status code before publishing so subscribers see the correct value
        if (!reply[kReplyHasStatusCode] || reply.statusCode === 200) {
          const statusCode = err.statusCode || err.status
          reply.code(statusCode >= 400 ? statusCode : 500)
        }
        channels.error.publish(store)
      }

      reply[kReplyIsError] = true
      reply.send(err)
      return
    }

Review comment (suggestion):
We have this logic in 4 places, I wonder if it is better to isolate it into a Reply.js function

```js
function verifyAndUpdateHTTPResponseStatusCode(reply) {
  if (!reply[kReplyHasStatusCode] || reply.statusCode === 200) {
    const statusCode = err.statusCode || err.status
    reply.code(statusCode >= 400 ? statusCode : 500)
  }
}
```

Code after:
    const store = {
      request,
      reply,
      async: false,
      route: {
        url: context.config.url,
        method: context.config.method
      }
    }
    channels.start.runStores(store, preHandlerCallbackInner, undefined, err, request, reply, store)
  }
}

function preHandlerCallbackInner (err, request, reply, store) {
  const context = request[kRouteContext]

  try {
    if (err != null) {
      reply[kReplyIsError] = true
      if (store) {
        store.error = err
        // Set status code before publishing so subscribers see the correct value
        setErrorStatusCode(reply, err)
        channels.error.publish(store)
      }
      reply.send(err)
      return
    }

    let result

    try {
      result = context.handler(request, reply)
    } catch (err) {
      if (store) {
        store.error = err
        // Set status code before publishing so subscribers see the correct value
        setErrorStatusCode(reply, err)
        channels.error.publish(store)
      }

      reply[kReplyIsError] = true
      reply.send(err)
      return
    }

    if (result !== undefined) {
      if (result !== null && typeof result.then === 'function') {
        wrapThenable(result, reply, store)
      } else {
        reply.send(result)
Repository: fastify/fastify
File: test/web-api.test.js (JavaScript)

Code before:
  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    const http = require('node:http')
    const req = http.get(`http://localhost:${fastify.server.address().port}`, (res) => {
      res.once('data', () => {
        req.destroy()
        setTimeout(() => {
          t.assert.strictEqual(readerCancelled, true)
          done()
        }, 50)
      })
    })
  })
})

test('WebStream should warn when headers already sent', async (t) => {
  t.plan(2)

  let warnCalled = false
  const spyLogger = {
    level: 'warn',
    fatal: () => { },
    error: () => { },
    warn: (msg) => {
      if (typeof msg === 'string' && msg.includes('writeHead')) {
        warnCalled = true
      }
    },
    info: () => { },
    debug: () => { },
    trace: () => { },
    child: () => spyLogger
  }

  const fastify = Fastify({ loggerInstance: spyLogger })
  t.after(() => fastify.close())

  fastify.get('/', function (request, reply) {
    reply.raw.writeHead(200, { 'content-type': 'text/plain' })
    const stream = new ReadableStream({
      start (controller) {
        controller.enqueue('hello')
        controller.close()
      }
    })
    reply.send(stream)
  })

  await fastify.listen({ port: 0 })


Review comment (suggestion):
Facultative:
```suggestion
      if (typeof msg === 'string' && msg.includes('use res.writeHead in stream mode')) {
```

Code after:
  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    const req = http.get(`http://localhost:${fastify.server.address().port}`, (res) => {
      res.once('data', () => {
        req.destroy()
        setTimeout(() => {
          t.assert.strictEqual(readerCancelled, true)
          done()
        }, 50)
      })
    })
  })
})

test('WebStream should warn when headers already sent', async (t) => {
  t.plan(2)

  let warnCalled = false
  const spyLogger = {
    level: 'warn',
    fatal: () => { },
    error: () => { },
    warn: (msg) => {
      if (typeof msg === 'string' && msg.includes('use res.writeHead in stream mode')) {
        warnCalled = true
      }
    },
    info: () => { },
    debug: () => { },
    trace: () => { },
    child: () => spyLogger
  }

  const fastify = Fastify({ loggerInstance: spyLogger })
  t.after(() => fastify.close())

  fastify.get('/', function (request, reply) {
    reply.raw.writeHead(200, { 'content-type': 'text/plain' })
    const stream = new ReadableStream({
      start (controller) {
        controller.enqueue('hello')
        controller.close()
      }
    })
    reply.send(stream)
  })

  await fastify.listen({ port: 0 })

  const response = await fetch(`http://localhost:${fastify.server.address().port}/`)
Repository: files-community/Files
File: src/Files.App/Views/Properties/MainPropertiesPage.xaml.cs (C#)

Code before:
using Windows.UI;
using Microsoft.UI.Input;

namespace Files.App.Views.Properties
{
	public sealed partial class MainPropertiesPage : BasePropertiesPage
	{
		private AppWindow AppWindow => Window.AppWindow;

		private Window Window;

		private SettingsViewModel AppSettings { get; set; }

		private MainPropertiesViewModel MainPropertiesViewModel { get; set; }

		public MainPropertiesPage()
		{
			InitializeComponent();

			if (FilePropertiesHelpers.FlowDirectionSettingIsRightToLeft)
				FlowDirection = FlowDirection.RightToLeft;
		}

		public bool TryNavigateToPage(PropertiesNavigationViewItemType pageType)
		{
			var page = MainPropertiesViewModel.NavigationViewItems.FirstOrDefault(item => item.ItemType == pageType);
			if (page is null)
				return false;

			MainPropertiesViewModel.SelectedNavigationViewItem = page;
			return true;
		}

		protected override void OnNavigatedTo(NavigationEventArgs e)
		{
			var parameter = (PropertiesPageNavigationParameter)e.Parameter;

			Window = parameter.Window;

			base.OnNavigatedTo(e);

			MainPropertiesViewModel = new(Window, MainContentFrame, BaseProperties, parameter);
		}

		private void Page_Loaded(object sender, RoutedEventArgs e)
		{
			AppSettings = Ioc.Default.GetRequiredService<SettingsViewModel>();
			AppSettings.ThemeModeChanged += AppSettings_ThemeModeChanged;
			Window.Closed += Window_Closed;

			UpdatePageLayout();

Review comment (suggestion):
```suggestion
		
		// Navigates to specified properties page
		public bool TryNavigateToPage(PropertiesNavigationViewItemType pageType)
```

Code after:
using Windows.UI;
using Microsoft.UI.Input;

namespace Files.App.Views.Properties
{
	public sealed partial class MainPropertiesPage : BasePropertiesPage
	{
		private AppWindow AppWindow => Window.AppWindow;

		private Window Window;

		private SettingsViewModel AppSettings { get; set; }

		private MainPropertiesViewModel MainPropertiesViewModel { get; set; }

		public MainPropertiesPage()
		{
			InitializeComponent();

			if (FilePropertiesHelpers.FlowDirectionSettingIsRightToLeft)
				FlowDirection = FlowDirection.RightToLeft;
		}

		
		// Navigates to specified properties page
		public bool TryNavigateToPage(PropertiesNavigationViewItemType pageType)
		{
			var page = MainPropertiesViewModel.NavigationViewItems.FirstOrDefault(item => item.ItemType == pageType);
			if (page is null)
				return false;

			MainPropertiesViewModel.SelectedNavigationViewItem = page;
			return true;
		}

		protected override void OnNavigatedTo(NavigationEventArgs e)
		{
			var parameter = (PropertiesPageNavigationParameter)e.Parameter;

			Window = parameter.Window;

			base.OnNavigatedTo(e);

			MainPropertiesViewModel = new(Window, MainContentFrame, BaseProperties, parameter);
		}

		private void Page_Loaded(object sender, RoutedEventArgs e)
		{
			AppSettings = Ioc.Default.GetRequiredService<SettingsViewModel>();
			AppSettings.ThemeModeChanged += AppSettings_ThemeModeChanged;
			Window.Closed += Window_Closed;
Repository: files-community/Files
File: src/Files.App/Actions/Content/Tags/OpenAllTaggedActions.cs (C#)

Code before:
		public string Label
			=> "OpenAllTaggedItems".GetLocalizedResource();

		public string Description
			=> "OpenAllTaggedItemsDescription".GetLocalizedResource();

		public RichGlyph Glyph
			=> new("\uE71D");

		public bool IsExecutable => 
			_pageContext.ShellPage is not null &&
			_tagsContext.TaggedItems.Any();

		public OpenAllTaggedActions()
		{
			_pageContext = Ioc.Default.GetRequiredService<IContentPageContext>();
			_tagsContext = Ioc.Default.GetRequiredService<ITagsContext>();

			_pageContext.PropertyChanged += Context_PropertyChanged;
			_tagsContext.PropertyChanged += Context_PropertyChanged;
		}

		public async Task ExecuteAsync()
		{
			var files = _tagsContext.TaggedItems.Where(item => !item.isFolder).Select(f => f.path);
			var folders = _tagsContext.TaggedItems.Where(item => item.isFolder).Select(f => f.path);

			foreach (var file in files)
			{
			    await NavigationHelpers.OpenPath(file, _pageContext.ShellPage!);
			}
			
			foreach (var folder in folders)
			{
			    await NavigationHelpers.OpenPath(folder, _pageContext.ShellPage!);
			}
		}

		private void Context_PropertyChanged(object? sender, PropertyChangedEventArgs e)
		{
			switch (e.PropertyName)
			{
				case nameof(IContentPageContext.ShellPage):
				case nameof(ITagsContext.TaggedItems):
					OnPropertyChanged(nameof(IsExecutable));
					break;
			}
		}
	}
}


Review comment (suggestion):
```suggestion
			var filePaths = _tagsContext.TaggedItems.Where(item => !item.isFolder).Select(f => f.path);
			var folderPaths = _tagsContext.TaggedItems.Where(item => item.isFolder).Select(f => f.path);
```

Code after:
		public string Label
			=> "OpenAllTaggedItems".GetLocalizedResource();

		public string Description
			=> "OpenAllTaggedItemsDescription".GetLocalizedResource();

		public RichGlyph Glyph
			=> new("\uE71D");

		public bool IsExecutable => 
			_pageContext.ShellPage is not null &&
			_tagsContext.TaggedItems.Any();

		public OpenAllTaggedActions()
		{
			_pageContext = Ioc.Default.GetRequiredService<IContentPageContext>();
			_tagsContext = Ioc.Default.GetRequiredService<ITagsContext>();

			_pageContext.PropertyChanged += Context_PropertyChanged;
			_tagsContext.PropertyChanged += Context_PropertyChanged;
		}

		public async Task ExecuteAsync()
		{
			var filePaths = _tagsContext.TaggedItems
				.Where(item => !item.isFolder)
				.Select(f => f.path)
				.ToList();
			
			var folderPaths = _tagsContext
				.TaggedItems
				.Where(item => item.isFolder)
				.Select(f => f.path)
				.ToList();
			
			await Task.WhenAll(filePaths.Select(path => NavigationHelpers.OpenPath(path, _pageContext.ShellPage!)));

			foreach (var path in folderPaths) 
				await NavigationHelpers.OpenPathInNewTab(path, false);
		}

		private void Context_PropertyChanged(object? sender, PropertyChangedEventArgs e)
		{
			switch (e.PropertyName)
			{
				case nameof(IContentPageContext.ShellPage):
				case nameof(ITagsContext.TaggedItems):
					OnPropertyChanged(nameof(IsExecutable));
					break;
			}
		}
Repository: files-community/Files
File: src/Files.App/Actions/Content/Tags/OpenAllTaggedActions.cs (C#)

Code before:
		public bool IsExecutable => 
			_pageContext.ShellPage is not null &&
			_tagsContext.TaggedItems.Any();

		public OpenAllTaggedActions()
		{
			_pageContext = Ioc.Default.GetRequiredService<IContentPageContext>();
			_tagsContext = Ioc.Default.GetRequiredService<ITagsContext>();

			_pageContext.PropertyChanged += Context_PropertyChanged;
			_tagsContext.PropertyChanged += Context_PropertyChanged;
		}

		public async Task ExecuteAsync()
		{
			var filePaths = _tagsContext.TaggedItems.Where(item => !item.isFolder).Select(f => f.path);
			var folderPaths = _tagsContext.TaggedItems.Where(item => item.isFolder).Select(f => f.path);

			foreach (var file in filePaths)
			{
			    await NavigationHelpers.OpenPath(file, _pageContext.ShellPage!);
			}
			
			foreach (var folder in folderPaths)
			{
			    await NavigationHelpers.OpenPathInNewTab(folder,false);
			}
		}

		private void Context_PropertyChanged(object? sender, PropertyChangedEventArgs e)
		{
			switch (e.PropertyName)
			{
				case nameof(IContentPageContext.ShellPage):
				case nameof(ITagsContext.TaggedItems):
					OnPropertyChanged(nameof(IsExecutable));
					break;
			}
		}
	}
}


Review comment (refactor):
```suggestion
			    await NavigationHelpers.OpenPathInNewTab(folder, false);
```

Code after:
		public bool IsExecutable => 
			_pageContext.ShellPage is not null &&
			_tagsContext.TaggedItems.Any();

		public OpenAllTaggedActions()
		{
			_pageContext = Ioc.Default.GetRequiredService<IContentPageContext>();
			_tagsContext = Ioc.Default.GetRequiredService<ITagsContext>();

			_pageContext.PropertyChanged += Context_PropertyChanged;
			_tagsContext.PropertyChanged += Context_PropertyChanged;
		}

		public async Task ExecuteAsync()
		{
			var filePaths = _tagsContext.TaggedItems
				.Where(item => !item.isFolder)
				.Select(f => f.path)
				.ToList();
			
			var folderPaths = _tagsContext
				.TaggedItems
				.Where(item => item.isFolder)
				.Select(f => f.path)
				.ToList();
			
			await Task.WhenAll(filePaths.Select(path => NavigationHelpers.OpenPath(path, _pageContext.ShellPage!)));

			foreach (var path in folderPaths) 
				await NavigationHelpers.OpenPathInNewTab(path, false);
		}

		private void Context_PropertyChanged(object? sender, PropertyChangedEventArgs e)
		{
			switch (e.PropertyName)
			{
				case nameof(IContentPageContext.ShellPage):
				case nameof(ITagsContext.TaggedItems):
					OnPropertyChanged(nameof(IsExecutable));
					break;
			}
		}
	}
}