Skip to content

feat(iOS, Stack v5): Align Stack implementation with RFC 753#3774

Open
kmichalikk wants to merge 18 commits intomainfrom
@kmichalikk/stack-v5-with-queue-ios
Open

feat(iOS, Stack v5): Align Stack implementation with RFC 753#3774
kmichalikk wants to merge 18 commits intomainfrom
@kmichalikk/stack-v5-with-queue-ios

Conversation

@kmichalikk
Copy link
Copy Markdown
Contributor

@kmichalikk kmichalikk commented Mar 18, 2026

Closes https://github.com/software-mansion/react-native-screens-labs/issues/1050

Description

This PR aligns the current Stack implementation with RFC 753 by adding an operations queue that enables us to defer the application of JS differences in a batch that moves the stack from one consistent state to another. The implementation has been adapted from Android implementation, with key difference being the absence of FragmentManager along with its FragmentOperations. These are handled within RNSStackNavigationController.

Changes

  • Introduced pendingPopOperations and pendingPushOperations that form a queue and need not be applied immediately.
  • Replaced swift controllers with objective-c to remove the need of interoperability: given the amount of props, enums, customizations that will be present in the future, I think it's better to avoid the interop entirely

Before & after - visual documentation

no visual changes

Test plan

Use Tests.Issue.TestScreenStack to test various scenarios, verify the state in the logs. Run Tests.Issue.Test3576 to check a specific scenario that caused problems on the android.

Checklist

  • Ensured that CI passes

@kmichalikk kmichalikk marked this pull request as draft March 18, 2026 12:35
@kmichalikk kmichalikk marked this pull request as ready for review March 18, 2026 12:55
@kmichalikk kmichalikk requested a review from Copilot March 18, 2026 12:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the iOS “gamma/stack” implementation to an Objective-C(/ObjC++) navigation-controller–based stack and introduces an operation queue/coordinator to batch apply push/pop changes in a consistent way (per RFC 753).

Changes:

  • Replaces Swift RNSStackController / RNSStackScreenController with Objective-C equivalents.
  • Adds RNSStackOperationCoordinator + operation objects to queue and order push/pop updates.
  • Updates RNSStackHostComponentView / RNSStackScreenComponentView to use the new coordinator and track native dismissals.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ios/gamma/stack/screen/RNSStackScreenController.swift Removes Swift screen controller implementation.
ios/gamma/stack/screen/RNSStackScreenController.h Adds ObjC header for the new screen controller.
ios/gamma/stack/screen/RNSStackScreenController.mm Adds ObjC implementation for lifecycle + dismiss event emission.
ios/gamma/stack/screen/RNSStackScreenComponentView.mm Initializes isNativelyDismissed and updates default props type for the stack screen component.
ios/gamma/stack/screen/RNSStackScreenComponentView.h Adds isNativelyDismissed property to support native-dismiss handling.
ios/gamma/stack/host/RNSStackOperationCoordinator.h Declares coordinator interface for pending operations.
ios/gamma/stack/host/RNSStackOperationCoordinator.mm Implements ordering/execution of queued push/pop operations.
ios/gamma/stack/host/RNSStackOperation.h Defines push/pop operation types and shared protocol.
ios/gamma/stack/host/RNSStackOperation.mm Implements push/pop operation initializers.
ios/gamma/stack/host/RNSStackNavigationController.h Declares navigation controller API for enqueuing operations and applying updates.
ios/gamma/stack/host/RNSStackNavigationController.mm Implements queued push/pop application and stack model logging.
ios/gamma/stack/host/RNSStackHostComponentView.mm Replaces old controller usage with navigation controller + coordinator and wires operation enqueueing to Fabric lifecycle.
ios/gamma/stack/host/RNSStackHostComponentView.h Removes old Swift controller exposure from the host view public interface.
ios/gamma/stack/host/RNSStackController.swift Removes Swift stack controller implementation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines -12 to -15
@property (nonatomic, nonnull, strong, readonly) RNSStackController *stackController;

- (nonnull NSMutableArray<RNSStackScreenComponentView *> *)reactSubviews;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These weren't used outside the implementation or anywhere

- (void)addPopOperationIfNeeded:(nonnull RNSStackScreenComponentView *)stackScreen
{
if (stackScreen.activityMode == RNSStackScreenActivityModeAttached && !stackScreen.isNativelyDismissed) {
// This shouldn't happen in typical scenarios but it can happen with fast-refresh.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment taken from Android, should still be relevant

Comment on lines +7 to +27
@protocol RNSStackOperation

@property (nonatomic, strong, readonly, nonnull) RNSStackScreenComponentView *stackScreen;

@end

@interface RNSPushOperation : NSObject <RNSStackOperation>

@property (nonatomic, strong, readonly, nonnull) RNSStackScreenComponentView *stackScreen;

- (instancetype)initWithScreen:(nonnull RNSStackScreenComponentView *)stackScreen;

@end

@interface RNSPopOperation : NSObject <RNSStackOperation>

@property (nonatomic, strong, readonly, nonnull) RNSStackScreenComponentView *stackScreen;

- (instancetype)initWithScreen:(nonnull RNSStackScreenComponentView *)stackScreen;

@end
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if there's a better way to do this comparing to sealed class implementation in kotlin

Comment on lines +71 to +86
NSMutableArray<NSDictionary *> *operationsWithIndices = [NSMutableArray array];
for (id<RNSStackOperation> operation in operations) {
NSInteger index = [stackScreens indexOfObject:operation.stackScreen];
[operationsWithIndices addObject:@{@"index" : @(index), @"operation" : operation}];
}

[operationsWithIndices sortUsingComparator:^NSComparisonResult(NSDictionary *obj1, NSDictionary *obj2) {
return [obj1[@"index"] compare:obj2[@"index"]];
}];

NSMutableArray<id<RNSStackOperation>> *orderedOperations = [NSMutableArray new];
for (NSDictionary *dict in operationsWithIndices) {
[orderedOperations addObject:dict[@"operation"]];
}

return orderedOperations;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In comparison to kotlin stream, this is 10x worse. Is there a better way to sort by indices from other array?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could try: https://developer.apple.com/documentation/foundation/nsarray/sortedarray(comparator:)?language=objc to avoid intermediate allocations in operationsWithIndices and compare indices inside comparator?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_reactEventEmitter = [RNSStackScreenComponentEventEmitter new];

_hasUpdatedActivityMode = NO;
self.isNativelyDismissed = NO;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be reset somewhere? This is set to YES when native dismiss is triggered

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we currently assume that screens won't be re-used so I don't think that we need to reset it for now.

@kmichalikk
Copy link
Copy Markdown
Contributor Author

There is another thing. For now, I didn't set up cancelTouches so if you try to pop a screen while touching pop action, it will result in app crashing.

- (void)stackScreenChangedActivityMode:(nonnull RNSStackScreenComponentView *)stackScreen
{
[_controller setNeedsUpdateOfChildViewControllers];
switch (stackScreen.activityMode) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add error handling?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Comment on lines 144 to 148
- (void)mountingTransactionWillMount:(const facebook::react::MountingTransaction &)transaction
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
{
_hasModifiedReactSubviewsInCurrentTransaction = false;
[_controller reactMountingTransactionWillMount];
// noop
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


NS_ASSUME_NONNULL_BEGIN

@protocol RNSStackOperation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might consider a class here, which will allow to move up both stackScreen and initWithScreen to the base

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, they're both equally meh. I need some base for both i.e. in common sort function. If I had base class here, I would have two derived classes that don't change anything. It's few lines either way. I think we could leave it be.


@property (nonatomic, strong, readonly, nonnull) RNSStackScreenComponentView *stackScreen;

- (instancetype)initWithScreen:(nonnull RNSStackScreenComponentView *)stackScreen;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonnull seems to be redundant, as we have NS_ASSUME_NONNULL_BEGIN

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


@implementation RNSPopOperation

- (instancetype)initWithScreen:(nonnull RNSStackScreenComponentView *)stackScreen
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is duplicated with RNSPushOperation, I'd consider creating a base class and moving the logic there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Comment on lines +71 to +86
NSMutableArray<NSDictionary *> *operationsWithIndices = [NSMutableArray array];
for (id<RNSStackOperation> operation in operations) {
NSInteger index = [stackScreens indexOfObject:operation.stackScreen];
[operationsWithIndices addObject:@{@"index" : @(index), @"operation" : operation}];
}

[operationsWithIndices sortUsingComparator:^NSComparisonResult(NSDictionary *obj1, NSDictionary *obj2) {
return [obj1[@"index"] compare:obj2[@"index"]];
}];

NSMutableArray<id<RNSStackOperation>> *orderedOperations = [NSMutableArray new];
for (NSDictionary *dict in operationsWithIndices) {
[orderedOperations addObject:dict[@"operation"]];
}

return orderedOperations;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could try: https://developer.apple.com/documentation/foundation/nsarray/sortedarray(comparator:)?language=objc to avoid intermediate allocations in operationsWithIndices and compare indices inside comparator?


- (void)didMoveToParentViewController:(UIViewController *)parent
{
NSLog(@"[RNScreens] Screen view with tag=%ld didMoveToParentViewController %@", (long)_screenView.tag, parent);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: RNSLog

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I left some minor suggestions.

_reactEventEmitter = [RNSStackScreenComponentEventEmitter new];

_hasUpdatedActivityMode = NO;
self.isNativelyDismissed = NO;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.isNativelyDismissed = NO;
_isNativelyDismissed = NO;

nit: I think that in the implementation we're usually using backing variable directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_reactEventEmitter = [RNSStackScreenComponentEventEmitter new];

_hasUpdatedActivityMode = NO;
self.isNativelyDismissed = NO;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we currently assume that screens won't be re-used so I don't think that we need to reset it for now.

NS_ASSUME_NONNULL_BEGIN

@class RNSStackScreenComponentView;
@class RNSStackController;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@class RNSStackController;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +19 to +22
- (instancetype)initWithCoder:(NSCoder *)aDecoder
{
return nil;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we really need this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RCTAssert(_controller != nil, @"[RNScreens] Controller must not be nil while attaching to window");

[self reactAddControllerToClosestParent:_controller];
RNSLog(@"[RNScreens] StackHost [%pv] attached to window", self);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RNScreens] StackHost [0x1033e7b80v] attached to window

I don't think that this is what we want, especially the random v at the end of the address. Let's use %@. Applies also to RNSStackNavigationController.mm (dumpStackModel).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +125 to +126
@"[RNScreens] ignoring pop operation of %s, already not attached or natively dismissed",
[stackScreen.screenKey cStringUsingEncoding:NSUTF8StringEncoding]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@"[RNScreens] ignoring pop operation of %s, already not attached or natively dismissed",
[stackScreen.screenKey cStringUsingEncoding:NSUTF8StringEncoding]);
@"[RNScreens] ignoring pop operation of %@, already not attached or natively dismissed",
stackScreen.screenKey);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applies to other places as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmichalikk kmichalikk force-pushed the @kmichalikk/stack-v5-with-queue-ios branch from 34cb54d to 425972c Compare April 8, 2026 11:03
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.

4 participants