ronantakizawa/codereview-bench huggingface dataset:
Repository: appsmithorg/appsmith
File: app/client/src/utils/Analytics/betterbugs.ts (TypeScript)

Code before:
    if (this.instance) {
      log.warn("BetterBugs is already initialised.");

      return;
    }

    if (!apiKey) {
      log.warn("BetterBugs API key not configured.");

      return;
    }

    try {
      const { default: Betterbugs } = await import("@betterbugs/web-sdk");
      const instance = new Betterbugs({
        apiKey,
        mode:
          process.env.NODE_ENV === "development" ? "development" : "production",
        ...(user?.email ? { email: user.email } : {}),
        showActionButton: false,
        styles: this.getDefaultStyles(),
        position: { bottom: "30px", right: "20px" },
        metaData: {
          ...(user?.email ? { user_email: user.email } : {}),
          ...(user?.name ? { user_name: user.name } : {}),
          instance_id: getInstanceId(store.getState()),
          tenant_id: store.getState().organization?.tenantId,
          version: appVersion.id,
          commit_sha: appVersion.sha,
          edition: appVersion.edition,
          release_date: appVersion.releaseDate,
        },
      });

      this.instance = instance as BetterbugsInstance;
    } catch (e) {
      log.error("Failed to initialize BetterBugs:", e);
    }
  }

  public static destroy() {
    try {
      this.instance?.destroy?.();
    } catch (e) {
      log.error("Failed to destroy BetterBugs:", e);
    } finally {
      this.instance = null;
    }
  }

  public static async show(user?: User) {

Review comment (bug):
Betterbugs also has a function to update metadata. Its called setMetadata. 
We can use that to capture additional information like url path, or applicationid, etc

Code after:
        process.env.NODE_ENV === "development" ? "development" : "production",
      showActionButton: false,
      styles: this.getDefaultStyles(),
      mainHeading: "Send support info",
      recordType: "recordVideo",
      position: { bottom: "30px", right: "20px" },
      successMessageHeaderText: "Information received",
      successMessageSubHeaderText:
        "Our support team will use it to review the issue",
      metaData: metadata,
    };

    if (user?.email) {
      params.email = user.email;
      metadata.user_email = user.email;
    }

    if (user?.name) {
      metadata.user_name = user.name;
    }

    return params;
  }

  /**
   * Get default styling for BetterBugs widget (light theme)
   */
  private static getDefaultStyles() {
    return {
      theme: "light" as const,
      primaryColor: APPSMITH_BRAND_PRIMARY_COLOR,
      primaryTextColor: "#ffffff",
    };
  }

  private static getRuntimeMetadata(): Record<string, string | null> {
    if (typeof window === "undefined") {
      return {
        url_path: null,
        application_id: null,
        page_id: null,
      };
    }

    const state = store.getState();
    const applicationId = getCurrentApplicationId(state);
    const pageId = getCurrentPageId(state);
    const urlPath = `${window.location.pathname}${window.location.search}${window.location.hash}`;

    return {
      url_path: urlPath,
Repository: appsmithorg/appsmith
File: app/client/src/RouteChangeListener.tsx (TypeScript)

Code before:
import { routeChanged } from "actions/focusHistoryActions";
import { useEffect, useRef } from "react";
import { useDispatch } from "react-redux";
import { useLocation } from "react-router";
import type { AppsmithLocationState } from "utils/history";
import BetterbugsUtil from "utils/Analytics/betterbugs";

export default function RouteChangeListener() {
  const location = useLocation<AppsmithLocationState>();
  const dispatch = useDispatch();
  const prevLocationRef = useRef(location);

  useEffect(() => {
    const prevLocation = prevLocationRef;

    dispatch(routeChanged(location, prevLocation.current));
    BetterbugsUtil.updateMetadata();
    prevLocation.current = location;
  }, [location.pathname, location.hash]);

  return null;
}


Review comment (bug):
Why do we need to keep capturing the appid and pageid? Shouldn't we just capture this when someone opens the better bug interface?

Code after:
import { routeChanged } from "actions/focusHistoryActions";
import { useEffect, useRef } from "react";
import { useDispatch } from "react-redux";
import { useLocation } from "react-router";
import type { AppsmithLocationState } from "utils/history";

export default function RouteChangeListener() {
  const location = useLocation<AppsmithLocationState>();
  const dispatch = useDispatch();
  const prevLocationRef = useRef(location);

  useEffect(() => {
    const prevLocation = prevLocationRef;

    dispatch(routeChanged(location, prevLocation.current));
    prevLocation.current = location;
  }, [location.pathname, location.hash]);

  return null;
}
Repository: appwrite/appwrite
File: src/Appwrite/Platform/Tasks/ScheduleExecutions.php (PHP)

Code before:
            if (!$schedule['active'] || CronExpression::isValidExpression($schedule['schedule'])) {
                unset($this->schedules[$schedule['resourceId']]);
                continue;
            }

            $now = new \DateTime();
            $scheduledAt = new \DateTime($schedule['schedule']);

            if ($scheduledAt > $now) {
                continue;
            }

            \go(function () use ($schedule, $pools, $dbForConsole) {
                $queue = $pools->get('queue')->pop();
                $connection = $queue->getResource();

                $queueForFunctions = new Func($connection);

                $queueForFunctions
                    ->setType('schedule')
                    ->setFunctionId($schedule['resource']['functionId'])
                    ->setExecution($schedule['resource'])
                    ->setMethod('POST')
                    ->setPath('/')
                    ->setProject($schedule['project'])
                    ->trigger();

                $dbForConsole->deleteDocument(
                    'schedules',
                    $schedule['$id'],
                );

                $queue->reclaim();

                unset($this->schedules[$schedule['resourceId']]);
            });
        }
    }
}


Review comment (suggestion):
setType, setMethod and setPath doesnt sound right. Method and path should be coming from the execution itself - this worker isn't setting it.
setType might in theory stay, but behaviour is closer to HTTP execution, compared to scheduled one.

Code after:
        $queueForFunctions = new Func($connection);

        foreach ($this->schedules as $schedule) {
            if (!$schedule['active']) {
                $dbForConsole->deleteDocument(
                    'schedules',
                    $schedule['$id'],
                );

                unset($this->schedules[$schedule['resourceId']]);
                continue;
            }

            $now = new \DateTime();
            $scheduledAt = new \DateTime($schedule['schedule']);

            if ($scheduledAt > $now) {
                continue;
            }

            $queueForFunctions
                ->setType('schedule')
                // Set functionId instead of function as we don't have $dbForProject
                // TODO: Refactor to use function instead of functionId
                ->setFunctionId($schedule['resource']['functionId'])
                ->setExecution($schedule['resource'])
                ->setMethod($schedule['data']['method'] ?? 'POST')
                ->setPath($schedule['data']['path'] ?? '/')
                ->setHeaders($schedule['data']['headers'] ?? [])
                ->setBody($schedule['data']['body'] ?? '')
                ->setProject($schedule['project'])
                ->trigger();

            $dbForConsole->deleteDocument(
                'schedules',
                $schedule['$id'],
            );

            unset($this->schedules[$schedule['resourceId']]);
        }

        $queue->reclaim();
    }
}
Repository: appwrite/appwrite
File: src/Appwrite/Platform/Tasks/ScheduleExecutions.php (PHP)

Code before:
            }

            $now = new \DateTime();
            $scheduledAt = new \DateTime($schedule['schedule']);

            if ($scheduledAt > $now) {
                continue;
            }

            $queueForFunctions
                ->setType('schedule')
                ->setFunctionId($schedule['resource']['functionId'])
                ->setExecution($schedule['resource'])
                ->setProject($schedule['project'])
                ->trigger();

            $dbForConsole->deleteDocument(
                'schedules',
                $schedule['$id'],
            );

            unset($this->schedules[$schedule['resourceId']]);
        }

        $queue->reclaim();
    }
}


Review comment (suggestion):
As mention in API, here we need to also set stuff like body, headers, method, path, user jwt...
To get those data here, we could add attribute to schedules as temporary storage for those info (since we delete the document right after)

Code after:
                    $schedule['$id'],
                );

                unset($this->schedules[$schedule['resourceId']]);
                continue;
            }

            $now = new \DateTime();
            $scheduledAt = new \DateTime($schedule['schedule']);

            if ($scheduledAt > $now) {
                continue;
            }

            $queueForFunctions
                ->setType('schedule')
                // Set functionId instead of function as we don't have $dbForProject
                // TODO: Refactor to use function instead of functionId
                ->setFunctionId($schedule['resource']['functionId'])
                ->setExecution($schedule['resource'])
                ->setMethod($schedule['data']['method'] ?? 'POST')
                ->setPath($schedule['data']['path'] ?? '/')
                ->setHeaders($schedule['data']['headers'] ?? [])
                ->setBody($schedule['data']['body'] ?? '')
                ->setProject($schedule['project'])
                ->trigger();

            $dbForConsole->deleteDocument(
                'schedules',
                $schedule['$id'],
            );

            unset($this->schedules[$schedule['resourceId']]);
        }

        $queue->reclaim();
    }
}
Repository: appwrite/appwrite
File: src/Appwrite/Auth/Validator/MockNumber.php (PHP)

Code before:
        return $this->message;
    }

    /**
     * Is valid.
     *
     * @param mixed $value
     *
     * @return bool
     */
    public function isValid($value): bool
    {
        if (!\is_array($value) || !isset($value['phone']) || !isset($value['otp'])) {
            $this->message = 'Invalid payload structure. Please check the "phone" and "otp" fields';
            return false;
        }

        $phone = new Phone();
        if (!$phone->isValid($value['phone'])) {
            $this->message = $phone->getDescription();
            return false;
        }

        $otp = new Text(6, 6);
        if (!$otp->isValid($value['otp']) || !\ctype_digit($value['otp'])) {
            $this->message = 'Invalid OTP. Please make sure the OTP is a 6 digit number';
            return false;
        }

        return true;
    }

    /**
     * Is array
     *
     * Function will return true if object is array.
     *
     * @return bool
     */
    public function isArray(): bool
    {
        return false;
    }

    /**
     * Get Type
     *
     * Returns validator type.
     *
     * @return string
     */

Review comment (suggestion):
Third parameter in text validator is `array $allowList = [])`, you can pass `Text::NUMBERS` to only allow numbers.

Code after:
        return $this->message;
    }

    /**
     * Is valid.
     *
     * @param mixed $value
     *
     * @return bool
     */
    public function isValid($value): bool
    {
        if (!\is_array($value) || !isset($value['phone']) || !isset($value['otp'])) {
            $this->message = 'Invalid payload structure. Please check the "phone" and "otp" fields';
            return false;
        }

        $phone = new Phone();
        if (!$phone->isValid($value['phone'])) {
            $this->message = $phone->getDescription();
            return false;
        }

        $otp = new Text(6, 6, Text::NUMBERS);
        if (!$otp->isValid($value['otp'])) {
            $this->message = 'Invalid OTP. Please make sure the OTP is a 6 digit number';
            return false;
        }

        return true;
    }

    /**
     * Is array
     *
     * Function will return true if object is array.
     *
     * @return bool
     */
    public function isArray(): bool
    {
        return false;
    }

    /**
     * Get Type
     *
     * Returns validator type.
     *
     * @return string
     */
Repository: appwrite/appwrite
File: src/Appwrite/Migration/Version/V21.php (PHP)

Code before:
                break;
            case 'functions':
                // Add scopes attribute
                $document->setAttribute('scopes', []);

                // Add size attribute
                $document->setAttribute('size', 's-1vcpu-512m');
        }

        return $document;
    }

    /**
     * Migrating Buckets.
     *
     * @return void
     * @throws Exception
     * @throws PDOException
     */
    private function migrateBuckets()
    {
        foreach ($this->documentsIterator('buckets') as $bucket) {
            $bucketId = 'bucket_' . $bucket['$internalId'];

            $this->projectDB->updateAttribute($bucketId, 'metadata', size: 75000);
        }
    }
}


Review comment (bug):
We should wrap with try catch so it doesn't fail catastrophically and purge cache. For example: 

https://github.com/appwrite/appwrite/blob/4b1bf59091b747c5855c355b6f48a5346b1cbe0c/src/Appwrite/Migration/Version/V19.php#L101-L114

Code after:
                break;
            case 'functions':
                // Add scopes attribute
                $document->setAttribute('scopes', []);

                // Add size attribute
                $document->setAttribute('size', 's-1vcpu-512m');
        }

        return $document;
    }

    /**
     * Migrating Buckets.
     *
     * @return void
     */
    private function migrateBuckets()
    {
        foreach ($this->documentsIterator('buckets') as $bucket) {
            $bucketId = 'bucket_' . $bucket['$internalId'];

            try {
                $this->projectDB->updateAttribute($bucketId, 'metadata', size: 75000);
                $this->projectDB->purgeCachedCollection($bucketId);
            } catch (\Throwable $th) {
                Console::warning("'bucketId' from {$bucketId}: {$th->getMessage()}");
            }
        }
    }
}
Repository: appwrite/appwrite
File: app/init.php (PHP)

Code before:
            $p = Authorization::skip(fn () => $dbForConsole->getDocument('projects', $pid));
            $teamInternalId = $p->getAttribute('teamInternalId', '');
        } elseif ($path === '/v1/projects') {
            $teamId = $request->getParam('teamId', '');
            $team = Authorization::skip(fn () => $dbForConsole->getDocument('teams', $teamId));
            return $team;
        }
    }

    $team = Authorization::skip(function () use ($dbForConsole, $teamInternalId) {
        return $dbForConsole->findOne('teams', [
            Query::equal('$internalId', [$teamInternalId]),
        ]);
    });

    return $team;
}, ['project', 'dbForConsole', 'utopia', 'request']);

App::setResource(
    'isResourceBlocked',
    fn () => fn (Document $project, string $resourceType, ?string $resourceId) => false
);

App::setResource('previewHost', function (Request $request) {
    if (App::isDevelopment()) {
        $host = $request->getQuery('preview') ?? '';
        if (!empty($host)) {
            return $host;
        }
    }

    return '';
}, ['request']);


Review comment (suggestion):
```suggestion
        $host = $request->getQuery('appwrite-hostname') ?? '';
```

Code after:
            $p = Authorization::skip(fn () => $dbForConsole->getDocument('projects', $pid));
            $teamInternalId = $p->getAttribute('teamInternalId', '');
        } elseif ($path === '/v1/projects') {
            $teamId = $request->getParam('teamId', '');
            $team = Authorization::skip(fn () => $dbForConsole->getDocument('teams', $teamId));
            return $team;
        }
    }

    $team = Authorization::skip(function () use ($dbForConsole, $teamInternalId) {
        return $dbForConsole->findOne('teams', [
            Query::equal('$internalId', [$teamInternalId]),
        ]);
    });

    return $team;
}, ['project', 'dbForConsole', 'utopia', 'request']);

App::setResource(
    'isResourceBlocked',
    fn () => fn (Document $project, string $resourceType, ?string $resourceId) => false
);

App::setResource('previewHostname', function (Request $request) {
    if (App::isDevelopment()) {
        $host = $request->getQuery('appwrite-hostname') ?? '';
        if (!empty($host)) {
            return $host;
        }
    }

    return '';
}, ['request']);
Repository: appwrite/appwrite
File: app/controllers/api/console.php (PHP)

Code before:
            $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]);

            return $len;
        });

        curl_setopt($ch, CURLOPT_POSTFIELDS, $query);

        curl_exec($ch);

        curl_close($ch);

        $response->chunk('', true);
    });

App::get('v1/console/resources/:resourceId')
    ->desc('Check resource ID availability')
    ->groups(['api', 'projects'])
    ->label('scope', 'projects.read')
    ->label('sdk.auth', [APP_AUTH_TYPE_ADMIN])
    ->label('sdk.namespace', 'console')
    ->label('sdk.method', 'checkResourceAvailability')
    ->label('sdk.response.code', Response::STATUS_CODE_OK)
    ->label('sdk.response.type', Response::CONTENT_TYPE_JSON)
    ->label('sdk.response.model', Response::MODEL_NONE)
    ->label('abuse-limit', 10)
    ->label('abuse-key', 'userId:{userId}')
    ->param('resourceId', '', new UID(), 'ID of the resource.')
    ->param('type', '', new WhiteList(['rules']), 'Resource type.')
    ->inject('response')
    ->inject('dbForConsole')
    ->action(function (string $resourceId, string $type, Response $response, Database $dbForConsole) {
        $document = Authorization::skip(fn () => $dbForConsole->getDocument('rules', $resourceId));

        if (!$document->isEmpty()) {
            throw new Exception(Exception::RESOURCE_ALREADY_EXISTS, 'Resource ID is already in use.');
        }

        $response->noContent();
    });


Review comment (suggestion):
```suggestion
    ->label('abuse-key', 'userId:{userId},url:{url}')
```

Code after:
        curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'POST');
        curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
        curl_setopt($ch, CURLOPT_HTTPHEADER, $headers);
        curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 0);
        curl_setopt($ch, CURLOPT_TIMEOUT, 9000);
        curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) {
            $len = strlen($header);
            $header = explode(':', $header, 2);

            if (count($header) < 2) { // ignore invalid headers
                return $len;
            }

            $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]);

            return $len;
        });

        curl_setopt($ch, CURLOPT_POSTFIELDS, $query);

        curl_exec($ch);

        curl_close($ch);

        $response->chunk('', true);
    });
Repository: appwrite/appwrite
File: app/controllers/api/console.php (PHP)

Code before:

            if (count($header) < 2) { // ignore invalid headers
                return $len;
            }

            $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]);

            return $len;
        });

        curl_setopt($ch, CURLOPT_POSTFIELDS, $query);

        curl_exec($ch);

        curl_close($ch);

        $response->chunk('', true);
    });

App::get('v1/console/resources/:resourceId')
    ->desc('Check resource ID availability')
    ->groups(['api', 'projects'])
    ->label('scope', 'projects.read')
    ->label('sdk.auth', [APP_AUTH_TYPE_ADMIN])
    ->label('sdk.namespace', 'console')
    ->label('sdk.method', 'checkResourceAvailability')
    ->label('sdk.response.code', Response::STATUS_CODE_OK)
    ->label('sdk.response.type', Response::CONTENT_TYPE_JSON)
    ->label('sdk.response.model', Response::MODEL_NONE)
    ->label('abuse-limit', 10)
    ->label('abuse-key', 'userId:{userId}')
    ->param('resourceId', '', new UID(), 'ID of the resource.')
    ->param('type', '', new WhiteList(['rules']), 'Resource type.')
    ->inject('response')
    ->inject('dbForConsole')
    ->action(function (string $resourceId, string $type, Response $response, Database $dbForConsole) {
        $document = Authorization::skip(fn () => $dbForConsole->getDocument('rules', $resourceId));

        if (!$document->isEmpty()) {
            throw new Exception(Exception::RESOURCE_ALREADY_EXISTS, 'Resource ID is already in use.');
        }

        $response->noContent();
    });


Review comment (suggestion):
```suggestion
    ->label('sdk.method', 'getResourceAvailability')
```

Code after:
            return \strlen($data);
        };

        curl_setopt($ch, CURLOPT_WRITEFUNCTION, $handleEvent);

        curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'POST');
        curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
        curl_setopt($ch, CURLOPT_HTTPHEADER, $headers);
        curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 0);
        curl_setopt($ch, CURLOPT_TIMEOUT, 9000);
        curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) {
            $len = strlen($header);
            $header = explode(':', $header, 2);

            if (count($header) < 2) { // ignore invalid headers
                return $len;
            }

            $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]);

            return $len;
        });

        curl_setopt($ch, CURLOPT_POSTFIELDS, $query);

        curl_exec($ch);

        curl_close($ch);

        $response->chunk('', true);
    });