fix(tweaks): proxy host validation + save-needs-two-taps#576
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughThis PR adds RFC-compliant proxy host validation using IPv4/IPv6/hostname patterns across ViewModel and UI layers, with real-time error feedback and 13 localized messages. Additionally, it includes unrelated UI refinements: platform-conditional scrollbar visibility, snackbar dismissal actions, and refactored profile options rendering. ChangesProxy Host Validation
Settings UI Refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes two bugs in the Tweaks → Network → Proxy form: (1) host accepted any non-blank text — now validated against hostname/IPv4/IPv6 patterns with clear error messages across 13 locales; (2) Save required two taps on Android — fixed by calling Confidence Score: 3/5Not safe to merge as-is — the IPv6 regex lets invalid hosts like bare One P1 defect in both TweaksViewModel.kt and Network.kt: IPV6_PATTERN accepts degenerate inputs, allowing an invalid proxy config to be saved. The two-tap and host-rejection fixes are otherwise sound. TweaksViewModel.kt and Network.kt — both define IPV6_PATTERN and need the regex tightened before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User types in Host field] --> B{hostValue.isNotEmpty?}
B -- No --> C[isHostInvalid = false\nSave disabled]
B -- Yes --> D{isLikelyValidProxyHost?}
D -- No --> E[isHostInvalid = true\nShow inline error\nSave disabled]
D -- Yes --> F[isHostInvalid = false]
F --> G{Port valid 1..65535?}
G -- No --> H[Save disabled]
G -- Yes --> I[Save/Test enabled]
I --> J[User taps Save/Test]
J --> K[clearFocus — IME commits input]
K --> L[TweaksAction dispatched to VM]
L --> M{isValidProxyHost in VM?}
M -- No --> N[OnProxySaveError / OnProxyTestError snackbar]
M -- Yes --> O[ProxyConfig persisted / tested]
style E fill:#f88,color:#000
style N fill:#f88,color:#000
style O fill:#8f8,color:#000
Reviews (1): Last reviewed commit: "ui: hide stars when logged out, scrollba..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // optional `::` shortcut). Permissive on canonical form because | ||
| // proxy clients normalize it; we only need to reject "looks | ||
| // wrong" inputs like `not a url`. | ||
| private val IPV6_PATTERN = Regex("^[0-9A-Fa-f:]+$") |
There was a problem hiding this comment.
:
IPV6_PATTERN = Regex("^[0-9A-Fa-f:]+$") matches any string consisting only of hex characters and colons — including : alone, ::::, abc:, :def, or a:b:c:d:e:f:0:1:2:3 (9 groups). Because ":" satisfies both contains(":") and IPV6_PATTERN.matches(":"), a user entering : as their proxy host passes both the UI and VM validators, the Save button enables, and an invalid config is persisted — silently breaking all proxy-scoped network traffic until manually corrected.
The same pattern is duplicated in Network.kt, so both paths are affected. A minimal fix:
private val IPV6_PATTERN = Regex(
"^(?:[0-9A-Fa-f]{0,4}:){2,7}[0-9A-Fa-f]{0,4}$"
)This requires at least two colon-separated groups (covering :: compressed form) while staying permissive about canonical form as the comment intends.
|
|
||
| /** |
There was a problem hiding this comment.
isLikelyValidProxyHost and its three companion regex patterns (IPV4_PATTERN, IPV6_PATTERN, HOSTNAME_PATTERN) are a verbatim copy of the logic already in TweaksViewModel. If either side is updated (e.g., the IPv6 fix above) both files must be kept in sync manually. Consider extracting the validator to a shared module (e.g., core/domain) or at minimum referencing the same object-level constants.
| } | ||
|
|
||
| Spacer(Modifier.height(4.dp)) |
There was a problem hiding this comment.
Spacer(Modifier.height(4.dp)) sits outside the if (isUserLoggedIn) block, so it renders an invisible 4 dp gap at the top of the options list whenever the user is logged out. Move the spacer inside the block to restore the original conditional layout.
| } | |
| Spacer(Modifier.height(4.dp)) | |
| if (isUserLoggedIn) { | |
| OptionCard( | |
| icon = Icons.Default.Star, | |
| label = stringResource(Res.string.stars), | |
| description = stringResource(Res.string.profile_stars_description), | |
| onClick = { | |
| onAction(ProfileAction.OnStarredReposClick) | |
| }, | |
| ) | |
| Spacer(Modifier.height(4.dp)) | |
| } |
1 participant
Two bugs in the Tweaks → Network → Proxy form found in device-side bug-bash (#534 follow-up):
Host accepted any non-blank text (
not a url, garbage with spaces, etc.). Only port had real-time validation. Added a hostname / IPv4 / IPv6 validator (isValidProxyHostin VM, mirrored asisLikelyValidProxyHostin Network.kt for the real-timeisErrorhighlight). Save + Test paths now reject malformed hosts with a clear error (proxy_host_invalidstring across 13 locales).Save required two taps — first tap lost IME focus from the host/port field, second tap actually fired the click. Wrapped both Save and Test
onClickto callLocalFocusManager.clearFocus()first so the IME commits pending input synchronously.Bonus:
isFormValidnow incorporates the host validity gate so the Save button stays disabled until the host parses cleanly — matches the port-side UX.Summary by CodeRabbit
New Features
Improvements
Localization