ronantakizawa/codereview-bench huggingface dataset:
Repository: tikv/tikv
File: components/resolved_ts/tests/failpoints/mod.rs (Rust)

Code before:
    fail::cfg("resolved_ts_after_handle_change_log", "pause").unwrap();
    // Reduce memory quota to force quota-exceeded behavior.
    suite.must_change_memory_quota(1, 10 * 1024); // 10 KB
    for i in 1..300 {
        prewrite(&mut suite, region.id, "a", i);
    }

    let pending_bytes = resolved_ts::RTS_CHANNEL_PENDING_CMD_BYTES.get();
    assert!(
        pending_bytes > 10 * 1024,
        "pending_bytes: {}",
        pending_bytes
    );

    // Removing the pause allows the active memory quota check to run and causes
    // the resolved-ts service to re-register regions. Because we set a long
    // backoff, the initial scans won't start immediately, so diagnosis requests
    // should report zero locks.
    fail::remove("resolved_ts_after_handle_change_log");
    let num_locks = must_get_num_locks(&suite, region.id);
    assert_eq!(num_locks, 0, "num_locks: {}", num_locks);
    let num_locks = must_get_num_locks(&suite, region_b.id);
    assert_eq!(num_locks, 0, "num_locks: {}", num_locks);

    let pending_bytes = resolved_ts::RTS_CHANNEL_PENDING_CMD_BYTES.get();
    assert!(pending_bytes == 0, "pending_bytes: {}", pending_bytes);

    suite.stop();
}


Review comment (bug):
The comparison `pending_bytes == 0` should use `assert_eq!` for consistency with the rest of the test assertions and to provide better error messages if it fails. The current form with `assert!` would just print "assertion failed: pending_bytes == 0" without showing the actual value.
```suggestion
    assert_eq!(pending_bytes, 0, "pending_bytes: {}", pending_bytes);
```

Code after:
    fail::cfg("resolved_ts_after_handle_change_log", "pause").unwrap();
    // Reduce memory quota to force quota-exceeded behavior.
    suite.must_change_memory_quota(1, 10 * 1024); // 10 KB
    for i in 1..300 {
        prewrite(&mut suite, region.id, "a", i);
    }

    let pending_bytes = resolved_ts::RTS_CHANNEL_PENDING_CMD_BYTES.get();
    assert!(
        pending_bytes > 10 * 1024,
        "pending_bytes: {}",
        pending_bytes
    );

    // Removing the pause allows the active memory quota check to run and causes
    // the resolved-ts service to re-register regions. Because we set a long
    // backoff, the initial scans won't start immediately, so diagnosis requests
    // should report zero locks.
    fail::remove("resolved_ts_after_handle_change_log");
    let num_locks = must_get_num_locks(&suite, region.id);
    assert_eq!(num_locks, 0, "num_locks: {}", num_locks);
    let num_locks = must_get_num_locks(&suite, region_b.id);
    assert_eq!(num_locks, 0, "num_locks: {}", num_locks);

    let pending_bytes = resolved_ts::RTS_CHANNEL_PENDING_CMD_BYTES.get();
    assert_eq!(pending_bytes, 0, "pending_bytes: {}", pending_bytes);

    suite.stop();
}
Repository: tikv/tikv
File: tests/integrations/server/lock_manager.rs (Rust)

Code before:
    let resp = kv_pessimistic_lock_resumable(
        &client,
        ctx.clone(),
        vec![key_b.to_vec()],
        30,
        30,
        Some(1000),
        false,
        false,
    );
    assert!(resp.region_error.is_none());
    assert_eq!(resp.results[0].get_type(), LockResultFailed);
    assert!(resp.errors[0].has_deadlock(), "{:?}", resp.errors[0]);

    // Release txn30 on key_a, then rx_txn40_ka can finish.
    must_kv_pessimistic_rollback(&client, ctx.clone(), key_a.to_vec(), 30, 30);

    let resp = rx_txn40_ka
        .recv_timeout(Duration::from_millis(100))
        .unwrap();
    assert!(resp.region_error.is_none());
    assert!(resp.errors.is_empty(), "{:?}", resp.errors);
    assert_eq!(resp.results[0].get_type(), LockResultNormal);

    must_kv_pessimistic_rollback(&client, ctx.clone(), key_a.to_vec(), 40, 40);
    must_kv_pessimistic_rollback(&client, ctx, key_b.to_vec(), 30, 30);
}


Review comment (bug):
This rollback appears incorrect. Transaction 30 attempted to lock key_b but received a deadlock error (line 658), so it never acquired the lock. The lock on key_b is held by transaction 40 (line 616), so this should be rolling back txn40's lock on key_b instead: `must_kv_pessimistic_rollback(&client, ctx, key_b.to_vec(), 40, 40);`
```suggestion
    must_kv_pessimistic_rollback(&client, ctx, key_b.to_vec(), 40, 40);
```

Code after:
    let resp = kv_pessimistic_lock_resumable(
        &client,
        ctx.clone(),
        vec![key_b.to_vec()],
        30,
        30,
        Some(1000),
        false,
        false,
    );
    assert!(resp.region_error.is_none());
    assert_eq!(resp.results[0].get_type(), LockResultFailed);
    assert!(resp.errors[0].has_deadlock(), "{:?}", resp.errors[0]);

    // Release txn30 on key_a, then rx_txn40_ka can finish.
    must_kv_pessimistic_rollback(&client, ctx.clone(), key_a.to_vec(), 30, 30);

    let resp = rx_txn40_ka
        .recv_timeout(Duration::from_millis(100))
        .unwrap();
    assert!(resp.region_error.is_none());
    assert!(resp.errors.is_empty(), "{:?}", resp.errors);
    assert_eq!(resp.results[0].get_type(), LockResultNormal);

    must_kv_pessimistic_rollback(&client, ctx.clone(), key_a.to_vec(), 40, 40);
    must_kv_pessimistic_rollback(&client, ctx, key_b.to_vec(), 40, 40);
}
Repository: toeverything/AFFiNE
File: blocksuite/blocks/src/_specs/common.ts (TypeScript)

Code before:
  FontLoaderService,
].flat();

export const EdgelessFirstPartyBlockSpecs: ExtensionType[] = [
  CommonBlockSpecs,

  EdgelessNoteBlockSpec,
  EdgelessSurfaceBlockSpec,
  EdgelessSurfaceRefBlockSpec,
  FrameBlockSpec,
  EdgelessTextBlockSpec,
  FontLoaderService,
].flat();

export const StoreExtensions: ExtensionType[] = [
  FeatureFlagService,
  BlockSelectionExtension,
  TextSelectionExtension,
  SurfaceSelectionExtension,
  CursorSelectionExtension,
  HighlightSelectionExtension,
  ImageSelectionExtension,
  DatabaseSelectionExtension,
  LinkPreviewerService,
];


Review comment (suggestion):
Store extension should be listed in https://github.com/toeverything/AFFiNE/blob/canary/blocksuite/blocks/src/_specs/common.ts#L93

Code after:
  PageSurfaceBlockSpec,
  PageSurfaceRefBlockSpec,
  FontLoaderService,
].flat();

export const EdgelessFirstPartyBlockSpecs: ExtensionType[] = [
  CommonBlockSpecs,

  EdgelessNoteBlockSpec,
  EdgelessSurfaceBlockSpec,
  EdgelessSurfaceRefBlockSpec,
  FrameBlockSpec,
  EdgelessTextBlockSpec,
  FontLoaderService,
].flat();

export const StoreExtensions: ExtensionType[] = [
  FeatureFlagService,
  BlockSelectionExtension,
  TextSelectionExtension,
  SurfaceSelectionExtension,
  CursorSelectionExtension,
  HighlightSelectionExtension,
  ImageSelectionExtension,
  DatabaseSelectionExtension,
  LinkPreviewerService,
];
Repository: toeverything/AFFiNE
File: packages/backend/server/src/models/base.ts (TypeScript)

Code before:
import { Inject } from '@nestjs/common';
import { PrismaClient } from '@prisma/client';

import { AFFiNELogger, Config } from '../base';
import type { Models } from '.';
import { MODELS_SYMBOL } from './provider';

export class BaseModel {
  protected readonly logger = new AFFiNELogger(this.constructor.name);

  @Inject(MODELS_SYMBOL)
  protected readonly models!: Models;

  @Inject(Config)
  protected readonly config!: Config;

  @Inject(PrismaClient)
  protected readonly db!: PrismaClient;
}


Review comment (suggestion):
`AFFiNELogger` can only be injected, otherwise the GCloud implemented `AFFiNELogger` would not work

Code after:
import { Inject, Logger } from '@nestjs/common';
import { PrismaClient } from '@prisma/client';

import { Config } from '../base';
import type { Models } from '.';
import { MODELS_SYMBOL } from './provider';

export class BaseModel {
  protected readonly logger = new Logger(this.constructor.name);

  @Inject(MODELS_SYMBOL)
  protected readonly models!: Models;

  @Inject(Config)
  protected readonly config!: Config;

  @Inject(PrismaClient)
  protected readonly db!: PrismaClient;
}
Repository: toeverything/AFFiNE
File: packages/backend/server/src/app.ts (TypeScript)

Code before:
import { NestFactory } from '@nestjs/core';
import type { NestExpressApplication } from '@nestjs/platform-express';
import cookieParser from 'cookie-parser';
import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.mjs';

import {
  AFFiNELogger,
  CacheInterceptor,
  CloudThrottlerGuard,
  GlobalExceptionFilter,
} from './base';
import { SocketIoAdapter } from './base/websocket';
import { AuthGuard } from './core/auth';
import { ENABLED_FEATURES } from './core/config/server-feature';
import { serverTimingAndCache } from './middleware/timing';

export async function createApp() {
  const { AppModule } = await import('./app.module');

  const app = await NestFactory.create<NestExpressApplication>(AppModule, {
    cors: true,
    rawBody: true,
    bodyParser: true,
  });

  app.useLogger(app.get(AFFiNELogger));

  if (AFFiNE.server.path) {
    app.setGlobalPrefix(AFFiNE.server.path);
  }

  app.use(serverTimingAndCache);

  app.use(
    graphqlUploadExpress({
      // TODO(@darkskygit): dynamic limit by quota maybe?
      maxFileSize: 100 * 1024 * 1024,
      maxFiles: 32,
    })
  );

  app.useGlobalGuards(app.get(AuthGuard), app.get(CloudThrottlerGuard));
  app.useGlobalInterceptors(app.get(CacheInterceptor));
  app.useGlobalFilters(new GlobalExceptionFilter(app.getHttpAdapter()));
  app.use(cookieParser());

  if (AFFiNE.flavor.sync) {
    const adapter = new SocketIoAdapter(app);

Review comment (security):
Add `bufferLogs: true` to `NestFactory.create` options should be better, follow the document example https://docs.nestjs.com/techniques/logger#dependency-injection

Code after:
import { NestFactory } from '@nestjs/core';
import type { NestExpressApplication } from '@nestjs/platform-express';
import cookieParser from 'cookie-parser';
import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.mjs';

import {
  AFFiNELogger,
  CacheInterceptor,
  CloudThrottlerGuard,
  GlobalExceptionFilter,
} from './base';
import { SocketIoAdapter } from './base/websocket';
import { AuthGuard } from './core/auth';
import { ENABLED_FEATURES } from './core/config/server-feature';
import { serverTimingAndCache } from './middleware/timing';

export async function createApp() {
  const { AppModule } = await import('./app.module');

  const app = await NestFactory.create<NestExpressApplication>(AppModule, {
    cors: true,
    rawBody: true,
    bodyParser: true,
    bufferLogs: true,
  });

  app.useLogger(app.get(AFFiNELogger));

  if (AFFiNE.server.path) {
    app.setGlobalPrefix(AFFiNE.server.path);
  }

  app.use(serverTimingAndCache);

  app.use(
    graphqlUploadExpress({
      // TODO(@darkskygit): dynamic limit by quota maybe?
      maxFileSize: 100 * 1024 * 1024,
      maxFiles: 32,
    })
  );

  app.useGlobalGuards(app.get(AuthGuard), app.get(CloudThrottlerGuard));
  app.useGlobalInterceptors(app.get(CacheInterceptor));
  app.useGlobalFilters(new GlobalExceptionFilter(app.getHttpAdapter()));
  app.use(cookieParser());

  if (AFFiNE.flavor.sync) {
Repository: toeverything/AFFiNE
File: blocksuite/blocks/src/root-block/edgeless/frame-manager.ts (TypeScript)

Code before:
  getTopElements,
  GfxBlockElementModel,
  type GfxController,
  GfxExtension,
  GfxExtensionIdentifier,
  type GfxModel,
  isGfxGroupCompatibleModel,
  renderableInEdgeless,
} from '@blocksuite/block-std/gfx';
import { BlockSuiteError, ErrorCode } from '@blocksuite/global/exceptions';
import {
  Bound,
  deserializeXYWH,
  DisposableGroup,
  type IVec,
  type SerializedXYWH,
} from '@blocksuite/global/utils';
import type { Store } from '@blocksuite/store';
import { Text } from '@blocksuite/store';
import * as Y from 'yjs';

import {
  EditPropsStore,
  type FrameBlockModel,
  type NoteBlockModel,
} from '../../index.js';
import { areSetsEqual } from './utils/misc.js';
import { isFrameBlock } from './utils/query.js';

const FRAME_PADDING = 40;

export class FrameOverlay extends Overlay {
  static override overlayName: string = 'frame';

  private _disposable = new DisposableGroup();

  private _frame: FrameBlockModel | null = null;

  private _innerElements = new Set<GfxModel>();

  private readonly _prevXYWH: SerializedXYWH | null = null;

  private get _frameManager() {
    return this.gfx.std.get(
      GfxExtensionIdentifier('frame-manager')
    ) as EdgelessFrameManager;
  }

  constructor(gfx: GfxController) {
    super(gfx);
  }

Review comment (suggestion):
For avoiding circular import, `EditPropsStore` should be imported from `@blocksuite/affine-shared/services`

Code after:
import {
  generateKeyBetweenV2,
  getTopElements,
  GfxBlockElementModel,
  type GfxController,
  GfxExtension,
  GfxExtensionIdentifier,
  type GfxModel,
  isGfxGroupCompatibleModel,
  renderableInEdgeless,
} from '@blocksuite/block-std/gfx';
import { BlockSuiteError, ErrorCode } from '@blocksuite/global/exceptions';
import {
  Bound,
  deserializeXYWH,
  DisposableGroup,
  type IVec,
  type SerializedXYWH,
} from '@blocksuite/global/utils';
import type { Store } from '@blocksuite/store';
import { Text } from '@blocksuite/store';
import * as Y from 'yjs';

import { areSetsEqual } from './utils/misc.js';
import { isFrameBlock } from './utils/query.js';

const FRAME_PADDING = 40;

export class FrameOverlay extends Overlay {
  static override overlayName: string = 'frame';

  private _disposable = new DisposableGroup();

  private _frame: FrameBlockModel | null = null;

  private _innerElements = new Set<GfxModel>();

  private readonly _prevXYWH: SerializedXYWH | null = null;

  private get _frameManager() {
    return this.gfx.std.get(
      GfxExtensionIdentifier('frame-manager')
    ) as EdgelessFrameManager;
  }

  constructor(gfx: GfxController) {
    super(gfx);
  }

  private _reset() {
    this._disposable.dispose();
Repository: toeverything/AFFiNE
File: packages/frontend/core/src/blocksuite/presets/ai/chat-panel/components/add-popover.ts (TypeScript)

Code before:
  };

  @state()
  private accessor _activatedItemIndex = 0;

  @property({ attribute: false })
  accessor docSearchMenuConfig!: DocSearchMenuConfig;

  @property({ attribute: false })
  accessor addChip!: (chip: ChatChip) => void;

  @property({ attribute: false })
  accessor abortController!: AbortController;

  @query('.search-input')
  accessor searchInput!: HTMLInputElement;

  override connectedCallback() {
    super.connectedCallback();
    this._updateDocGroup();

    requestAnimationFrame(() => {
      this.searchInput.focus();
    });
  }

  override render() {
    const items = Array.isArray(this._docGroup.items)
      ? this._docGroup.items
      : this._docGroup.items.value;
    return html`<div class="add-popover">
      <div class="search-input-wrapper">
        ${SearchIcon()}
        <input
          class="search-input"
          type="text"
          placeholder="Search Doc"
          .value=${this._query}
          @input=${this._onInput}
        />
      </div>
      <div class="divider"></div>
      <div class="search-group" style=${this._docGroup.styles ?? ''}>
        ${items.length > 0
          ? items.map(({ key, name, icon, action }, curIdx) => {
              return html`<icon-button
                width="280px"
                height="30px"
                data-id=${key}
                .text=${name}
                hover=${this._activatedItemIndex === curIdx}

Review comment (suggestion):
I think moving `focus` to `firstUpdated` is better
```suggestion
  }

  override firstUpdated() {
      this.searchInput.focus();
```

Code after:
  };

  @state()
  private accessor _activatedItemIndex = 0;

  @property({ attribute: false })
  accessor docSearchMenuConfig!: DocSearchMenuConfig;

  @property({ attribute: false })
  accessor addChip!: (chip: ChatChip) => void;

  @property({ attribute: false })
  accessor abortController!: AbortController;

  @query('.search-input')
  accessor searchInput!: HTMLInputElement;

  override connectedCallback() {
    super.connectedCallback();
    this._updateDocGroup();
  }

  override firstUpdated() {
    requestAnimationFrame(() => {
      this.searchInput.focus();
    });
  }

  override render() {
    const items = Array.isArray(this._docGroup.items)
      ? this._docGroup.items
      : this._docGroup.items.value;
    return html`<div class="add-popover">
      <div class="search-input-wrapper">
        ${SearchIcon()}
        <input
          class="search-input"
          type="text"
          placeholder="Search Doc"
          .value=${this._query}
          @input=${this._onInput}
        />
      </div>
      <div class="divider"></div>
      <div class="search-group" style=${this._docGroup.styles ?? ''}>
        ${items.length > 0
          ? items.map(({ key, name, icon, action }, curIdx) => {
              return html`<icon-button
                width="280px"
                height="30px"
                data-id=${key}
Repository: toeverything/AFFiNE
File: packages/backend/server/src/core/doc-renderer/event.ts (TypeScript)

Code before:
import { FactoryProvider, Injectable } from '@nestjs/common';
import { ModuleRef } from '@nestjs/core';

import { Config, OnEvent } from '../../base';
import { DocContentService } from './service';

@Injectable()
class DocEventsListener {
  constructor(private readonly doc: DocContentService) {}

  @OnEvent('doc.snapshot.updated')
  async handleDocSnapshotUpdated({
    workspaceId,
    docId,
  }: Events['doc.snapshot.updated']) {
    await this.doc.markDocContentCacheStale(workspaceId, docId);
  }
}

export const DocEventsListenerProvider: FactoryProvider = {
  provide: DocEventsListener,
  useFactory: (config: Config, ref: ModuleRef) => {
    if (config.flavor.renderer) {
      return ref.create(DocEventsListener);
    }
    return;
  },
  inject: [Config, ModuleRef],
};


Review comment (suggestion):
`ModuleRef` is too low level, it can be rewritten with `new DocEventsListener(doc)`:

```ts
export const DocEventsListenerProvider: FactoryProvider = {
  provide: DocEventsListener,
  useFactory: (config: Config, doc: DocContentService) => {
    if (config.flavor.renderer) {
      return new DocEventsListener(doc);
    }
    return;
  },
  inject: [Config, DocContentService],
};
```

Code after:
import { FactoryProvider } from '@nestjs/common';

import { Config, OnEvent } from '../../base';
import { DocContentService } from './service';

class DocEventsListener {
  constructor(private readonly doc: DocContentService) {}

  @OnEvent('doc.snapshot.updated')
  async handleDocSnapshotUpdated({
    workspaceId,
    docId,
  }: Events['doc.snapshot.updated']) {
    await this.doc.markDocContentCacheStale(workspaceId, docId);
  }
}

export const DocEventsListenerProvider: FactoryProvider = {
  provide: DocEventsListener,
  useFactory: (config: Config, doc: DocContentService) => {
    if (config.flavor.renderer) {
      return new DocEventsListener(doc);
    }
    return;
  },
  inject: [Config, DocContentService],
};
Repository: toeverything/AFFiNE
File: packages/frontend/core/src/modules/peek-view/view/ai-chat-block-peek-view/index.tsx (TypeScript)

Code before:
import { toReactNode } from '@affine/component';
import { AIChatBlockPeekViewTemplate } from '@affine/core/blocksuite/presets/ai';
import type { EditorHost } from '@blocksuite/affine/block-std';
import { useFramework } from '@toeverything/infra';

import type { AIChatBlockModel } from '../../../../blocksuite/blocks/ai-chat-block/ai-chat-model';
import { createPageModePreviewSpecs } from '../../../../components/blocksuite/block-suite-editor/specs/preview';

export type AIChatBlockPeekViewProps = {
  model: AIChatBlockModel;
  host: EditorHost;
};

export const AIChatBlockPeekView = ({
  model,
  host,
}: AIChatBlockPeekViewProps) => {
  const framework = useFramework();
  const previewSpecBuilder = createPageModePreviewSpecs(framework);
  const template = AIChatBlockPeekViewTemplate(model, host, previewSpecBuilder);
  return toReactNode(template);
};


Review comment (suggestion):
I think this should be memo'ed

```tsx
return useMemo(() => {
  const previewSpecBuilder = createPageModePreviewSpecs(framework);
  const template = AIChatBlockPeekViewTemplate(model, host, previewSpecBuilder);
  return toReactNode(template);
}, [framework, model, host]);
```

Code after:
import { toReactNode } from '@affine/component';
import { AIChatBlockPeekViewTemplate } from '@affine/core/blocksuite/presets/ai';
import type { EditorHost } from '@blocksuite/affine/block-std';
import { useFramework } from '@toeverything/infra';
import { useMemo } from 'react';

import type { AIChatBlockModel } from '../../../../blocksuite/blocks/ai-chat-block/ai-chat-model';
import { createPageModePreviewSpecs } from '../../../../components/blocksuite/block-suite-editor/specs/preview';

export type AIChatBlockPeekViewProps = {
  model: AIChatBlockModel;
  host: EditorHost;
};

export const AIChatBlockPeekView = ({
  model,
  host,
}: AIChatBlockPeekViewProps) => {
  const framework = useFramework();
  return useMemo(() => {
    const previewSpecBuilder = createPageModePreviewSpecs(framework);
    const template = AIChatBlockPeekViewTemplate(
      model,
      host,
      previewSpecBuilder
    );
    return toReactNode(template);
  }, [framework, model, host]);
};
Repository: toeverything/AFFiNE
File: blocksuite/affine/components/src/rich-text/keymap/format.ts (TypeScript)

Code before:
import {
  type BlockStdScope,
  TextSelection,
  type UIEventHandler,
} from '@blocksuite/block-std';

import { textFormatConfigs } from '../format/index.js';

export const textFormatKeymap = (std: BlockStdScope) =>
  textFormatConfigs
    .filter(config => config.hotkey)
    .reduce(
      (acc, config) => {
        return {
          ...acc,
          [config.hotkey as string]: ctx => {
            const { store: doc, selection } = std;
            if (doc.readonly) return;

            const textSelection = selection.find(TextSelection);
            if (!textSelection) return;

            // checks text length
            if (config.id === 'link') {
              const existed = Boolean(
                !textSelection.isCollapsed() &&
                  textSelection.from.length + (textSelection.to?.length ?? 0)
              );
              if (!existed) return;
            }

            const event = ctx.get('keyboardState').raw;
            event.stopPropagation();
            event.preventDefault();

            config.action(std.host);
            return true;
          },
        };
      },
      {} as Record<string, UIEventHandler>
    );


Review comment (suggestion):
You should modify the link config instead of the handler.

Code after:
import {
  type BlockStdScope,
  TextSelection,
  type UIEventHandler,
} from '@blocksuite/block-std';

import { textFormatConfigs } from '../format/index.js';

export const textFormatKeymap = (std: BlockStdScope) =>
  textFormatConfigs
    .filter(config => config.hotkey)
    .reduce(
      (acc, config) => {
        return {
          ...acc,
          [config.hotkey as string]: ctx => {
            const { store: doc, selection } = std;
            if (doc.readonly) return;

            const textSelection = selection.find(TextSelection);
            if (!textSelection) return;

            const allowed = config.textChecker?.(std.host) ?? true;
            if (!allowed) return;

            const event = ctx.get('keyboardState').raw;
            event.stopPropagation();
            event.preventDefault();

            config.action(std.host);
            return true;
          },
        };
      },
      {} as Record<string, UIEventHandler>
    );