useEffect loop crashes new demo BottomTabBar component

[ehenighan]

Hi,

By pure coincidence I was having similar problems relating to the lack of a sticky nav bar at the bottom and tried this recent new component out when my other idea didn't work (show-during-load = existing screen, hide-during-load = existing screen content box -> fails because the hide-during-load isn't applied during navigations, booo)

Immediately ran into a problem where the useEffect in the component gets into a loop, and I think it's because of the context provider:

hyperview/demo/src/Contexts/BottomTabBar.tsx

Line 27 in b5a6ada

setElementProps: (id: string, p: HvComponentProps) => {

<BottomTabBarContext.Provider
  value={{
    elementsProps,
    setElementProps: (id: string, p: HvComponentProps) => {
      //this de-structures the object and therefore always results in a new one.
      //I think this is what causes a render loop via the useEffect in the rendering component
      setElementsProps({ ...elementsProps, [id]: p });
    },
  }}
>

'Fixed' it with the following refactoring:

const setElementProps = useCallback((id: string, p: HvComponentProps) => {
    //only de-structure and replace the props on the element
    // if the component props for the specified ID is actually a new object
    if (!!p && (elementsProps[id] || {}) === p) {
        return;
    }
    setElementsProps({ ...elementsProps, [id]: p });
}, [elementsProps, setElementsProps]);
return (
    <BottomTabBarContext.Provider
        value={{
            elementsProps,
            setElementProps: setElementProps,
        }}
    >
        {props.children}
    </BottomTabBarContext.Provider>
);

I think the useCallback is probably overkill and it's the early-return condition in the function that's making the difference. Initially I thought this might be a niche case because I was using URL-loaded SVG icons and I considered if they might be causing lots of re-renders when the loading completed, but I took them out and the problem persisted.

I've not raised a PR for this directly because after my fix I'm also getting some other weird behaviour where after a successful initial load, the rest of the screen content just doesn't render when you select an item, leaving the nav bar floating in the centre of the page half-styled. The network request goes off in the background and the full screen content is returned to the app, but it doesn't load properly. If you click back and forth between tabs a bit, sometimes the destination page eventually loads once, but not again after that.

I've either got no rendering because of the useEffect crash, or weird UI behaviour with my fix which I've not yet been able to debug, so I figured I'd raise an issue with what I've found so far and see if anyone has advice before I go further with it.

[flochtililoch]

Thanks for raising the issue @ehenighan - coincidently I've added a fix a couple days ago to solve that regression, are you running the code with or without my fix? Let me know.

As we're still iterating on this ability to set a custom tab bar component, we might need to make a few more adjustments to this approach, so I'll consider your proposed fix soon.

On this topic:

when my other idea didn't work (show-during-load = existing screen, hide-during-load = existing screen content box -> fails because the hide-during-load isn't applied during navigations, booo)

This should work if you refer to screen element IDs in the show-during-load attribute. You'll need to define these screen elements as a siblings of your first visible tab's screen, each of which should render the tab bar in the state matching the target navigation tab. We're using this technique for our app and it's working ok, it does have usability issues thought.

[ehenighan]

Hi @flochtililoch,

Looks like I don't have your fix, which is a surprise because I thought I'd pulled the latest master today while trying out those components - will try again with your change instead of mine tomorrow and see if it resolves my other weird rendering issue too.

W.r.t the first approach I tried, what I specifically wanted to do actually didn't work for me because of other limitations - but they might be interesting to discuss separately because I think there's a possible simple change to the rules on show/hide-during-load that might solve it and also be quite powerful for some other tasks - if you don't mind, I'll create a separate discussion ticket to go into detail?

Thanks for the help and the quick response!

Ed

[flochtililoch]

will try again with your change instead of mine tomorrow and see if it resolves my other weird rendering issue too

keep me posted

if you don't mind, I'll create a separate discussion ticket to go into detail?

sure thing, please go ahead!

[ehenighan]

sure thing, please go ahead!

#957

[ehenighan]

Okay, the alternative fix didn't make any difference but I've made progress - I am using an SVG gradient background for the menu bar so initially had it in a parent view as a sibling of the select-single:

 - navigator:bottom-tab-bar
    - view
       - svg
       - select-single

This appears to break something about the props caching for state preservation.
Removed the parent view and pushed the SVG inside the select-single:

 - navigator:bottom-tab-bar
    - select-single
       - svg

Now the selection works as expected.

HOWEVER

There's now a flicker of a second nav bar during the first screen transition, and after that it appears permanently on the next screen you swap to but with the previous state selected - so the one at the bottom maintains the current selection but the copy above has the other tab.

I'll keep digging.

[ehenighan]

N.B I think it's a rendering issue on the first page - the one you're leaving.

Sequence of events:
Launch on Tab 1
Click Tab 2 menu option
Momentary flicker of second menu bar
Loading spinner shows, second menu bar disappears (i.e. it was part of the removed page)
Tab 2 loads
Click Tab 1 menu option
Navigation works
Second menu bar now permanently visible, with Tab 2 selected despite Tab 1 being visible and selected on the bottom (real) menu bar.

[ehenighan]

Okay, making some progress now - a lot of the complexity and need for more effects is driven by the context provider - it causes renders all the time because the function it returns is always a new object. Changing that by (I admit a bit dirtily) using useRef to let the function safely update the props without itself changing every time lets me take out a bunch of complexity elsewhere, which stabilises the rendering loop.

Annoyingly I've now broken it again in mysterious ways after getting it working reliably so I can't raise a PR yet

[flochtililoch]

Thanks for your investigation, I'm facing the same issue as you described here:

after that it appears permanently on the next screen you swap to but with the previous state selected

Spent a decent amount of time on this but haven't quite figured what the issue is. I was a bit baffled at first by how this would work fine on the demo app, and I just realize it doesn't, just the styles we apply there obfuscate the issue: the tab bar is absolute-positioned at the bottom of the screen, so that second unexpected instance always shows under the main, navigator driven tab bar.

I've tried a few variations on the way to share the element state between the screen and the navigator (caching the state setter with useCallback, handling the state with useReducer) but no luck so far. From what I found the issue stems from the call to onUpdate here. The reason we make this call is to execute behaviors which ultimately perform the navigation. I've tried to instead call the navigate function directly on a navigation ref that I'm storing, and this partially solves the issue, but navigating back to the first tab causes the second tab to be immediately selected again.
As you pointed out there seem to be something off with the re-render caused by that context sharing. Looking forward to see your solution!

[ehenighan]

So far I've resolved those early issues and been able to simplify the overall rendering as a result, but now I'm stuck on a competing rendering loop when you switch back between tabs that are already loaded. No problem when the tab is loaded for the first time, but when it's already cached I'm getting a race condition between screens.

I have a sneaking suspicion it's because with the fetch path there's going to be an async / await bit somewhere but when there's a pre-cached version there won't be, so there's no break in the event loop to allow the handling to complete in the right order.

Code changes so far:

COMPONENT BottomTabBar.tsx:
const BottomTabBar = (props: HvComponentProps) => {
    //main change in here is that we only need to depend on the function from the context, not the whole context object
    const { setElementProps } = useContext(BottomTabBarContext);
    const navigator = props.element.getAttributeNS(namespaceURI, 'navigator');
    useEffect(() => {
        if (!navigator) {
            console.warn(
                '<navigation:bottom-tab-bar> element is missing `navigator` attribute',
            );
            return;
        }
        if (!setElementProps) {
            return;
        }
        console.log(`${Date()} ${props.element.getAttribute('id')}: Registering navigator props from BottomTabBar no-render component`);
        setElementProps(navigator, props);
    }, [navigator, props, setElementProps]);
    return null;
};

CONTEXT BottomTabBar.tsx:
import React, {createContext, useCallback, useRef, useState} from 'react';
import type { HvComponentProps } from 'hyperview';

type ElementProps = Record<string, HvComponentProps>;

/**
 * React context that provides the Hyperview demo app with a state
 * holding the navigation elements rendered by each screens that
 * React navigation navigators use to drive navigation.
 */
export const BottomTabBarContext = createContext<{
    elementsProps: ElementProps | undefined;
    setElementProps: ((navigator: string, props: HvComponentProps) => void) | undefined;
}>({
    elementsProps: undefined,
    setElementProps: undefined,
});

export function BottomTabBarContextProvider(props: {
    children: React.ReactNode;
}) {
    const [elementsProps, setElementsProps] = useState<ElementProps>({});
    //dirty dirty useRef abuse so the update function itself never needs to be recreated.
    // this cuts down on the number of unnecessary re-renders and props updates from the component itself.
    const elementsPropsRef = useRef<ElementProps>(elementsProps);
    elementsPropsRef.current = elementsProps;
    const setElementProps = useCallback((navigator: string, p: HvComponentProps) => {
        const currentNavigatorProps = elementsPropsRef.current[navigator] || {};
        if (!!p && currentNavigatorProps === p) {
            console.log(`${Date()} ${currentNavigatorProps['element']?.getAttribute('id')} Skipping context props cache update to ${p.element?.getAttribute('id')} because the props object has not changed`);
            return;
        }
        console.log(`${Date()} ${currentNavigatorProps['element']?.getAttribute('id')} Doing context props cache update to ${p.element?.getAttribute('id')} because the props object has changed`);
        const newElementsProps = { ...elementsPropsRef.current, [navigator]: p };
        elementsPropsRef.current = newElementsProps;
        setElementsProps(newElementsProps);
    }, [elementsPropsRef, setElementsProps]);

    return (
        <BottomTabBarContext.Provider
            value={{
                elementsProps,
                setElementProps
            }}
        >
            {props.children}
        </BottomTabBarContext.Provider>
    );
}

React Navigation component - BottomTabBar index.tsx:
export const BottomTabBar = ({ id }: Props): JSX.Element | null => {
    // id is provided by Hyperview, and represents a tab navigator id
    const { elementsProps } = useContext(BottomTabBarContext);

    // Props are the props received by the component backing the custom
    // Hyperview element <navigation:bottom-tab-bar>
    const props = elementsProps?.[id];

    // It is assumed here the component will nest a single child
    const child = Array.from(props?.element?.childNodes || []).find(
        node => node.nodeType === NODE_TYPE.ELEMENT_NODE,
    ) as Element | undefined;

    // Since the state of the element is no longer held by the screen's doc
    // that provides it, we create a local state to store the child element
    // and keep it in sync with the child provided by the props
    const [element, setElement] = useState<Element | undefined>(child);
    useEffect(() => {
        setElement(child);
    }, [child]);

    //Now we've got cleaner behaviour and more obvious dependencies from the context provider,
    // we don't need so much special handling in here to keep the components in sync
    const onUpdateProps = props?.onUpdate;
    const onUpdate: HvComponentOnUpdate = useCallback<HvComponentOnUpdate>((
        href,
        action,
        currentElement,
        opts
    ) => {
        if (!onUpdateProps) {
            return;
        }

        if (currentElement.parentElement) {
            console.info(`${Date()} ${currentElement.parentElement.getAttribute('id')} / ${currentElement.getAttribute('id')}: onUpdate href: [${href}], action: [${action}]`);
        } else {
            console.info(`${Date()} ${currentElement.getAttribute('id')}: onUpdate href: [${href}], action: [${action}]`);
        }

        onUpdateProps(href, action, currentElement, opts);
    }, [onUpdateProps]);

    if (!props || !element) {
        return null;
    }

    return Render.renderElement(element, props.stylesheets, onUpdate, {
        componentRegistry: props.options?.componentRegistry,
    }) as JSX.Element | null;
};

Annotated logs from working the above example from one tab to another (good) and back (bad):

#Load the navigator
 Wed Oct 09 2024 16:53:34 GMT+0100 http://192.168.0.24:8083/native-index.xml - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"sohgi-pvfb0DcI-777fAoq0NjNOQmBHnSuhx4raJD5k1gizd07FSu9pfTd4uSbje2Jr0lJQ4oer2_HLKco5AgI-_Oa1T4R3u"}
 INFO  Wed Oct 09 2024 16:53:34 GMT+0100 http://192.168.0.24:8083/native-index.xml - COMPLETE

#Select the first available route in the navigator
 LOG  Wed Oct 09 2024 16:53:34 GMT+0100 http://192.168.0.24:8083/home - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"Mwi0MwqehIfSWsjtAxeMdTcGvhXnXpaIehyzt4e103zgYTsQUjGGAzqutOT_Y_-INDq4Qw4zkyyBOvWlQnqC1b6D5UiGAgoj"}
 INFO  Wed Oct 09 2024 16:53:35 GMT+0100 http://192.168.0.24:8083/home - COMPLETE

#We've loaded it, so we expect to see this happen - we're registering our navbar component for the first time.
 LOG  Wed Oct 09 2024 16:53:35 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:35 GMT+0100 undefined Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#I've clicked the Questionnaires tab, so we're now starting the swap action
 INFO  Wed Oct 09 2024 16:53:51 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]

#The swap action goes via the parent onUpdate function, which I think will end up changing the parent props and therefore causing a re-render.
 LOG  Wed Oct 09 2024 16:53:51 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:51 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#THEN we pick up the navigation behaviour, which is via the hasUpdated function on the option itself. Not totally sure why that's later than the swap, but it is.
 INFO  Wed Oct 09 2024 16:53:51 GMT+0100 nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 http://192.168.0.24:8083/questionnaires - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"pD6eEaAHzhptowJ4zrPocSQlsMM1gBES7v7mh0W7zMreeD8kxQesIZA3_nlAmjUd-Z7cRx0QnfpT5HI_1pjX5XyN-v64Gw4X"}

#While the network request is in flight, we re-render again. I suspect because the screen is being removed, but not totally sure? Seems harmless anyway.
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:52 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 INFO  Wed Oct 09 2024 16:53:52 GMT+0100 http://192.168.0.24:8083/questionnaires - COMPLETE

#And now we're loading the questionnaires screen just like we did for the home page originally. So far, so good.
 LOG  Wed Oct 09 2024 16:53:53 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:53:53 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#Now I've clicked the Home tab, so we're starting the swap action just as you'd expect.
 INFO  Wed Oct 09 2024 16:54:00 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]

#Just like when we switched to the Questionnaires tab, the swap action goes via the parent onUpdate function, which I think will end up changing the parent props and therefore causing a re-render.
 LOG  Wed Oct 09 2024 16:54:00 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:00 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#THEN just as before we pick up the navigation behaviour
 INFO  Wed Oct 09 2024 16:54:00 GMT+0100 nav-bar-option-main-page-tab-home: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-home\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]

#And here we're into an update loop.
#Interestingly it's the home screen which starts the loop off, even though we're currently navigating off the questionnaires page.
#If there was an in-flight fetch request, there'd be no home page at all yet, so this must only be possible because we're
# navigating back to a pre-cached screen.

#Loop step 1: home page starts taking over earlier than expected
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
#Loop step 2: questionnaire page re-renders as well and tries to register itself as the display component on the context - maybe because it's being hidden, as I suspected earlier? Still not sure.
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed
#Loop step 3: home page strikes back
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
#Loop step 4: just tit-for-tat forever at this point.
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:01 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed
 LOG  Wed Oct 09 2024 16:54:02 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:02 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Wed Oct 09 2024 16:54:02 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Wed Oct 09 2024 16:54:02 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

[ehenighan]

The last file changes work because with better behaviour from the context provider the component re-renders as needed based on the context updates anyway, so there's less of a need for explicit local state management - I actually think it's worth experimenting with removing its internal useEffect/useState entirely and clean up the rendering loop by just getting the element directly from the context object every time

[ehenighan]

Still not sure if genuinely relevant but here's (possibly) the code snippet I predicted would exist to explain the behaviour differences:

hyperview/src/core/components/hv-screen/index.js

Lines 197 to 208 in b5a6ada

	if (this.initialDoc) {
	doc = this.initialDoc;
	this.initialDoc = null;
	} else {
	// eslint-disable-next-line react/no-access-state-in-setstate
	const {
	doc: loadedDoc,
	staleHeaderType: loadedType,
	} = await this.parser.loadDocument(this.state.url);
	doc = loadedDoc;
	staleHeaderType = loadedType;
	}

[flochtililoch]

My explorations today, with help from @hgray-instawork, surfaced that this line might be the culprit in the infinite loop we're seeing when navigation is performed.

@hgray-instawork we should discuss further on how to accomplish differently what this code currently do.

[ehenighan]

Could it just be that if (foundIndex > -1) should become if (foundIndex > -1 && foundIndex !== curState.index) to prevent unnecessary resets?

Separately, I think the loop I'm seeing in my patched version might be resolvable in a different way - the no-render component registers itself with the context on render but it's only there to make sure we show the UI components currently in the DOM, so it only needs to happen when the screen is focused.

I'm going to try replacing the useEffect in my current solution with a useFocusEffect (https://reactnavigation.org/docs/use-focus-effect) so it only registers its UI component with the context on props change if it is the current screen.

[ehenighan]

Okay, still got a loop but it's now much better defined so I'm hoping I can debug it fairly easily from here.
Changes:

1. Changed the useEffect to a useFocusEffect in the no-render component so it only tries to register its child components for display when it's the active screen
2. Stripped out the useEffect/useState loop from the 'real' navbar component now the context provider is well-behaved and the no-render component is only pushing new UI components to it when genuinely appropriate.

Code now looks like this:

const BottomTabBar = (props: HvComponentProps) => {
    const { setElementProps } = useContext(BottomTabBarContext);
    const navigator = props.element.getAttributeNS(namespaceURI, 'navigator');
    useFocusEffect(React.useCallback(
        () => {
            if (!navigator) {
                console.warn(
                    '<navigation:bottom-tab-bar> element is missing `navigator` attribute',
                );
                return;
            }
            if (!setElementProps) {
                return;
            }
            console.log(`${Date()} ${props.element.getAttribute('id')}: Registering navigator props from BottomTabBar no-render component`);
            setElementProps(navigator, props);
        },
        [navigator, props, setElementProps]
    ));
    return null;
};

export function BottomTabBarContextProvider(props: {
    children: React.ReactNode;
}) {
    const [elementsProps, setElementsProps] = useState<ElementProps>({});
    const elementsPropsRef = useRef<ElementProps>(elementsProps);
    elementsPropsRef.current = elementsProps;
    const setElementProps = useCallback((navigator: string, p: HvComponentProps) => {
        const currentNavigatorProps = elementsPropsRef.current[navigator] || {};
        if (!!p && currentNavigatorProps === p) {
            console.log(`${Date()} ${currentNavigatorProps['element']?.getAttribute('id')} Skipping context props cache update to ${p.element?.getAttribute('id')} because the props object has not changed`);
            return;
        }
        console.log(`${Date()} ${currentNavigatorProps['element']?.getAttribute('id')} Doing context props cache update to ${p.element?.getAttribute('id')} because the props object has changed`);
        const newElementsProps = { ...elementsPropsRef.current, [navigator]: p };
        elementsPropsRef.current = newElementsProps;
        setElementsProps(newElementsProps);
    }, [elementsPropsRef, setElementsProps]);

    return (
        <BottomTabBarContext.Provider
            value={{
                elementsProps,
                setElementProps
            }}
        >
            {props.children}
        </BottomTabBarContext.Provider>
    );
}

export const BottomTabBar = ({ id }: Props): JSX.Element | null => {
    // id is provided by Hyperview, and represents a tab navigator id
    const { elementsProps } = useContext(BottomTabBarContext);

    // Props are the props received by the component backing the custom
    // Hyperview element <navigation:bottom-tab-bar>
    const props = elementsProps?.[id];

    // It is assumed here the component will nest a single child
    const child = Array.from(props?.element?.childNodes || []).find(
        node => node.nodeType === NODE_TYPE.ELEMENT_NODE,
    ) as Element | undefined;

    //This is only here to let me inject logging into the callback - can be removed later.
    const onUpdateProps = props?.onUpdate;
    const onUpdate: HvComponentOnUpdate = useCallback<HvComponentOnUpdate>((
        href,
        action,
        currentElement,
        opts
    ) => {
        if (!onUpdateProps) {
            return;
        }
        console.info(`${Date()} ${currentElement.getAttribute('id')}: onUpdate href: [${href}], action: [${action}]`);
        onUpdateProps(href, action, currentElement, opts);
    }, [onUpdateProps]);

    if (!props || !child) {
        return null;
    }

    return Render.renderElement(child, props.stylesheets, onUpdate, {
        componentRegistry: props.options?.componentRegistry,
    }) as JSX.Element | null;
};

Now the tab bar display is totally stable and updates like you'd expect, with no unnecessary re-renders.

The remaining loop is again when you navigate back to a previously-loaded tab, but the source is clearer now - the hv-option componentDidUpdate function is called when the destination tab is rendered, but the prevProps and props are in disagreement so it thinks it's re-selecting the option you previously clicked on to leave the screen in the first place.

However, the loop behaviour is 'improved', if you like - it completes a real navigation between tabs gracefully with no unnecessary re-renders, and it's just flicking properly between them constantly. If you put a delay="1" on the behavior element you can even see the switch take place instead of locking the render loop out completely.

I think this is going to be something to do with the cached props from the context in some way - if the 'prevProps' are from the page you just left, they will have tab 1 selected now instead of tab 2, but the current component previously had tab 2 selected. Need to chase the loop around a bit and see where it's coming from.

New logs:

#Load the navigator
 LOG  Thu Oct 10 2024 09:00:26 GMT+0100 http://192.168.0.24:8083/native-index.xml - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h"}
 INFO  Thu Oct 10 2024 09:00:26 GMT+0100 http://192.168.0.24:8083/native-index.xml - COMPLETE

#Load the first tab automatically
 LOG  Thu Oct 10 2024 09:00:27 GMT+0100 http://192.168.0.24:8083/home - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"ygG_dN-hTiqS8dZstiw2ofB7zv7cxCZLlxzdWyDjcTYJ8tg5_mSOQb3He0u_weQI1QECksNO45zvokJm9C3tahGASAJrke1f"}
 INFO  Thu Oct 10 2024 09:00:27 GMT+0100 http://192.168.0.24:8083/home - COMPLETE

#Now we're rendering the first screen so we'd expect to register the component
 LOG  Thu Oct 10 2024 09:00:28 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:00:28 GMT+0100 undefined Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Token
 LOG  Notification  undefined
 LOG  Thu Oct 10 2024 09:00:28 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:00:28 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#Begin the swap action on selection of the second tab
 INFO  Thu Oct 10 2024 09:00:52 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]

#Re-rendering here due to the swap completion
 LOG  Thu Oct 10 2024 09:00:52 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:00:52 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#And now we're beginning the navigation action
 INFO  Thu Oct 10 2024 09:00:52 GMT+0100 nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]

#Request the new screen
 LOG  Thu Oct 10 2024 09:00:52 GMT+0100 http://192.168.0.24:8083/questionnaires - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"M2azvefbaxIlXP601cBX9DvSoGzTsimMF17yJ6sfMLcVAKFBBwOCiIW9XnMIbMzQtu1jxwjnjQ7g1E2hdG_CFpp8CYN3Y5Qn"}
 INFO  Thu Oct 10 2024 09:00:52 GMT+0100 http://192.168.0.24:8083/questionnaires - COMPLETE

#We've got the new screen now and we're going to render it in - great.
 LOG  Thu Oct 10 2024 09:00:53 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:00:53 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#Begin the swap action on selection of the first tab again
 INFO  Thu Oct 10 2024 09:01:03 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]

#Re-rendering here due to the swap completion
 LOG  Thu Oct 10 2024 09:01:03 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:03 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#And now we're beginning the navigation action
 INFO  Thu Oct 10 2024 09:01:04 GMT+0100 nav-bar-option-main-page-tab-home: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-home\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]

#Home page is taking over, kind of as expected
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
#And now it's doing it again? Not sure why.
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#But we're attempting a new navigation action back to questionnaires? Why?
 INFO  Thu Oct 10 2024 09:01:04 GMT+0100 nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:04 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#And now we're navigating back to the home page again?
 INFO  Thu Oct 10 2024 09:01:05 GMT+0100 nav-bar-option-main-page-tab-home: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-home\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:05 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#At this point, we're still in the loop - BUT it's now a much cleaner and better-behaved loop.
 INFO  Thu Oct 10 2024 09:01:05 GMT+0100 nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Thu Oct 10 2024 09:01:06 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:06 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed
 LOG  Thu Oct 10 2024 09:01:06 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:06 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

 INFO  Thu Oct 10 2024 09:01:06 GMT+0100 nav-bar-option-main-page-tab-home: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" target=\"main-page-tab-home\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Thu Oct 10 2024 09:01:07 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:07 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed
 LOG  Thu Oct 10 2024 09:01:07 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 09:01:07 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

[ehenighan]

Yep, confirmed with some additional logging - the prevProps received by componentDidUpdate on hv-option come from the previous screen where the selection has already changed, but its current props are preserved from our previous visit to the new screen, where the selection is back on the other tab.

I'll have a think about how we could avoid this - it's definitely due to the way we're caching the props and overwriting them, but I'm not quite clear on exactly why this means the new screen components are actually receiving the old props from the previous screen in this case when they aren't when we have to await a server hit.

#Initial swap to the new un-cached tabpage:
 INFO  Thu Oct 10 2024 10:53:53 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]
 LOG  Thu Oct 10 2024 10:53:54 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 10:53:54 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#Swap completed. Now we're going to fire hv-option componentDidUpdate, and the previous and new components both came from the first screen as we haven't left it yet.
 LOG  Option selected - previous props component ID: main-screen-home-nav-bar-option-main-page-tab-questionnaires
 LOG  Option selected - current  props component ID: main-screen-home-nav-bar-option-main-page-tab-questionnaires

#...and as it should, that triggers a navigation.
 INFO  Thu Oct 10 2024 10:53:54 GMT+0100 main-screen-home-nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" delay=\"1\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Thu Oct 10 2024 10:53:54 GMT+0100 http://192.168.0.24:8083/questionnaires - {"Cache-Control":"no-cache, no-store, must-revalidate","Expires":"0","Pragma":"no-cache","Accept":"application/xml, application/vnd.hyperview+xml","X-Hyperview-Version":"0.85.0","X-Hyperview-Dimensions":"392.72727272727275w 783.2727272727273h","X-CSRF-Token":"URnbvFGYaxVfUwvzHlbBIPhpscALESJk9f_d4GJl6NJ19jvAZSHjjmahCXFyMDiXfHv1E5wNnPlqKERJzci_hABUi-NEzl31"}
 INFO  Thu Oct 10 2024 10:53:54 GMT+0100 http://192.168.0.24:8083/questionnaires - COMPLETE
 LOG  Thu Oct 10 2024 10:53:54 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 10:53:54 GMT+0100 navbar-footer-container-main-screen-home Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#Now we've clicked to go back to the first tab.
 INFO  Thu Oct 10 2024 10:54:03 GMT+0100 navbar-footer: onUpdate href: [#], action: [swap]
 LOG  Thu Oct 10 2024 10:54:03 GMT+0100 navbar-footer-container-main-screen-questionnaires: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 10:54:03 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-questionnaires because the props object has changed

#Swap completed. Now we're going to fire hv-option componentDidUpdate, and the previous and new components both came from the second screen as we haven't left it yet.
 LOG  Option selected - previous props component ID: main-screen-questionnaires-nav-bar-option-main-page-tab-home
 LOG  Option selected - current  props component ID: main-screen-questionnaires-nav-bar-option-main-page-tab-home

#...and as it should, that triggers a navigation.
 INFO  Thu Oct 10 2024 10:54:03 GMT+0100 main-screen-questionnaires-nav-bar-option-main-page-tab-home: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" delay=\"1\" target=\"main-page-tab-home\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]
 LOG  Thu Oct 10 2024 10:54:03 GMT+0100 navbar-footer-container-main-screen-home: Registering navigator props from BottomTabBar no-render component
 LOG  Thu Oct 10 2024 10:54:03 GMT+0100 navbar-footer-container-main-screen-questionnaires Doing context props cache update to navbar-footer-container-main-screen-home because the props object has changed

#However, now we have a problem - the previous props for the hv-options element are from the SECOND screen we've just left
# On that screen, the selection has already moved to the FIRST tab.
# On THIS screen, the current selection left over from last time (and restored via the props!) is the SECOND tab
# - so the comparison thinks we're returning to the previous tab.
 LOG  Option selected - previous props component ID: main-screen-questionnaires-nav-bar-option-main-page-tab-questionnaires
 LOG  Option selected - current  props component ID: main-screen-home-nav-bar-option-main-page-tab-questionnaires

#And voila - a navigation.
 INFO  Thu Oct 10 2024 10:54:04 GMT+0100 main-screen-home-nav-bar-option-main-page-tab-questionnaires: onUpdate href: [], action: [navigate]
 INFO  [behavior] | action: navigate | ["<behavior trigger=\"select\" delay=\"1\" target=\"main-page-tab-questionnaires\" action=\"navigate\" xmlns=\"https://hyperview.org/hyperview\"/>"]

[ehenighan]

So far I have a solution of sorts but it is absolutely disgusting and there's no way it's tolerable.

The problem as I understand it:

1. When you have two Hyperview screen definitions for two tabs, you have two sets of 'DOM'-type elements, one representing each of the screens
2. These are mapped eventually onto underlying React Native components with props.
3. At the framework layer there's a wrapper HvComponentProps which contains a reference to the Element and some other things
4. We have one navigation bar component, which is permanent-ish across the bottom of the app, and this component uses a context to tell it which of the UI elements to render based on the current active screen.
5. When you make a tab selection, that updates the UI selection on the current active screen from Tab A to Tab B, and then it switches in the screen for Tab B with a new set of UI components which come with Tab B pre-selected.
6. At this point the screen components for Tab A are no longer visible, but in the background the select-single components still have Tab B selected
7. When you switch back from Tab B to Tab A, Screen A is rendered again, with its old props as the current ones with Tab B selected and the props you are no longer applying from Screen B now having Tab A selected, as the 'previous props' - because these components are switched through the same parent.
8. This looks like a select action to the underlying options component, and this causes a navigation back to Tab B, etc. etc.

I have currently some really hacky code in place which abuses useRef to check for screen changes between known cached screens and then sets a 'settling' flag which we use to ignore the next spurious navigation event. This works but it still leaves events bouncing round which slows down tab transitions, plus it's absolutely gross and will almost certainly break in weird situations.

I think my slightly better but still not very nice solution will be something like:

• add the screen key somehow to the props on the child objects, so we can reliably tell the source of a navigation action, and then ignore navigation actions from screens other the current screen (I feel like this prop already exists somewhere but I can't see it)
• upon scheduling a navigation action, schedule a custom event to fire for 'reset-selection' with <navigation delay + 1> delay which must be handled by a custom on-event behaviour relevant to the UI component you implement and which will set the source page's tab selection component back to the original value
• this will trigger a navigation event from the old screen, but we'll ignore it because we're after the navigation has completed and therefore we're on the new screen

It would be a lot nicer to implement this if instead there was a nice on-completion callback I could hook into for the navigation action, but I can't see one - is there something I'm missing?

[ehenighan]

Slightly better solution seems to work stably but is a bit sluggish due to tonnes of logging and some excessive re-renders - I don't think I've got the screen-key setting right but I think I know what I need to change. Hoping to have a working PR tomorrow.

Will require people defining a bottom nav bar component to make sure they have a reset-selection event handling behaviour defined but the alternative was assuming that the display component was always going to be a select-single and resetting it directly, which felt structurally wrong.

Let me know if there actually is a nice post-navigation callback hook I can use that I've missed, because it'll clean things up even further! Hopefully what I've got working isn't too mad now.

Thanks,

Ed

[ehenighan]

Good news - managed to find a solution that let me ditch most of the evil stuff I was doing!

@flochtililoch this branch has the relevant stuff in it:
master...ehenighan:hyperview:navigator-loop-refactor

Under normal circumstances this works fine now, without any extra event firing needed etc., and without too many unnecessary renders.

It's still definitely possible to get it stuck in an infinite loop of navigation if you click around fast enough for long enough, but that was still the case even with my nastiest but most belt & braces attempts, so I'm going to pause it here if you don't mind and let someone with better React Native skills than me try to solve the remaining issue with clicking around too fast.

Hope this is useful to you, and apologies if my branch isn't usable directly - got fed up with the linter shouting at me so there may be bits that aren't quite right.

EDIT: Disregard that, think I've sorted the last remaining problem and now I can't make it infinite loop any more. All yours :)

[flochtililoch]

Thanks Ed, sorry for not getting back to you earlier, I appreciate your effort in helping solve this issue!
I've decided to bring some of your suggestions and my side explorations back into the demo app, and will look into integrating your latest findings. I'm currently working on this branch, and will update this thread when I have more details.

[ehenighan]

No problem, and thanks for taking an interest! FWIW I suspect the best thing for the long run will be to remove this tension, around the fact that the components are rendered as part of a screen but are actually managed by a component that sits outside the screen lifecycle, by lifting the navigation components up out of the individual screen definition to become a first-class citizen in its own right at the same level as screen and navigator - or perhaps nested in navigator documents instead, although that pollutes the concept a bit. Looking forward to seeing where you go with it!

[flochtililoch]

@ehenighan Thanks again for your investigation work, it definitely helped crafting a fix for this issue! Closing as resolved for now.