ronantakizawa/codereview-bench huggingface dataset:
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/copilot/context/session.ts (TypeScript)

Code before:
    return categories.filter(c => c.type === ContextCategories.Collection);
  }

  get docs(): ContextDoc[] {
    return this.config.docs.map(d => ({ ...d }));
  }

  get files() {
    return this.config.files.map(f => ({ ...f }));
  }

  get sortedList(): ContextList {
    const { docs, files } = this.config;
    return [...docs, ...files].toSorted(
      (a, b) => a.createdAt - b.createdAt
    ) as ContextList;
  }

  async addCategoryRecord(type: ContextCategories, id: string, docs: string[]) {
    const category = this.config.categories.find(
      c => c.type === type && c.id === id
    );
    if (category) {
      const missingDocs = docs.filter(
        docId => !category.docs.some(d => d.id === docId)
      );
      if (missingDocs.length) {
        category.docs.push(
          ...missingDocs.map(id => ({
            id,
            createdAt: Date.now(),
            status: null,
          }))
        );
        await this.save();
      }

      return category;
    }
    const createdAt = Date.now();
    const record = {
      id,
      type,
      docs: docs.map(id => ({ id, createdAt, status: null })),
      createdAt,
    };
    this.config.categories.push(record);
    await this.save();
    return record;
  }


Review comment (suggestion):
Better to use lodash difference like:
```typescript
_.difference(docs, _.map(category.docs, 'id'))
```

Code after:
    return categories.filter(c => c.type === ContextCategories.Collection);
  }

  get docs(): ContextDoc[] {
    return this.config.docs.map(d => ({ ...d }));
  }

  get files() {
    return this.config.files.map(f => ({ ...f }));
  }

  get sortedList(): ContextList {
    const { docs, files } = this.config;
    return [...docs, ...files].toSorted(
      (a, b) => a.createdAt - b.createdAt
    ) as ContextList;
  }

  async addCategoryRecord(type: ContextCategories, id: string, docs: string[]) {
    const category = this.config.categories.find(
      c => c.type === type && c.id === id
    );
    if (category) {
      const missingDocs = docs.filter(
        docId => !category.docs.some(d => d.id === docId)
      );
      if (missingDocs.length) {
        category.docs.push(
          ...missingDocs.map(id => ({
            id,
            createdAt: Date.now(),
            status: ContextEmbedStatus.processing,
          }))
        );
        await this.save();
      }

      return category;
    }
    const createdAt = Date.now();
    const record = {
      id,
      type,
      docs: docs.map(id => ({
        id,
        createdAt,
        status: ContextEmbedStatus.processing,
      })),
      createdAt,
    };
    this.config.categories.push(record);
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/copilot/context/session.ts (TypeScript)

Code before:
      (a, b) => a.createdAt - b.createdAt
    ) as ContextList;
  }

  async addCategoryRecord(type: ContextCategories, id: string, docs: string[]) {
    const category = this.config.categories.find(
      c => c.type === type && c.id === id
    );
    if (category) {
      const missingDocs = docs.filter(
        docId => !category.docs.some(d => d.id === docId)
      );
      if (missingDocs.length) {
        category.docs.push(
          ...missingDocs.map(id => ({
            id,
            createdAt: Date.now(),
            status: null,
          }))
        );
        await this.save();
      }

      return category;
    }
    const createdAt = Date.now();
    const record = {
      id,
      type,
      docs: docs.map(id => ({ id, createdAt, status: null })),
      createdAt,
    };
    this.config.categories.push(record);
    await this.save();
    return record;
  }

  async removeCategoryRecord(type: ContextCategories, id: string) {
    const index = this.config.categories.findIndex(
      c => c.type === type && c.id === id
    );
    if (index >= 0) {
      this.config.categories.splice(index, 1);
      await this.save();
    }
    return true;
  }

  async addDocRecord(docId: string): Promise<ContextDoc> {
    const doc = this.config.docs.find(f => f.id === docId);
    if (doc) {

Review comment (suggestion):
- The initial state of the document/file to be embedded should be `processing`
- Documents that do not require embedding (eg. not existing document) should have a status of `null`.

Code after:
      (a, b) => a.createdAt - b.createdAt
    ) as ContextList;
  }

  async addCategoryRecord(type: ContextCategories, id: string, docs: string[]) {
    const category = this.config.categories.find(
      c => c.type === type && c.id === id
    );
    if (category) {
      const missingDocs = docs.filter(
        docId => !category.docs.some(d => d.id === docId)
      );
      if (missingDocs.length) {
        category.docs.push(
          ...missingDocs.map(id => ({
            id,
            createdAt: Date.now(),
            status: ContextEmbedStatus.processing,
          }))
        );
        await this.save();
      }

      return category;
    }
    const createdAt = Date.now();
    const record = {
      id,
      type,
      docs: docs.map(id => ({
        id,
        createdAt,
        status: ContextEmbedStatus.processing,
      })),
      createdAt,
    };
    this.config.categories.push(record);
    await this.save();
    return record;
  }

  async removeCategoryRecord(type: ContextCategories, id: string) {
    const index = this.config.categories.findIndex(
      c => c.type === type && c.id === id
    );
    if (index >= 0) {
      this.config.categories.splice(index, 1);
      await this.save();
    }
    return true;
  }
Repository: toeverything/AFFiNE
File: packages/backend/server/src/base/job/queue/queue.ts (TypeScript)

Code before:
import { Injectable, Logger } from '@nestjs/common';
import { ModuleRef } from '@nestjs/core';
import { Job, JobsOptions, Queue } from 'bullmq';
import { ClsServiceManager } from 'nestjs-cls';

import { genRequestId } from '../../utils';
import { namespace } from './def';

@Injectable()
export class JobQueue {
  private readonly logger = new Logger(JobQueue.name);

  constructor(private readonly moduleRef: ModuleRef) {}

  async add<T extends JobName>(name: T, payload: Jobs[T], opts?: JobsOptions) {
    const ns = namespace(name);
    const queue = this.getQueue(ns);
    const job = await queue.add(
      name,
      {
        $$requestId:
          ClsServiceManager.getClsService().getId() ?? genRequestId('job'),
        payload,
      },
      opts
    );
    this.logger.log(`Job [${name}] added; id=${job.id}`);
    return job;
  }

  async remove<T extends JobName>(jobId: string, jobName: T) {
    const ns = namespace(jobName);
    const queue = this.getQueue(ns);
    const job = (await queue.getJob(jobId)) as Job<Jobs[T]> | undefined;

    if (!job) {
      return;
    }

    const removed = await queue.remove(jobId);
    if (removed) {
      this.logger.log(`Job ${jobName} removed from queue ${ns}`);
      return job.data;
    }

    return undefined;
  }

  async get<T extends JobName>(jobId: string, jobName: T) {
    const ns = namespace(jobName);
    const queue = this.getQueue(ns);

Review comment (suggestion):
The data structure should be like this to keep the online code compatible

```ts
{
  ...payload,
  $$requestId
}

Code after:
import { Injectable, Logger } from '@nestjs/common';
import { ModuleRef } from '@nestjs/core';
import { Job, JobsOptions, Queue } from 'bullmq';
import { ClsServiceManager } from 'nestjs-cls';

import { genRequestId } from '../../utils';
import { namespace } from './def';

interface JobData<T extends JobName> {
  $$requestId: string;
  payload: Jobs[T];
}

@Injectable()
export class JobQueue {
  private readonly logger = new Logger(JobQueue.name);

  constructor(private readonly moduleRef: ModuleRef) {}

  async add<T extends JobName>(name: T, payload: Jobs[T], opts?: JobsOptions) {
    const ns = namespace(name);
    const queue = this.getQueue(ns);
    const job = await queue.add(
      name,
      {
        $$requestId:
          ClsServiceManager.getClsService().getId() ?? genRequestId('job'),
        payload,
      } as JobData<T>,
      opts
    );
    this.logger.log(`Job [${name}] added; id=${job.id}`);
    return job;
  }

  async remove<T extends JobName>(
    jobId: string,
    jobName: T
  ): Promise<Jobs[T] | undefined> {
    const ns = namespace(jobName);
    const queue = this.getQueue(ns);
    const job = (await queue.getJob(jobId)) as Job<JobData<T>> | undefined;

    if (!job) {
      return;
    }

    const removed = await queue.remove(jobId);
    if (removed) {
      this.logger.log(`Job ${jobName} removed from queue ${ns}`);
      return job.data.payload;
Repository: toeverything/AFFiNE
File: tests/kit/src/utils/keyboard.ts (TypeScript)

Code before:

export async function cutByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('x', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function pasteByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('v', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function selectAllByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('a', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function undoByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('z', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

const clipboardMutex = {
  lock: Promise.resolve(),
  async acquire() {
    await this.lock;
    const { promise, resolve } = Promise.withResolvers<void>();
    this.lock = promise;
    return resolve;
  },
};

export async function writeTextToClipboard(
  page: Page,
  text: string,
  paste = true
) {
  const release = await clipboardMutex.acquire();
  // paste the url
  await page.evaluate(
    async ([text]) => {
      navigator.clipboard.writeText('');
      const e = new ClipboardEvent('paste', {
        clipboardData: new DataTransfer(),
      });
      Object.defineProperty(e, 'target', {
        writable: false,
        value: document,

Review comment (suggestion):
The lock may not function correctly if two parties (B/C) are waiting for the same lock (created by A). After A released, both B and C will create their own new lock at line 112.

btw you can use `AsyncLock` from `@toeverything/infra`

Code after:
export async function cutByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('x', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function pasteByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('v', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function selectAllByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('a', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

export async function undoByKeyboard(page: Page) {
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('z', { delay: 50 });
  await keyUpCtrlOrMeta(page);
}

const clipboardMutex = new AsyncLock();

export async function writeTextToClipboard(
  page: Page,
  text: string,
  paste = true
) {
  using _release = await clipboardMutex.acquire();
  // paste the url
  await page.evaluate(
    async ([text]) => {
      navigator.clipboard.writeText('');
      const e = new ClipboardEvent('paste', {
        clipboardData: new DataTransfer(),
      });
      Object.defineProperty(e, 'target', {
        writable: false,
        value: document,
      });
      e.clipboardData!.setData('text/plain', text);
      document.dispatchEvent(e);
    },
    [text]
  );
  if (paste) {
    await keyDownCtrlOrMeta(page);
    await page.keyboard.press('v', { delay: 50 });
Repository: toeverything/AFFiNE
File: packages/backend/server/src/prelude.ts (TypeScript)

Code before:
import 'reflect-metadata';
import './dotenv';
import './env';

import { existsSync, readFileSync } from 'node:fs';
import { homedir } from 'node:os';
import { join } from 'node:path';

import { config } from 'dotenv';

const CUSTOM_CONFIG_PATH = `${homedir()}/.affine/config`;

function loadPrivateKey() {
  const file = join(CUSTOM_CONFIG_PATH, 'private.key');
  if (!process.env.AFFINE_PRIVATE_KEY && existsSync(file)) {
    const privateKey = readFileSync(file, 'utf-8');
    process.env.AFFINE_PRIVATE_KEY = privateKey;
  }
}

function load() {
  let isPrivateKeyFromEnv = !!process.env.AFFINE_PRIVATE_KEY;
  // load `.env` under pwd
  config();
  // load `.env` under user config folder
  config({
    path: join(CUSTOM_CONFIG_PATH, '.env'),

Review comment (suggestion):
better doing it by removing the `gloablThis.env = new Env()` in `./env.ts` and creating it after `load()`

Code after:
import 'reflect-metadata';

import { existsSync, readFileSync } from 'node:fs';
import { homedir } from 'node:os';
import { join } from 'node:path';

import { config } from 'dotenv';

import { createGlobalEnv } from './env';

const CUSTOM_CONFIG_PATH = `${homedir()}/.affine/config`;

function loadPrivateKey() {
  const file = join(CUSTOM_CONFIG_PATH, 'private.key');
  if (!process.env.AFFINE_PRIVATE_KEY && existsSync(file)) {
    const privateKey = readFileSync(file, 'utf-8');
    process.env.AFFINE_PRIVATE_KEY = privateKey;
  }
}

function load() {
  let isPrivateKeyFromEnv = !!process.env.AFFINE_PRIVATE_KEY;
  // load `.env` under pwd
  config();
  // load `.env` under user config folder
  config({
    path: join(CUSTOM_CONFIG_PATH, '.env'),
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/copilot/context/resolver.ts (TypeScript)

Code before:
        .user(user.id)
        .workspace(session.workspaceId)
        .allowLocal()
        .assert('Workspace.Copilot');
      const allowEmbedding = await this.models.workspace.allowEmbedding(
        session.workspaceId
      );
      if (!allowEmbedding) {
        return [];
      }

      const chunks = await session.matchWorkspaceChunks(
        content,
        limit,
        this.getSignal(ctx.req),
        threshold
      );
      const docsMap = await Promise.all(
        chunks.map(c =>
          this.ac
            .user(user.id)
            .workspace(session.workspaceId)
            .doc(c.docId)
            .can('Doc.Read')
            .then(ret => [c.docId, ret] as const)
        )
      ).then(r => new Map(r));

      return chunks.filter(c => docsMap.get(c.docId));
    } catch (e: any) {
      // passthrough user friendly error
      if (e instanceof UserFriendlyError) {
        throw e;
      }
      throw new CopilotFailedToMatchContext({
        contextId: context.id,
        // don't record the large content
        content: content.slice(0, 512),
        message: e.message,
      });
    }
  }
}


Review comment (security):
Add an interface to `ac` for batch checking whether `docId` has the same permission? This way, the database can be queried only once instead of N times

Code after:
      const session = await this.context.get(context.id);
      await this.ac
        .user(user.id)
        .workspace(session.workspaceId)
        .allowLocal()
        .assert('Workspace.Copilot');
      const allowEmbedding = await this.models.workspace.allowEmbedding(
        session.workspaceId
      );
      if (!allowEmbedding) {
        return [];
      }

      const chunks = await session.matchWorkspaceChunks(
        content,
        limit,
        this.getSignal(ctx.req),
        scopedThreshold,
        threshold
      );
      const docsMap = await Promise.all(
        chunks.map(c =>
          this.ac
            .user(user.id)
            .workspace(session.workspaceId)
            .doc(c.docId)
            .can('Doc.Read')
            .then(ret => [c.docId, ret] as const)
        )
      ).then(r => new Map(r));

      return chunks.filter(c => docsMap.get(c.docId));
    } catch (e: any) {
      // passthrough user friendly error
      if (e instanceof UserFriendlyError) {
        throw e;
      }
      throw new CopilotFailedToMatchContext({
        contextId: context.id,
        // don't record the large content
        content: content.slice(0, 512),
        message: e.message,
      });
    }
  }
}
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/indexer/job.ts (TypeScript)

Code before:
    }
    const nextSid = workspaces[workspaces.length - 1].sid;
    this.logger.log(
      `Auto added ${addedCount} workspaces to queue, lastIndexedWorkspaceSid: ${startSid} -> ${nextSid}`
    );

    // update the lastIndexedWorkspaceSid in the payload and repeat the job after 30 seconds
    payload.lastIndexedWorkspaceSid = nextSid;
    return JOB_SIGNAL.Repeat;
  }

  @Cron(CronExpression.EVERY_30_SECONDS)
  async scheduleAutoIndexWorkspaces() {
    if (!this.config.indexer.enabled) {
      return;
    }
    await this.queue.add(
      'indexer.autoIndexWorkspaces',
      {},
      {
        // make sure only one job is running at a time
        delay: 30 * 1000,
        jobId: 'indexer:auto-index-workspaces',
      }
    );
  }
}


Review comment (suggestion):
@forehalo  Reconstitute a pure job to pass the data, and add the `indexed` property to determine whether to update the index fully

Code after:
            updatedAt: true,
          },
        }
      );
      // ignore 180 days not updated workspaces
      if (
        !snapshotMeta?.updatedAt ||
        Date.now() - snapshotMeta.updatedAt.getTime() >
          180 * 24 * 60 * 60 * 1000
      ) {
        continue;
      }
      await this.queue.add(
        'indexer.indexWorkspace',
        { workspaceId: workspace.id },
        { jobId: workspace.id }
      );
      addedCount++;
    }
    const nextSid = workspaces[workspaces.length - 1].sid;
    this.logger.log(
      `Auto added ${addedCount} workspaces to queue, lastIndexedWorkspaceSid: ${startSid} -> ${nextSid}`
    );

    // update the lastIndexedWorkspaceSid in the payload and repeat the job after 30 seconds
    payload.lastIndexedWorkspaceSid = nextSid;
    return JOB_SIGNAL.Repeat;
  }

  @Cron(CronExpression.EVERY_30_SECONDS)
  async scheduleAutoIndexWorkspaces() {
    if (!this.config.indexer.enabled) {
      return;
    }
    await this.queue.add(
      'indexer.autoIndexWorkspaces',
      {},
      {
        // make sure only one job is running at a time
        delay: 30 * 1000,
        jobId: 'indexer:auto-index-workspaces',
      }
    );
  }
}
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/indexer/job.ts (TypeScript)

Code before:
      if (
        !snapshotMeta?.updatedAt ||
        Date.now() - snapshotMeta.updatedAt.getTime() >
          180 * 24 * 60 * 60 * 1000
      ) {
        continue;
      }
      await this.queue.add(
        'indexer.indexWorkspace',
        { workspaceId: workspace.id },
        { jobId: workspace.id }
      );
      addedCount++;
    }
    const nextSid = workspaces[workspaces.length - 1].sid;
    this.logger.log(
      `Auto added ${addedCount} workspaces to queue, lastIndexedWorkspaceSid: ${startSid} -> ${nextSid}`
    );

    // update the lastIndexedWorkspaceSid in the payload and repeat the job after 30 seconds
    payload.lastIndexedWorkspaceSid = nextSid;
    return JOB_SIGNAL.Repeat;
  }

  @Cron(CronExpression.EVERY_30_SECONDS)
  async scheduleAutoIndexWorkspaces() {
    if (!this.config.indexer.enabled) {
      return;
    }
    await this.queue.add(
      'indexer.autoIndexWorkspaces',
      {},
      {
        // make sure only one job is running at a time
        delay: 30 * 1000,
        jobId: 'indexer:auto-index-workspaces',
      }
    );
  }
}


Review comment (suggestion):
cron is not required because of handler returns `REPEAT`

Code after:
      // ignore 180 days not updated workspaces
      if (
        !snapshotMeta?.updatedAt ||
        Date.now() - snapshotMeta.updatedAt.getTime() >
          180 * 24 * 60 * 60 * 1000
      ) {
        continue;
      }
      await this.queue.add(
        'indexer.indexWorkspace',
        { workspaceId: workspace.id },
        { jobId: workspace.id }
      );
      addedCount++;
    }
    const nextSid = workspaces[workspaces.length - 1].sid;
    this.logger.log(
      `Auto added ${addedCount} workspaces to queue, lastIndexedWorkspaceSid: ${startSid} -> ${nextSid}`
    );

    // update the lastIndexedWorkspaceSid in the payload and repeat the job after 30 seconds
    payload.lastIndexedWorkspaceSid = nextSid;
    return JOB_SIGNAL.Repeat;
  }

  @Cron(CronExpression.EVERY_30_SECONDS)
  async scheduleAutoIndexWorkspaces() {
    if (!this.config.indexer.enabled) {
      return;
    }
    await this.queue.add(
      'indexer.autoIndexWorkspaces',
      {},
      {
        // make sure only one job is running at a time
        delay: 30 * 1000,
        jobId: 'indexer:auto-index-workspaces',
      }
    );
  }
}
Repository: toeverything/AFFiNE
File: packages/backend/server/src/plugins/copilot/utils.ts (TypeScript)

Code before:
}

type RequestClosedCallback = (isAborted: boolean) => void;
type SignalReturnType = {
  signal: AbortSignal;
  onConnectionClosed: (cb: RequestClosedCallback) => void;
};

export function getSignal(req: Request): SignalReturnType {
  const controller = new AbortController();

  let isAborted = true;
  let callback: ((isAborted: boolean) => void) | undefined = undefined;

  const onSocketEnd = () => {
    isAborted = true;
  };
  const onSocketClose = (hadError: boolean) => {
    const aborted = hadError || isAborted;
    if (aborted) {
      controller.abort();
    }

    callback?.(aborted);
    req.socket.off('end', onSocketEnd);
    req.socket.off('close', onSocketClose);
  };

  req.socket.on('end', onSocketEnd);
  req.socket.on('close', onSocketClose);

  return {
    signal: controller.signal,
    onConnectionClosed: cb => (callback = cb),
  };
}


Review comment (suggestion):
You need to `off` the event listener first before triggering the callback to avoid the callback exception causing the `off` not to execute

Code after:
}

type RequestClosedCallback = (isAborted: boolean) => void;
type SignalReturnType = {
  signal: AbortSignal;
  onConnectionClosed: (cb: RequestClosedCallback) => void;
};

export function getSignal(req: Request): SignalReturnType {
  const controller = new AbortController();

  let isAborted = true;
  let callback: ((isAborted: boolean) => void) | undefined = undefined;

  const onSocketEnd = () => {
    isAborted = false;
  };
  const onSocketClose = (hadError: boolean) => {
    req.socket.off('end', onSocketEnd);
    req.socket.off('close', onSocketClose);
    const aborted = hadError || isAborted;
    if (aborted) {
      controller.abort();
    }

    callback?.(aborted);
  };

  req.socket.on('end', onSocketEnd);
  req.socket.on('close', onSocketClose);

  return {
    signal: controller.signal,
    onConnectionClosed: cb => (callback = cb),
  };
}