full page scrreenshot enhancement vrt#4492
Conversation
WalkthroughThe PR implements a full-page screenshot toggle feature for the Visual Testing action editor by changing the IsFullPageScreenshot property default to false, adding XAML layout for the checkbox control, binding it to the action model, and conditionally showing it based on action type and baseline configuration. ChangesFull-Page Screenshot Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Not up to standards ⛔
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Ginger/Ginger/Actions/ActionEditPages/VisualTesting/VRTComparePage.xaml`:
- Around line 96-97: The new controls xFullPageScreenshotLabel and
xFullPageScreenshotCheckbox currently lack an explicit Visibility setting and
briefly render visible before InitLayout() collapses them for the default Track
action state; add Visibility="Collapsed" to both control declarations (the Label
named xFullPageScreenshotLabel and the CheckBox named
xFullPageScreenshotCheckbox in VRTComparePage.xaml) so they start hidden and
avoid flicker, matching the pattern used by other conditionally-shown rows
handled by InitLayout().
In `@Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/ActVisualTesting.cs`:
- Around line 184-186: The change introduced a new in-memory default of false
that causes existing saved actions missing the IsFullPageScreenshot param to
silently flip behavior; update the call that synthesizes the param in
ActVisualTesting (the GetOrCreateInputParam usage around IsFullPageScreenshot)
to use true as the runtime default (i.e., pass true.ToString() or otherwise
treat missing param as true) or implement a one-time migration/upgrade path that
detects absent IsFullPageScreenshot and sets it to "True" for persisted actions
so pre-existing actions keep their original full-page behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f00a3781-7a62-44ee-89e7-4edb425b6939
📒 Files selected for processing (3)
Ginger/Ginger/Actions/ActionEditPages/VisualTesting/VRTComparePage.xamlGinger/Ginger/Actions/ActionEditPages/VisualTesting/VRTComparePage.xaml.csGinger/GingerCoreNET/ActionsLib/UI/VisualTesting/ActVisualTesting.cs
| <Label x:Name="xFullPageScreenshotLabel" Style="{StaticResource $LabelStyle}" Grid.Row="11" Grid.Column="0" Margin="0,10,0,0" VerticalAlignment="Center" Content="Full Page Screenshot:"/> | ||
| <CheckBox x:Name="xFullPageScreenshotCheckbox" Grid.Row="11" Grid.Column="1" VerticalAlignment="Center" Margin="5,10,0,0" Style="{StaticResource @InputCheckBoxStyle}" ToolTip="Capture the full page screenshot of the active window for baseline and checkpoint comparison"/> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add Visibility="Collapsed" as the XAML default for the new controls.
Neither xFullPageScreenshotLabel nor xFullPageScreenshotCheckbox declares an initial Visibility. WPF defaults both to Visible, so they are briefly shown until InitLayout() runs in the constructor and collapses them for the default Track action state. Adding the attribute eliminates any possible visual flicker and makes the intent explicit, consistent with how other conditionally-shown rows behave in this page.
🛡️ Proposed fix
- <Label x:Name="xFullPageScreenshotLabel" Style="{StaticResource $LabelStyle}" Grid.Row="11" Grid.Column="0" Margin="0,10,0,0" VerticalAlignment="Center" Content="Full Page Screenshot:"/>
- <CheckBox x:Name="xFullPageScreenshotCheckbox" Grid.Row="11" Grid.Column="1" VerticalAlignment="Center" Margin="5,10,0,0" Style="{StaticResource `@InputCheckBoxStyle`}" ToolTip="Capture the full page screenshot of the active window for baseline and checkpoint comparison"/>
+ <Label x:Name="xFullPageScreenshotLabel" Visibility="Collapsed" Style="{StaticResource $LabelStyle}" Grid.Row="11" Grid.Column="0" Margin="0,10,0,0" VerticalAlignment="Center" Content="Full Page Screenshot:"/>
+ <CheckBox x:Name="xFullPageScreenshotCheckbox" Visibility="Collapsed" Grid.Row="11" Grid.Column="1" VerticalAlignment="Center" Margin="5,10,0,0" Style="{StaticResource `@InputCheckBoxStyle`}" ToolTip="Capture the full page screenshot of the active window for baseline and checkpoint comparison"/>🤖 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 `@Ginger/Ginger/Actions/ActionEditPages/VisualTesting/VRTComparePage.xaml`
around lines 96 - 97, The new controls xFullPageScreenshotLabel and
xFullPageScreenshotCheckbox currently lack an explicit Visibility setting and
briefly render visible before InitLayout() collapses them for the default Track
action state; add Visibility="Collapsed" to both control declarations (the Label
named xFullPageScreenshotLabel and the CheckBox named
xFullPageScreenshotCheckbox in VRTComparePage.xaml) so they start hidden and
avoid flicker, matching the pattern used by other conditionally-shown rows
handled by InitLayout().
| bool value = false; | ||
| bool.TryParse(GetOrCreateInputParam(nameof(IsFullPageScreenshot), value.ToString()).Value, out value); | ||
| return value; |
There was a problem hiding this comment.
Silent behavioral regression for pre-existing actions lacking a stored IsFullPageScreenshot param.
Changing the in-memory default from true to false affects any action that was created (and saved) before this property was populated. For those actions, GetOrCreateInputParam will synthesize the value at runtime using "False" as the default — switching them from implicitly-enabled full-page screenshots to implicitly-disabled without any explicit save/migration step.
If the property was newly introduced in an earlier PR and users could have run actions that persisted "True" to their XML, this change is safe. But if any existing saved action XML files lack this param entirely, they will silently regress.
Consider checking whether a migration note or one-time upgrade path is needed, or assert in release notes that the new default is intentionally false.
🤖 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 `@Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/ActVisualTesting.cs` around
lines 184 - 186, The change introduced a new in-memory default of false that
causes existing saved actions missing the IsFullPageScreenshot param to silently
flip behavior; update the call that synthesizes the param in ActVisualTesting
(the GetOrCreateInputParam usage around IsFullPageScreenshot) to use true as the
runtime default (i.e., pass true.ToString() or otherwise treat missing param as
true) or implement a one-time migration/upgrade path that detects absent
IsFullPageScreenshot and sets it to "True" for persisted actions so pre-existing
actions keep their original full-page behavior.
2 participants
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit