-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
NIFI-13632: Updating content access. #9221
Conversation
mcgilman
commented
Aug 29, 2024
- Removing existing content viewer application.
- Introduced new front end based content viewer applicaiton.
- Introduced new bundled content viewers for hex and images.
- Deleted previous image content viewer.
- Migrated existing standard content viewer (which handles json, xml, yaml, csv, and text) to utilize new content access interface.
- Moved standard content viewer into its own NAR.
- Moved and renamed custom ui utils and content viewer utils into nifi-common.
- Added mime type to FlowFileSummary response payload to help drive the initially opened content viewer.
Reviewing from a front-end perspective... |
Thanks for the substantial work @mcgilman! I will review the server-side implementation soon. I did notice a build failure related to a missing reference in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good work, @mcgilman. I did leave some comments inline. Feel free to reach out if you want to discuss them further.
...nt-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.ts
Outdated
Show resolved
Hide resolved
...src/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.ts
Outdated
Show resolved
Hide resolved
...src/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.ts
Outdated
Show resolved
Hide resolved
...c/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.html
Show resolved
Hide resolved
...c/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.html
Show resolved
Hide resolved
...src/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.ts
Show resolved
Hide resolved
...nt-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.ts
Show resolved
Hide resolved
...c/main/frontend/apps/nifi/src/app/pages/content-viewer/feature/content-viewer.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks solid from a server implementation perspective, just noted two questions regarding the Spring Boot version and the configuration file.
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-content-viewer/pom.xml
Outdated
Show resolved
Hide resolved
...bundles/nifi-standard-bundle/nifi-standard-content-viewer/src/main/resources/application.yml
Outdated
Show resolved
Hide resolved
Testing this out a bit. Making great progress. I tried a scenario with GenerateFlowFile to a python script. The python script writes out flowfile content that is just "1.25.0". The flowfile has only filename, path, and uuid attributes set. I then route to updateAttribute that is stopped. I then list queue and view content for one of these. It shows up by default in a hex editor. That looks good. Then when I select view-text it shows a blank/white screen. If I go back to hex editor it looks good. Then back to text and it looks good. So something funky there but perhaps related to flowfiles with minimal attributes. Also notable in that same panel if I click around to csv it will sometimes give the white screen and sometimes render. |
- Removing existing content viewer application. - Introduced new front end based content viewer applicaiton. - Introduced new bundled content viewers for hex and images. - Deleted previous image content viewer. - Migrated existing standard content viewer (which handles json, xml, yaml, csv, and text) to utilize new content access interface. - Moved standard content viewer into its own NAR. - Moved and renamed custom ui utils and content viewer utils into nifi-common. - Added mime type to FlowFileSummary response payload to help drive the initially opened content viewer.
...-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.scss
Outdated
Show resolved
Hide resolved
...-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.scss
Outdated
Show resolved
Hide resolved
...-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.html
Outdated
Show resolved
Hide resolved
additional testing after a hard refresh the white screen i described still happens but it is very rare. in a local docker deployment it seems a bit easier to reproduce. in local mode without docker it is very hard to reproduce. it happened once in maybe 50 attempts. So in short - i dont think it is worth bothering with at this point. might be a local issue/hard refresh certainly seems to have made the issue less likely. thanks |
...-viewer/src/app/pages/standard-content-viewer/feature/standard-content-viewer.component.html
Outdated
Show resolved
Hide resolved
- Rendering mime type in the content viewer. - Preventing additions to the browser history as the user changes how the content is viewed. - Appending forward slash to content viewer UIs to prevent unnecessary redirect. - Removing Standard Content Viewer title based on review feedback. - Fixing double scroll bar in the Standard Content Viewer on small browser heights. - Removed unnecessary application.yml. - Bumping spring boot version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great from a UI-perspective. Thanks for the hard work @mcgilman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest changes work well. Thanks @mcgilman!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the work on this @mcgilman, looks good!