Repository navigation
v0.24.0 fix: address review findings and image backfill follow-ups - #48
Merged
Merged
Conversation
- write_tweets: commit seen IDs only after the JSONL file is flushed and closed — a failed write left the batch IDs in the dedup set, so the extension's retry was counted as all-duplicates and silently dropped - resolve_output_dir: scope seen-ID sets per output directory — switching outputDir suppressed every tweet already captured into the old directory - explicit encoding='utf-8' on all text-mode file I/O — Windows locales (cp1252) raised UnicodeEncodeError on emoji, failing nearly every batch - load_seen_ids: glob-escape the directory, tolerate undecodable files (daemon crash-looped at startup), warn about iCloud-evicted placeholders - xtap_daemon: run server.shutdown() on a separate thread — calling it inside the signal handler deadlocked serve_forever until SIGKILL - xtap_daemon: sanitize article local_path regardless of the image toggle; write /log under the state lock - xtap_host: drain oversized native-messaging frames so the stream realigns instead of parsing payload bytes as headers - download_direct: add a socket timeout so a stalled CDN can't hang the download thread forever Repro tests included for each issue; daemon tests now isolate the default output dir so they never touch the real ~/Downloads/xtap.
- sendToHost: a 401/403 from the daemon (token rotated by a reinstall) now resets the transport so the next flush re-probes and fetches a fresh token via the native host — previously the same stale token was retried forever while tweets piled up until buffer overflow - flush: send batches without removing them from the buffer until the daemon acks, so service-worker death mid-POST can't lose them (the WAL entries are already cleared and seenIds blocked recapture); cap POSTs at 200 tweets so a backlog can't exceed the daemon's 10 MB body limit; guard against concurrent flushes double-sending - add a chrome.alarms backstop and flush at startup — the setTimeout flush timer dies with the SW (~30s idle), stranding the final partial batch of a session until a future session; requires 'alarms' permission - clear the stale '!' error badge once the transport recovers - cap the debug log buffer; JSON.stringify failures no longer throw into callers of console.log - popup/background: treat download status 'unknown' (daemon restarted) as terminal instead of polling forever with a stuck button - content-bridge: retry beacon discovery with setTimeout — rAF callbacks never fire in hidden tabs, dropping all events until the tab is shown - content-main: drop the stale GraphQL URL mapping when an XHR object is reused for a non-GraphQL request New vm-rig tests cover the auth reset, in-flight durability, concurrent flush, and alarm behavior.
X delivers the full quoted tweet inline at quoted_status_result.result, but the parser only recorded legacy.quoted_status_id_str — the quoted content (text, author, media) was discarded unless the user separately opened that tweet. Retweets already received this treatment; quotes did not. normalizeWithQuoted() now emits the quoted tweet alongside the quoting tweet on all extraction paths (timeline entries, modules, and TweetResultByRestId), unwrapping TweetWithVisibilityResults wrappers and skipping tombstones. For retweets, the quoted payload on the retweeted original is also captured. Existing dedup handles repeats. Golden fixtures regenerated: +8 quoted tweets in timeline-basic, +1 in user-highlights (verified each addition is a quoted payload in the raw fixture). E2E suite passes against the regenerated goldens.
…uard - bake XTAP_OUTPUT_DIR into the launchd plist / systemd unit at install time — the README told users to export it before launching the browser, but the daemon is a system service and never saw it - install.ps1: write a per-browser generated manifest file — Chrome and Firefox installs shared one file, so the second install stripped the other browser's allowed_origins/allowed_extensions and broke its host registration (it also clobbered the checked-in template) - manifest.test.mjs: regenerate manifest.firefox.json into a temp file and diff against the checked-in copy — the old test regenerated in-place before asserting, so a stale committed manifest could never fail CI (this mistake class has shipped before) - update stale 'writes captured tweets to JSONL' host descriptions (token-bootstrap only since the HTTP daemon split) and the README output-directory docs
Cross-model review (Claude + Codex adversarial passes, 4 specialists) on this branch surfaced follow-up issues; all confirmed findings fixed: - xtap_daemon: call server.server_close() after serve_forever() so in-flight handler threads (daemonic) are joined instead of being killed mid-JSONL-write at interpreter exit - background: propagate the HTTP status from sendToHost and split the flush batch on 413 — count-capped batches of large article tweets could exceed the daemon's 10 MB body limit and wedge the queue; a single tweet that alone exceeds the limit is dropped with a DROPPED_OVERSIZED trace event - install.sh: escape XTAP_OUTPUT_DIR for sed and XML, quote the systemd Environment= value (paths with spaces silently truncated and redirected output on Linux), and try-restart the systemd unit so re-installs actually apply the re-baked environment - xtap_core: realpath DEFAULT_OUTPUT_DIR so a symlinked home can't split one physical directory into two seen-ID sets; allowlist download_direct URLs to https://*.twimg.com, matching the image pipeline's trust boundary - tweet-parser: dedupe quoted tweets by id — a retweet-of-a-quote can carry the same payload on both wrapper and original, and articles bypass all downstream dedup layers - content-bridge: cap beacon polling at ~10s instead of polling forever when MAIN-world injection fails - background: startup flush also delivers buffered debug logs - manifest/README: document the alarms permission in Stealth Measures - .gitignore: ignore Windows installer generated manifests New regression tests: batch chunking, 413 split + oversized drop, quoted-tweet-inside-retweet, XHR stale mapping, download_direct allowlist and timeout. 153 Python + 114 JS + e2e all green.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- flush() removed acked batches positionally (splice(0,n)/shift), racing enqueueTweets' overflow eviction during the in-flight POST and silently deleting unsent tweets. removeDelivered() now removes by object identity. - DaemonHandler had no socket timeout while daemon_threads=False joins handler threads on shutdown — one idle pre-auth connection hung shutdown until SIGKILL. Class-level timeout (default 15s, XTAP_CONN_TIMEOUT_S override) lets stalled connections expire. - Regression tests for both, each verified to fail against the old code.
mkubicek
changed the title
The per-connection socket timeout is per blocking read, not a total request deadline — a peer trickling one byte per read window keeps its non-daemon handler thread alive forever, and server_close() would join it until launchd/systemd SIGKILLs mid-write (cross-model review finding). server_close() now runs under SHUTDOWN_GRACE_S (default 10s, env XTAP_SHUTDOWN_GRACE_S): legit writes finish well inside the grace, stragglers get force-exited before the service manager's kill window. Regression tests: trickle client must not block SIGTERM exit (verified to fail against an unbounded join), and a stalled oversized upload must still get its 413 within the drain idle timeout. Shared daemon subprocess boilerplate extracted into _start_daemon(). Both env vars documented in README and AGENTS.
- download-status cleanup: done/error/unknown clear activeDownloads, downloading keeps it, other IDs untouched - debugLog: 5000-line cap, String() fallback for circular args, no-op while disabled - content-bridge (new file): beacon relay, setTimeout-not-rAF retries, 200-attempt give-up bound, malformed payloads, DOMContentLoaded path - startup flush (new rig keeping the Init block): restored buffer is POSTed before any timer/alarm fires, alarm backstop armed, acked batch leaves persisted state - xtap_core: load_seen_ids OSError branch, XTAP_MAX_VIDEO_MB boundaries (0/negative disable, junk falls back to default), _downloads leak in the ytdlp template test fixed with try/finally
mkubicek
marked this pull request as ready for review
July 4, 2026 21:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
IMAGE_BACKFILLfor backfill re-emissions.Fixes #38.
Fixes #39.
Fixes #40.
Fixes #41.
Fixes #42.
Fixes #43.
Why
The capture path had several review-found edge cases where service-worker shutdown, oversized batches, stale daemon tokens, or image-backfill retries could produce confusing user-visible state or missed recovery behavior. This PR keeps the browser-side path passive while making delivery, diagnostics, and image-backfill accounting more predictable.
Validation
.venv/bin/python -m pytest tests/ -vnode --test tests/*.test.mjsPre-landing review fixes (2026-07-04)
Full multi-model review (Claude + Codex + 5 specialists) found two critical issues, both fixed with regression tests verified to fail against the old code:
splice(0, n)/shift()); a buffer-overflow eviction during the in-flight POST shifted the buffer front, so the ack-splice silently deleted never-sent tweets (ids already in seenIds — permanent loss). Now removed by object identity viaremoveDelivered().daemon_threads = Falsejoins handler threads on shutdown, butDaemonHandlerhad no socket timeout — one idle pre-auth connection blocked SIGTERM until SIGKILL. Nowtimeout = 15s (env overrideXTAP_CONN_TIMEOUT_S).Version bumped to 0.24.0 (new feature: quoted-tweet capture).
Validation: 126 JS tests, 172 Python tests, all passing after fixes + bump.
Final review round (2026-07-04, pre-merge)
Cross-model delta review (Codex, high reasoning) found the connection socket timeout is per-read, not a total deadline — a trickle client (1 byte per window) could still pin a handler thread and hang shutdown. Fixed with a bounded shutdown join:
server_close()getsXTAP_SHUTDOWN_GRACE_S(default 10s) and force-exits past it. Verified by a regression test that fails against an unbounded join.Coverage pass closed all review-flagged gaps: download-status
unknowncleanup, debugLog cap + stringify fallback, content-bridge (new test file: beacon retry/give-up/relay), startup flush of a restored buffer (new rig exercising the Init block),load_seen_idsOSError branch,XTAP_MAX_VIDEO_MBboundaries. Both new daemon env vars documented in README/AGENTS.Final: 141 JS + 180 Python tests, all passing (was 108 + 171 at branch start).