Conversation
|
Bosun CI signal: Bosun-created PR currently has failing checks.
|
There was a problem hiding this comment.
Pull request overview
Adds a new Ink-based TUI “Settings” screen intended to edit bosun.config.json inline (schema-driven) with AJV validation, and attempts to trigger runtime config reloads after saves.
Changes:
- Introduces
ui/tui/SettingsScreen.jsto flattenbosun.schema.json, render grouped fields, edit values, validate, and persist changes. - Adds a process-local
config-eventsemitter/listener and wires the Settings tab to render the new screen. - Adds/updates tests for the Settings screen and attempts to hook monitor reload behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/tui/constants.js | Removes the figures dependency usage by swapping to an empty figures object with fallbacks. |
| ui/tui/config-events.js | Adds a globalThis-backed EventEmitter for config-reload events. |
| ui/tui/SettingsScreen.js | New Settings screen: schema flattening, inline editing, AJV validation, atomic write, emits reload event. |
| ui/tui/App.js | Wires Settings tab to SettingsScreen; refactors some tab rendering (including Logs). |
| tests/tui/settings-screen.test.mjs | Adds SettingsScreen tests using Ink test harness (currently schema-mismatched assertions). |
| tests/tui-settings-screen.test.mjs | Adds another SettingsScreen test file (overlapping intent; also schema-mismatched). |
| infra/monitor.mjs | Attempts to listen for config reload events and call reloadConfig (currently contains invalid injected rn sequences). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const originalEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| process.env = { ...originalEnv }; | ||
| delete process.env.KANBAN_BACKEND; | ||
| delete process.env.TELEGRAM_UI_PORT; | ||
| delete process.env.TELEGRAM_BOT_TOKEN; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env = originalEnv; | ||
| vi.restoreAllMocks(); |
There was a problem hiding this comment.
originalEnv is captured as a reference (const originalEnv = process.env) and later restored via process.env = originalEnv, which can reintroduce mutations from other tests. Use the repo’s common pattern of snapshotting (const originalEnv = { ...process.env }) and restoring with a shallow copy in afterEach/beforeEach.
| it("renders grouped config fields with masked secrets and source labels", async () => { | ||
| process.env.TELEGRAM_BOT_TOKEN = "env-token"; | ||
|
|
||
| const { dir } = createConfigDir({ | ||
| kanban: { backend: "github" }, | ||
| telegram: { uiPort: 3080, token: "config-token" }, | ||
| costRates: { inputPer1M: 1.5 }, | ||
| }); | ||
|
|
||
| const view = await renderInk( | ||
| React.createElement(SettingsScreen, { | ||
| configDir: dir, | ||
| config: { kanban: { backend: "github" }, telegram: { uiPort: 3080, token: "config-token" }, costRates: { inputPer1M: 1.5 } }, | ||
| }), | ||
| { columns: 220, rows: 60 }, | ||
| ); | ||
|
|
||
| const text = view.text(); | ||
| expect(text).toContain("General"); | ||
| expect(text).toContain("Kanban"); | ||
| expect(text).toContain("Integrations"); | ||
| expect(text).toContain("Cost Rates"); | ||
| expect(text).toContain("backend"); | ||
| expect(text).toContain("github"); | ||
| expect(text).toContain("from config"); | ||
| expect(text).toContain("from env"); | ||
| expect(text).toContain("****"); | ||
| expect(text).not.toContain("env-token"); | ||
|
|
There was a problem hiding this comment.
This test expects schema-driven fields like telegram.uiPort / telegram.token and costRates.*, but bosun.schema.json does not define a telegram object or costRates, so SettingsScreen won’t render these rows and waitFor(() => view.text().includes("uiPort: 3080")) will time out. Update the test to use fields that exist in the schema (e.g., cloudflareDnsMaxRetries, kanban.backend, kanban.github.project.webhook.secret, etc.) and the correct env var names.
| } from "../task/task-store.mjs"; | ||
| import { createAgentEndpoint } from "../agent/agent-endpoint.mjs"; | ||
| import { createAgentEventBus } from "../agent/agent-event-bus.mjs"; | ||
| import { createAgentEventBus } from "../agent/agent-event-bus.mjs";`r`nimport { onConfigReload } from "../ui/tui/config-events.js"; |
There was a problem hiding this comment.
The new import line contains literal rncharacters (likely from an escaped CRLF) which makes the file invalid JavaScript. Split this into a normal standaloneimport { onConfigReload ... }` statement on its own line and remove the stray characters/backticks.
| import { createAgentEventBus } from "../agent/agent-event-bus.mjs";`r`nimport { onConfigReload } from "../ui/tui/config-events.js"; | |
| import { createAgentEventBus } from "../agent/agent-event-bus.mjs"; | |
| import { onConfigReload } from "../ui/tui/config-events.js"; |
| process.on("exit", () => {`r`n try { stopTuiConfigReloadListener?.(); } catch { /* best effort */ } | ||
| shuttingDown = true; |
There was a problem hiding this comment.
The exit handler line is missing proper newlines/bracing and includes stray rntext, leaving the handler body unterminated. Format the handler as a normal multi-line function and ensurestopTuiConfigReloadListener()` is called inside a try/finally (or best-effort try/catch) without breaking the rest of the shutdown logic.
| function atomicWriteJson(path, config) { | ||
| const tempPath = `${path}.tmp`; | ||
| writeFileSync(tempPath, `${JSON.stringify(config, null, 2)}\n`, "utf8"); | ||
| renameSync(tempPath, path); | ||
| } |
There was a problem hiding this comment.
atomicWriteJson uses renameSync(tempPath, path) without any fallback/cleanup. On Windows (and some network filesystems) rename can fail when the destination exists or is locked, leaving the .tmp file behind. Follow the repo’s existing atomic-write pattern (tmp + rename with fallback to direct write/copy + best-effort temp cleanup).
|
|
||
| export function emitConfigReload(payload = {}) { | ||
| bus.emit("config-reload", payload); |
There was a problem hiding this comment.
This event bus is process-local (globalThis + EventEmitter), so emitConfigReload() from the TUI won’t reach the running monitor/ui-server process (which is a separate Node process). If the intent is to reload runtime config when SettingsScreen saves, this needs to go over an IPC boundary (e.g., a WebSocket command/event to the server/monitor, a REST endpoint, or a file watcher in the monitor), not an in-memory emitter.
| export function emitConfigReload(payload = {}) { | |
| bus.emit("config-reload", payload); | |
| /** | |
| * Attempt to notify a parent/monitor process about a config reload over IPC. | |
| * This uses Node's built-in `process.send` when an IPC channel is available | |
| * (e.g., when this process was spawned with the `ipc` stdio option). | |
| */ | |
| function sendConfigReloadIPC(payload) { | |
| if (typeof process !== "undefined" && typeof process.send === "function") { | |
| try { | |
| process.send({ type: "config-reload", payload }); | |
| } catch { | |
| // Swallow IPC errors to avoid breaking local event emission. | |
| } | |
| } | |
| } | |
| export function emitConfigReload(payload = {}) { | |
| // Notify listeners within this process. | |
| bus.emit("config-reload", payload); | |
| // Also attempt to notify any parent/monitor process over IPC. | |
| sendConfigReloadIPC(payload); |
| return JSON.parse(readFileSync(path, "utf8")); | ||
| } | ||
|
|
||
| describe("tui settings screen", () => { |
There was a problem hiding this comment.
There are now two separate SettingsScreen test files (tests/tui/settings-screen.test.mjs and tests/tui-settings-screen.test.mjs) that largely overlap in intent. Consider consolidating into one location/pattern to avoid redundant coverage and future drift (pick either the tests/tui/* convention or the root tests/tui-*.test.mjs convention used elsewhere).
| describe("tui settings screen", () => { | |
| describe.skip("tui settings screen (moved to tests/tui/settings-screen.test.mjs)", () => { |
infra/monitor.mjs
Outdated
| const stopTuiConfigReloadListener = onConfigReload((payload = {}) => {`r`n runDetached("config-reload:tui", () => reloadConfig(payload.reason || "tui-settings"));`r`n});`r`n`r`nprocess.on("SIGINT", async () => { | ||
| shuttingDown = true; | ||
| stopWorkspaceSyncTimers(); |
There was a problem hiding this comment.
This onConfigReload listener block is currently injected inline with literal rnsequences and is concatenated with the followingprocess.on("SIGINT")handler, which will break parsing. Rewrite this section as properly separated statements (listener registration, thenprocess.on`), and ensure the listener cleanup is syntactically correct and runs during shutdown.
| <//> | ||
| `; | ||
| body = html`<${ScreenFrame} title="Logs" subtitle="Recent monitor and transport events."> | ||
| ${wsState.logs.length ? wsState.logs.slice(0, 12).map((entry, index) => html`<${Text} key=${index}>${entry}<//>`) : html`<${Text} color=${ANSI_COLORS.muted}>No log entries yet.<//>`} |
There was a problem hiding this comment.
wsState.logs entries are logs:stream payload objects (with fields like timestamp/level/line/raw). Rendering ${entry} will display [object Object] and loses the useful formatting previously implemented. Render a formatted string (e.g., timestamp + level + line/raw) or extract entry.line/entry.raw instead.
| ${wsState.logs.length ? wsState.logs.slice(0, 12).map((entry, index) => html`<${Text} key=${index}>${entry}<//>`) : html`<${Text} color=${ANSI_COLORS.muted}>No log entries yet.<//>`} | |
| ${wsState.logs.length | |
| ? wsState.logs.slice(0, 12).map((entry, index) => { | |
| const timestamp = entry?.timestamp ? String(entry.timestamp) : ""; | |
| const level = entry?.level ? String(entry.level).toUpperCase() : ""; | |
| const message = entry?.line ?? entry?.raw ?? JSON.stringify(entry); | |
| const parts = []; | |
| if (timestamp) parts.push(timestamp); | |
| if (level) parts.push(`[${level}]`); | |
| if (message) parts.push(message); | |
| const formatted = parts.join(" "); | |
| return html`<${Text} key=${index}>${formatted}<//>`; | |
| }) | |
| : html`<${Text} color=${ANSI_COLORS.muted}>No log entries yet.<//>`} |
| const ENV_PATHS = Object.freeze({ | ||
| "kanban.backend": "KANBAN_BACKEND", | ||
| telegramUiPort: "TELEGRAM_UI_PORT", | ||
| "githubProjectSync.webhookSecret": "GITHUB_WEBHOOK_SECRET", | ||
| "linear.apiKey": "LINEAR_API_KEY", | ||
| "costRates.inputPer1M": "COST_RATES_INPUT_PER_1M", | ||
| "costRates.outputPer1M": "COST_RATES_OUTPUT_PER_1M", | ||
| }); |
There was a problem hiding this comment.
ENV_PATHS keys here don’t match the actual config/schema paths (e.g., githubProjectSync.webhookSecret isn’t a config key; the config loader reads kanban.github.project.webhook.secret, and the env var is GITHUB_PROJECT_WEBHOOK_SECRET/GITHUB_WEBHOOK_SECRET). As written, many fields will never show from env/read-only status. Align these mappings with the real config shape in config/config.mjs.
eadff8e to
df1b666
Compare
|
Bosun PR classification: Bosun-created.
|
…ead-edit-bosun-config-
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Task-ID: 3ad65810-1698-452e-91e0-ffc3822728d0\n\nAutomated PR for task 3ad65810-1698-452e-91e0-ffc3822728d0