Skip to content

Conversation

andig
Copy link
Member

@andig andig commented Oct 8, 2025

This PR removes the special case of vehicle having their "own" title and icon instead of title/icon being part of the device meta data. This has the following consequences:

  • title/icon configuration via yaml is no longer possible- it is via config UI
  • vehicle API nick names are removed (it was never clear if an API or given title had preference)
  • this supported better OAuth user experience for Refactor provider authorization #24264

TODO

@andig andig added the infrastructure Basic functionality label Oct 8, 2025
@andig andig marked this pull request as draft October 8, 2025 19:45
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • There’s a lot of repetitive withContext(ctx) boilerplate in each vehicle constructor—consider refactoring the registry.AddCtx wrapper to automatically inject context into embeds and reduce duplication.
  • Instead of leaving large blocks of commented-out code (e.g. legacy site.Vehicles and Icon/SetTitle methods), remove them entirely to clean up the codebase and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- There’s a lot of repetitive withContext(ctx) boilerplate in each vehicle constructor—consider refactoring the registry.AddCtx wrapper to automatically inject context into embeds and reduce duplication.
- Instead of leaving large blocks of commented-out code (e.g. legacy site.Vehicles and Icon/SetTitle methods), remove them entirely to clean up the codebase and avoid confusion.

## Individual Comments

### Comment 1
<location> `core/site_vehicles.go:87` </location>
<code_context>
 }

-var _ site.Vehicles = (*vehicles)(nil)
+// var _ site.Vehicles = (*vehicles)(nil)

-type vehicles struct {
</code_context>

<issue_to_address>
**issue (complexity):** Consider removing the commented-out code to improve code clarity and maintainability.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andig andig marked this pull request as ready for review October 9, 2025 05:59
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andig andig changed the title Vehicles: remove title/icon from yaml config Vehicles: remove title/icon from yaml config (BC) Oct 9, 2025
@naltatis
Copy link
Member

naltatis commented Oct 14, 2025

@andig any chance we can keep supporting the deprecated yaml title/icon for a couple of releases? Since config ui is not marked as stable yet, there will be no non-experimental way to set a vehicle title. Having a vehicle title is much more critical from a user-base-perspective than meter titles.

If we can't support both, I'd say we should hold this PR until config ui is stable.

@naltatis
Copy link
Member

this supported better OAuth user experience

Is the new vehicle title api really necessary for this? Publishing a vehicle name would also be fine for UI. We have everything there to perform a lookup for title.

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

Labels

infrastructure Basic functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants