fmaclen / hollama

A minimal web-UI for talking to Ollama servers
https://hollama.fernando.is
MIT License
316 stars 27 forks source link

fix: abort completion if user confirms to change the current session #145

Closed GregoMac1 closed 3 weeks ago

GregoMac1 commented 1 month ago
fmaclen commented 1 month ago

@GregoMac1 also, a test should have caught this bug when I switched to ollama-js.

I think we have a test for the "Stop" behavior pero I don't know if it's specifically testing the abortController behavior, can you see if we can improve the coverage?

cloudflare-pages[bot] commented 1 month ago

Deploying hollama with  Cloudflare Pages  Cloudflare Pages

Latest commit: bfb6e58
Status: ✅  Deploy successful!
Preview URL: https://1baa3594.hollama.pages.dev
Branch Preview URL: https://126-output-from-multiple-ses.hollama.pages.dev

View logs

fmaclen commented 1 month ago

I found a bug when you stop an ongoing 'completion' and then click on another link, it asks if you are sure about leaving.

https://github.com/user-attachments/assets/a452738f-2dfa-49a8-8987-6040b3ee3c38

GregoMac1 commented 1 month ago

@fmaclen the code is ready!

About the test, I initially didn't know how to emulate the behavior of the 'pending' requests, so I asked Claude for help and I ended up with this test case.

As far as I understand, it should work, but it doesn't. It fails because, at line 609, it expects the completion to be started, but it is not. I think it has to be something about the stream response emulated at line 591.

Do you see something wrong?

GregoMac1 commented 4 weeks ago

I've changed the code and I've split the tests but I still could not make them work. I'll dig deeper tomorrow.

fmaclen commented 4 weeks ago

I've split the tests but I still could not make them work

There's a few issues with the tests:

The rest of the implementation looks good 👍

GregoMac1 commented 4 weeks ago

Thanks for the feedback!

  • The shape of the streamed mocked responses should match the Ollama API

About this, I followed the shape of the real responses from Ollama in the app, which look like this:

image

Instead of response, the replies contain message: { role: string, content: string }.

I'm still working on the rest of tests!

fmaclen commented 3 weeks ago

Instead of response, the replies contain message: { role: string, content: string }.

Sorry, you are right. I was looking at the /api/generate streamed response instead of /api/chat. My bad!

fmaclen commented 3 weeks ago

@GregoMac1 I updated this PR with the latest merges on main and fixed the conflicts: https://github.com/fmaclen/hollama/pull/156 https://github.com/fmaclen/hollama/pull/157

fmaclen commented 3 weeks ago

:tada: This PR is included in version 0.10.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: