Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload the page resets back the default pipeline #2041

Merged
merged 18 commits into from
Sep 5, 2024
Merged

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Aug 16, 2024

Description

Related to: #1997

When you're in any other pipeline view rather the default pipeline, if you load the URL again, it will reset to the default page.

Development notes

  • Introduced the modularPipelineTree logic to ensure that only the children of a modular pipeline are expanded.

  • Added a check to avoid overwriting the active pipeline if one is already set.

  • Removed the urlParams from preparePipelineState in the getInitialState method, as they were causing unexpected overwrites.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@SajidAlamQB SajidAlamQB requested a review from Huongg August 16, 2024 12:10
@SajidAlamQB
Copy link
Contributor Author

One issue I noticed was when you're in any other pipeline view except the default and you click a node when the page refreshes the node is not highlighted anymore and the metadata panel doesn't open. In the default view reloading with a node selected keeps it highlighted and opens metadata panel.

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

works for me. thanks

@Huongg
Copy link
Contributor

Huongg commented Aug 19, 2024

hey @SajidAlamQB I think the pipeline bug is fixed now but as you said there's other bug about the selected node is not highlighted and metadata panel is not opened. And even in the default pipeline, the node is not highlighted even the meta data panel opens 🤔

Screenshot 2024-08-19 at 17 00 18

@SajidAlamQB
Copy link
Contributor Author

SajidAlamQB commented Aug 20, 2024

hey @SajidAlamQB I think the pipeline bug is fixed now but as you said there's other bug about the selected node is not highlighted and metadata panel is not opened. And even in the default pipeline, the node is not highlighted even the meta data panel opens 🤔

I'm not able to see the bug with the default pipeline, but it does exist with the other pipeline views. I'm not too sure what the best fix for this is, do you have any ideas?

@Huongg
Copy link
Contributor

Huongg commented Aug 21, 2024

hey @SajidAlamQB I think the pipeline bug is fixed now but as you said there's other bug about the selected node is not highlighted and metadata panel is not opened. And even in the default pipeline, the node is not highlighted even the meta data panel opens 🤔

I'm not able to see the bug with the default pipeline, but it does exist with the other pipeline views. I'm not too sure what the best fix for this is, do you have any ideas?

hey @SajidAlamQB I can still see the issue from my side. I think it might be related to your previous issue. Have you looked at redirectToSelectedNode in FlowchartWrapper? Perhaps the issue started there. This is where we get the params from the URL and load the node if nodeID existed etc. Perhaps something went missing there? 🤔 happy to jump on the call to do it together if it helps

@rashidakanchwala rashidakanchwala self-requested a review September 2, 2024 11:30
@SajidAlamQB
Copy link
Contributor Author

SajidAlamQB commented Sep 4, 2024

This PR also fixes two bugs:

  1. Flowchart View Splitting into Two Trees When Expand Modular Pipelines is Enabled:
    When the Expand Modular Pipelines button was enabled with expandAllPipelines=true in the query string, selecting a node and refreshing the page caused the flowchart view to break, splitting it into two separate trees (see the screenshot below):

image

  1. Node Not Highlighted After Page Refresh in Non-Default Pipeline views:
    The second bug happened when a node was not highlighted after refreshing the page in non-default pipeline views.

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

it's working from my side, thanks @SajidAlamQB for the changes 👍

@jitu5
Copy link
Contributor

jitu5 commented Sep 5, 2024

@SajidAlamQB have you tested with this scenario #1879 ?

@SajidAlamQB
Copy link
Contributor Author

SajidAlamQB commented Sep 5, 2024

@SajidAlamQB have you tested with this scenario #1879 ?

I've tested it and it now opens on this page in Incognito:

image

in Safari:

image

With the following in pipeline_registry.py:

    return {
        "": (
            ingestion_pipeline
            + feature_pipeline
            + modelling_pipeline
            + reporting_pipeline
        ),
        "Data ingestion": ingestion_pipeline,
        "Modelling stage": modelling_pipeline,
        "Feature engineering": feature_pipeline,
        "Reporting stage": reporting_pipeline,
        "Pre-modelling": ingestion_pipeline + feature_pipeline,
    }

Copy link
Contributor

@jitu5 jitu5 left a comment

Choose a reason for hiding this comment

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

@SajidAlamQB SajidAlamQB merged commit 0ff262e into main Sep 5, 2024
19 checks passed
@SajidAlamQB SajidAlamQB deleted the feat/reload-page branch September 5, 2024 17:04
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