LVT-40: Check if match exists and also switch alliance if selected wrong#84
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a pre-flight “match existence” check before starting scouting, and attempts to auto-correct the selected alliance color based on server data. This integrates a new API helper and uses network state to avoid running the check when not on Wi‑Fi.
Changes:
- Added
@react-native-community/netinfodependency to support connectivity checks. - Introduced
lib/lovatAPI/checkMatch.tsto call/v1/manager/checkmatch(with Wi‑Fi gating). - Updated the Home screen “Scout this match” action to (a) confirm match existence and (b) override alliance color when returned by the server.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| package.json | Adds NetInfo dependency needed by the new match-check helper. |
| package-lock.json | Locks NetInfo dependency resolution for reproducible installs. |
| lib/lovatAPI/checkMatch.ts | New helper for checking match existence and returning alliance info. |
| app/home.tsx | Uses checkMatch before scouting; prompts on failures and switches alliance color when available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MangoSwirl
left a comment
MangoSwirl
left a comment
There was a problem hiding this comment.
This seems like a very well thought-out implementation that would solve lots of issues at comps. I'm impressed.
The one issue I can see is that we're requiring a distinct network call every time new match info is entered. If I were to design this feature, I would use a single endpoint that fetches the entire match schedule to be saved on-device. That way, even if there are connection issues later, the feature would continue to work. We could also visibly switch the alliance color in the picker as the user is entering the team number without bombarding the server with requests.
Using API calls to update a local copy of the data is actually how we're currently handling scouter schedules, tourhaments, etc (see lib/services.ts) and there's a pretty straightforward system for it.
| const state = await NetInfo.fetch(); | ||
|
|
||
| if (!state.isConnected) { | ||
| return { exists: true, alliance: alliance }; |
There was a problem hiding this comment.
Having exists: true mean "we don't know whether it exists" seems counterintuitive to me
| alliance: exists | ||
| ? responseSchema.parse(await response.json())?.alliance === "red" | ||
| ? AllianceColor.Red | ||
| : AllianceColor.Blue | ||
| : alliance, |
There was a problem hiding this comment.
I don't see any client-side guard for if the server responds with 200 but with a null body. In that case the alliance color would always be blue.
There was a problem hiding this comment.
Also nit: I'm very against using nested ternaries. There's almost always a more readable way to write the code more reminiscent of how a human would explain the logic.
| teamNumber: number, | ||
| alliance: AllianceColor, | ||
| ) => { | ||
| const state = await NetInfo.fetch(); |
There was a problem hiding this comment.
Checking the network state isn't super necessary to do. The pattern we typically follow is just surrounding the fetch call with a try/catch block.
I assume what you're trying to do here is have different behavior for a server-side failure (actual problem) vs connectivity failure (expected from time to time)
| } | ||
| reportState.scoutMatch({ | ||
| ...meta, | ||
| allianceColor: match.alliance, |
There was a problem hiding this comment.
If the user selects the incorrect alliance color in the manual match builder, it'll be silently corrected when the user taps "Scout this match". Effectively, this means that if the user has an internet connection, then the alliance color selector does nothing and will likely confuse users (a la elevator door close button)
We're also making it appear as though manually selecting an alliance color is always necessary when we could easily be saving users an extra tap.
No description provided.