Fix browser control logic and UI status reporting - #15
Conversation
- Update Start/Stop browser buttons to correctly reflect browser presence - Fix status indicator to show browser status independently of automation - Add immediate UI feedback with st.rerun() and brief success message pauses - Add .gitignore to keep the repository clean of build artifacts and logs
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR refactors browser lifecycle management in the Streamlit app sidebar to depend on session state presence rather than automation status, simplifies the status indicator accordingly, and adds a .gitignore file for Python development artifacts. ChangesBrowser Controls and Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Review Summary by QodoFix browser control states and improve UI status feedback
WalkthroughsDescription• Fix browser control button states to reflect actual browser presence • Decouple browser status from automation state for accurate UI feedback • Add immediate UI feedback with success messages and page rerun • Improve user experience with clearer button enable/disable logic Diagramflowchart LR
A["Browser State"] -->|"Check if browser exists"| B["Start/Stop Button Logic"]
B -->|"Enabled based on browser presence"| C["Button State Updated"]
C -->|"User clicks button"| D["Browser Action Executed"]
D -->|"Show success message"| E["UI Rerun for Feedback"]
F["Automation State"] -->|"Independent from browser"| G["Status Indicator"]
G -->|"Shows browser status only"| H["Accurate Status Display"]
File Changes1. app.py
|
Code Review by Qodo
1. Stale browser state on failure
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if st.sidebar.button("🚀 Start Browser", disabled=st.session_state.browser is not None): | ||
| try: | ||
| st.session_state.browser = BrowserAutomation() | ||
| st.session_state.browser.start_browser() | ||
| st.session_state.automation_active = False # Browser started, but automation not yet active | ||
| st.sidebar.success("✅ Browser started") | ||
| time.sleep(1) # Give user a moment to see the success message | ||
| st.rerun() | ||
| except Exception as e: | ||
| st.sidebar.error(f"❌ Failed to start browser: {str(e)}") | ||
|
|
There was a problem hiding this comment.
1. Stale browser state on failure 🐞 Bug ☼ Reliability
In setup_sidebar(), st.session_state.browser is set to a BrowserAutomation instance before start_browser() succeeds; if start_browser() raises, the exception path does not reset st.session_state.browser to None. On the next render this disables “Start Browser” and reports “🟢 Running” even though BrowserAutomation.driver was never created, breaking recovery and misleading status.
Agent Prompt
### Issue description
`setup_sidebar()` sets `st.session_state.browser = BrowserAutomation()` before `start_browser()` succeeds. If `start_browser()` throws, the `except` path does not clear `st.session_state.browser`, so the UI disables **Start Browser** and shows **Running** even though the underlying driver was never created.
### Issue Context
`BrowserAutomation.start_browser()` re-raises exceptions and only assigns `self.driver` after `webdriver.Firefox(...)` succeeds.
### Fix Focus Areas
- app.py[52-62]
- app.py[79-80]
### Suggested fix
- Only assign `st.session_state.browser` *after* a successful `start_browser()` call (use a local variable first), **or** ensure the `except` block calls `close()` (if needed) and sets `st.session_state.browser = None`.
- Consider computing “running”/button disabled state based on an explicit runtime flag, e.g. `st.session_state.browser is not None and st.session_state.browser.driver is not None` (or a new `BrowserAutomation.is_running()` helper).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.py`:
- Around line 57-59: The success messages added with st.sidebar.success(...) are
never shown because st.rerun() immediately restarts the script before Streamlit
can render them; remove the immediate st.rerun() calls in the Start and Stop
browser handlers (the blocks that call st.sidebar.success, time.sleep(1), and
st.rerun()) or instead persist a flag/message in st.session_state (e.g.,
session_state["browser_status_message"]) and call st.rerun() only after setting
that state so the UI reads and displays the message across reruns; update the
handlers that use st.sidebar.success, time.sleep, and st.rerun accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| st.sidebar.success("✅ Browser started") | ||
| time.sleep(1) # Give user a moment to see the success message | ||
| st.rerun() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Streamlit st.rerun() prevent success messages from displaying when called immediately after st.success()?
💡 Result:
Yes, calling st.rerun immediately after st.success prevents the success message from displaying to the user. st.rerun halts script execution immediately and queues a rerun from the top, so the st.success element is never flushed to the frontend [1][2][3]. The message does not persist across the rerun because status elements like st.success are not preserved during reruns [4][5][6]. Community solutions include using third-party components like streamlit-notify, which queues notifications in session_state to display them after rerun [5][6][7], or st.toast with session_state workarounds [4]. Official docs confirm st.rerun stops execution without further output [1].
Citations:
- 1: https://docs.streamlit.io/develop/api-reference/execution-flow/st.rerun
- 2: https://docs.streamlit.io/1.28.0/develop/api-reference/execution-flow/st.rerun
- 3: https://github.com/streamlit/streamlit/blob/1.25.0/lib/streamlit/commands/execution_control.py
- 4: Toasts are not preserved when page is rerun streamlit/streamlit#7740
- 5: https://discuss.streamlit.io/t/streamlit-notify-allows-streamlit-status-elements-to-be-displayed-upon-app-rerun/114149
- 6: https://pypi.org/project/streamlit-notify/
- 7: https://github.com/pgarrett-scripps/Streamlit_Notify
Success message will not display to users.
The st.sidebar.success() call followed immediately by st.rerun() prevents the success message from being displayed. Streamlit's st.rerun() halts script execution and restarts from the top before rendering occurs. Status elements like st.sidebar.success() are not preserved across reruns, so the message is never flushed to the frontend. The time.sleep(1) executes server-side but does not affect the rendering pipeline.
This pattern appears in both the Start and Stop browser button handlers (lines 52-61 and 63-73) and prevents the UI feedback improvement goal stated in the PR from being achieved.
Consider removing the st.rerun() calls to allow the success messages to display, or use session state to persist messages across reruns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.py` around lines 57 - 59, The success messages added with
st.sidebar.success(...) are never shown because st.rerun() immediately restarts
the script before Streamlit can render them; remove the immediate st.rerun()
calls in the Start and Stop browser handlers (the blocks that call
st.sidebar.success, time.sleep(1), and st.rerun()) or instead persist a
flag/message in st.session_state (e.g., session_state["browser_status_message"])
and call st.rerun() only after setting that state so the UI reads and displays
the message across reruns; update the handlers that use st.sidebar.success,
time.sleep, and st.rerun accordingly.
1 participant
I analyzed the browser launching process and identified several UI bugs in the control sidebar. Specifically, the "Stop Browser" button was only enabled when automation was active, even if the browser was running. I've updated the logic to use the actual browser instance state, improved the status reporting, and added better UI feedback when starting/stopping. I also added a .gitignore to prevent binary artifacts from being tracked.
Summary by CodeRabbit
Bug Fixes
Chores
.gitignorefile to exclude common Python development artifacts and local environment files.