Fix Runtime warning on pushing multiple items into NavigationStackController (TCA only)

[loongallday]

as per
https://pointfreecommunity.slack.com/archives/C04KQQ7NXHV/p1730815911553849

[mbrandonw]

Hi @loongallday, thanks for taking the time to look into this, however your changes do break the test suite:

2024-11-14T20:31:17.8608910Z Failing tests:
2024-11-14T20:31:17.8610470Z 	NavigationPathTests.testAppend_UnrecognizedType()
2024-11-14T20:31:17.8611740Z 	NavigationPathTests.testDeepLink_UnrecognizedType()
2024-11-14T20:31:17.8613520Z 	NavigationPathTests.testDeepLink_UnrecognizedType()
2024-11-14T20:31:17.8632940Z 	NavigationPathTests.testPush_UnrecognizedType()
2024-11-14T20:31:17.8639810Z 
2024-11-14T20:31:17.8639970Z ** TEST FAILED **

You can run the tests in the Examples project to see these failures for yourself. Would you be interested in looking into a fix?

Without looking into the issue very much I can definitely say that your change must be tweaked a bit. With the way the logic is now nothing is ever appending indices to invalidIndices. It is just always an empty index set. Even your if !invalidIndices.isEmpty check isn't doing anything. So this needs a bit more work.

[loongallday]

Thanks, @mbrandonw I'll look into it but from basic inspecting I accidentally change the min iOS version to 15 since I wanna test with both version but forgot to change it back, will try that and run the test locally again :)

[loongallday]

silly me, I put the the isEmtpy check in the wrong position facepalm

[mbrandonw]

Thanks @loongallday! This is the exact fix we planned on making when the issue was originally brought up, but we lost track of it. Will merge and release soon.

[mbrandonw]

@loongallday One last question... would you happen to know how to write a test that triggers the runtime warning without this change? It would be nice to get some test coverage on this behavior.

[loongallday]

@mbrandonw thanks for approval Brandon !

but to reproduce the warning might be impossible if we don't import NavigationStack+Observation from swift-composable-architecture (the warning is in there)

possible test might be to create scenario to execute this else block (push > 1 vc) right here in NavigationStackController and observe the value produced by _UIBindingAppendKeyPath.wrappedValue (keyPath: PathView.path) setter if it's the same twice
(the setter is invoked by UIBinding setting location.wrappedValue)

but how to write it, im not really sure about that since it's not really related to the NavigationStackController but more about the swift's underlying syntax, and how _UIBindingAppendKeyPath works.

Here's the code block i've mentioned (in NavigationStackController)

} else {
          var newPath = newPath
          let oldViewControllers =
            viewControllers.isEmpty
            ? root.map { [$0] } ?? []
            : viewControllers
          var newViewControllers: [UIViewController] = []
          newViewControllers.reserveCapacity(max(viewControllers.count, newPath.count))

          loop: for viewController in oldViewControllers {
            if let navigationID = viewController.navigationID {
              guard navigationID == newPath.first
              else {
                break loop
              }
              newViewControllers.append(viewController)
              newPath.removeFirst()
            } else {
              newViewControllers.append(viewController)
            }
          }
          var invalidIndices = IndexSet()
          var didPushNewViewController = false
          for (index, navigationID) in newPath.enumerated() {
            if let viewController =
              viewControllers
              .first(where: { $0.navigationID == navigationID })
            {
              newViewControllers.append(viewController)
            } else if let viewController = viewController(for: navigationID) {
              newViewControllers.append(viewController)
              didPushNewViewController = true
            } else if case .lazy(.element) = navigationID,
              !didPushNewViewController,
              let elementType = navigationID.elementType
            {
              reportIssue(
                """
                No "navigationDestination(for: \(String(customDumping: elementType))) { … }" was \
                found among the view controllers on the path.
                """
              )
              invalidIndices.insert(index)
            }
          }
          path.remove(atOffsets: invalidIndices)
          setViewControllers(newViewControllers, animated: !transaction.uiKit.disablesAnimations)
        }

Here's the NavigationStack+Observation from swift-composable-architecture

extension Store {
  @_spi(Internals)
  public subscript<ElementState, ElementAction>(
    fileID fileID: _HashableStaticString,
    filePath filePath: _HashableStaticString,
    line line: UInt,
    column column: UInt
  ) -> StackState<ElementState>.PathView
  where State == StackState<ElementState>, Action == StackAction<ElementState, ElementAction> {
    get { self.currentState.path }
    set {
      let newCount = newValue.count
      guard newCount != self.currentState.count else {
        reportIssue(
          """
          A navigation stack binding at "\(fileID.rawValue):\(line)" was written to with a \
          path that has the same number of elements that already exist in the store. A view \
          should only write to this binding with a path that has pushed a new element onto the \
          stack, or popped one or more elements from the stack.

          This usually means the "forEach" has not been integrated with the reducer powering the \
          store, and this reducer is responsible for handling stack actions.

          To fix this, ensure that "forEach" is invoked from the reducer's "body":

              Reduce { state, action in
                // ...
              }
              .forEach(\\.path, action: \\.path) {
                Path()
              }

          And ensure that every parent reducer is integrated into the root reducer that powers \
          the store.
          """,
          fileID: fileID.rawValue,
          filePath: filePath.rawValue,
          line: line,
          column: column
        )
        return
      }
      if newCount > self.currentState.count, let component = newValue.last {
        self.send(.push(id: component.id, state: component.element))
      } else {
        self.send(.popFrom(id: self.currentState.ids[newCount]))
      }
    }
  }
}

which is called from the setter block of _UIBindingAppendingPath.wrappedValue twice in this repository

private final class _UIBindingAppendKeyPath<Base: _UIBinding, Value>: _UIBinding, Sendable {
  let base: Base
  let keyPath: _SendableWritableKeyPath<Base.Value, Value>
  init(base: Base, keyPath: _SendableWritableKeyPath<Base.Value, Value>) {
    self.base = base
    self.keyPath = keyPath
  }
  var wrappedValue: Value {
    get { base.wrappedValue[keyPath: keyPath] }
    set {
      base.wrappedValue[keyPath: keyPath] = newValue
    }
  }
  static func == (lhs: _UIBindingAppendKeyPath, rhs: _UIBindingAppendKeyPath) -> Bool {
    lhs.base == rhs.base && lhs.keyPath == rhs.keyPath
  }
  func hash(into hasher: inout Hasher) {
    hasher.combine(base)
    hasher.combine(keyPath)
  }
}

as i've tracked this exact line is executed when pushing multiple viewController it enters the else block which i modified the code to fix the runtime warning

and the culprit was the exact line i adjust

path.remove(atOffsets: invalidIndices)

this line triggers the binding's setter twice when executing it (Please correct me if im wrong here), resulting in a run time warning in NavigationStack+Observation above

You can try to reproduce it to get a better idea from the minimal project i attached here

tca-navigation-warning.zip