🎨 Palette: Add progress bar for fetching existing rules#147
🎨 Palette: Add progress bar for fetching existing rules#147
Conversation
- Added progress bar to `get_all_existing_rules` for better user feedback during parallel fetch. - Removed duplicate `render_progress_bar` definition in `main.py`. - Added test case `test_get_all_existing_rules_shows_progress`. - Created `.jules/palette.md` for journal entries. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 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. |
|
Merging to
|
Summary of ChangesHello @abhimehro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the Palette tool by introducing a progress bar for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| writes = [str(args[0]) for args, _ in mock_stderr.write.call_args_list] | ||
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) | ||
| # Total calls >= 6 | ||
| assert len(progress_writes) >= 6 |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
test_main.py
Outdated
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
test_main.py
Outdated
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| def side_effect(client, url): | ||
| mock_resp = MagicMock() | ||
| mock_resp.json.return_value = {"body": {"rules": []}} | ||
| return mock_resp |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| ) | ||
|
|
||
| if USE_COLORS: | ||
| sys.stderr.write(f"\r\033[K") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Using an f-string that does not have any interpolated variables Note
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Unused argument 'url' Note test
| ) | ||
|
|
||
| if USE_COLORS: | ||
| sys.stderr.write(f"\r\033[K") |
Check warning
Code scanning / Prospector (reported by Codacy)
Using an f-string that does not have any interpolated variables (f-string-without-interpolation) Warning
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) |
Check warning
Code scanning / Prospector (reported by Codacy)
Unused argument 'client' (unused-argument) Warning test
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) |
Check warning
Code scanning / Prospector (reported by Codacy)
Unused argument 'url' (unused-argument) Warning test
| def side_effect(client, url): | ||
| mock_resp = MagicMock() | ||
| mock_resp.json.return_value = {"body": {"rules": []}} | ||
| return mock_resp |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning test
test_main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Do not use len(SEQUENCE) to determine if a sequence is empty Warning test
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) |
Check notice
Code scanning / Pylint (reported by Codacy)
Unused argument 'client' Note test
There was a problem hiding this comment.
Pull request overview
Adds visual progress feedback when fetching existing rules and removes a duplicate progress bar implementation, plus documents a related UX/accessibility learning.
Changes:
- Removed the older duplicate
render_progress_barimplementation and consolidated on a single, documented helper with consistent behavior. - Updated
get_all_existing_rulesto show a progress bar while fetching folder rules in parallel, clearing stderr appropriately around warnings. - Added tests to verify that
get_all_existing_rulesemits progress output and created a.jules/palette.mdentry documenting the stderr/progress-bar interaction pattern.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
main.py |
Uses the shared render_progress_bar in get_all_existing_rules and adds stderr line-clearing around parallel fetch warnings to keep the progress UI clean. |
test_main.py |
Extends the test suite to confirm get_all_existing_rules writes progress bar updates to stderr when colors/TTY are enabled. |
.jules/palette.md |
Records a UX/accessibility learning about managing stderr and logging when using CLI progress bars with parallel operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a progress bar for fetching existing rules, which is a great UX improvement. However, a security vulnerability was identified in the new error handling logic where external data is logged without sanitization, potentially allowing for terminal hijacking. This should be addressed by using the existing sanitize_for_log utility. Additionally, ensure exceptions from worker threads are handled correctly to prevent UI corruption, and consider improving the new test case to cover error scenarios.
| sys.stderr.flush() | ||
|
|
||
| folder_id = future_to_folder[future] | ||
| log.warning(f"Failed to fetch rules for folder ID {folder_id}: {e}") |
There was a problem hiding this comment.
A security vulnerability exists here: the folder_id and exception e are interpolated directly into the log message without sanitization. Since folder_id comes from an external API and e can contain untrusted data, this could allow an attacker to inject ANSI escape sequences into the terminal, leading to terminal hijacking or log injection. Please use the existing sanitize_for_log function to prevent this. Additionally, ensure that exceptions from the _fetch_folder_rules function are properly re-raised and propagate to this except block, as logging directly from worker threads can interfere with the progress bar rendering and this handler might not be triggered as expected.
| log.warning(f"Failed to fetch rules for folder ID {folder_id}: {e}") | |
| log.warning(f"Failed to fetch rules for folder ID {sanitize_for_log(folder_id)}: {sanitize_for_log(e)}") |
| def test_get_all_existing_rules_shows_progress(monkeypatch): | ||
| m = reload_main_with_env(monkeypatch, no_color=None, isatty=True) | ||
| mock_client = MagicMock() | ||
| profile_id = "test_profile" | ||
|
|
||
| mock_stderr = MagicMock() | ||
| monkeypatch.setattr(sys, "stderr", mock_stderr) | ||
|
|
||
| # Mock list_existing_folders to return multiple folders | ||
| folders = {f"Folder{i}": f"id_{i}" for i in range(5)} | ||
| monkeypatch.setattr(m, "list_existing_folders", MagicMock(return_value=folders)) | ||
|
|
||
| # Mock _api_get | ||
| def side_effect(client, url): | ||
| mock_resp = MagicMock() | ||
| mock_resp.json.return_value = {"body": {"rules": []}} | ||
| return mock_resp | ||
| monkeypatch.setattr(m, "_api_get", side_effect) | ||
|
|
||
| # Run | ||
| m.get_all_existing_rules(mock_client, profile_id) | ||
|
|
||
| # Check that progress bar logic was invoked | ||
| # render_progress_bar writes to stderr with \r... | ||
| # We check if there were writes containing "Fetching existing rules" | ||
| writes = [str(args[0]) for args, _ in mock_stderr.write.call_args_list] | ||
| progress_writes = [w for w in writes if "Fetching existing rules" in w] | ||
|
|
||
| assert len(progress_writes) > 0 | ||
| # Should be called initially (0/5) + for each folder (1/5 ... 5/5) | ||
| # Total calls >= 6 | ||
| assert len(progress_writes) >= 6 |
There was a problem hiding this comment.
This test effectively covers the success path for the progress bar. To ensure the new error handling logic is also working correctly, consider adding a test case for the failure scenario.
You could configure the _api_get mock to raise an httpx.HTTPError for one of the calls. Then you could assert that:
log.warningis called with the expected error message.- The
stderroutput contains the warning. - The progress bar calls continue for all folders, including the one that failed.
This would provide confidence that errors are handled gracefully without corrupting the progress bar display.
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
💡 What: Added a progress bar to the
get_all_existing_rulesfunction and removed a duplicaterender_progress_bardefinition.🎯 Why: Users had no visual feedback when the script was fetching existing rules from multiple folders, which could take time.
📸 Before/After: Before: Static "Wait..." (or nothing). After: Progress bar "🔍 Fetching existing rules: [██░░] 50%"
PR created automatically by Jules for task 17720429687181325290 started by @abhimehro