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.

Sign up for GitHub

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

Jump to bottom

WebUI: Add pipeline status tab #1090

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

WebUI: Add pipeline status tab #1090

wants to merge 15 commits into from

Conversation

ArnoChenFx
Copy link
Contributor

screenshot

@ArnoChenFx ArnoChenFx changed the title Add pipeline status tab and document pipeline status API WebUI: Add pipeline status tab and document pipeline status API Mar 14, 2025
@ArnoChenFx ArnoChenFx changed the title WebUI: Add pipeline status tab and document pipeline status API WebUI: Add pipeline status tab Mar 14, 2025
@LarFii LarFii requested a review from danielaskdd March 15, 2025 11:17
ParisNeo
ParisNeo approved these changes Mar 15, 2025
View reviewed changes
Copy link
Contributor

@ParisNeo ParisNeo left a comment

Choose a reason for hiding this comment

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

To me it seems ok

@danielaskdd danielaskdd added the ui web ui label Mar 15, 2025
danielaskdd
danielaskdd requested changes Mar 16, 2025
View reviewed changes
Copy link
Collaborator

@danielaskdd danielaskdd left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM. Please merge from webui-node-expansion and submit the request agian. Due to significant refactoring work in WebUI's underlying structure, we need to resolve some conflicts.

@ArnoChenFx
Merge remote-tracking branch 'origin/main' into dev-webui
fd36baa 
# Conflicts:
#	lightrag/api/webui/assets/index-B0mjnzMd.js
#	lightrag/api/webui/index.html
#	lightrag_webui/src/api/lightrag.ts
#	lightrag_webui/src/components/graph/PropertiesView.tsx
#	lightrag_webui/src/components/graph/Settings.tsx
#	lightrag_webui/src/components/graph/ZoomControl.tsx
#	lightrag_webui/src/features/DocumentManager.tsx
#	lightrag_webui/src/features/SiteHeader.tsx
#	lightrag_webui/src/i18n.js
#	lightrag_webui/src/main.tsx
@ArnoChenFx
lint
801ae77
@ArnoChenFx
feat: display document filepath in the document manager UI
9eb7e07
@ArnoChenFx
build webui
41d51cd
@ArnoChenFx
fix lint
c5fe656
@ArnoChenFx
Copy link
Contributor Author

ArnoChenFx commented Mar 19, 2025 •
edited
Loading

Thanks. LGTM. Please merge from webui-node-expansion and submit the request agian. Due to significant refactoring work in WebUI's underlying structure, we need to resolve some conflicts.

Resolved. And I also fixed many issues I found in the WebUI.
By the way, these two icons do not display correctly in dark mode.
image

@danielaskdd
Copy link
Collaborator

Thank again. There are indeed some issues with the WebUI, and some improvements have been made on the main branch. This has resulted in some new conflicts need to be resolved. Also, please take care of the conflicts in Dark mode while you're at it.

@danielaskdd
Copy link
Collaborator

Dark mode style for node expansion and prune icons is fixed.

@ArnoChenFx
Merge remote-tracking branch 'origin/main' into dev-webui
e8f4db3 
# Conflicts:
#	lightrag/api/webui/assets/index-BhS40EMK.js
#	lightrag/api/webui/assets/index-C56SCRGK.js
#	lightrag/api/webui/assets/index-T7hdp_6t.js
#	lightrag/api/webui/index.html
#	lightrag_webui/src/components/graph/GraphLabels.tsx
#	lightrag_webui/src/components/graph/Settings.tsx
#	lightrag_webui/src/components/graph/SettingsDisplay.tsx
#	lightrag_webui/src/hooks/useLightragGraph.tsx
@ArnoChenFx
Merge remote-tracking branch 'origin/main' into dev-webui
04e1c0d 
# Conflicts:
#	lightrag/api/webui/assets/index-4I5HV9Fr.js
#	lightrag/api/webui/assets/index-T7hdp_6t.js
#	lightrag/api/webui/assets/index-z38K1p1W.js
#	lightrag/api/webui/index.html
#	lightrag_webui/src/AppRouter.tsx
#	lightrag_webui/src/services/navigation.ts
@ArnoChenFx
Copy link
Contributor Author

Thank again. There are indeed some issues with the WebUI, and some improvements have been made on the main branch. This has resulted in some new conflicts need to be resolved. Also, please take care of the conflicts in Dark mode while you're at it.

Resolved

danielaskdd
danielaskdd requested changes Mar 20, 2025
View reviewed changes
Copy link
Collaborator

@danielaskdd danielaskdd left a comment

Choose a reason for hiding this comment

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

It seems that a lot of work has been done in this PR, thank you very much. During the review process, I discovered an issue - your modification has broken the working mode of the TAB pages. We require that TAB pages should be hidden through CSS, not controlled by React. The reason is that GraphView needs to maintain its state between TAB switches, and it shouldn't reload data and change layout every time you switch back. For this reason, Document and Pipeline states also need to control their timers based on the Current TAB in storage. Please revert back to the original TAB switching mode, thank you.

@ArnoChenFx
Copy link
Contributor Author

ArnoChenFx commented Mar 20, 2025 •
edited
Loading

It seems that a lot of work has been done in this PR, thank you very much. During the review process, I discovered an issue - your modification has broken the working mode of the TAB pages. We require that TAB pages should be hidden through CSS, not controlled by React. The reason is that GraphView needs to maintain its state between TAB switches, and it shouldn't reload data and change layout every time you switch back. For this reason, Document and Pipeline states also need to control their timers based on the Current TAB in storage. Please revert back to the original TAB switching mode, thank you.

Previous Tab cause a lot of issues in my local enviroment. I think it should be reimplement. This PR has brought the WebUI back to a stable state. Subsequent changes should not be made with the premise of breaking stability.

We already have GraphStore to store the GraphViewer state, and we should continue relying on GraphStore to restore the GraphViewer, rather than using the Tab magic.

@danielaskdd
Copy link
Collaborator

There is another minor issue: the application settings icon in the upper right corner duplicates the settings icon in the control bar at the lower left corner of the GraphViewer. This needs to be adjusted.

@ArnoChenFx
Copy link
Contributor Author

There is another minor issue: the application settings icon in the upper right corner duplicates the settings icon in the control bar at the lower left corner of the GraphViewer. This needs to be adjusted.

I think it's okay to use the same settings icon.

@ParisNeo
Copy link
Contributor

By the way i added more locales if you don't mind. I did not build yet. Can you please take that into consideration?

My twins are finally born. Yey!!. They are taking all of my time these days. Really sorry for intermittant participation.

@danielaskdd
Copy link
Collaborator

By the way i added more locales if you don't mind. I did not build yet. Can you please take that into consideration?

My twins are finally born. Yey!!. They are taking all of my time these days. Really sorry for intermittant participation.

The locales is merged, and language selection also works.

@danielaskdd
Copy link
Collaborator

There is another minor issue: the application settings icon in the upper right corner duplicates the settings icon in the control bar at the lower left corner of the GraphViewer. This needs to be adjusted.

I think it's okay to use the same settings icon.

I am satisfied with your decision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielaskdd danielaskdd danielaskdd requested changes

@ParisNeo ParisNeo ParisNeo approved these changes

Assignees
No one assigned
Labels
ui web ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArnoChenFx @danielaskdd @ParisNeo