Skip to content

fix(web): address installation feedback from user testing#398

Merged
penso merged 5 commits intomainfrom
installation-feedback
Mar 12, 2026
Merged

fix(web): address installation feedback from user testing#398
penso merged 5 commits intomainfrom
installation-feedback

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented Mar 11, 2026

Summary

Fixes multiple issues reported during user installation testing (Discord feedback from David Cumps):

  • Tailscale onboarding: Fix field mismatch (connectedtailscale_up) causing "Installed but not connected" to never show as connected. Add "Configure in Settings" link when Tailscale is installed but not connected.
  • Model list sorting: Sort preferred models alphabetically by display name within groups instead of arbitrary order.
  • Emoji picker cutoff: Increase max-height from 240px to 320px so all 57 emojis are visible without scrolling.
  • ElevenLabs Scribe test error: Humanize browser getUserMedia errors ("Invalid constraint" → "No compatible microphone found").
  • Skill deletion UX: Add ServiceError::Forbidden variant with proper error code. Expose protected field in REST /api/skills response so the UI correctly disables the delete button for built-in skills (template-skill, template, tmux).
  • Auth protection gaps: Detect UNAUTHORIZED in RPC responses and trigger redirect to login. Clear cached sensitive data (sessions, models, identity) on logout. Hide session list when vault is sealed. Handle 401/403 in bootstrap fetch.

Feature requests filed separately

  • Local faster-whisper STT provider support
  • Headscale / direct Tailscale IP binding support

Validation

Completed

  • cargo check passes
  • cargo +nightly-2025-11-30 fmt --all -- --check passes
  • biome check --write passes (no new errors)
  • cargo test -p moltis-service-traits passes
  • cargo test -p moltis-chat passes

Remaining

  • ./scripts/local-validate.sh <PR_NUMBER>
  • Full cargo test
  • E2E tests for auth protection changes

Manual QA

  1. Tailscale: Run onboarding with Tailscale installed but not connected → should show warning icon + "Configure in Settings" link
  2. Models: Open model dropdown → preferred models should be sorted A-Z
  3. Emoji picker: Open Identity settings → emoji picker should show all emojis without scrollbar
  4. STT test: Configure ElevenLabs Scribe, test without microphone → should show human-readable error
  5. Skills: View skills page → template/tmux skills should have disabled delete button showing "Protected"
  6. Auth: Log out → session sidebar should show "Vault is sealed", no settings should be modifiable

- Fix Tailscale onboarding: use `tailscale_up` field (not `connected`)
  and add "Configure in Settings" link when installed but not connected
- Sort preferred model list alphabetically within groups
- Increase emoji picker max-height to prevent cutoff (240→320px)
- Humanize STT test errors (constraint, permission, device errors)
- Fix skill deletion: add ServiceError::Forbidden variant, expose
  `protected` field in REST API so UI disables delete for built-in skills
- Auth protection: detect UNAUTHORIZED in RPC responses, clear sensitive
  data on logout, hide session list when vault is sealed, handle 401/403
  in bootstrap fetch

Entire-Checkpoint: 7f64b9b73958
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq bot commented Mar 11, 2026

Merging this PR will degrade performance by 22.33%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 38 untouched benchmarks
⏩ 5 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
env_substitution 10 µs 12.8 µs -22.33%

Comparing installation-feedback (21f8c71) with main (742bce9)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR addresses a batch of user-reported installation pain points across the Tailscale onboarding, model picker sorting, emoji picker UX, microphone error messages, skill deletion protection, and auth session hygiene. The Rust-side changes (new ServiceError::Forbidden variant, is_protected_discovered_skill made public, protected field in the skills API) are clean and well-structured. The JavaScript auth-hardening changes (bootstrap 401/403 guard, clearSensitiveData on logout, UNAUTHORIZED detection in the WebSocket layer) are good in intent but have a few gaps noted below.

Key issues found:

  • Memory leak in session-list.js: gon.onChange("vault_status", setVaultStatus) is called in a useEffect with no cleanup. gon.onChange does not return an unsubscribe handle, so each component mount appends a stale listener that is never removed.
  • Incomplete vault-sealed UX in app.js: clearSensitiveData() clears session/model/identity data but does not call gon.set("vault_status", "sealed"), so the SessionList "Vault is sealed" placeholder described in the PR will not appear during the logout transition.
  • No debounce on UNAUTHORIZED dispatch in ws-connect.js: Multiple in-flight RPC calls expiring simultaneously each fire moltis:auth-status-changed, triggering redundant parallel fetches to /api/auth/status.
  • Minor server/UI guard mismatch in api.rs: The UI marks a skill protected only for Personal/Project sources, while the server rejects deletes by name alone — a latent inconsistency if other source types are introduced.

Confidence Score: 3/5

  • Safe to merge after addressing the gon.onChange memory leak and the missing vault_status update in clearSensitiveData.
  • The backend Rust changes are solid and well-tested. The majority of the JS fixes are correct. However, the gon.onChange listener leak in a Preact component is a real bug that will accumulate stale closures over the component lifecycle, and the clearSensitiveData gap means the stated "Vault is sealed" logout UX won't actually appear as described. These two issues are directly tied to the PR's stated goals and should be resolved before merging.
  • crates/web/src/assets/js/components/session-list.js (listener leak) and crates/web/src/assets/js/app.js (missing vault_status update in clearSensitiveData).

Important Files Changed

Filename Overview
crates/web/src/assets/js/components/session-list.js Adds vault-sealed guard to hide session list; introduces a memory leak — gon.onChange listener registered in a useEffect with no cleanup, causing stale listeners to accumulate on each component mount.
crates/web/src/assets/js/app.js Adds clearSensitiveData() on logout and 401/403 handling in bootstrap fetch; clearSensitiveData omits gon.set("vault_status", "sealed"), so the session-list "Vault is sealed" placeholder won't appear during the logout transition as described in the PR.
crates/web/src/assets/js/ws-connect.js Dispatches moltis:auth-status-changed on UNAUTHORIZED RPC errors; multiple concurrent UNAUTHORIZED responses can fire the event many times without debouncing.
crates/web/src/api.rs Exposes protected field for discovered skills; guards against Personal/Project sources which is slightly stricter than the server-side name-only check in delete_discovered_skill, creating a theoretical UI/server mismatch.
crates/service-traits/src/lib.rs Clean addition of ServiceError::Forbidden variant with proper FORBIDDEN error code mapping — well-structured and does not break existing error handling.
crates/gateway/src/services.rs Makes is_protected_discovered_skill public and upgrades the deletion error from a generic message to ServiceError::Forbidden — straightforward and correct.
crates/chat/src/lib.rs Sorting now alphabetises preferred models by display name rather than preserving user-specified preference order; functionally correct per the PR goal but overrides explicit priority ordering within the preferred group.
crates/web/src/assets/js/onboarding-view.js Fixes connectedtailscale_up field mismatch and adds "Configure in Settings" link for the installed-but-not-connected state — straightforward and correct.
crates/web/src/assets/js/page-settings.js Extracts humanizeMicError helper to map browser getUserMedia error names to user-friendly messages; well-structured with no issues.
crates/web/src/assets/css/components.css Single-line max-height increase from 240px to 320px for the emoji picker — correct and safe.

Sequence Diagram

sequenceDiagram
    participant UI as Browser UI
    participant WS as ws-connect.js
    participant App as app.js
    participant API as /api/*
    participant Server as Rust Backend

    Note over UI,Server: Normal auth expiry / logout flow

    UI->>WS: RPC request (any method)
    Server-->>WS: res { error: { code: "UNAUTHORIZED" } }
    WS->>UI: dispatchEvent("moltis:auth-status-changed")

    UI->>API: GET /api/auth/status
    API-->>UI: { authenticated: false }
    UI->>UI: clearSensitiveData()
    Note right of UI: sessions/models/identity cleared<br/>vault_status NOT set → "sealed"
    UI->>UI: window.location.assign("/login")

    Note over UI,Server: Bootstrap fetch 401/403 guard
    UI->>API: GET /api/bootstrap
    API-->>UI: 401 or 403
    UI->>UI: dispatchEvent("moltis:auth-status-changed")
    UI->>API: GET /api/auth/status
    API-->>UI: { authenticated: false }
    UI->>UI: clearSensitiveData() → redirect /login

    Note over UI,Server: Skill delete protection
    UI->>API: DELETE skill (template / tmux)
    API-->>UI: 403 Forbidden (ServiceError::Forbidden)
    Note right of UI: protected=true disables button<br/>in UI before request is made
Loading

Last reviewed commit: f09063f

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f09063f50e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- Add gon.offChange() and clean up listener in SessionList useEffect
- Debounce UNAUTHORIZED auth redirect with a flag to avoid N concurrent
  dispatches when multiple RPCs fail simultaneously
- Set vault_status to "sealed" in clearSensitiveData() so SessionList
  shows the correct placeholder on logout
- Simplify protected skill check in REST API to match server-side logic
  (check name only, not source type)

Entire-Checkpoint: 0d9f0ce04c2b
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 947a8c6bf6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Adjust expected order in priority_models_pin_raw_model_ids_first,
priority_models_match_separator_variants, and
models_without_priority_prefer_subscription_providers to reflect
alphabetical sorting within preferred and non-preferred groups.

Entire-Checkpoint: d8d3382a3e17
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 67.39130% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/service-traits/src/lib.rs 0.00% 9 Missing ⚠️
crates/gateway/src/services.rs 0.00% 4 Missing ⚠️
crates/web/src/api.rs 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@penso penso merged commit c2b6109 into main Mar 12, 2026
33 of 35 checks passed
@penso penso deleted the installation-feedback branch March 12, 2026 12:03
penso added a commit that referenced this pull request Mar 23, 2026
* fix(web): address installation feedback from user testing

- Fix Tailscale onboarding: use `tailscale_up` field (not `connected`)
  and add "Configure in Settings" link when installed but not connected
- Sort preferred model list alphabetically within groups
- Increase emoji picker max-height to prevent cutoff (240→320px)
- Humanize STT test errors (constraint, permission, device errors)
- Fix skill deletion: add ServiceError::Forbidden variant, expose
  `protected` field in REST API so UI disables delete for built-in skills
- Auth protection: detect UNAUTHORIZED in RPC responses, clear sensitive
  data on logout, hide session list when vault is sealed, handle 401/403
  in bootstrap fetch

Entire-Checkpoint: 7f64b9b73958

* fix(web): address PR review feedback

- Add gon.offChange() and clean up listener in SessionList useEffect
- Debounce UNAUTHORIZED auth redirect with a flag to avoid N concurrent
  dispatches when multiple RPCs fail simultaneously
- Set vault_status to "sealed" in clearSensitiveData() so SessionList
  shows the correct placeholder on logout
- Simplify protected skill check in REST API to match server-side logic
  (check name only, not source type)

Entire-Checkpoint: 0d9f0ce04c2b

* test(chat): update model ordering tests for alphabetical sort

Adjust expected order in priority_models_pin_raw_model_ids_first,
priority_models_match_separator_variants, and
models_without_priority_prefer_subscription_providers to reflect
alphabetical sorting within preferred and non-preferred groups.

Entire-Checkpoint: d8d3382a3e17

* fix(review): address remaining PR feedback

* style(web): apply biome formatting
jmikedupont2 pushed a commit to meta-introspector/moltis that referenced this pull request Mar 23, 2026
…#398)

* fix(web): address installation feedback from user testing

- Fix Tailscale onboarding: use `tailscale_up` field (not `connected`)
  and add "Configure in Settings" link when installed but not connected
- Sort preferred model list alphabetically within groups
- Increase emoji picker max-height to prevent cutoff (240→320px)
- Humanize STT test errors (constraint, permission, device errors)
- Fix skill deletion: add ServiceError::Forbidden variant, expose
  `protected` field in REST API so UI disables delete for built-in skills
- Auth protection: detect UNAUTHORIZED in RPC responses, clear sensitive
  data on logout, hide session list when vault is sealed, handle 401/403
  in bootstrap fetch

Entire-Checkpoint: 7f64b9b73958

* fix(web): address PR review feedback

- Add gon.offChange() and clean up listener in SessionList useEffect
- Debounce UNAUTHORIZED auth redirect with a flag to avoid N concurrent
  dispatches when multiple RPCs fail simultaneously
- Set vault_status to "sealed" in clearSensitiveData() so SessionList
  shows the correct placeholder on logout
- Simplify protected skill check in REST API to match server-side logic
  (check name only, not source type)

Entire-Checkpoint: 0d9f0ce04c2b

* test(chat): update model ordering tests for alphabetical sort

Adjust expected order in priority_models_pin_raw_model_ids_first,
priority_models_match_separator_variants, and
models_without_priority_prefer_subscription_providers to reflect
alphabetical sorting within preferred and non-preferred groups.

Entire-Checkpoint: d8d3382a3e17

* fix(review): address remaining PR feedback

* style(web): apply biome formatting
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.

1 participant