Skip to content

force cli screenshot bug fix for CLI JSON#4319

Merged
Maheshkale447 merged 1 commit into
Releases/Official-Releasefrom
BugFix/CLIFlagForceScreenshot
Oct 6, 2025
Merged

force cli screenshot bug fix for CLI JSON#4319
Maheshkale447 merged 1 commit into
Releases/Official-Releasefrom
BugFix/CLIFlagForceScreenshot

Conversation

@AmanPrasad43

@AmanPrasad43 AmanPrasad43 commented Oct 4, 2025 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Thank you for your contribution.
Before submitting this PR, please make sure:

  • PR description and commit message should describe the changes done in this PR
  • Verify the PR is pointing to correct branch i.e. Release or Beta branch if the code fix is for specific release , else point it to master
  • Latest Code from master or specific release branch is merged to your branch
  • No unwanted\commented\junk code is included
  • No new warning upon build solution
  • Code Summary\Comments are added to my code which explains what my code is doing
  • Existing unit test cases are passed
  • New Unit tests are added for your development
  • Sanity Tests are successfully executed for New and Existing Functionality
  • Verify that changes are compatible with all relevant browsers and platforms.
  • After creating pull request there should not be any conflicts
  • Resolve all Codacy comments
  • Builds and checks are passed before PR is sent for review
  • Resolve code review comments
  • Update the Help Library document to match any feature changes

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where the “Capture UI screenshots” setting in execution configurations was not applied when loading runsets. The preference is now correctly honored, ensuring screenshots are captured as configured. Users should see more consistent screenshot behavior and more complete visual evidence in reports and logs during automated runs. No changes to user workflows or controls are required.

@coderabbitai

coderabbitai Bot commented Oct 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

Propagates the ForceUiScreenshot flag from GingerRunnerExecution config to the GingerRunner instance in LoadRunsetFromExecutionConfig within DynamicExecutionManager. The assignment is added adjacent to other runner property initializations, after KeepAgentsOn is set and before environment binding.

Changes

Cohort / File(s) Summary
Dynamic execution setup
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs		 Adds assignment to set GingerRunner.ForceUiScreenshot from GingerRunnerExecution config during LoadRunsetFromExecutionConfig initialization.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested reviewers

  • Maheshkale447

Poem

A hop, a skip, a screenshot too,
I toggled a flag—now pictures ensue.
From config to runner, the thread is tight,
A carrot of clarity in morning light.
Snap! says the hare, with ears held high—
One tiny tweak, and pixels fly. 🥕📸

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description only repeats the repository’s checklist template without any details of the actual changes, missing a summary of the ForceUiScreenshot propagation, testing information, and confirmation of completed checklist items, so it does not fulfill the required template’s intent. Please update the PR description by replacing the placeholder checklist with a completed summary of the ForceUiScreenshot change, details of added or passing tests, confirmation of branch and merge status, and any other required information from the repository template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title directly references the CLI screenshot issue and specifies that the fix applies to JSON output, which aligns with the main change of propagating the ForceUiScreenshot flag in the CLI JSON logic, making it a clear and focused summary of the primary modification.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BugFix/CLIFlagForceScreenshot
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a20e96e and c1725fb.

📒 Files selected for processing (1)
  • Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (1 hunks)
🔇 Additional comments (1)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (1)

1223-1223: Verify nullability of runnerConfig.ForceUiScreenshot

This completes the serialization round-trip, but adjacent nullable flags check .HasValue before assignment. It’s unclear if runnerConfig.ForceUiScreenshot is bool?. Please confirm its declaration and, if nullable, wrap the assignment:

- gingerRunner.ForceUiScreenshot = runnerConfig.ForceUiScreenshot;
+ if (runnerConfig.ForceUiScreenshot.HasValue)
+ {
+     gingerRunner.ForceUiScreenshot = runnerConfig.ForceUiScreenshot.Value;
+ }

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Maheshkale447
Maheshkale447 merged commit 2c78046 into Releases/Official-Release Oct 6, 2025
11 checks passed
@Maheshkale447
Maheshkale447 deleted the BugFix/CLIFlagForceScreenshot branch October 6, 2025 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Maheshkale447 Maheshkale447 Maheshkale447 approved these changes
@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AmanPrasad43 @Maheshkale447