Fix: Display WebApp Versioning #150
Conversation
--Updated app.py to display the app name from settings.json. --Updated common.py to display the version in the web UI from settings.py.
WalkthroughThe update introduces a mechanism to load configuration settings from a JSON file into the Streamlit session state. In Changes
Sequence Diagram(s)sequenceDiagram
participant App as Streamlit App
participant SS as Session State
participant File as "settings.json" File
App->>SS: Check for "settings" key
alt Settings Missing
App->>File: Read settings.json
File-->>App: Return settings data
App->>SS: Store settings data
end
App->>App: Use st.session_state.settings["app-name"] for pages
sequenceDiagram
participant Sidebar as Application Sidebar
participant SS as Session State
participant UI as UI Container/Markdown
Sidebar->>SS: Retrieve "version" key
SS-->>Sidebar: Return version value
Sidebar->>UI: Display styled markdown with version info
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/common/common.py (1)
103-105: 🛠️ Refactor suggestionDuplicate settings loading code
The code to load settings from settings.json appears in both
app.pyand here. This creates duplication and potential for inconsistency.Consider refactoring to have a single function that loads settings from the JSON file:
# In a new file, e.g., src/common/settings.py def load_settings(): try: with open("settings.json", "r") as f: return json.load(f) except FileNotFoundError: default_settings = {"app-name": "OpenMS Web App", "version": "Unknown"} st.error("settings.json file not found. Using default settings.") return default_settings except json.JSONDecodeError: default_settings = {"app-name": "OpenMS Web App", "version": "Unknown"} st.error("settings.json contains invalid JSON. Using default settings.") return default_settings # Then in common.py and app.py: if "settings" not in st.session_state: from src.common.settings import load_settings st.session_state.settings = load_settings()This way, changes to the loading logic only need to be made in one place.
🧹 Nitpick comments (2)
app.py (1)
5-5:Details
❓ Verification inconclusive
Remove unused import
The static analysis tool has flagged that
pyopenmsis imported but not used.# For some reason the windows version only works if this is imported here -import pyopenmsHowever, the comment suggests this import is necessary for Windows compatibility, so verification is needed before removal.
🏁 Script executed:
#!/bin/bash # Check if pyopenms is used in this file or mentioned in other files with a reference to its usage echo "Checking if pyopenms is used:" rg -A 5 -B 5 -p "pyopenms.*import.*windows" --glob "*.py" --glob "*.md"Length of output: 142
I'll now further verify the usage of
pyopenmsacross the repository. Let's run a more comprehensive search.
🏁 Script executed:
#!/bin/bash echo "Searching for any instance of 'pyopenms' usage:" rg -n "pyopenms" --glob "*.py"Length of output: 724
Unused Import in app.py: Verify Windows Compatibility Before Removal
The static analysis confirms that
pyopenmsis imported inapp.pyat line 5 but not used directly in that file. While our repository search shows thatpyopenmsis used in several other modules (e.g., in files undersrc/and elsewhere), we did not find any evidence inapp.pyitself that indicates a side-effect-driven dependency for the Windows version. Given the existing comment—“for some reason the windows version only works if this is imported here”—please verify on a Windows environment that removing this import does not adversely affect functionality before making any changes.
- Location:
app.py, line 5- Action: Manually confirm Windows compatibility without the
pyopenmsimport or provide an additional comment clarifying its necessity if retained.🧰 Tools
🪛 Ruff (0.8.2)
5-5:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
src/common/common.py (1)
326-342: Add error handling for version display and improve stylingThe added code to display version information is a good enhancement, but there are a few improvements that could be made:
- Missing error handling if the "version" key doesn't exist in settings
- The styling is embedded directly in the code rather than in a separate CSS file
- The HTML is directly injected without proper escaping
Here's an improved implementation:
# Display OpenMS WebApp Template Version from settings.json with st.container(): st.markdown( """ <style> .version-box { border: 1px solid #a4a5ad; padding: 10px; border-radius: 0.5rem; } </style> """, unsafe_allow_html=True ) - version_info = st.session_state.settings["version"] - st.markdown(f'<div class="version-box"><b>OpenMS WebApp:<b/> V{version_info}</div>', unsafe_allow_html=True) + try: + version_info = st.session_state.settings.get("version", "Unknown") + st.markdown(f'<div class="version-box"><b>OpenMS WebApp:</b> V{version_info}</div>', unsafe_allow_html=True) + except Exception as e: + st.markdown('<div class="version-box"><b>OpenMS WebApp:</b> Version Unknown</div>', unsafe_allow_html=True) + st.error(f"Error retrieving version information: {str(e)}")Also note that there's a small HTML error in the original code:
<b>OpenMS WebApp:<b/>should be<b>OpenMS WebApp:</b>(closing tag uses the wrong slash direction).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.py(1 hunks)settings.json(1 hunks)src/common/common.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- settings.json
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
5-5: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🔇 Additional comments (3)
app.py (2)
3-3: Added necessary json importThis is appropriately added to support reading from the settings.json file.
13-13: Correctly implemented dynamic application nameGood job replacing the hardcoded application name with a dynamic reference from the settings.
src/common/common.py (1)
340-341: Verify version display rendering in various browsersThe version display uses custom CSS styling which may render differently across browsers. Ensure testing is done with different browsers to confirm consistent appearance.
Also, verify that the version string from settings.json is always a string format that will render properly when inserted into HTML.
| if "settings" not in st.session_state: | ||
| with open("settings.json", "r") as f: | ||
| st.session_state.settings = json.load(f) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling needed for settings.json loading
The code adds functionality to load settings from a JSON file, but doesn't include error handling for cases where the file might be missing or contain invalid JSON.
Add error handling to gracefully manage file access or JSON parsing errors:
if "settings" not in st.session_state:
- with open("settings.json", "r") as f:
- st.session_state.settings = json.load(f)
+ try:
+ with open("settings.json", "r") as f:
+ st.session_state.settings = json.load(f)
+ except FileNotFoundError:
+ st.error("settings.json file not found. Please create this file with required settings.")
+ st.session_state.settings = {"app-name": "OpenMS Web App", "version": "Unknown"}
+ except json.JSONDecodeError:
+ st.error("settings.json contains invalid JSON. Please check the file format.")
+ st.session_state.settings = {"app-name": "OpenMS Web App", "version": "Unknown"}Also, the indentation is inconsistent (8 spaces instead of 4), which differs from the rest of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "settings" not in st.session_state: | |
| with open("settings.json", "r") as f: | |
| st.session_state.settings = json.load(f) | |
| if "settings" not in st.session_state: | |
| try: | |
| with open("settings.json", "r") as f: | |
| st.session_state.settings = json.load(f) | |
| except FileNotFoundError: | |
| st.error("settings.json file not found. Please create this file with required settings.") | |
| st.session_state.settings = {"app-name": "OpenMS Web App", "version": "Unknown"} | |
| except json.JSONDecodeError: | |
| st.error("settings.json contains invalid JSON. Please check the file format.") | |
| st.session_state.settings = {"app-name": "OpenMS Web App", "version": "Unknown"} |
t0mdavid-m
left a comment
t0mdavid-m
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Once the minor change requests are incorporated I think it is ready to merge.
src/common/common.py
Outdated
| unsafe_allow_html=True | ||
| ) | ||
| version_info = st.session_state.settings["version"] | ||
| st.markdown(f'<div class="version-box"><b>OpenMS WebApp:<b/> V{version_info}</div>', unsafe_allow_html=True) |
There was a problem hiding this comment.
Could we either remove the OpenMS WebApp: prefix here or replace it by the App name?
There was a problem hiding this comment.
Yes but app name is "OpenMS WebApp Template", @jcharkow mention it's very lengthy, so we can have OpenMS WebApp, so I have hard coded it
There was a problem hiding this comment.
I see - sorry I missed that as its in the other PR.
@jcharkow I would suggest to display the version of the concrete WebApp instead of the version of the template used as this will likely have a greater effect on the WebApps output data and thus more relevant to the user. What do you think?
There was a problem hiding this comment.
@t0mdavid-m @jcharkow , you have discussed this already in #91, this PR is just to show the version info from the settings.json
We would implement and close the issue #91, so that whatever version in the settings.json will be displayed in the UI
Implemented text centering for improved visual presentation within the designated version.
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Looks good to me. After @t0mdavid-m review I think we can merge
|
One more thing I would like to have addressed: Hence, I request to display the following: {App name from settings.json} The newline should resolve any issues regarding length. Sorry for the back-and-forth @asterisk-ragavan! |
@t0mdavid-m no sorry please, it's a great opportunity to work with you, and I am even learning new things. post commit: done with the changes, added an image of how it looks on the UI |
{App name from settings.json}
Version {Version number from settings.json}
|
Thanks a lot for your contribution! I just removed the V before the version number as we already have the "Version: " prefix. Merging now! |
3 participants
Update app name and version display from settings #140
Updated app.py to display the app name from settings.json.
Updated common.py to display the version in the web UI from settings.py.
Testing
Summary by CodeRabbit