Dashboard: prominent, provider-gated Memory Formation control on Settings#17
Merged
Merged
Conversation
…ings
Memory formation is now a real, discoverable setting (the honeycomb daemon
exposes `POST /api/actions/memory` + `/health reasons.memory`). Add a prominent
"Memory Formation" section to the Settings page, mirroring the existing
embeddings control piece-for-piece through the shared `wire` client + health
reasons block (no bespoke data path):
- wire.ts: `actionsMemory` endpoint, `MemoryActionSchema` (echo-check),
`reasons.memory { enabled, provider }` on HealthReasonsSchema (optional +
fail-closed: provider defaults "unconfigured" on absent/malformed payload),
and `setMemory(enabled)` mirroring `setEmbeddings`.
- settings.tsx: `MemoryFormationSection` — a prominent panel that is
PROVIDER-GATED: when `provider === "unconfigured"` it shows a "configure a
model provider (Portkey / Anthropic / OpenAI / OpenRouter)" prompt with the
enable action hidden; when "configured" it offers the enable control
reflecting `reasons.memory.enabled`. Honest applies-on-restart note + a
"Restart now" affordance (the toggle takes effect on next daemon restart, not
live). No optimistic flip — the badge always reflects persisted truth.
Consumes the honeycomb backend contracts (settings-driven memory.enabled,
provider-gated). security-worker-bee: zero findings (XSS-safe, no secret in UI,
action mirrors embeddings, schema fail-closed). quality-worker-bee: SHIP (all 6
plan points PASS). Gate: typecheck clean; 14 new tests pass (the 2 failing
funnel-telemetry tests are pre-existing + unrelated, identical on clean HEAD).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Memory Formation section to Settings that reads live status reasons, lets users persist memory enabled state, and offers a restart action when changes apply on next daemon restart. The wire client gains status and memory action endpoints, schemas, and request methods, with tests covering UI and API parsing. ChangesMemory Formation Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant MemoryFormationSection
participant WireClient
participant API as /api/status
participant ActionAPI as /api/actions/memory
User->>SettingsPage: open settings
SettingsPage->>MemoryFormationSection: render wire
MemoryFormationSection->>WireClient: status()
WireClient->>API: GET /api/status
API-->>WireClient: reasons.memory
WireClient-->>MemoryFormationSection: status probe
User->>MemoryFormationSection: toggle memory
MemoryFormationSection->>WireClient: setMemory(enabled)
WireClient->>ActionAPI: POST { enabled }
ActionAPI-->>WireClient: ack { ok, enabled }
WireClient-->>MemoryFormationSection: persisted result
User->>MemoryFormationSection: click Restart now
MemoryFormationSection->>WireClient: restartDaemon()
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/dashboard/web/pages/settings.tsx (2)
812-833: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"Restart now" fires immediately, unlike the confirm-gated restart elsewhere on this page.
SystemActionsSection(Line 890-897) requires a two-step confirm before calling the samewire.restartDaemon(). Here, a single click on "Restart now" restarts the daemon immediately, momentarily dropping all live dashboard connections. Consider a lightweight confirm (or at least a "click again to confirm" affordance) for consistency and to avoid an accidental click having real, if temporary, availability impact.💡 Illustrative fix (inline confirm, no new dependency)
- ) : ( - <Button - variant={pendingRestart ? "primary" : "secondary"} - size="sm" - onClick={() => void doRestart()} - data-testid="memory-restart-button" - > - Restart now - </Button> - )} + ) : restartConfirm ? ( + <div style={{ display: "flex", gap: 8 }}> + <Button variant="secondary" size="sm" onClick={() => setRestartConfirm(false)}>Cancel</Button> + <Button variant="primary" size="sm" onClick={() => void doRestart()} data-testid="memory-restart-confirm"> + Confirm restart + </Button> + </div> + ) : ( + <Button + variant={pendingRestart ? "primary" : "secondary"} + size="sm" + onClick={() => setRestartConfirm(true)} + data-testid="memory-restart-button" + > + Restart now + </Button> + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/web/pages/settings.tsx` around lines 812 - 833, The “Restart now” action in the settings page is firing `doRestart()` immediately, unlike the confirm-gated restart flow used in `SystemActionsSection`. Update the restart control in `settings.tsx` to add a lightweight confirmation step (for example, a second click to confirm or inline confirm state) before calling `wire.restartDaemon()`, and keep the existing `pendingRestart`/`restarting` behavior intact so the action is consistent and less prone to accidental daemon restarts.
692-741: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffLoad/toggle/badge boilerplate duplicates
EmbeddingsSection(Lines 573-616) almost verbatim.Both sections implement the same
null-while-loading +health()-derived state +busytoggle + fail-soft-catch pattern. A shared hook (e.g.useHealthAction<T>(wire, deriveState, action)) could remove this duplication as more toggle-style dashboard actions are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/web/pages/settings.tsx` around lines 692 - 741, The load/toggle/badge logic in MemoryFormationSection is duplicating the same pattern already used by EmbeddingsSection, so factor the shared state-management into a reusable hook or helper instead of keeping two near-identical implementations. Extract the common `null`-while-loading, `wire.health()` fail-soft read, busy toggle, and restart/pending state handling into something like `useHealthAction` that both sections can call, while keeping each section’s specific derive/update logic and badge labels separate. Use the unique symbols `MemoryFormationSection`, `EmbeddingsSection`, `load`, `toggle`, and `doRestart` to locate and consolidate the shared code.tests/dashboard/memory-formation-section.test.tsx (1)
16-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the
setMemoryfailure path.
mockWirealready supportsopts.setMemoryOk, but no test passes{ setMemoryOk: false }to verifypendingRestart/the restart note stay hidden and the toggle button returns to its pre-click label when the daemon rejects the write.✅ Suggested additional test
+ it("setMemory failure → does not surface the restart-to-apply affordance", async () => { + const { wire } = mockWire({ enabled: false, provider: "configured" }, { setMemoryOk: false }); + render(<MemoryFormationSection wire={wire} />); + + await waitFor(() => expect(screen.getByTestId("memory-configured")).toBeTruthy()); + fireEvent.click(screen.getByTestId("memory-toggle")); + + await waitFor(() => expect(screen.getByTestId("memory-toggle").textContent).toBe("Turn on")); + expect(screen.getByTestId("memory-restart-note").textContent).not.toContain("Restart now to apply"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dashboard/memory-formation-section.test.tsx` around lines 16 - 93, Add a test in MemoryFormationSection that uses mockWire with opts.setMemoryOk set to false and verifies the setMemory failure path. After clicking the memory-toggle, assert the toggle label returns to its original state and that memory-restart-note and memory-restart-button remain hidden, using MemoryFormationSection, mockWire, and the setMemory/restartDaemon fakes to locate the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/dashboard/web/pages/settings.tsx`:
- Around line 812-833: The “Restart now” action in the settings page is firing
`doRestart()` immediately, unlike the confirm-gated restart flow used in
`SystemActionsSection`. Update the restart control in `settings.tsx` to add a
lightweight confirmation step (for example, a second click to confirm or inline
confirm state) before calling `wire.restartDaemon()`, and keep the existing
`pendingRestart`/`restarting` behavior intact so the action is consistent and
less prone to accidental daemon restarts.
- Around line 692-741: The load/toggle/badge logic in MemoryFormationSection is
duplicating the same pattern already used by EmbeddingsSection, so factor the
shared state-management into a reusable hook or helper instead of keeping two
near-identical implementations. Extract the common `null`-while-loading,
`wire.health()` fail-soft read, busy toggle, and restart/pending state handling
into something like `useHealthAction` that both sections can call, while keeping
each section’s specific derive/update logic and badge labels separate. Use the
unique symbols `MemoryFormationSection`, `EmbeddingsSection`, `load`, `toggle`,
and `doRestart` to locate and consolidate the shared code.
In `@tests/dashboard/memory-formation-section.test.tsx`:
- Around line 16-93: Add a test in MemoryFormationSection that uses mockWire
with opts.setMemoryOk set to false and verifies the setMemory failure path.
After clicking the memory-toggle, assert the toggle label returns to its
original state and that memory-restart-note and memory-restart-button remain
hidden, using MemoryFormationSection, mockWire, and the setMemory/restartDaemon
fakes to locate the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2936a2ab-83dc-477e-b770-77e6314a94ca
📒 Files selected for processing (4)
src/dashboard/web/pages/settings.tsxsrc/dashboard/web/wire.tstests/dashboard/memory-formation-section.test.tsxtests/wire/memory-formation.test.ts
…/health
The Memory Formation + Embeddings controls read the daemon health `reasons` via
wire.health() → the portal's /health — but that is hive's OWN liveness ({status,
uptimeMs, version}) with no reasons. So both controls always fail-closed (memory
"provider needed", embeddings "off") regardless of the real daemon state. Found by
live end-to-end verification: memory showed "provider needed" while Portkey was
configured.
- wire.ts: add ENDPOINTS.status = "/api/status", StatusBodySchema (reuses
HealthReasonsSchema for `reasons`, ignores every other status field), and a
fail-closed wire.status() mirroring health() (non-2xx / non-JSON / malformed /
absent-reasons → { reasons: null }, never throws).
- settings.tsx: MemoryFormationSection + EmbeddingsSection now read reasons from
wire.status() (which honeycomb PR #248 populates on /api/status and the portal
reliably proxies) instead of the reasons-less /health. Fail-closed defaults
preserved; wire.health() left intact for the shell's coarse liveness pill.
Depends on honeycomb PR #248 (reasons on /api/status). security-worker-bee: zero
findings (fail-closed on every path; no secret; StatusBodySchema drops non-reasons
fields; action calls unchanged). quality-worker-bee: SHIP. Gate: typecheck clean; 23
touched-suite tests pass (the 2 funnel-telemetry failures are pre-existing/unrelated).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/dashboard/web/pages/settings.tsx (1)
723-744: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
doRestartduplicatesSystemActionsSectionbut skips its confirm step.
MemoryFormationSection.doRestart(Line 737-744) calls the identicalwire.restartDaemon()action asSystemActionsSection.doRestart(Line 865-872), but here it fires immediately on a single click, whereasSystemActionsSectiongates the same disruptive action behind a two-step "Confirm restart" (Line 896-900). This is an inconsistent safety posture for the same daemon-restart action on the same page, and the logic itself is duplicated rather than shared.Consider either reusing a shared restart hook/component (with the same confirm behavior), or confirming that the reduced-friction, no-confirm restart here is an intentional UX choice given the contextual "you just changed a setting" framing.
♻️ Sketch: extract a shared restart affordance
+function useRestartDaemon(wire: PageProps["wire"]) { + const [restarting, setRestarting] = React.useState(false); + const restart = React.useCallback(async (): Promise<boolean> => { + setRestarting(true); + const ok = await wire.restartDaemon(); + if (!ok) setRestarting(false); + return ok; + }, [wire]); + return { restarting, restart }; +}Both
MemoryFormationSectionandSystemActionsSectioncan then consume the same hook, keeping the confirm/no-confirm decision as an explicit, single point of truth.Also applies to: 817-838
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/web/pages/settings.tsx` around lines 723 - 744, `MemoryFormationSection.doRestart` duplicates the same `wire.restartDaemon()` flow used by `SystemActionsSection.doRestart` but bypasses the confirm step, creating inconsistent restart behavior on the same page. Update the restart handling so both `MemoryFormationSection` and `SystemActionsSection` share a single restart affordance or hook, and make the confirm/no-confirm behavior an explicit shared decision rather than separate duplicated logic. Use the existing `doRestart` methods and `wire.restartDaemon()` as the locating symbols when refactoring.tests/dashboard/memory-formation-section.test.tsx (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the
mockWire-style helper across settings tests.
embeddings-section.test.tsxand this file each define a near-identicalmockWirehelper (both stub a reasons-lesshealth()and a configurablestatus()). Extracting a small shared test utility would reduce duplication as more Settings sections adopt the samestatus()-based pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dashboard/memory-formation-section.test.tsx` around lines 1 - 45, The test file duplicates a near-identical `mockWire` helper already used by other settings section tests, so extract it into a shared test utility and reuse it from both `memory-formation-section.test.tsx` and `embeddings-section.test.tsx`. Move the common `health()`, `status()`, `setMemory`, and `restartDaemon` stubbing logic into a single helper with the same configurable inputs, then update the `MemoryFormationSection` tests to import and use that shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/dashboard/web/pages/settings.tsx`:
- Around line 723-744: `MemoryFormationSection.doRestart` duplicates the same
`wire.restartDaemon()` flow used by `SystemActionsSection.doRestart` but
bypasses the confirm step, creating inconsistent restart behavior on the same
page. Update the restart handling so both `MemoryFormationSection` and
`SystemActionsSection` share a single restart affordance or hook, and make the
confirm/no-confirm behavior an explicit shared decision rather than separate
duplicated logic. Use the existing `doRestart` methods and
`wire.restartDaemon()` as the locating symbols when refactoring.
In `@tests/dashboard/memory-formation-section.test.tsx`:
- Around line 1-45: The test file duplicates a near-identical `mockWire` helper
already used by other settings section tests, so extract it into a shared test
utility and reuse it from both `memory-formation-section.test.tsx` and
`embeddings-section.test.tsx`. Move the common `health()`, `status()`,
`setMemory`, and `restartDaemon` stubbing logic into a single helper with the
same configurable inputs, then update the `MemoryFormationSection` tests to
import and use that shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d34bfbb-82a7-4d24-9714-8ac4cce84739
📒 Files selected for processing (5)
src/dashboard/web/pages/settings.tsxsrc/dashboard/web/wire.tstests/dashboard/embeddings-section.test.tsxtests/dashboard/memory-formation-section.test.tsxtests/wire/memory-formation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/wire/memory-formation.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Makes memory formation a real, discoverable feature in the portal — the missing UI half of the honeycomb backend that turned memory formation into a settings-driven, provider-gated feature.
A prominent Memory Formation section on the Settings page, mirroring the existing embeddings control through the shared
wireclient +/healthreasons block (no bespoke data path):Backend contracts consumed (already built in honeycomb PR #248)
POST /api/actions/memory { enabled }→ persistsmemory.enabledGET /health → reasons.memory { enabled, provider: "configured"|"unconfigured" }Why
Memory formation — the product's core function — was gated behind env-only, default-off flags with no UI; a fresh install formed zero memories. Configuring a model provider is the real prerequisite, so this control only lights up once a provider exists.
Verification
Reviewed via security → quality: security-worker-bee zero findings (XSS-safe, no secret in UI/state, action mirrors the vetted embeddings call, schema fail-closes
providertounconfigured); quality-worker-bee SHIP (all 6 plan points PASS, stricter than the embeddings mirror — no optimistic desync). Gate:typecheckclean; 14 new tests pass. (The 2 failingfunnel-telemetrytests are pre-existing + unrelated — identical on clean HEAD.)🤖 Generated with Claude Code
Summary by CodeRabbit