feat(oceanpark): add Ocean Park Hong Kong#130
Conversation
cubehouse
left a comment
There was a problem hiding this comment.
Nice work — well-structured implementation with clean auth flow and good use of the decorator patterns. A few items to address:
| m[0][1] * (m[1][0] * m[2][2] - m[1][2] * m[2][0]) + | ||
| m[0][2] * (m[1][0] * m[2][1] - m[1][1] * m[2][0]); | ||
|
|
||
| const D = det(M); |
There was a problem hiding this comment.
If the map API ever returns collinear or duplicate reference points, D will be zero and all coordinates become NaN/Infinity — which silently propagates through entity locations (the if (coords) check passes because {latitude: NaN, longitude: NaN} is truthy).
Add a guard here:
const D = det(M);
if (Math.abs(D) < 1e-10) return null;Then handle the null in getCoordinateMapEntries by falling back to no coordinates for that category.
| // ── Implementation ────────────────────────────────────────────────────────── | ||
|
|
||
| @destinationController({category: 'Ocean Park'}) | ||
| @config |
There was a problem hiding this comment.
The @config class decorator is not needed here — @destinationController already applies @config internally (see destinationRegistry.ts line 208). Having both double-wraps the class in config proxies. Remove this line.
| const coeffs = computeAffineTransform(refPoints); | ||
| const entries: [string, {latitude: number; longitude: number}][] = []; | ||
|
|
||
| for (const category of MAP_CATEGORIES) { |
There was a problem hiding this comment.
These 6 map category fetches are independent but awaited sequentially. With the HTTP queue rate limit (250ms), this adds ~1.5s of wall time on a cold cache.
Use Promise.all to parallelize:
const responses = await Promise.all(
MAP_CATEGORIES.map(cat => this.fetchMapCategoryData(cat))
);
for (const resp of responses) {
const entities: OceanParkMapEntity[] = await resp.json();
// ...
}|
Hey @technodisney — just checking in on this. There are 3 inline review comments from April 8 that still need addressing:
Let us know if you need any help with these or if you have questions! |
|
Hi @technodisney — I've pushed a fixup commit (
CI didn't trigger automatically on the fork branch. @cubehouse — this should be good to merge if you're happy with the implementation after a local |
|
Hi @technodisney — really nice contribution, thank you for the thorough work on this. The Sanrio/etc. coordinate fallback behaviour is sensible and the affine-transform approach for deriving lat/lng from map pixels is elegant. I've pushed a maintainer-edit commit (e1bd7c0) addressing three small issues that came up during review. Quick rundown so you can see what changed and weigh in: 1. Schedule timestamps were not RFC 3339. Output looked like: Root cause turned out to be a pre-existing bug in (Side benefit: any future park hitting this same case is now safe.) 2. Hardcoded URL defaults. The two @config baseURL: string = 'https://sop.oceanpark.com.hk';
@config mapURL: string = 'https://map.oceanpark.com.hk';This repo's convention is empty 3. Number coercion guard on schedule timestamps. One thing I checked but didn't change: I initially thought the affine-transform was dead code (every entity was getting the destination fallback location), but that turned out to be a stale-cache artifact. After a clean run, ~50 of 94 entities receive real coordinates. The unmatched ones are newer attractions (Sanrio, Pompompurin, etc.) not yet present in the static map JSONs — falling back to destination centroid is reasonable for those. All 1094 tests pass; verified live output. Happy to revert any of this if you'd prefer to handle it differently — and again, thanks for the contribution! 🙏 |
|
Thank you for that. Give me a few more days to check out the unmapped attractions. Sanrio wasn't present in my initial testing. |
- Auth token (optoken) injected via @Inject; device UUID persisted 90 days via @cache; dynamic TTL from API tokenExpire field - Coordinate affine transform: fetches reference_points.json from map subdomain and projects pixel positions to lat/lng for all entities - Entities: rides, transport, shows, dining — with height restriction tags, wet rides, pregnant warning, and FastPass (paidReturnTime) - Live data: wait times + today's operating hours (rides/transport), showtimes from activityList (shows) - Schedules: 30-day park operating hours, parking, Summit zone entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard affine transform against degenerate reference points (determinant ≈ 0 → return null, caller returns empty coords) - Remove duplicate @config class decorator (already applied by @destinationController) - Parallelize map category fetches with Promise.all Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ber guards Three issues surfaced when reviewing the output: 1. **Schedule timestamps were `2026-05-01T10:00:00GMT+8`** instead of `+08:00`. The pre-existing `formatInTimezone(... 'iso')` helper passed `Intl.DateTimeFormat`'s `shortOffset` value through verbatim, but for timezones like Asia/Hong_Kong the API returns `GMT+8` rather than the RFC 3339 `+HH:MM` form. Other parks dodge this by routing through `constructDateTime`, which has its own GMT-style normalizer (lines 183-191 of datetime.ts). Move the same normalization into `formatInTimezone` so callers always get a valid ISO 8601 string. 2. **Hardcoded URL defaults** in `@config baseURL` / `@config mapURL` violated the project convention (CLAUDE.md: "no hardcoded URLs/secrets; all in @config with empty defaults, loaded from .env"). Set both to `''`; configuration is via `OCEANPARK_BASEURL` / `OCEANPARK_MAPURL` env vars per the project's standard config-prefix mechanism. 3. **`Number()` coercion on schedule timestamps** could silently produce `NaN` → `Invalid Date` for malformed `parkOpenTime` / `parkCloseTime` / `parkingOpenTime` / `parkingCloseTime` / `summitCloseTime`. Add a `parseTs` helper that uses `Number.isFinite()` (per CLAUDE.md numeric- validation guidance) and returns null for non-finite values; skip the day or sub-block when any required timestamp fails to parse. Coordinate transform was not dead code as initially feared — after a fresh cache, ~50 of 94 entities receive real coordinates (the rest are newer attractions not yet present in the static map JSONs and fall back to the destination centroid). No change required there. Also updates one DST-transition test that asserted on the legacy `GMT-5` / `GMT-4` substring; relaxed to accept the new canonical `-05:00` / `-04:00` form alongside the old shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Correctness:
- waitTime: coerce + Number.isFinite() guard before emitting; the
interface declares number|null but upstreams sometimes send strings.
- operatingHours.type: 'Operating' → 'OPERATING' so it matches every
other park's emission and any downstream string-match.
- Live operatingHours / showtimes now use formatInTimezone(..., 'iso')
instead of .toISOString() so timestamps carry the +08:00 offset
consistently with buildSchedules.
- Showtimes filter to t.endTime >= now; an 11am performance shouldn't
appear as upcoming when checked at 3pm.
Cache-poisoning (same shape as the Genting ThemeParks#196 fix):
- getCoordinateMapEntries now throws on degenerate input instead of
returning []. The @cache wrapper was caching the empty failure for
24h, pinning every entity to the destination's default lat/lng for
a full day after one bad upstream response. Callers in
buildEntityList catch and fall back to no-coords without poisoning.
- Dropped @cache decorators on getEntityList / getEntityDetail /
getParkSchedule — the underlying @http fetchers already cache the
raw responses at the same TTL, so the @cache layer was just
double-writing parsed payloads and amplifying the same
empty-result-poisons-TTL pattern at smaller scale.
Quality / polish:
- Removed TagBuilder.location() call on attractions — the TagBuilder
docs explicitly flag this as the wrong pattern when the coordinates
are also the entity's own location (which they are).
- Dropped 6 unnecessary `as any` casts on decorator option literals;
only the `tags: ['auth']` cast remains (it's the one case the @http
decorator's option type genuinely doesn't accept yet — annotated).
- Summit zone: defensive fallback for the `summitStaus` (sic) typo in
case upstream ever corrects it; clarified the description to make
the "Summit opens with the park" assumption explicit since the API
only exposes summitCloseTime, not summitOpenTime.
All 1174 tests still pass; harness 4/4 on Ocean Park with the timestamp
format and showtime-filter behaviour confirmed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds Ocean Park Hong Kong as a new destination integration, including auth, entity/live data/schedule fetching, coordinate projection, and README/TODO updates for the new destination.
Changes:
- Adds
OceanParkHongKongdestination implementation with token auth, entities, live data, schedules, tags, and coordinate mapping. - Updates TODO destination totals.
- Reworks README destination/configuration documentation and includes Ocean Park in the destination list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/parks/oceanpark/oceanpark.ts |
Adds the Ocean Park Hong Kong destination implementation. |
README.md |
Updates project documentation, destination list, and generated env-var section. |
TODO.MD |
Adds Ocean Park Hong Kong to the completed migration list and updates totals. |
| const wt = Number(pflow.entityWaitTime); | ||
| if (isOpen && Number.isFinite(wt) && wt >= 0) { |
| Best current documentation for this is to check out `scripts/templateDestination.js` and look at the existing supported destinations. | ||
|
|
||
| Setup `index.js` with your new class, and edit `test.js` to test your destination to check everything is setup correctly. |
| <!-- BEGIN_DESTINATIONS --> | ||
| * WaltDisneyWorldResort |
| * Attraction | ||
| * A ride / transport / etc. | ||
| * Attraction entities must be within a Destination entity (usually also within a park, but not always) | ||
| * eg. Pirates of the Carribean |
| * A ride / transport / etc. | ||
| * Attraction entities must be within a Destination entity (usually also within a park, but not always) | ||
| * eg. Pirates of the Carribean | ||
| * Resturant |
| WALIBIBELGIUM_DESTINATIONSLUG | ||
| WALIBIBELGIUM_PARKSLUG | ||
| WALIBIBELGIUM_APISHORTCODE | ||
| WALIBIBELGIUM_CULTURE |
Two follow-ups from Copilot's review of the 10-fix commit: 1. wait time: Number(null) is 0, so the previous coercion was silently emitting a 0-minute "wait" for open attractions whose API response left entityWaitTime null/undefined. Reject null/undefined/empty BEFORE coercing so only real numeric waits flow through. 2. README: the rebase resolution earlier in this PR kept the contributor's old README rewrite on top of main's newer rewrite (took --theirs instead of --ours during conflict resolution). That pulled in stale paths, an out-of-date destination list, spelling errors, and an env-var section missing Ocean Park's own keys — all five of Copilot's README findings. README is out of scope for adding a park; restoring main's version drops all five at once. TODO.MD's "Ocean Park" addition stays — that's a legitimate cross-cut that goes with the park add. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| ); | ||
|
|
||
| const attractionEntities: Entity[] = attractions.map((entity, i) => { | ||
| const isTransport = entity.typeId === SORT_ID.TRANSPORT; |
Per Copilot review on the previous fix commit: `isTransport` was checking `entity.typeId === SORT_ID.TRANSPORT`, but `typeId` is optional on the API and uses a different ID space than the SORT_ID constants — so every transport entity was silently being emitted with attractionType RIDE. Use the slice position instead: the concatenation is `[...rides, ...transport]`, so everything at index >= rides.length came from the transport list. Verified locally: now 4 entities are TRANSPORT (Ocean Express, etc.) and the remaining 46 stay RIDE. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Adds Ocean Park Hong Kong as a new destination.
Single destination, one park. No API credentials required — the park's mobile API uses a short-lived bearer token obtained from a public endpoint.
Implementation
Auth
optokenheader injected via@injecton all requests tosop.oceanpark.com.hk@cachetokenExpirefield in the API responseCoordinates
reference_points.jsonfrommap.oceanpark.com.hkprovides pixel→lat/lng anchor pointsEntities
sortId: 8), transport (sortId: 7), shows (sortId: 15), dining (sortId: 17)paidReturnTime)Live data
pflowInfo)activityListon entity detail endpointSchedules
Test plan
npm run dev -- oceanparkhongkong— entities, live data, and schedules return datanpm run dev -- oceanparkhongkong --ignore-cache— fresh fetch works correctlynpm test— existing tests unaffected🤖 Generated with Claude Code