fix: update proxy status handling (#115) - #132
Conversation
LeyckerS
left a comment
There was a problem hiding this comment.
Welcome, and thank you for taking this one. The three-state idea is exactly right — "no file", "file with nothing usable in it" and "N loaded" really are different things and the chip currently collapses the first two. That is the part of #115 that mattered and you got it.
The delivery needs work, though, and one of the problems is invisible to CI because nothing tests the JavaScript.
Blocking
1. snapshot()["proxies"] changes from a number to an object, and that breaks the caller.
web/app.js:998 guards the update with a value comparison:
if (snap.proxies != null && snap.proxies !== ui.lastProxies) setProxies(snap.proxies);With an integer that works. With an object, !== compares references — and _get_proxy_status() builds a fresh dict on every call, so the comparison is always true and setProxies runs on every poll, forever. POLL_MS is 80, so that is 12.5 pointless DOM updates a second for the life of the run.
snapshot() is the contract both front-ends read (that is what #57 is about), so changing a key's type is not a small edit. Add a new key and leave proxies as the integer it has always been.
2. You already added that new key — and then kept both.
'proxies': self._get_proxy_status(),
"proxy_info": {"status": proxy_status, "count": proxy_count},The same information, twice, in two shapes. One of them will drift from the other. Keep proxy_info, restore "proxies": self._proxies.
Related: start() at line 535 still returns "proxies": self._proxies as an integer. Right now the same key name means two different types in one API.
3. The hardcoded string.
chip.textContent = "0 valid proxies found"; // am just gonna use a hardcoded string for now, someone else add translation keys later?The GUI is EN/IT and every other label goes through T(). The dictionaries are at the top of app.js — add a key beside no_proxy and proxy_n, both languages. It is two lines, and leaving it for someone else means it ships in English to Italian users.
4. chip.className is no longer set.
The old code did chip.className = n ? "chip mint" : "chip". Yours sets only textContent, so the chip keeps whatever class it had — the mint colouring that distinguishes "proxies active" from "none" stops tracking the state. empty_file probably wants its own treatment too, since it is a warning rather than a neutral state.
Worth fixing while you are in there
snapshot() now does disk I/O. _get_proxy_status() runs os.path.exists and os.path.getmtime on every call — 12.5 stat calls a second at idle. The mtime check keeps load() from re-running, which is good thinking, but the stats themselves are on the hot path.
Worse, load() mutates the global _PROXY_POOL as a side effect of what is meant to be a read-only status query. If you need the count without loading, read the file and count usable lines; if you genuinely want to load, do it somewhere that is not called 12 times a second.
The isinstance guard is unnecessary. ProxyPool.load is annotated -> tuple[int, int] (moon_download.py:109). It cannot return a list. Checking the signature beats guarding against a shape that does not exist — that is the house rule and it is in CONTRIBUTING.md.
Two stray blank lines were added above class Engine:. Unrelated to the fix; please drop them.
To be clear about why this is a round trip and not a rejection
The diagnosis in #115 was right and your reading of it was right. Everything above is mechanical — restore one key's type, add a dictionary entry, put the class back, move the file check off the poll path. None of it touches the idea.
Take your time; the issue stays yours.
|
Thanks for taking the time to review this and for the detailed suggestions! I’ve pushed an update addressing all the points (API contract alignment, GUI translations, and the proxy I/O optimization). Please let me know if anything else needs tweaking or any additional changes! |
LeyckerS
left a comment
There was a problem hiding this comment.
You addressed every point, and the JavaScript half is now right — the guard compares primitives instead of object identity, chip.className is back with a warn state for empty_file, both dictionaries have the key, and the throttle keeps snapshot() off the disk. That is a thorough second pass.
One thing left, and the ambiguity is mine. I wrote "read the file and count usable lines". You counted non-empty non-comment lines, which is a fair reading and is not the same thing.
What breaks
ProxyPool.load() accepts exactly three shapes (moon_download.py:114-141):
- anything starting with
http://,https://orsocks ip:port- four colon-separated fields, either
ip:port:user:passoruser:pass:ip:port
Everything else increments skipped and is discarded. Your count treats all of them as proxies.
Concretely, with a proxies.txt that reads:
http://1.2.3.4:8080
this-line-is-garbage
your check reports _proxies = 2, status loaded, and the chip says "2 proxies". Press Start and load() reports 1 loaded, 1 skipped, and the number silently changes.
The worse case is a file where every line is malformed: the chip says "3 proxies", the user believes they are covered, and empty_file — the state this PR exists to introduce — never fires. That is the case #115 is actually about, and it is the one that stays broken.
And it matters more than a wrong number usually would: #112 merged yesterday documenting that proxies are the only thing standing between the user's address and the file host for the download half. A chip that confidently claims coverage nobody has is worse than the "no proxy" it replaces.
The fix, and it removes a duplication rather than adding one
Do not re-implement the parsing in moon_engine.py — that is the two-copies problem this repo keeps having (#41, #97, #99 are all the same disease).
Lift the per-line test out of load() into something both can call, e.g. in moon_download.py:
def count_usable_proxies(path: str) -> tuple[int, int]:
"""(usable, skipped) without building or mutating the pool."""Have load() use the same predicate for its own parsing, so the two can never disagree. Then _get_proxy_status() calls it and gets the number start() will produce, by construction.
That also fixes the second half of the problem, which I should have named in the first review: self._proxies currently means "line count" before a run and "parsed count" during one. One field, two meanings, and the value changes under the user when they press Start.
Two small things
time.time() for the throttle — moon_engine.py uses time.monotonic() everywhere else for elapsed measurements, and for good reason: a wall-clock jump backwards (NTP correction, DST) makes now - self._last_proxy_check negative, so the check never runs again until the clock catches up.
Trailing whitespace on several of the new JS lines, and a double blank line in setProxies. Nothing enforces it — ruff's whitespace rules are preview-only and ruff.toml does not enable them — but the file is otherwise clean.
Sorry for the second round; the first review sent you down this path. The remaining change is small and the issue stays yours.
|
Thanks for the review! I've pushed an update addressing all your feedback (fixed the duplicate proxy parsing, switched to time.monotonic(), and cleaned up the trailing whitespaces and formatting). Let me know if it looks good to go! |
LeyckerS
left a comment
There was a problem hiding this comment.
Third round, and you got there. Merging — thank you for sticking with it.
Extracting parse_proxy_line() was the right shape, and it does more than fix the count. load() now calls it too, so the status check and the real load cannot disagree by construction rather than by two implementations happening to agree today. That is the same disease as #41, #97 and #99, caught before it started rather than after.
I checked it against the case that failed last time. A file containing:
http://1.2.3.4:8080
this-line-is-garbage
now reports 1 usable, 1 skipped and status loaded with a count of 1 — where the previous revision said "2 proxies" and would have had the user believing they were covered when half the file was junk. Confirmed against load()'s own three accepted shapes: a URL scheme, ip:port, or four colon-separated fields. A three-part line is skipped by both.
.chip.warn exists in styles.css:304 and is amber, so empty_file gets a genuinely distinct treatment rather than falling back to the neutral chip. Good catch using the class that was already there.
time.monotonic() for the throttle is right — a wall-clock jump backwards would have frozen the check until the clock caught up.
Everything else from the first two rounds holds: proxies stays the integer it always was so nothing downstream breaks, proxy_info carries the new shape, the poll guard compares primitives instead of object identity, and both dictionaries have the key so an Italian user sees Italian.
The reason this took three rounds is worth stating plainly: the first was a real contract break, the second came from a loose sentence in my own review, and only this one is on nobody. The idea you started with — three states rather than two — was right from the first commit and never changed.
Closes #115. Welcome.
|
Thank you so much for the detailed feedback and guidance throughout the process, LeyckerS! I really appreciated digging into the design with you and getting the logic airtight. It was a great learning experience (super excited to see this merged). I really enjoyed working on this project and would love to contribute more in the future. Looking forward to working together again! |
Description
Updates proxy status reporting to distinguish between three distinct states (
none_configured,empty_file, andloaded). Refactorsmoon_engine.pyto return structured proxy info and updatesweb/app.jsto render the appropriate proxy status chip in the GUI. Fixes #115.Type of change
Checklist
applied the equivalent change to
moon_cli.pydescription