Skip to content

Optimizer: enable by default#28213

Open
andig wants to merge 8 commits intomasterfrom
feat/optimizer-2
Open

Optimizer: enable by default#28213
andig wants to merge 8 commits intomasterfrom
feat/optimizer-2

Conversation

@andig
Copy link
Copy Markdown
Member

@andig andig commented Mar 14, 2026

Add ability to enable optimizer via UI.

  • enable "experimental"
  • enable "Optimizer 🧪" (Integrations)

Screenshots

Bildschirmfoto 2026-03-20 um 15 08 44 Bildschirmfoto 2026-03-17 um 17 07 48

Todos

Future
Add more status info like last update, server fails, ... to the optimizer tile.

@andig andig requested a review from naltatis March 14, 2026 16:43
@andig andig added the infrastructure Basic functionality label Mar 14, 2026
Copy link
Copy Markdown
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 - I've left some high level feedback:

  • The new OPTIMIZER_URI constant is exported and all-caps; consider using an unexported, Go-style optimizerURI (or similar) unless it’s intentionally part of the public API.
  • Since the optimizer is now always enabled with a default URL, consider whether the on/off behavior should be controlled explicitly (e.g., by a config flag) rather than implicitly via the environment variable.
  • You might want to read and cache the optimizer base URI once (e.g., at site initialization) instead of calling os.Getenv on every optimizerUpdate to avoid repeated environment lookups in a potentially hot path.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new `OPTIMIZER_URI` constant is exported and all-caps; consider using an unexported, Go-style `optimizerURI` (or similar) unless it’s intentionally part of the public API.
- Since the optimizer is now always enabled with a default URL, consider whether the on/off behavior should be controlled explicitly (e.g., by a config flag) rather than implicitly via the environment variable.
- You might want to read and cache the optimizer base URI once (e.g., at site initialization) instead of calling `os.Getenv` on every `optimizerUpdate` to avoid repeated environment lookups in a potentially hot path.

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.

@naltatis
Copy link
Copy Markdown
Member

We should do this as an opt-in, since we're transmitting usage data here. I'd introduce a dedicated optimizer toggle in the experimental dialog and also add a documentation page explaining the optimizer concept, its goals and its current state.

@naltatis naltatis self-assigned this Mar 14, 2026
@naltatis naltatis marked this pull request as draft March 14, 2026 18:05
@CiNcH83
Copy link
Copy Markdown
Contributor

CiNcH83 commented Mar 19, 2026

I wonder if "cloud-based" is the right terminology. At least not for people who run their local instance.

@naltatis
Copy link
Copy Markdown
Member

naltatis commented Mar 20, 2026

I wonder if "cloud-based" is the right terminology. At least not for people who run their local instance.

Good point. Updated the text to be more concrete and use "external service" terminology. (updated screenshot)

@naltatis naltatis marked this pull request as ready for review March 21, 2026 15:42
Copy link
Copy Markdown
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 - I've found 1 issue, and left some high level feedback:

  • The OPTIMIZER_URI constant in core/site_optimizer.go is exported but only used internally; consider making it unexported and following Go naming conventions (e.g. optimizerURI) to avoid polluting the public API surface.
  • You now have three helpers around optimizer/experimental state (optimizerEnabled, isOptimizer, isExperimental); consider consolidating them or reusing optimizerEnabled where appropriate to avoid diverging logic between CLI and runtime behavior.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `OPTIMIZER_URI` constant in `core/site_optimizer.go` is exported but only used internally; consider making it unexported and following Go naming conventions (e.g. `optimizerURI`) to avoid polluting the public API surface.
- You now have three helpers around optimizer/experimental state (`optimizerEnabled`, `isOptimizer`, `isExperimental`); consider consolidating them or reusing `optimizerEnabled` where appropriate to avoid diverging logic between CLI and runtime behavior.

## Individual Comments

### Comment 1
<location path="assets/js/components/Config/OptimizerModal.vue" line_range="56-65" />
<code_context>
+		},
+	},
+	methods: {
+		async change(e: Event) {
+			try {
+				this.error = null;
+				await api.post(`config/optimizer/${(e.target as HTMLInputElement).checked}`);
+				} catch (err) {
+				const e = err as AxiosError<{ error: string }>;
+				this.error = e.response?.data?.error || e.message;
+			}
</code_context>
<issue_to_address>
**suggestion:** Avoid reusing the name `e` for both the event parameter and the error object in `change`.

Inside `change`, `e` is used both as the `Event` parameter and then redefined in the `catch` block. Even though the scopes don’t conflict, this hurts readability and can cause confusion during future changes. Consider using clearer names, e.g. `event` for the parameter and `error`/`axiosError` in the `catch` block.

```suggestion
	methods: {
		async change(event: Event) {
			try {
				this.error = null;
				await api.post(`config/optimizer/${(event.target as HTMLInputElement).checked}`);
			} catch (err) {
				const axiosError = err as AxiosError<{ error: string }>;
				this.error = axiosError.response?.data?.error || axiosError.message;
			}
		},
```
</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.

@github-actions github-actions bot added the stale Outdated and ready to close label Mar 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Basic functionality stale Outdated and ready to close

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants