Preserve legacy tag configurations - #34
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConnection tag validation now compares edits with existing tags, preserving legacy over-limit values when unchanged while rejecting new violations. Application and batch storage paths use this behavior, with tests covering preservation, progressive cleanup, and rejected additions. ChangesLegacy tag validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/mountmate-core/src/storage.rs (1)
1137-1185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new preservation exception.
The reduction reaches the normal limit, so pre-PR validation would also pass. Start with at least
MAX_CONNECTION_TAGS + 2tags and reduce toMAX_CONNECTION_TAGS + 1; also cover an unchanged legacy overlong tag. This verifies storage actually permits preserve-only legacy states while rejecting additions.🤖 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 `@crates/mountmate-core/src/storage.rs` around lines 1137 - 1185, The test batch_preferences_allow_progressive_cleanup_of_legacy_tag_limits currently only exercises the normal MAX_CONNECTION_TAGS boundary. Initialize the server with at least MAX_CONNECTION_TAGS + 2 tags, reduce it to MAX_CONNECTION_TAGS + 1, and assert that an unchanged legacy overlong tag set is also accepted while any further tag addition is rejected.
🤖 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 `@crates/mountmate-app/src/main.rs`:
- Around line 8355-8357: Update the Message::BatchAddTag handling to use
existing-aware validation via validated_connection_tags_for_existing when
selected servers’ tags remain unchanged, or skip those no-op updates. Preserve
validation for actual tag changes so legacy over-limit servers already
containing the tag do not reject valid updates for other servers.
---
Nitpick comments:
In `@crates/mountmate-core/src/storage.rs`:
- Around line 1137-1185: The test
batch_preferences_allow_progressive_cleanup_of_legacy_tag_limits currently only
exercises the normal MAX_CONNECTION_TAGS boundary. Initialize the server with at
least MAX_CONNECTION_TAGS + 2 tags, reduce it to MAX_CONNECTION_TAGS + 1, and
assert that an unchanged legacy overlong tag set is also accepted while any
further tag addition is rejected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 587d20e0-835b-4638-a5a9-fc43b443d754
📒 Files selected for processing (4)
crates/mountmate-app/src/main.rscrates/mountmate-core/src/connection.rscrates/mountmate-core/src/model.rscrates/mountmate-core/src/storage.rs
| fn validated_connection_tags(tags: &[String], locale: Locale) -> Result<Vec<String>, String> { | ||
| validated_connection_tags_for_existing(tags, None, locale) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply existing-aware validation to batch no-op updates.
Message::BatchAddTag still calls this strict wrapper at Line 2048 even when Lines 2044-2046 leave a selected server’s tags unchanged. A legacy-over-limit server that already has the chosen tag then rejects the entire batch, including valid changes for other selected servers. Use the existing-aware validator there (or omit no-op updates).
Proposed fix
- let tags = validated_connection_tags(&tags, locale)?;
+ let tags = validated_connection_tags_for_existing(
+ &tags,
+ Some(server.tags.as_slice()),
+ locale,
+ )?;🤖 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 `@crates/mountmate-app/src/main.rs` around lines 8355 - 8357, Update the
Message::BatchAddTag handling to use existing-aware validation via
validated_connection_tags_for_existing when selected servers’ tags remain
unchanged, or skip those no-op updates. Preserve validation for actual tag
changes so legacy over-limit servers already containing the tag do not reject
valid updates for other servers.
Summary
Validation
git diff --checkSummary by CodeRabbit