refactor: extract pure AD-fallback helpers and fold overview's double-pass - #9
Merged
Merged
Conversation
…-pass get_ad_users mixed cache/TTL/fetch orchestration with two untested pure steps (fallback-user building, filter+truncate); extract those into a new ad_users module with unit tests, so the pure logic is testable independent of the tauri::State-based command. build_overview scanned devices twice to compute upgrade_needed (once in the main tally loop, once via a separate .filter().count()); fold the second pass into the existing loop. status.upgrade and upgrade_needed remain distinct counters as before (guarded by a new regression test). No behavior change; both refactors are pure code-quality follow-ups from a full repo review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
get_ad_usersinto small, pure, independently testable functions in a newad_usersmodule. The stateful AD cache/TTL/fetch/lock-ordering logic (ad_fetch-before-inner) is untouched.build_overview's separate second.filter(needs_upgrade).count()pass into the existing single tallying loop via one more accumulator, removing a redundant full-slice scan while preserving bothstatus.upgrade(status=="upgrade" only) andupgrade_needed(upgrade OR stale-with-reasons) as distinct values.Defaultderive toDeviceFullto enable terse test fixtures for the above.Why
Readability/maintainability follow-ups from a full code review — no behavior changes, no new dependencies, no speculative abstractions. Both extractions follow the existing pattern in this codebase (see
store/facts.rs,upgrade.rs) of keeping pure decision logic in small plain functions that are easy to unit test, separate from the stateful orchestration layer.Changes
app/src-tauri/src/ad_users.rs:fallback_users_from_devices,filter_and_truncate(pure, unit-tested).app/src-tauri/src/ad_users_tests.rs: unit tests for the above.app/src-tauri/src/commands.rs:get_ad_usersnow calls into the two extracted helpers instead of inlining the logic; lock ordering / TTL / cache orchestration unchanged.app/src-tauri/src/store/overview.rs:build_overviewcomputesupgrade_neededinside the existing single accumulation loop instead of a separate pass;status.upgradecomputation unchanged.app/src-tauri/src/store/overview_tests.rs: new focused unit test assertingstatus.upgradeandupgrade_neededdiverge correctly for a stale-with-reasons device.app/src-tauri/src/model.rs:DeviceFullderivesDefault(all field types already support it) to keep test fixtures terse.Test plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test --all(32 tests pass;overview_aggregatesassertions unchanged:status.upgrade == 4,upgrade_needed == 5)npm run check