generated from nhcarrigan/template
fix: critical permission modal and config issues (#127)
## Summary This PR resolves several critical bugs that were blocking the permission modal and causing config loss: - **Permission modal not appearing** - Fixed z-index issues and runtime errors - **Config store race condition** - Resolved critical race condition causing settings to be lost - **Excessive logging** - Removed redundant fmt layer that was writing to hidden stdout - **System tool prompts** - Prevented unnecessary permission prompts for built-in tools - **Permission batching** - Added support for parallel permission requests - **ExitPlanMode tool** - Fixed ExitPlanMode tool not functioning correctly ## Changes Made ### Permission Modal Fixes - Updated z-index to proper value (9999) to ensure modal appears above all other UI elements - Fixed runtime errors that were preventing modal from rendering - Resolved issues with permission grants not being properly applied ### Config Store Race Condition - Fixed critical race condition where multiple rapid config updates would result in lost settings - Ensured config writes are properly sequenced to prevent data loss - Added proper synchronisation for config store operations ### Logging Cleanup - Removed redundant fmt formatting layer that was outputting to hidden stdout - Cleaned up excessive debug logging added during troubleshooting - Removed temporary debugging documentation files ### UX Improvements - Added close confirmation modal with minimise to tray option - Implemented batching for parallel permission requests - Added debug console for viewing frontend and backend logs ### ExitPlanMode Fix - Fixed ExitPlanMode tool not functioning correctly, ensuring proper transitions out of plan mode ## Issues Resolved Closes #112 - Permission flow now properly handles multiple tool requests Closes #113 - ExitPlanMode tool now functions correctly Closes #126 - Debug console feature added (partial - basic implementation complete) ## Test Plan - [x] Permission modal appears and functions correctly - [x] Config settings persist across app restarts - [x] No excessive logging in production builds - [x] System tools don't trigger permission prompts - [x] Parallel permission requests are properly batched - [x] Debug console displays frontend and backend logs - [x] ExitPlanMode properly exits plan mode --- ✨ This PR was created with help from Hikari~ 🌸 Co-authored-by: Naomi Carrigan <commits@nhcarrigan.com> Reviewed-on: #127 Co-authored-by: Hikari <hikari@nhcarrigan.com> Co-committed-by: Hikari <hikari@nhcarrigan.com>
This commit was merged in pull request #127.
This commit is contained in:
@@ -14,6 +14,7 @@ import {
|
||||
type Theme,
|
||||
type CustomThemeColors,
|
||||
} from "./config";
|
||||
import { invoke } from "@tauri-apps/api/core";
|
||||
|
||||
// Mock Tauri APIs
|
||||
vi.mock("@tauri-apps/api/core", () => ({
|
||||
@@ -167,7 +168,6 @@ describe("config store", () => {
|
||||
notifications_enabled: true,
|
||||
notification_volume: 0.7,
|
||||
always_on_top: false,
|
||||
minimize_to_tray: true,
|
||||
update_checks_enabled: true,
|
||||
character_panel_width: 300,
|
||||
font_size: 14,
|
||||
@@ -213,7 +213,6 @@ describe("config store", () => {
|
||||
notifications_enabled: true,
|
||||
notification_volume: 0.7,
|
||||
always_on_top: false,
|
||||
minimize_to_tray: false,
|
||||
update_checks_enabled: true,
|
||||
character_panel_width: null,
|
||||
font_size: 14,
|
||||
@@ -489,4 +488,329 @@ describe("config store", () => {
|
||||
expect(typeof configStore.saveError.subscribe).toBe("function");
|
||||
});
|
||||
});
|
||||
|
||||
describe("Race Condition Tests", () => {
|
||||
beforeEach(async () => {
|
||||
// Setup mock to return a default config for load_config
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
mockInvokeImpl.mockResolvedValue({
|
||||
model: null,
|
||||
api_key: null,
|
||||
custom_instructions: null,
|
||||
mcp_servers_json: null,
|
||||
auto_granted_tools: [],
|
||||
theme: "dark",
|
||||
greeting_enabled: false,
|
||||
greeting_custom_prompt: null,
|
||||
notifications_enabled: false,
|
||||
notification_volume: 0.7,
|
||||
always_on_top: false,
|
||||
update_checks_enabled: false,
|
||||
character_panel_width: null,
|
||||
font_size: 14,
|
||||
streamer_mode: false,
|
||||
streamer_hide_paths: false,
|
||||
compact_mode: false,
|
||||
profile_name: null,
|
||||
profile_avatar_path: null,
|
||||
profile_bio: null,
|
||||
custom_theme_colors: {
|
||||
bg_primary: null,
|
||||
bg_secondary: null,
|
||||
bg_terminal: null,
|
||||
accent_primary: null,
|
||||
accent_secondary: null,
|
||||
text_primary: null,
|
||||
text_secondary: null,
|
||||
border_color: null,
|
||||
},
|
||||
budget_enabled: false,
|
||||
session_token_budget: null,
|
||||
session_cost_budget: null,
|
||||
budget_action: "warn",
|
||||
budget_warning_threshold: 0.8,
|
||||
discord_rpc_enabled: false,
|
||||
});
|
||||
|
||||
// Load initial config
|
||||
await configStore.loadConfig();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("handles rapid sequential config updates correctly", async () => {
|
||||
// This test validates the fix for the config race condition that caused data loss
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
const invokeCalls: Array<{ command: string; config: HikariConfig }> = [];
|
||||
|
||||
mockInvokeImpl.mockImplementation(async (command: string, args?: unknown) => {
|
||||
if (command === "save_config" && args && typeof args === "object" && "config" in args) {
|
||||
invokeCalls.push({ command, config: args.config as HikariConfig });
|
||||
// Simulate small delay in saving
|
||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||
}
|
||||
return null;
|
||||
});
|
||||
|
||||
// Perform rapid updates
|
||||
await Promise.all([
|
||||
configStore.updateConfig({ font_size: 16 }),
|
||||
configStore.updateConfig({ theme: "light" }),
|
||||
configStore.updateConfig({ compact_mode: true }),
|
||||
]);
|
||||
|
||||
// All three updates should have been saved
|
||||
expect(invokeCalls.length).toBe(3);
|
||||
|
||||
// Get final config
|
||||
const finalConfig = configStore.getConfig();
|
||||
|
||||
// Final config should have all updates
|
||||
// Note: The last update wins for each field, but all fields should be preserved
|
||||
expect(finalConfig.compact_mode).toBe(true);
|
||||
});
|
||||
|
||||
it("preserves previous field values during concurrent updates", async () => {
|
||||
// Set initial values
|
||||
await configStore.updateConfig({
|
||||
font_size: 16,
|
||||
theme: "dark",
|
||||
compact_mode: false,
|
||||
streamer_mode: false,
|
||||
});
|
||||
|
||||
vi.clearAllMocks();
|
||||
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
const invokeCalls: Array<{ command: string; config: HikariConfig }> = [];
|
||||
|
||||
mockInvokeImpl.mockImplementation(async (command: string, args?: unknown) => {
|
||||
if (command === "save_config" && args && typeof args === "object" && "config" in args) {
|
||||
invokeCalls.push({ command, config: args.config as HikariConfig });
|
||||
await new Promise((resolve) => setTimeout(resolve, 5));
|
||||
}
|
||||
return null;
|
||||
});
|
||||
|
||||
// Update different fields concurrently
|
||||
await Promise.all([
|
||||
configStore.updateConfig({ font_size: 18 }),
|
||||
configStore.updateConfig({ theme: "light" }),
|
||||
configStore.updateConfig({ compact_mode: true }),
|
||||
]);
|
||||
|
||||
// Check that each save included all previous config values
|
||||
invokeCalls.forEach((call) => {
|
||||
// Each save should have a complete config, not just the updated field
|
||||
expect(call.config).toHaveProperty("font_size");
|
||||
expect(call.config).toHaveProperty("theme");
|
||||
expect(call.config).toHaveProperty("compact_mode");
|
||||
expect(call.config).toHaveProperty("streamer_mode");
|
||||
expect(call.config).toHaveProperty("model");
|
||||
expect(call.config).toHaveProperty("api_key");
|
||||
});
|
||||
});
|
||||
|
||||
it("handles update during save operation", async () => {
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
let firstSaveStarted = false;
|
||||
let firstSaveCompleted = false;
|
||||
|
||||
mockInvokeImpl.mockImplementation(async (command: string) => {
|
||||
if (command === "save_config") {
|
||||
if (!firstSaveStarted) {
|
||||
firstSaveStarted = true;
|
||||
// Simulate slow save
|
||||
await new Promise((resolve) => setTimeout(resolve, 50));
|
||||
firstSaveCompleted = true;
|
||||
} else {
|
||||
// Second save starts while first is in progress
|
||||
expect(firstSaveStarted).toBe(true);
|
||||
// First save might not be complete yet (race condition scenario)
|
||||
}
|
||||
}
|
||||
return null;
|
||||
});
|
||||
|
||||
// Start first update
|
||||
const firstUpdate = configStore.updateConfig({ font_size: 16 });
|
||||
|
||||
// Wait a bit then start second update whilst first is still saving
|
||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||
const secondUpdate = configStore.updateConfig({ theme: "light" });
|
||||
|
||||
// Wait for both to complete
|
||||
await Promise.all([firstUpdate, secondUpdate]);
|
||||
|
||||
// Both should complete successfully without errors
|
||||
expect(firstSaveCompleted).toBe(true);
|
||||
});
|
||||
|
||||
it("getConfig returns most recently set configuration", async () => {
|
||||
await configStore.updateConfig({ font_size: 14 });
|
||||
expect(configStore.getConfig().font_size).toBe(14);
|
||||
|
||||
await configStore.updateConfig({ font_size: 16 });
|
||||
expect(configStore.getConfig().font_size).toBe(16);
|
||||
|
||||
await configStore.updateConfig({ font_size: 18 });
|
||||
expect(configStore.getConfig().font_size).toBe(18);
|
||||
});
|
||||
|
||||
it("updates do not lose data from previous operations", async () => {
|
||||
// Set multiple fields
|
||||
await configStore.updateConfig({
|
||||
font_size: 16,
|
||||
theme: "dark",
|
||||
compact_mode: true,
|
||||
streamer_mode: true,
|
||||
model: "claude-sonnet-4",
|
||||
});
|
||||
|
||||
// Update just one field
|
||||
await configStore.updateConfig({ theme: "light" });
|
||||
|
||||
// Other fields should be preserved
|
||||
const config = configStore.getConfig();
|
||||
expect(config.theme).toBe("light");
|
||||
expect(config.font_size).toBe(16);
|
||||
expect(config.compact_mode).toBe(true);
|
||||
expect(config.streamer_mode).toBe(true);
|
||||
expect(config.model).toBe("claude-sonnet-4");
|
||||
});
|
||||
|
||||
it("auto granted tools are not lost during other updates", async () => {
|
||||
// Add some tools
|
||||
await configStore.addAutoGrantedTool("Read");
|
||||
await configStore.addAutoGrantedTool("Write");
|
||||
|
||||
expect(configStore.getConfig().auto_granted_tools).toContain("Read");
|
||||
expect(configStore.getConfig().auto_granted_tools).toContain("Write");
|
||||
|
||||
// Update another field
|
||||
await configStore.updateConfig({ theme: "light" });
|
||||
|
||||
// Tools should still be there
|
||||
expect(configStore.getConfig().auto_granted_tools).toContain("Read");
|
||||
expect(configStore.getConfig().auto_granted_tools).toContain("Write");
|
||||
});
|
||||
|
||||
it("custom theme colors persist across other config updates", async () => {
|
||||
const customColors: CustomThemeColors = {
|
||||
bg_primary: "#1a1a2e",
|
||||
bg_secondary: "#16213e",
|
||||
bg_terminal: "#0f0f23",
|
||||
accent_primary: "#e94560",
|
||||
accent_secondary: "#533483",
|
||||
text_primary: "#eaeaea",
|
||||
text_secondary: "#a0a0a0",
|
||||
border_color: "#333355",
|
||||
};
|
||||
|
||||
await configStore.setCustomThemeColors(customColors);
|
||||
|
||||
// Update another field
|
||||
await configStore.updateConfig({ font_size: 18 });
|
||||
|
||||
// Colors should still be there
|
||||
const config = configStore.getConfig();
|
||||
expect(config.custom_theme_colors.bg_primary).toBe("#1a1a2e");
|
||||
expect(config.custom_theme_colors.accent_primary).toBe("#e94560");
|
||||
});
|
||||
|
||||
it("handles save errors gracefully without losing data", async () => {
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
|
||||
// Set initial config
|
||||
await configStore.updateConfig({ font_size: 14 });
|
||||
|
||||
// Make next save fail
|
||||
mockInvokeImpl.mockRejectedValueOnce(new Error("Save failed"));
|
||||
|
||||
// Try to update - should throw
|
||||
await expect(configStore.updateConfig({ theme: "light" })).rejects.toThrow();
|
||||
|
||||
// Original config should still be accessible
|
||||
expect(configStore.getConfig().font_size).toBe(14);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Config Persistence Tests", () => {
|
||||
it("loadConfig retrieves saved configuration", async () => {
|
||||
const mockConfig: HikariConfig = {
|
||||
model: "claude-sonnet-4",
|
||||
api_key: "test-key",
|
||||
custom_instructions: "Be helpful",
|
||||
mcp_servers_json: "{}",
|
||||
auto_granted_tools: ["Read", "Write"],
|
||||
theme: "light",
|
||||
greeting_enabled: false,
|
||||
greeting_custom_prompt: null,
|
||||
notifications_enabled: false,
|
||||
notification_volume: 0.5,
|
||||
always_on_top: true,
|
||||
update_checks_enabled: false,
|
||||
character_panel_width: 400,
|
||||
font_size: 18,
|
||||
streamer_mode: true,
|
||||
streamer_hide_paths: true,
|
||||
compact_mode: true,
|
||||
profile_name: "Test User",
|
||||
profile_avatar_path: "/test/avatar.png",
|
||||
profile_bio: "Test bio",
|
||||
custom_theme_colors: {
|
||||
bg_primary: null,
|
||||
bg_secondary: null,
|
||||
bg_terminal: null,
|
||||
accent_primary: null,
|
||||
accent_secondary: null,
|
||||
text_primary: null,
|
||||
text_secondary: null,
|
||||
border_color: null,
|
||||
},
|
||||
budget_enabled: true,
|
||||
session_token_budget: 100000,
|
||||
session_cost_budget: 1.5,
|
||||
budget_action: "block",
|
||||
budget_warning_threshold: 0.9,
|
||||
discord_rpc_enabled: false,
|
||||
};
|
||||
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
mockInvokeImpl.mockResolvedValueOnce(mockConfig);
|
||||
|
||||
await configStore.loadConfig();
|
||||
|
||||
const loadedConfig = configStore.getConfig();
|
||||
expect(loadedConfig.model).toBe("claude-sonnet-4");
|
||||
expect(loadedConfig.theme).toBe("light");
|
||||
expect(loadedConfig.font_size).toBe(18);
|
||||
expect(loadedConfig.auto_granted_tools).toEqual(["Read", "Write"]);
|
||||
});
|
||||
|
||||
it("saveConfig persists configuration to backend", async () => {
|
||||
const mockInvokeImpl = vi.mocked(invoke);
|
||||
const savedConfigs: HikariConfig[] = [];
|
||||
|
||||
mockInvokeImpl.mockImplementation(async (command: string, args?: unknown) => {
|
||||
if (command === "save_config" && args && typeof args === "object" && "config" in args) {
|
||||
savedConfigs.push(args.config as HikariConfig);
|
||||
}
|
||||
return null;
|
||||
});
|
||||
|
||||
const configToSave: Partial<HikariConfig> = {
|
||||
model: "claude-sonnet-4",
|
||||
theme: "dark",
|
||||
font_size: 16,
|
||||
};
|
||||
|
||||
await configStore.updateConfig(configToSave);
|
||||
|
||||
expect(savedConfigs.length).toBeGreaterThan(0);
|
||||
const lastSaved = savedConfigs[savedConfigs.length - 1];
|
||||
expect(lastSaved.model).toBe("claude-sonnet-4");
|
||||
expect(lastSaved.theme).toBe("dark");
|
||||
expect(lastSaved.font_size).toBe(16);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user