feat(watch): theatre/venue prefer-filter on /watch - #85
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe ChangesTheatre filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db.ts`:
- Line 63: Update the migration around the ALTER TABLE statement in src/db.ts to
ignore only the duplicate-column error for theatre_filter. Inspect the caught
error’s code/message for that specific case, and re-throw all other migration
failures so addWatch does not continue after locked, read-only, or otherwise
invalid database errors.
In `@src/index.ts`:
- Around line 51-58: Update the one-shot watch creation validation around the
open.length branch to filter open shows using the requested formatFilter,
theatreFilter, and dayFilter predicates before deciding tickets are already on
sale. Ensure the watch is saved when no show matches all requested filters,
while preserving the existing already-on-sale response for matching live shows.
- Around line 224-235: Add a per-cycle shared promise cache for showtime lookups
keyed by city, event code, and date, and update the fresh-date filtering logic
around fetchShowtimes to reuse the cached promise instead of calling
fetchShowtimes directly. Ensure watches with identical lookup keys share one
request while distinct keys remain independent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8fa8cc9-31d0-40d5-b6b4-6133774c7a8b
📒 Files selected for processing (8)
README.mdsrc/db.test.tssrc/db.tssrc/filters.tssrc/index.tssrc/messages.tssrc/register.tssrc/theatre-filter.test.ts
| // Migration: add filter columns if the table predates them. | ||
| try { db.exec("ALTER TABLE watches ADD COLUMN format_filter TEXT"); } catch { /* already exists */ } | ||
| try { db.exec("ALTER TABLE watches ADD COLUMN day_filter TEXT"); } catch { /* already exists */ } | ||
| try { db.exec("ALTER TABLE watches ADD COLUMN theatre_filter TEXT"); } catch { /* already exists */ } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not suppress unexpected migration errors.
Line 63 treats every ALTER TABLE failure as an existing column. If the database is locked or read-only, the migration is skipped. The later insert then fails and addWatch returns the duplicate-watch message.
Ignore only the duplicate-column case. Re-throw every other error.
Proposed fix
-try { db.exec("ALTER TABLE watches ADD COLUMN theatre_filter TEXT"); } catch { /* already exists */ }
+const columns = db.query("PRAGMA table_info(watches)").all() as { name: string }[];
+if (!columns.some((column) => column.name === "theatre_filter")) {
+ db.exec("ALTER TABLE watches ADD COLUMN theatre_filter TEXT");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { db.exec("ALTER TABLE watches ADD COLUMN theatre_filter TEXT"); } catch { /* already exists */ } | |
| const columns = db.query("PRAGMA table_info(watches)").all() as { name: string }[]; | |
| if (!columns.some((column) => column.name === "theatre_filter")) { | |
| db.exec("ALTER TABLE watches ADD COLUMN theatre_filter TEXT"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db.ts` at line 63, Update the migration around the ALTER TABLE statement
in src/db.ts to ignore only the duplicate-column error for theatre_filter.
Inspect the caught error’s code/message for that specific case, and re-throw all
other migration failures so addWatch does not continue after locked, read-only,
or otherwise invalid database errors.
| const theatreFilter = normaliseTheatres(i.options.getString("theatre")); | ||
| try { | ||
| const parsed = parseWatchUrl(i.options.getString("link", true)); | ||
| const dateOpt = i.options.getString("date")?.trim(); | ||
| // "any" (or a link with no date and no date option) subscribes to the movie: | ||
| // ping me every time a NEW date unlocks, rather than watching one date. | ||
| const wantsAny = dateOpt ? /^(any|all|every|new)$/i.test(dateOpt) : !parsed.date; | ||
| if (wantsAny) return void (await subscribeToMovie(i, parsed, formatFilter, dayFilter)); | ||
| if (wantsAny) return void (await subscribeToMovie(i, parsed, formatFilter, dayFilter, theatreFilter)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply filters during one-shot creation validation.
Line 51 accepts theatreFilter, but the open.length branch at Lines 95-98 checks unfiltered shows. If only INOX has shows and the user requests theatre:PVR, the command reports that tickets are already on sale and does not save the watch.
Filter open with the requested format, theatre, and day predicates before the already-on-sale response. Save the watch when no matching show is live.
Proposed fix
- if (open.length) {
+ const matchingOpen = open.filter((show) =>
+ (!formatFilter || matchesFormat(show.attributes, formatFilter))
+ && (!dayFilter || matchesDay(show.showDateCode, dayFilter))
+ && (!theatreFilter || matchesTheatre(show.venueName, show.venueCode, theatreFilter)),
+ );
+
+ if (matchingOpen.length) {
return void i.editReply(
- msg.alreadyOnSale({ title, city: target.city, date: target.date, shows: open, url: showtimesUrl(target) }),
+ msg.alreadyOnSale({ title, city: target.city, date: target.date, shows: matchingOpen, url: showtimesUrl(target) }),
);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 51 - 58, Update the one-shot watch creation
validation around the open.length branch to filter open shows using the
requested formatFilter, theatreFilter, and dayFilter predicates before deciding
tickets are already on sale. Ensure the watch is saved when no show matches all
requested filters, while preserving the existing already-on-sale response for
matching live shows.
| // Format / theatre filters: only announce dates that actually have matching shows. | ||
| // Costs an extra fetchShowtimes per fresh date — only spent when one of them is set, | ||
| // and a single fetch covers both so adding a theatre filter never doubles the requests. | ||
| let matchedFormats: string[] = []; | ||
| if (w.format_filter && freshDates.length) { | ||
| if ((w.format_filter || w.theatre_filter) && freshDates.length) { | ||
| const kept: string[] = []; | ||
| for (const d of freshDates) { | ||
| try { | ||
| const shows = showsOnDate((await fetchShowtimes({ city: w.city, slug: w.slug, eventCode: w.event_code, date: d })).shows, d); | ||
| const hits = shows.filter((s) => matchesFormat(s.attributes, w.format_filter!)); | ||
| const hits = shows.filter((s) => | ||
| (!w.format_filter || matchesFormat(s.attributes, w.format_filter)) | ||
| && (!w.theatre_filter || matchesTheatre(s.venueName, s.venueCode, w.theatre_filter)), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant polling and BookMyShow request functions.
ast-grep outline src/index.ts --items all --match 'checkSubscription|bookableDatesCached|fetchShowtimes'
ast-grep outline src/bms.ts --items all --match 'fetchShowtimes|fetchPage'
# Inspect whether fetchShowtimes or fetchPage already deduplicates in-flight requests.
rg -n -C 5 'checkSubscription|bookableDatesCached|fetchShowtimes|fetchPage|Promise<|new Map' \
src/index.ts src/bms.tsRepository: Ishannaik/seatsniper
Length of output: 11338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the complete cycle-cache implementation and the poll scheduling.
sed -n '360,410p' src/bms.ts
sed -n '175,255p' src/index.ts
sed -n '430,490p' src/index.ts
rg -n -C 4 'beginCycle\(|Promise\.all|allWatches\(|checkWatch\(' src/index.ts src/*.test.ts tests 2>/dev/null || true
# Check whether any shared cache covers date-specific fetchShowtimes calls.
rg -n -C 3 'fetchShowtimes\(|cycle\.|Map<string, Promise|Cached' src/index.ts src/bms.tsRepository: Ishannaik/seatsniper
Length of output: 12913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use a small read-only verifier to model the request keys used by the current
# implementation for two watches that share a movie but have filter-specific
# fresh dates.
python3 - <<'PY'
from pathlib import Path
src = Path("src/index.ts").read_text()
bms = Path("src/bms.ts").read_text()
print("direct_date_fetches_in_index", src.count("await fetchShowtimes({ city: w.city, slug: w.slug, eventCode: w.event_code, date: d })"))
print("date_fetch_cache_key_present", "date: d" in bms and "cycle.get" in bms)
print("begin_cycle_calls", src.count("beginCycle()"))
PYRepository: Ishannaik/seatsniper
Length of output: 244
Coalesce fresh-date showtime lookups across watches.
Line 232 calls fetchShowtimes directly and bypasses the existing per-cycle cache. Multiple watches for the same movie and date can therefore issue duplicate BookMyShow requests. Add a shared promise cache keyed by city, event code, and date.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 224 - 235, Add a per-cycle shared promise cache
for showtime lookups keyed by city, event code, and date, and update the
fresh-date filtering logic around fetchShowtimes to reuse the cached promise
instead of calling fetchShowtimes directly. Ensure watches with identical lookup
keys share one request while distinct keys remain independent.
Source: Coding guidelines
Ishannaik
left a comment
Ishannaik
left a comment
There was a problem hiding this comment.
Theatre filter is a real user need and the semantics are documented well (substring on name or code; new-cinema alerts are theatre-filtered unlike format). Tests for empty comma entries (no wildcard) are the important ones.
Watch:
-
ghshowssrc/filters.tsas a binary diff (0/0). That usually means CRLF or encoding. Please make it a normal text patch so review/rebase with #84 is possible. -
Substring
theatre:IMAXwill match any venue whose name contains IMAX even when the user meant the IMAX format. Worth a README note. -
Conflicts with #84 on the same
addWatch/ poll paths — rebase after whichever lands first.
Ishannaik
left a comment
Ishannaik
left a comment
There was a problem hiding this comment.
minor src/filters.ts shows as a binary diff (0/0). That is CRLF or encoding. Make it a text patch so rebase with #84 is possible.
minor theatre:IMAX is a substring on venue name, so it also matches IMAX-named cinemas when the user meant the format. One README line is enough.
VERDICT: APPROVE
TESTS: empty comma entries (no wildcard) and name/code match. CI green.
|
minor rebase needed: #84 just landed on main. Same �ddWatch / ilterSummary / subscribeToMovie paths. |
"Notify me when PVR Phoenix opens" was the one filter SeatSniper could not express. It already parsed venueCode/venueName and alerted when a new cinema appeared, but there was no way to say "only these cinemas". Adds `theatre:` alongside `format:` and `days:`, following the same shape: normalise on input, persist in its own column (ALTER TABLE + catch), show in /list and in the DM filter line. matchesTheatre is a case-insensitive substring against the venue name *and* the venue code, so `theatre:PVR` catches "PVR: Phoenix Palladium" and a user who pasted `IMOB` still matches. Whitespace is collapsed on both sides. An empty filter entry (a stray trailing comma) never matches, so it cannot silently become a wildcard. Unlike format:/days:, this one also filters the **new-cinema** half of a subscription. That half is a deliberate gap for format:/days: because checking them needs a showtimes fetch per venue plus coalescing; a venue's name and code are already in hand from the response the diff is computed from, so filtering it there costs nothing. README's caveat section is updated to say so. On the fresh-dates path the theatre check rides the existing per-date fetchShowtimes rather than adding a second one, so combining format: and theatre: does not double the requests.
f6cec3e to
03656dc
Compare
|
Rebased onto current The union was mechanical in most places (
Also fixed something that was my own bug, not a conflict:
|
2 participants
Closes #27.
What
theatre:alongside the existingformat:anddays:, following the same shape end to end — normalise on input, persist in its own column (ALTER TABLE+ catch), surface in/listand in the DM filter line.matchesTheatreis a case-insensitive substring against the venue name and the venue code, sotheatre:PVRcatchesPVR: Phoenix Palladium, Lower Pareland someone who'd rather pasteIMOBstill matches. Whitespace is collapsed on both sides, soIMAX WadalamatchesIMAX WADALA.One deliberate guard: an empty filter entry never matches. A stray trailing comma (
theatre:PVR,) would otherwise produce an""needle, andhaystack.includes("")is always true — the filter would silently become a wildcard that matches every cinema. There's a test pinning it.The interesting bit — new-cinema alerts
format:anddays:are deliberately not applied to the new-cinema half of a subscription; the code comments and the README both explain that filtering it would need a showtimes fetch per venue plus careful coalescing.theatre:doesn't have that problem. A fresh venue already carries itsnameandcodefrom the very response the venue diff is computed from — so filtering it there costs no extra request and needs none of the coalescing. Sotheatre:PVRdoes stay silent when an INOX starts listing the film, which is what the issue's "only announce dates/cinemas that match" asks for.I updated the code comment and the README caveat section to say exactly this, rather than leaving the docs claiming all filters behave alike.
On the fresh-dates path the theatre check rides the existing per-date
fetchShowtimesinstead of adding a second pass, so combiningformat:andtheatre:doesn't double the requests.Acceptance
/listdisplaycheckWatch)Docs updated: README intro, feature table, commands table, the
format:/days:caveat block, and the/helpembed.Checks run locally
bun testbunx tsc --noEmitdb.test.ts:145-146Those two
TS2532s are pre-existing onmainand untouched here; my newdb.test.tscases use optional chaining and add none.One thing worth knowing
While building this I briefly had a genuine bug — the
INSERTcolumn list and the bound args got out of sync.addWatch'scatch { return null }swallowedSQLite query expected 10 values, received 11and reported it as "already watching that movie", which is what aUNIQUEviolation looks like. Everydb.test.tscase failed with a misleadingaddWatch failedand no cause.Not fixing that here since it's out of scope, but a
catch (e)that re-throws anything which isn't a constraint violation would have turned a confusing ten minutes into an obvious one. Happy to open a separate issue.Summary by CodeRabbit
New Features
/watchconfirmations and watch lists now display configured theatre filters.Bug Fixes
Documentation
Greptile Summary
This PR adds a normalized, persisted
theatre:preference and applies it to fixed-date watches, new-date checks, and new-cinema subscription alerts.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Discord /watch theatre filter] --> B[Normalize theatre values] B --> C[(Persist on watch)] C --> D{Monitoring path} D -->|Fixed date| E[Filter fetched shows by venue name or code] D -->|New subscription date| F[Fetch shows and filter by venue] D -->|New cinema| G[Filter venue snapshot by name or code] E --> H[Send matching alert] F --> H G --> HReviews (2): Last reviewed commit: "feat(watch): theatre/venue prefer-filter..." | Re-trigger Greptile
Context used (5)