Conversation
This also uses the official OpenAI library instead of an older one.
This is to reduce code duplication when we get around to supporting multiple models. With this change, the common code to call a provider is shared.
5 tasks
H-Shay
reviewed
Dec 23, 2025
|
|
||
| // We create our own HTTP client to intercept and act as the OpenAI API | ||
| apiKey := "not_a_real_key" | ||
| mockApi := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
Contributor
There was a problem hiding this comment.
I am vaguely wondering if putting the mock api code in a separate file makes sense, as it might a little easier/cleaner to read?
Member
Author
There was a problem hiding this comment.
probably, yea. I'll take a look and see what I can do.
Member
Author
There was a problem hiding this comment.
I've split it out to the test package - please take a look
H-Shay
reviewed
Dec 23, 2025
H-Shay
reviewed
Dec 23, 2025
| RawJSON() string // same definition that's shared with the OpenAI response parts | ||
| } | ||
|
|
||
| func compressJsonResponse(target compressible) string { |
Contributor
There was a problem hiding this comment.
this removes duplicate keys and newlines?
Member
Author
There was a problem hiding this comment.
in a way, yea. For whatever reason the OpenAI response structs duplicate all of their fields and then use "pretty" JSON too, which makes things noisy.
H-Shay
approved these changes
Dec 23, 2025
Co-authored-by: Shay <hillerys@element.io>
... and split it out a bit further for readability.
H-Shay
approved these changes
Jan 2, 2026
H-Shay
left a comment
Contributor
There was a problem hiding this comment.
looks great thanks for splitting that out - much clearer!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewable commit-by-commit (recommended)
I wasn't overly happy with the original code, but it worked, so it was fine. Now that we're looking at adding other models/providers/classifiers it feels like a good idea to make the code a bit cleaner.
Previously, the code abstracted the idea of a provider within a single generic "OpenAI filter". This meant it was testable, but was complicated to set up and follow the code path. The abstraction was also at the wrong layer: it should have been at a higher level, away from the filter itself, so it could (theoretically) be reused elsewhere, possibly in the future. The old code could theoretically be abused to be reusable, like in this PR, but the result is a bit messy and screams for wanting to abstract away the
AIProviderstuff to a higher level.This PR moves the abstraction to that higher level, promoting models (what we call "AI providers") to a first-class citizen in our code. This PR also splits the AI filter to have a shareable
InstancedFilterfor future model support with overly specific non-instanced filters.Because of the new layering and new OpenAI library, we're also able to mock the OpenAI layer and test that our code handles responses in a reasonably safe way. Previously, we'd have to just try it out or wait for something to happen.
This refactoring also moves the
filter.Mediastruct tomedia.Itembecause that's probably where it should have been at first. The event rendering code is also moved to a dedicated package for reusability. Both moves are to prevent import cycles (filter -> ai -> filter).All of this means that when we go to add a second model, we'd:
aipackage (with appropriate test)filter_openai_omnito create a new non-instanced filter... and that's it, hopefully. This then takes care of per-room enablement, vendor lock-in avoidance, etc for us. (Vendor lock-in is avoided by simply adding more and more models with minimal dependencies or copy/paste.)
Pull Request Checklist
mainbranch.