Skip to content

UX improvements and final touches#13

Merged
ripventura merged 9 commits intomainfrom
feat/ux-polishing
Feb 20, 2026
Merged

UX improvements and final touches#13
ripventura merged 9 commits intomainfrom
feat/ux-polishing

Conversation

@ripventura
Copy link
Copy Markdown
Owner

The goal of this PR is to bring final touches to the MVP, with a major focus on user experience.

Changes

  • Added custom lightning when viewing the asset on immersive mode for better UX
  • Improved the control of view and view models lifecycle by leveraging the Equatable SPM library
  • Improved the handling of volumetric and immersive windows when transitioning between experiences
  • Improved debug logging throughout the app

Added custom lightning to the asset for better UX
- “@Equatable” is now being use to affirm a view identity
- Asset model is now properly injected instead of hardcoded
- Other UX improvements
@ripventura ripventura self-assigned this Feb 20, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Feb 20, 2026

Code Review

Three issues found:

1. Overlapping ornaments at the same anchor — AssetDetailView.swift

Two .ornament(attachmentAnchor: .scene(.top)) modifiers are applied to the same view (lines 46–59). The first (pre-existing) shows Text(asset.title) when hasLoadedModel == true; the PR adds a second at the same anchor showing the loading indicator when isLoadingExperience == true. These conditions are not mutually exclusive — when the model has loaded but the immersive space is still opening (phase == .opening), both ornaments render at the same .top anchor, overlapping each other. Merge them into a single .ornament(attachmentAnchor: .scene(.top)) with internal conditional logic.

2. Broken test — AssetDisplayViewModel.swift

The PR removed logger.warning("startLoading() called while already loading — ignoring") from startLoading() (replaced with a silent guard !isLoading else { return } at line 56), but the existing test duplicateStartLoadingIsIgnored in AssetDisplayViewModelTests.swift asserts that exactly one warning-level entry containing "already loading" is logged. This test will always fail.

3. AsyncThrowingStream continuation never finished on deallocation — AssetDownloadOperator.swift

The PR introduces [weak self] on the Task inside AsyncThrowingStream (lines 75–76). If self is deallocated before the task body runs, guard let self else { return } exits without ever calling continuation.finish() or continuation.finish(throwing:). Any caller for try await-ing over the stream will hang indefinitely. Either capture self strongly (the prior behavior), or add continuation.finish(throwing: CancellationError()) in the guard failure path.

@ripventura ripventura merged commit dc8009f into main Feb 20, 2026
1 check passed
@ripventura ripventura deleted the feat/ux-polishing branch February 20, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant