Skip to content

Commit 2443473

Browse files
cubehouseclaude
andcommitted
chore: add copilot-instructions for PR review calibration
GitHub Copilot's PR reviewer reads .github/copilot-instructions.md to calibrate its feedback. The previous two park PRs both surfaced the same false-positive comments (empty-string config defaults, this-typing in inject callbacks). This file documents the project conventions and known intentional patterns so Copilot stops re-flagging them. Also covers the expected park-implementation skeleton, status/wait-time mapping, cache TTL conventions, and the audit checks that ship with every new park. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b9a6e4e commit 2443473

1 file changed

Lines changed: 175 additions & 0 deletions

File tree

.github/copilot-instructions.md

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
# Copilot review instructions for parksapi
2+
3+
These instructions describe project-specific conventions. **Do not flag the patterns below as defects** — they are intentional. Use this file to calibrate review feedback before commenting.
4+
5+
> Authoritative source for conventions: [`CLAUDE.md`](../CLAUDE.md). This file is a Copilot-focused condensation; if there is a conflict, `CLAUDE.md` wins.
6+
7+
## Project overview
8+
9+
parksapi is a TypeScript ESM library that fetches real-time theme park data (wait times, schedules, entities) from 75+ park implementations. All park integrations live under `src/parks/<park>/`. The core abstraction is the `Destination` base class with a Template Method pattern: subclasses implement `_init()`, `buildEntityList()`, `buildLiveData()`, `buildSchedules()`, **never** the public `get*()` methods.
10+
11+
Node 24+, npm 11+, ES2022 modules, decorators enabled.
12+
13+
## Decorator-based design
14+
15+
Five decorators carry most of the framework:
16+
17+
| Decorator | Purpose |
18+
|---|---|
19+
| `@destinationController({category})` | Auto-registers the destination and applies `@config` to the class. **Never apply `@config` to a class directly when using this decorator.** |
20+
| `@config` | Property-level config injection. Values resolve from instance config → `{CLASSNAME}_{PROP}` env var → `{PREFIX}_{PROP}` env var → property default. |
21+
| `@cache({ttlSeconds, key?, callback?, cacheVersion?})` | SQLite-backed memoisation with TTL. |
22+
| `@http({cacheSeconds?, retries?, ...})` | Queue-based HTTP request wrapper. |
23+
| `@inject({eventName, hostname?, tags?, priority?})` | Sift.js (MongoDB-like) event filter for cross-cutting concerns (auth, headers, error handling). |
24+
25+
## Patterns the project uses (do **not** flag these)
26+
27+
### 1. Empty-string config defaults are intentional
28+
29+
```ts
30+
@config apiBase: string = '';
31+
@config token: string = '';
32+
@config email: string = '';
33+
```
34+
35+
`CLAUDE.md` requires that **no URLs, keys, tokens, or app versions ever appear hardcoded in source**. Empty-string defaults force configuration via `.env` and are the documented convention. **Do not suggest providing fallback URLs, default keys, or upfront validation that throws on empty strings.** The HTTP layer's URL-construction error is the documented failure signal when env vars are missing.
36+
37+
It is fine — and preferred — to throw a clear error when a credential is needed *for a specific operation* (e.g., `if (!this.email || !this.password) throw new Error('… requires …_EMAIL and …_PASSWORD …')` inside the auth method that needs it). What you should not suggest is gating the whole class on non-empty `apiBase` upfront.
38+
39+
### 2. `this` in `@inject` callbacks does not need typing
40+
41+
```ts
42+
@inject({
43+
eventName: 'httpRequest',
44+
hostname: function () { return hostnameFromUrl(this.apiBase); },
45+
})
46+
```
47+
48+
The `function () { ... this.apiBase ... }` form without an explicit `this: ClassName` parameter is used by 10+ destinations and `tsc --noEmit` passes. Do not suggest adding `this: FooDestination` or rewriting as an arrow function (the latter would change `this` binding and break the pattern).
49+
50+
### 3. Entity IDs are always strings
51+
52+
Even when the upstream API provides numbers, the framework requires string IDs. `String(numericId)` is the standard idiom; do not flag this as redundant.
53+
54+
### 4. Numeric validation uses `Number.isFinite`, not `isNaN`
55+
56+
```ts
57+
const n = Number(raw);
58+
if (Number.isFinite(n)) { ... }
59+
```
60+
61+
`isNaN("")` returns `false` (truthy "is not NaN"), which is dangerous for waitTime fields coming from APIs that may return empty strings. Use `Number.isFinite(Number(x))` or `parseInt(x, 10)` paired with `Number.isFinite`. **Do not suggest replacing `Number.isFinite` with `isNaN`.**
62+
63+
### 5. `as any` casts on framework boundaries are tolerated
64+
65+
The `HTTPObj`/`Entity`/`LiveData`/`EntitySchedule` types from `@themeparks/typelib` are intentionally strict to enforce shape at the public API boundary. Park implementations frequently use `as any as HTTPObj` when constructing request objects from minimal field sets, and `as Entity` / `as EntitySchedule` when assembling output. **Do not suggest replacing these casts with full-shape literal construction** unless the cast is hiding an actual type mismatch.
66+
67+
### 6. Throwing inside cached/wrapped methods is fine
68+
69+
Methods decorated with `@cache`, `@reusable`, or that go through `CacheLib.wrap` are expected to throw on auth failures, missing config, or upstream errors. The framework handles propagation. **Do not suggest wrapping these in try/catch unless there is a specific recovery path** that makes sense (e.g. `EMAIL_NOT_FOUND` → fall back to sign-up).
70+
71+
### 7. Public-method overrides are forbidden
72+
73+
Subclasses must implement `buildEntityList`, `buildLiveData`, `buildSchedules`, and (rarely) `_init`. Do **not** suggest overriding `getEntities()`, `getLiveData()`, `getSchedules()`, or `getDestinations()` on a `Destination` subclass — those are non-overridable public entry points that the framework wires up.
74+
75+
## Park-implementation expectations
76+
77+
When reviewing a new file under `src/parks/<park>/<park>.ts`:
78+
79+
### Required structure
80+
81+
```ts
82+
import {Destination, DestinationConstructor} from '../../destination.js';
83+
import {cache} from '../../cache.js';
84+
import {http, HTTPObj} from '../../http.js';
85+
import {inject} from '../../injector.js';
86+
import config from '../../config.js';
87+
import {destinationController} from '../../destinationRegistry.js';
88+
import {Entity, LiveData, EntitySchedule} from '@themeparks/typelib';
89+
import {hostnameFromUrl, constructDateTime} from '../../datetime.js';
90+
import {TagBuilder} from '../../tags/index.js';
91+
92+
const DESTINATION_ID = 'foopark';
93+
const PARK_ID = 'foopark-park';
94+
95+
@destinationController({category: 'Foo'})
96+
export class FooPark extends Destination {
97+
@config apiBase: string = '';
98+
@config token: string = '';
99+
@config timezone: string = 'Region/City';
100+
101+
constructor(options?: DestinationConstructor) {
102+
super(options);
103+
this.addConfigPrefix('FOOPARK');
104+
}
105+
106+
@inject({eventName: 'httpRequest', hostname: function () { return hostnameFromUrl(this.apiBase); }})
107+
async injectAuth(req: HTTPObj): Promise<void> { … }
108+
109+
@http({cacheSeconds: 60, retries: 2})
110+
async fetchLive(): Promise<HTTPObj> { … }
111+
112+
@cache({ttlSeconds: 60})
113+
async getLive(): Promise<...> { … }
114+
115+
async getDestinations(): Promise<Entity[]> { return [{id: DESTINATION_ID, …, entityType: 'DESTINATION'}]; }
116+
protected async buildEntityList(): Promise<Entity[]> { … }
117+
protected async buildLiveData(): Promise<LiveData[]> { … }
118+
protected async buildSchedules(): Promise<EntitySchedule[]> { … }
119+
}
120+
```
121+
122+
### Cache-TTL conventions
123+
124+
- Live data: ~60s (matches typical app polling cadence)
125+
- Entity / facility lists: ~6h (`21600`s)
126+
- Schedules / opening calendars: ~6h
127+
- Auth tokens: as long as the issuer permits, often 30 days+; clear on 401 via an `httpError` injector
128+
129+
### Status / wait-time mapping
130+
131+
`LiveData.status` is one of: `OPERATING`, `CLOSED`, `DOWN`, `REFURBISHMENT`, plus a few rarer values from `@themeparks/typelib`.
132+
133+
- Numeric wait time → `OPERATING` + `queue: {STANDBY: {waitTime: N}}`
134+
- "Maintenance" / "refurbishment" / equivalent → `REFURBISHMENT`
135+
- Anything that isn't running or maintaining → `CLOSED`
136+
- Down/temporary fault → `DOWN`
137+
138+
When the source API uses localised strings (e.g. `"営業準備中"` / `"Preparing"`), normalise via a phrase enum + numeric regex; do not assume a single locale.
139+
140+
### Entities to surface
141+
142+
- `DESTINATION` — the resort umbrella (always 1)
143+
- `PARK` — each ticketed gate (often 1; multiple for multi-park resorts)
144+
- `ATTRACTION` — rides, simulators, dark rides, walkthroughs
145+
- `RESTAURANT` — counter service, table service, snack carts
146+
- Skip: services, toilets, themed-area markers, parking, photo points, shops (unless the project asks for them)
147+
148+
### Schedules
149+
150+
- One `EntitySchedule` per `PARK`, with daily entries
151+
- Emit only days the park is **open** (skip closed days; the absence is the signal)
152+
- Use `constructDateTime(date, hhmmss, timezone)` to produce timezone-aware ISO strings
153+
154+
## Things that **are** worth flagging
155+
156+
- Hardcoded URLs / API keys / tokens / app versions in source — must be in `.env`
157+
- Use of `isNaN()` on values from external APIs (use `Number.isFinite` or `parseInt`/`parseFloat`)
158+
- `waitTime` ever being NaN or non-numeric (must be a finite number or null/undefined)
159+
- Cache keys that collide across destinations sharing a base class (use `getCacheKeyPrefix()` or unique method args)
160+
- Direct overrides of `getDestinations` / `getEntities` / `getLiveData` / `getSchedules` (must implement `build*` instead)
161+
- Entity IDs that are not strings
162+
- Adding `@config` to a class that already uses `@destinationController`
163+
- Caching non-JSON-safe types (Set, Map, Date)
164+
- Missing `addConfigPrefix(...)` call in the constructor
165+
- Real bugs: off-by-one in loops, unhandled promise rejections, swallowed errors, type coercion that silently corrupts data
166+
167+
## Test plan for new parks
168+
169+
```bash
170+
npm run dev -- <destinationid> # 4/4 tests should pass
171+
npm run audit:live -- --local --only=<destinationid> # 0 schema errors, 0 warnings
172+
npx tsc --noEmit # type-check across the whole project
173+
```
174+
175+
A new park PR is expected to ship with all three green.

0 commit comments

Comments
 (0)