ronantakizawa/codereview-bench huggingface dataset:
Repository: Automattic/pocket-casts-android
File: wear/src/main/kotlin/au/com/shiftyjelly/pocketcasts/wear/ui/WatchListScreen.kt (Kotlin)

Code before:
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.profile_navigation_files),
                iconRes = IR.drawable.ic_file,
                onClick = {
                    viewModel.onFilesClicked()
                    navigateToRoute(FilesScreen.ROUTE)
                },
            )
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.profile_navigation_starred),
                iconRes = IR.drawable.ic_starred,
                onClick = {
                    viewModel.onFilesClicked()
                    navigateToRoute(StarredScreen.ROUTE)
                },
            )
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.settings),
                iconRes = IR.drawable.ic_profile_settings,
                onClick = {
                    viewModel.onSettingsClicked()
                    navigateToRoute(SettingsScreen.ROUTE)
                },
            )
        }
    }
}

@Preview(device = WearDevices.SMALL_ROUND)
@Composable
private fun WatchListPreview() {
    WearAppTheme {
        WatchListScreen(
            toNowPlaying = {},
            navigateToRoute = {},
            columnState = ScalingLazyColumnState(),
        )
    }
}


Review comment (suggestion):
`showSystemUi` was showing the Pocket Casts icon in the middle of the screen. It might be an Android Studio issue but I have removed it for now.

Code after:
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.profile_navigation_files),
                iconRes = IR.drawable.ic_file,
                onClick = {
                    viewModel.onFilesClicked()
                    navigateToRoute(FilesScreen.ROUTE)
                },
            )
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.profile_navigation_starred),
                iconRes = IR.drawable.ic_starred,
                onClick = {
                    viewModel.onStarredClicked()
                    navigateToRoute(StarredScreen.ROUTE)
                },
            )
        }

        item {
            WatchListChip(
                title = stringResource(LR.string.settings),
                iconRes = IR.drawable.ic_profile_settings,
                onClick = {
                    viewModel.onSettingsClicked()
                    navigateToRoute(SettingsScreen.ROUTE)
                },
            )
        }
    }
}

@Preview(device = WearDevices.SMALL_ROUND)
@Composable
private fun WatchListPreview() {
    WearAppTheme {
        WatchListScreen(
            toNowPlaying = {},
            navigateToRoute = {},
            columnState = ScalingLazyColumnState(),
        )
    }
}
Repository: Automattic/pocket-casts-ios
File: podcasts/DiscoverViewController+CategoryRedesign.swift (Swift)

Code before:
import PocketCastsServer

private let popularItemsCount = 5

extension DiscoverViewController {
    /// Reloads discover, keeping the items listed in `exclude`
    /// - Parameters:
    ///   - items: Items to exclude from the reload process. These items will REMAIN in Discover
    ///   - category: The `DiscoverCategory` to add to the layout. This is sort of an artifical `DiscoverLayout`.
    func reload(except items: [DiscoverItem], category: DiscoverCategory?) {
        let newLayout: DiscoverLayout?

        if let category {
            newLayout = modified(layout: discoverLayout, for: category, with: items)
        } else {
            newLayout = discoverLayout
        }

        populateFrom(discoverLayout: newLayout, shouldInclude: {
            ($0.categoryID == category?.id) || items.contains($0)
        }, shouldReset: {
            !items.contains($0)
        })

        guard let category else { return }
        addCategoryVC(for: category, regions: items.first?.regions ?? [])
    }


Review comment (suggestion):
Instead of this why not declare an enum inside the category like this:

```
extension DiscoverViewController {
    private enum Constant {
        static let popularItemsCount = 5
    }
...
```

Code after:
import PocketCastsServer

extension DiscoverViewController {
    private enum Constants {
        static let popularItemsCount = 5
    }

    /// Reloads discover, keeping the items listed in `exclude`
    /// - Parameters:
    ///   - items: Items to exclude from the reload process. These items will REMAIN in Discover
    ///   - category: The `DiscoverCategory` to add to the layout. This is sort of an artifical `DiscoverLayout`.
    func reload(except items: [DiscoverItem], category: DiscoverCategory?) {
        let newLayout: DiscoverLayout?

        if let category {
            newLayout = modified(layout: discoverLayout, for: category, with: items)
        } else {
            newLayout = discoverLayout
        }

        populateFrom(discoverLayout: newLayout, shouldInclude: {
            ($0.categoryID == category?.id) || items.contains($0)
        }, shouldReset: {
            !items.contains($0)
        })

        guard let category else { return }
        addCategoryVC(for: category, regions: items.first?.regions ?? [])
Repository: Automattic/pocket-casts-ios
File: podcasts/HeadingCell.swift (Swift)

Code before:
import UIKit

class HeadingCell: ThemeableCell {
    @IBOutlet var heading: UILabel!
    @IBOutlet var button: UIButton!

    var action: (() -> ())?

    override func setSelected(_ selected: Bool, animated: Bool) {}
    override func setHighlighted(_ highlighted: Bool, animated: Bool) {}

    @IBAction func buttonTapped(_ sender: UIButton) {
        action?()
    }
}


Review comment (suggestion):
Nit: 
```suggestion
    var action: (() -> Void)?
```

Code after:
import UIKit

class HeadingCell: ThemeableCell {
    @IBOutlet var heading: UILabel!
    @IBOutlet var button: UIButton!

    var action: (() -> Void)?

    override func setSelected(_ selected: Bool, animated: Bool) {}
    override func setHighlighted(_ highlighted: Bool, animated: Bool) {}

    @IBAction func buttonTapped(_ sender: UIButton) {
        action?()
    }
}
Repository: Automattic/pocket-casts-ios
File: Modules/DataModel/Sources/PocketCastsDataModel/Private/Managers/Util/DatabaseHelper.swift (Swift)

Code before:
import FMDB
import Foundation
import PocketCastsUtils

class DatabaseHelper {
    class func setup(queue: PCDBQueue) {
        var startingSchemaVersion: Int32 = 0

        queue.inDatabase { db in
            do {
                try db.executeQuery("PRAGMA busy_timeout = 10000", values: nil).close()

                let rs = try db.executeQuery("PRAGMA user_version", values: nil)
                if rs.next() { startingSchemaVersion = rs.int(forColumnIndex: 0) }
                rs.close()
            } catch {
                FileLog.shared.addMessage("Failed to setup database \(db.lastErrorCode()): \(db.lastErrorMessage()) actual error: \(error)")
            }
        }

        queue.inDatabase { db in
            do {
                var newSchemaVersion = startingSchemaVersion
                upgradeIfRequired(schemaVersion: &newSchemaVersion, db: db)

                if newSchemaVersion != startingSchemaVersion {
                    try db.executeUpdate("PRAGMA user_version = \(newSchemaVersion)", values: nil)
                }
            } catch {
                FileLog.shared.addMessage("Failed to setup database \(db.lastErrorCode()): \(db.lastErrorMessage()) actual error: \(error)")
            }
        }

Review comment (bug):
This call was split in two in one of the GRDB PRs as [explained here](https://github.com/Automattic/pocket-casts-ios/pull/2820/files#r1985476408).

This method is back to being a single block, which is what it should be. I had to add a method so that the crash that was happening before doesn't happen anymore.

With this change we're back at how it was before + working correctly with GRDB.

Code after:
import FMDB
import Foundation
import PocketCastsUtils

class DatabaseHelper {
    class func setup(queue: PCDBQueue) {
        queue.write { db in
            do {
                try db.executeQuery("PRAGMA busy_timeout = 10000", values: nil).close()

                let startingSchemaVersion = db.pragmaUserVersion() ?? 0

                var newSchemaVersion = startingSchemaVersion
                upgradeIfRequired(schemaVersion: &newSchemaVersion, db: db)

                if newSchemaVersion != startingSchemaVersion {
                    try db.executeUpdate("PRAGMA user_version = \(newSchemaVersion)", values: nil)
                }
            } catch {
                FileLog.shared.addMessage("Failed to setup database \(db.lastErrorCode()): \(db.lastErrorMessage()) actual error: \(error)")
            }
        }
    }

    private class func upgradeIfRequired(schemaVersion: inout Int32, db: PCDatabase) {
        db.beginTransaction()

        let failedAt = { (statement: Int) in
            let lastErrorCode = db.lastErrorCode()
            let lastErrorMessage = db.lastErrorMessage()
            db.rollback()
            FileLog.shared.addMessage("Schema update \(statement) failed, code \(lastErrorCode): \(lastErrorMessage)")
Repository: Automattic/pocket-casts-ios
File: podcasts/Analytics/Adapters/Crash Logging/CrashLoggingAdapter.swift (Swift)

Code before:
import Foundation
import AutomatticRemoteLogging

class CrashLoggingAdapter: AnalyticsAdapter {
    let crashLogging: CrashLogging?

    init() {
        self.crashLogging = try? CrashLogging(dataProvider: CrashLoggingDataProvider()).start()
    }

    func track(name: String, properties: [AnyHashable: Any]?) { }
}


Review comment (refactor):
Not proud of that.

However, I don't want to refactor this as it would be it's own task and comes with another complexities.

Since this is mean to be temporary I believe it is a good trade-off for the time being.

Code after:
import Foundation
import AutomatticRemoteLogging

class CrashLoggingAdapter: AnalyticsAdapter {
    let crashLogging: CrashLogging?

    static var sharedManager: CrashLoggingAdapter?

    init() {
        self.crashLogging = try? CrashLogging(dataProvider: CrashLoggingDataProvider()).start()
        Self.sharedManager = self
    }

    func track(name: String, properties: [AnyHashable: Any]?) { }
}
Repository: Automattic/pocket-casts-ios
File: podcasts/PCBundleDoc.swift (Swift)

Code before:
import PocketCastsDataModel
import SwiftUI
import PocketCastsUtils

extension UTType {
    static var pcasts = UTType(filenameExtension: "pcasts", conformingTo: .package)!
}

struct PCBundleDoc: FileDocument {
    static var readableContentTypes = [UTType.pcasts]

    enum Constants {
        static let databaseFilename = "database.sqlite3"
        static let preferencesFilename = "preferences.plist"
    }

    init() {}

    static func delete() {
        try! FileManager.default.removeItem(at: FileManager.databaseURL)
        try! FileManager.default.removeItem(at: FileManager.preferencesURL!)

        UserDefaults.resetStandardUserDefaults()
        exit(0)
    }

    static func performImport(from wrapper: FileWrapper) throws {
        DataManager.sharedManager.close()

        // Import database
        let databaseDestination = FileManager.databaseURL
        if let database = wrapper.fileWrappers?.first(where: { $0.key == Constants.databaseFilename }) {
            try database.value.write(to: databaseDestination, originalContentsURL: nil)
        }

        // Import Preferences
        if let preferences = wrapper.fileWrappers?.first(where: { $0.key == Constants.preferencesFilename }) {
            let plistToCopy = FileManager.default.temporaryDirectory.appendingPathComponent("PocketCastsImportedPreferences", conformingTo: .propertyList)

Review comment (suggestion):
@bjtitus I think there are two things we need to take into account with GRDB: the `shm` and the `wal` files. Eg.:

<img width="174" height="69" alt="Image" src="https://github.com/user-attachments/assets/e7c8f302-587a-4c2a-8411-75288ea02a07" />

Code after:
import PocketCastsDataModel
import SwiftUI
import PocketCastsUtils

extension UTType {
    static var pcasts = UTType(filenameExtension: "pcasts", conformingTo: .package)!
}

struct PCBundleDoc: FileDocument {
    static var readableContentTypes = [UTType.pcasts]

    enum Constants {
        static let databaseFilename = "database.sqlite3"
        static var databaseWalSuffix = "-wal"
        static let preferencesFilename = "preferences.plist"
    }

    init() {}

    static func delete() {
        try! FileManager.default.removeItem(at: FileManager.databaseURL)
        try! FileManager.default.removeItem(at: FileManager.preferencesURL!)

        UserDefaults.resetStandardUserDefaults()
        exit(0)
    }

    static func performImport(from wrapper: FileWrapper) throws {
        // Import database
        let databaseDestination = FileManager.databaseURL
        if let database = wrapper.fileWrappers?.first(where: { $0.key == Constants.databaseFilename }) {
            try database.value.write(to: databaseDestination, originalContentsURL: nil)
        }

        let databaseWALDestination = URL(fileURLWithPath: DataManager.pathToDb().appending("-wal"))
        if let database = wrapper.fileWrappers?.first(where: { $0.key == Constants.databaseFilename.appending("-wal") }) {
            try database.value.write(to: databaseWALDestination, originalContentsURL: nil)
        }
Repository: Automattic/pocket-casts-ios
File: podcasts/PlaylistSelectionViewController.swift (Swift)

Code before:
import PocketCastsDataModel
import PocketCastsUtils
import UIKit

class PlaylistSelectionViewController: PCViewController, UITableViewDelegate, UITableViewDataSource {
    private static let filterAutoDownloadCell = "FilterDownloadCell"

    var allPlaylists = [EpisodeFilter]()
    var selectedPlaylists = [String]()
    var playlistSelected: ((EpisodeFilter) -> Void)?
    var playlistUnselected: ((EpisodeFilter) -> Void)?

    private var didChange = false
    var didChangePlaylist: (() -> Void)?

    var navigationTitle: String = L10n.settingsSelectFiltersPlural {
        didSet {
            title = navigationTitle
        }
    }

    @IBOutlet var playlistSelectionTable: UITableView! {
        didSet {
            if FeatureFlag.playlistsRebranding.enabled {
                playlistSelectionTable.register(PlaylistCell.self, forCellReuseIdentifier: PlaylistCell.reuseIdentifier)
            } else {
                playlistSelectionTable.register(UINib(nibName: "FilterDownloadCell", bundle: nil), forCellReuseIdentifier: PlaylistSelectionViewController.filterAutoDownloadCell)
            }
        }
    }


Review comment (suggestion):
Should this be renamed to `playlistAutoDownloadCell` or just `autoDownloadCell`?

```suggestion
    private static let playlistAutoDownloadCell = "PlaylistDownloadCell"
```

Code after:
import PocketCastsDataModel
import PocketCastsUtils
import UIKit
import SwiftUI

class PlaylistSelectionViewController: PCViewController, UITableViewDelegate, UITableViewDataSource {
    private static let playlistAutoDownloadCell = "PlaylistAutoDownloadCell"

    var allPlaylists = [EpisodeFilter]()
    var selectedPlaylists = [String]()
    var playlistSelected: ((EpisodeFilter) -> Void)?
    var playlistUnselected: ((EpisodeFilter) -> Void)?

    private var didChange = false
    var didChangePlaylist: (() -> Void)?

    var navigationTitle: String = L10n.settingsSelectFiltersPlural {
        didSet {
            title = navigationTitle
        }
    }

    @IBOutlet var playlistSelectionTable: UITableView! {
        didSet {
            if FeatureFlag.playlistsRebranding.enabled {
                playlistSelectionTable.register(PlaylistCell.self, forCellReuseIdentifier: PlaylistCell.reuseIdentifier)
            } else {
                playlistSelectionTable.register(UINib(nibName: "FilterDownloadCell", bundle: nil), forCellReuseIdentifier: PlaylistSelectionViewController.playlistAutoDownloadCell)
            }
        }
    }
Repository: Automattic/pocket-casts-ios
File: podcasts/MediaFileHandle.swift (Swift)

Code before:
        close()
    }
}

// MARK: Internal methods

extension MediaFileHandle {
    var attributes: [FileAttributeKey: Any]? {
        do {
            return try FileManager.default.attributesOfItem(atPath: filePath)
        } catch let error as NSError {
            FileLog.shared.addMessage("MediaFileHandle: FileAttribute error: \(error)")
        }
        return nil
    }

    var fileSize: Int {
        return attributes?[.size] as? Int ?? 0
    }

    func readData(withOffset offset: Int, forLength length: Int) -> Data? {
        lock.lock()
        defer { lock.unlock() }

        do {
            try readHandle?.seek(toOffset: UInt64(offset))
        } catch {
            FileLog.shared.addMessage("MediaFileHandle: File seek error: \(error)")
            return nil
        }
        return readHandle?.readData(ofLength: length)
    }

    func append(data: Data) throws {
        lock.lock()
        defer { lock.unlock() }

        guard let writeHandle = writeHandle else { return }

        try writeHandle.seekToEnd()

        try writeHandle.write(contentsOf: data)
    }

    func synchronize() {
        lock.lock()
        defer { lock.unlock() }

        guard let writeHandle = writeHandle else { return }

        try? writeHandle.synchronize()

Review comment (suggestion):
I wonder if we should catch the `nil` `readHandle` early and log it just to avoid confusion?

```swift
guard let readHandle = readHandle else {
    FileLog.shared.addMessage("MediaFileHandle: nil readHandle for path \(filePath)")
    return nil
}
```

Code after:
        close()
    }
}

// MARK: Internal methods

extension MediaFileHandle {
    var attributes: [FileAttributeKey: Any]? {
        do {
            return try FileManager.default.attributesOfItem(atPath: filePath)
        } catch let error as NSError {
            FileLog.shared.addMessage("MediaFileHandle: File [\(filePath)] attribute error: \(error)")
        }
        return nil
    }

    var fileSize: Int {
        return attributes?[.size] as? Int ?? 0
    }

    func readData(withOffset offset: Int, forLength length: Int) -> Data? {
        lock.lock()
        defer { lock.unlock() }

        guard let readHandle else {
            FileLog.shared.addMessage("MediaFileHandle: File [\(filePath)] read handle is nil")
            return nil
        }

        do {
            try readHandle.seek(toOffset: UInt64(offset))
        } catch {
            FileLog.shared.addMessage("MediaFileHandle: File [\(filePath)] seek error: \(error)")
            return nil
        }

        do {
            return try readHandle.read(upToCount: length)
        } catch {
            FileLog.shared.addMessage("MediaFileHandle: File [\(filePath)] read error: \(error)")
            return nil
        }
    }

    func append(data: Data) throws {
        lock.lock()
        defer { lock.unlock() }

        guard let writeHandle = writeHandle else { return }

        try writeHandle.seekToEnd()
Repository: Automattic/pocket-casts-ios
File: podcasts/EpisodeCell.swift (Swift)

Code before:
        updateSizeConstraints(of: episodeImage, to: imageSize)

        let buttonSize = max(44, metric.scaledValue(for: 44))
        updateSizeConstraints(of: actionButton, to: buttonSize)
        actionButton.enlargementScale = buttonSize / 44
        updateSizeConstraints(of: selectView, to: buttonSize)

        let iconSize = max(16, metric.scaledValue(for: 16))
        updateSizeConstraints(of: statusIndicator, to: iconSize)
        updateSizeConstraints(of: uploadStatusIndicator, to: iconSize)
        updateSizeConstraints(of: upNextIndicator, to: iconSize)
        updateSizeConstraints(of: bookmarkIcon, to: iconSize)
        updateSizeConstraints(of: starIndicator, to: iconSize)
        updateSizeConstraints(of: downloadingIndicator, to: iconSize)

        let tickSize = max(24, metric.scaledValue(for: 24))
        updateSizeConstraints(of: selectTickImageView, to: tickSize)
        updateSizeConstraints(of: selectCircleView, to: tickSize)
        selectTickImageView.layer.cornerRadius = tickSize / 2
        selectCircleView.layer.cornerRadius = tickSize / 2
    }

    override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
        super.traitCollectionDidChange(previousTraitCollection)
        updateSize()
    }
}


Review comment (suggestion):
Does it make sense to `guard` this for changes to `contentSizeCategory`?

```swift
guard traitCollection.preferredContentSizeCategory != previousTraitCollection?.preferredContentSizeCategory else { return }
```

Code after:
        updateSizeConstraints(of: episodeImage, to: imageSize)

        let buttonSize = max(44, metric.scaledValue(for: 44))
        updateSizeConstraints(of: actionButton, to: buttonSize)
        actionButton.enlargementScale = buttonSize / 44
        updateSizeConstraints(of: selectView, to: buttonSize)

        let iconSize = max(16, metric.scaledValue(for: 16))
        updateSizeConstraints(of: statusIndicator, to: iconSize)
        updateSizeConstraints(of: uploadStatusIndicator, to: iconSize)
        updateSizeConstraints(of: upNextIndicator, to: iconSize)
        updateSizeConstraints(of: bookmarkIcon, to: iconSize)
        updateSizeConstraints(of: starIndicator, to: iconSize)
        updateSizeConstraints(of: downloadingIndicator, to: iconSize)

        let tickSize = max(24, metric.scaledValue(for: 24))
        updateSizeConstraints(of: selectTickImageView, to: tickSize)
        updateSizeConstraints(of: selectCircleView, to: tickSize)
        selectTickImageView.layer.cornerRadius = tickSize / 2
        selectCircleView.layer.cornerRadius = tickSize / 2
    }

    override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
        super.traitCollectionDidChange(previousTraitCollection)
        guard traitCollection.preferredContentSizeCategory != previousTraitCollection?.preferredContentSizeCategory else { return }
        updateSize()
    }
}