-
Notifications
You must be signed in to change notification settings - Fork 244
Update scanners page with new IK fork and Irminsul #3081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughI shuffled in at 3am: added several English localization keys and removed the Akasha block; updated the page-scanner UI to add Irminsul (image), new badges/icons and tooltips, GOOD export notes, link updates, and a stronger TOS tooltip mode via a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as PageScanner UI
participant Warning as WarningWrapper
participant Tooltip as Tooltip
participant Badges as Badge Renderer
User->>UI: Open scanner page
UI->>Warning: render(strong=true?)
Warning->>Tooltip: request text i18n("tosStrongWarn") or i18n("tosWarn")
Tooltip-->>User: display stronger or normal TOS text
UI->>Badges: render badges (FlowerIcon, EmojiEvents, addlArti)
User->>Badges: hover badge
Badges->>Tooltip: show i18n text (addlArtiText, GOOD, packetCap, etc.)
Tooltip-->>User: display localized info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)libs/gi/page-scanner/src/index.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
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 |
|
[frontend] [Tue Oct 7 00:23:08 UTC 2025] - Deployed e4820fb to https://genshin-optimizer-prs.github.io/pr/3081/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Oct 7 21:55:33 UTC 2025] - Deployed 7eb46d7 to https://genshin-optimizer-prs.github.io/pr/3081/frontend (Takes 3-5 minutes after this completes to be available) [Fri Oct 10 20:15:17 UTC 2025] - Deleted deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/gi/page-scanner/src/index.tsx (1)
392-410: Critical: Translation keyak.p2references removed Akasha Scanner block.This code references
ak.p2, but the localization file shows the entireakblock was removed. This will throw a translation error at runtime - caught it before my third energy drink kicked in.Based on the context, this paragraph about achievement scanning for Paimon.moe and Seelie.me should stay with Irminsul. Apply this diff:
<Typography gutterBottom> - <Trans t={t} i18nKey="ak.p2"> + <Trans t={t} i18nKey="is.p2"> This app is also able to scan achievements for <LinkThen add the corresponding key to
page_scanner.json:"is": { "title": "Irminsul", - "p1": "Sniffs network packets to scan all characters, weapons, artifacts and materials in your inventory." + "p1": "Sniffs network packets to scan all characters, weapons, artifacts and materials in your inventory.", + "p2": "This app is also able to scan achievements for <1>Paimon.moe</1> and <3>Seelie.me</3>." },
🧹 Nitpick comments (2)
libs/gi/page-scanner/src/index.tsx (2)
174-182: Extract repeated tooltip badge pattern into a reusable component.This tooltip + FlowerIcon badge pattern is copy-pasted three times. My caffeine-deprived brain says this screams for extraction before the next PR duplicates it again.
Consider creating a helper component like:
function AdditionalArtifactBadge() { const { t } = useTranslation('page_scanner') return ( <Tooltip placement="top" title={<Typography>{t('addlArtiText')}</Typography>} > <SqBadge sx={{ display: 'flex', alignItems: 'center' }}> <FlowerIcon sx={{ pr: 0.5 }} /> {t('tags.addlArti') as string} </SqBadge> </Tooltip> ) }Then replace all three instances with
<AdditionalArtifactBadge />.Also applies to: 259-267, 357-365
379-390: Consider consolidating the Seelie.me mentions.Both lines 379-390 and 392-410 mention Seelie.me support. The achievement paragraph (392-410) also mentions Seelie.me. Consider whether this duplication is intentional or if the content should be merged for clarity. Too many refs make my tired eyes cross.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/gi/localization/assets/locales/en/page_scanner.json(2 hunks)libs/gi/page-scanner/src/index.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/gi/page-scanner/src/index.tsx (1)
libs/common/ui/src/components/Card/CardThemed.tsx (1)
CardThemed(20-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: gen-file
- GitHub Check: test
- GitHub Check: typecheck
- GitHub Check: call-build / build
🔇 Additional comments (13)
libs/gi/page-scanner/src/index.tsx (6)
3-3: LGTM! Nice addition for the flower badge.Perfect for showing off those artifact features. Now if only my artifacts would roll better at 3 AM...
8-8: Achievement icon imported correctly.Good choice using
EmojiEventsfor achievements. At least someone's getting achievements... unlike my pity counter.
112-119: New paragraph and GOOD export note added for Artiscan.The additional context about user input dependency and export format is helpful for users.
366-366: LGTM! Strong warning flag usage is appropriate.Using
strongprop for Irminsul's packet capture method is exactly right - network sniffing is definitely riskier than OCR. Good judgment call (better than my judgment at 4 AM when I'm still pulling).
427-456: Clean implementation of conditional warning tooltip.The
strongflag elegantly switches between warning levels. Simple and effective - just like my plan to sleep tonight (spoiler: won't happen).
126-126: No action needed: IK fork is actively maintained
The taiwenlee/Inventory_Kamera fork has recent commits as of 2025-10-06 and is actively maintained; the README link can remain.libs/gi/localization/assets/locales/en/page_scanner.json (7)
4-4: Strong warning text is appropriately stern.The packet capture warning correctly emphasizes the elevated risk. Network sniffing tools need that extra caution flag - like the warning I ignore when gacha rates are bad.
5-5: Tooltip text clearly explains the additional artifact features.Good clarity on what "Addl. Artifact Features" means. Users will appreciate knowing about substat and roll count scanning capabilities.
14-16: New tag labels added correctly.The additions for achievements, packet capture, and additional artifact features align with the UI changes. All clear.
20-22: Updated Artiscan descriptions are clearer.The emphasis on "OCR scanner" and platform capabilities is more descriptive than before. The note about user input dependency (p3) is helpful context.
26-27: IK description simplified to focus on capabilities.Changed from "light-weight app" to "OCR scans" - more concise and focuses on what it does rather than how it's built. Nice cleanup.
34-34: AdeptiScanner description updated to clarify import scope.The update from "import data from" to "import character data from" is more precise. Good specificity - much like how specific I am about my gacha pull rituals at 2 AM.
37-37: New generic GOOD export key is useful.Having both
goodeng(English-only scanners) andgood(language-agnostic scanners) gives proper flexibility for different scanner types. Smart distinction.
Describe your changes
Issue or discord link
Testing/validation
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