Skip to content

feat: add toast message for warnings array#346

Closed
lourduradjou wants to merge 5 commits intovalhalla:masterfrom
lourduradjou:warnings-toastmessages
Closed

feat: add toast message for warnings array#346
lourduradjou wants to merge 5 commits intovalhalla:masterfrom
lourduradjou:warnings-toastmessages

Conversation

@lourduradjou
Copy link
Contributor

@lourduradjou lourduradjou commented Mar 5, 2026

🛠️ 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)

message CodedDescription {
  string description = 1;
  uint64 code = 2;
}

message Info {
  repeated Statistic statistics = 1;      // stats that we collect during request processing
  repeated CodedDescription errors = 2;   // errors that occurred during request processing
  repeated CodedDescription warnings = 3; // warnings that occurred during request processing
  bool is_service = 4;                    // was this a service request/response rather than a direct call to the library
}

Going through the docs, the changes were only required at,

  • use-directions-queries.ts
  • use-isochrones-queries.ts
  • use-optimized-route-query.ts

Explanation of the changes:

  • An utility file was created to handle this warnings array
  • The response data received from the server was checked to have warnings in it, to display the warning message using existing toast from shadcn.

Location: utils/handle-valhalla-warnings.ts

  const TOAST_CONFIG: ExternalToast = {
  position: 'bottom-center',
  duration: 5000,
  closeButton: true,
};

export const handleValhallaWarnings = (warnings?: ValhallaWarning[]): void => {
  if (!warnings?.length) return;

  warnings?.forEach((warning) => {
    toast.warning('Routing warning', {
      description: warning.description,
      ...TOAST_CONFIG,
    });
  });
};
  • Typescript Types of the relevant responses where changed to remove the warnings and improve the maintainability and catch bugs.
    Location file: components/types.ts
    Warning was created as a seperate interface to avoid repetition
export interface ValhallaWarning {
  code: number;
  description: string;
}

ValhallaIsochroneResponse, ValhallaRouteResponse, FetchGeocodeObject in all these interfaces warning structure was added as mentioned in the docs

Changed types:

export interface ValhallaRouteResponse {
  id: 'valhalla_directions';
  trip: Trip;
  alternates?: ValhallaRouteResponse[];
  warnings?: ValhallaWarning[];
}

export interface ValhallaIsochroneResponse extends GeoJSON.FeatureCollection {
  id: string;
  warnings?: ValhallaWarning[];
}

export interface ValhallaOptimizedRouteResponse {
  trip: Trip;
  id?: string;
  warnings?: ValhallaWarning[];
}

📄 Note to reviewers

  • There are so many places in the docs Warnings(optional) were mentioned but I only placed the checking where the data received from the Valhalla server.
  • Relevant changes were made directly to the hooks which helps to get the data via axios
  • One more place where we could potentially add the check at nominatim.ts but it uses VITE_NOMINATIM_URL=https://nominatim.openstreetmap.org so considered as not needed.
  • I can't simulate the warnings messages via the existing slider (like clamp error), so I changed the data to add warnings in it. (So it proves in future when server returns data with warnings it will work)
  • Any endpoints if I missed, please let me know.

New additions: (08/March/2026)

  • Unit test cases were written to check whether it shows warnings when there is a warnings and not when it is empty.
  • Existing code was turned into a utils file to remove the redundancy.
  • You can find test cases in handle-valhalla-warnings.spec.ts file inside the utils file itself
  • Comments were added in the places and also to explain the util function.
  • Attached the test cases in the screenshot section.
    ** (09/March/2026) **
  • e2e testing was added for one of the route and test cases were added in the screenshot section.
    ** (11/March/2026) **
  • Required changes in the test cases were added and all relevant files were changed to adapt it.

📷 Screenshots

Sample output

Valhalla issue warnings

Test cases outputs

Screenshot from 2026-03-08 20-13-24 Screenshot from 2026-03-08 20-14-33

e2e test case output

image image

AI Applications

  • LLM were used to understand the problem and needs as I am new to this repository and ecosystem
  • LLM was used to write test cases which is thoroughly checked.
  • Code, Description and comments was written by me, I am willing to change even further to help other get an idea of this issue and how I addressed the problem.

@lourduradjou
Copy link
Contributor Author

Hi @nilsnolde , please lemme know if you need any changes.

@nilsnolde
Copy link
Member

Yep, first thing is to follow our GSoC contribution guidelines properly, pls update your PR description accordingly.

@lourduradjou
Copy link
Contributor Author

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 🙂

@nilsnolde
Copy link
Member

Now we're missing proper unit & integration tests.

@lourduradjou
Copy link
Contributor Author

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.

@nilsnolde
Copy link
Member

cool, thanks.

Since hooks in this project currently don't have dedicated tests

we have e2e tests. it'd be nice to have at least routing warnings (never mind the other endpoints).

@lourduradjou lourduradjou force-pushed the warnings-toastmessages branch from ff2da17 to 5b0ba09 Compare March 9, 2026 10:48
@lourduradjou lourduradjou force-pushed the warnings-toastmessages branch from 5b0ba09 to a40ed3a Compare March 9, 2026 11:22
@lourduradjou
Copy link
Contributor Author

lourduradjou commented Mar 9, 2026

we have e2e tests. it'd be nice to have at least routing warnings (never mind the other endpoints).

Hi @nilsnolde , one e2e test case was added and I updated the PR description with screenshots.

Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' }];
Copy link
Member

@nilsnolde nilsnolde Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lourduradjou lourduradjou Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lourduradjou
Copy link
Contributor Author

you'll need to find out the real response format for the warnings array, AI clearly hallucinated a bit.

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.

@github-actions
Copy link

Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/346

@lourduradjou
Copy link
Contributor Author

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.

Screenshots:
image
image

@nilsnolde
Copy link
Member

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.

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:

  • one fail initially when you didn't verify AI code yourself or even having AI verify itself (it won't do it automatically, you need to prompt it to check itself, which will often be successful and definitely would've been here! the more I work with it the more I realize that AI prompting is a very nuanced skill by itself, which you'll hopefully still learn!)

  • fine, I gave you a second chance (other maintainers wanted to close this PR already) and asked you to research the actual field name

  • I expected you to at least prompt AI with more context so it finds out and corrects it, though from any grown-up developer I'd expect that being done without AI and prove the PR with his/her own brain, showing us some proof

  • I even told you explicitly and fully on purpose

    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..

    which clearly suggests that description is not the right field name.

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.

@nilsnolde nilsnolde closed this Mar 11, 2026
@lourduradjou
Copy link
Contributor Author

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.

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:

* one fail initially when you didn't verify AI code yourself or even having AI verify itself (it won't do it automatically, you need to prompt it to check itself, which will often be successful and definitely would've been here! the more I work with it the more I realize that AI prompting is a very nuanced skill by itself, which you'll hopefully still learn!)

* fine, I gave you a second chance (other maintainers wanted to close this PR already) and asked you to research the actual field name

* I expected you to _at least_ prompt AI with more context so it finds out and corrects it, though from any grown-up developer I'd expect that being done without AI and prove the PR with his/her own brain, showing us some proof

* I **even** [told you explicitly](https://github.com/valhalla/web-app/pull/346#discussion_r2910283650) and fully on purpose
  > 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..
  
  
  which clearly suggests that `description` is not the right field name.

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.

@nilsnolde
Copy link
Member

Thanks for taking it with dignity and best of luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add warnings from Valhalla

2 participants