Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8afb8d4091
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa12aca02c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Isn't this half baked? If you are supporting v2 why convert it internally? Feels like an agent just did half the job |
fa12aca to
003b3e8
Compare
|
@jfberry Fair point. The first version translated v2 back into v1 rows internally, which threw away what v2 adds. I've reworked it (force-pushed, one commit): rules are now v2-shaped end to end, from the Poracle service through GraphQL ( |
|
Several v2 schema mismatches break rule creation, invasion editing, and location updates; v1 list errors also regress. Changed-file lint/format checks and all 15 form/query tests passed. HTTP-backed tests were blocked by sandbox loopback restrictions; production build, standalone typechecking, and browser verification were not performed in this focused review. Full review comments:
|
|
@Mygod Thanks for the review. Three findings are fixed in 0cb4121:
The other three are differences between PoracleNG
v2 support therefore requires PoracleNG |
17f4ffb to
33c588b
Compare
|
The patch contains reproducible v2 payload incompatibilities and a filtered-list synchronization regression. Focused form/query tests and lint/formatting passed; HTTP-stub tests were blocked by loopback EPERM, and production build/live UI validation was not run. Full review comments:
|
|
@Fabio1988 I notice factual mistakes about the API in this review, so be careful just accepting it |
|
@Mygod Checked each point against PoracleNG
Not changed, because they don't match the current v2 API:
The fixes from the first round are now in 33c588b after the rebase onto |
|
Yes, the previous review was looking at outdated PoracleNG. The patch introduces rule-editing regressions and incomplete incident/error handling. Focused form and GraphQL-document tests plus changed-file lint/format checks passed; HTTP-backed tests were blocked by the sandbox's localhost bind restriction ( Full review comments:
|
|
@Mygod All six addressed in d2af983:
Tests added for each; 217/217 server tests, lint, prettier and build pass locally. |
|
The patch introduces a cross-user write vulnerability and reproducible form/search regressions. All 29 focused pure tests and changed-file lint/formatting checks passed; HTTP tests were blocked by localhost listener permissions, and production build/live integration validation was not performed. Full review comments:
|
|
@Mygod All four confirmed and fixed in bef2108:
219/219 server tests, lint, prettier and build pass locally. |
|
The v1 adapter introduces a rule-placement regression during profile switches. Focused form/query tests and changed-file lint/format checks passed; HTTP-backed tests were blocked by sandbox socket restrictions. Review comment:
|
|
@Mygod Confirmed and fixed in c76fe1c. Checked against both v1 servers: PoracleJS
The other v1 categories don't update by uid (PoracleJS diffs them within the current profile), so only pokemon could move. v2 isn't affected: 221/221 server tests, lint and prettier pass locally. |
|
The new v2 profile path silently converts upstream HTTP failures into empty profile results. The production build, changed-file lint/format checks, and 32 focused tests passed; HTTP-backed tests were blocked by sandbox localhost-listening restrictions. Review comment:
|
|
@Mygod Confirmed and fixed in 6a97dcd.
Changes:
v1's non-tracking calls are unchanged. They still use the original New tests cover a 503 on the profile read (error, |
|
Profile switches can cause v2 deletions to silently do nothing, and v1 authentication failures are misreported as success. Focused form/schema tests and changed-file lint/format checks passed; HTTP-backed tests were blocked by sandbox listener restrictions. Full review comments:
|
ReactMap now works with Poracle tracking rules in the PoracleNG v2 shape end to end: GraphQL rule lists, the webhook store, the editor and the saved rules all use v2 fields (null means any, named enums, separate edit/summary booleans). Servers that only speak the legacy v1 API are served by an adapter that converts rules to and from v1 rows. - server: poracle/http (problem+json and v1 error messages, generic transport errors), trackingV2 (batch POST, PUT keeps uid, bulk delete), trackingV1 over v1Adapter - new incident category (gold stop, kecleon, showcase) on v2 - edit and summary switches shown only when the server supports them - rule settings ReactMap has no input for are kept on edit - gym battle_changes exposed, ping and stepped profile schedules kept - quick gym errors shown instead of a success toast Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nt search label
- `all` is set by the selector tile and is not a rule field; v2 rejects it
- PoracleJS reports some failures as 200 { status: 'error' }; treat them as
errors again instead of an empty list
- add the missing search_incidents label
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…slation fallbacks - mutation results update the tracked query's cache, so the active search still applies and list rows and tiles index the same rules - Manage uses the shared result handling - unknown incident types use the declared incident_unknown label and the default template label has no fallback string Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ad errors - an untouched slider keeps a stored null (e.g. no pvp rank ceiling) instead of saving the default it displays - switching between distance and areas clears the override the new mode can't hold (location label needs distance, areas exclude it) - apply-to-all honours an explicit All Forms - a failed tracking read returns no list; ALL_PROFILES selects status and message, so the client shows the error and keeps the rules it had - on v1, incident rules use the invasion templates - the invasion disable flag (and v1 invasion block) also covers incidents Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-all forms - rule uids, profile numbers and coordinates from the client must be numeric before they go into a Poracle URL; unknown categories and profile actions are refused instead of being sent as paths - the apply-to-all preset starts with All Forms off (an absent form no longer reads as "all forms") - the store gets the filtered rows also while a failed read keeps the last list, so list and tiles stay on the same rules; the error alert shows once per read result - named labels (team, gender, rsvp) have no fallback string; add the missing genderless label Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v1 adapter dropped profile_no, so a pokemon edit was posted without it. PoracleJS and PoracleNG v1 update pokemon rows by uid and file a missing profile_no under the active profile, so a profile switch during the edit moved the rule. The rule now carries the profile_no it was read with; new rules still get the active profile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v2 human and profile calls went through fetchJson, which returns the failed response instead of throwing, so a 503 on the profile list read as an empty list. They now use the Poracle transport and come back as status: 'error' without the list or human, so the client keeps what it shows and displays the error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- webhookCategories passed the { human } wrapper's blocked_alerts, which
is always undefined, so blocked alerts never hid a tab; an invasion
block now hides incidents on v2 too, and a monster block hides pokemon
- apply-to-all on incidents with nothing selected goes to every event
tile instead of a '0-0' rule without display_type
- the fallback size label treats null bounds as any size
- init settles every request before it returns and tracking is rebuilt
right after API detection, so a half-failed init can't leave human
calls on v2 and tracking on v1
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d24a4ba to
f35a085
Compare
Summary
PoracleNG's v1 tracking, humans and profiles routes are frozen and deprecated, and new behaviour only ships on
/api/v2. This PR makes ReactMap's Poracle integration v2-native: from the Poracle service through GraphQL, the webhook store and the editor, rules are PoracleNG v2 rules. v1 is kept only as an adapter at the edge, for PoracleJS and older PoracleNG.The first version of this PR converted v2 responses back into v1 rows internally (jfberry's review: "why convert it internally?"). That is gone; this is a rewrite of that approach, squashed into one commit.
Requirements
v2 needs PoracleNG
developat jfberry/PoracleNG@715c0d7e (merge of jfberry/PoracleNG#328) or later, until that reachesmain. Earlier v2 versions use a different v2 shape (lat/lon, no invasiongrunt_type, requiredpokemon_id), and this PR does not support it. Servers without a v2 API (PoracleJS) keep working through the v1 adapter.How it works
apiVersion(autoby default, orv1/v2).autofetches the public/openapi.jsononce and uses v2 only if every v2 route ReactMap uses is listed. PoracleJS has no spec, so it stays on v1 quietly.nullmeans any, enums are names (team: "valor",gender: "female",rsvp_changes: "rsvp_only"), andclean,editandsummaryare separate booleans. GraphQL returns rule lists asJSON, so ReactMap no longer drops fields it has no schema for. Settings ReactMap has no input for (costume, overrides, PVP evolution,battle_changes, …) are therefore kept on edit without any merging logic.server/src/services/poracle/:http.js: one HTTP client. Error messages come from problem+jsondetailanderrors[](v2) ormessage(v1). Network failures become "Poracle server unreachable" / "timed out" and don't expose the internal Poracle URL to the browser.trackingV2.js: new rules in one batchPOST; edits arePUT …/tracking/{type}/{uid}, so the rule keeps its uid; bulk delete via?uid=.trackingV1.js+v1Adapter.js: same interface over the v1 routes. Converts v2 rules to v1 rows (sentinels, packedcleanbitmask) and back. Every rule is converted before anything is sent, so a rule v1 can't represent is reported as an error and nothing is half-written.incidentcategory (gold stop, Kecleon, showcase), its own tab, as in PoracleNG v2. On v1 servers incident rules map to the invasion rows they used to be.editandsummaryswitches are shown only when the server supports them (capabilitiesin the webhook context). An incident type v1 has no name for is rejected with a readable error rather than dropped silently.WebhookAdv.jsx) builds its form from the rule (ruleForm.js:ruleToForm/formToRule). "Any" selects map tonull, and the template select has a "Default" entry (null, server default).{status, message}; the manage, selecting and tracked-tile views and the gym popup's quick gym show the error instead of a success toast.Test plan
server/test/poracleTracking.test.js: v1 adapter round trips for every category, v2 and v1 clients against a stub server (batch POST, PUT by uid, bulk delete, invasion/incident split on v1, error message extraction, transport errors)server/test/ruleForm.test.js: rule ⇄ form conversion, tile ids, null handlingserver/test/webhookQueries.test.js: client GraphQL documents validate against the server schemanode --test(local Node 22.23.3 is one patch below the engine pin, soyarn testrefuses to start): 203/203 pass. eslint clean, vite build oknullon the nullable enums (gender,team,rsvp_changes), which PoracleNG accepts (its validator skipsnullthere, andTestV2_EveryNullableFieldRoundTripsNullsends it)Known limitations
costume, overrides or PVP evolution; it keeps them on edit.enhas the new locale keys (incident,edit,summary,default_template); other languages fall back.🤖 Generated with Claude Code