Skip to content

Conversation

@ignatovv
Copy link
Member

@ignatovv ignatovv commented Nov 3, 2025

Summary

  • Implemented space-aware filtering to hide chat type/chat widget in Chat spaces across the entire app
  • Consolidated chat layout visibility logic into a single showsChatLayouts property on SpaceUxType
  • Refactored code to use optional SpaceUxType parameters with internal fallback handling

Changes

Core Infrastructure

  • SpaceUxType+Extensions: Added showsChatLayouts property (returns false for .chat, true for others)
  • DetailsLayoutExtension: Updated all space-aware methods to accept optional SpaceUxType? with .data fallback
    • visibleLayouts(spaceUxType:)
    • visibleLayoutsWithFiles(spaceUxType:)
    • supportedForCreation(spaceUxType:)
    • widgetTypeLayouts(spaceUxType:)
  • SearchHelper+Extension: Updated defaultObjectTypeSort to accept SpaceUxType? instead of isChat boolean

Architecture Improvements

  • SetSubscriptionData: Removed Container.shared access, now receives spaceUxType as parameter
  • Dependency Injection: Added spaceViewsStorage injection to ViewModels that needed it:
    • EditorSetViewModel
    • SetObjectWidgetInternalViewModel
    • ObjectTypeRowsBuilder
    • HomeBottomNavigationPanelViewModel

Updated Components (21 files)

All search, mention, widget, and type selection components now properly filter chat layouts in chat spaces:

  • Global search and object search
  • Mentions and link lists
  • Home widgets and set widgets
  • Object type selection
  • Recent subscriptions
  • Date-related object subscriptions

Benefits

  • Single source of truth for chat layout visibility
  • Cleaner API with semantic property names
  • No more scattered ?.isChat ?? false patterns
  • Proper dependency injection without hidden Container access
  • Consistent fallback behavior across the codebase

- Add showsChatLayouts property to SpaceUxType (true for non-chat spaces)
- Update DetailsLayout methods to accept optional SpaceUxType and filter chat layouts
- Update SearchHelper.defaultObjectTypeSort to use SpaceUxType parameter
- Remove Container.shared access from SetSubscriptionData struct
- Inject spaceViewsStorage in all ViewModels and pass spaceUxType as parameter
- Replace isChat boolean checks with showsChatLayouts property across codebase
@ignatovv ignatovv requested a review from a team as a code owner November 3, 2025 12:49
@claude
Copy link
Contributor

claude bot commented Nov 3, 2025

Code Review - PR #4198

Architecture & Design ✅

Strengths:

  • Excellent refactoring that consolidates chat layout filtering logic into a single source of truth (SpaceUxType.showsChatLayouts)
  • Clean API design with space-aware static methods on DetailsLayout that accept optional SpaceUxType? parameters
  • Proper dependency injection improvements - removed Container.shared access from SetSubscriptionData and added proper @Injected properties
  • Consistent fallback pattern (spaceUxType ?? .data) ensures safe nil handling throughout

Implementation Quality:

  • All 21 files consistently apply the new filtering pattern
  • SearchHelper+Extension.swift uses proper logic inversion: !showsChatLayouts excludes chat from custom sort order
  • SpaceUxType+Extensions.swift correctly returns false for showsChatLayouts in chat/stream/none spaces, true only for data spaces

Potential Issues

1. Stale SpaceUxType in Long-Running ViewModels ⚠️

Location: Multiple ViewModels load spaceUxType once during initialization:

  • GlobalSearchViewModel.swift:41 - loadSpaceUxType() called only in init
  • ObjectTypeSearchViewModel.swift:75 - Loaded only at search task start
  • MentionsViewModel.swift:56 - Loaded once per search

Issue: If a user switches a space from datachat type while these ViewModels are active, they'll continue using the old cached value and show incorrect chat layout visibility.

Impact: Medium - In practice, space type changes are rare and may require app restart, but this creates subtle state inconsistency.

Recommendation: Consider either:

  1. Subscribing to spaceViewsStorage.spaceViewPublisher(spaceId:) to reactively update when space type changes
  2. Document this limitation if space type changes require app restart
  3. Reload spaceUxType before each operation instead of caching

2. SearchService Container Access Pattern 🔍

Location: SearchService.swift:13

private let spaceViewsStorage: any SpaceViewsStorageProtocol = Container.shared.spaceViewsStorage()

Issue: While you properly removed Container.shared from SetSubscriptionData, SearchService still uses direct container access. This is inconsistent with the PR's stated goal of "proper dependency injection without hidden Container access".

Impact: Low - SearchService is a service-layer class where container access is more acceptable, but it's worth noting the inconsistency.

3. nil Coalescing Logic Inconsistency 🤔

Locations:

  • DetailsLayoutExtension.swift:32-34 - All methods use spaceUxType ?? .data, then check !spaceUxType.showsChatLayouts
  • SearchHelper+Extension.swift:8 - Uses spaceUxType?.showsChatLayouts ?? true

Observation: Two different patterns for handling nil:

  1. Pattern A: Coalesce to .data, then check property → Always returns true for chat layouts when nil (since .data.showsChatLayouts == true)
  2. Pattern B: Optional chain with ?? true → Same result, but more explicit

Verdict: Both are semantically equivalent and intentionally default to showing chat layouts. However, the inconsistency could confuse future maintainers.

Recommendation: Document the intentional nil → show chat layouts behavior in a code comment, or standardize on one pattern.

4. ObjectSearchWithMetaModuleData.supportedLayouts ⚠️

Location: ObjectSearchWithMetaViewModel.swift:115-116

static let supportedLayouts: [DetailsLayout] =
    ObjectTypeSection.pages.supportedLayouts(spaceUxType: nil) +
    ObjectTypeSection.lists.supportedLayouts(spaceUxType: nil)

Issue: This static property always passes nil, meaning it will always include chat layouts regardless of actual space type. This is used as a constant, so it can't be space-aware.

Impact: Unclear without seeing usage context - if this is used for validation/filtering elsewhere, it may allow chat layouts in chat spaces when they should be hidden.

Recommendation: Investigate if this static property is still appropriate, or if it should be computed dynamically per-space.

Code Quality ✅

  • No hardcoded strings in UI (not applicable for this PR)
  • No generated files edited
  • Proper Swift style and conventions
  • Clean separation of concerns

Testing Considerations 🧪

Recommended manual tests:

  1. Verify chat type/widget hidden in chat spaces across all 21 updated components
  2. Test space type changes don't cause layout visibility bugs (if supported)
  3. Verify data spaces still show chat layouts correctly
  4. Test nil space handling (edge case where spaceView(spaceId:) returns nil)

Summary

This is a well-executed refactoring that achieves its goals of consolidating chat layout filtering logic and improving dependency injection. The implementation is consistent and follows good architectural patterns.

Main concerns:

  • Stale spaceUxType caching in long-running ViewModels (if space type changes are supported at runtime)
  • ObjectSearchWithMetaModuleData.supportedLayouts static property may not respect space filtering

Recommendation: Approve with consideration for the stale state issue if space type changes are supported without restart.

Code Quality: Excellent
⚠️ Minor Issues: 2 - Stale caching, static property edge case

@ignatovv ignatovv merged commit 56f8038 into develop Nov 3, 2025
7 checks passed
@ignatovv ignatovv deleted the ios-5425-hide-chat-type-chat-widget-in-chat-spaces-from-every-wehere branch November 3, 2025 15:21
@github-actions github-actions bot locked and limited conversation to collaborators Nov 3, 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.

2 participants