feat: add toast message for warnings array#346
feat: add toast message for warnings array#346lourduradjou wants to merge 5 commits intovalhalla:masterfrom
Conversation
|
Hi @nilsnolde , please lemme know if you need any changes. |
|
Yep, first thing is to follow our GSoC contribution guidelines properly, pls update your PR description accordingly. |
Hi thank you so much for the feedback, I learnt a lot from this. I can see lot of other contributors are also making the same mistakes like I did and every time you are asking them to review the PR description. Will it be appropriate if I make a PR to add contributor guidelines file. And I updated my PR, Looking forward to your review 🙂 |
|
Now we're missing proper unit & integration tests. |
Hi @nilsnolde, thanks for the feedback. I extracted the warnings handling logic into a reusable utility (handleValhallaWarnings) and added unit tests for it under utils/, following the existing testing pattern used in the repository. Since hooks in this project currently don't have dedicated tests, I focused on unit testing the utility that handles the warnings logic. Please let me know if you'd prefer an additional integration test at the hook level as well. I’ve also updated the PR description and attached screenshots of the test results. |
|
cool, thanks.
we have e2e tests. it'd be nice to have at least routing warnings (never mind the other endpoints). |
ff2da17 to
5b0ba09
Compare
5b0ba09 to
a40ed3a
Compare
Hi @nilsnolde , one e2e test case was added and I updated the PR description with screenshots. |
nilsnolde
left a comment
There was a problem hiding this comment.
you'll need to find out the real response format for the warnings array, AI clearly hallucinated a bit.
e2e/map.spec.ts
Outdated
| await setupNominatimMock(page); | ||
|
|
||
| const mockResponse = JSON.parse(JSON.stringify(customRouteResponse)); | ||
| mockResponse.warnings = [{ code: 1, message: 'Test routing warning' }]; |
There was a problem hiding this comment.
uh, wait.. this is not how valhalla returns a warning! good that you did that e2e test, it went almost unnoticed. so you never actually produced a warning right from valhalla? I mean, frankly, it's not directly documented what it represents and hardly any hints how to generate one, so it might need a little bit digging into the valhalla code. I'm sure that's what you had AI do and it fails fantastically. thanks for this, it's a clear reminder how unfit AI still is and still hallucinates instead of providing ways to really find out. may I ask which one you used, I'm really curious?
anyways, message doesn't exist, the description field name is called differently. I don't tell you on purpose though;) and tbh, message (or description) would have been the better field name, but well..
There was a problem hiding this comment.
it should also be a reminder to you. you have to check every single line of code and more importantly, every assumption it makes. it won't necessarily tell you when it has 0 proof for anything it wrote.
There was a problem hiding this comment.
honestly, this could be the primary reason why you still might find a job once you graduate. noone will need devs who don't challenge AI.
There was a problem hiding this comment.
so you never actually produced a warning right from valhalla? I mean, frankly, it's not directly documented what it represents and hardly any hints how to generate one, so it might need a little bit digging into the valhalla code. I'm sure that's what you had AI do and it fails fantastically. thanks for this, it's a clear reminder how unfit AI still is and still hallucinates instead of providing ways to really find out. may I ask which one you used, I'm really curious?
I tried to reproduce it but yeah I couldn't able to do it, somewhere this ai gave an example of code and message, I accept that I should have checked the docs clearly thought I missed it with this ai, you are right one example where this ai would fail.
I used chatgpt only. I'll go to valhalla now to understand this warning further, thanks!! give me some time.
There was a problem hiding this comment.
honestly, this could be the primary reason why you still might find a job once you graduate. noone will need devs who don't challenge AI.
I'll keep that in mind, I like this challenge.
Hi yes somewhere I also missed that, thanks for seeing it, really e2e saved both of us.. give me some time i'll go little deeper this time. |
|
Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/346 |
|
Hi @nilsnolde I referred the valhalla repo and understood the warnings structure, I changed all the files to adapt it from "message" to "description" now, PR description was also updated for future reference and ran the e2e, unit test cases. Lemme know if you need anything else. And I have a doubt, when I run the e2e test cases, sometimes it goes well, sometimes one test case this one starting 276th line test cases fails. Is this an issue?. As far my understanding it is a flaky one , due to rendering issue it goes timeout sometimes. If it is not an issue then fine ,else lemme know I'll try to fix it up. |
did you really hear what I suggested there? or maybe you don't agree? sorry, man, but I'll close this and we can't consider you as a GSoC candidate. this is why:
I hope you can see the problem yourself! not only didn't you check anything yourself or were critical enough with AI, you didn't understand what I was saying either. that might be a language barrier, but sorry, only one of those things is usually a no-go for a successful mentoring program, but both together make it impossible. |
ok nils I respect your decision. thank you. |
|
Thanks for taking it with dignity and best of luck! |


🛠️ Fixes Issue
Closes #45
Current Issue
Valhalla web-app ignores warnings array coming from the server, which could potential improve the User experience and help them navigate the issues (clamp errors or any new features errors)
👨💻 Changes proposed
As stated in the issue #45 there is a requirement to add warnings toast message to the places where server sends warnings.
Structure of warning (from valhalla package info.proto file)
Going through the docs, the changes were only required at,
Explanation of the changes:
Location: utils/handle-valhalla-warnings.ts
Location file: components/types.ts
Warning was created as a seperate interface to avoid repetition
ValhallaIsochroneResponse, ValhallaRouteResponse, FetchGeocodeObject in all these interfaces warning structure was added as mentioned in the docs
Changed types:
📄 Note to reviewers
New additions: (08/March/2026)
** (09/March/2026) **
** (11/March/2026) **
📷 Screenshots
Sample output
Test cases outputs
e2e test case output
AI Applications