ronantakizawa/codereview-bench huggingface dataset:
Repository: logto-io/logto
File: packages/schemas/tables/daily_active_users.sql (SQL)

Code before:
create table daily_active_users (
  id varchar(21) not null,
  tenant_id varchar(21) not null
    references tenants (id) on update cascade on delete cascade,
  user_id varchar(21) not null,
  date timestamptz not null default (now()),
  primary key (id),
  constraint daily_active_users__user_id_date
    unique (user_id, date)
);

create index daily_active_users__id
  on daily_active_users (tenant_id, id);

create index daily_active_users__tenant_date
  on daily_active_users (tenant_id, date);


Review comment (suggestion):
```suggestion
create index daily_active_users__date
```

Code after:
create table daily_active_users (
  id varchar(21) not null,
  tenant_id varchar(21) not null
    references tenants (id) on update cascade on delete cascade,
  user_id varchar(21) not null,
  date timestamptz not null default (now()),
  primary key (id),
  constraint daily_active_users__user_id_date
    unique (user_id, date)
);

create index daily_active_users__id
  on daily_active_users (tenant_id, id);

create index daily_active_users__date
  on daily_active_users (tenant_id, date);
Repository: logto-io/logto
File: packages/console/src/contexts/AppDataProvider.tsx (TypeScript)

Code before:
import ky from 'ky';
import type { ReactNode } from 'react';
import { useContext, useMemo, createContext } from 'react';
import useSWRImmutable from 'swr/immutable';

import { adminTenantEndpoint } from '@/consts';

import { TenantsContext } from './TenantsProvider';

type Props = {
  readonly children: ReactNode;
};

type AppData = {
  /**
   * The Logto endpoint for the current tenant.
   *
   * Always use this value as the base URL when referring to the Logto URL of the current user's tenant.
   */
  tenantEndpoint?: URL;
};

export const AppDataContext = createContext<AppData>({});

const useTenantEndpoint = (tenantId: string) => {
  return useSWRImmutable(Boolean(tenantId) && `api/.well-known/endpoints/${tenantId}`, async (pathname) => {
    const { user } = await ky.get(new URL(pathname, adminTenantEndpoint)).json<{ user: string }>();
    return new URL(user);
  });
};

/** The context provider for the global app data. */
function AppDataProvider({ children }: Props) {
  const { currentTenantId } = useContext(TenantsContext);
  const { data: tenantEndpoint } = useTenantEndpoint(currentTenantId);
  const memorizedContext = useMemo(
    () =>
      ({
        tenantEndpoint,
      }) satisfies AppData,
    [tenantEndpoint]
  );

  return <AppDataContext.Provider value={memorizedContext}>{children}</AppDataContext.Provider>;
}

export default AppDataProvider;


Review comment (suggestion):
```suggestion
  return useSWRImmutable(tenantId && `api/.well-known/endpoints/${tenantId}`, async (pathname) => {
```

Code after:
import ky from 'ky';
import type { ReactNode } from 'react';
import { useContext, useMemo, createContext } from 'react';
import useSWRImmutable from 'swr/immutable';

import { adminTenantEndpoint } from '@/consts';

import { TenantsContext } from './TenantsProvider';

type Props = {
  readonly children: ReactNode;
};

type AppData = {
  /**
   * The Logto endpoint for the current tenant.
   *
   * Always use this value as the base URL when referring to the Logto URL of the current user's tenant.
   */
  tenantEndpoint?: URL;
};

export const AppDataContext = createContext<AppData>({});

const useTenantEndpoint = (tenantId: string) => {
  return useSWRImmutable(
    Boolean(tenantId) && `api/.well-known/endpoints/${tenantId}`,
    async (pathname) => {
      const { user } = await ky
        .get(new URL(pathname, adminTenantEndpoint))
        .json<{ user: string }>();
      return new URL(user);
    }
  );
};

/** The context provider for the global app data. */
function AppDataProvider({ children }: Props) {
  const { currentTenantId } = useContext(TenantsContext);
  const { data: tenantEndpoint } = useTenantEndpoint(currentTenantId);
  const memorizedContext = useMemo(
    () =>
      ({
        tenantEndpoint,
      }) satisfies AppData,
    [tenantEndpoint]
  );

  return <AppDataContext.Provider value={memorizedContext}>{children}</AppDataContext.Provider>;
}
Repository: logto-io/logto
File: packages/account-center/src/App.tsx (TypeScript)

Code before:
      return;
    }

    if (!isAuthenticated) {
      void signIn({ redirectUri });
    }
  }, [isAuthenticated, isInCallback, isInitialAuthLoading, signIn]);

  if (isInCallback) {
    return <Callback />;
  }

  if (accountApiUnauthorized) {
    return (
      <ErrorPage
        titleKey="error.something_went_wrong"
        messageKey="error.invalid_session"
        action={{
          titleKey: 'action.sign_in',
          onClick: () => {
            void signIn({ redirectUri });
          },
        }}
      />
    );
  }

  if (experienceError ?? userInfoError) {
    return (
      <ErrorPage
        titleKey="error.something_went_wrong"
        rawMessage="We were unable to load your experience settings. Please refresh the page."
      />
    );
  }

  if (isInitialAuthLoading || isLoadingExperience) {
    return <div className={styles.status}>Loading…</div>;
  }

  if (!isAuthenticated) {
    return <div className={styles.status}>Redirecting to sign in…</div>;
  }

  return (
    <Routes>
      <Route path="email" element={<Email />} />
      <Route index element={<Home />} />
      <Route path="*" element={<Home />} />
    </Routes>
  );

Review comment (suggestion):
I think using router navigation is a better option, since we can have a dedicated router URL for the error page, and also we don't have to maintain the context.

Code after:
    if (isInCallback || isInitialAuthLoading) {
      return;
    }

    if (!isAuthenticated) {
      void signIn({ redirectUri });
    }
  }, [isAuthenticated, isInCallback, isInitialAuthLoading, signIn]);

  if (isInCallback) {
    return <Callback />;
  }

  if (experienceError ?? userInfoError) {
    return (
      <ErrorPage
        titleKey="error.something_went_wrong"
        rawMessage="We were unable to load your experience settings. Please refresh the page."
      />
    );
  }

  if (isInitialAuthLoading || isLoadingExperience) {
    return <div className={styles.status}>Loading…</div>;
  }

  if (!isAuthenticated) {
    return <div className={styles.status}>Redirecting to sign in…</div>;
  }

  return (
    <Routes>
      <Route path={sessionExpiredRoute} element={<SessionExpired />} />
      <Route path={emailRoute} element={<Email />} />
      <Route index element={<Home />} />
      <Route path="*" element={<Home />} />
    </Routes>
  );
};

const App = () => (
  <BrowserRouter basename={accountCenterBasePath}>
    <LogtoProvider
      config={{
        endpoint: window.location.origin,
        appId: accountCenterApplicationId,
        scopes: [UserScope.Profile, UserScope.Email, UserScope.Phone, UserScope.Identities],
      }}
    >
      <LoadingContextProvider>
        <PageContextProvider>
Repository: logto-io/logto
File: packages/core/src/routes/organization-invitation/index.ts (TypeScript)

Code before:
          details: 'The value of `expiresAt` must be in the future.',
        })
      );

      ctx.body = await organizationInvitations.insert(body, messagePayload);
      ctx.status = 201;
      return next();
    }
  );

  router.post(
    '/:id/message',
    koaGuard({
      params: z.object({ id: z.string() }),
      body: sendMessagePayloadGuard,
      status: [204],
    }),
    koaEmailI18n(queries),
    async (ctx, next) => {
      const {
        params: { id },
        body,
      } = ctx.guard;
      assertThat(
        'emailI18n' in ctx && isObject(ctx.emailI18n),
        new RequestError({
          status: 422,
          code: 'request.invalid_input',
          details: 'The email i18n context is missing.',
        })
      );
      const { invitee, organizationId, inviterId } = await invitations.findById(id);

      const templateContext =
        await organizationInvitations.getOrganizationInvitationTemplateContext(
          organizationId,
          inviterId
        );

      await organizationInvitations.sendEmail(invitee, {
        ...ctx.emailI18n,
        ...templateContext,
        ...body,
      });
      ctx.status = 204;
      return next();
    }
  );

  router.put(
    '/:id/status',

Review comment (suggestion):
Add some comments like, make typescript happy, since the koaRoute can not infer the context type properly with koaEmathe ilI18n middleware applied ahead. 

Code after:
          details: 'The value of `expiresAt` must be in the future.',
        })
      );

      ctx.body = await organizationInvitations.insert(body, messagePayload);
      ctx.status = 201;
      return next();
    }
  );

  router.post(
    '/:id/message',
    koaGuard({
      params: z.object({ id: z.string() }),
      body: sendMessagePayloadGuard,
      status: [204],
    }),
    koaEmailI18n(queries),
    async (ctx, next) => {
      const {
        params: { id },
        body,
      } = ctx.guard;
      // The `koaEmailI18n` properties are not available in the `SchemaRouter` context.
      // So we have to assert its existence here to make TypeScript happy.
      assertThat(
        'emailI18n' in ctx && isObject(ctx.emailI18n),
        new RequestError({
          status: 422,
          code: 'request.invalid_input',
          details: 'The email i18n context is missing.',
        })
      );
      const { invitee, organizationId, inviterId } = await invitations.findById(id);

      const templateContext =
        await organizationInvitations.getOrganizationInvitationTemplateContext(
          organizationId,
          inviterId
        );

      await organizationInvitations.sendEmail(invitee, {
        ...ctx.emailI18n,
        ...templateContext,
        ...body,
      });
      ctx.status = 204;
      return next();
    }
  );
Repository: logto-io/logto
File: packages/connectors/connector-yunpian-sms/src/index.ts (TypeScript)

Code before:
    if (!phone.startsWith('+')) {
      return `+${phone}`;
    }
    return phone;
  }

  // If it starts with +86 or 86, truncate the last 11 digits
  if (phone.startsWith('+86')) {
    return phone.slice(3);
  }
  if (phone.startsWith('86')) {
    return phone.slice(2);
  }

  return phone;
};

const sendMessage =
  (getConfig: GetConnectorConfig): SendMessageFunction =>
  async (data, inputConfig) => {
    const { to, type, payload } = data;
    const config = inputConfig ?? (await getConfig(defaultMetadata.id));
    validateConfig(config, yunpianSmsConfigGuard);
    const { apikey, enableInternational, unsupportedCountriesMsg } = config;

    const template = getConfigTemplateByType<typeof config>(type, config);

    assert(
      template,
      new ConnectorError(
        ConnectorErrorCodes.TemplateNotFound,
        `No SMS template found for type ${type}`
      )
    );

    const messageContent = replaceSendMessageHandlebars(template.content, payload);

    const formattedPhone = formatPhoneNumber(to);

    if (!enableInternational && formattedPhone.startsWith('+')) {
      if (unsupportedCountriesMsg) {
        throw new ConnectorError(ConnectorErrorCodes.General, unsupportedCountriesMsg);
      } else {
        console.warn(`connector-yunpian-sms: unsupported phone number: ${formattedPhone}`);
        return;
      }
    }

    const body: YunpianSmsPayload = {
      apikey,
      mobile: formattedPhone,

Review comment (suggestion):
it seems no need to add `<>` here as TS can infer the type correctly

Code after:
    if (!phone.startsWith('+')) {
      return `+${phone}`;
    }
    return phone;
  }

  // If it starts with +86 or 86, truncate the last 11 digits
  if (phone.startsWith('+86')) {
    return phone.slice(3);
  }
  if (phone.startsWith('86')) {
    return phone.slice(2);
  }

  return phone;
};

const sendMessage =
  (getConfig: GetConnectorConfig): SendMessageFunction =>
  async (data, inputConfig) => {
    const { to, type, payload } = data;
    const config = inputConfig ?? (await getConfig(defaultMetadata.id));
    validateConfig(config, yunpianSmsConfigGuard);
    const { apikey, enableInternational, unsupportedCountriesMsg } = config;

    const template = getConfigTemplateByType(type, config);

    assert(
      template,
      new ConnectorError(
        ConnectorErrorCodes.TemplateNotFound,
        `No SMS template found for type ${type}`
      )
    );

    const messageContent = replaceSendMessageHandlebars(template.content, payload);

    const formattedPhone = formatPhoneNumber(to);

    if (!enableInternational && formattedPhone.startsWith('+')) {
      if (unsupportedCountriesMsg) {
        throw new ConnectorError(ConnectorErrorCodes.General, unsupportedCountriesMsg);
      } else {
        console.warn(`connector-yunpian-sms: unsupported phone number: ${formattedPhone}`);
        return;
      }
    }

    const body: YunpianSmsPayload = {
      apikey,
      mobile: formattedPhone,
Repository: logto-io/logto
File: packages/console/src/utils/validator.ts (TypeScript)

Code before:
export const uriValidator = (value: string) => {
  try {
    // eslint-disable-next-line no-new
    new URL(value);
  } catch {
    return false;
  }

  return true;
};

export const redirectUriValidator = (value: string) => {
  if (!value.includes('*')) {
    return uriValidator(value);
  }

  const schemeSeparatorIndex = value.indexOf('://');
  if (schemeSeparatorIndex <= 0) {
    return false;
  }

  const scheme = value.slice(0, schemeSeparatorIndex).toLowerCase();
  if (scheme !== 'http' && scheme !== 'https') {
    return false;
  }

  // Disallow wildcards in scheme.
  if (value.slice(0, schemeSeparatorIndex).includes('*')) {
    return false;
  }

  // Disallow wildcards in query/hash to keep matching deterministic and safer.
  const queryIndex = value.indexOf('?');
  if (queryIndex >= 0 && value.slice(queryIndex).includes('*')) {
    return false;
  }

  const hashIndex = value.indexOf('#');
  if (hashIndex >= 0 && value.slice(hashIndex).includes('*')) {
    return false;
  }

  const authority = value.slice(schemeSeparatorIndex + 3).split(/[/?#]/)[0] ?? '';

  // Disallow credentials and IPv6 literals for simplicity.
  if (!authority || authority.includes('@') || authority.startsWith('[')) {
    return false;
  }

  // Disallow wildcards in port segment.

Review comment (suggestion):
It seems the native app scheme is not supported by this validator, e.g. `io.logto://callback`. Can we also include this type of condition check?

Maybe something like this would be better?

```ts
if (scheme !== 'http' && scheme !== 'https' && !scheme.include('.')) {
  return false;
}
```

Code after:
import { validateRedirectUrl } from '@logto/core-kit';

export const uriValidator = (value: string) => {
  try {
    // eslint-disable-next-line no-new
    new URL(value);
  } catch {
    return false;
  }

  return true;
};

export const redirectUriValidator = (value: string) =>
  validateRedirectUrl(value, 'web') || validateRedirectUrl(value, 'mobile');

export const jsonValidator = (value: string) => {
  try {
    JSON.parse(value);
  } catch {
    return false;
  }

  return true;
};
Repository: logto-io/logto
File: packages/shared/src/node/env/GlobalValues.ts (TypeScript)

Code before:
import {
  assertEnv,
  getEnv,
  getEnvAsStringArray,
  tryThat,
  yes,
  type Optional,
} from '@silverhand/essentials';

import UrlSet from './UrlSet.js';
import { throwErrorWithDsnMessage } from './throw-errors.js';

export const parseTimeoutEnv = (value?: string): Optional<number | 'DISABLE_TIMEOUT'> => {
  if (!value) {
    return undefined;
  }

  const normalized = value.trim();

  if (!normalized) {
    return undefined;
  }

  if (normalized === 'DISABLE_TIMEOUT') {
    return 'DISABLE_TIMEOUT';
  }

  const parsed = Number(normalized);

  return Number.isFinite(parsed) ? parsed : undefined;
};

export default class GlobalValues {
  public readonly isProduction = getEnv('NODE_ENV') === 'production';
  public readonly isIntegrationTest = yes(getEnv('INTEGRATION_TEST'));
  public readonly isUnitTest = getEnv('NODE_ENV') === 'test';
  public readonly isDevFeaturesEnabled = !this.isProduction || yes(getEnv('DEV_FEATURES_ENABLED'));

  public readonly httpsCert = process.env.HTTPS_CERT_PATH;
  public readonly httpsKey = process.env.HTTPS_KEY_PATH;

Review comment (suggestion):
Should there be a default value on our side as well? If not, I think we should add some comments around this function to say that if it returns `undefined`, the postgres default timeout value (60000ms) will be applied blah blah.

Code after:
import {
  assertEnv,
  getEnv,
  getEnvAsStringArray,
  tryThat,
  yes,
  type Optional,
} from '@silverhand/essentials';

import UrlSet from './UrlSet.js';
import { throwErrorWithDsnMessage } from './throw-errors.js';

/**
 * Parses a timeout value from an environment variable string.
 *
 * The input may be:
 * - a numeric string (milliseconds), or
 * - the literal `DISABLE_TIMEOUT` to omit the startup parameter.
 *
 * Empty/whitespace or invalid values return `undefined`, which lets Slonik apply
 * its default timeout of 60000 ms.
 *
 * @param value - Raw string value from an environment variable.
 * @returns A finite numeric timeout, `DISABLE_TIMEOUT`, or `undefined`.
 *
 * @example
 * const timeout = parseTimeoutEnv(process.env.DATABASE_STATEMENT_TIMEOUT);
 * if (timeout === 'DISABLE_TIMEOUT') {
 *   // omit the startup parameter entirely so server defaults apply
 * }
 */
export const parseTimeoutEnv = (value?: string): Optional<number | 'DISABLE_TIMEOUT'> => {
  if (value === undefined) {
    return;
  }

  const normalized = value.trim();

  if (normalized === '') {
    return;
Repository: logto-io/logto
File: packages/core/src/env-set/create-pool.ts (TypeScript)

Code before:
import { assert } from '@silverhand/essentials';
import {
  createMockPool,
  createMockQueryResult,
  createPool,
  parseDsn,
  createInterceptorsPreset,
} from '@silverhand/slonik';

const createPoolByEnv = async (
  databaseDsn: string,
  mockDatabaseConnection: boolean,
  poolSize?: number,
  connectionTimeout?: number,
  statementTimeout?: number | 'DISABLE_TIMEOUT'
) => {
  // Database connection is disabled in unit test environment
  if (mockDatabaseConnection) {
    return createMockPool({ query: async () => createMockQueryResult([]) });
  }

  assert(parseDsn(databaseDsn).databaseName, new Error('Database name is required'));

  return createPool(databaseDsn, {
    interceptors: createInterceptorsPreset(),
    maximumPoolSize: poolSize,
    connectionTimeout,
    statementTimeout,
  });
};

export default createPoolByEnv;


Review comment (suggestion):
`statementTimeout?` doesn’t convert `undefined` to `DISABLE_TIMEOUT`. If we pass
 `statementTimeout: undefined`, it may override Slonik’s default timeout.

Code after:
import { assert, conditional } from '@silverhand/essentials';
import {
  createMockPool,
  createMockQueryResult,
  createPool,
  parseDsn,
  createInterceptorsPreset,
} from '@silverhand/slonik';

const createPoolByEnv = async (
  databaseDsn: string,
  mockDatabaseConnection: boolean,
  poolSize?: number,
  connectionTimeout?: number,
  statementTimeout?: number | 'DISABLE_TIMEOUT'
) => {
  // Database connection is disabled in unit test environment
  if (mockDatabaseConnection) {
    return createMockPool({ query: async () => createMockQueryResult([]) });
  }

  assert(parseDsn(databaseDsn).databaseName, new Error('Database name is required'));

  const poolOptions = {
    interceptors: createInterceptorsPreset(),
    maximumPoolSize: poolSize,
    connectionTimeout,
    ...conditional(statementTimeout !== undefined && { statementTimeout }),
  };

  return createPool(databaseDsn, poolOptions);
};

export default createPoolByEnv;
Repository: logto-io/logto
File: packages/schemas/tables/users.sql (SQL)

Code before:
  identities jsonb /* @use Identities */ not null default '{}'::jsonb,
  custom_data jsonb /* @use JsonObject */ not null default '{}'::jsonb,
  logto_config jsonb /* @use JsonObject */ not null default '{}'::jsonb,
  mfa_verifications jsonb /* @use MfaVerifications */ not null default '[]'::jsonb,
  is_suspended boolean not null default false,
  last_sign_in_at timestamptz,
  created_at timestamptz not null default (now()),
  updated_at timestamptz not null default (now()),
  primary key (id),
  constraint users__username
    unique (tenant_id, username),
  constraint users__primary_email
    unique (tenant_id, primary_email),
  constraint users__primary_phone
    unique (tenant_id, primary_phone)
);

/* Unique index on (tenant_id, id) required for foreign key constraint in organization_user_relations table. */
create unique index users__id
  on users (tenant_id, id);

create index users__name
  on users (tenant_id, name);

create index users_mfa_verifications_gin
  on users using gin (mfa_verifications);

create trigger set_updated_at
  before update on users
  for each row
  execute procedure set_updated_at();


Review comment (performance):
```suggestion
  on users using gin (mfa_verifications, jsonb_path_ops);
```

using `jsonb_path_ops` instead of the default `jsonb_ops` (which creates larger index data) to increase the index performance. 

Code after:
  identities jsonb /* @use Identities */ not null default '{}'::jsonb,
  custom_data jsonb /* @use JsonObject */ not null default '{}'::jsonb,
  logto_config jsonb /* @use JsonObject */ not null default '{}'::jsonb,
  mfa_verifications jsonb /* @use MfaVerifications */ not null default '[]'::jsonb,
  is_suspended boolean not null default false,
  last_sign_in_at timestamptz,
  created_at timestamptz not null default (now()),
  updated_at timestamptz not null default (now()),
  primary key (id),
  constraint users__username
    unique (tenant_id, username),
  constraint users__primary_email
    unique (tenant_id, primary_email),
  constraint users__primary_phone
    unique (tenant_id, primary_phone)
);

/* Unique index on (tenant_id, id) required for foreign key constraint in organization_user_relations table. */
create unique index users__id
  on users (tenant_id, id);

create index users__name
  on users (tenant_id, name);

create index users_mfa_verifications_gin
  on users using gin (mfa_verifications jsonb_path_ops);

create trigger set_updated_at
  before update on users
  for each row
  execute procedure set_updated_at();