Preserve Locked State for Artifacts #2575
Conversation
…anner doesn't detect locks
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/gi/db/src/Database/DataManagers/ArtifactDataManager.ts (2)
218-219: LGTM! Consider adding a clarifying commentThe logic correctly preserves the lock state of existing artifacts when updating them with unlocked artifacts. This implementation aligns well with the PR objective.
Consider expanding the comment to be more explicit about the behavior:
- // Preserve existing lock state for artifacts reported as unlocked or when the scanner doesn't detect locks ("lock": false JSON) + // Preserve existing lock state in two cases: + // 1. When the imported artifact is explicitly unlocked (lock: false) + // 2. When the scanner doesn't detect locks (missing "lock" property in JSON)
218-219: Well-designed data preservation approachThe implementation follows good architectural principles by:
- Preserving user preferences during data updates
- Maintaining backward compatibility with existing import formats
- Gracefully handling edge cases with scanners that don't detect locks
This approach could serve as a pattern for preserving other user-set states during future import operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/gi/db/src/Database/DataManagers/ArtifactDataManager.ts(1 hunks)
🔇 Additional comments (1)
libs/gi/db/src/Database/DataManagers/ArtifactDataManager.ts (1)
218-219: Verify lock state handling consistency
While the implementation looks correct, let's verify that lock state handling is consistent across the codebase.
✅ Verification successful
Let me try a different approach to verify the lock state handling.
Let me try one more search to specifically focus on the artifact lock handling in the codebase.
Based on the search results, I can see that the lock state handling is consistently implemented across the codebase. The key findings show:
- The lock state is properly preserved in
ArtifactDataManager.tswhen importing artifacts - Lock state is consistently used in UI components for toggling and displaying lock status
- Lock state is properly handled in artifact filtering and deletion operations
- Lock state is included in artifact interfaces and types
Let me generate the final response:
Lock state handling is correctly implemented
The implementation preserves existing lock states appropriately when importing artifacts, and the lock state is consistently handled across artifact operations in the codebase:
- Lock state preservation during import considers both explicit unlocks and scanner limitations
- Lock state is properly enforced in deletion operations
- Lock state is consistently toggled and displayed in UI components
- Lock state is properly typed in interfaces and database operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify lock state handling across the codebase
# Check for all lock state manipulations
echo "Checking lock state handling patterns:"
rg -A 2 -B 2 'lock[^a-zA-Z]' --type typescript
# Check for potential lock state serialization/deserialization
echo -e "\nChecking lock state serialization patterns:"
rg -A 2 -B 2 '(JSON\.|parse|stringify).*lock' --type typescript
# Look for any GOOD-related type definitions that might affect lock state
echo -e "\nChecking GOOD-related type definitions:"
ast-grep --pattern 'interface $name {
$$$
lock$_
$$$
}'
Length of output: 533
Script:
#!/bin/bash
# Search for lock-related patterns without specifying file type
echo "Checking lock state handling patterns:"
rg -A 2 -B 2 'lock[^a-zA-Z]'
# Check for GOOD-related interfaces and types
echo -e "\nChecking GOOD-related definitions:"
rg -A 5 -B 1 'interface.*GOOD|type.*GOOD'
# Check for artifact-related lock handling
echo -e "\nChecking artifact lock handling:"
rg -A 2 -B 2 'art(ifact)?\.lock'
Length of output: 93440
Script:
#!/bin/bash
# Search for artifact lock handling in the database layer
echo "Checking artifact lock handling in database:"
rg -A 3 -B 3 'lock.*art|art.*lock' --type ts
# Search for artifact-related interfaces and types
echo -e "\nChecking artifact interfaces and types:"
rg -A 3 -B 1 'interface.*Artifact|type.*Artifact' --type ts
# Search for GOOD-related imports and exports
echo -e "\nChecking GOOD-related imports/exports:"
rg -A 2 -B 2 'import.*GOOD|export.*GOOD' --type ts
Length of output: 67592
|
[frontend] [Fri Dec 13 09:35:45 UTC 2024] - Deployed f84442b to https://genshin-optimizer-prs.github.io/pr/2575/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Dec 17 06:22:00 UTC 2024] - Deployed c57295e to https://genshin-optimizer-prs.github.io/pr/2575/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Dec 18 05:01:23 UTC 2024] - Deployed f0d5a53 to https://genshin-optimizer-prs.github.io/pr/2575/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Dec 18 05:19:36 UTC 2024] - Deployed 6911762 to https://genshin-optimizer-prs.github.io/pr/2575/frontend (Takes 3-5 minutes after this completes to be available) [Wed Dec 18 19:49:10 UTC 2024] - Deleted deployment |
| @@ -215,6 +215,8 @@ export class ArtifactDataManager extends DataManager< | |||
| importArt = { | |||
| ...art, | |||
| location: hasEquipment ? art.location : match.location, | |||
| // Preserve existing lock state for artifacts reported as unlocked or when the scanner doesn't detect locks ("lock": false JSON) | |||
| lock: art.lock === false ? match.lock : art.lock, | |||
There was a problem hiding this comment.
I would preferred if we do
const hasLock = artifacts.some((a)=>a.lock)
...
lock: hasLock? art.lock : match.lock
similiar to hasEquipment
since the current PR logic would not allow unlocking of artifacts from the scanned data (if user unlocked in game, and scanner scanned it, but it's locked in GO)
There was a problem hiding this comment.
I made the requested change and confirmed with testing where having all the lock properties as false keeps the original lock statuses (locked/unlocked), and if there are locks present, artifacts can become be locked or unlocked
Previously thought to not overwrite locks, but now it can if locks are present. If no locks are present then assume no lock detection and do not modify the existing lock statuses
Describe your changes
When existing locked artifacts get updated by one that is unlocked, the locked state is preserved
Issue or discord link
Testing/validation
I tested it by uploading a database with unlocked artifacts, manually locked them in the site, and then re-uploaded the same JSON batch to see if the artifacts I locked became unlocked or not
Additionally tested for if the lock property is missing from the new JSON upload ("lock": false/true) by removing it, and the artifact remains in its former state (locked/unlocked)
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes