Add workspace management features and disable workspace setting #136
Conversation
WalkthroughThe pull request introduces a new configuration option and adds conditional logic to handle workspace functionalities. A new key Changes
Sequence Diagram(s)sequenceDiagram
participant P as page_setup
participant C as Config (settings.json)
participant Q as Query Params
participant U as UI Renderer
P->>C: Check "enable_workspaces"
alt Workspace Enabled
P->>Q: Retrieve existing or generate new workspace ID
P->>U: Render page with workspace-specific UI (including switcher)
else Workspace Disabled
P->>U: Render page with default workspace settings
end
Poem
✨ Finishing Touches
🪧 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: 5
🧹 Nitpick comments (2)
src/common/common.py (2)
267-314: Optimize conditional checks and fix nestedifstatement.Several improvements can be made to this section:
- The conditional check for workspace being enabled is appropriate here
- When checking workspace existence in the delete button handler, the nested if could be simplified
- In the
change_workspacefunction, the loop can be optimizedConsider these optimizations:
# Display workspace switcher if workspace is enabled in local mode if st.session_state.settings["is_workspace_enabled"]: with st.expander("🖥️ **Workspaces**"): # Define workspaces directory outside of repository workspaces_dir = Path("..", "workspaces-" + st.session_state.settings["repository-name"]) # Online: show current workspace name in info text and option to change to other existing workspace if st.session_state.location == "local": # Define callback function to change workspace def change_workspace(): - for key in params.keys(): - if key in st.session_state.keys(): + for key in params: + if key in st.session_state: del st.session_state[key] st.session_state.workspace = Path( workspaces_dir, st.session_state["chosen-workspace"] ) st.query_params.workspace = st.session_state["chosen-workspace"] # Get all available workspaces as options options = [ file.name for file in workspaces_dir.iterdir() if file.is_dir() ] # Let user chose an already existing workspace st.selectbox( "choose existing workspace", options, index=options.index(str(st.session_state.workspace.stem)), on_change=change_workspace, key="chosen-workspace", ) # Create or Remove workspaces create_remove = st.text_input("create/remove workspace", "") path = Path(workspaces_dir, create_remove) # Create new workspace if st.button("**Create Workspace**"): path.mkdir(parents=True, exist_ok=True) st.session_state.workspace = path st.query_params.workspace = create_remove # Temporary as the query update takes a short amount of time time.sleep(1) st.rerun() # Remove existing workspace and fall back to default if st.button("⚠️ Delete Workspace"): - if path.exists(): - shutil.rmtree(path) - st.session_state.workspace = Path(workspaces_dir, "default") - st.query_params.workspace = "default" - st.rerun() + # Only attempt deletion if the path exists + if not path.exists(): + return + + shutil.rmtree(path) + st.session_state.workspace = Path(workspaces_dir, "default") + st.query_params.workspace = "default" + st.rerun()🧰 Tools
🪛 Ruff (0.8.2)
276-276: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
277-277: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
308-309: Use a single
ifstatement instead of nestedifstatements(SIM102)
44-48: Consider adding logging when workspace features are disabled.There's no logging or notification when workspace operations are skipped due to the feature being disabled, which might make debugging harder.
Consider adding informative debug logs:
# Check if workspace is enabled. If not, load default parameters. if not st.session_state.settings["is_workspace_enabled"]: + st.debug.log("Workspace feature is disabled, loading default parameters.") default = TrueAnd similarly for the save_params function:
# Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory if not st.session_state.settings["is_workspace_enabled"]: + st.debug.log("Workspace feature is disabled, skipping parameter saving.") returnAlso applies to: 83-87
🧰 Tools
🪛 Ruff (0.8.2)
46-46: Use
st.session_state.settings["is_workspace_enabled"] != Trueinstead ofnot st.session_state.settings["is_workspace_enabled"] == TrueReplace with
!=operator(SIM201)
46-46: Avoid equality comparisons to
True; useif st.session_state.settings["is_workspace_enabled"]:for truth checksReplace with
st.session_state.settings["is_workspace_enabled"](E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
settings.json(1 hunks)src/common/common.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
46-46: Use st.session_state.settings["is_workspace_enabled"] != True instead of not st.session_state.settings["is_workspace_enabled"] == True
Replace with != operator
(SIM201)
46-46: Avoid equality comparisons to True; use if st.session_state.settings["is_workspace_enabled"]: for truth checks
Replace with st.session_state.settings["is_workspace_enabled"]
(E712)
85-85: Use st.session_state.settings["is_workspace_enabled"] != True instead of not st.session_state.settings["is_workspace_enabled"] == True
Replace with != operator
(SIM201)
85-85: Avoid equality comparisons to True; use if st.session_state.settings["is_workspace_enabled"]: for truth checks
Replace with st.session_state.settings["is_workspace_enabled"]
(E712)
213-213: Avoid equality comparisons to True; use if st.session_state.settings["is_workspace_enabled"]: for truth checks
Replace with st.session_state.settings["is_workspace_enabled"]
(E712)
232-232: Avoid equality comparisons to True; use if st.session_state.settings["is_workspace_enabled"]: for truth checks
Replace with st.session_state.settings["is_workspace_enabled"]
(E712)
276-276: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
277-277: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
308-309: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (1)
settings.json (1)
15-16: Configuration settings correctly implemented.The addition of
is_workspace_enabledwith a default value oftrueis a good approach, allowing for toggling workspace functionality while maintaining backward compatibility with existing implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/common/common.py (2)
45-47: 🛠️ Refactor suggestionAdd error handling for missing settings key.
The new code assumes that
st.session_state.settings["is_workspace_enabled"]always exists, but doesn't handle the case where this key might be missing (for backward compatibility).Create a helper function to handle this case consistently throughout the code:
def is_workspace_enabled(): """Check if workspace feature is enabled in settings.""" return st.session_state.settings.get("is_workspace_enabled", True)Then use it in this function:
# Check if workspace is enabled. If not, load default parameters. -if not st.session_state.settings["is_workspace_enabled"]: +if not is_workspace_enabled(): default = True
84-87: 🛠️ Refactor suggestionUse Pythonic condition and handle missing settings.
Similar issue as above with checking the setting, plus there's a return statement without a value.
# Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory -if not st.session_state.settings["is_workspace_enabled"]: - return +if not is_workspace_enabled(): + return paramsThis ensures the function always returns the parameters (consistent with the return type in the docstring), even when workspace is disabled.
🧹 Nitpick comments (2)
src/common/common.py (2)
277-279: Use more Pythonic key checks.The static analysis tool correctly identified that key lookups can be simplified.
def change_workspace(): - for key in params.keys(): - if key in st.session_state.keys(): + for key in params: + if key in st.session_state: del st.session_state[key]🧰 Tools
🪛 Ruff (0.8.2)
277-277: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
278-278: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
309-314: Simplify nested conditions.The nested if statements can be combined for better readability.
# Remove existing workspace and fall back to default -if st.button("⚠️ Delete Workspace"): - if path.exists(): +if st.button("⚠️ Delete Workspace") and path.exists(): shutil.rmtree(path) st.session_state.workspace = Path(workspaces_dir, "default") st.query_params.workspace = "default" st.rerun()🧰 Tools
🪛 Ruff (0.8.2)
309-310: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/common.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
277-277: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
278-278: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
309-310: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (4)
src/common/common.py (4)
212-226: Use consistent indentation and fix condition syntax.There are inconsistent indentation levels and non-Pythonic condition checks.
# Check if workspace logic is enabled -if st.session_state.settings["is_workspace_enabled"]: +if is_workspace_enabled(): if "workspace" in st.query_params: - st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) + st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) elif st.session_state.location == "online": - workspace_id = str(uuid.uuid1()) - st.session_state.workspace = Path(workspaces_dir, workspace_id) - st.query_params.workspace = workspace_id + workspace_id = str(uuid.uuid1()) + st.session_state.workspace = Path(workspaces_dir, workspace_id) + st.query_params.workspace = workspace_id else: st.session_state.workspace = Path(workspaces_dir, "default") st.query_params.workspace = "default"
232-234: Fix condition expression.The condition uses a non-Pythonic pattern for checking a boolean.
# If no workspace is specified and workspace feature is enabled, set default workspace and query param -if "workspace" not in st.query_params and st.session_state.settings["is_workspace_enabled"]: +if "workspace" not in st.query_params and is_workspace_enabled(): st.query_params.workspace = st.session_state.workspace.name
269-270: Use helper function for consistent settings check.Use the same helper function here for consistency.
# Display workspace switcher if workspace is enabled in local mode -if st.session_state.settings["is_workspace_enabled"]: +if is_workspace_enabled():
45-315:Details
✅ Verification successful
Verify all workspace features have been properly conditionalized.
This PR adds checks for workspace enablement in multiple places. Make sure that all workspace-related functionality is properly disabled when needed.
🏁 Script executed:
#!/bin/bash # Description: Find all references to workspace functionality in the code that might need conditional checks # Look for all places where workspace-related operations occur rg "workspace" "src/common/common.py" | grep -v "is_workspace_enabled" # Check if any other files might need similar workspace enablement checks rg "st\.session_state\.workspace" --type pyLength of output: 6506
Workspace conditionals validated – No further action required.
After reviewing the references in the code, the workspace-related functionality appears to be properly gated by the enablement flag. For example:
- In
load_params, whenst.session_state.settings["is_workspace_enabled"]is false the function defaults to loading parameters from the fallback file.- In
save_params, an early return is triggered when workspace functionality is disabled.- In
page_setup, a default workspace is assigned when the workspace feature is not enabled, ensuring that all downstream references (such as those in sidebar rendering and file-handling modules) work consistently.- Other modules (e.g., file upload and raw data viewers) rely on the initialization performed in
page_setupso that they always use a valid workspace path even if the feature is disabled.Please verify that modules outside of
src/common/common.pycontinue to rely solely on the default workspace (as set inpage_setup) when the workspace feature is off. Assuming that is the intended behavior, the conditional checks across the codebase are correct.🧰 Tools
🪛 Ruff (0.8.2)
90-90: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
151-151: Avoid equality comparisons to
True; useif ...:for truth checksReplace comparison
(E712)
169-169: Avoid equality comparisons to
True; useif st.session_state.tracking_consent["piwik-pro"]:for truth checksReplace with
st.session_state.tracking_consent["piwik-pro"](E712)
277-277: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
278-278: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
309-310: Use a single
ifstatement instead of nestedifstatements(SIM102)
|
@definitelynotchirag please resolve conflicts with the base branch. This can be done by merging the main branch into your branch locally and then pushing the changes. |
|
@jcharkow Thanks i have resolved the conflicts! |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/common/common.py (1)
98-98: 🛠️ Refactor suggestionEnsure consistent return value.
The function returns
paramshere, but there's an early return above when workspaces are disabled.To ensure consistency with the function signature and docstring, modify the early return:
# Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory if not st.session_state.settings.get("is_workspace_enabled", True): - return + return paramsThis ensures the function always returns the parameter dictionary, even when no operations are performed.
♻️ Duplicate comments (4)
src/common/common.py (4)
44-48:⚠️ Potential issueAdd error handling for missing settings key.
The new code assumes that
st.session_state.settings["is_workspace_enabled"]always exists, but doesn't handle cases where this key might be missing (for backward compatibility with older versions of the app).Add defensive code to handle this case:
# Check if workspace is enabled. If not, load default parameters. - if not st.session_state.settings["is_workspace_enabled"]: + if not st.session_state.settings.get("is_workspace_enabled", True): default = True
232-234: 🛠️ Refactor suggestionFix the condition expression syntax.
The condition compares directly to a key that might not exist. Add defensive handling for backward compatibility.
# If no workspace is specified and workspace feature is enabled, set default workspace and query param - if "workspace" not in st.query_params and st.session_state.settings["is_workspace_enabled"]: + if "workspace" not in st.query_params and st.session_state.settings.get("is_workspace_enabled", True): st.query_params.workspace = st.session_state.workspace.name
279-281: 🛠️ Refactor suggestionCreate a helper function for checking workspace enablement.
To ensure consistent handling of the workspace enablement check throughout the codebase, it would be better to extract this check into a helper function.
+def is_workspace_enabled(): + """ + Helper function to check if workspace functionality is enabled. + Returns True if enabled, False otherwise. + + Returns: + bool: Whether workspace functionality is enabled + """ + return st.session_state.settings.get("is_workspace_enabled", True) def render_sidebar(page: str = "") -> None: ... # Display workspace switcher if workspace is enabled in local mode - if st.session_state.settings["is_workspace_enabled"]: + if is_workspace_enabled():This function should then be used consistently throughout the code for all workspace enablement checks.
84-87:⚠️ Potential issueAdd error handling for missing settings key and address return type inconsistency.
Same issue with assuming the existence of the
is_workspace_enabledkey. Additionally, the function's docstring indicates it returns a dictionary, but when workspaces are disabled, it now returnsNone.Apply this change:
# Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory - if not st.session_state.settings["is_workspace_enabled"]: + if not st.session_state.settings.get("is_workspace_enabled", True): returnAlso, update the function's return type documentation to include the possibility of returning
None.
🧹 Nitpick comments (3)
src/common/common.py (3)
212-226: Fix indentation and simplify branching logic.There's an inconsistent indentation pattern at line 215, with an extra level of indentation that doesn't match the control flow.
Apply this diff to fix the indentation:
# Check if workspace logic is enabled if st.session_state.settings["is_workspace_enabled"]: if "workspace" in st.query_params: - st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) + st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) elif st.session_state.location == "online": - workspace_id = str(uuid.uuid1()) - st.session_state.workspace = Path(workspaces_dir, workspace_id) - st.query_params.workspace = workspace_id + workspace_id = str(uuid.uuid1()) + st.session_state.workspace = Path(workspaces_dir, workspace_id) + st.query_params.workspace = workspace_id else: st.session_state.workspace = Path(workspaces_dir, "default") st.query_params.workspace = "default"
288-290: Simplify dictionary access operations.The
.keys()method call is unnecessary when checking for key existence.def change_workspace(): - for key in params.keys(): - if key in st.session_state.keys(): + for key in params: + if key in st.session_state: del st.session_state[key]🧰 Tools
🪛 Ruff (0.8.2)
288-288: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
289-289: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
320-326: Simplify the nested if statements.The button's action is conditional upon the path existing. Consider combining these checks.
# Remove existing workspace and fall back to default - if st.button("⚠️ Delete Workspace"): - if path.exists(): + if st.button("⚠️ Delete Workspace") and path.exists(): shutil.rmtree(path) st.session_state.workspace = Path(workspaces_dir, "default") st.query_params.workspace = "default" st.rerun()🧰 Tools
🪛 Ruff (0.8.2)
320-321: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
settings.json(1 hunks)src/common/common.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- settings.json
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
288-288: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
289-289: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
320-321: Use a single if statement instead of nested if statements
(SIM102)
| @@ -75,6 +80,11 @@ def save_params(params: dict[str, Any]) -> None: | |||
| Returns: | |||
| dict[str, Any]: Updated parameters. | |||
| """ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix return type annotation in docstring.
The function save_params now returns either a dictionary or None, but the docstring only specifies dict[str, Any] as the return type.
Update the return type in the docstring to accurately reflect the possible return values:
Returns:
- dict[str, Any]: Updated parameters.
+ dict[str, Any] or None: Updated parameters, or None if workspace is disabled.📝 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.
| """ | |
| Returns: | |
| - dict[str, Any]: Updated parameters. | |
| + dict[str, Any] or None: Updated parameters, or None if workspace is disabled. |
t0mdavid-m
left a comment
t0mdavid-m
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This looks good to me. I have on minor revision request:
Could you rename the is_workspace_enabled option to enable_workspaces?. After this, the PR is ready to merge from my end.
|
Fixes #133 |
|
Sure I will rename it in a while!! |
|
Hello @t0mdavid-m I have renamed the option to "enable_workspaces". Please review it now :) |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/common/common.py (1)
82-86: 🛠️ Refactor suggestionUpdate docstring return type to match implementation.
The function can now return
Noneif workspace is disabled, but the docstring only specifiesdict[str, Any]as the return type.Returns: - dict[str, Any]: Updated parameters. + dict[str, Any] | None: Updated parameters, or None if workspace is disabled.Also, add error handling for the settings key access:
- # Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory - if not st.session_state.settings["enable_workspaces"]: + # Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory + if not st.session_state.settings.get("enable_workspaces", True): return
🧹 Nitpick comments (3)
src/common/common.py (3)
288-290: Optimize dictionary key check.The current implementation is using
key in params.keys()which is less efficient than directly checkingkey in params.- for key in params.keys(): - if key in st.session_state.keys(): + for key in params: + if key in st.session_state: del st.session_state[key]🧰 Tools
🪛 Ruff (0.8.2)
288-288: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
289-289: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
320-325: Simplify nested conditionals.The nested
ifstatements for deleting a workspace can be simplified by combining them.- # Remove existing workspace and fall back to default - if st.button("⚠️ Delete Workspace"): - if path.exists(): + # Remove existing workspace and fall back to default + if st.button("⚠️ Delete Workspace") and path.exists(): shutil.rmtree(path) st.session_state.workspace = Path(workspaces_dir, "default") st.query_params.workspace = "default" st.rerun()🧰 Tools
🪛 Ruff (0.8.2)
320-321: Use a single
ifstatement instead of nestedifstatements(SIM102)
85-86: Consider adding logging for silent return.The function now returns early without saving parameters when workspaces are disabled. Consider adding a debug log to indicate that parameter saving was skipped.
# Check if the workspace is enabled and if a 'params.json' file exists in the workspace directory if not st.session_state.settings.get("enable_workspaces", True): + if st.session_state.settings.get("debug", False): + print("Workspace feature disabled. Skipping parameter save.") return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
settings.json(1 hunks)src/common/common.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- settings.json
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
288-288: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
289-289: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
320-321: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (1)
src/common/common.py (1)
44-47:Details
✅ Verification successful
Verify consistent naming throughout the codebase.
The PR introduces the key
enable_workspacesto control workspace functionality. Ensure this naming is consistent across the entire codebase, including other files that might reference this setting.Also applies to: 82-86, 212-226, 232-234, 279-281
🏁 Script executed:
#!/bin/bash # Check for all occurrences of workspace-related settings across the codebase echo "Checking for workspace-related settings..." rg -A 1 -B 1 "enable_workspaces|is_workspace_enabled" --glob "*.py" --glob "*.json" echo "Checking settings.json file..." cat settings.json | grep -E "workspace|enable"Length of output: 1777
Consistent Naming Verified
The key
"enable_workspaces"is used uniformly across the codebase. Our search shows that both insrc/common/common.pyandsettings.json, the workspace functionality is checked and configured using"enable_workspaces", with no discrepancies noted in the referenced lines (including 44-47, 82-86, 212-226, 232-234, and 279-281).
| # If no workspace is specified and workspace feature is enabled, set default workspace and query param | ||
| if "workspace" not in st.query_params and st.session_state.settings["enable_workspaces"]: | ||
| st.query_params.workspace = st.session_state.workspace.name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive code for settings access.
Add error handling for the settings key:
- # If no workspace is specified and workspace feature is enabled, set default workspace and query param
- if "workspace" not in st.query_params and st.session_state.settings["enable_workspaces"]:
+ # If no workspace is specified and workspace feature is enabled, set default workspace and query param
+ if "workspace" not in st.query_params and st.session_state.settings.get("enable_workspaces", True):
st.query_params.workspace = st.session_state.workspace.name📝 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 no workspace is specified and workspace feature is enabled, set default workspace and query param | |
| if "workspace" not in st.query_params and st.session_state.settings["enable_workspaces"]: | |
| st.query_params.workspace = st.session_state.workspace.name | |
| # If no workspace is specified and workspace feature is enabled, set default workspace and query param | |
| if "workspace" not in st.query_params and st.session_state.settings.get("enable_workspaces", True): | |
| st.query_params.workspace = st.session_state.workspace.name |
| # Display workspace switcher if workspace is enabled in local mode | ||
| if st.session_state.settings["enable_workspaces"]: | ||
| with st.expander("🖥️ **Workspaces**"): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive code for settings access.
Add error handling for the settings key:
- # Display workspace switcher if workspace is enabled in local mode
- if st.session_state.settings["enable_workspaces"]:
+ # Display workspace switcher if workspace is enabled in local mode
+ if st.session_state.settings.get("enable_workspaces", True):📝 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.
| # Display workspace switcher if workspace is enabled in local mode | |
| if st.session_state.settings["enable_workspaces"]: | |
| with st.expander("🖥️ **Workspaces**"): | |
| # Display workspace switcher if workspace is enabled in local mode | |
| if st.session_state.settings.get("enable_workspaces", True): | |
| with st.expander("🖥️ **Workspaces**"): |
|
|
||
| # Check if workspace is enabled. If not, load default parameters. | ||
| if not st.session_state.settings["enable_workspaces"]: | ||
| default = True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing settings key.
The code directly accesses st.session_state.settings["enable_workspaces"] without checking if this key exists in the settings dictionary. For backward compatibility and to prevent KeyError exceptions, consider using a safer approach.
- # Check if workspace is enabled. If not, load default parameters.
- if not st.session_state.settings["enable_workspaces"]:
+ # Check if workspace is enabled. If not, load default parameters.
+ if not st.session_state.settings.get("enable_workspaces", True):
default = True📝 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.
| # Check if workspace is enabled. If not, load default parameters. | |
| if not st.session_state.settings["enable_workspaces"]: | |
| default = True | |
| # Check if workspace is enabled. If not, load default parameters. | |
| if not st.session_state.settings.get("enable_workspaces", True): | |
| default = True |
| # Check if workspace logic is enabled | ||
| if st.session_state.settings["enable_workspaces"]: | ||
| if "workspace" in st.query_params: | ||
| st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) | ||
| elif st.session_state.location == "online": | ||
| workspace_id = str(uuid.uuid1()) | ||
| st.session_state.workspace = Path(workspaces_dir, workspace_id) | ||
| st.query_params.workspace = workspace_id | ||
| else: | ||
| st.session_state.workspace = Path(workspaces_dir, "default") | ||
| st.query_params.workspace = "default" | ||
|
|
||
| else: | ||
| # Use default workspace when workspace feature is disabled | ||
| st.session_state.workspace = Path(workspaces_dir, "default") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix indentation and add error handling.
There's inconsistent indentation in this code block, and there's no error handling for the settings key.
# Check if workspace logic is enabled
- if st.session_state.settings["enable_workspaces"]:
+ if st.session_state.settings.get("enable_workspaces", True):
if "workspace" in st.query_params:
- st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace)
+ st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace)
elif st.session_state.location == "online":
- workspace_id = str(uuid.uuid1())
- st.session_state.workspace = Path(workspaces_dir, workspace_id)
- st.query_params.workspace = workspace_id
+ workspace_id = str(uuid.uuid1())
+ st.session_state.workspace = Path(workspaces_dir, workspace_id)
+ st.query_params.workspace = workspace_id
else:
st.session_state.workspace = Path(workspaces_dir, "default")
st.query_params.workspace = "default"📝 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.
| # Check if workspace logic is enabled | |
| if st.session_state.settings["enable_workspaces"]: | |
| if "workspace" in st.query_params: | |
| st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) | |
| elif st.session_state.location == "online": | |
| workspace_id = str(uuid.uuid1()) | |
| st.session_state.workspace = Path(workspaces_dir, workspace_id) | |
| st.query_params.workspace = workspace_id | |
| else: | |
| st.session_state.workspace = Path(workspaces_dir, "default") | |
| st.query_params.workspace = "default" | |
| else: | |
| # Use default workspace when workspace feature is disabled | |
| st.session_state.workspace = Path(workspaces_dir, "default") | |
| # Check if workspace logic is enabled | |
| if st.session_state.settings.get("enable_workspaces", True): | |
| if "workspace" in st.query_params: | |
| st.session_state.workspace = Path(workspaces_dir, st.query_params.workspace) | |
| elif st.session_state.location == "online": | |
| workspace_id = str(uuid.uuid1()) | |
| st.session_state.workspace = Path(workspaces_dir, workspace_id) | |
| st.query_params.workspace = workspace_id | |
| else: | |
| st.session_state.workspace = Path(workspaces_dir, "default") | |
| st.query_params.workspace = "default" | |
| else: | |
| # Use default workspace when workspace feature is disabled | |
| st.session_state.workspace = Path(workspaces_dir, "default") |
|
Thank you for your contribution! Merging now (after tests have completed). |
3 participants
updated the
settings.jsonfile to include a new key use_workspace_logic with a default value of true. This setting controls whether to use the existing workspace logic or not.
updated
src/common/common.pyto handle the single workspace scenario when is_workspace_enabled is false. This includes loading default parameters if no other parameters are found and saving changes to a JSON file in the workspace directory.Summary by CodeRabbit
"enable_workspaces": truein settings.