dash-bootstrap-components icon indicating copy to clipboard operation
dash-bootstrap-components copied to clipboard

Tooltip closing race condition with children change callback

Open FRosner opened this issue 5 months ago • 6 comments

  • dash version: #2.14.2
  • dash-bootstrap-components version: #1.5.0
  • components affected by bug: tooltip

What is happening?

In a dash SPA when opening a page with a dbc tooltip, then navigating to a new page, the page fails to render with

Error: An object was provided as `children` instead of a component, string, or number (or list of those). Check the children property that looks something like:
{
  "3": {
    "props": {
      "is_open": true
    }
  }
}

It's not specific to the SPA callback. I managed to reproduce it with a callback that upon clicking a button with a tooltip removes the button and the tooltip from the children of their parents.

See https://stackoverflow.com/questions/75175955/an-object-was-provided-as-children-instead-of-a-component-error-in-python-da for more details.

What should be happening?

The tooltip should be removed and the client side callback should not attempt to patch the non-existing tooltip component, effectively corrupting the children property.

Code

mport dash
from dash import dcc
from dash import html
import dash_bootstrap_components as dbc
from dash.dependencies import Input, Output

app = dash.Dash(__name__, suppress_callback_exceptions=True)


app.layout = html.Div([
    dcc.Location(id='url', refresh=False),
    html.Div(id='page-content'),
])


def get_overview_page():
    page = html.Div([
        dcc.Link(
            html.Button('Neu', id='new_button',
                        style={'margin-left': '10px', 'width': '100px', 'height': '27px',
                               'fontSize': '16px'}),
            href='/new-entry'
        ),
        dbc.Tooltip(
            "A.",
            target="new_button", placement="top"
        )
    ], style={'width': '30%', 'margin-top': '10px', 'display': 'inline-block', 'text-align': 'left'})
    return page


# Update the index
@app.callback(Output('page-content', 'children'),
              [Input('url', 'pathname')])
def display_page(pathname):
    if pathname == '/new-entry':
        return html.Div()
    else:
        return get_overview_page()

if __name__ == '__main__':
    app.run_server(debug=True, port=8086, host='127.0.0.1')

Error messages

Error: An object was provided as `children` instead of a component, string, or number (or list of those). Check the children property that looks something like:
{
  "1": {
    "props": {
      "is_open": true
    }
  }
}

FRosner avatar Jan 15 '24 15:01 FRosner

I wonder if this is caused by fade or delay? I'm trying to find the code that updates the is_open erroneously... @tcbegley can you help me?

Is it here?

https://github.com/facultyai/dash-bootstrap-components/blob/71b3070de6bc457e971978c98d3042f4feece51b/src/private/Overlay.js#L62-L80

If it's delay, setting delay={"show": 0, "hide": 0}, should be a reasonable workaround.

FRosner avatar Jan 15 '24 15:01 FRosner

Hi @FRosner,

Thanks for reporting this and for providing so much detail and a minimal reproduction.

The issue is indeed in hideWithDelay. When the "close" even happens, we call setProps({is_open: false}) with a delay. In this case in the intervening time the component is unmounted. That means when Dash tries to update the prop the component no longer exists.

I think the fix here will be to either:

  1. modify the timeout so that the call to setProps only happens if the component hasn't been unmounted
  2. cancel outstanding timeouts when the component unmounts

I think 2) is probably cleaner but looking over the code briefly might require a bit of refactoring. I'll see what I can do.

tcbegley avatar Jan 16 '24 21:01 tcbegley

I'll see what I can do.

Thank you so much!

FRosner avatar Jan 17 '24 15:01 FRosner

I think the fix here will be to either:

To me, 1 sounds preferrable, as it has limited side effects and makes it clearer when the function is passed to setTimeout that this is something to be aware of. Why do you think 2 is cleaner?

FRosner avatar Jan 17 '24 15:01 FRosner

Why do you think 2 is cleaner?

Well the timeouts only make sense in the context of the component, so if the component no longer exists it seems neater to stop them from running at all rather than to run and do little / nothing. But tbh both options are similar so I'll take whatever solution is simpler to implement.

I did have a go at getting this to work yesterday but it turned out to be fiddlier than expected. I might need to refresh some of my react knowledge and try again when I get a chance.

tcbegley avatar Jan 17 '24 21:01 tcbegley

Thanks! I am trying the workaround of setting hide delay to 0 in the meantime. I'd also take a stab at it but last time I used react is 5 years ago :D

FRosner avatar Jan 18 '24 12:01 FRosner

Small addition from my side: I assume the same happens with the alert component and duration. It gave me a headache why the app raised an error after a few seconds when switching pages in a multi-page app, displaying the new page correctly. More confusingly for me: it sometimes took a bit longer, but in the end, it was related to how fast you switched to the new page before the duration time ran out.

jonicohn avatar Apr 02 '24 13:04 jonicohn

Hi @jonicohn

Which version of dash-bootstrap-components are you using? It sounds like what you're describing could be related to this issue which should have been fixed in v1.4.2

tcbegley avatar Apr 02 '24 20:04 tcbegley

Hi tcbegley,

sorry, my bad. You are absolutely correct. 1.5.0 is working fine.

jonicohn avatar Apr 03 '24 04:04 jonicohn

Sorry this took some time, it took me a while to get my head around it. It's now fixed in the latest version.

pip install -U dash-bootstrap-components

tcbegley avatar Apr 14 '24 22:04 tcbegley