ronantakizawa/codereview-bench huggingface dataset:
Repository: All-Hands-AI/OpenHands
File: frontend/__tests__/components/features/home/git-repo-dropdown.test.tsx (TypeScript)

Code before:
import { render, screen, waitFor } from "@testing-library/react";
import { describe, expect, vi, beforeEach, it } from "vitest";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import userEvent from "@testing-library/user-event";
import { GitRepoDropdown } from "../../../../src/components/features/home/git-repo-dropdown/git-repo-dropdown";
import { GitRepository } from "#/types/git";

// Mock the repository data hook
const mockUseRepositoryData = vi.fn();
vi.mock(
  "../../../../src/components/features/home/git-repo-dropdown/use-repository-data",
  () => ({
    useRepositoryData: (...args: unknown[]) => mockUseRepositoryData(...args),
  }),
);

// Mock the URL search hook
const mockUseUrlSearch = vi.fn();
vi.mock(
  "../../../../src/components/features/home/git-repo-dropdown/use-url-search",
  () => ({
    useUrlSearch: (...args: unknown[]) => mockUseUrlSearch(...args),
  }),
);

// Mock useConfig
vi.mock("#/hooks/query/use-config", () => ({
  useConfig: () => ({ data: null }),
}));

// Mock useHomeStore
vi.mock("#/stores/home-store", () => ({
  useHomeStore: () => ({ recentRepositories: [] }),
}));

const MOCK_REPOSITORIES: GitRepository[] = [

Review comment (suggestion):
**question:** Would it be possible to use a pattern like this? Thank you! 🙏 

```ts
vi.mock("#/components/.../use-repository-data", ...)
```

Code after:
import { render, screen, waitFor } from "@testing-library/react";
import { describe, expect, vi, beforeEach, it } from "vitest";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import userEvent from "@testing-library/user-event";
import { GitRepoDropdown } from "../../../../src/components/features/home/git-repo-dropdown/git-repo-dropdown";
import { GitRepository } from "#/types/git";

// Mock the repository data hook
const mockUseRepositoryData = vi.fn();
vi.mock(
  "#/components/features/home/git-repo-dropdown/use-repository-data",
  () => ({
    useRepositoryData: (...args: unknown[]) => mockUseRepositoryData(...args),
  }),
);

// Mock the URL search hook
const mockUseUrlSearch = vi.fn();
vi.mock("#/components/features/home/git-repo-dropdown/use-url-search", () => ({
  useUrlSearch: (...args: unknown[]) => mockUseUrlSearch(...args),
}));

// Mock useConfig
vi.mock("#/hooks/query/use-config", () => ({
  useConfig: () => ({ data: null }),
}));

// Mock useHomeStore
vi.mock("#/stores/home-store", () => ({
  useHomeStore: () => ({ recentRepositories: [] }),
}));

const MOCK_REPOSITORIES: GitRepository[] = [
  {
    id: "1",
    full_name: "user/repo-one",
Repository: AmbientRun/Ambient
File: crates/wasm/src/shared/implementation/asset.rs (Rust)

Code before:
use ambient_std::{
    asset_cache::SyncAssetKeyExt,
    asset_url::{AssetUrl, ServerBaseUrlKey, ProxyBaseUrlKey},
};

use ambient_core::{
    asset_cache
};

use ambient_ecs::World;

pub(crate) fn url(
    world: &mut World,
    path: String,
) -> anyhow::Result<Option<String>> {
    let assets = world.resource(asset_cache());
    let base_url = ProxyBaseUrlKey.try_get(assets).unwrap_or_else(|| ServerBaseUrlKey.get(assets));
    Ok(Some(AssetUrl::parse(path)?.resolve(&base_url)?.to_string()))
    // Ok(Some(String::from("42")))
}

Review comment (suggestion):
ProxyBaseUrlKey and ServerBaseUrlKey won't exist on the client side when users join with `ambient join`. They need to be communicated to the client in that case (that's what the `synced_resource` thing I mentioned could be used for).

Code after:
use ambient_std::{
    asset_url::{AbsAssetUrl, AssetUrl}
};

use ambient_network::{ServerWorldExt, server::content_base_url};
use ambient_ecs::World;

pub(crate) fn url(
    world: &World,
    path: String,
) -> anyhow::Result<Option<String>> {
    let url = world.synced_resource(content_base_url()).unwrap();
    let base_url = &AbsAssetUrl::parse(url)?;
    let asset_url = AssetUrl::parse(path)?.resolve(&base_url)?;
    Ok(Some(asset_url.to_string()))
}
Repository: AmbientRun/Ambient
File: campfire/src/web/build.rs (Rust)

Code before:
use anyhow::Context;
use clap::{Args, ValueEnum};
use std::path::PathBuf;
use tokio::process::Command;

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)]
pub(crate) enum Target {
    /// Generates a wasm and js shim that uses `require` to import the `.wasm`
    Bundler,
    /// The shim won't import the `.wasm` itself, allowing for external fetching
    Standalone,
}

#[derive(Debug, Args, Clone)]
pub struct BuildOptions {
    #[arg(long, default_value = "dev")]
    pub profile: String,
    #[arg(long, default_value = "pkg")]
    pub pkg_name: String,
    #[arg(long, value_enum, default_value = "bundler")]
    target: Target,
}

impl BuildOptions {
    pub async fn check(&self) -> anyhow::Result<()> {
        let mut command = Command::new("cargo");

        command
            .args(["check", "-p", "ambient_web"])
            .current_dir("web")
            .kill_on_drop(true);

        match &self.profile[..] {
            "dev" | "debug" => {
                // default
            }
            "release" => {
                command.arg("--release");
            }
            v => anyhow::bail!("Unknown profile: {v:?}"),
        };

        let res = command.spawn()?.wait().await?;

Review comment (suggestion):
Would suggest `Profile` (yes, `Profile::Profile`) so that they're all nouns and users won't be confused

Code after:
use anyhow::Context;
use clap::{Args, ValueEnum};
use std::path::PathBuf;
use tokio::process::Command;

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)]
pub(crate) enum Target {
    /// Generates a wasm and js shim that uses `require` to import the `.wasm`
    Bundler,
    /// The shim won't import the `.wasm` itself, allowing for external fetching
    Standalone,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)]
pub(crate) enum Profile {
    Dev,
    Release,
    Profiling,
}

#[derive(Debug, Args, Clone)]
pub struct BuildOptions {
    #[arg(long, default_value = "dev")]
    pub(crate) profile: Profile,
    #[arg(long, default_value = "pkg")]
    pub pkg_name: String,
    #[arg(long, value_enum, default_value = "bundler")]
    target: Target,
}

impl BuildOptions {
    pub async fn check(&self) -> anyhow::Result<()> {
        let mut command = Command::new("cargo");

        command
            .args(["check", "-p", "ambient_web"])
            .current_dir("web")
            .kill_on_drop(true);

        match &self.profile {
            Profile::Dev => {
                // default
            }
Repository: Automattic/pocket-casts-android
File: .buildkite/commands/assemble-release-apk.sh (Shell)

Code before:
#!/bin/bash -eu

if .buildkite/commands/should-skip-job.sh --job-type build; then
  exit 0
fi

"$(dirname "${BASH_SOURCE[0]}")/restore-cache.sh"

echo "--- :rubygems: Setting up Gems"

install_gems

echo "--- :closed_lock_with_key: Installing Secrets"

bundle exec fastlane run configure_apply

echo "--- ⚙️ Building release variant"

./gradlew "$1:assembleRelease" -PskipSentryProguardMappingUpload=true


Review comment (suggestion):
What do you think of using this:

```suggestion
if "$(dirname "${BASH_SOURCE[0]}")/should-skip-job.sh --job-type build; then
```

I like it because it makes the path relative. I doubt we'll ever want to move the files from where they are, but I like that we _could_, seems like a good practice to have.

Code after:
#!/bin/bash -eu

if "$(dirname "${BASH_SOURCE[0]}")/should-skip-job.sh" --job-type build; then
  exit 0
fi

"$(dirname "${BASH_SOURCE[0]}")/restore-cache.sh"

echo "--- :rubygems: Setting up Gems"

install_gems

echo "--- :closed_lock_with_key: Installing Secrets"

bundle exec fastlane run configure_apply

echo "--- ⚙️ Building release variant"

./gradlew "$1:assembleRelease" -PskipSentryProguardMappingUpload=true
Repository: Automattic/pocket-casts-android
File: modules/features/account/src/main/java/au/com/shiftyjelly/pocketcasts/account/onboarding/upgrade/OnboardingUpgradeScreen.kt (Kotlin)

Code before:
private fun ScheduleContent(
    trialSchedule: ContentPage.TrialSchedule,
    onCtaClicked: () -> Unit,
) {
    LazyColumn {
        item {
            UpgradeTrialTimeline(
                items = trialSchedule.timelineItems,
            )
        }
        if (trialSchedule.showCta) {
            item {
                Spacer(modifier = Modifier.height(24.dp))
                TextP40(
                    text = stringResource(LR.string.onboarding_upgrade_schedule_see_features),
                    modifier = Modifier.clickable { onCtaClicked() },
                    color = MaterialTheme.theme.colors.primaryInteractive01,
                )
            }
        }
    }
}

@Preview
@Composable
private fun PreviewOnboardingUpgradeScreen(
    @PreviewParameter(ThemePreviewParameterProvider::class) theme: ThemeType,
) {
    AppThemeWithBackground(theme) {
        OnboardingUpgradeScreen(
            modifier = Modifier.fillMaxSize(),
            onSubscribePressed = {},
            variant = Variants.VARIANT_FEATURES,
            onClosePressed = {},
        )
    }
}


Review comment (suggestion):
This should to be wrapped in `Column`, split into separate items, or changed into top padding of the `TextP40`.

Code after:
            item {
                TextP40(
                    text = stringResource(LR.string.onboarding_upgrade_features_trial_schedule),
                    modifier = Modifier.padding(top = 24.dp)
                        .clickable { onCtaClick() },
                    color = MaterialTheme.theme.colors.primaryInteractive01,
                )
            }
        }
    }
}

@Composable
private fun ScheduleContent(
    trialSchedule: UpgradePagerContent.TrialSchedule,
    onCtaClick: () -> Unit,
) {
    FadedLazyColumn {
        item {
            UpgradeTrialTimeline(
                items = trialSchedule.timelineItems,
            )
        }
        if (trialSchedule.showCta) {
            item {
                TextP40(
                    text = stringResource(LR.string.onboarding_upgrade_schedule_see_features),
                    modifier = Modifier.padding(top = 24.dp)
                        .clickable { onCtaClick() },
                    color = MaterialTheme.theme.colors.primaryInteractive01,
                )
            }
        }
    }
}

@Preview
@Composable
private fun PreviewOnboardingUpgradeScreen(
    @PreviewParameter(ThemePreviewParameterProvider::class) theme: ThemeType,
) {
    AppThemeWithBackground(theme) {
        OnboardingUpgradeScreen(
            modifier = Modifier.fillMaxSize(),
            onSubscribePress = {},
            onClosePress = {},
            variant = Variants.VARIANT_FEATURES,
        )
    }
}
Repository: Automattic/pocket-casts-android
File: app/build.gradle.kts (Kotlin)

Code before:
    }

    buildFeatures {
        buildConfig = true
        viewBinding = true
        compose = true
    }

    buildTypes {
        named("debug") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_radioactive"
        }

        named("debugProd") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_3"
        }

        maybeCreate("prototype").apply {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_prototype"

            isMinifyEnabled = true
            isShrinkResources = true
            proguardFiles.addAll(
                listOf(
                    getDefaultProguardFile("proguard-android-optimize.txt"),
                    rootProject.file("proguard-rules.pro"),
                    rootProject.file("proguard-rules-no-obfuscate.pro"),
                ),
            )
        }

        named("release") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher"

            if (!file("${project.rootDir}/sentry.properties").exists()) {
                println("WARNING: Sentry configuration file 'sentry.properties' not found. The ProGuard mapping files won't be uploaded.")
            }
        }
    }

    lint {
        checkDependencies = true
    }
}

dependencies {
    ksp(libs.dagger.hilt.compiler)
    ksp(libs.hilt.compiler)

    implementation(platform(libs.compose.bom))
    implementation(platform(libs.firebase.bom))

Review comment (suggestion):
Question (❓): Do we need  to add this `proguard-rules.pro` if we then (kind of) overwrite that with the next `proguard-rules-no-obfuscate.pro` file? 🤔

Code after:
    }

    buildFeatures {
        buildConfig = true
        viewBinding = true
        compose = true
    }

    buildTypes {
        named("debug") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_radioactive"
        }

        named("debugProd") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_3"
        }

        maybeCreate("prototype").apply {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher_prototype"
        }

        named("release") {
            manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher"

            if (!file("${project.rootDir}/sentry.properties").exists()) {
                println("WARNING: Sentry configuration file 'sentry.properties' not found. The ProGuard mapping files won't be uploaded.")
            }
        }
    }

    lint {
        checkDependencies = true
    }
}

dependencies {
    ksp(libs.dagger.hilt.compiler)
    ksp(libs.hilt.compiler)

    implementation(platform(libs.compose.bom))
    implementation(platform(libs.firebase.bom))

    implementation(libs.androidx.annotation)
    implementation(libs.androidx.appcompat)
    implementation(libs.androidx.core.ktx)
    implementation(libs.androidx.mediarouter)
    implementation(libs.appsflyer) {
        exclude(group = "com.google.android.play", module = "integrity")
    }
    implementation(libs.automattic.crashlogging)
    implementation(libs.coil)
Repository: Automattic/pocket-casts-android
File: modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/search/SearchService.kt (Kotlin)

Code before:
package au.com.shiftyjelly.pocketcasts.servers.search

import retrofit2.http.GET
import retrofit2.http.Query

interface SearchService {
    @GET("/autocomplete/search")
    suspend fun autoCompleteSearch(
        @Query("q") query: String,
        @Query("termsLimit") terms: Int,
        @Query("podcastsLimit") podcasts: Int,
        @Query("language") language: String,
    ) : AutoCompleteResponse
}

Review comment (suggestion):
❓ Could we use the full names for the parameters as well? Instead of `terms`, `termsLimit`? `autoCompleteSearch(terms = 5, podcasts = 6)` doesn't read clearly.

Code after:
package au.com.shiftyjelly.pocketcasts.servers.search

import retrofit2.http.GET
import retrofit2.http.Query

interface SearchService {
    @GET("/autocomplete/search")
    suspend fun autoCompleteSearch(
        @Query("q") query: String,
        @Query("termsLimit") termsLimit: Int,
        @Query("podcastsLimit") podcastsLimit: Int,
        @Query("language") language: String,
    ): AutoCompleteResponse
}
Repository: Automattic/pocket-casts-android
File: modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/coroutines/SyncedAction.kt (Kotlin)

Code before:
package au.com.shiftyjelly.pocketcasts.utils.coroutines

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock

class SyncedAction<InputT, OutputT>(
    private val action: suspend (InputT) -> OutputT,
) {
    private val executeMutex = Mutex()
    private var executeCount = 0
    private var inFlightResult: Deferred<OutputT>? = null

    suspend fun run(input: InputT, scope: CoroutineScope): OutputT {
        val deferred = executeMutex.withLock {
            val inFlight = inFlightResult ?: scope.async(start = CoroutineStart.LAZY) { action(input) }
            inFlightResult = inFlight
            executeCount++
            inFlight
        }

        return try {
            deferred.await()
        } finally {
            executeMutex.withLock {
                executeCount--
                if (executeCount == 0) {
                    inFlightResult = null
                }
            }
        }
    }

Review comment (suggestion):
⛏️ personal opinion:`InputT` and `OutputT` sounds a bit wacky to me, I'd rather name them `XType` or just use `In` and `Out`

Code after:
package au.com.shiftyjelly.pocketcasts.utils.coroutines

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock

class SyncedAction<InputType, OutputType>(
    private val action: suspend (InputType) -> OutputType,
) {
    private val executeMutex = Mutex()
    private var executeCount = 0
    private var inFlightResult: Deferred<OutputType>? = null

    suspend fun run(input: InputType, scope: CoroutineScope): OutputType {
        val deferred = executeMutex.withLock {
            val inFlight = inFlightResult ?: scope.async(start = CoroutineStart.LAZY) { action(input) }
            inFlightResult = inFlight
            executeCount++
            inFlight
        }

        return try {
            deferred.await()
        } finally {
            executeMutex.withLock {
                executeCount--
                if (executeCount == 0) {
                    inFlightResult = null
                }
            }
        }
    }
Repository: Automattic/pocket-casts-android
File: modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/appreview/AppReviewExceptionHandler.kt (Kotlin)

Code before:
package au.com.shiftyjelly.pocketcasts.repositories.appreview

import au.com.shiftyjelly.pocketcasts.preferences.Settings
import java.time.Clock
import javax.inject.Inject

class AppReviewExceptionHandler @Inject constructor(
    private val settings: Settings,
    private val clock: Clock,
) : Thread.UncaughtExceptionHandler {
    override fun uncaughtException(t: Thread, e: Throwable) {
        runCatching {
            settings.appReviewCrashTimestamp.set(clock.instant(), updateModifiedAt = false)
        }
    }
}


Review comment (bug):
```suggestion
            settings.appReviewCrashTimestamp.set(clock.instant(), updateModifiedAt = false, commit = true)
```
I had a few times where this timestamp wasn't being set on my Pixel 9. Adding the commit set to true seems to help. 

Code after:
package au.com.shiftyjelly.pocketcasts.repositories.appreview

import au.com.shiftyjelly.pocketcasts.preferences.Settings
import java.time.Clock
import javax.inject.Inject

class AppReviewExceptionHandler @Inject constructor(
    private val settings: Settings,
    private val clock: Clock,
) : Thread.UncaughtExceptionHandler {
    override fun uncaughtException(t: Thread, e: Throwable) {
        runCatching {
            settings.appReviewCrashTimestamp.set(clock.instant(), updateModifiedAt = false, commit = true)
        }
    }
}
Repository: Automattic/pocket-casts-android
File: modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/payment/AnalyticsPaymentListenerTest.kt (Kotlin)

Code before:
                "product" to "com.pocketcasts.plus.yearly",
                "source" to "purchase_source",
                "error" to "unknown",
                "error_code" to 404,
            ),
        )
    }

    @Test
    fun `legacy product property`() = runTest {
        val keys = listOf(
            SubscriptionPlan.PlusMonthlyPreview.key,
            SubscriptionPlan.PlusYearlyPreview.key,
            SubscriptionPlan.PatronMonthlyPreview.key,
            SubscriptionPlan.PatronYearlyPreview.key,
        )

        for (key in keys) {
            paymentClient.purchaseSubscriptionPlan(key, "purchase_source", mock<Activity>())
        }

        val productProperties = tracker.events.map { it.properties["product"] }
        assertEquals(
            listOf(
                "com.pocketcasts.plus.monthly",
                "com.pocketcasts.plus.yearly",
                "com.pocketcasts.monthly.patron",
                "com.pocketcasts.yearly.patron",
            ),
            productProperties,
        )
    }
}

private class TestTracker : Tracker {
    private val _events = mutableListOf<TrackEvent>()

    val events get() = _events.toList()

    override fun track(event: AnalyticsEvent, properties: Map<String, Any>) {
        _events += TrackEvent(event, properties)
    }

    override fun refreshMetadata() = Unit

    override fun flush() = Unit

    override fun clearAllData() = Unit
}

private data class TrackEvent(

Review comment (suggestion):
❓ Was this discussed somewhere in details? This change can potentially break any dashboards that we have and rely on the `product` analytics value.

Code after:
                "error_code" to 404,
            ),
        )
    }

    @Test
    fun `legacy product property`() = runTest {
        val keys = listOf(
            SubscriptionPlan.PlusMonthlyPreview.key,
            SubscriptionPlan.PlusYearlyPreview.key,
            SubscriptionPlan.PatronMonthlyPreview.key,
            SubscriptionPlan.PatronYearlyPreview.key,
        )

        for (key in keys) {
            paymentClient.purchaseSubscriptionPlan(key, "purchase_source", mock<Activity>())
        }

        val productProperties = tracker.events.map { it.properties["product"] }
        assertEquals(
            listOf(
                "monthly",
                "yearly",
                "com.pocketcasts.monthly.patron",
                "com.pocketcasts.yearly.patron",
            ),
            productProperties,
        )
    }
}

private class TestTracker : Tracker {
    private val _events = mutableListOf<TrackEvent>()

    val events get() = _events.toList()

    override fun track(event: AnalyticsEvent, properties: Map<String, Any>) {
        _events += TrackEvent(event, properties)
    }

    override fun refreshMetadata() = Unit

    override fun flush() = Unit

    override fun clearAllData() = Unit
}

private data class TrackEvent(
    val type: AnalyticsEvent,
    val properties: Map<String, Any>,
) {