Conversation
WalkthroughThis update renames the application from "FLASHViewer" to "FLASHApp" across configuration, documentation, and Docker build files, including updating the GitHub user and repository references. It introduces persistent caching of layout settings with a new "side-by-side" view flag in both FLASHDeconv and FLASHTnT modules, enabling saving, loading, and editing of layouts across sessions. The viewer components are refactored to use these cached layouts and side-by-side settings, supporting flexible multi-experiment and side-by-side views with updated UI controls and state management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant LayoutManager
participant FileManager
User->>UI: Open layout manager
UI->>LayoutManager: Initialize
LayoutManager->>FileManager: get_layout()
alt Layout exists
FileManager-->>LayoutManager: Return layout and side_by_side flag
LayoutManager-->>UI: Load layout, set edit_mode = False
else No layout
LayoutManager-->>UI: Reset to default, set edit_mode = True
end
User->>UI: Edit layout, toggle side-by-side
UI->>LayoutManager: set_layout(new_layout, side_by_side)
LayoutManager->>FileManager: Save layout and flag
User->>UI: Save layout
UI->>LayoutManager: get_layout()
LayoutManager->>FileManager: Retrieve layout and flag
LayoutManager-->>UI: Provide layout for experiment selection
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 4
♻️ Duplicate comments (1)
content/FLASHDeconv/FLASHDeconvViewer.py (1)
183-205: Constant f‑strings & possibleNoneindexDuplicate of the Streamlit key / index issue flagged in the Tagger viewer.
Please apply the same clean‑up here to pass Ruff F541 and avoid runtime errors.🧰 Tools
🪛 Ruff (0.8.2)
198-198: f-string without any placeholders
Remove extraneous
fprefix(F541)
199-199: f-string without any placeholders
Remove extraneous
fprefix(F541)
199-199: f-string without any placeholders
Remove extraneous
fprefix(F541)
202-202: f-string without any placeholders
Remove extraneous
fprefix(F541)
🧹 Nitpick comments (9)
README.md (1)
3-3: Remove unnecessary backslash in description
The trailing backslash (\) used to join lines in Markdown is confusing. Consider merging the lines or replacing the backslash with proper punctuation.Proposed change:
- FLASHApp for visualizing FLASHDeconv's results \ + FLASHApp for visualizing FLASHDeconv's results. This app is based on [OpenMS streamlit template project](https://github.com/OpenMS/streamlit-template).🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing comma found.
Context: ... FLASHApp for visualizing FLASHDeconv's results \ This app is based on [OpenMS streamli...(AI_HYDRA_LEO_MISSING_COMMA)
content/FLASHTnT/FLASHTnTLayoutManager.py (1)
161-171: Repeatedget_layout()calls waste I/O and can race with external writes
get_layout()hits the cache (potentially disk) twice in the same handler:
once to obtain the experiment count and again for the comprehension below.
Cache the tuple once and reuse it – this shaves redundant work and ensures the
two reads stay consistent even if another thread/process overwrites the cache
between the calls.- st.session_state["num_of_experiment_to_show"] = len(get_layout()[0]) if get_layout() is not None else 1 - st.session_state["layout_setting_tagger"] = [[[COMPONENT_OPTIONS[COMPONENT_NAMES.index(col)] - for col in row if col] - for row in exp if row] - for exp in get_layout()[0]] + cached_layout = get_layout() + st.session_state["num_of_experiment_to_show"] = len(cached_layout[0]) if cached_layout else 1 + if cached_layout: + st.session_state["layout_setting_tagger"] = [ + [[COMPONENT_OPTIONS[COMPONENT_NAMES.index(col)] for col in row if col] + for row in exp if row] + for exp in cached_layout[0] + ]content/FLASHTnT/FLASHTnTViewer.py (2)
273-295: Static‑analysis warnings: constant f‑strings and possibleNoneindex
Keys like
f"selected_experiment_dropdown_1_tagger"contain no expression;
drop thefto silence Ruff F541.
index=expects anint; passingNoneraises a runtime error in recent
Streamlit versions. Guard withname_to_index.get(..., 0)or omit the
parameter when the key is absent.-key=f'selected_experiment_dropdown_1_tagger', -index = name_to_index[st.session_state[f'selected_experiment1_tagger']] if f'selected_experiment1_tagger' in st.session_state else None, +key='selected_experiment_dropdown_1_tagger', +index=name_to_index.get(st.session_state.get('selected_experiment1_tagger', results[0]), 0),🧰 Tools
🪛 Ruff (0.8.2)
288-288: f-string without any placeholders
Remove extraneous
fprefix(F541)
289-289: f-string without any placeholders
Remove extraneous
fprefix(F541)
289-289: f-string without any placeholders
Remove extraneous
fprefix(F541)
292-292: f-string without any placeholders
Remove extraneous
fprefix(F541)
312-327: Unused loop variable and micro‑style issues
exp_layoutis never referenced inside the loop, triggering Ruff B007. Rename
to_and move thecontinueonto its own line to clear the E701 diagnostic.-for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment +for exp_index, _ in enumerate(layout): + if exp_index == 0: + continue # skip the first experiment🧰 Tools
🪛 Ruff (0.8.2)
312-312: Loop control variable
exp_layoutnot used within loop bodyRename unused
exp_layoutto_exp_layout(B007)
313-313: Multiple statements on one line (colon)
(E701)
content/FLASHDeconv/FLASHDeconvViewer.py (2)
166-174: Same variable shadowing as in FLASHTnTViewerRe‑use of
layoutfor two different data types hampers readability and static
checking. Apply the same renaming strategy suggested in the Tagger viewer.
223-238: Loop variable not used & single‑linecontinueSilence Ruff B007/E701 as shown below:
-for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment +for exp_index, _ in enumerate(layout): + if exp_index == 0: + continue # skip the first experiment🧰 Tools
🪛 Ruff (0.8.2)
223-223: Loop control variable
exp_layoutnot used within loop bodyRename unused
exp_layoutto_exp_layout(B007)
224-224: Multiple statements on one line (colon)
(E701)
content/FLASHDeconv/FLASHDeconvLayoutManager.py (3)
172-179: Avoid repeated disk I/O – cache the loaded layout once per run
get_layout()is called three times inside this short block (and again later in display logic), each hitting the FileManager and re‑parsing JSON.
Store the result in a local variable and reuse it; this will cut down unnecessary reads and simplify the code.Example sketch:
cached_layout = get_layout() if cached_layout is not None: layout_data, side_by_side_flag = cached_layout ...
57-60: Unused loop variable (index) picked up by Ruff
for index in range(1, num_of_exp):– the variable is never referenced.
Rename to_or drop the enumerator altogether:-for index in range(1, num_of_exp): +for _ in range(1, num_of_exp):🧰 Tools
🪛 Ruff (0.8.2)
59-59: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
207-214:COMPONENT_OPTIONSgrows on every rerun
setSequenceView()appends items unconditionally. When Streamlit re‑runs the script (e.g., after widget interaction) the new options are appended again, leading to duplicates.Guard with a membership check or convert to a
setfirst.if 'Sequence view (Mass table needed)' not in COMPONENT_OPTIONS: COMPONENT_OPTIONS += [...]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)Dockerfile(1 hunks)README.md(4 hunks)content/FLASHDeconv/FLASHDeconvLayoutManager.py(6 hunks)content/FLASHDeconv/FLASHDeconvViewer.py(3 hunks)content/FLASHTnT/FLASHTnTLayoutManager.py(6 hunks)content/FLASHTnT/FLASHTnTViewer.py(3 hunks)settings.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
content/FLASHTnT/FLASHTnTViewer.py (2)
src/workflow/FileManager.py (2)
result_exists(447-465)get_results(403-445)content/FLASHDeconv/FLASHDeconvViewer.py (2)
select_experiment(141-147)sendDataToJS(19-119)
content/FLASHTnT/FLASHTnTLayoutManager.py (3)
src/common/common.py (3)
page_setup(91-228)v_space(300-315)save_params(59-88)src/workflow/FileManager.py (5)
FileManager(14-496)store_data(308-323)result_exists(447-465)get_results(403-445)remove_results(467-480)content/FLASHDeconv/FLASHDeconvLayoutManager.py (8)
set_layout(39-45)get_layout(47-54)resetSettingsToDefault(56-63)getExpandedLayoutSetting(154-167)handleEditAndSaveButtons(169-187)validateSubmittedLayout(117-136)getTrimmedLayoutSetting(139-152)handleSettingButtons(190-204)
🪛 LanguageTool
README.md
[uncategorized] ~3-~3: Possible missing comma found.
Context: ... FLASHApp for visualizing FLASHDeconv's results \ This app is based on [OpenMS streamli...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...: docker image ls After it has been built you can run the image with: `docker ru...
(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~55-~55: Did you mean the noun “width” (= size)?
Context: ...it has been built you can run the image with: `docker run -p 8501:8501 flashapp:lat...
(WITH_WIDTH)
🪛 Ruff (0.8.2)
content/FLASHDeconv/FLASHDeconvViewer.py
198-198: f-string without any placeholders
Remove extraneous f prefix
(F541)
199-199: f-string without any placeholders
Remove extraneous f prefix
(F541)
199-199: f-string without any placeholders
Remove extraneous f prefix
(F541)
202-202: f-string without any placeholders
Remove extraneous f prefix
(F541)
223-223: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
224-224: Multiple statements on one line (colon)
(E701)
content/FLASHDeconv/FLASHDeconvLayoutManager.py
59-59: Loop control variable index not used within loop body
Rename unused index to _index
(B007)
content/FLASHTnT/FLASHTnTViewer.py
288-288: f-string without any placeholders
Remove extraneous f prefix
(F541)
289-289: f-string without any placeholders
Remove extraneous f prefix
(F541)
289-289: f-string without any placeholders
Remove extraneous f prefix
(F541)
292-292: f-string without any placeholders
Remove extraneous f prefix
(F541)
312-312: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
313-313: Multiple statements on one line (colon)
(E701)
content/FLASHTnT/FLASHTnTLayoutManager.py
51-51: Loop control variable index not used within loop body
Rename unused index to _index
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (9)
.gitignore (1)
9-9: Update project reference in .gitignore
The comment has been correctly updated fromFLASHViewertoFLASHApp, ensuring consistency with the renamed project.settings.json (1)
3-4: Synchronize GitHub user and repository fields
Both"github-user"and"repository-name"have been updated toOpenMSandFLASHApp, respectively, aligning with the repository rename. The JSON remains valid and free of trailing commas.README.md (5)
1-1: Rename title to FLASHApp
The main heading has been updated to# FLASHApp, matching the new project name.
31-31: Update build instructions for FLASHApp
The build step now referencesFLASHApp, reflecting the renamed application.
40-40: Clarify prerequisite steps reference
The instruction “These steps should be done before building any version of FLASHApp.” correctly guides the user to perform the renaming-related steps upfront.
49-49: Correct Docker build command
The Docker build command has been updated to tag the image asflashapp:latest, matching the new repository name.
57-57: Correct Docker run command
The Docker run instruction now usesflashapp:latestfor consistency with the renamed image tag.Dockerfile (1)
37-39: Update Docker ARGs for GitHub context
TheGITHUB_USERandGITHUB_REPObuild arguments now correctly referenceOpenMSandFLASHApp, aligning the Docker build with the renamed repository.content/FLASHDeconv/FLASHDeconvLayoutManager.py (1)
33-37: Ensure cache directory exists before use
FileManageris instantiated withPath(..., 'cache'), but the directory may not exist on first run.
IfFileManagerdoesn’t create it internally, anOSErrorwill surface when attempting to write.
Consider callingmkdir(parents=True, exist_ok=True)on the path during setup.
| def resetSettingsToDefault(num_of_exp=1): | ||
| st.session_state["layout_setting_tagger"] = [[['']]] # 1D: experiment, 2D: row, 3D: column, element=component name | ||
| st.session_state["num_of_experiment_to_show_tagger"] = num_of_exp | ||
| for index in range(1, num_of_exp): | ||
| st.session_state.layout_setting_tagger.append([['']]) | ||
| if file_manager.result_exists('layout', 'layout'): | ||
| file_manager.remove_results('layout') | ||
| st.session_state["edit_mode"] = True |
There was a problem hiding this comment.
Mismatch in session_state key prevents the UI from reflecting the real experiment count
resetSettingsToDefault() writes the count to num_of_experiment_to_show_tagger,
while every other place (select‑box, edit/save handlers, etc.) expects the value
under num_of_experiment_to_show.
As a consequence, the select‑box will always fallback to the default value and the
subsequent logic (e.g. layout size checks) drifts out of sync.
A minimal, local fix:
- st.session_state["num_of_experiment_to_show_tagger"] = num_of_exp
+ st.session_state["num_of_experiment_to_show"] = num_of_expWhile touching the loop, silence the Ruff warning by renaming the unused
variable:
-for index in range(1, num_of_exp):
- st.session_state.layout_setting_tagger.append([['']])
+for _ in range(1, num_of_exp):
+ st.session_state.layout_setting_tagger.append([['']])📝 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.
| def resetSettingsToDefault(num_of_exp=1): | |
| st.session_state["layout_setting_tagger"] = [[['']]] # 1D: experiment, 2D: row, 3D: column, element=component name | |
| st.session_state["num_of_experiment_to_show_tagger"] = num_of_exp | |
| for index in range(1, num_of_exp): | |
| st.session_state.layout_setting_tagger.append([['']]) | |
| if file_manager.result_exists('layout', 'layout'): | |
| file_manager.remove_results('layout') | |
| st.session_state["edit_mode"] = True | |
| def resetSettingsToDefault(num_of_exp=1): | |
| st.session_state["layout_setting_tagger"] = [[['']]] # 1D: experiment, 2D: row, 3D: column, element=component name | |
| st.session_state["num_of_experiment_to_show"] = num_of_exp | |
| for _ in range(1, num_of_exp): | |
| st.session_state.layout_setting_tagger.append([['']]) | |
| if file_manager.result_exists('layout', 'layout'): | |
| file_manager.remove_results('layout') | |
| st.session_state["edit_mode"] = True |
🧰 Tools
🪛 Ruff (0.8.2)
51-51: Loop control variable index not used within loop body
Rename unused index to _index
(B007)
| if file_manager.result_exists('layout', 'layout'): | ||
| layout = file_manager.get_results('layout', 'layout')['layout'] | ||
| side_by_side = layout['side_by_side'] | ||
| layout = layout['layout'] | ||
|
|
||
| else: | ||
| layout = [DEFAULT_LAYOUT] | ||
| side_by_side = False | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shadowing layout obscures intention and risks type errors
You first assign the dict returned from FileManager to layout, then
reuse the same variable to store the extracted list. Although it works, the
double assignment is confusing and makes type‑checking impossible.
Prefer a two‑step approach with descriptive names:
-cache_blob = file_manager.get_results('layout', 'layout')['layout']
-side_by_side = cache_blob['side_by_side']
-layout = cache_blob['layout']
+cached = file_manager.get_results('layout', 'layout')['layout']
+side_by_side = cached['side_by_side']
+layout = cached['layout']Committable suggestion skipped: line range outside the PR's diff.
| def get_layout(): | ||
| # Check if layout has been set | ||
| if not file_manager.result_exists('layout', 'layout'): | ||
| return None | ||
| # fetch layout from cache | ||
| layout = file_manager.get_results('layout', 'layout')['layout'] | ||
|
|
||
| return layout['layout'], layout['side_by_side'] | ||
|
|
There was a problem hiding this comment.
Broken return logic – will raise TypeError on first call
get_layout() extracts the inner layout list and then treats that list as a dict on the next line (layout['layout']).
When a saved layout exists this will immediately explode, preventing the whole page from loading.
def get_layout():
# Check if layout has been set
if not file_manager.result_exists('layout', 'layout'):
return None
# fetch layout from cache
- layout = file_manager.get_results('layout', 'layout')['layout']
-
- return layout['layout'], layout['side_by_side']
+ data = file_manager.get_results('layout', 'layout')
+ return data['layout'], data['side_by_side']📝 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.
| def get_layout(): | |
| # Check if layout has been set | |
| if not file_manager.result_exists('layout', 'layout'): | |
| return None | |
| # fetch layout from cache | |
| layout = file_manager.get_results('layout', 'layout')['layout'] | |
| return layout['layout'], layout['side_by_side'] | |
| def get_layout(): | |
| # Check if layout has been set | |
| if not file_manager.result_exists('layout', 'layout'): | |
| return None | |
| # fetch layout from cache | |
| data = file_manager.get_results('layout', 'layout') | |
| return data['layout'], data['side_by_side'] |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
content/FLASHTnT/FLASHTnTLayoutManager.py (1)
279-287:⚠️ Potential issueAdd error handling for
get_layout()calls.Multiple calls to
get_layout()are made without checking if the result isNonefirst, which could lead to errors.-if (not st.session_state.edit_mode) and (get_layout() is not None): +if (not st.session_state.edit_mode): + layout_data = get_layout() + if layout_data is not None: + # show saved-mode + layout_settings, _ = layout_data + for index_of_experiment in range(len(layout_settings)): + layout_info_per_experiment = layout_settings[index_of_experiment] - # show saved-mode - for index_of_experiment in range(len(get_layout()[0])): - layout_info_per_experiment = get_layout()[0][index_of_experiment]🧰 Tools
🪛 Ruff (0.8.2)
284-284: Loop control variable
row_indexnot used within loop bodyRename unused
row_indexto_row_index(B007)
♻️ Duplicate comments (1)
content/FLASHTnT/FLASHTnTLayoutManager.py (1)
48-55:⚠️ Potential issueMismatch in
session_statekey prevents the UI from reflecting the real experiment count.
resetSettingsToDefault()writes the count tonum_of_experiment_to_show_tagger, while every other place (select‑box, edit/save handlers, etc.) expects the value undernum_of_experiment_to_show. This will cause inconsistencies in the UI.- st.session_state["num_of_experiment_to_show_tagger"] = num_of_exp + st.session_state["num_of_experiment_to_show"] = num_of_exp🧰 Tools
🪛 Ruff (0.8.2)
51-51: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
🧹 Nitpick comments (4)
content/FLASHTnT/FLASHTnTLayoutManager.py (4)
51-52: Fix unused loop variable warning.The loop control variable
indexis not used within the loop body. This triggers a static analysis warning.-for index in range(1, num_of_exp): +for _ in range(1, num_of_exp): st.session_state.layout_setting_tagger.append([['']])🧰 Tools
🪛 Ruff (0.8.2)
51-51: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
240-254: Improve side-by-side view condition logic for better readability.The condition for showing side-by-side view option is complex and could be structured more clearly. Consider extracting this logic to a helper function.
-if ( - ('num_of_experiment_to_show' in st.session_state - and st.session_state.num_of_experiment_to_show == 2) - or - (not st.session_state.edit_mode - and (get_layout() is not None and len(get_layout()[0]) == 2)) -): +def should_show_side_by_side_option(): + """Determine if side-by-side view option should be shown.""" + edit_mode_with_two_exps = ( + 'num_of_experiment_to_show' in st.session_state and + st.session_state.num_of_experiment_to_show == 2 + ) + + saved_mode_with_two_exps = ( + not st.session_state.edit_mode and + get_layout() is not None and + len(get_layout()[0]) == 2 + ) + + return edit_mode_with_two_exps or saved_mode_with_two_exps + +if should_show_side_by_side_option():
264-267: Update the JSON file name to match the renamed application.The JSON filename still uses "FLASHViewer" while the PR indicates the application is being renamed to "FLASHApp".
- file_name='FLASHViewer_layout_settings.json', + file_name='FLASHApp_layout_settings.json',
53-55: Improve cache handling with more specific error handling.The code for removing layout cache doesn't include any error handling for cases where the file operation might fail.
- if file_manager.result_exists('layout', 'layout'): - file_manager.remove_results('layout') + try: + if file_manager.result_exists('layout', 'layout'): + file_manager.remove_results('layout') + except Exception as e: + st.error(f"Failed to clear cached layout: {str(e)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
content/FLASHTnT/FLASHTnTLayoutManager.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
content/FLASHTnT/FLASHTnTLayoutManager.py
51-51: Loop control variable index not used within loop body
Rename unused index to _index
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (5)
content/FLASHTnT/FLASHTnTLayoutManager.py (5)
25-29: LGTM on the file manager setup.The implementation of persistent caching through the FileManager is well-structured and properly configured with workspace paths.
31-47: Well-implemented layout persistence functions.The
set_layoutandget_layoutfunctions provide a clear API for storing and retrieving layout configurations with the side-by-side flag.
146-159: Good implementation of expanded layout settings.The
getExpandedLayoutSettingfunction provides a nice symmetry withgetTrimmedLayoutSettingand clearly handles the conversion between component options and names.
299-301: Well-implemented edit/save button states.The disabled states for Edit and Save buttons are properly tied to the
edit_modeflag, ensuring a good user experience.
224-226:⚠️ Potential issuePotential
AttributeErroron cold start.When accessing
st.session_state.layout_setting_tagger, make sure to handle the case where it might not exist yet, especially if the cache was just loaded.- st.session_state['num_of_experiment_to_show'] = len(st.session_state.layout_setting_tagger) + st.session_state['num_of_experiment_to_show'] = len(st.session_state['layout_setting_tagger'])Likely an incorrect or invalid review comment.
| # side-by-side view option for 2 experiments | ||
| if 'side_by_side_view' not in st.session_state: | ||
| st.session_state['side_by_side_view'] = False | ||
| if ( | ||
| ('num_of_experiment_to_show' in st.session_state | ||
| and st.session_state.num_of_experiment_to_show == 2) | ||
| or | ||
| (not st.session_state.edit_mode | ||
| and (get_layout() is not None and len(get_layout()[0]) == 2)) | ||
| ): | ||
| v_space(1, c2) | ||
| st.session_state['side_by_side_view'] = c2.checkbox( | ||
| "Side-by-Side View", value=st.session_state['side_by_side_view'], | ||
| help="If checked, experiments will be shown side-by-side", | ||
| disabled=(not st.session_state.edit_mode) | ||
| ) |
There was a problem hiding this comment.
Add protection against KeyError when accessing st.session_state.edit_mode.
The code checks not st.session_state.edit_mode but there's no validation that this key exists in the session state at this point in the code.
- or
- (not st.session_state.edit_mode
+ or
+ ('edit_mode' in st.session_state and not st.session_state.edit_mode📝 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.
| # side-by-side view option for 2 experiments | |
| if 'side_by_side_view' not in st.session_state: | |
| st.session_state['side_by_side_view'] = False | |
| if ( | |
| ('num_of_experiment_to_show' in st.session_state | |
| and st.session_state.num_of_experiment_to_show == 2) | |
| or | |
| (not st.session_state.edit_mode | |
| and (get_layout() is not None and len(get_layout()[0]) == 2)) | |
| ): | |
| v_space(1, c2) | |
| st.session_state['side_by_side_view'] = c2.checkbox( | |
| "Side-by-Side View", value=st.session_state['side_by_side_view'], | |
| help="If checked, experiments will be shown side-by-side", | |
| disabled=(not st.session_state.edit_mode) | |
| ) | |
| # side-by-side view option for 2 experiments | |
| if 'side_by_side_view' not in st.session_state: | |
| st.session_state['side_by_side_view'] = False | |
| if ( | |
| ('num_of_experiment_to_show' in st.session_state | |
| and st.session_state.num_of_experiment_to_show == 2) | |
| or | |
| ('edit_mode' in st.session_state and not st.session_state.edit_mode | |
| and (get_layout() is not None and len(get_layout()[0]) == 2)) | |
| ): | |
| v_space(1, c2) | |
| st.session_state['side_by_side_view'] = c2.checkbox( | |
| "Side-by-Side View", value=st.session_state['side_by_side_view'], | |
| help="If checked, experiments will be shown side-by-side", | |
| disabled=(not st.session_state.edit_mode) | |
| ) |
| st.session_state["num_of_experiment_to_show"] = len(get_layout()[0]) if get_layout() is not None else 1 | ||
| st.session_state["layout_setting_tagger"] = [[[COMPONENT_OPTIONS[COMPONENT_NAMES.index(col)] | ||
| for col in row if col] | ||
| for row in exp if row] | ||
| for exp in st.session_state.saved_layout_setting_tagger] | ||
| # remove saved state, if any | ||
| del st.session_state["saved_layout_setting_tagger"] | ||
| for exp in get_layout()[0]] |
There was a problem hiding this comment.
Potential AttributeError when get_layout() returns None.
If get_layout() returns None, the code will still attempt to index into the result with [0], which will cause an AttributeError.
- st.session_state["num_of_experiment_to_show"] = len(get_layout()[0]) if get_layout() is not None else 1
+ layout_data = get_layout()
+ st.session_state["num_of_experiment_to_show"] = len(layout_data[0]) if layout_data is not None else 1The same issue applies to the subsequent line where get_layout()[0] is accessed directly.
Committable suggestion skipped: line range outside the PR's diff.
Clear captcha textbox if user enters wrong captcha
- Add is_safe_workspace_name() to reject names with path separators or parent directory references (/, \, .., .) - Apply validation in find_demo_workspace_path() to prevent demo name path traversal attacks - Apply validation in page_setup() for workspace query parameter, falling back to new UUID workspace if invalid Addresses Copilot security review comments #1 and #2.
1 participant
This PR implements the following changes:
It also updates everything to work with new repository location.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores