Skip to content

Conversation

@mgolovko
Copy link
Collaborator

No description provided.

@mgolovko mgolovko requested a review from a team as a code owner October 30, 2025 14:24
@claude
Copy link
Contributor

claude bot commented Oct 30, 2025

Bugs/Issues

SpaceHubViewModel.swift:217 - updateFilteredSpaces() async/await pattern
The async function is called with await in line 136 (await updateFilteredSpaces()) and line 127 (await updateFilteredSpaces()), blocking the main actor unnecessarily. The spaceCardModelBuilder.build() already runs in Task.detached, but awaiting it in an async context creates an unnecessary async boundary.

Recommendation: Make updateFilteredSpaces() non-async and wrap the async work in an unstructured Task:

private func updateFilteredSpaces() {
    Task {
        guard let spaces else {
            filteredSpaces = []
            return
        }
        
        let spacesToFilter: [ParticipantSpaceViewDataWithPreview]
        if searchText.isEmpty {
            spacesToFilter = spaces
        } else {
            spacesToFilter = spaces.filter { space in
                space.spaceView.name.localizedCaseInsensitiveContains(searchText)
            }
        }
        
        self.filteredSpaces = await spaceCardModelBuilder.build(from: spacesToFilter, wallpapers: wallpapers)
    }
}

Then call it without await: updateFilteredSpaces() (lines 127, 136).


SpaceHubDropDelegate.swift:20 - Incorrect property access after refactoring
Line 20 uses allSpaces.firstIndex(where: { $0.spaceView.id == destinationSpaceViewId }) but line 28 uses allSpaces.firstIndex(where: { $0.space.spaceView.id == draggedSpaceViewId }) with .space.spaceView instead of just .spaceView.

Line 29 has the same issue: allSpaces.firstIndex(where: { $0.space.spaceView.id == destinationSpaceViewId })

This will cause a compilation error - ParticipantSpaceViewDataWithPreview doesn't have a .space property that contains another .spaceView. The correct path is just .spaceView.id.

Fix: Change lines 28-29 to:

guard let fromIndex = allSpaces.firstIndex(where: { $0.spaceView.id == draggedSpaceViewId } ) else { return }
guard let toIndex = allSpaces.firstIndex(where: { $0.spaceView.id == destinationSpaceViewId } ) else { return }

Best Practices

View+DragDrop.swift:46, 66, 85 - Redundant matchedGeometryEffect
The conditional drag/drop modifiers apply matchedGeometryEffect(id: "content", ...) in both branches (when condition is true and false). This is redundant - the geometry effect serves no purpose here since the content doesn't change between states, and using the same ID "content" in multiple independent views could cause conflicts.

Recommendation: Remove matchedGeometryEffect from these modifiers entirely, or explain why it's needed for the animation.


SpaceHubList.swift:82 - Unnecessary .id(cardModel.id) modifier
The .id() modifier is redundant since ForEach already uses cardModel as the identity (it conforms to Identifiable). Adding an explicit .id() on the child view doesn't provide additional diffing benefits and may cause unnecessary view recreations.


SpaceCard.swift:15-16 - Feature flags stored in @State
Feature flags are stored in @State properties that are initialized once and never updated. If feature flags change at runtime, these views won't reflect the changes. Consider reading flags directly or using a reactive pattern.


🚨 Major Issues - Compilation error in SpaceHubDropDelegate.swift lines 28-29, and potential performance issue with async/await pattern

@mgolovko mgolovko merged commit 5a450f1 into develop Oct 30, 2025
7 checks passed
@mgolovko mgolovko deleted the ios-5387-space-hub-coordinator-a-lot-if-sheet-methods-make-body-very branch October 30, 2025 16:23
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants