fix(ui): contain non-square item art and keep Details open across scans (#60, #61) - #63
Merged
Merged
Conversation
RefreshResult() reset _detailsOpen on every scan-driven PropertyChanged, collapsing the Details section every time Auto Scan enqueued a result. The user's Details choice now lives in ResultCardUiState and is preserved by scan-driven refreshes; only explicit user actions (toggle, recent-scan click, search pick) change it. Also render aria-expanded as an explicit lowercase string: Blazor omits bool-valued attributes when false, so the toggle never exposed its expanded state while collapsed.
Percentage max-width/max-height on a grid item can leave the grid area height indefinite in Chromium, so tall intrinsic icons (e.g. the 64x127 Pevko grid image) painted below the 88x64 frame onto the verdict row; square icons overflowed slightly too. The image is now absolutely positioned to the frame box (5px inset) with object-fit: contain, which letterboxes any aspect ratio without cropping or stretching, plus an overflow: hidden invariant on the frame.
Extract the launch/CDP/cleanup machinery from the WebView smoke test into a reusable UiSession so a second durable fact can share it, and add a regression fact that: - seeds a tiny offline catalog (portrait 1x2 Pevko, landscape 2x1 Makarov PM, square M4A1 with locally installed icons) so the result card renders hermetically without network or a developer cache; - asserts the art image stays inside the frame at desktop and narrow breakpoints for portrait, landscape, and square icons (#60); - asserts Details follows user actions: closed after a deliberate search pick, open via the toggle, closed again by a deliberate switch and by a recent-scan click, with aria-expanded exposed throughout (#61). Unit coverage for the scan-driven preserve rule lives in ResultCardUiStateTests; the harness cannot reach a scan-driven refresh without a game client.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR centralizes result-card UI state, constrains non-square artwork within its frame, and adds catalog-backed WebView coverage. The UI test harness now manages application resources, diagnostics, artifacts, process shutdown, and cache fixtures through shared session components. ChangesResult card UI behavior
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟡 Moderate · up to The PR fixes localized result-card behavior, but its new UI-test setup can depend on network refreshes, obscure the original test failure during cleanup, or leave existing cache files renamed if setup fails. These bounded test-harness risks should be addressed or explicitly accepted before merge. Fixed issue severity: Medium Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses the PR review finding: if UiSession.StartAsync threw after Process.Start succeeded (DevToolsActivePort timeout, Playwright connect, tracing, or endpoint write), the launched RatScanner process, its temp WebView2 profile, and the run directory leaked — breaking the single-instance guard for every later UI run. The whole post-start sequence now shares one failure path that stops the owned process, disposes any Playwright/browser resources created so far, and best-effort deletes the profile before rethrowing the original startup failure. Also hardens the suite while touching this file: - SelectCatalogItemAsync matches the ShortName <small> exactly instead of substring-matching item names, so a query like "PM" cannot land on a different list item. - Cancellation token is plumbed into resize waits and failure-evidence writes. - CatalogCacheSeed cleanup retries transient WebView2 file locks and surfaces persistent cleanup failures instead of swallowing them.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
coderabbitai
Bot
added
the
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses the review round on the previous commit. All three findings target CatalogCacheSeed teardown/rollback and its hermeticity claim: - Dispose no longer throws: during a test failure the AggregateException would have replaced the real assertion failure while the using scope unwound, hiding the evidence MarkFailedAsync captured. Cleanup failures are now written to catalog-seed-cleanup-errors.log in the run's own artifact directory (UiSession.CreateRunDirectory is shared so the seed and session land in the same folder), matching UiSession.DisposeAsync's report-don't-throw teardown pattern. - The constructor rolls back on partial seeding: if backing up or writing a fixture file fails, every file moved to .bak is restored and written fixtures are removed before rethrowing, so the developer's real cache can never be left renamed. - The cache seed now covers every family the app refreshes at startup (tasks_v2, hideout, crafts, barters, maps alongside items). The test previously still fetched those catalogs from the network — 'hermetic' in name only. Verified in the run log: 0 'Fetching data' lines, all core caches loaded from offline storage.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
If a backup File.Move threw during seeding, the developer's real cache file stayed at the original path unrecorded, and the previous rollback (which deleted every cache key before restoring recorded backups) would delete that unowned file permanently. Seeding now tracks both the backups moved aside and the paths it wrote; rollback restores the former and deletes only the latter, leaving every file the seed never touched exactly as found.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
RollbackSeeding swallowed restore/delete failures silently, unlike the same class of cleanup failure in Dispose, which reports to catalog-seed-cleanup-errors.log. Rollback now collects and writes its failures to the same diagnostics file (best effort, non-throwing), so a stranded .bak or leftover fixture file never goes undiagnosed.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #60 and #61 in one batch of scan-result-card fixes.
#61 — Details dropdown auto-collapses on every scan
RefreshResult()unconditionally reset_detailsOpen = false, and every scan enqueue raisesMenuVM.PropertyChangedsynchronously, so with Auto Scan on the Details section collapsed on every scan.ResultCardUiState(src/App/Presentation/ResultCardUiState.cs).SelectRecentandSelectItemkeep collapsing deliberately.aria-expanded="@_uiState.DetailsOpen"was rendered with a bool, and Blazor omits bool attributes when false — the toggle never exposed its expanded state while collapsed. It now renders an explicit lowercase"true"/"false"string.#60 — Non-square item icons overflow the art frame
Reproduced in isolation: percentage
max-heighton the grid item is ignored by Chromium (indefinite grid-area height), so the 64×127 Pevko icon painted 73×146 below the 88×64 frame onto the verdict row — and square icons overflowed slightly too (68×68)..item-art imgis now absolutely positioned to the frame box (5px inset) withobject-fit: contain— deterministic at every breakpoint, letterboxes without cropping/stretching.overflow: hiddenon the frame as a clip invariant.Verification
ResultCardUiStateTests(7 facts) locking the scan-driven preserve rule — the UI harness can't reach a scan-driven refresh without a game client.Scan_result_card_contains_non_square_icons_and_details_state_follows_user_actions: seeds a tiny offline catalog (Pevko 1×2 portrait, Makarov PM 2×1 landscape, M4A1 square, all with locally installed icons) and asserts art containment at desktop + narrow breakpoints and the Details user-action contract viaaria-expanded.UiSession(launch/CDP/trace/cleanup) shared by both facts — original assertions unchanged, still passes.scripts/verify.ps1 -Mode Fastpasses (csharpier, markdown lint, agent docs, Debug build + analyzer gate, 368 unit tests); Release build and unit tests pass; full UI suite (2/2) passes with screenshots underartifacts/ui-tests/.Summary by cubic
Contain non-square result-card item art and keep the Details section open across scan-driven refreshes. Previously, tall/square icons overflowed the frame and Details collapsed on every auto scan; now art is letterboxed and Details only closes on explicit user actions, with aria-expanded rendered as an explicit "true"/"false" string.
Tests and hardening
Written for commit 42e4b0c. Summary will update on new commits.