ronantakizawa/codereview-bench huggingface dataset:
Repository: spotify/backstage
File: plugins/catalog-react/src/hooks/useEntityListProvider.tsx (TypeScript)

Code before:
      setOffset,
    ],
  );

  return (
    <EntityListContext.Provider value={value}>
      <NewEntityListContext.Provider
        value={createVersionedValueMap({ 1: value })}
      >
        {props.children}
      </NewEntityListContext.Provider>
    </EntityListContext.Provider>
  );
};

/**
 * Hook for interacting with the entity list context provided by the {@link EntityListProvider}.
 * @public
 */
export function useEntityList<
  EntityFilters extends DefaultEntityFilters = DefaultEntityFilters,
>(): EntityListContextProps<EntityFilters> {
  const versionedHolder = useVersionedContext<{
    1: EntityListContextProps<any>;
  }>('entity-list-context');
  const oldContext = useContext(EntityListContext);

  if (versionedHolder) {
    const value = versionedHolder.atVersion(1);
    if (!value) {
      throw new Error('EntityListContext v1 not available');
    }
    return value;
  }

  if (oldContext) {
    return oldContext;
  }

  throw new Error('useEntityList must be used within EntityListProvider');
}


Review comment (suggestion):
Don't think this is needed really, because if the producer isn't providing the new versions context we'll have the duplication problem on the old context anyway. i.e. it should be safe to just throw away the old context and use the new one only straight away

Code after:
    ],
  );

  return (
    <OldEntityListContext.Provider value={value}>
      <NewEntityListContext.Provider
        value={createVersionedValueMap({ 1: value })}
      >
        {props.children}
      </NewEntityListContext.Provider>
    </OldEntityListContext.Provider>
  );
};

/**
 * Hook for interacting with the entity list context provided by the {@link EntityListProvider}.
 * @public
 */
export function useEntityList<
  EntityFilters extends DefaultEntityFilters = DefaultEntityFilters,
>(): EntityListContextProps<EntityFilters> {
  const versionedHolder = useVersionedContext<{
    1: EntityListContextProps<any>;
  }>('entity-list-context');
  const oldContext = useContext(OldEntityListContext);

  if (versionedHolder) {
    const value = versionedHolder.atVersion(1);
    if (!value) {
      throw new Error('EntityListContext v1 not available');
    }
    return value;
  }

  if (oldContext) {
    return oldContext;
  }

  throw new Error('useEntityList must be used within EntityListProvider');
}
Repository: spotify/backstage
File: scripts/verify-plugin-directory.js (JavaScript)

Code before:
  const pluginDirectoryPath = resolve(rootPath, 'microsite', 'data', 'plugins');

  const pluginDirectoryFiles = await fs.readdir(pluginDirectoryPath);

  let hasErrors = false;
  for (const pluginDirectoryFile of pluginDirectoryFiles) {
    if (!pluginDirectoryFile.toLocaleLowerCase().includes('.yaml')) {
      hasErrors = true;
      console.log(`${pluginDirectoryFile} is missing the '.yaml' extension`);
    }

    const pluginDirectoryFilePath = join(
      pluginDirectoryPath,
      pluginDirectoryFile,
    );

    const pluginDataYaml = yaml.load(
      fs.readFileSync(pluginDirectoryFilePath, { encoding: 'utf-8' }),
    );

    try {
      configSchema.parse(pluginDataYaml);
    } catch (error) {
      if (error instanceof z.ZodError) {
        console.log(
          `YAML data validation failed for ${pluginDirectoryFile}:`,
          error.errors,
        );
      } else {
        console.log(
          `An unexpected error occurred for ${pluginDirectoryFile}:`,
          error,
        );
      }
    }
  }
  if (hasErrors) {
    throw new Error(
      'There was an error verifying the Plugin Directory data files, review the output for guidance on what corrections are needed',
    );
  }
}

main(process.argv.slice(2)).catch(error => {
  console.error(error.stack || error);
  process.exit(1);
});


Review comment (suggestion):
Could perhaps flip this around (same in the others): `${pluginDirectoryFilePath}: Rest of message here` - this makes the full path clickable in the terminal of vscode and the context is the first thing you see. The one at the top that says it lacks the yaml extension could do the same too (and therefore also forming the full path first thing you do at the top)

Code after:
  const pluginDirectoryPath = resolve(rootPath, 'microsite', 'data', 'plugins');

  const pluginDirectoryFiles = await fs.readdir(pluginDirectoryPath);

  let hasErrors = false;
  for (const pluginDirectoryFile of pluginDirectoryFiles) {
    const pluginDirectoryFilePath = join(
      pluginDirectoryPath,
      pluginDirectoryFile,
    );

    if (!pluginDirectoryFile.toLocaleLowerCase().includes('.yaml')) {
      hasErrors = true;
      console.log(
        `${pluginDirectoryFilePath}: The '.yaml' extension is missing`,
      );
    }

    const pluginDataYaml = yaml.load(
      fs.readFileSync(pluginDirectoryFilePath, { encoding: 'utf-8' }),
    );

    try {
      configSchema.parse(pluginDataYaml);
    } catch (error) {
      if (error instanceof z.ZodError) {
        console.log(
          `${pluginDirectoryFilePath}: YAML data validation failed`,
          error.errors,
        );
      } else {
        console.log(
          `${pluginDirectoryFilePath}: An unexpected error occurred`,
          error,
        );
      }
    }
  }
  if (hasErrors) {
    throw new Error(
      'There was an error verifying the Plugin Directory data files, review the output for guidance on what corrections are needed',
    );
  }
}

main().catch(error => {
  console.error(error.stack || error);
  process.exit(1);
});
Repository: spotify/backstage
File: packages/cli/src/modules/info/index.ts (TypeScript)

Code before:
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import yargs from 'yargs';
import { createCliPlugin } from '../../wiring/factory';
import { lazy } from '../../lib/lazy';

export default createCliPlugin({
  pluginId: 'info',
  init: async reg => {
    reg.addCommand({
      path: ['info'],
      description: 'Show helpful information for debugging and reporting bugs',
      execute: async ({ args }) => {
        const argv = await yargs()
          .options({
            include: {
              type: 'string',
              array: true,
              default: [],
              description:
                'Glob patterns for additional packages to include (e.g., @spotify/backstage*)',
            },
            'output-file': {
              type: 'string',
              description:
                'Write the info output to a JSON file instead of stdout',
            },
          })
          .help()
          .parse(args);
        await lazy(() => import('./commands/info'), 'default')(argv);
      },
    });
  },
});


Review comment (suggestion):
Is it better to have people do --format json > file.json and potentially skip the output file option? The current proposal mixes two things (format and target). Other commands like `config:print` already have a format option.

Code after:
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import yargs from 'yargs';
import { createCliPlugin } from '../../wiring/factory';
import { lazy } from '../../lib/lazy';

export default createCliPlugin({
  pluginId: 'info',
  init: async reg => {
    reg.addCommand({
      path: ['info'],
      description: 'Show helpful information for debugging and reporting bugs',
      execute: async ({ args }) => {
        const argv = await yargs()
          .options({
            include: {
              type: 'string',
              array: true,
              default: [],
              description:
                'Glob patterns for additional packages to include (e.g., @spotify/backstage*)',
            },
            format: {
              type: 'string',
              choices: ['text', 'json'],
              default: 'text',
              description: 'Output format (text or json)',
            },
          })
          .help()
          .parse(args);
        await lazy(() => import('./commands/info'), 'default')(argv);
      },
    });
  },
});
Repository: spotify/backstage
File: plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/wait.ts (TypeScript)

Code before:
            .optional(),
        milliseconds: z =>
          z
            .number({
              description: 'Waiting period in milliseconds.',
            })
            .optional(),
      },
    },
    async handler(ctx) {
      const delayTime = Duration.fromObject(ctx.input);
      const maxWait = toDuration(options?.maxWaitTime);

      if (delayTime.minus(maxWait).toMillis() > 0) {
        throw new Error(
          `Waiting duration is longer than the maximum threshold of ${maxWait.toHuman()}`,
        );
      }

      await new Promise(resolve => {
        const timeoutHandle = setTimeout(abort, delayTime.toMillis());
        ctx.signal?.addEventListener('abort', abort, { once: true });

        function abort() {
          clearTimeout(timeoutHandle);
          resolve('finished');
        }
      });
    },
  });
}


Review comment (suggestion):
Unrelated, but you can change to `new Promise<void>` to be able to just do `resolve()` here without a dummy value.

Code after:
            .optional(),
        milliseconds: z =>
          z
            .number({
              description: 'Waiting period in milliseconds.',
            })
            .optional(),
      },
    },
    async handler(ctx) {
      const delayTime = Duration.fromObject(ctx.input);
      const maxWait = toDuration(options?.maxWaitTime);

      if (delayTime.minus(maxWait).toMillis() > 0) {
        throw new Error(
          `Waiting duration is longer than the maximum threshold of ${maxWait.toHuman()}`,
        );
      }

      await new Promise<void>(resolve => {
        const timeoutHandle = setTimeout(abort, delayTime.toMillis());
        ctx.signal?.addEventListener('abort', abort);

        function abort() {
          ctx.signal?.removeEventListener('abort', abort);
          clearTimeout(timeoutHandle);
          resolve();
        }
      });
    },
  });
}
Repository: spotify/backstage
File: plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/wait.ts (TypeScript)

Code before:
          z
            .number({
              description: 'Waiting period in seconds.',
            })
            .optional(),
        milliseconds: z =>
          z
            .number({
              description: 'Waiting period in milliseconds.',
            })
            .optional(),
      },
    },
    async handler(ctx) {
      const delayTime = Duration.fromObject(ctx.input);
      const maxWait = toDuration(options?.maxWaitTime);

      if (delayTime.minus(maxWait).toMillis() > 0) {
        throw new Error(
          `Waiting duration is longer than the maximum threshold of ${maxWait.toHuman()}`,
        );
      }

      await new Promise(resolve => {
        const timeoutHandle = setTimeout(abort, delayTime.toMillis());
        ctx.signal?.addEventListener('abort', abort, { once: true });

        function abort() {
          clearTimeout(timeoutHandle);
          resolve('finished');
        }
      });
    },
  });
}


Review comment (suggestion):
Note that this still "leaks". The `once` setting only makes it so that the listener is removed IF it triggers. The only way that it triggers is if the user selects to cancel the task run or if the service is shutting down or whatnot. So normally, this will never get unregistered. I recommend reinstating the `removeEventListener` below

Code after:
          z
            .number({
              description: 'Waiting period in seconds.',
            })
            .optional(),
        milliseconds: z =>
          z
            .number({
              description: 'Waiting period in milliseconds.',
            })
            .optional(),
      },
    },
    async handler(ctx) {
      const delayTime = Duration.fromObject(ctx.input);
      const maxWait = toDuration(options?.maxWaitTime);

      if (delayTime.minus(maxWait).toMillis() > 0) {
        throw new Error(
          `Waiting duration is longer than the maximum threshold of ${maxWait.toHuman()}`,
        );
      }

      await new Promise<void>(resolve => {
        const timeoutHandle = setTimeout(abort, delayTime.toMillis());
        ctx.signal?.addEventListener('abort', abort);

        function abort() {
          ctx.signal?.removeEventListener('abort', abort);
          clearTimeout(timeoutHandle);
          resolve();
        }
      });
    },
  });
}
Repository: spotify/backstage
File: packages/cli-common/src/paths.ts (TypeScript)

Code before:
    throw new Error(
      `No package.json found while searching for package root of ${searchDir}`,
    );
  }
  return path;
}

// Finds the root of the monorepo that the package exists in. Only accessible when running inside Backstage repo.
export function findOwnRootDir(ownDir: string) {
  const isLocal = fs.existsSync(resolvePath(ownDir, 'src'));
  if (!isLocal) {
    throw new Error(
      'Tried to access monorepo package root dir outside of Backstage repository',
    );
  }

  return resolvePath(ownDir, '../..');
}

/**
 * Gets the workspaces pattern from a package.json object.
 * @param pkgJson - The package.json object to get the workspaces pattern from.
 * @returns The workspaces patterns (glob not resolved).
 * @public
 */
export function getWorkspacesPatterns(pkgJson: any): string[] {
  if (Array.isArray(pkgJson.workspaces)) {
    return pkgJson.workspaces;
  } else if (
    typeof pkgJson.workspaces === 'object' &&
    pkgJson.workspaces !== null &&
    Array.isArray(pkgJson.workspaces.packages)
  ) {
    return pkgJson.workspaces.packages;
  }
  return [];
}

/**
 * Find paths related to a package and its execution context.
 *
 * @public
 * @example
 *
 * const paths = findPaths(__dirname)
 */
export function findPaths(searchDir: string): Paths {
  const ownDir = findOwnDir(searchDir);
  // Drive letter can end up being lowercased here on Windows, bring back to uppercase for consistency
  const targetDir = fs
    .realpathSync(process.cwd())

Review comment (suggestion):
`any` is too lax here - you could make it `unknown` or perhaps `JsonObject`. Either way, you'll find that it forces you to do an `object` type check in there to be safe.

Code after:
  let path = searchDir;

  // Some confidence check to avoid infinite loop
  for (let i = 0; i < 1000; i++) {
    const packagePath = resolvePath(path, 'package.json');
    const exists = fs.existsSync(packagePath);
    if (exists && filterFunc(packagePath)) {
      return path;
    }

    const newPath = dirname(path);
    if (newPath === path) {
      return undefined;
    }
    path = newPath;
  }

  throw new Error(
    `Iteration limit reached when searching for root package.json at ${searchDir}`,
  );
}

// Finds the root of the monorepo that the package exists in.
export function findOwnRootDir(ownDir: string) {
  const isLocal = fs.existsSync(resolvePath(ownDir, 'src'));
  if (!isLocal) {
    throw new Error(
      'Tried to access monorepo package root dir outside of Backstage repository',
    );
  }

  const rootDir = findRootPath(ownDir, pkgJsonPath => {
    try {
      const content = fs.readFileSync(pkgJsonPath, 'utf8');
      const data = JSON.parse(content);
      return Boolean(data.workspaces);
    } catch (error) {
      throw new Error(
        `Failed to read package.json at '${pkgJsonPath}', ${error}`,
      );
    }
  });

  if (!rootDir) {
    throw new Error(`No monorepo root found when searching from '${ownDir}'`);
  }

  return rootDir;
}

// Hierarchical directory cache shared across all OwnPathsImpl instances.
Repository: spotify/backstage
File: packages/ui/src/components/Table/types.ts (TypeScript)

Code before:
  item: T;
  index: number;
}) => ReactNode;

/** @public */
export interface TableSelection {
  mode?: ReactAriaTableProps['selectionMode'];
  behavior?: ReactAriaTableProps['selectionBehavior'];
  selected?: ReactAriaTableProps['selectedKeys'];
  onSelectionChange?: ReactAriaTableProps['onSelectionChange'];
}

/** @public */
export interface TableProps<T extends TableItem> {
  columnConfig: readonly ColumnConfig<T>[];
  data: T[] | undefined;
  loading?: boolean;
  isStale?: boolean;
  error?: Error;
  pagination: TablePaginationType;
  sort?: SortState;
  rowConfig?: RowConfig<T> | RowRenderFn<T>;
  selection?: TableSelection;
  emptyState?: ReactNode;
  className?: string;
  tableLayout?: 'auto' | 'fixed';
  style?: React.CSSProperties;
  styles?: Partial<Record<'tableRoot', React.CSSProperties>>;
}


Review comment (suggestion):
I wonder if it wouldn't make more sense to have a prop that purely turns on/off the `ResizableTableContainer` wrapping instead of this, and then rely on styling via classname to apply the `table-layout` like you want it (see below). 

The naming is a little confusing to me, and wasn't obvious from the start that this maps partially to the css property with the same name and partially to the resizable wrapping. 🤔 


Code after:
  item: T;
  index: number;
}) => ReactNode;

/** @public */
export interface TableSelection {
  mode?: ReactAriaTableProps['selectionMode'];
  behavior?: ReactAriaTableProps['selectionBehavior'];
  selected?: ReactAriaTableProps['selectedKeys'];
  onSelectionChange?: ReactAriaTableProps['onSelectionChange'];
}

/** @public */
export interface TableProps<T extends TableItem> {
  columnConfig: readonly ColumnConfig<T>[];
  data: T[] | undefined;
  loading?: boolean;
  isStale?: boolean;
  error?: Error;
  pagination: TablePaginationType;
  sort?: SortState;
  rowConfig?: RowConfig<T> | RowRenderFn<T>;
  selection?: TableSelection;
  emptyState?: ReactNode;
  className?: string;
  style?: React.CSSProperties;
}
Repository: spotify/backstage
File: plugins/auth-backend-module-auth0-provider/src/strategy.ts (TypeScript)

Code before:
    super.authenticate(req, {
      ...options,
      organization: req.query.organization?.toString(),
      organization_name: req.query.organization_name?.toString(),
      invitation: req.query.invitation?.toString(),
    });
  }

  authorizationParams(options: Record<string, any>): Record<string, any> {
    const params = super.authorizationParams(options);

    if (options.organization || this.organization) {
      params.organization = options.organization || this.organization;
    }

    if (options.organization_name) {
      params.organization_name = options.organization_name;
    }

    if (options.invitation) {
      params.invitation = options.invitation;
    }

    return params;
  }
}


Review comment (security):
My main question is whether we should allow the invitation link to specify the `organizationId` or should we always go with `organizationId` found in the auth config?

Code after:
  authenticate(req: express.Request, options: Record<string, any>): void {
    const { organization, invitation } = req.query;

    // Throw an error if the organization in the request does not match the organization configured in the strategy
    if (
      organization &&
      this.organization &&
      organization !== this.organization
    ) {
      throw new InputError(
        'Organization mismatch. The organization provided in the request does not match the organization configured in the strategy.',
      );
    }

    super.authenticate(req, {
      ...options,
      ...(organization ? { organization } : {}),
      ...(invitation ? { invitation } : {}),
    });
  }

  authorizationParams(options: Record<string, any>): Record<string, any> {
    const params = super.authorizationParams(options);

    if (options.organization || this.organization) {
      params.organization = options.organization || this.organization;
    }

    if (options.invitation) {
      params.invitation = options.invitation;
    }

    return params;
  }
}
Repository: spotify/backstage
File: plugins/scaffolder-backend/src/actions/index.ts (TypeScript)

Code before:
/*
 * Copyright 2026 The Backstage Authors
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha';
import { AuthService, DiscoveryService } from '@backstage/backend-plugin-api';
import { createValidateScaffolderAction } from './createValidateScaffolderAction';

export const createScaffolderActions = (options: {
  actionsRegistry: ActionsRegistryService;
  auth: AuthService;
  discovery: DiscoveryService;
}) => {
  createValidateScaffolderAction(options);
};


Review comment (refactor):
Thinking here might be a good place to create a client that gets passed through to `createDryRunTemplateAction`.

Code after:
/*
 * Copyright 2026 The Backstage Authors
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha';
import { ScaffolderService } from '@backstage/plugin-scaffolder-node';
import { createDryRunTemplateAction } from './createDryRunTemplateAction';

export const createScaffolderActions = (options: {
  actionsRegistry: ActionsRegistryService;
  scaffolderService: ScaffolderService;
}) => {
  createDryRunTemplateAction(options);
};